From 03eb680a83e304cfb3818058a90527ab294174bd Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 18 Sep 2024 14:55:09 +0200 Subject: [PATCH 01/13] fix(browser): Try multiple options for `lazyLoadIntegration` script parent element lookup (#13717) Change the order for looking up parent elements to inject the integration script: 1. `document.body` 2. `document.head` 3. `document.currentScript.parentElement` --- packages/browser/src/utils/lazyLoadIntegration.ts | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/packages/browser/src/utils/lazyLoadIntegration.ts b/packages/browser/src/utils/lazyLoadIntegration.ts index 168d1fd1013b..82260ae9724f 100644 --- a/packages/browser/src/utils/lazyLoadIntegration.ts +++ b/packages/browser/src/utils/lazyLoadIntegration.ts @@ -68,7 +68,14 @@ export async function lazyLoadIntegration( script.addEventListener('error', reject); }); - WINDOW.document.body.appendChild(script); + const currentScript = WINDOW.document.currentScript; + const parent = WINDOW.document.body || WINDOW.document.head || (currentScript && currentScript.parentElement); + + if (parent) { + parent.appendChild(script); + } else { + throw new Error(`Could not find parent element to insert lazy-loaded ${name} script`); + } try { await waitForLoad; From 479aa1120dc9f2254296f71119e706b2a181563e Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 18 Sep 2024 16:35:01 +0200 Subject: [PATCH 02/13] ref: Update http instrumentation name for logging (#13716) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With this change, it is easier to figure out from logs if the correct or incorrect http instrumentation is added. Now, if you see e.g. this in the logs, if users have enabled logs (`debug: true` if not using `skipOpenTelemetrySetup: true`, else using native OTEL debug logs with e.g. `diag.setLogger(new DiagConsoleLogger(), DiagLogLevel.DEBUG)`): ```js @opentelemetry/instrumentation-http-sentry Applying instrumentation patch for nodejs core module on require hook { module: 'http' } @opentelemetry/instrumentation-http Applying instrumentation patch for nodejs core module on require hook { module: 'http' } ``` you can tell that that it has been double instrumenting this incorrectly. You should never see the `@opentelemetry/instrumentation-http` entry anymore, otherwise something is wrong there. This came out of https://github.com/getsentry/sentry-docs/pull/11378, I looked into various ways to debug this but there is not really an API provided by OTEL that allows us to figure this out 😬 --- packages/node/src/integrations/http.ts | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/packages/node/src/integrations/http.ts b/packages/node/src/integrations/http.ts index d9e5e671b702..126f22a06063 100644 --- a/packages/node/src/integrations/http.ts +++ b/packages/node/src/integrations/http.ts @@ -1,5 +1,6 @@ import type { ClientRequest, IncomingMessage, RequestOptions, ServerResponse } from 'node:http'; import type { Span } from '@opentelemetry/api'; +import { diag } from '@opentelemetry/api'; import { HttpInstrumentation } from '@opentelemetry/instrumentation-http'; import { addOpenTelemetryInstrumentation } from '@sentry/opentelemetry'; @@ -23,6 +24,8 @@ import { getRequestUrl } from '../utils/getRequestUrl'; const INTEGRATION_NAME = 'Http'; +const INSTRUMENTATION_NAME = '@opentelemetry_sentry-patched/instrumentation-http'; + interface HttpOptions { /** * Whether breadcrumbs should be recorded for requests. @@ -195,6 +198,17 @@ export const instrumentHttp = Object.assign( }, }); + // We want to update the logger namespace so we can better identify what is happening here + try { + _httpInstrumentation['_diag'] = diag.createComponentLogger({ + namespace: INSTRUMENTATION_NAME, + }); + + // @ts-expect-error This is marked as read-only, but we overwrite it anyhow + _httpInstrumentation.instrumentationName = INSTRUMENTATION_NAME; + } catch { + // ignore errors here... + } addOpenTelemetryInstrumentation(_httpInstrumentation); }, { From 14f0b6efeee5add22257a76e8d309891adbef839 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 18 Sep 2024 16:37:49 +0200 Subject: [PATCH 03/13] fix(opentelemetry): Always use active span in `Propagator.inject` (#13381) This PR removes the check for `hasTracingEnabled` in `getInjectionData` and simply always takes the non-recording span if there is one. Which is always the case in server applications when handling a request. For non-request-handling situations, we fall back anyway to the propagation context. --- .../tracing/meta-tags-twp-errors/no-server.js | 20 +++++++ .../tracing/meta-tags-twp-errors/server.js | 30 +++++++++++ .../tracing/meta-tags-twp-errors/test.ts | 52 +++++++++++++++++++ packages/opentelemetry/src/propagator.ts | 3 +- 4 files changed, 103 insertions(+), 2 deletions(-) create mode 100644 dev-packages/node-integration-tests/suites/tracing/meta-tags-twp-errors/no-server.js create mode 100644 dev-packages/node-integration-tests/suites/tracing/meta-tags-twp-errors/server.js create mode 100644 dev-packages/node-integration-tests/suites/tracing/meta-tags-twp-errors/test.ts diff --git a/dev-packages/node-integration-tests/suites/tracing/meta-tags-twp-errors/no-server.js b/dev-packages/node-integration-tests/suites/tracing/meta-tags-twp-errors/no-server.js new file mode 100644 index 000000000000..ac0122b48380 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/meta-tags-twp-errors/no-server.js @@ -0,0 +1,20 @@ +const { loggingTransport } = require('@sentry-internal/node-integration-tests'); +const Sentry = require('@sentry/node'); + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + release: '1.0', + transport: loggingTransport, + beforeSend(event) { + event.contexts = { + ...event.contexts, + traceData: { + ...Sentry.getTraceData(), + metaTags: Sentry.getTraceMetaTags(), + }, + }; + return event; + }, +}); + +Sentry.captureException(new Error('test error')); diff --git a/dev-packages/node-integration-tests/suites/tracing/meta-tags-twp-errors/server.js b/dev-packages/node-integration-tests/suites/tracing/meta-tags-twp-errors/server.js new file mode 100644 index 000000000000..19877ffe3613 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/meta-tags-twp-errors/server.js @@ -0,0 +1,30 @@ +const { loggingTransport, startExpressServerAndSendPortToRunner } = require('@sentry-internal/node-integration-tests'); +const Sentry = require('@sentry/node'); + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + transport: loggingTransport, + beforeSend(event) { + event.contexts = { + ...event.contexts, + traceData: { + ...Sentry.getTraceData(), + metaTags: Sentry.getTraceMetaTags(), + }, + }; + return event; + }, +}); + +// express must be required after Sentry is initialized +const express = require('express'); + +const app = express(); + +app.get('/test', () => { + throw new Error('test error'); +}); + +Sentry.setupExpressErrorHandler(app); + +startExpressServerAndSendPortToRunner(app); diff --git a/dev-packages/node-integration-tests/suites/tracing/meta-tags-twp-errors/test.ts b/dev-packages/node-integration-tests/suites/tracing/meta-tags-twp-errors/test.ts new file mode 100644 index 000000000000..e6c0bfff822d --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/meta-tags-twp-errors/test.ts @@ -0,0 +1,52 @@ +import { cleanupChildProcesses, createRunner } from '../../../utils/runner'; + +describe('errors in TwP mode have same trace in trace context and getTraceData()', () => { + afterAll(() => { + cleanupChildProcesses(); + }); + + test('in incoming request', async () => { + createRunner(__dirname, 'server.js') + .expect({ + event: event => { + const { contexts } = event; + const { trace_id, span_id } = contexts?.trace || {}; + expect(trace_id).toMatch(/^[a-f0-9]{32}$/); + expect(span_id).toMatch(/^[a-f0-9]{16}$/); + + const traceData = contexts?.traceData || {}; + + expect(traceData['sentry-trace']).toEqual(`${trace_id}-${span_id}`); + expect(traceData.baggage).toContain(`sentry-trace_id=${trace_id}`); + + expect(traceData.metaTags).toContain(``); + expect(traceData.metaTags).toContain(`sentr y-trace_id=${trace_id}`); + expect(traceData.metaTags).not.toContain('sentry-sampled='); + }, + }) + .start() + .makeRequest('get', '/test'); + }); + + test('outside of a request handler', done => { + createRunner(__dirname, 'no-server.js') + .expect({ + event: event => { + const { contexts } = event; + const { trace_id, span_id } = contexts?.trace || {}; + expect(trace_id).toMatch(/^[a-f0-9]{32}$/); + expect(span_id).toMatch(/^[a-f0-9]{16}$/); + + const traceData = contexts?.traceData || {}; + + expect(traceData['sentry-trace']).toEqual(`${trace_id}-${span_id}`); + expect(traceData.baggage).toContain(`sentry-trace_id=${trace_id}`); + + expect(traceData.metaTags).toContain(``); + expect(traceData.metaTags).toContain(`sentry-trace_id=${trace_id}`); + expect(traceData.metaTags).not.toContain('sentry-sampled='); + }, + }) + .start(done); + }); +}); diff --git a/packages/opentelemetry/src/propagator.ts b/packages/opentelemetry/src/propagator.ts index 387943cf9cf0..f4fcb1fa91e9 100644 --- a/packages/opentelemetry/src/propagator.ts +++ b/packages/opentelemetry/src/propagator.ts @@ -5,7 +5,6 @@ import { propagation, trace } from '@opentelemetry/api'; import { W3CBaggagePropagator, isTracingSuppressed } from '@opentelemetry/core'; import { ATTR_URL_FULL, SEMATTRS_HTTP_URL } from '@opentelemetry/semantic-conventions'; import type { continueTrace } from '@sentry/core'; -import { hasTracingEnabled } from '@sentry/core'; import { getRootSpan } from '@sentry/core'; import { spanToJSON } from '@sentry/core'; import { @@ -198,7 +197,7 @@ function getInjectionData(context: Context): { spanId: string | undefined; sampled: boolean | undefined; } { - const span = hasTracingEnabled() ? trace.getSpan(context) : undefined; + const span = trace.getSpan(context); const spanIsRemote = span?.spanContext().isRemote; // If we have a local span, we can just pick everything from it From 1e9a1a3c80b10d2aca19b640666af9134d3bf40a Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 18 Sep 2024 16:42:15 +0200 Subject: [PATCH 04/13] test(ci): Use latest node 22 version again for Node unit tests (#13720) Looks like the bug that made us pin to Node 22.6.0 in our Node unit tests was fixed and released in 22.8.0. This PR reverts the pin, meaning we test against the latest Node 22 version again. --- .github/workflows/build.yml | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index c1ff3d4b8e9c..9b36572032b1 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -458,8 +458,7 @@ jobs: strategy: fail-fast: false matrix: - # TODO(lforst): Unpin Node.js version 22 when https://github.com/protobufjs/protobuf.js/issues/2025 is resolved which broke the nodejs tests - node: [14, 16, 18, 20, '22.6.0'] + node: [14, 16, 18, 20, 22] steps: - name: Check out base commit (${{ github.event.pull_request.base.sha }}) uses: actions/checkout@v4 From fc7634ebafeeaa53e79086ad5944cab503fdecdb Mon Sep 17 00:00:00 2001 From: Julian Garcia Castillo Date: Thu, 19 Sep 2024 13:03:34 +0200 Subject: [PATCH 05/13] feat(gatsby): Add optional `deleteSourcemapsAfterUpload` (#13610) Related #13582 This work adds the `deleteSourcemapsAfterUpload` option to the Gatsby plugin, allowing it to be passed to the Webpack plugin to set the `sourceMapFilesToDeleteAfterUpload` without exposing the API. This simplifies our workflow by eliminating the need to apply a Yarn patch to modify the package whenever we want to use `filesToDeleteAfterUpload`. Before submitting a pull request, please take a look at our [Contributing](https://github.com/getsentry/sentry-javascript/blob/master/CONTRIBUTING.md) guidelines and verify: - [x] If you've added code that should be tested, please add tests. - [x] Ensure your code lints and the test suite passes (`yarn lint`) & (`yarn test`). --------- Co-authored-by: Luca Forstner --- packages/gatsby/README.md | 18 ++++++++++++++ packages/gatsby/gatsby-node.js | 3 +++ packages/gatsby/test/gatsby-node.test.ts | 31 ++++++++++++++++++++++++ 3 files changed, 52 insertions(+) diff --git a/packages/gatsby/README.md b/packages/gatsby/README.md index 5de12ed78410..cf5eadf7045b 100644 --- a/packages/gatsby/README.md +++ b/packages/gatsby/README.md @@ -65,6 +65,24 @@ module.exports = { }; ``` +Additionally, you can delete source map files after they have been uploaded by setting the `deleteSourcemapsAfterUpload` +option to be `true`. + +```javascript +module.exports = { + // ... + plugins: [ + { + resolve: '@sentry/gatsby', + options: { + deleteSourcemapsAfterUpload: true, + }, + }, + // ... + ], +}; +``` + ## Links - [Official SDK Docs](https://docs.sentry.io/quickstart/) diff --git a/packages/gatsby/gatsby-node.js b/packages/gatsby/gatsby-node.js index de88ee73adc0..911fcda7b437 100644 --- a/packages/gatsby/gatsby-node.js +++ b/packages/gatsby/gatsby-node.js @@ -7,12 +7,15 @@ const SENTRY_USER_CONFIG = ['./sentry.config.js', './sentry.config.ts']; exports.onCreateWebpackConfig = ({ getConfig, actions }, options) => { const enableClientWebpackPlugin = options.enableClientWebpackPlugin !== false; if (process.env.NODE_ENV === 'production' && enableClientWebpackPlugin) { + const deleteSourcemapsAfterUpload = options.deleteSourcemapsAfterUpload === true; actions.setWebpackConfig({ plugins: [ sentryWebpackPlugin({ sourcemaps: { // Only include files from the build output directory assets: ['./public/**'], + // Delete source files after uploading + filesToDeleteAfterUpload: deleteSourcemapsAfterUpload ? ['./public/**/*.map'] : undefined, // Ignore files that aren't users' source code related ignore: [ 'polyfill-*', // related to polyfills diff --git a/packages/gatsby/test/gatsby-node.test.ts b/packages/gatsby/test/gatsby-node.test.ts index 2e80ac03dcaa..006cb6f9e2c0 100644 --- a/packages/gatsby/test/gatsby-node.test.ts +++ b/packages/gatsby/test/gatsby-node.test.ts @@ -1,5 +1,12 @@ +import { sentryWebpackPlugin } from '@sentry/webpack-plugin'; import { onCreateWebpackConfig } from '../gatsby-node'; +jest.mock('@sentry/webpack-plugin', () => ({ + sentryWebpackPlugin: jest.fn().mockReturnValue({ + apply: jest.fn(), + }), +})); + describe('onCreateWebpackConfig', () => { let originalNodeEnv: string | undefined; @@ -12,6 +19,10 @@ describe('onCreateWebpackConfig', () => { process.env.NODE_ENV = originalNodeEnv; }); + afterEach(() => { + jest.clearAllMocks(); + }); + it('sets a webpack config', () => { const actions = { setWebpackConfig: jest.fn(), @@ -36,4 +47,24 @@ describe('onCreateWebpackConfig', () => { expect(actions.setWebpackConfig).toHaveBeenCalledTimes(0); }); + + it('sets sourceMapFilesToDeleteAfterUpload when provided in options', () => { + const actions = { + setWebpackConfig: jest.fn(), + }; + + const getConfig = jest.fn(); + + onCreateWebpackConfig({ actions, getConfig }, { deleteSourcemapsAfterUpload: true }); + + expect(actions.setWebpackConfig).toHaveBeenCalledTimes(1); + + expect(sentryWebpackPlugin).toHaveBeenCalledWith( + expect.objectContaining({ + sourcemaps: expect.objectContaining({ + filesToDeleteAfterUpload: ['./public/**/*.map'], + }), + }), + ); + }); }); From 32f5f00d9d3d4d8a85c3ee141b0803f9a687dbbb Mon Sep 17 00:00:00 2001 From: Sigrid Huemer <32902192+s1gr1d@users.noreply.github.com> Date: Thu, 19 Sep 2024 14:26:30 +0200 Subject: [PATCH 06/13] fix(nuxt): Use correct server output file path (#13725) Depending on the [nitro preset](https://nitro.unjs.io/deploy), the build output changes. By using the `serverDir` option, the directory can be retrieved dynamically. --- packages/nuxt/src/module.ts | 28 ++++++++++++----------- packages/nuxt/src/vite/addServerConfig.ts | 28 ++++++++++++++++------- 2 files changed, 35 insertions(+), 21 deletions(-) diff --git a/packages/nuxt/src/module.ts b/packages/nuxt/src/module.ts index f43f30d7e5ee..b379a7d7a206 100644 --- a/packages/nuxt/src/module.ts +++ b/packages/nuxt/src/module.ts @@ -64,21 +64,23 @@ export default defineNuxtModule({ setupSourceMaps(moduleOptions, nuxt); } - if (serverConfigFile && serverConfigFile.includes('.server.config')) { - addServerConfigToBuild(moduleOptions, nuxt, serverConfigFile); + nuxt.hooks.hook('nitro:init', nitro => { + if (serverConfigFile && serverConfigFile.includes('.server.config')) { + addServerConfigToBuild(moduleOptions, nuxt, nitro, serverConfigFile); - if (moduleOptions.experimental_basicServerTracing) { - addSentryTopImport(moduleOptions, nuxt); - } else { - if (moduleOptions.debug) { - consoleSandbox(() => { - // eslint-disable-next-line no-console - console.log( - `[Sentry] Using your \`${serverConfigFile}\` file for the server-side Sentry configuration. In case you have a \`public/instrument.server\` file, the \`public/instrument.server\` file will be ignored. Make sure the file path in your node \`--import\` option matches the Sentry server config file in your \`.output\` folder and has a \`.mjs\` extension.`, - ); - }); + if (moduleOptions.experimental_basicServerTracing) { + addSentryTopImport(moduleOptions, nitro); + } else { + if (moduleOptions.debug) { + consoleSandbox(() => { + // eslint-disable-next-line no-console + console.log( + `[Sentry] Using your \`${serverConfigFile}\` file for the server-side Sentry configuration. In case you have a \`public/instrument.server\` file, the \`public/instrument.server\` file will be ignored. Make sure the file path in your node \`--import\` option matches the Sentry server config file in your \`.output\` folder and has a \`.mjs\` extension.`, + ); + }); + } } } - } + }); }, }); diff --git a/packages/nuxt/src/vite/addServerConfig.ts b/packages/nuxt/src/vite/addServerConfig.ts index 60f2452cde5b..845228c58b0c 100644 --- a/packages/nuxt/src/vite/addServerConfig.ts +++ b/packages/nuxt/src/vite/addServerConfig.ts @@ -2,6 +2,7 @@ import * as fs from 'fs'; import { createResolver } from '@nuxt/kit'; import type { Nuxt } from '@nuxt/schema'; import { consoleSandbox } from '@sentry/utils'; +import type { Nitro } from 'nitropack'; import type { SentryNuxtModuleOptions } from '../common/types'; /** @@ -13,6 +14,7 @@ import type { SentryNuxtModuleOptions } from '../common/types'; export function addServerConfigToBuild( moduleOptions: SentryNuxtModuleOptions, nuxt: Nuxt, + nitro: Nitro, serverConfigFile: string, ): void { nuxt.hook('vite:extendConfig', async (viteInlineConfig, _env) => { @@ -29,10 +31,11 @@ export function addServerConfigToBuild( * When the build process is finished, copy the `sentry.server.config` file to the `.output` directory. * This is necessary because we need to reference this file path in the node --import option. */ - nuxt.hook('close', async () => { - const rootDirResolver = createResolver(nuxt.options.rootDir); - const source = rootDirResolver.resolve('.nuxt/dist/server/sentry.server.config.mjs'); - const destination = rootDirResolver.resolve('.output/server/sentry.server.config.mjs'); + nitro.hooks.hook('close', async () => { + const buildDirResolver = createResolver(nitro.options.buildDir); + const serverDirResolver = createResolver(nitro.options.output.serverDir); + const source = buildDirResolver.resolve('dist/server/sentry.server.config.mjs'); + const destination = serverDirResolver.resolve('sentry.server.config.mjs'); try { await fs.promises.access(source, fs.constants.F_OK); @@ -66,10 +69,19 @@ export function addServerConfigToBuild( * This is necessary for environments where modifying the node option `--import` is not possible. * However, only limited tracing instrumentation is supported when doing this. */ -export function addSentryTopImport(moduleOptions: SentryNuxtModuleOptions, nuxt: Nuxt): void { - nuxt.hook('close', async () => { - const rootDirResolver = createResolver(nuxt.options.rootDir); - const entryFilePath = rootDirResolver.resolve('.output/server/index.mjs'); +export function addSentryTopImport(moduleOptions: SentryNuxtModuleOptions, nitro: Nitro): void { + nitro.hooks.hook('close', () => { + // other presets ('node-server' or 'vercel') have an index.mjs + const presetsWithServerFile = ['netlify']; + const entryFileName = + typeof nitro.options.rollupConfig?.output.entryFileNames === 'string' + ? nitro.options.rollupConfig?.output.entryFileNames + : presetsWithServerFile.includes(nitro.options.preset) + ? 'server.mjs' + : 'index.mjs'; + + const serverDirResolver = createResolver(nitro.options.output.serverDir); + const entryFilePath = serverDirResolver.resolve(entryFileName); try { fs.readFile(entryFilePath, 'utf8', (err, data) => { From 37c4c42a83fa549bdd213e50cb8dd459ba05a522 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Thu, 19 Sep 2024 14:33:46 +0200 Subject: [PATCH 07/13] ref: Add external contributor to CHANGELOG.md (#13727) This PR adds the external contributor to the CHANGELOG.md file, so that they are credited for their contribution. See #13610 --------- Co-authored-by: lforst <8118419+lforst@users.noreply.github.com> Co-authored-by: Abhijeet Prasad --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 005f0204866b..936edff4c346 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,7 +10,7 @@ - "You miss 100 percent of the chances you don't take. — Wayne Gretzky" — Michael Scott -Work in this release was contributed by @KyGuy2002 and @artzhookov. Thank you for your contributions! +Work in this release was contributed by @KyGuy2002, @artzhookov, and @julianCast. Thank you for your contributions! ## 8.30.0 From 2ab7518b7f559bf2f21eb6277e8cb1c1097473d8 Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Thu, 19 Sep 2024 16:57:35 +0300 Subject: [PATCH 08/13] feat(node): Add `dataloader` integration (#13664) Adds integration for `dataloader` using [`@opentelemetry/instrumentation-dataloader`](https://www.npmjs.com/package/@opentelemetry/instrumentation-dataloader) on the background. A few notes: - We currently don't have access to the lookup / request as there is no hook from `@opentelemetry/instrumentation-dataloader`. So, we don't have `cache.hit`, `cache.key`, `cache.item_size` and so on, in this integration. I can try to implement those upstream, but if you have another way in mind to access those please let me know. - `@opentelemetry/instrumentation-dataloader` only records spans for `load`, `loadMany` and `batch`, which all are `cache.get` operations. There are also `prime`, `clear`, `clearAll`. We also can implement those upstream and update the integration in future. --- .../node-integration-tests/package.json | 1 + .../suites/tracing/dataloader/scenario.js | 33 +++++++++++ .../suites/tracing/dataloader/test.ts | 40 +++++++++++++ packages/astro/src/index.server.ts | 1 + packages/aws-serverless/src/index.ts | 1 + packages/bun/src/index.ts | 1 + packages/google-cloud-serverless/src/index.ts | 1 + packages/node/package.json | 1 + packages/node/src/index.ts | 1 + .../src/integrations/tracing/dataloader.ts | 57 +++++++++++++++++++ .../node/src/integrations/tracing/index.ts | 3 + yarn.lock | 12 ++++ 12 files changed, 152 insertions(+) create mode 100644 dev-packages/node-integration-tests/suites/tracing/dataloader/scenario.js create mode 100644 dev-packages/node-integration-tests/suites/tracing/dataloader/test.ts create mode 100644 packages/node/src/integrations/tracing/dataloader.ts diff --git a/dev-packages/node-integration-tests/package.json b/dev-packages/node-integration-tests/package.json index 8c5a7dfe1bc3..e954fb631c97 100644 --- a/dev-packages/node-integration-tests/package.json +++ b/dev-packages/node-integration-tests/package.json @@ -43,6 +43,7 @@ "connect": "^3.7.0", "cors": "^2.8.5", "cron": "^3.1.6", + "dataloader": "2.2.2", "express": "^4.17.3", "generic-pool": "^3.9.0", "graphql": "^16.3.0", diff --git a/dev-packages/node-integration-tests/suites/tracing/dataloader/scenario.js b/dev-packages/node-integration-tests/suites/tracing/dataloader/scenario.js new file mode 100644 index 000000000000..569d23276f0b --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/dataloader/scenario.js @@ -0,0 +1,33 @@ +const { loggingTransport, startExpressServerAndSendPortToRunner } = require('@sentry-internal/node-integration-tests'); +const Sentry = require('@sentry/node'); + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + release: '1.0', + tracesSampleRate: 1.0, + transport: loggingTransport, +}); + +const PORT = 8008; + +// Stop the process from exiting before the transaction is sent +setInterval(() => {}, 1000); + +const run = async () => { + const express = require('express'); + const Dataloader = require('dataloader'); + + const app = express(); + const dataloader = new Dataloader(async keys => keys.map((_, idx) => idx), { + cache: false, + }); + + app.get('/', (req, res) => { + const user = dataloader.load('user-1'); + res.send(user); + }); + + startExpressServerAndSendPortToRunner(app, PORT); +}; + +run(); diff --git a/dev-packages/node-integration-tests/suites/tracing/dataloader/test.ts b/dev-packages/node-integration-tests/suites/tracing/dataloader/test.ts new file mode 100644 index 000000000000..27a2511f1a6e --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/dataloader/test.ts @@ -0,0 +1,40 @@ +import { cleanupChildProcesses, createRunner } from '../../../utils/runner'; + +describe('dataloader auto-instrumentation', () => { + afterAll(async () => { + cleanupChildProcesses(); + }); + + const EXPECTED_TRANSACTION = { + transaction: 'GET /', + spans: expect.arrayContaining([ + expect.objectContaining({ + data: expect.objectContaining({ + 'sentry.origin': 'auto.db.otel.dataloader', + 'sentry.op': 'cache.get', + }), + description: 'dataloader.load', + origin: 'auto.db.otel.dataloader', + op: 'cache.get', + status: 'ok', + }), + expect.objectContaining({ + data: expect.objectContaining({ + 'sentry.origin': 'auto.db.otel.dataloader', + 'sentry.op': 'cache.get', + }), + description: 'dataloader.batch', + origin: 'auto.db.otel.dataloader', + op: 'cache.get', + status: 'ok', + }), + ]), + }; + + test('should auto-instrument `dataloader` package.', done => { + createRunner(__dirname, 'scenario.js') + .expect({ transaction: EXPECTED_TRANSACTION }) + .start(done) + .makeRequest('get', '/'); + }); +}); diff --git a/packages/astro/src/index.server.ts b/packages/astro/src/index.server.ts index 2645151a9ede..0239e2a55798 100644 --- a/packages/astro/src/index.server.ts +++ b/packages/astro/src/index.server.ts @@ -29,6 +29,7 @@ export { createGetModuleFromFilename, createTransport, cron, + dataloaderIntegration, debugIntegration, dedupeIntegration, DEFAULT_USER_INCLUDES, diff --git a/packages/aws-serverless/src/index.ts b/packages/aws-serverless/src/index.ts index 19c90e3aef3f..44414824cdc1 100644 --- a/packages/aws-serverless/src/index.ts +++ b/packages/aws-serverless/src/index.ts @@ -78,6 +78,7 @@ export { SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE, + dataloaderIntegration, expressIntegration, expressErrorHandler, setupExpressErrorHandler, diff --git a/packages/bun/src/index.ts b/packages/bun/src/index.ts index fcb3d1331f46..267adda6fac4 100644 --- a/packages/bun/src/index.ts +++ b/packages/bun/src/index.ts @@ -99,6 +99,7 @@ export { SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE, + dataloaderIntegration, expressIntegration, expressErrorHandler, setupExpressErrorHandler, diff --git a/packages/google-cloud-serverless/src/index.ts b/packages/google-cloud-serverless/src/index.ts index 14aa0996cb7c..33fdc6ea314f 100644 --- a/packages/google-cloud-serverless/src/index.ts +++ b/packages/google-cloud-serverless/src/index.ts @@ -79,6 +79,7 @@ export { SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE, + dataloaderIntegration, expressIntegration, expressErrorHandler, setupExpressErrorHandler, diff --git a/packages/node/package.json b/packages/node/package.json index dace1125f2e4..d80869cd9251 100644 --- a/packages/node/package.json +++ b/packages/node/package.json @@ -70,6 +70,7 @@ "@opentelemetry/core": "^1.25.1", "@opentelemetry/instrumentation": "^0.53.0", "@opentelemetry/instrumentation-connect": "0.39.0", + "@opentelemetry/instrumentation-dataloader": "0.12.0", "@opentelemetry/instrumentation-express": "0.42.0", "@opentelemetry/instrumentation-fastify": "0.39.0", "@opentelemetry/instrumentation-fs": "0.15.0", diff --git a/packages/node/src/index.ts b/packages/node/src/index.ts index d4cbcb9544a9..f3c945f5316d 100644 --- a/packages/node/src/index.ts +++ b/packages/node/src/index.ts @@ -28,6 +28,7 @@ export { koaIntegration, setupKoaErrorHandler } from './integrations/tracing/koa export { connectIntegration, setupConnectErrorHandler } from './integrations/tracing/connect'; export { spotlightIntegration } from './integrations/spotlight'; export { genericPoolIntegration } from './integrations/tracing/genericPool'; +export { dataloaderIntegration } from './integrations/tracing/dataloader'; export { SentryContextManager } from './otel/contextManager'; export { generateInstrumentOnce } from './otel/instrument'; diff --git a/packages/node/src/integrations/tracing/dataloader.ts b/packages/node/src/integrations/tracing/dataloader.ts new file mode 100644 index 000000000000..d4567ea0dfbe --- /dev/null +++ b/packages/node/src/integrations/tracing/dataloader.ts @@ -0,0 +1,57 @@ +import { DataloaderInstrumentation } from '@opentelemetry/instrumentation-dataloader'; +import { + SEMANTIC_ATTRIBUTE_SENTRY_OP, + SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, + defineIntegration, + spanToJSON, +} from '@sentry/core'; +import type { IntegrationFn } from '@sentry/types'; +import { generateInstrumentOnce } from '../../otel/instrument'; + +const INTEGRATION_NAME = 'Dataloader'; + +export const instrumentDataloader = generateInstrumentOnce( + INTEGRATION_NAME, + () => + new DataloaderInstrumentation({ + requireParentSpan: true, + }), +); + +const _dataloaderIntegration = (() => { + return { + name: INTEGRATION_NAME, + setupOnce() { + instrumentDataloader(); + }, + + setup(client) { + client.on('spanStart', span => { + const spanJSON = spanToJSON(span); + if (spanJSON.description?.startsWith('dataloader')) { + span.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, 'auto.db.otel.dataloader'); + } + + // These are all possible dataloader span descriptions + // Still checking for the future versions + // in case they add support for `clear` and `prime` + if ( + spanJSON.description === 'dataloader.load' || + spanJSON.description === 'dataloader.loadMany' || + spanJSON.description === 'dataloader.batch' + ) { + span.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_OP, 'cache.get'); + // TODO: We can try adding `key` to the `data` attribute upstream. + // Or alternatively, we can add `requestHook` to the dataloader instrumentation. + } + }); + }, + }; +}) satisfies IntegrationFn; + +/** + * Dataloader integration + * + * Capture tracing data for Dataloader. + */ +export const dataloaderIntegration = defineIntegration(_dataloaderIntegration); diff --git a/packages/node/src/integrations/tracing/index.ts b/packages/node/src/integrations/tracing/index.ts index 69ffc24a8be2..0248e3fbae21 100644 --- a/packages/node/src/integrations/tracing/index.ts +++ b/packages/node/src/integrations/tracing/index.ts @@ -2,6 +2,7 @@ import type { Integration } from '@sentry/types'; import { instrumentHttp } from '../http'; import { connectIntegration, instrumentConnect } from './connect'; +import { dataloaderIntegration, instrumentDataloader } from './dataloader'; import { expressIntegration, instrumentExpress } from './express'; import { fastifyIntegration, instrumentFastify } from './fastify'; import { genericPoolIntegration, instrumentGenericPool } from './genericPool'; @@ -41,6 +42,7 @@ export function getAutoPerformanceIntegrations(): Integration[] { connectIntegration(), genericPoolIntegration(), kafkaIntegration(), + dataloaderIntegration(), ]; } @@ -67,5 +69,6 @@ export function getOpenTelemetryInstrumentationToPreload(): (((options?: any) => instrumentGraphql, instrumentRedis, instrumentGenericPool, + instrumentDataloader, ]; } diff --git a/yarn.lock b/yarn.lock index 9f6846f29f95..c94576d2e979 100644 --- a/yarn.lock +++ b/yarn.lock @@ -7101,6 +7101,13 @@ "@opentelemetry/semantic-conventions" "^1.27.0" "@types/connect" "3.4.36" +"@opentelemetry/instrumentation-dataloader@0.12.0": + version "0.12.0" + resolved "https://registry.yarnpkg.com/@opentelemetry/instrumentation-dataloader/-/instrumentation-dataloader-0.12.0.tgz#de03a3948dec4f15fed80aa424d6bd5d6a8d10c7" + integrity sha512-pnPxatoFE0OXIZDQhL2okF//dmbiWFzcSc8pUg9TqofCLYZySSxDCgQc69CJBo5JnI3Gz1KP+mOjS4WAeRIH4g== + dependencies: + "@opentelemetry/instrumentation" "^0.53.0" + "@opentelemetry/instrumentation-express@0.42.0": version "0.42.0" resolved "https://registry.yarnpkg.com/@opentelemetry/instrumentation-express/-/instrumentation-express-0.42.0.tgz#279f195aa66baee2b98623a16666c6229c8e7564" @@ -15235,6 +15242,11 @@ data-urls@^4.0.0: whatwg-mimetype "^3.0.0" whatwg-url "^12.0.0" +dataloader@2.2.2: + version "2.2.2" + resolved "https://registry.yarnpkg.com/dataloader/-/dataloader-2.2.2.tgz#216dc509b5abe39d43a9b9d97e6e5e473dfbe3e0" + integrity sha512-8YnDaaf7N3k/q5HnTJVuzSyLETjoZjVmHc4AeKAzOvKHEFQKcn64OKBfzHYtE9zGjctNM7V9I0MfnUVLpi7M5g== + date-fns@^2.29.2: version "2.29.3" resolved "https://registry.yarnpkg.com/date-fns/-/date-fns-2.29.3.tgz#27402d2fc67eb442b511b70bbdf98e6411cd68a8" From 336a23664f1bfbc8f83d176b30d4e41649847356 Mon Sep 17 00:00:00 2001 From: Sigrid Huemer <32902192+s1gr1d@users.noreply.github.com> Date: Thu, 19 Sep 2024 17:11:14 +0200 Subject: [PATCH 09/13] feat(nuxt): Improve logs about adding Node option 'import' (#13726) Adding the node option can be a confusing step. This adds a log output which already includes the correct file path to add. It looks like this: ``` [Sentry] Using your sentry.server.config.ts file for the server-side Sentry configuration. Make sure to add the Node option import to the Node command where you deploy and/or run your application. This preloads the Sentry configuration at server startup. You can do this via a command-line flag (node --import ./.output/server/sentry.server.config.mjs [...]) or via an environment variable (NODE_OPTIONS='--import ./.output/server/sentry.server.config.mjs' node [...]). ``` --- packages/nuxt/src/module.ts | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/packages/nuxt/src/module.ts b/packages/nuxt/src/module.ts index b379a7d7a206..c74fe32b93fe 100644 --- a/packages/nuxt/src/module.ts +++ b/packages/nuxt/src/module.ts @@ -1,3 +1,4 @@ +import * as path from 'path'; import { addPlugin, addPluginTemplate, addServerPlugin, createResolver, defineNuxtModule } from '@nuxt/kit'; import { consoleSandbox } from '@sentry/utils'; import type { SentryNuxtModuleOptions } from './common/types'; @@ -72,10 +73,16 @@ export default defineNuxtModule({ addSentryTopImport(moduleOptions, nitro); } else { if (moduleOptions.debug) { + const serverDirResolver = createResolver(nitro.options.output.serverDir); + const serverConfigPath = serverDirResolver.resolve('sentry.server.config.mjs'); + + // For the default nitro node-preset build output this relative path would be: ./.output/server/sentry.server.config.mjs + const serverConfigRelativePath = `.${path.sep}${path.relative(nitro.options.rootDir, serverConfigPath)}`; + consoleSandbox(() => { // eslint-disable-next-line no-console console.log( - `[Sentry] Using your \`${serverConfigFile}\` file for the server-side Sentry configuration. In case you have a \`public/instrument.server\` file, the \`public/instrument.server\` file will be ignored. Make sure the file path in your node \`--import\` option matches the Sentry server config file in your \`.output\` folder and has a \`.mjs\` extension.`, + `[Sentry] Using your \`${serverConfigFile}\` file for the server-side Sentry configuration. Make sure to add the Node option \`import\` to the Node command where you deploy and/or run your application. This preloads the Sentry configuration at server startup. You can do this via a command-line flag (\`node --import ${serverConfigRelativePath} [...]\`) or via an environment variable (\`NODE_OPTIONS='--import ${serverConfigRelativePath}' node [...]\`).`, ); }); } From cf0152a9e25e1c2911e30c154c54d56fe0b79117 Mon Sep 17 00:00:00 2001 From: Billy Vong Date: Thu, 19 Sep 2024 12:30:30 -0400 Subject: [PATCH 10/13] feat(replay): Add `onError` callback + other small improvements to debugging (#13721) * Adds an `onError` callback for replay SDK exceptions * Do not log empty messages when calling `logger.exception` * Send `ratelimit_backoff` client report when necessary (instead of generic `send_error`) --- packages/replay-internal/src/replay.ts | 9 +- packages/replay-internal/src/types/replay.ts | 8 +- packages/replay-internal/src/util/logger.ts | 10 +-- .../replay-internal/src/util/sendReplay.ts | 9 +- .../test/integration/flush.test.ts | 4 +- .../test/unit/util/logger.test.ts | 88 +++++++++++++++++++ 6 files changed, 113 insertions(+), 15 deletions(-) create mode 100644 packages/replay-internal/test/unit/util/logger.test.ts diff --git a/packages/replay-internal/src/replay.ts b/packages/replay-internal/src/replay.ts index 06f81b6982c6..ea7df8b7afa7 100644 --- a/packages/replay-internal/src/replay.ts +++ b/packages/replay-internal/src/replay.ts @@ -54,6 +54,7 @@ import { getHandleRecordingEmit } from './util/handleRecordingEmit'; import { isExpired } from './util/isExpired'; import { isSessionExpired } from './util/isSessionExpired'; import { sendReplay } from './util/sendReplay'; +import { RateLimitError } from './util/sendReplayRequest'; import type { SKIPPED } from './util/throttle'; import { THROTTLED, throttle } from './util/throttle'; @@ -245,6 +246,9 @@ export class ReplayContainer implements ReplayContainerInterface { /** A wrapper to conditionally capture exceptions. */ public handleException(error: unknown): void { DEBUG_BUILD && logger.exception(error); + if (this._options.onError) { + this._options.onError(error); + } } /** @@ -1157,8 +1161,8 @@ export class ReplayContainer implements ReplayContainerInterface { segmentId, eventContext, session: this.session, - options: this.getOptions(), timestamp, + onError: err => this.handleException(err), }); } catch (err) { this.handleException(err); @@ -1173,7 +1177,8 @@ export class ReplayContainer implements ReplayContainerInterface { const client = getClient(); if (client) { - client.recordDroppedEvent('send_error', 'replay'); + const dropReason = err instanceof RateLimitError ? 'ratelimit_backoff' : 'send_error'; + client.recordDroppedEvent(dropReason, 'replay'); } } } diff --git a/packages/replay-internal/src/types/replay.ts b/packages/replay-internal/src/types/replay.ts index 0605ba97449a..6892c05ee179 100644 --- a/packages/replay-internal/src/types/replay.ts +++ b/packages/replay-internal/src/types/replay.ts @@ -26,7 +26,7 @@ export interface SendReplayData { eventContext: PopEventContext; timestamp: number; session: Session; - options: ReplayPluginOptions; + onError?: (err: unknown) => void; } export interface Timeouts { @@ -222,6 +222,12 @@ export interface ReplayPluginOptions extends ReplayNetworkOptions { */ beforeErrorSampling?: (event: ErrorEvent) => boolean; + /** + * Callback when an internal SDK error occurs. This can be used to debug SDK + * issues. + */ + onError?: (err: unknown) => void; + /** * _experiments allows users to enable experimental or internal features. * We don't consider such features as part of the public API and hence we don't guarantee semver for them. diff --git a/packages/replay-internal/src/util/logger.ts b/packages/replay-internal/src/util/logger.ts index 80445409164b..1b505f41703a 100644 --- a/packages/replay-internal/src/util/logger.ts +++ b/packages/replay-internal/src/util/logger.ts @@ -1,6 +1,6 @@ import { addBreadcrumb, captureException } from '@sentry/core'; import type { ConsoleLevel, SeverityLevel } from '@sentry/types'; -import { logger as coreLogger } from '@sentry/utils'; +import { logger as coreLogger, severityLevelFromString } from '@sentry/utils'; import { DEBUG_BUILD } from '../debug-build'; @@ -64,13 +64,13 @@ function makeReplayLogger(): ReplayLogger { _logger[name] = (...args: unknown[]) => { coreLogger[name](PREFIX, ...args); if (_trace) { - _addBreadcrumb(args[0]); + _addBreadcrumb(args.join(''), severityLevelFromString(name)); } }; }); _logger.exception = (error: unknown, ...message: unknown[]) => { - if (_logger.error) { + if (message.length && _logger.error) { _logger.error(...message); } @@ -79,9 +79,9 @@ function makeReplayLogger(): ReplayLogger { if (_capture) { captureException(error); } else if (_trace) { - // No need for a breadcrumb is `_capture` is enabled since it should be + // No need for a breadcrumb if `_capture` is enabled since it should be // captured as an exception - _addBreadcrumb(error); + _addBreadcrumb(error, 'error'); } }; diff --git a/packages/replay-internal/src/util/sendReplay.ts b/packages/replay-internal/src/util/sendReplay.ts index 973c3fb9a556..c0c6483502b9 100644 --- a/packages/replay-internal/src/util/sendReplay.ts +++ b/packages/replay-internal/src/util/sendReplay.ts @@ -1,8 +1,7 @@ import { setTimeout } from '@sentry-internal/browser-utils'; -import { captureException, setContext } from '@sentry/core'; +import { setContext } from '@sentry/core'; import { RETRY_BASE_INTERVAL, RETRY_MAX_COUNT, UNABLE_TO_SEND_REPLAY } from '../constants'; -import { DEBUG_BUILD } from '../debug-build'; import type { SendReplayData } from '../types'; import { RateLimitError, TransportStatusCodeError, sendReplayRequest } from './sendReplayRequest'; @@ -16,7 +15,7 @@ export async function sendReplay( interval: RETRY_BASE_INTERVAL, }, ): Promise { - const { recordingData, options } = replayData; + const { recordingData, onError } = replayData; // short circuit if there's no events to upload (this shouldn't happen as _runFlush makes this check) if (!recordingData.length) { @@ -36,8 +35,8 @@ export async function sendReplay( _retryCount: retryConfig.count, }); - if (DEBUG_BUILD && options._experiments && options._experiments.captureExceptions) { - captureException(err); + if (onError) { + onError(err); } // If an error happened here, it's likely that uploading the attachment diff --git a/packages/replay-internal/test/integration/flush.test.ts b/packages/replay-internal/test/integration/flush.test.ts index 52654fa909d3..72ef104d0633 100644 --- a/packages/replay-internal/test/integration/flush.test.ts +++ b/packages/replay-internal/test/integration/flush.test.ts @@ -188,8 +188,8 @@ describe('Integration | flush', () => { segmentId: 0, eventContext: expect.anything(), session: expect.any(Object), - options: expect.any(Object), timestamp: expect.any(Number), + onError: expect.any(Function), }); // Add this to test that segment ID increases @@ -238,7 +238,7 @@ describe('Integration | flush', () => { segmentId: 1, eventContext: expect.anything(), session: expect.any(Object), - options: expect.any(Object), + onError: expect.any(Function), timestamp: expect.any(Number), }); diff --git a/packages/replay-internal/test/unit/util/logger.test.ts b/packages/replay-internal/test/unit/util/logger.test.ts new file mode 100644 index 000000000000..075a6f27a841 --- /dev/null +++ b/packages/replay-internal/test/unit/util/logger.test.ts @@ -0,0 +1,88 @@ +import { beforeEach, describe, expect, it } from 'vitest'; + +import * as SentryCore from '@sentry/core'; +import { logger as coreLogger } from '@sentry/utils'; +import { logger } from '../../../src/util/logger'; + +const mockCaptureException = vi.spyOn(SentryCore, 'captureException'); +const mockAddBreadcrumb = vi.spyOn(SentryCore, 'addBreadcrumb'); +const mockLogError = vi.spyOn(coreLogger, 'error'); +vi.spyOn(coreLogger, 'info'); +vi.spyOn(coreLogger, 'log'); +vi.spyOn(coreLogger, 'warn'); + +describe('logger', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + describe.each([ + [false, false], + [false, true], + [true, false], + [true, true], + ])('with options: captureExceptions:%s, traceInternals:%s', (captureExceptions, traceInternals) => { + beforeEach(() => { + logger.setConfig({ + captureExceptions, + traceInternals, + }); + }); + + it.each([ + ['info', 'info', 'info message'], + ['log', 'log', 'log message'], + ['warn', 'warning', 'warn message'], + ['error', 'error', 'error message'], + ])('%s', (fn, level, message) => { + logger[fn](message); + expect(coreLogger[fn]).toHaveBeenCalledWith('[Replay] ', message); + + if (traceInternals) { + expect(mockAddBreadcrumb).toHaveBeenLastCalledWith( + { + category: 'console', + data: { logger: 'replay' }, + level, + message: `[Replay] ${message}`, + }, + { level }, + ); + } + }); + + it('logs exceptions with a message', () => { + const err = new Error('An error'); + logger.exception(err, 'a message'); + if (captureExceptions) { + expect(mockCaptureException).toHaveBeenCalledWith(err); + } + expect(mockLogError).toHaveBeenCalledWith('[Replay] ', 'a message'); + expect(mockLogError).toHaveBeenLastCalledWith('[Replay] ', err); + expect(mockLogError).toHaveBeenCalledTimes(2); + + if (traceInternals) { + expect(mockAddBreadcrumb).toHaveBeenCalledWith( + { + category: 'console', + data: { logger: 'replay' }, + level: 'error', + message: '[Replay] a message', + }, + { level: 'error' }, + ); + } + }); + + it('logs exceptions without a message', () => { + const err = new Error('An error'); + logger.exception(err); + if (captureExceptions) { + expect(mockCaptureException).toHaveBeenCalledWith(err); + expect(mockAddBreadcrumb).not.toHaveBeenCalled(); + } + expect(mockLogError).toHaveBeenCalledTimes(1); + expect(mockLogError).toHaveBeenLastCalledWith('[Replay] ', err); + }); + }); +}); From 97974baebd2a5ed8636c1bed5d35b3782b754209 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 20 Sep 2024 09:52:46 +0200 Subject: [PATCH 11/13] ref(nextjs): Improve app router routing instrumentation accuracy (#13695) Improves the Next.js routing instrumentation by patching the Next.js router and instrumenting window popstates. A few details on this PR that might explain weird-looking logic: - The patching of the router is in a setInterval because Next.js may take a while to write the router to the window object and we don't have a cue when that has happened. - We are using a combination of patching `router.back`/`router.forward` and the `popstate` event to emit a properly named transaction, because `router.back` and `router.forward` aren't passed any useful strings we could use as txn names. - Because there is a slight delay between the `router.back`/`router.forward` calls and the `popstate` event, we temporarily give the navigation span an invalid name that we use as an indicator to drop if one may leak through. --- .../navigation/[param]/browser-back/page.tsx | 7 + .../navigation/[param]/link-replace/page.tsx | 7 + .../app/navigation/[param]/link/page.tsx | 7 + .../navigation/[param]/router-back/page.tsx | 7 + .../navigation/[param]/router-push/page.tsx | 5 + .../[param]/router-replace/page.tsx | 5 + .../nextjs-app-dir/app/navigation/page.tsx | 57 ++++++ ...client-app-routing-instrumentation.test.ts | 145 +++++++++++++++ package.json | 2 +- packages/nextjs/package.json | 1 + packages/nextjs/src/client/index.ts | 8 + .../appRouterRoutingInstrumentation.ts | 151 +++++++++------ packages/nextjs/test/clientSdk.test.ts | 5 + .../appRouterInstrumentation.test.ts | 174 ------------------ 14 files changed, 352 insertions(+), 229 deletions(-) create mode 100644 dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/navigation/[param]/browser-back/page.tsx create mode 100644 dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/navigation/[param]/link-replace/page.tsx create mode 100644 dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/navigation/[param]/link/page.tsx create mode 100644 dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/navigation/[param]/router-back/page.tsx create mode 100644 dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/navigation/[param]/router-push/page.tsx create mode 100644 dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/navigation/[param]/router-replace/page.tsx create mode 100644 dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/navigation/page.tsx delete mode 100644 packages/nextjs/test/performance/appRouterInstrumentation.test.ts diff --git a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/navigation/[param]/browser-back/page.tsx b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/navigation/[param]/browser-back/page.tsx new file mode 100644 index 000000000000..9e32c27abce2 --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/navigation/[param]/browser-back/page.tsx @@ -0,0 +1,7 @@ +import Link from 'next/link'; + +export const dynamic = 'force-dynamic'; + +export default function Page() { + return Go back home; +} diff --git a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/navigation/[param]/link-replace/page.tsx b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/navigation/[param]/link-replace/page.tsx new file mode 100644 index 000000000000..9e32c27abce2 --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/navigation/[param]/link-replace/page.tsx @@ -0,0 +1,7 @@ +import Link from 'next/link'; + +export const dynamic = 'force-dynamic'; + +export default function Page() { + return Go back home; +} diff --git a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/navigation/[param]/link/page.tsx b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/navigation/[param]/link/page.tsx new file mode 100644 index 000000000000..9e32c27abce2 --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/navigation/[param]/link/page.tsx @@ -0,0 +1,7 @@ +import Link from 'next/link'; + +export const dynamic = 'force-dynamic'; + +export default function Page() { + return Go back home; +} diff --git a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/navigation/[param]/router-back/page.tsx b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/navigation/[param]/router-back/page.tsx new file mode 100644 index 000000000000..9e32c27abce2 --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/navigation/[param]/router-back/page.tsx @@ -0,0 +1,7 @@ +import Link from 'next/link'; + +export const dynamic = 'force-dynamic'; + +export default function Page() { + return Go back home; +} diff --git a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/navigation/[param]/router-push/page.tsx b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/navigation/[param]/router-push/page.tsx new file mode 100644 index 000000000000..de789f9af524 --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/navigation/[param]/router-push/page.tsx @@ -0,0 +1,5 @@ +export const dynamic = 'force-dynamic'; + +export default function Page() { + return

hello world

; +} diff --git a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/navigation/[param]/router-replace/page.tsx b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/navigation/[param]/router-replace/page.tsx new file mode 100644 index 000000000000..de789f9af524 --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/navigation/[param]/router-replace/page.tsx @@ -0,0 +1,5 @@ +export const dynamic = 'force-dynamic'; + +export default function Page() { + return

hello world

; +} diff --git a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/navigation/page.tsx b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/navigation/page.tsx new file mode 100644 index 000000000000..4f03a59d71cf --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/navigation/page.tsx @@ -0,0 +1,57 @@ +'use client'; + +import Link from 'next/link'; +import { useRouter } from 'next/navigation'; + +export default function Page() { + const router = useRouter(); + + return ( +
    +
  • + +
    • +
  • + +
    • +
  • + +
    • +
  • + +
    • +
  • + Normal Link +
    • +
  • + + Link Replace + +
    • +
+ ); +} diff --git a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/client-app-routing-instrumentation.test.ts b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/client-app-routing-instrumentation.test.ts index 9143bd0b2f90..35984640bcf6 100644 --- a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/client-app-routing-instrumentation.test.ts +++ b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/client-app-routing-instrumentation.test.ts @@ -53,3 +53,148 @@ test('Creates a navigation transaction for app router routes', async ({ page }) expect(await clientNavigationTransactionPromise).toBeDefined(); expect(await serverComponentTransactionPromise).toBeDefined(); }); + +test('Creates a navigation transaction for `router.push()`', async ({ page }) => { + const navigationTransactionPromise = waitForTransaction('nextjs-app-dir', transactionEvent => { + return ( + transactionEvent?.transaction === `/navigation/42/router-push` && + transactionEvent.contexts?.trace?.op === 'navigation' && + transactionEvent.contexts.trace.data?.['navigation.type'] === 'router.push' + ); + }); + + await page.goto('/navigation'); + await page.waitForTimeout(3000); + await page.getByText('router.push()').click(); + + expect(await navigationTransactionPromise).toBeDefined(); +}); + +test('Creates a navigation transaction for `router.replace()`', async ({ page }) => { + const navigationTransactionPromise = waitForTransaction('nextjs-app-dir', transactionEvent => { + return ( + transactionEvent?.transaction === `/navigation/42/router-replace` && + transactionEvent.contexts?.trace?.op === 'navigation' && + transactionEvent.contexts.trace.data?.['navigation.type'] === 'router.replace' + ); + }); + + await page.goto('/navigation'); + await page.waitForTimeout(3000); + await page.getByText('router.replace()').click(); + + expect(await navigationTransactionPromise).toBeDefined(); +}); + +test('Creates a navigation transaction for `router.back()`', async ({ page }) => { + const navigationTransactionPromise = waitForTransaction('nextjs-app-dir', transactionEvent => { + return ( + transactionEvent?.transaction === `/navigation/1337/router-back` && + transactionEvent.contexts?.trace?.op === 'navigation' + ); + }); + + await page.goto('/navigation/1337/router-back'); + await page.waitForTimeout(3000); + await page.getByText('Go back home').click(); + await page.waitForTimeout(3000); + await page.getByText('router.back()').click(); + + expect(await navigationTransactionPromise).toMatchObject({ + contexts: { + trace: { + data: { + 'navigation.type': 'router.back', + }, + }, + }, + }); +}); + +test('Creates a navigation transaction for `router.forward()`', async ({ page }) => { + const navigationTransactionPromise = waitForTransaction('nextjs-app-dir', transactionEvent => { + return ( + transactionEvent?.transaction === `/navigation/42/router-push` && + transactionEvent.contexts?.trace?.op === 'navigation' && + transactionEvent.contexts.trace.data?.['navigation.type'] === 'router.forward' + ); + }); + + await page.goto('/navigation'); + await page.waitForTimeout(3000); + await page.getByText('router.push()').click(); + await page.waitForTimeout(3000); + await page.goBack(); + await page.waitForTimeout(3000); + await page.getByText('router.forward()').click(); + + expect(await navigationTransactionPromise).toBeDefined(); +}); + +test('Creates a navigation transaction for ``', async ({ page }) => { + const navigationTransactionPromise = waitForTransaction('nextjs-app-dir', transactionEvent => { + return ( + transactionEvent?.transaction === `/navigation/42/link` && + transactionEvent.contexts?.trace?.op === 'navigation' && + transactionEvent.contexts.trace.data?.['navigation.type'] === 'router.push' + ); + }); + + await page.goto('/navigation'); + await page.getByText('Normal Link').click(); + + expect(await navigationTransactionPromise).toBeDefined(); +}); + +test('Creates a navigation transaction for ``', async ({ page }) => { + const navigationTransactionPromise = waitForTransaction('nextjs-app-dir', transactionEvent => { + return ( + transactionEvent?.transaction === `/navigation/42/link-replace` && + transactionEvent.contexts?.trace?.op === 'navigation' && + transactionEvent.contexts.trace.data?.['navigation.type'] === 'router.replace' + ); + }); + + await page.goto('/navigation'); + await page.waitForTimeout(3000); + await page.getByText('Link Replace').click(); + + expect(await navigationTransactionPromise).toBeDefined(); +}); + +test('Creates a navigation transaction for browser-back', async ({ page }) => { + const navigationTransactionPromise = waitForTransaction('nextjs-app-dir', transactionEvent => { + return ( + transactionEvent?.transaction === `/navigation/42/browser-back` && + transactionEvent.contexts?.trace?.op === 'navigation' && + transactionEvent.contexts.trace.data?.['navigation.type'] === 'browser.popstate' + ); + }); + + await page.goto('/navigation/42/browser-back'); + await page.waitForTimeout(3000); + await page.getByText('Go back home').click(); + await page.waitForTimeout(3000); + await page.goBack(); + + expect(await navigationTransactionPromise).toBeDefined(); +}); + +test('Creates a navigation transaction for browser-forward', async ({ page }) => { + const navigationTransactionPromise = waitForTransaction('nextjs-app-dir', transactionEvent => { + return ( + transactionEvent?.transaction === `/navigation/42/router-push` && + transactionEvent.contexts?.trace?.op === 'navigation' && + transactionEvent.contexts.trace.data?.['navigation.type'] === 'browser.popstate' + ); + }); + + await page.goto('/navigation'); + await page.getByText('router.push()').click(); + await page.waitForTimeout(3000); + await page.goBack(); + await page.waitForTimeout(3000); + await page.goForward(); + + expect(await navigationTransactionPromise).toBeDefined(); +}); diff --git a/package.json b/package.json index 4b9ad0383c02..365e1eb13922 100644 --- a/package.json +++ b/package.json @@ -17,7 +17,7 @@ "clean:build": "lerna run clean", "clean:caches": "yarn rimraf eslintcache .nxcache && yarn jest --clearCache", "clean:deps": "lerna clean --yes && rm -rf node_modules && yarn", - "clean:tarballs": "rimraf **/*.tgz", + "clean:tarballs": "rimraf -g **/*.tgz", "clean:all": "run-s clean:build clean:tarballs clean:caches clean:deps", "fix": "run-s fix:biome fix:prettier fix:lerna", "fix:lerna": "lerna run fix", diff --git a/packages/nextjs/package.json b/packages/nextjs/package.json index c401e82890dc..a9cd24e72bab 100644 --- a/packages/nextjs/package.json +++ b/packages/nextjs/package.json @@ -71,6 +71,7 @@ "@opentelemetry/instrumentation-http": "0.53.0", "@opentelemetry/semantic-conventions": "^1.27.0", "@rollup/plugin-commonjs": "26.0.1", + "@sentry-internal/browser-utils": "8.30.0", "@sentry/core": "8.30.0", "@sentry/node": "8.30.0", "@sentry/opentelemetry": "8.30.0", diff --git a/packages/nextjs/src/client/index.ts b/packages/nextjs/src/client/index.ts index a68734a10398..c66f50a293f2 100644 --- a/packages/nextjs/src/client/index.ts +++ b/packages/nextjs/src/client/index.ts @@ -8,6 +8,7 @@ import { devErrorSymbolicationEventProcessor } from '../common/devErrorSymbolica import { getVercelEnv } from '../common/getVercelEnv'; import { browserTracingIntegration } from './browserTracingIntegration'; import { nextjsClientStackFrameNormalizationIntegration } from './clientNormalizationIntegration'; +import { INCOMPLETE_APP_ROUTER_INSTRUMENTATION_TRANSACTION_NAME } from './routing/appRouterRoutingInstrumentation'; import { applyTunnelRouteOption } from './tunnelRoute'; export * from '@sentry/react'; @@ -39,6 +40,13 @@ export function init(options: BrowserOptions): Client | undefined { filterTransactions.id = 'NextClient404Filter'; addEventProcessor(filterTransactions); + const filterIncompleteNavigationTransactions: EventProcessor = event => + event.type === 'transaction' && event.transaction === INCOMPLETE_APP_ROUTER_INSTRUMENTATION_TRANSACTION_NAME + ? null + : event; + filterIncompleteNavigationTransactions.id = 'IncompleteTransactionFilter'; + addEventProcessor(filterIncompleteNavigationTransactions); + if (process.env.NODE_ENV === 'development') { addEventProcessor(devErrorSymbolicationEventProcessor); } diff --git a/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts b/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts index 25c1496d25b4..741849c481ab 100644 --- a/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts +++ b/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts @@ -4,8 +4,10 @@ import { SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, } from '@sentry/core'; import { WINDOW, startBrowserTracingNavigationSpan, startBrowserTracingPageLoadSpan } from '@sentry/react'; -import type { Client } from '@sentry/types'; -import { addFetchInstrumentationHandler, browserPerformanceTimeOrigin } from '@sentry/utils'; +import type { Client, Span } from '@sentry/types'; +import { GLOBAL_OBJ, browserPerformanceTimeOrigin } from '@sentry/utils'; + +export const INCOMPLETE_APP_ROUTER_INSTRUMENTATION_TRANSACTION_NAME = 'incomplete-app-router-transaction'; /** Instruments the Next.js app router for pageloads. */ export function appRouterInstrumentPageLoad(client: Client): void { @@ -21,70 +23,111 @@ export function appRouterInstrumentPageLoad(client: Client): void { }); } -/** Instruments the Next.js app router for navigation. */ -export function appRouterInstrumentNavigation(client: Client): void { - addFetchInstrumentationHandler(handlerData => { - // The instrumentation handler is invoked twice - once for starting a request and once when the req finishes - // We can use the existence of the end-timestamp to filter out "finishing"-events. - if (handlerData.endTimestamp !== undefined) { - return; - } - - // Only GET requests can be navigating RSC requests - if (handlerData.fetchData.method !== 'GET') { - return; - } +interface NextRouter { + back: () => void; + forward: () => void; + push: (target: string) => void; + replace: (target: string) => void; +} - const parsedNavigatingRscFetchArgs = parseNavigatingRscFetchArgs(handlerData.args); +// Yes, yes, I know we shouldn't depend on these internals. But that's where we are at. We write the ugly code, so you don't have to. +const GLOBAL_OBJ_WITH_NEXT_ROUTER = GLOBAL_OBJ as typeof GLOBAL_OBJ & { + // Available until 13.4.4-canary.3 - https://github.com/vercel/next.js/pull/50210 + nd?: { + router?: NextRouter; + }; + // Avalable from 13.4.4-canary.4 - https://github.com/vercel/next.js/pull/50210 + next?: { + router?: NextRouter; + }; +}; - if (parsedNavigatingRscFetchArgs === null) { - return; - } +/* + * The routing instrumentation needs to handle a few cases: + * - Router operations: + * - router.push() (either explicitly called or implicitly through tags) + * - router.replace() (either explicitly called or implicitly through tags) + * - router.back() + * - router.forward() + * - Browser operations: + * - native Browser-back / popstate event (implicitly called by router.back()) + * - native Browser-forward / popstate event (implicitly called by router.forward()) + */ - const newPathname = parsedNavigatingRscFetchArgs.targetPathname; +/** Instruments the Next.js app router for navigation. */ +export function appRouterInstrumentNavigation(client: Client): void { + let currentNavigationSpan: Span | undefined = undefined; - startBrowserTracingNavigationSpan(client, { - name: newPathname, - attributes: { - [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation', - [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.nextjs.app_router_instrumentation', - [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url', - }, - }); + WINDOW.addEventListener('popstate', () => { + if (currentNavigationSpan && currentNavigationSpan.isRecording()) { + currentNavigationSpan.updateName(WINDOW.location.pathname); + } else { + currentNavigationSpan = startBrowserTracingNavigationSpan(client, { + name: WINDOW.location.pathname, + attributes: { + [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation', + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.nextjs.app_router_instrumentation', + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url', + 'navigation.type': 'browser.popstate', + }, + }); + } }); -} -function parseNavigatingRscFetchArgs(fetchArgs: unknown[]): null | { - targetPathname: string; -} { - // Make sure the first arg is a URL object - if (!fetchArgs[0] || typeof fetchArgs[0] !== 'object' || (fetchArgs[0] as URL).searchParams === undefined) { - return null; - } + let routerPatched = false; + let triesToFindRouter = 0; + const MAX_TRIES_TO_FIND_ROUTER = 500; + const ROUTER_AVAILABILITY_CHECK_INTERVAL_MS = 20; + const checkForRouterAvailabilityInterval = setInterval(() => { + triesToFindRouter++; + const router = GLOBAL_OBJ_WITH_NEXT_ROUTER?.next?.router ?? GLOBAL_OBJ_WITH_NEXT_ROUTER?.nd?.router; - // Make sure the second argument is some kind of fetch config obj that contains headers - if (!fetchArgs[1] || typeof fetchArgs[1] !== 'object' || !('headers' in fetchArgs[1])) { - return null; - } + if (routerPatched || triesToFindRouter > MAX_TRIES_TO_FIND_ROUTER) { + clearInterval(checkForRouterAvailabilityInterval); + } else if (router) { + clearInterval(checkForRouterAvailabilityInterval); + routerPatched = true; + (['back', 'forward', 'push', 'replace'] as const).forEach(routerFunctionName => { + if (router?.[routerFunctionName]) { + // @ts-expect-error Weird type error related to not knowing how to associate return values with the individual functions - we can just ignore + router[routerFunctionName] = new Proxy(router[routerFunctionName], { + apply(target, thisArg, argArray) { + const span = startBrowserTracingNavigationSpan(client, { + name: INCOMPLETE_APP_ROUTER_INSTRUMENTATION_TRANSACTION_NAME, + attributes: { + [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation', + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.nextjs.app_router_instrumentation', + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url', + }, + }); - try { - const url = fetchArgs[0] as URL; - const headers = fetchArgs[1].headers as Record; + currentNavigationSpan = span; - // Not an RSC request - if (headers['RSC'] !== '1') { - return null; - } + if (routerFunctionName === 'push') { + span?.updateName(transactionNameifyRouterArgument(argArray[0])); + span?.setAttribute('navigation.type', 'router.push'); + } else if (routerFunctionName === 'replace') { + span?.updateName(transactionNameifyRouterArgument(argArray[0])); + span?.setAttribute('navigation.type', 'router.replace'); + } else if (routerFunctionName === 'back') { + span?.setAttribute('navigation.type', 'router.back'); + } else if (routerFunctionName === 'forward') { + span?.setAttribute('navigation.type', 'router.forward'); + } - // Prefetch requests are not navigating RSC requests - if (headers['Next-Router-Prefetch'] === '1') { - return null; + return target.apply(thisArg, argArray); + }, + }); + } + }); } + }, ROUTER_AVAILABILITY_CHECK_INTERVAL_MS); +} - return { - targetPathname: url.pathname, - }; +function transactionNameifyRouterArgument(target: string): string { + try { + return new URL(target, 'http://some-random-base.com/').pathname; } catch { - return null; + return '/'; } } diff --git a/packages/nextjs/test/clientSdk.test.ts b/packages/nextjs/test/clientSdk.test.ts index ac159564410b..f136b29e6887 100644 --- a/packages/nextjs/test/clientSdk.test.ts +++ b/packages/nextjs/test/clientSdk.test.ts @@ -16,13 +16,18 @@ const loggerLogSpy = jest.spyOn(logger, 'log'); const dom = new JSDOM(undefined, { url: 'https://example.com/' }); Object.defineProperty(global, 'document', { value: dom.window.document, writable: true }); Object.defineProperty(global, 'location', { value: dom.window.document.location, writable: true }); +Object.defineProperty(global, 'addEventListener', { value: () => undefined, writable: true }); const originalGlobalDocument = WINDOW.document; const originalGlobalLocation = WINDOW.location; +// eslint-disable-next-line @typescript-eslint/unbound-method +const originalGlobalAddEventListener = WINDOW.addEventListener; + afterAll(() => { // Clean up JSDom Object.defineProperty(WINDOW, 'document', { value: originalGlobalDocument }); Object.defineProperty(WINDOW, 'location', { value: originalGlobalLocation }); + Object.defineProperty(WINDOW, 'addEventListener', { value: originalGlobalAddEventListener }); }); function findIntegrationByName(integrations: Integration[] = [], name: string): Integration | undefined { diff --git a/packages/nextjs/test/performance/appRouterInstrumentation.test.ts b/packages/nextjs/test/performance/appRouterInstrumentation.test.ts deleted file mode 100644 index 16992a498f83..000000000000 --- a/packages/nextjs/test/performance/appRouterInstrumentation.test.ts +++ /dev/null @@ -1,174 +0,0 @@ -import { WINDOW } from '@sentry/react'; -import type { Client, HandlerDataFetch } from '@sentry/types'; -import * as sentryUtils from '@sentry/utils'; -import { JSDOM } from 'jsdom'; - -import { - appRouterInstrumentNavigation, - appRouterInstrumentPageLoad, -} from '../../src/client/routing/appRouterRoutingInstrumentation'; - -const addFetchInstrumentationHandlerSpy = jest.spyOn(sentryUtils, 'addFetchInstrumentationHandler'); - -function setUpPage(url: string) { - const dom = new JSDOM('

nothingness

', { url }); - - // The Next.js routing instrumentations requires a few things to be present on pageload: - // 1. Access to window.document API for `window.document.getElementById` - // 2. Access to window.location API for `window.location.pathname` - Object.defineProperty(WINDOW, 'document', { value: dom.window.document, writable: true }); - Object.defineProperty(WINDOW, 'location', { value: dom.window.document.location, writable: true }); -} - -describe('appRouterInstrumentPageLoad', () => { - const originalGlobalDocument = WINDOW.document; - const originalGlobalLocation = WINDOW.location; - - afterEach(() => { - // Clean up JSDom - Object.defineProperty(WINDOW, 'document', { value: originalGlobalDocument }); - Object.defineProperty(WINDOW, 'location', { value: originalGlobalLocation }); - }); - - it('should create a pageload transactions with the current location name', () => { - setUpPage('https://example.com/some/page?someParam=foobar'); - - const emit = jest.fn(); - const client = { - emit, - } as unknown as Client; - - appRouterInstrumentPageLoad(client); - - expect(emit).toHaveBeenCalledTimes(1); - expect(emit).toHaveBeenCalledWith( - 'startPageLoadSpan', - expect.objectContaining({ - name: '/some/page', - attributes: { - 'sentry.op': 'pageload', - 'sentry.origin': 'auto.pageload.nextjs.app_router_instrumentation', - 'sentry.source': 'url', - }, - }), - undefined, - ); - }); -}); - -describe('appRouterInstrumentNavigation', () => { - const originalGlobalDocument = WINDOW.document; - const originalGlobalLocation = WINDOW.location; - - afterEach(() => { - // Clean up JSDom - Object.defineProperty(WINDOW, 'document', { value: originalGlobalDocument }); - Object.defineProperty(WINDOW, 'location', { value: originalGlobalLocation }); - }); - - it('should create a navigation transactions when a navigation RSC request is sent', () => { - setUpPage('https://example.com/some/page?someParam=foobar'); - let fetchInstrumentationHandlerCallback: (arg: HandlerDataFetch) => void; - - addFetchInstrumentationHandlerSpy.mockImplementationOnce(callback => { - fetchInstrumentationHandlerCallback = callback; - }); - - const emit = jest.fn(); - const client = { - emit, - } as unknown as Client; - - appRouterInstrumentNavigation(client); - - fetchInstrumentationHandlerCallback!({ - args: [ - new URL('https://example.com/some/server/component/page?_rsc=2rs8t'), - { - headers: { - RSC: '1', - }, - }, - ], - fetchData: { method: 'GET', url: 'https://example.com/some/server/component/page?_rsc=2rs8t' }, - startTimestamp: 1337, - }); - - expect(emit).toHaveBeenCalledTimes(1); - expect(emit).toHaveBeenCalledWith('startNavigationSpan', { - name: '/some/server/component/page', - attributes: { - 'sentry.op': 'navigation', - 'sentry.origin': 'auto.navigation.nextjs.app_router_instrumentation', - 'sentry.source': 'url', - }, - }); - }); - - it.each([ - [ - 'no RSC header', - { - args: [ - new URL('https://example.com/some/server/component/page?_rsc=2rs8t'), - { - headers: {}, - }, - ], - fetchData: { method: 'GET', url: 'https://example.com/some/server/component/page?_rsc=2rs8t' }, - startTimestamp: 1337, - }, - ], - [ - 'no GET request', - { - args: [ - new URL('https://example.com/some/server/component/page?_rsc=2rs8t'), - { - headers: { - RSC: '1', - }, - }, - ], - fetchData: { method: 'POST', url: 'https://example.com/some/server/component/page?_rsc=2rs8t' }, - startTimestamp: 1337, - }, - ], - [ - 'prefetch request', - { - args: [ - new URL('https://example.com/some/server/component/page?_rsc=2rs8t'), - { - headers: { - RSC: '1', - 'Next-Router-Prefetch': '1', - }, - }, - ], - fetchData: { method: 'GET', url: 'https://example.com/some/server/component/page?_rsc=2rs8t' }, - startTimestamp: 1337, - }, - ], - ])( - 'should not create navigation transactions for fetch requests that are not navigating RSC requests (%s)', - (_, fetchCallbackData) => { - setUpPage('https://example.com/some/page?someParam=foobar'); - let fetchInstrumentationHandlerCallback: (arg: HandlerDataFetch) => void; - - addFetchInstrumentationHandlerSpy.mockImplementationOnce(callback => { - fetchInstrumentationHandlerCallback = callback; - }); - - const emit = jest.fn(); - const client = { - emit, - } as unknown as Client; - - appRouterInstrumentNavigation(client); - fetchInstrumentationHandlerCallback!(fetchCallbackData); - - expect(emit).toHaveBeenCalledTimes(0); - }, - ); -}); From dbfd8e2d235a11ee2b59eee13386a0fa85e16495 Mon Sep 17 00:00:00 2001 From: Sigrid Huemer <32902192+s1gr1d@users.noreply.github.com> Date: Fri, 20 Sep 2024 10:29:25 +0200 Subject: [PATCH 12/13] meta(changelog): Add PR link to changelog entry (#13735) Adds the PR link to the changelog for easier access. An entry would look like this: > - feat(node): Add `dataloader` integration ([#13664](https://github.com/getsentry/sentry-javascript/pull/13664)) > ``` > - feat(node): Add `dataloader` integration ([#13664](https://github.com/getsentry/sentry-javascript/pull/13664)) > ``` --- scripts/get-commit-list.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/scripts/get-commit-list.ts b/scripts/get-commit-list.ts index 3992694cf8f0..bceccfb317de 100644 --- a/scripts/get-commit-list.ts +++ b/scripts/get-commit-list.ts @@ -24,8 +24,11 @@ function run(): void { newCommits.sort((a, b) => a.localeCompare(b)); + const issueUrl = 'https://github.com/getsentry/sentry-javascript/pull/'; + const newCommitsWithLink = newCommits.map(commit => commit.replace(/#(\d+)/, `[#$1](${issueUrl}$1)`)); + // eslint-disable-next-line no-console - console.log(newCommits.join('\n')); + console.log(newCommitsWithLink.join('\n')); } run(); From 216aaeba1ee27cce8a4876e1f9212ba374eb30b3 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Fri, 20 Sep 2024 10:53:45 +0200 Subject: [PATCH 13/13] chore: Remove alpha note from cloudflare readme (#13728) Removes alpha note from cloudflare sdk readme. ref https://github.com/getsentry/sentry-javascript/issues/12620 --- packages/cloudflare/README.md | 3 --- 1 file changed, 3 deletions(-) diff --git a/packages/cloudflare/README.md b/packages/cloudflare/README.md index 398153563f1c..8fc88a578808 100644 --- a/packages/cloudflare/README.md +++ b/packages/cloudflare/README.md @@ -15,9 +15,6 @@ - [Official SDK Docs](https://docs.sentry.io/quickstart/) - [TypeDoc](http://getsentry.github.io/sentry-javascript/) -**Note: This SDK is in an alpha state. Please follow the -[tracking GH issue](https://github.com/getsentry/sentry-javascript/issues/12620) for updates.** - ## Install To get started, first install the `@sentry/cloudflare` package: