From 0d3087a786d0194ea587efd1e3c0ad8b182ba939 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Thu, 3 Sep 2026 09:12:56 +0200 Subject: [PATCH 01/15] feat(node): Deprecate `ignoreStatusCodes` in http integrations (#23973) This PR deprecates `ignoreStatusCodes` on - Node `httpIntegration` / `httpServerSpansIntegration` - Deno's `denoHttpIntegration` / `denoServeIntegration` both scheduled for removal in v12 without replacement. The option filters finished transaction events by response status code, which span streaming no longer supports. Child spans are sent as they end, potentially before the response status is known, so a request's spans can't be dropped retroactively. `ignoreIncomingRequests` is the nearest alternative for keeping requests out of Sentry, but it matches on the request rather than on the response, so the migration note is explicit that it isn't fully equivalent. Fixes #23956 --- MIGRATION.md | 23 ++++++++++++++++++- packages/deno/src/integrations/deno-serve.ts | 6 +++++ packages/deno/src/integrations/http.ts | 6 +++++ .../http/httpServerSpansIntegration.ts | 8 +++++++ 4 files changed, 42 insertions(+), 1 deletion(-) diff --git a/MIGRATION.md b/MIGRATION.md index 994557cbadc4..dadca8ed4cc2 100644 --- a/MIGRATION.md +++ b/MIGRATION.md @@ -481,6 +481,24 @@ Sentry.init({ `ignoreSpans` itself is unchanged in shape, but it now takes effect when a span **starts** rather than when the transaction is sent. Matched spans are never recorded at all, which means a matched non-segment span's children are re-parented to its parent instead of being dropped. +#### `ignoreStatusCodes` is deprecated + +The `ignoreStatusCodes` option is deprecated on `httpIntegration` and `httpServerSpansIntegration` (Node and the SDKs built on it) as well as on `denoHttpIntegration` and `denoServeIntegration`. It will be removed in v12, without a direct replacement. + +The filter runs on the finished transaction event, which is no longer supported span streaming. Child spans are sent as they end, before the response status code is known, so a request's spans can no longer be dropped once the status turns out to be uninteresting. The option therefore only has an effect with `traceLifecycle: 'static'`. + +To keep specific requests out of Sentry, decide before they are instrumented: Use `tracesSampler`, or ignore the request via `ignoreIncomingRequests`, which matches on the incoming request instead of on the response: + +```js +Sentry.init({ + integrations: [ + Sentry.httpIntegration({ + ignoreIncomingRequests: urlPath => urlPath.startsWith('/admin'), + }), + ], +}); +``` + #### Opting out of span streaming To keep the previous transaction-based model, set `traceLifecycle: 'static'`: @@ -656,7 +674,8 @@ Sentry.init({ This filter runs on transaction events (`processEvent`), so it only takes effect when `traceLifecycle` is `'static'`. The default `'stream'` lifecycle does not produce transaction events, and typical Deno apps are unaffected. Node's -`httpIntegration` has the same limitation. +`httpIntegration` has the same limitation. For that reason, [`ignoreStatusCodes` is deprecated](#ignorestatuscodes-is-deprecated) +and will be removed in v12. Transactions that are kept now also carry the HTTP status in the top-level `response` context, as in the other server SDKs. @@ -1424,6 +1443,8 @@ Sentry.httpIntegration({ }); ``` +Note that `ignoreStatusCodes` is itself [deprecated](#ignorestatuscodes-is-deprecated) and will be removed in v12. + ### `@sentry/cloudflare` - The `@sentry/cloudflare/nodejs_compat` subpath export was removed. Since `nodejs_compat` is now required for all users, the main `@sentry/cloudflare` entry point includes everything that was previously only available via the subpath. diff --git a/packages/deno/src/integrations/deno-serve.ts b/packages/deno/src/integrations/deno-serve.ts index 527bb1f062fe..156fb9fd8559 100644 --- a/packages/deno/src/integrations/deno-serve.ts +++ b/packages/deno/src/integrations/deno-serve.ts @@ -29,6 +29,11 @@ export type DenoServeIntegrationOptions = { * produce transaction events, so the filter does not run. * * @default `[[401, 404], [301, 303], [305, 399]]` + * + * @deprecated This option only has an effect if `traceLifecycle` is set to `'static'`. With span streaming + * (`traceLifecycle: 'stream'`, the default), the SDK ignores it: child spans are sent as they end, before the + * response status code is known, so a request's spans cannot be dropped retroactively. `ignoreStatusCodes` will be + * removed in v12 of the SDK. */ ignoreStatusCodes?: (number | [number, number])[]; }; @@ -88,6 +93,7 @@ const instrumentedDenoServe = (serve: typeof Deno.serve): typeof Deno.serve => }); const _denoServeIntegration = ((options: DenoServeIntegrationOptions = {}) => { + // oxlint-disable-next-line typescript/no-deprecated const ignoreStatusCodes = options.ignoreStatusCodes ?? DEFAULT_IGNORE_STATUS_CODES; return { diff --git a/packages/deno/src/integrations/http.ts b/packages/deno/src/integrations/http.ts index f696186b9261..f4cfeee81d17 100644 --- a/packages/deno/src/integrations/http.ts +++ b/packages/deno/src/integrations/http.ts @@ -107,6 +107,11 @@ export interface DenoHttpIntegrationOptions { * limitation. * * @default `[[401, 404], [301, 303], [305, 399]]` + * + * @deprecated This option only has an effect if `traceLifecycle` is set to `'static'`. With span streaming + * (`traceLifecycle: 'stream'`, the default), the SDK ignores it: child spans are sent as they end, before the + * response status code is known, so a request's spans cannot be dropped retroactively. `ignoreStatusCodes` will be + * removed in v12 of the SDK. */ ignoreStatusCodes?: (number | [number, number])[]; @@ -146,6 +151,7 @@ export interface DenoHttpIntegrationOptions { const _denoHttpIntegration = ((options: DenoHttpIntegrationOptions = {}) => { const breadcrumbs = options.breadcrumbs ?? true; const tracePropagation = options.tracePropagation ?? true; + // oxlint-disable-next-line typescript/no-deprecated const ignoreStatusCodes = options.ignoreStatusCodes ?? DEFAULT_IGNORE_STATUS_CODES; return { diff --git a/packages/node/src/integrations/http/httpServerSpansIntegration.ts b/packages/node/src/integrations/http/httpServerSpansIntegration.ts index cb9de82677b0..a00d048591a0 100644 --- a/packages/node/src/integrations/http/httpServerSpansIntegration.ts +++ b/packages/node/src/integrations/http/httpServerSpansIntegration.ts @@ -84,7 +84,14 @@ export interface HttpServerSpansIntegrationOptions { * By default, spans with some 3xx and 4xx status codes are ignored (see @default). * Expects an array of status codes or a range of status codes, e.g. [[300,399], 404] would ignore 3xx and 404 status codes. * + * Important: This option is ignored by default! It only has an effect if `traceLifecycle` is set to `'static'`. + * * @default `[[401, 404], [301, 303], [305, 399]]` + * + * @deprecated This option only has an effect if `traceLifecycle` is set to `'static'`. With span streaming + * (`traceLifecycle: 'stream'`, the default), the SDK ignores it: child spans are sent as they end, before the + * response status code is known, so a request's spans cannot be dropped retroactively. `ignoreStatusCodes` will be + * removed in v12 of the SDK, without replacement. */ ignoreStatusCodes?: (number | [number, number])[]; @@ -98,6 +105,7 @@ export interface HttpServerSpansIntegrationOptions { const _httpServerSpansIntegration = ((options: HttpServerSpansIntegrationOptions = {}) => { const ignoreStaticAssets = options.ignoreStaticAssets ?? true; const ignoreIncomingRequests = options.ignoreIncomingRequests; + // oxlint-disable-next-line typescript/no-deprecated const ignoreStatusCodes = options.ignoreStatusCodes ?? DEFAULT_IGNORE_STATUS_CODES; const { onSpanCreated } = options; From e168622947a26c8d2b112e77898f005d975506ca Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Thu, 3 Sep 2026 08:13:28 +0000 Subject: [PATCH 02/15] feat(deps): Bump fast-uri from 3.1.5 to 3.1.7 (#23980) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bumps [fast-uri](https://github.com/fastify/fast-uri) from 3.1.5 to 3.1.7.
Release notes

Sourced from fast-uri's releases.

v3.1.7

⚠️ Security Warning

This is a security release that fixes the following high-severity security advisories:

Users of the v3.x release line should upgrade to v3.1.7.

Full Changelog: https://github.com/fastify/fast-uri/compare/v3.1.6...v3.1.7

v3.1.6

⚠️ Security Warning

This release addresses the following high-severity security advisories:

Users of the v3.x release line should upgrade to v3.1.6.

Full Changelog: https://github.com/fastify/fast-uri/compare/v3.1.5...v3.1.6

Commits

[![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=fast-uri&package-manager=npm_and_yarn&previous-version=3.1.5&new-version=3.1.7)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) ---
Dependabot commands and options
You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot show ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) You can disable automated security fix PRs for this repo from the [Security Alerts page](https://github.com/getsentry/sentry-javascript/network/alerts).
Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- yarn.lock | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/yarn.lock b/yarn.lock index 27491c31a197..48870ca9a512 100644 --- a/yarn.lock +++ b/yarn.lock @@ -15726,9 +15726,9 @@ fast-text-encoding@^1.0.0: integrity sha512-dtm4QZH9nZtcDt8qJiOH9fcQd1NAgi+K1O2DbE6GG1PPCK/BWfOH3idCTRQ4ImXRUOyopDEgDEnVEE7Y/2Wrig== fast-uri@^3.0.0, fast-uri@^3.0.1: - version "3.1.5" - resolved "https://registry.yarnpkg.com/fast-uri/-/fast-uri-3.1.5.tgz#610f37419a030270430cecd68d74e3d4d96725d0" - integrity sha512-gHwA1O9LDIcKunMKhObS/HimwtehO1nPUECKAu5TpKgaO19fcWEl4bliWe1jWxVFvIXztJjjQ4L8XQ1EU9f7Jw== + version "3.1.7" + resolved "https://registry.yarnpkg.com/fast-uri/-/fast-uri-3.1.7.tgz#743157d957f3cbb4c65310e033dc2ad4ad7dc60a" + integrity sha512-dOvZVzjdZdz7phd9v6jCbwxrBW3fK6n8Rc0CtdmM4bumzMnxywBYhuph6J819RRw/ku+rLbelwfMunktuzVVHg== fast-wrap-ansi@^0.2.0: version "0.2.2" From 13f2632f485b1986ded54fad0f58e6add8a1c409 Mon Sep 17 00:00:00 2001 From: Sigrid <32902192+s1gr1d@users.noreply.github.com> Date: Thu, 3 Sep 2026 10:41:44 +0200 Subject: [PATCH 03/15] test(e2e): Migrate nuxt-4-cloudflare to span streaming (#23950) Reference https://github.com/getsentry/sentry-javascript/issues/23804 Co-authored-by: Claude Fable 5 --- .../server/plugins/sentry.ts | 1 - .../nuxt-4-cloudflare/tests/db.test.ts | 68 +++++++++++-------- 2 files changed, 41 insertions(+), 28 deletions(-) diff --git a/dev-packages/e2e-tests/test-applications/nuxt-4-cloudflare/server/plugins/sentry.ts b/dev-packages/e2e-tests/test-applications/nuxt-4-cloudflare/server/plugins/sentry.ts index f1725d6bf9e4..faa56e1db7ef 100644 --- a/dev-packages/e2e-tests/test-applications/nuxt-4-cloudflare/server/plugins/sentry.ts +++ b/dev-packages/e2e-tests/test-applications/nuxt-4-cloudflare/server/plugins/sentry.ts @@ -3,7 +3,6 @@ import { sentryCloudflareNitroPlugin } from '@sentry/nuxt/module/plugins'; export default defineNitroPlugin( sentryCloudflareNitroPlugin({ - traceLifecycle: 'static', dsn: 'https://public@dsn.ingest.sentry.io/1337', tracesSampleRate: 1.0, tunnel: 'http://localhost:3031/', // proxy server diff --git a/dev-packages/e2e-tests/test-applications/nuxt-4-cloudflare/tests/db.test.ts b/dev-packages/e2e-tests/test-applications/nuxt-4-cloudflare/tests/db.test.ts index d2f846fd29cf..af9f1e7259d4 100644 --- a/dev-packages/e2e-tests/test-applications/nuxt-4-cloudflare/tests/db.test.ts +++ b/dev-packages/e2e-tests/test-applications/nuxt-4-cloudflare/tests/db.test.ts @@ -1,43 +1,57 @@ import { expect, test } from '@playwright/test'; -import { waitForTransaction } from '@sentry-internal/test-utils'; +import { collectStreamedSpans, getSpanOp } from '@sentry-internal/test-utils'; + +// Streamed child spans arrive across several envelopes, so collect until the request's segment +// span and the expected db spans have all been flushed. The streamed segment name is method-only +// (`GET`), so the segment is selected via `url.path`. +function collectDbSpans(minDbSpans: number) { + return collectStreamedSpans( + 'nuxt-4-cloudflare', + spans => + spans.some(span => span.is_segment && span.attributes['url.path']?.value === '/api/db-mysql') && + spans.filter(span => getSpanOp(span) === 'db').length >= minDbSpans, + ); +} test('a real mysql query emits a db span with orchestrion-channel attributes', async ({ request }) => { - const transactionPromise = waitForTransaction('nuxt-4-cloudflare', transactionEvent => { - return ( - transactionEvent.contexts?.trace?.op === 'http.server' && - (transactionEvent.spans?.some(span => span.op === 'db') ?? false) - ); - }); + const spansPromise = collectDbSpans(1); const res = await request.get('/api/db-mysql'); expect(res.status()).toBe(200); - const transactionEvent = await transactionPromise; - const dbSpans = transactionEvent.spans!.filter(span => span.op === 'db'); + const spans = await spansPromise; + const rootSpan = spans.find(span => span.is_segment && span.attributes['url.path']?.value === '/api/db-mysql'); + expect(rootSpan).toBeDefined(); + expect(getSpanOp(rootSpan!)).toBe('http.server'); + + const dbSpans = spans.filter(span => getSpanOp(span) === 'db' && span.trace_id === rootSpan!.trace_id); - const firstQuery = dbSpans.find(span => span.description === 'SELECT 1 + 1 AS solution'); + const firstQuery = dbSpans.find(span => span.attributes['db.query.text']?.value === 'SELECT 1 + 1 AS solution'); expect(firstQuery).toBeDefined(); - expect(firstQuery!.data?.['sentry.origin']).toBe('auto.db.mysql'); - expect(firstQuery!.data?.['db.system.name']).toBe('mysql'); - expect(firstQuery!.data?.['db.query.text']).toBe('SELECT 1 + 1 AS solution'); - expect(firstQuery!.data?.['server.address']).toBe('127.0.0.1'); - expect(firstQuery!.data?.['server.port']).toBe(3306); - expect(firstQuery!.data?.['db.user']).toBe('root'); + expect(firstQuery!.name).toBe('SELECT'); + expect(firstQuery!.attributes).toMatchObject({ + 'sentry.origin': { type: 'string', value: 'auto.db.mysql' }, + 'db.system.name': { type: 'string', value: 'mysql' }, + 'db.query.text': { type: 'string', value: 'SELECT 1 + 1 AS solution' }, + 'server.address': { type: 'string', value: '127.0.0.1' }, + 'server.port': { type: 'integer', value: 3306 }, + 'db.user': { type: 'string', value: 'root' }, + }); }); -test('a nested query lands on the same transaction (async context restored)', async ({ request }) => { - const transactionPromise = waitForTransaction('nuxt-4-cloudflare', transactionEvent => { - return ( - transactionEvent.contexts?.trace?.op === 'http.server' && - (transactionEvent.spans?.filter(span => span.op === 'db').length ?? 0) >= 2 - ); - }); +test('a nested query lands on the same trace (async context restored)', async ({ request }) => { + const spansPromise = collectDbSpans(2); const res = await request.get('/api/db-mysql'); expect(res.status()).toBe(200); - const transactionEvent = await transactionPromise; - const descriptions = transactionEvent.spans!.filter(span => span.op === 'db').map(span => span.description); - expect(descriptions).toContain('SELECT 1 + 1 AS solution'); - expect(descriptions).toContain('SELECT NOW()'); + const spans = await spansPromise; + const rootSpan = spans.find(span => span.is_segment && span.attributes['url.path']?.value === '/api/db-mysql'); + expect(rootSpan).toBeDefined(); + + const queryTexts = spans + .filter(span => getSpanOp(span) === 'db' && span.trace_id === rootSpan!.trace_id) + .map(span => span.attributes['db.query.text']?.value); + expect(queryTexts).toContain('SELECT 1 + 1 AS solution'); + expect(queryTexts).toContain('SELECT NOW()'); }); From 29b67d3c219c6fbb47cadc6676ee2768ca655484 Mon Sep 17 00:00:00 2001 From: Sigrid <32902192+s1gr1d@users.noreply.github.com> Date: Thu, 3 Sep 2026 10:45:19 +0200 Subject: [PATCH 04/15] feat(server-utils)!: Start Redis spans as `cache` spans (#23933) With span streaming, `ignoreSpans` runs at span start. Redis cache spans used to start as `db.query` and get renamed to `cache.*` at response time, so a filter matching what you see in the UI never matched anything (part of https://github.com/getsentry/sentry-javascript/pull/23830). Now a command whose key matches `cachePrefixes` starts as a `cache.*` span on all four instrumentation paths. At response time we only add `cache.hit`/`cache.item_size` (as we only know it at this time). Static-lifecycle output is unchanged. Behavior changes and limitations: - Failed cache commands now report as `cache.*` spans. They previously stayed `db.query`. - `maxCacheKeyLength` only applies to `traceLifecycle: 'static'` (added to JSDoc). Streamed span names don't need truncation. - Multi-key commands on node-redis >=5.12 still stay `db.query`: the library sanitizes keys to `?` before we see them (pre-existing). Closes https://github.com/getsentry/sentry-javascript/issues/23832 --------- Co-authored-by: Claude Fable 5 --- MIGRATION.md | 2 + .../suites/tracing/redis-dc/instrument.mjs | 3 + .../redis-dc/scenario-redis-5-tracing.mjs | 4 + .../suites/tracing/redis-dc/test.ts | 64 ++++++- .../src/integrations/redis/index.ts | 27 +-- .../redis/ioredis-channel-subscriber.ts | 39 +++-- .../src/integrations/redis/redis-cache.ts | 99 ++++++----- .../integrations/redis/redis-dc-subscriber.ts | 29 ++-- .../redis/ioredis-channel-subscriber.test.ts | 39 +++-- .../integrations/redis/redis-cache.test.ts | 163 ++++++++++++------ 10 files changed, 313 insertions(+), 156 deletions(-) diff --git a/MIGRATION.md b/MIGRATION.md index dadca8ed4cc2..567e076eeb9a 100644 --- a/MIGRATION.md +++ b/MIGRATION.md @@ -999,6 +999,8 @@ Messaging span names now read ` ` in every integrat Cache keys are unbounded, so they are no longer part of a cache span name. They remain available on the `cache.key` attribute, and every cache span now also carries a `cache.operation` attribute (`get`, `put`, `remove`) — the value the name is built from. That attribute is set in both trace lifecycles. This affects the redis/ioredis cache spans (`cachePrefixes`), the Nuxt and Nitro storage spans, and the dataloader spans. +A Redis command whose key matches `cachePrefixes` now starts as a `cache.*` span instead of being converted from a `db.query` span at response time. `ignoreSpans` is evaluated at span start, so filters can match these spans by their cache op and name. A failed cache command reports as a cache span too, where it previously stayed a `db.query` span. + A dataloader span no longer carries the loader's `name` either (`dataloader.load usersLoader` becomes `cache.get`), because the cache conventions have no slot for it in the name. It is reported on the `db.collection.name` attribute instead — a loader batches one entity type, so it is the closest thing dataloader has to a collection — and that attribute is set in both trace lifecycles. Unnamed loaders do not set it. Redis has no SQL statement to summarize and no collection to pair a command with, so redis and ioredis `db.query` spans are named after the operation and the connection instead of the command that was sent. The command and its arguments remain available on `db.query.text`, redacted as before. `MULTI`/`PIPELINE` batch spans are unchanged — they were already named after their operation, which they now also report on `db.operation.name`. `db.namespace` is deliberately not used in the name: for redis it is the numeric database index, which says nothing about what the command did. diff --git a/dev-packages/node-integration-tests/suites/tracing/redis-dc/instrument.mjs b/dev-packages/node-integration-tests/suites/tracing/redis-dc/instrument.mjs index c0a1998369a5..d4f8cc7801fc 100644 --- a/dev-packages/node-integration-tests/suites/tracing/redis-dc/instrument.mjs +++ b/dev-packages/node-integration-tests/suites/tracing/redis-dc/instrument.mjs @@ -8,4 +8,7 @@ Sentry.init({ tracesSampleRate: 1.0, transport: loggingTransport, integrations: [Sentry.redisIntegration({ cachePrefixes: ['dc-cache:'] })], + ignoreSpans: process.env.IGNORE_CACHE_GET === 'true' ? [{ op: 'cache.get' }] : undefined, + // so the `ignored` span outcomes flush while the scenario is still running + clientReportFlushInterval: 1_000, }); diff --git a/dev-packages/node-integration-tests/suites/tracing/redis-dc/scenario-redis-5-tracing.mjs b/dev-packages/node-integration-tests/suites/tracing/redis-dc/scenario-redis-5-tracing.mjs index b98a66cdd273..661b37ef9c6d 100644 --- a/dev-packages/node-integration-tests/suites/tracing/redis-dc/scenario-redis-5-tracing.mjs +++ b/dev-packages/node-integration-tests/suites/tracing/redis-dc/scenario-redis-5-tracing.mjs @@ -21,6 +21,10 @@ async function run() { await redisClient.get('dc-cache:unavailable-data'); await redisClient.mGet(['dc-test-key', 'dc-cache:test-key', 'dc-cache:unavailable-data']); + + // a failing command on a cache key (GET on a list rejects with WRONGTYPE) + await redisClient.lPush('dc-cache:list-key', 'value'); + await redisClient.get('dc-cache:list-key').catch(() => {}); } finally { await redisClient.disconnect(); } diff --git a/dev-packages/node-integration-tests/suites/tracing/redis-dc/test.ts b/dev-packages/node-integration-tests/suites/tracing/redis-dc/test.ts index a02326b0a1cf..db965e173046 100644 --- a/dev-packages/node-integration-tests/suites/tracing/redis-dc/test.ts +++ b/dev-packages/node-integration-tests/suites/tracing/redis-dc/test.ts @@ -24,7 +24,7 @@ describeWithDockerCompose( 'db.query.text': 'SET dc-test-key ?', }), }), - // cache SET: span name updated to key by cacheResponseHook + // cache SET: starts as a cache span, named by its key in the static lifecycle expect.objectContaining({ description: 'dc-cache:test-key', op: 'cache.put', @@ -95,6 +95,18 @@ describeWithDockerCompose( 'db.query.text': 'MGET ? ? ?', }), }), + // a failing command on a cache key reports as an errored cache span: + // the span starts as a cache span, so the classification survives the error + expect.objectContaining({ + description: 'dc-cache:list-key', + op: 'cache.get', + status: 'internal_error', + origin: 'auto.db.redis.diagnostic_channel', + data: expect.objectContaining({ + 'cache.operation': 'get', + 'cache.key': ['dc-cache:list-key'], + }), + }), ]), }; @@ -112,6 +124,34 @@ describeWithDockerCompose( .start() .completed(); }); + + // `ignoreSpans` is evaluated at span start under streaming, so this only passes because the + // span starts as a cache span — a db span renamed at response time would slip through. + test('drops cache spans matching an ignoreSpans op filter at span start', { timeout: 60_000 }, async () => { + await createTestRunner() + .withEnv({ STREAMED: 'true', IGNORE_CACHE_GET: 'true' }) + .unignore('client_report') + // The span container and the client report flush on independent timers, so they can + // arrive in either order. + .unordered() + .expect({ + span: (container: SerializedStreamedSpanContainer) => { + const names = container.items.map(item => item.name); + expect(names).toContain('cache.put'); + expect(names).not.toContain('cache.get'); + }, + }) + .expect({ + client_report: { + discarded_events: [ + // the two GETs on cache keys plus the failing GET, which is also decided at start + { category: 'span', quantity: 3, reason: 'ignored' }, + ], + }, + }) + .start() + .completed(); + }); }); // The same commands as above, asserted on the streamed span container. With span streaming the @@ -165,8 +205,8 @@ describeWithDockerCompose( const PEER = { 'network.peer.address': HOST, 'network.peer.port': PORT }; - // A cache span is a db span the cache hook took over: it is renamed to its cache operation - // and reports the connection it inherited as peer attributes too. + // A cache span is a db span whose key matched a cache prefix: it starts named after its + // cache operation and reports the connection as peer attributes too. const cacheSpan = ( op: 'cache.get' | 'cache.put' | 'cache.remove', attributes: Record, @@ -194,7 +234,7 @@ describeWithDockerCompose( 'db.operation.name': 'SET', 'db.query.text': 'SET dc-test-key ?', }), - // cache SET: turned into a cache span, and renamed by the cache hook + // cache SET: starts as a cache span cacheSpan('cache.put', { 'db.operation.name': 'SET', 'db.query.text': 'SET dc-cache:test-key ?', @@ -233,6 +273,22 @@ describeWithDockerCompose( 'db.operation.name': 'MGET', 'db.query.text': 'MGET ? ? ?', }), + streamedSpan(`LPUSH ${HOST}:${PORT}`, 'db.query', { + 'db.operation.name': 'LPUSH', + 'db.query.text': 'LPUSH dc-cache:list-key ?', + }), + // a failing command on a cache key reports as an errored cache span: + // the span starts as a cache span, so the classification survives the error + { + ...(cacheSpan('cache.get', { + 'db.operation.name': 'GET', + 'db.query.text': 'GET dc-cache:list-key', + 'cache.key': ['dc-cache:list-key'], + 'error.type': 'Error', + 'sentry.status.message': 'WRONGTYPE Operation against a key holding the wrong kind of value', + }) as Record), + status: 'error', + }, ]); }, }) diff --git a/packages/server-utils/src/integrations/redis/index.ts b/packages/server-utils/src/integrations/redis/index.ts index 9241f4936e80..05e4135ab63c 100644 --- a/packages/server-utils/src/integrations/redis/index.ts +++ b/packages/server-utils/src/integrations/redis/index.ts @@ -25,7 +25,7 @@ import { CHANNELS } from '../../orchestrion/channels'; import { getRedisQueryNaming } from './redis-span-name'; import { defaultDbStatementSerializer } from './redis-statement-serializer'; import type { RedisCacheOptions } from './redis-cache'; -import { applyRedisCacheAttributes } from './redis-cache'; +import { applyCacheResponseAttributes, getRedisCacheAttributes } from './redis-cache'; import { bindTracingChannelToSpan } from '../../tracing-channel'; import { redisModuleNames } from '../../orchestrion/config/redis'; import { ioredisModuleNames } from '../../orchestrion/config/ioredis'; @@ -108,15 +108,21 @@ function nodeRedisAttributes(options: NodeRedisClientOptions | undefined): SpanA }; } -function startCommandSpan(commandName: string, commandArgs: Array, attributes: SpanAttributes): Span { +function startCommandSpan( + commandName: string, + commandArgs: Array, + attributes: SpanAttributes, + cacheOptions: RedisCacheOptions, +): Span { const dbStatement = defaultDbStatementSerializer(commandName, commandArgs); const { streamedName, attributes: namingAttributes } = getRedisQueryNaming(commandName, commandArgs, { host: attributes[SERVER_ADDRESS], port: attributes[SERVER_PORT], }); + const cacheProperties = getRedisCacheAttributes(commandName, commandArgs, attributes, cacheOptions); return startInactiveSpan({ - name: streamedName || dbStatement || `redis-${commandName}`, + name: cacheProperties?.name ?? streamedName ?? (dbStatement || `redis-${commandName}`), attributes: { [SENTRY_KIND]: 'client', ...attributes, @@ -124,6 +130,7 @@ function startCommandSpan(commandName: string, commandArgs: Array(); * * Exported for unit testing. */ -export function startIORedisCommandSpan(data: IORedisCommandContext): Span | undefined { +export function startIORedisCommandSpan( + data: IORedisCommandContext, + cacheOptions: RedisCacheOptions, +): Span | undefined { const command = data.arguments?.[0] as RedisCommand | undefined; if (!command || typeof command !== 'object') { return undefined; @@ -81,17 +84,19 @@ export function startIORedisCommandSpan(data: IORedisCommandContext): Span | und host, port, }); + const attributes: SpanAttributes = { + [SENTRY_KIND]: 'client', + ...connectionAttributes(host, port), + [SENTRY_OP]: DB_QUERY, + [DB_OPERATION_NAME]: command.name, + ...namingAttributes, + [DB_QUERY_TEXT]: statement, + }; + const cacheProperties = getRedisCacheAttributes(command.name, command.args ?? [], attributes, cacheOptions); return startInactiveSpan({ - name: streamedName || statement, - attributes: { - [SENTRY_KIND]: 'client', - ...connectionAttributes(host, port), - [SENTRY_OP]: DB_QUERY, - [DB_OPERATION_NAME]: command.name, - ...namingAttributes, - [DB_QUERY_TEXT]: statement, - }, + name: cacheProperties?.name ?? streamedName ?? statement, + attributes: { ...attributes, ...cacheProperties?.attributes }, }); } @@ -109,16 +114,12 @@ export function instrumentIoredis(options: RedisCacheOptions): void { CHANNELS.IOREDIS_CONNECT, ); - bindTracingChannelToSpan(commandChannel, startIORedisCommandSpan, { + bindTracingChannelToSpan(commandChannel, data => startIORedisCommandSpan(data, options), { // ioredis' `requireParentSpan` default: only create a span under an active span. requiresParentSpan: true, beforeSpanEnd(span, data) { - if ('error' in data) { - return; - } - const command = data.arguments?.[0] as RedisCommand | undefined; - if (command) { - applyRedisCacheAttributes(span, command.name, command.args, data.result, options); + if (!('error' in data)) { + applyCacheResponseAttributes(span, data.result); } }, }); diff --git a/packages/server-utils/src/integrations/redis/redis-cache.ts b/packages/server-utils/src/integrations/redis/redis-cache.ts index 633149b9fed7..428facdb01c6 100644 --- a/packages/server-utils/src/integrations/redis/redis-cache.ts +++ b/packages/server-utils/src/integrations/redis/redis-cache.ts @@ -1,14 +1,12 @@ import { CACHE_OPERATION, - NET_PEER_NAME, - NET_PEER_PORT, NETWORK_PEER_ADDRESS, NETWORK_PEER_PORT, SERVER_ADDRESS, SERVER_PORT, } from '@sentry/conventions/attributes'; import { CACHE_GET, CACHE_PUT, CACHE_REMOVE } from '@sentry/conventions/op'; -import type { Span } from '@sentry/core'; +import type { Span, SpanAttributes } from '@sentry/core'; import { CACHE_OPERATION_NAMES, getClient, @@ -48,6 +46,10 @@ export interface RedisCacheOptions { * Passing `0` will use the full cache key without truncation. * * By default, the full cache key is used. + * + * Only applies with `traceLifecycle: 'static'`. With span streaming (the default), span names are + * low cardinality: cache spans are named after the cache operation (e.g. `cache.get`) and the + * key is only added to the `cache.key` attribute, so there is nothing to truncate. */ maxCacheKeyLength?: number; } @@ -142,21 +144,22 @@ export function calculateCacheItemSize(response: unknown): number | undefined { } /** - * Turns a redis command span into a cache span when its key matches one of the configured - * `cachePrefixes`: sets the cache op, operation, key, hit/miss and item-size attributes and renames - * the span to the cache key (or, with span streaming, to the low-cardinality cache operation). - * A no-op when no `cachePrefixes` are set or the command/key is not cache-relevant. + * Decides at span-start time whether a redis command is a cache operation (its key matches one of + * the configured `cachePrefixes`) and returns the span name plus attribute overrides to merge into + * the db span options, or `undefined` for a plain db span. Callers must spread the returned + * attributes after their db attributes, so the cache op overrides the db op. Deciding at start time + * — instead of renaming the db span at response time — makes `ignoreSpans` and span streaming see + * the same op/name the user sees in the UI. * - * Runs at command response time against the already-started db span, so it can read connection - * attributes off the span and derive the item size from the response. + * `dbAttributes` are the attributes the caller starts the span with; the network peer is derived + * from `server.address`/`server.port` in there. */ -export function applyRedisCacheAttributes( - span: Span, +export function getRedisCacheAttributes( redisCommand: string, cmdArgs: RedisCommandArgs, - response: unknown, + dbAttributes: SpanAttributes, options: RedisCacheOptions, -): void { +): { name: string; attributes: SpanAttributes } | undefined { const safeKey = getCacheKeySafely(redisCommand, cmdArgs); const cacheOperation = getCacheOperation(redisCommand); @@ -167,52 +170,58 @@ export function applyRedisCacheAttributes( !shouldConsiderForCache(redisCommand, safeKey, options.cachePrefixes) ) { // not relevant for cache - return; - } - - // otel/ioredis seems to be using the old standard, as there was a change to those params: https://github.com/open-telemetry/opentelemetry-specification/issues/3199 - // We are using params based on the docs: https://opentelemetry.io/docs/specs/semconv/attributes-registry/network/ - // Fall back to stable semconv attributes (server.address/server.port) when - // old-semconv ones are absent, eg OTEL_SEMCONV_STABILITY_OPT_IN=database - // set for node-redis v4/v5. - const attributes = spanToJSON(span).attributes; - // oxlint-disable-next-line typescript/no-deprecated - const networkPeerAddress = (attributes[NET_PEER_NAME] ?? attributes[SERVER_ADDRESS]) as string | undefined; - // oxlint-disable-next-line typescript/no-deprecated - const networkPeerPort = (attributes[NET_PEER_PORT] ?? attributes[SERVER_PORT]) as number | undefined; - - if (networkPeerPort && networkPeerAddress) { - span.setAttributes({ [NETWORK_PEER_ADDRESS]: networkPeerAddress, [NETWORK_PEER_PORT]: networkPeerPort }); - } - - // A remove response is a delete-count, not a cached value, so its size is meaningless. - const cacheItemSize = isInCommands(REMOVE_COMMANDS, redisCommand) ? undefined : calculateCacheItemSize(response); - - if (cacheItemSize) { - span.setAttribute(SEMANTIC_ATTRIBUTE_CACHE_ITEM_SIZE, cacheItemSize); - } - - if (isInCommands(GET_COMMANDS, redisCommand) && cacheItemSize !== undefined) { - span.setAttribute(SEMANTIC_ATTRIBUTE_CACHE_HIT, cacheItemSize > 0); + return undefined; } - span.setAttributes({ + const attributes: SpanAttributes = { [SEMANTIC_ATTRIBUTE_SENTRY_OP]: cacheOperation, [SEMANTIC_ATTRIBUTE_CACHE_KEY]: safeKey, [CACHE_OPERATION]: CACHE_OPERATION_NAMES[cacheOperation], - }); + }; + + const networkPeerAddress = dbAttributes[SERVER_ADDRESS] as string | undefined; + const networkPeerPort = dbAttributes[SERVER_PORT] as number | undefined; + if (networkPeerPort && networkPeerAddress) { + attributes[NETWORK_PEER_ADDRESS] = networkPeerAddress; + attributes[NETWORK_PEER_PORT] = networkPeerPort; + } const client = getClient(); if (client && hasSpanStreamingEnabled(client)) { // With span streaming, span names have to be low cardinality, so we can't fall back to the cache key. - span.updateName(cacheOperation); - return; + return { name: cacheOperation, attributes }; } // todo: change to string[] once EAP supports it const spanDescription = safeKey.join(', '); - span.updateName(options.maxCacheKeyLength ? truncate(spanDescription, options.maxCacheKeyLength) : spanDescription); + return { + name: options.maxCacheKeyLength ? truncate(spanDescription, options.maxCacheKeyLength) : spanDescription, + attributes, + }; +} + +/** + * Sets the response-derived cache attributes (`cache.hit`, `cache.item_size`) on a span that was + * started as a cache span via {@link getRedisCacheAttributes}. A no-op for plain db spans and for + * `cache.remove` spans — a remove response is a delete-count, not a cached value, so its size is + * meaningless. + */ +export function applyCacheResponseAttributes(span: Span, response: unknown): void { + const op = spanToJSON(span).attributes[SEMANTIC_ATTRIBUTE_SENTRY_OP]; + if (op !== CACHE_GET && op !== CACHE_PUT) { + return; + } + + const cacheItemSize = calculateCacheItemSize(response); + + if (cacheItemSize) { + span.setAttribute(SEMANTIC_ATTRIBUTE_CACHE_ITEM_SIZE, cacheItemSize); + } + + if (op === CACHE_GET && cacheItemSize !== undefined) { + span.setAttribute(SEMANTIC_ATTRIBUTE_CACHE_HIT, cacheItemSize > 0); + } } type NestedArray = Array | T>; diff --git a/packages/server-utils/src/integrations/redis/redis-dc-subscriber.ts b/packages/server-utils/src/integrations/redis/redis-dc-subscriber.ts index 8ff655078f3c..090b02d7272e 100644 --- a/packages/server-utils/src/integrations/redis/redis-dc-subscriber.ts +++ b/packages/server-utils/src/integrations/redis/redis-dc-subscriber.ts @@ -9,10 +9,11 @@ import { SENTRY_OP, } from '@sentry/conventions/attributes'; import { DB_QUERY, DB } from '@sentry/conventions/op'; +import type { SpanAttributes } from '@sentry/core'; import { SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, startInactiveSpan } from '@sentry/core'; import { bindTracingChannelToSpan } from '../../tracing-channel'; import type { RedisCacheOptions } from './redis-cache'; -import { applyRedisCacheAttributes } from './redis-cache'; +import { applyCacheResponseAttributes, getRedisCacheAttributes } from './redis-cache'; import { getRedisQueryNaming } from './redis-span-name'; // Channel names published by node-redis >= 5.12.0 and ioredis >= 5.11.0. @@ -146,24 +147,26 @@ function setupCommandChannel( host: data.serverAddress, port: data.serverPort, }); + const attributes: SpanAttributes = { + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: ORIGIN, + [SENTRY_OP]: DB_QUERY, + [DB_SYSTEM_NAME]: DB_SYSTEM_NAME_VALUE_REDIS, + [DB_OPERATION_NAME]: data.command, + ...namingAttributes, + [DB_QUERY_TEXT]: statement, + ...(data.serverAddress != null ? { [SERVER_ADDRESS]: data.serverAddress } : {}), + ...(data.serverPort != null ? { [SERVER_PORT]: data.serverPort } : {}), + }; + const cacheProperties = getRedisCacheAttributes(data.command, args, attributes, cacheOptions); return startInactiveSpan({ - name: streamedName || `redis-${data.command}`, - attributes: { - [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: ORIGIN, - [SENTRY_OP]: DB_QUERY, - [DB_SYSTEM_NAME]: DB_SYSTEM_NAME_VALUE_REDIS, - [DB_OPERATION_NAME]: data.command, - ...namingAttributes, - [DB_QUERY_TEXT]: statement, - ...(data.serverAddress != null ? { [SERVER_ADDRESS]: data.serverAddress } : {}), - ...(data.serverPort != null ? { [SERVER_PORT]: data.serverPort } : {}), - }, + name: cacheProperties?.name ?? streamedName ?? `redis-${data.command}`, + attributes: { ...attributes, ...cacheProperties?.attributes }, }); }, { beforeSpanEnd(span, data) { if ('error' in data) return; - applyRedisCacheAttributes(span, data.command, getCommandArgs(data), data.result, cacheOptions); + applyCacheResponseAttributes(span, data.result); }, }, ); diff --git a/packages/server-utils/test/integrations/redis/ioredis-channel-subscriber.test.ts b/packages/server-utils/test/integrations/redis/ioredis-channel-subscriber.test.ts index 4ed2fb440ec4..37cde9449001 100644 --- a/packages/server-utils/test/integrations/redis/ioredis-channel-subscriber.test.ts +++ b/packages/server-utils/test/integrations/redis/ioredis-channel-subscriber.test.ts @@ -24,7 +24,7 @@ describe('startIORedisCommandSpan', () => { }); it('builds a db query span with Sentry convention attributes', () => { - startIORedisCommandSpan(ctx({ name: 'set', args: ['test-key', 'test-value'] })); + startIORedisCommandSpan(ctx({ name: 'set', args: ['test-key', 'test-value'] }), {}); expect(startInactiveSpanSpy).toHaveBeenCalledWith( expect.objectContaining({ @@ -42,12 +42,31 @@ describe('startIORedisCommandSpan', () => { ); }); + it('starts the span as a cache span when the key matches a cache prefix', () => { + startIORedisCommandSpan(ctx({ name: 'get', args: ['ioredis-cache:test-key'] }), { + cachePrefixes: ['ioredis-cache:'], + }); + + expect(startInactiveSpanSpy).toHaveBeenCalledWith( + expect.objectContaining({ + name: 'ioredis-cache:test-key', + attributes: expect.objectContaining({ + 'sentry.op': 'cache.get', + 'cache.operation': 'get', + 'cache.key': ['ioredis-cache:test-key'], + 'network.peer.address': 'localhost', + 'network.peer.port': 6379, + }), + }), + ); + }); + it('names the span from the conventions with span streaming enabled', () => { vi.spyOn(SentryCore, 'getClient').mockReturnValue({ getOptions: () => ({ traceLifecycle: 'stream' }), } as unknown as ReturnType); - startIORedisCommandSpan(ctx({ name: 'set', args: ['test-key', 'test-value'] })); + startIORedisCommandSpan(ctx({ name: 'set', args: ['test-key', 'test-value'] }), {}); expect(startInactiveSpanSpy).toHaveBeenCalledWith( expect.objectContaining({ @@ -66,7 +85,7 @@ describe('startIORedisCommandSpan', () => { getOptions: () => ({ traceLifecycle: 'stream' }), } as unknown as ReturnType); - startIORedisCommandSpan(ctx({ name: 'fcall', args: ['my_func', '1', 'test-key'] })); + startIORedisCommandSpan(ctx({ name: 'fcall', args: ['my_func', '1', 'test-key'] }), {}); expect(startInactiveSpanSpy).toHaveBeenCalledWith( expect.objectContaining({ @@ -85,7 +104,7 @@ describe('startIORedisCommandSpan', () => { getOptions: () => ({ traceLifecycle: 'stream' }), } as unknown as ReturnType); - startIORedisCommandSpan(ctx({ name: 'fcall', args: ['?', '1', 'test-key'] })); + startIORedisCommandSpan(ctx({ name: 'fcall', args: ['?', '1', 'test-key'] }), {}); expect(startInactiveSpanSpy).toHaveBeenCalledWith( expect.objectContaining({ @@ -100,7 +119,7 @@ describe('startIORedisCommandSpan', () => { getOptions: () => ({ traceLifecycle: 'stream' }), } as unknown as ReturnType); - startIORedisCommandSpan(ctx({ name: 'set', args: ['test-key', 'test-value'] }, { port: 6379 })); + startIORedisCommandSpan(ctx({ name: 'set', args: ['test-key', 'test-value'] }, { port: 6379 }), {}); // `{db.system.name}` — the address/port template needs both halves expect(startInactiveSpanSpy).toHaveBeenCalledWith(expect.objectContaining({ name: 'redis' })); @@ -109,20 +128,20 @@ describe('startIORedisCommandSpan', () => { it('emits a single span when the same command is re-sent from the offline queue', () => { const command = { name: 'set', args: ['test-key', 'test-value'] }; - expect(startIORedisCommandSpan(ctx(command))).toBeDefined(); - expect(startIORedisCommandSpan(ctx(command))).toBeUndefined(); + expect(startIORedisCommandSpan(ctx(command), {})).toBeDefined(); + expect(startIORedisCommandSpan(ctx(command), {})).toBeUndefined(); expect(startInactiveSpanSpy).toHaveBeenCalledTimes(1); }); it('spans distinct command objects with the same statement', () => { - startIORedisCommandSpan(ctx({ name: 'get', args: ['k'] })); - startIORedisCommandSpan(ctx({ name: 'get', args: ['k'] })); + startIORedisCommandSpan(ctx({ name: 'get', args: ['k'] }), {}); + startIORedisCommandSpan(ctx({ name: 'get', args: ['k'] }), {}); expect(startInactiveSpanSpy).toHaveBeenCalledTimes(2); }); it('skips payloads without a command object', () => { - expect(startIORedisCommandSpan({ arguments: [], self: { options: CONNECTION } })).toBeUndefined(); + expect(startIORedisCommandSpan({ arguments: [], self: { options: CONNECTION } }, {})).toBeUndefined(); expect(startInactiveSpanSpy).not.toHaveBeenCalled(); }); }); diff --git a/packages/server-utils/test/integrations/redis/redis-cache.test.ts b/packages/server-utils/test/integrations/redis/redis-cache.test.ts index 326cb813ccc4..aebbc57eb733 100644 --- a/packages/server-utils/test/integrations/redis/redis-cache.test.ts +++ b/packages/server-utils/test/integrations/redis/redis-cache.test.ts @@ -1,8 +1,17 @@ -import { CACHE_KEY, CACHE_OPERATION, SENTRY_OP, SENTRY_SEGMENT_NAME_SOURCE } from '@sentry/conventions/attributes'; -import { setCurrentClient } from '@sentry/core'; -import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { - applyRedisCacheAttributes, + CACHE_KEY, + CACHE_OPERATION, + NETWORK_PEER_ADDRESS, + NETWORK_PEER_PORT, + SENTRY_OP, + SERVER_ADDRESS, + SERVER_PORT, +} from '@sentry/conventions/attributes'; +import { SentrySpan, setCurrentClient, spanToJSON } from '@sentry/core'; +import { afterEach, describe, expect, it } from 'vitest'; +import { + applyCacheResponseAttributes, + getRedisCacheAttributes, calculateCacheItemSize, GET_COMMANDS, getCacheKeySafely, @@ -19,75 +28,87 @@ function setUpClient(traceLifecycle: 'stream' | 'static'): void { } describe('redis cache', () => { - describe('applyRedisCacheAttributes', () => { - let mockSpan: any; - - beforeEach(() => { - mockSpan = { - setAttribute: vi.fn(), - setAttributes: vi.fn(), - updateName: vi.fn(), - spanContext: () => ({ spanId: 'test-span-id', traceId: 'test-trace-id' }), - }; + describe('getRedisCacheAttributes', () => { + it.each([ + { desc: 'no args', cmd: 'get', args: [], options: {} }, + { desc: 'unsupported command', cmd: 'exists', args: ['key'], options: {} }, + { desc: 'no cache prefixes', cmd: 'get', args: ['key'], options: {} }, + { desc: 'non-matching prefix', cmd: 'get', args: ['key'], options: { cachePrefixes: ['c'] } }, + ])('should return undefined when $desc', ({ cmd, args, options }) => { + expect(getRedisCacheAttributes(cmd, args, {}, options)).toBeUndefined(); }); - afterEach(() => { - vi.restoreAllMocks(); + it('should return cache op, key and network peer attributes for a matching key', () => { + const result = getRedisCacheAttributes( + 'get', + ['cache:test-key'], + { [SERVER_ADDRESS]: 'localhost', [SERVER_PORT]: 6379 }, + { cachePrefixes: ['cache:'] }, + ); + + expect(result).toStrictEqual({ + name: 'cache:test-key', + attributes: { + [SENTRY_OP]: 'cache.get', + [CACHE_KEY]: ['cache:test-key'], + [CACHE_OPERATION]: 'get', + [NETWORK_PEER_ADDRESS]: 'localhost', + [NETWORK_PEER_PORT]: 6379, + }, + }); }); - describe('early returns', () => { - it.each([ - { desc: 'no args', cmd: 'get', args: [], response: 'test', options: {} }, - { desc: 'unsupported command', cmd: 'exists', args: ['key'], response: 'test', options: {} }, - { desc: 'no cache prefixes', cmd: 'get', args: ['key'], response: 'test', options: {} }, - { desc: 'non-matching prefix', cmd: 'get', args: ['key'], response: 'test', options: { cachePrefixes: ['c'] } }, - ])('should return early without modifying span when $desc', ({ cmd, args, response, options }) => { - applyRedisCacheAttributes(mockSpan, cmd, args, response, options); - - expect(mockSpan.setAttribute).not.toHaveBeenCalled(); - expect(mockSpan.setAttributes).not.toHaveBeenCalled(); - expect(mockSpan.updateName).not.toHaveBeenCalled(); + it('should omit network peer attributes when the db attributes have no server address', () => { + const result = getRedisCacheAttributes('del', ['cache:test-key'], {}, { cachePrefixes: ['cache:'] }); + + expect(result).toStrictEqual({ + name: 'cache:test-key', + attributes: { + [SENTRY_OP]: 'cache.remove', + [CACHE_KEY]: ['cache:test-key'], + [CACHE_OPERATION]: 'remove', + }, }); }); describe('span name truncation', () => { it('should not truncate span name when maxCacheKeyLength is not set', () => { - applyRedisCacheAttributes( - mockSpan, + const result = getRedisCacheAttributes( 'mget', ['cache:very-long-key-name', 'cache:very-long-key-name-2', 'cache:very-long-key-name-3'], - 'value', + {}, { cachePrefixes: ['cache:'] }, ); - expect(mockSpan.updateName).toHaveBeenCalledWith( - 'cache:very-long-key-name, cache:very-long-key-name-2, cache:very-long-key-name-3', - ); - expect(mockSpan.setAttribute).not.toHaveBeenCalledWith(SENTRY_SEGMENT_NAME_SOURCE, undefined); + expect(result?.name).toBe('cache:very-long-key-name, cache:very-long-key-name-2, cache:very-long-key-name-3'); }); it('should truncate span name when maxCacheKeyLength is set', () => { - applyRedisCacheAttributes(mockSpan, 'get', ['cache:very-long-key-name'], 'value', { - cachePrefixes: ['cache:'], - maxCacheKeyLength: 10, - }); + const result = getRedisCacheAttributes( + 'get', + ['cache:very-long-key-name'], + {}, + { + cachePrefixes: ['cache:'], + maxCacheKeyLength: 10, + }, + ); - expect(mockSpan.updateName).toHaveBeenCalledWith('cache:very...'); + expect(result?.name).toBe('cache:very...'); }); it('should truncate multiple keys joined with commas', () => { - applyRedisCacheAttributes( - mockSpan, + const result = getRedisCacheAttributes( 'mget', ['cache:key1', 'cache:key2', 'cache:key3'], - ['val1', 'val2', 'val3'], + {}, { cachePrefixes: ['cache:'], maxCacheKeyLength: 20, }, ); - expect(mockSpan.updateName).toHaveBeenCalledWith('cache:key1, cache:ke...'); + expect(result?.name).toBe('cache:key1, cache:ke...'); }); }); @@ -103,30 +124,66 @@ describe('redis cache', () => { ])('names a streamed $op span after the cache operation', ({ cmd, op, operation }) => { setUpClient('stream'); - applyRedisCacheAttributes(mockSpan, cmd, ['cache:user-42'], 'value', { cachePrefixes: ['cache:'] }); + const result = getRedisCacheAttributes(cmd, ['cache:user-42'], {}, { cachePrefixes: ['cache:'] }); - expect(mockSpan.updateName).toHaveBeenCalledWith(op); // The key is high cardinality, so it only lives on the attribute. - expect(mockSpan.setAttributes).toHaveBeenCalledWith( - expect.objectContaining({ + expect(result).toStrictEqual({ + name: op, + attributes: { [SENTRY_OP]: op, [CACHE_OPERATION]: operation, [CACHE_KEY]: ['cache:user-42'], - }), - ); + }, + }); }); it('keeps the cache key as the span name when span streaming is off', () => { setUpClient('static'); - applyRedisCacheAttributes(mockSpan, 'get', ['cache:user-42'], 'value', { cachePrefixes: ['cache:'] }); + const result = getRedisCacheAttributes('get', ['cache:user-42'], {}, { cachePrefixes: ['cache:'] }); - expect(mockSpan.updateName).toHaveBeenCalledWith('cache:user-42'); - expect(mockSpan.setAttributes).toHaveBeenCalledWith(expect.objectContaining({ [CACHE_OPERATION]: 'get' })); + expect(result?.name).toBe('cache:user-42'); + expect(result?.attributes).toEqual(expect.objectContaining({ [CACHE_OPERATION]: 'get' })); }); }); }); + describe('applyCacheResponseAttributes', () => { + const cacheSpan = (op: string): SentrySpan => + new SentrySpan({ name: 'cache:test-key', attributes: { [SENTRY_OP]: op } }); + + it('should set item size and cache hit on a cache.get span', () => { + const span = cacheSpan('cache.get'); + applyCacheResponseAttributes(span, 'test-value'); + + expect(spanToJSON(span).attributes).toMatchObject({ 'cache.item_size': 10, 'cache.hit': true }); + }); + + it('should set a cache miss for an empty cache.get response', () => { + const span = cacheSpan('cache.get'); + applyCacheResponseAttributes(span, null); + + expect(spanToJSON(span).attributes).toMatchObject({ 'cache.hit': false }); + expect(spanToJSON(span).attributes).not.toHaveProperty('cache.item_size'); + }); + + it('should set only the item size on a cache.put span', () => { + const span = cacheSpan('cache.put'); + applyCacheResponseAttributes(span, 'OK'); + + expect(spanToJSON(span).attributes).toMatchObject({ 'cache.item_size': 2 }); + expect(spanToJSON(span).attributes).not.toHaveProperty('cache.hit'); + }); + + it.each(['cache.remove', 'db.query'])('should not modify a %s span', op => { + const span = cacheSpan(op); + applyCacheResponseAttributes(span, 'test-value'); + + expect(spanToJSON(span).attributes).not.toHaveProperty('cache.item_size'); + expect(spanToJSON(span).attributes).not.toHaveProperty('cache.hit'); + }); + }); + describe('getCacheKeySafely (single arg)', () => { it('should return an empty string if there are no command arguments', () => { const result = getCacheKeySafely('get', []); From 60113622e11204e52af6a1eee2fb14439c51b664 Mon Sep 17 00:00:00 2001 From: Martin Sonnberger Date: Thu, 3 Sep 2026 11:16:28 +0200 Subject: [PATCH 05/15] feat(core): Add `safeCallback` helper for isolating user-provided callbacks (#23760) Adds `safeCallback(message, fn, fallback)` helper which runs a user-provided callback, and on a sync throw or async rejection logs `message` via `debug.error` and returns `fallback(error)` instead of propagating. `applyBeforeSendSpanCallback` and the undici `safeExecute` are refactored onto the helper so there is a single implementation. First step of #23755, next PR will wrap the rest of the user-defined callbacks. Co-authored-by: Claude Fable 5 --- packages/core/src/index.ts | 1 + .../core/src/tracing/spans/beforeSendSpan.ts | 43 ++++++------ packages/core/src/utils/safeCallback.ts | 35 ++++++++++ .../core/test/lib/utils/safeCallback.test.ts | 70 +++++++++++++++++++ .../node-fetch/undici-instrumentation.ts | 26 +++---- 5 files changed, 138 insertions(+), 37 deletions(-) create mode 100644 packages/core/src/utils/safeCallback.ts create mode 100644 packages/core/test/lib/utils/safeCallback.test.ts diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index afdeac10921a..dc01c23fda8f 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -94,6 +94,7 @@ export { export { safeSetSpanJSONAttributes } from './tracing/spans/captureSpan'; export { isSentryRequestUrl } from './utils/isSentryRequestUrl'; export { handleCallbackErrors } from './utils/handleCallbackErrors'; +export { safeCallback } from './utils/safeCallback'; export { parameterize, fmt } from './utils/parameterize'; export type { HandleTunnelRequestOptions } from './utils/tunnel'; export { handleTunnelRequest } from './utils/tunnel'; diff --git a/packages/core/src/tracing/spans/beforeSendSpan.ts b/packages/core/src/tracing/spans/beforeSendSpan.ts index 1d1126d7f883..b2fcd8261079 100644 --- a/packages/core/src/tracing/spans/beforeSendSpan.ts +++ b/packages/core/src/tracing/spans/beforeSendSpan.ts @@ -2,7 +2,8 @@ import { DEBUG_BUILD } from '../../debug-build'; import type { BeforeSendStaticSpanCallback, BeforeSendStreamedSpanCallback } from '../../types/options'; import type { SpanJSON, StreamedSpanJSON } from '../../types/span'; import { addNonEnumerableProperty } from '../../utils/object'; -import { consoleSandbox, debug } from '../../utils/debug-logger'; +import { consoleSandbox } from '../../utils/debug-logger'; +import { safeCallback } from '../../utils/safeCallback'; /** * A wrapper to use the static, transaction-based span format in your `beforeSendSpan` callback. @@ -64,25 +65,25 @@ export function applyBeforeSendSpanCallback T, ): T { - try { - const modifedSpan = beforeSendSpan(span); - if (!modifedSpan) { - if (!hasShownSpanDropWarning) { - consoleSandbox(() => { - // eslint-disable-next-line no-console - console.warn( - '[Sentry] Returning null from `beforeSendSpan` is disallowed. To drop certain spans, configure the respective integrations directly or use `ignoreSpans`.', - ); - }); - hasShownSpanDropWarning = true; - } - return span; - } - return modifedSpan; - } catch (error) { - // Spans are captured synchronously when they end, so a throwing callback would otherwise - // propagate into whatever user code ended the span. - DEBUG_BUILD && debug.error('The `beforeSendSpan` callback threw an error, sending the span unmodified:', error); - return span; + // Spans are captured synchronously when they end, so a throwing callback would otherwise + // propagate into whatever user code ended the span. + const modifiedSpan = safeCallback( + DEBUG_BUILD ? 'The `beforeSendSpan` callback threw an error, sending the span unmodified:' : '', + () => beforeSendSpan(span), + () => span, + ); + if (modifiedSpan) { + return modifiedSpan; } + + if (!hasShownSpanDropWarning) { + consoleSandbox(() => { + // eslint-disable-next-line no-console + console.warn( + '[Sentry] Returning null from `beforeSendSpan` is disallowed. To drop certain spans, configure the respective integrations directly or use `ignoreSpans`.', + ); + }); + hasShownSpanDropWarning = true; + } + return span; } diff --git a/packages/core/src/utils/safeCallback.ts b/packages/core/src/utils/safeCallback.ts new file mode 100644 index 000000000000..5b9079ca7c5d --- /dev/null +++ b/packages/core/src/utils/safeCallback.ts @@ -0,0 +1,35 @@ +import { DEBUG_BUILD } from '../debug-build'; +import { debug } from './debug-logger'; +import { isThenable } from './is'; + +/** + * Invokes a user-provided callback (e.g. `beforeSend`, `tracesSampler`, an integration hook) so that + * neither a synchronous throw nor a rejected promise escapes into the caller. On failure the error is + * logged and `fallback(error)` supplies the result instead. + * + * Not for `startSpan` bodies: those must re-throw and are handled by `handleCallbackErrors`. + * + * @param message - Logged via `debug.error` together with the error. Pass it as `DEBUG_BUILD ? '...' : ''` + * so the string is tree-shaken from non-debug bundles. + * @param fn - Invokes the callback. + * @param fallback - Produces the result to use when the callback throws or rejects. + */ +export function safeCallback(message: string, fn: () => T, fallback: (error: unknown) => T): T { + let result: T; + try { + result = fn(); + } catch (error) { + return recover(message, error, fallback); + } + + if (isThenable(result)) { + return result.then(undefined, (error: unknown) => recover(message, error, fallback)) as T; + } + + return result; +} + +function recover(message: string, error: unknown, fallback: (error: unknown) => T): T { + DEBUG_BUILD && debug.error(message, error); + return fallback(error); +} diff --git a/packages/core/test/lib/utils/safeCallback.test.ts b/packages/core/test/lib/utils/safeCallback.test.ts new file mode 100644 index 000000000000..dc5c4552d6ae --- /dev/null +++ b/packages/core/test/lib/utils/safeCallback.test.ts @@ -0,0 +1,70 @@ +import { afterEach, describe, expect, it, vi } from 'vitest'; +import { debug } from '../../../src/utils/debug-logger'; +import { safeCallback } from '../../../src/utils/safeCallback'; + +describe('safeCallback', () => { + const debugErrorSpy = vi.spyOn(debug, 'error').mockImplementation(() => undefined); + + afterEach(() => { + debugErrorSpy.mockClear(); + }); + + it('returns the result of a sync callback', () => { + const fallback = vi.fn(() => 'fallback'); + + expect(safeCallback('callback threw:', () => 'value', fallback)).toBe('value'); + expect(fallback).not.toHaveBeenCalled(); + expect(debugErrorSpy).not.toHaveBeenCalled(); + }); + + it('returns the fallback and logs when a sync callback throws', () => { + const error = new Error('boom'); + const fallback = vi.fn(() => 'fallback'); + + expect( + safeCallback( + 'callback threw:', + () => { + throw error; + }, + fallback, + ), + ).toBe('fallback'); + expect(fallback).toHaveBeenCalledWith(error); + expect(debugErrorSpy).toHaveBeenCalledWith('callback threw:', error); + }); + + it('resolves to the result of an async callback', async () => { + const fallback = vi.fn(async () => 'fallback'); + + const result = safeCallback('callback threw:', async () => 'value', fallback); + + expect(result).toBeInstanceOf(Promise); + await expect(result).resolves.toBe('value'); + expect(fallback).not.toHaveBeenCalled(); + expect(debugErrorSpy).not.toHaveBeenCalled(); + }); + + it('resolves to the fallback and logs when an async callback rejects', async () => { + const error = new Error('boom'); + const fallback = vi.fn(async () => 'fallback'); + + const result = safeCallback('callback threw:', () => Promise.reject(error), fallback); + + await expect(result).resolves.toBe('fallback'); + expect(fallback).toHaveBeenCalledWith(error); + expect(debugErrorSpy).toHaveBeenCalledWith('callback threw:', error); + }); + + it('does not treat non-thenable objects as promises', () => { + const value = { then: 'not a function' }; + + expect( + safeCallback( + 'callback threw:', + () => value, + () => ({ then: 'fallback' }), + ), + ).toBe(value); + }); +}); diff --git a/packages/node/src/integrations/node-fetch/undici-instrumentation.ts b/packages/node/src/integrations/node-fetch/undici-instrumentation.ts index 31ef1da962e6..b2989e6c6bdf 100644 --- a/packages/node/src/integrations/node-fetch/undici-instrumentation.ts +++ b/packages/node/src/integrations/node-fetch/undici-instrumentation.ts @@ -30,6 +30,7 @@ import { isTracingSuppressed, LRUMap, parseUrl, + safeCallback, SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, SPAN_STATUS_ERROR, @@ -113,16 +114,6 @@ export function instrumentUndici(config: NodeFetchOptions = {}): void { subscribeToChannel('undici:request:error', message => onError(message as RequestErrorMessage)); } -/** Replaces OTel's `safeExecuteInTheMiddle`: run `fn`, route any error to `onError`, and swallow it. */ -function safeExecute(fn: () => T, onError: (error: unknown) => void): T | undefined { - try { - return fn(); - } catch (error) { - onError(error); - return undefined; - } -} - function subscribeToChannel( diagnosticChannel: string, onMessage: (message: unknown, name: string | symbol) => void, @@ -180,9 +171,10 @@ function parseRequestHeaders(request: UndiciRequest): Map !!config.ignoreOutgoingRequests?.(url), - e => e && DEBUG_BUILD && debug.error('caught ignoreOutgoingRequests error: ', e), + () => false, ); // Breadcrumbs & span-less trace propagation are additionally skipped when tracing is suppressed. @@ -288,9 +280,10 @@ function onRequestCreated(config: NodeFetchOptions, { request }: RequestMessage) }); // Execute the request hook if defined - safeExecute( + safeCallback( + DEBUG_BUILD ? 'The `requestHook` callback threw an error:' : '', () => config.requestHook?.(span, request), - e => e && DEBUG_BUILD && debug.error('caught requestHook error: ', e), + () => undefined, ); // Context propagation goes last so no hook can tamper the propagation headers. @@ -356,9 +349,10 @@ function onResponseHeaders(config: NodeFetchOptions, { request, response }: Resp }; // Execute the response hook if defined - safeExecute( + safeCallback( + DEBUG_BUILD ? 'The `responseHook` callback threw an error:' : '', () => config.responseHook?.(span, { request, response }), - e => e && DEBUG_BUILD && debug.error('caught responseHook error: ', e), + () => undefined, ); if (config.headersToSpanAttributes?.responseHeaders) { From 449b6429a546cb270e99b5779da2cd3ec4ba08e1 Mon Sep 17 00:00:00 2001 From: Martin Sonnberger Date: Thu, 3 Sep 2026 11:16:28 +0200 Subject: [PATCH 06/15] feat(core): Isolate throwing user callbacks instead of capturing them as events (#23770) Wraps `beforeSend`, `beforeSendTransaction`, event processors, `tracesSampler`, `beforeBreadcrumb`, `beforeSendLog` and `beforeSendMetric` in the `safeCallback` helper from #23760. A throwing or rejecting callback no longer escapes into the calling code and is no longer captured as an `internal` error event; the event/breadcrumb/log/metric is dropped, a client report is recorded where a category exists, and the error is logged in debug mode. part of #23755 --------- Co-authored-by: Claude Fable 5 --- .../before-send-throws/scenario.ts | 15 ++ .../drop-reasons/before-send-throws/test.ts | 24 ++ .../event-processor-throws/scenario-async.ts | 17 ++ .../event-processor-throws/scenario.ts | 16 ++ .../event-processor-throws/test.ts | 42 +++ .../scenario-fallback.ts | 19 ++ .../traces-sampler-throws/scenario.ts | 17 ++ .../traces-sampler-throws/test.ts | 35 +++ packages/core/src/breadcrumbs.ts | 8 +- packages/core/src/client.ts | 14 +- packages/core/src/eventProcessors.ts | 11 +- packages/core/src/logs/internal.ts | 11 +- packages/core/src/metrics/internal.ts | 10 +- packages/core/src/tracing/sampling.ts | 89 ++++--- packages/core/test/lib/client.test.ts | 240 ++++++++++++------ .../core/test/lib/eventProcessors.test.ts | 54 ++++ packages/core/test/lib/logs/internal.test.ts | 29 +++ .../core/test/lib/metrics/internal.test.ts | 32 +++ .../core/test/lib/tracing/sampling.test.ts | 50 ++++ 19 files changed, 624 insertions(+), 109 deletions(-) create mode 100644 dev-packages/node-integration-tests/suites/client-reports/drop-reasons/before-send-throws/scenario.ts create mode 100644 dev-packages/node-integration-tests/suites/client-reports/drop-reasons/before-send-throws/test.ts create mode 100644 dev-packages/node-integration-tests/suites/client-reports/drop-reasons/event-processor-throws/scenario-async.ts create mode 100644 dev-packages/node-integration-tests/suites/client-reports/drop-reasons/event-processor-throws/scenario.ts create mode 100644 dev-packages/node-integration-tests/suites/client-reports/drop-reasons/event-processor-throws/test.ts create mode 100644 dev-packages/node-integration-tests/suites/client-reports/drop-reasons/traces-sampler-throws/scenario-fallback.ts create mode 100644 dev-packages/node-integration-tests/suites/client-reports/drop-reasons/traces-sampler-throws/scenario.ts create mode 100644 dev-packages/node-integration-tests/suites/client-reports/drop-reasons/traces-sampler-throws/test.ts create mode 100644 packages/core/test/lib/eventProcessors.test.ts create mode 100644 packages/core/test/lib/tracing/sampling.test.ts diff --git a/dev-packages/node-integration-tests/suites/client-reports/drop-reasons/before-send-throws/scenario.ts b/dev-packages/node-integration-tests/suites/client-reports/drop-reasons/before-send-throws/scenario.ts new file mode 100644 index 000000000000..299630f41cce --- /dev/null +++ b/dev-packages/node-integration-tests/suites/client-reports/drop-reasons/before-send-throws/scenario.ts @@ -0,0 +1,15 @@ +import * as Sentry from '@sentry/node'; +import { loggingTransport } from '@sentry-internal/node-integration-tests'; + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + transport: loggingTransport, + beforeSend() { + throw new Error('beforeSend failed'); + }, +}); + +Sentry.captureException(new Error('this should get dropped because beforeSend throws')); + +// eslint-disable-next-line @typescript-eslint/no-floating-promises +Sentry.flush(); diff --git a/dev-packages/node-integration-tests/suites/client-reports/drop-reasons/before-send-throws/test.ts b/dev-packages/node-integration-tests/suites/client-reports/drop-reasons/before-send-throws/test.ts new file mode 100644 index 000000000000..e30038efc57b --- /dev/null +++ b/dev-packages/node-integration-tests/suites/client-reports/drop-reasons/before-send-throws/test.ts @@ -0,0 +1,24 @@ +import { afterAll, test } from 'vitest'; +import { cleanupChildProcesses, createRunner } from '../../../../utils/runner'; + +afterAll(() => { + cleanupChildProcesses(); +}); + +test('records a client report and no extra error event when beforeSend throws', async () => { + await createRunner(__dirname, 'scenario.ts') + .unignore('client_report') + .expect({ + client_report: { + discarded_events: [ + { + category: 'error', + quantity: 1, + reason: 'before_send', + }, + ], + }, + }) + .start() + .completed(); +}); diff --git a/dev-packages/node-integration-tests/suites/client-reports/drop-reasons/event-processor-throws/scenario-async.ts b/dev-packages/node-integration-tests/suites/client-reports/drop-reasons/event-processor-throws/scenario-async.ts new file mode 100644 index 000000000000..bade2d046447 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/client-reports/drop-reasons/event-processor-throws/scenario-async.ts @@ -0,0 +1,17 @@ +import * as Sentry from '@sentry/node'; +import { loggingTransport } from '@sentry-internal/node-integration-tests'; + +Sentry.init({ + traceLifecycle: 'static', + dsn: 'https://public@dsn.ingest.sentry.io/1337', + transport: loggingTransport, +}); + +Sentry.addEventProcessor(async () => { + throw new Error('async event processor failed'); +}); + +Sentry.captureException(new Error('this should get dropped because the async event processor rejects')); + +// eslint-disable-next-line @typescript-eslint/no-floating-promises +Sentry.flush(); diff --git a/dev-packages/node-integration-tests/suites/client-reports/drop-reasons/event-processor-throws/scenario.ts b/dev-packages/node-integration-tests/suites/client-reports/drop-reasons/event-processor-throws/scenario.ts new file mode 100644 index 000000000000..31d53fa48621 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/client-reports/drop-reasons/event-processor-throws/scenario.ts @@ -0,0 +1,16 @@ +import * as Sentry from '@sentry/node'; +import { loggingTransport } from '@sentry-internal/node-integration-tests'; + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + transport: loggingTransport, +}); + +Sentry.addEventProcessor(() => { + throw new Error('event processor failed'); +}); + +Sentry.captureException(new Error('this should get dropped because the event processor throws')); + +// eslint-disable-next-line @typescript-eslint/no-floating-promises +Sentry.flush(); diff --git a/dev-packages/node-integration-tests/suites/client-reports/drop-reasons/event-processor-throws/test.ts b/dev-packages/node-integration-tests/suites/client-reports/drop-reasons/event-processor-throws/test.ts new file mode 100644 index 000000000000..370cccc35410 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/client-reports/drop-reasons/event-processor-throws/test.ts @@ -0,0 +1,42 @@ +import { afterAll, test } from 'vitest'; +import { cleanupChildProcesses, createRunner } from '../../../../utils/runner'; + +afterAll(() => { + cleanupChildProcesses(); +}); + +test('records a client report and no extra error event when an event processor throws', async () => { + await createRunner(__dirname, 'scenario.ts') + .unignore('client_report') + .expect({ + client_report: { + discarded_events: [ + { + category: 'error', + quantity: 1, + reason: 'event_processor', + }, + ], + }, + }) + .start() + .completed(); +}); + +test('records a client report and no extra error event when an async event processor rejects', async () => { + await createRunner(__dirname, 'scenario-async.ts') + .unignore('client_report') + .expect({ + client_report: { + discarded_events: [ + { + category: 'error', + quantity: 1, + reason: 'event_processor', + }, + ], + }, + }) + .start() + .completed(); +}); diff --git a/dev-packages/node-integration-tests/suites/client-reports/drop-reasons/traces-sampler-throws/scenario-fallback.ts b/dev-packages/node-integration-tests/suites/client-reports/drop-reasons/traces-sampler-throws/scenario-fallback.ts new file mode 100644 index 000000000000..9d5524afe3ab --- /dev/null +++ b/dev-packages/node-integration-tests/suites/client-reports/drop-reasons/traces-sampler-throws/scenario-fallback.ts @@ -0,0 +1,19 @@ +import * as Sentry from '@sentry/node'; +import { loggingTransport } from '@sentry-internal/node-integration-tests'; + +Sentry.init({ + traceLifecycle: 'static', + dsn: 'https://public@dsn.ingest.sentry.io/1337', + transport: loggingTransport, + tracesSampleRate: 1, + tracesSampler: () => { + throw new Error('tracesSampler failed'); + }, +}); + +Sentry.startSpan({ name: 'sampled via tracesSampleRate fallback' }, () => { + // no-op +}); + +// eslint-disable-next-line @typescript-eslint/no-floating-promises +Sentry.flush(); diff --git a/dev-packages/node-integration-tests/suites/client-reports/drop-reasons/traces-sampler-throws/scenario.ts b/dev-packages/node-integration-tests/suites/client-reports/drop-reasons/traces-sampler-throws/scenario.ts new file mode 100644 index 000000000000..fac664dbde15 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/client-reports/drop-reasons/traces-sampler-throws/scenario.ts @@ -0,0 +1,17 @@ +import * as Sentry from '@sentry/node'; +import { loggingTransport } from '@sentry-internal/node-integration-tests'; + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + transport: loggingTransport, + tracesSampler: () => { + throw new Error('tracesSampler failed'); + }, +}); + +Sentry.startSpan({ name: 'this should not be sampled because tracesSampler throws' }, () => { + // no-op +}); + +// eslint-disable-next-line @typescript-eslint/no-floating-promises +Sentry.flush(); diff --git a/dev-packages/node-integration-tests/suites/client-reports/drop-reasons/traces-sampler-throws/test.ts b/dev-packages/node-integration-tests/suites/client-reports/drop-reasons/traces-sampler-throws/test.ts new file mode 100644 index 000000000000..df6cbb3195b2 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/client-reports/drop-reasons/traces-sampler-throws/test.ts @@ -0,0 +1,35 @@ +import { afterAll, test } from 'vitest'; +import { cleanupChildProcesses, createRunner } from '../../../../utils/runner'; + +afterAll(() => { + cleanupChildProcesses(); +}); + +test('records a client report and no error event when tracesSampler throws', async () => { + await createRunner(__dirname, 'scenario.ts') + .unignore('client_report') + .expect({ + client_report: { + discarded_events: [ + { + category: 'span', + quantity: 1, + reason: 'sample_rate', + }, + ], + }, + }) + .start() + .completed(); +}); + +test('sends the span when tracesSampler throws but tracesSampleRate is 1', async () => { + await createRunner(__dirname, 'scenario-fallback.ts') + .expect({ + transaction: { + transaction: 'sampled via tracesSampleRate fallback', + }, + }) + .start() + .completed(); +}); diff --git a/packages/core/src/breadcrumbs.ts b/packages/core/src/breadcrumbs.ts index d511c6f5801f..84a38fccae99 100644 --- a/packages/core/src/breadcrumbs.ts +++ b/packages/core/src/breadcrumbs.ts @@ -1,6 +1,8 @@ import { getClient, getIsolationScope } from './currentScopes'; +import { DEBUG_BUILD } from './debug-build'; import type { Breadcrumb, BreadcrumbHint } from './types/breadcrumb'; import { consoleSandbox } from './utils/debug-logger'; +import { safeCallback } from './utils/safeCallback'; import { dateTimestampInSeconds } from './utils/time'; /** @@ -28,7 +30,11 @@ export function addBreadcrumb(breadcrumb: Breadcrumb, hint?: BreadcrumbHint): vo const timestamp = dateTimestampInSeconds(); const mergedBreadcrumb = { timestamp, ...breadcrumb }; const finalBreadcrumb = beforeBreadcrumb - ? consoleSandbox(() => beforeBreadcrumb(mergedBreadcrumb, hint)) + ? safeCallback( + DEBUG_BUILD ? 'The `beforeBreadcrumb` callback threw an error, dropping the breadcrumb:' : '', + () => consoleSandbox(() => beforeBreadcrumb(mergedBreadcrumb, hint)), + () => null, + ) : mergedBreadcrumb; if (finalBreadcrumb === null) return; diff --git a/packages/core/src/client.ts b/packages/core/src/client.ts index 5a14b13c07fa..2de21ba37b41 100644 --- a/packages/core/src/client.ts +++ b/packages/core/src/client.ts @@ -50,6 +50,7 @@ import { parseSampleRate } from './utils/parseSampleRate'; import { prepareEvent } from './utils/prepareEvent'; import { makePromiseBuffer, type PromiseBuffer, SENTRY_BUFFER_FULL_ERROR } from './utils/promisebuffer'; import { safeMathRandom } from './utils/randomSafeContext'; +import { safeCallback } from './utils/safeCallback'; import { reparentChildSpans, shouldIgnoreSpan } from './utils/should-ignore-span'; import { safeUnref } from './utils/timer'; import { convertSpanJsonToTransactionEvent, convertTransactionEventToSpanJson } from './utils/transactionEvent'; @@ -1738,7 +1739,12 @@ function processBeforeSend( let processedEvent = event; if (isErrorEvent(processedEvent) && beforeSend) { - return beforeSend(processedEvent, hint); + const errorEvent = processedEvent; + return safeCallback( + DEBUG_BUILD ? 'The `beforeSend` callback threw an error, dropping the event:' : '', + () => beforeSend(errorEvent, hint), + () => null, + ); } if (isTransactionEvent(processedEvent)) { @@ -1809,7 +1815,11 @@ function processBeforeSend( spanCountBeforeProcessing: spanCountBefore, }; } - return beforeSendTransaction(processedEvent as TransactionEvent, hint); + return safeCallback( + DEBUG_BUILD ? 'The `beforeSendTransaction` callback threw an error, dropping the event:' : '', + () => beforeSendTransaction(processedEvent as TransactionEvent, hint), + () => null, + ); } } diff --git a/packages/core/src/eventProcessors.ts b/packages/core/src/eventProcessors.ts index 99a15781e06c..ef25375d7716 100644 --- a/packages/core/src/eventProcessors.ts +++ b/packages/core/src/eventProcessors.ts @@ -3,6 +3,7 @@ import type { Event, EventHint } from './types/event'; import type { EventProcessor } from './types/eventprocessor'; import { debug } from './utils/debug-logger'; import { isThenable } from './utils/is'; +import { safeCallback } from './utils/safeCallback'; import { rejectedSyncPromise, resolvedSyncPromise } from './utils/syncpromise'; /** @@ -34,9 +35,15 @@ function _notifyEventProcessors( return event; } - const result = processor({ ...event }, hint); + const processorName = `Event processor "${processor.id || '?'}"`; - DEBUG_BUILD && result === null && debug.log(`Event processor "${processor.id || '?'}" dropped event`); + const result = safeCallback( + DEBUG_BUILD ? `${processorName} threw an error, dropping event:` : '', + () => processor({ ...event }, hint), + () => null, + ); + + DEBUG_BUILD && result === null && debug.log(`${processorName} dropped event`); if (isThenable(result)) { return result.then(final => _notifyEventProcessors(final, hint, processors, index + 1)); diff --git a/packages/core/src/logs/internal.ts b/packages/core/src/logs/internal.ts index 4b610b288ca8..3ad106dfbb32 100644 --- a/packages/core/src/logs/internal.ts +++ b/packages/core/src/logs/internal.ts @@ -8,6 +8,7 @@ import type { Integration } from '../types/integration'; import type { Log, SerializedLog } from '../types/log'; import { consoleSandbox, debug } from '../utils/debug-logger'; import { isParameterizedString } from '../utils/is'; +import { safeCallback } from '../utils/safeCallback'; import { getCombinedScopeData } from '../utils/scopeData'; import { getActiveSpan } from '../utils/spanUtils'; import { timestampInSeconds } from '../utils/time'; @@ -142,8 +143,14 @@ export function _INTERNAL_captureLog( client.emit('beforeCaptureLog', processedLog); - // We need to wrap this in `consoleSandbox` to avoid recursive calls to `beforeSendLog` - const log = beforeSendLog ? consoleSandbox(() => beforeSendLog(processedLog)) : processedLog; + const log = beforeSendLog + ? safeCallback( + DEBUG_BUILD ? 'The `beforeSendLog` callback threw an error, dropping the log:' : '', + // We need to wrap this in `consoleSandbox` to avoid recursive calls to `beforeSendLog` + () => consoleSandbox(() => beforeSendLog(processedLog)), + () => null, + ) + : processedLog; if (!log) { client.recordDroppedEvent('before_send', 'log_item', 1); DEBUG_BUILD && debug.warn('beforeSendLog returned null, log will not be captured.'); diff --git a/packages/core/src/metrics/internal.ts b/packages/core/src/metrics/internal.ts index c884399624b8..621992b2ed70 100644 --- a/packages/core/src/metrics/internal.ts +++ b/packages/core/src/metrics/internal.ts @@ -8,6 +8,7 @@ import type { Integration } from '../types/integration'; import type { Metric, SerializedMetric } from '../types/metric'; import type { User } from '../types/user'; import { debug } from '../utils/debug-logger'; +import { safeCallback } from '../utils/safeCallback'; import { getCombinedScopeData } from '../utils/scopeData'; import { getActiveSpan } from '../utils/spanUtils'; import { timestampInSeconds } from '../utils/time'; @@ -181,9 +182,16 @@ export function _INTERNAL_captureMetric(beforeMetric: Metric, options?: Internal client.emit('processMetric', enrichedMetric); - const processedMetric = beforeSendMetric ? beforeSendMetric(enrichedMetric) : enrichedMetric; + const processedMetric = beforeSendMetric + ? safeCallback( + DEBUG_BUILD ? 'The `beforeSendMetric` callback threw an error, dropping the metric:' : '', + () => beforeSendMetric(enrichedMetric), + () => null, + ) + : enrichedMetric; if (!processedMetric) { + client.recordDroppedEvent('before_send', 'metric', 1); DEBUG_BUILD && debug.log('`beforeSendMetric` returned `null`, will not send metric.'); return; } diff --git a/packages/core/src/tracing/sampling.ts b/packages/core/src/tracing/sampling.ts index efc743238107..2b9e696fc3fe 100644 --- a/packages/core/src/tracing/sampling.ts +++ b/packages/core/src/tracing/sampling.ts @@ -4,6 +4,7 @@ import type { SamplingContext } from '../types/samplingcontext'; import { debug } from '../utils/debug-logger'; import { hasSpansEnabled } from '../utils/hasSpansEnabled'; import { parseSampleRate } from '../utils/parseSampleRate'; +import { safeCallback } from '../utils/safeCallback'; /** * Makes a sampling decision for the given options. @@ -21,37 +22,11 @@ export function sampleSpan( return [false]; } - let localSampleRateWasApplied = undefined; - - // we would have bailed already if neither `tracesSampler` nor `tracesSampleRate` were defined, so one of these should - // work; prefer the hook if so - let sampleRate; - if (typeof options.tracesSampler === 'function') { - sampleRate = options.tracesSampler({ - ...samplingContext, - inheritOrSampleWith: fallbackSampleRate => { - // If we have an incoming parent sample rate, we'll just use that one. - // The sampling decision will be inherited because of the sample_rand that was generated when the trace reached the incoming boundaries of the SDK. - if (typeof samplingContext.parentSampleRate === 'number') { - return samplingContext.parentSampleRate; - } - - // Fallback if parent sample rate is not on the incoming trace (e.g. if there is no baggage) - // This is to provide backwards compatibility if there are incoming traces from older SDKs that don't send a parent sample rate or a sample rand. In these cases we just want to force either a sampling decision on the downstream traces via the sample rate. - if (typeof samplingContext.parentSampled === 'boolean') { - return Number(samplingContext.parentSampled); - } - - return fallbackSampleRate; - }, - }); - localSampleRateWasApplied = true; - } else if (samplingContext.parentSampled !== undefined) { - sampleRate = samplingContext.parentSampled; - } else if (typeof options.tracesSampleRate !== 'undefined') { - sampleRate = options.tracesSampleRate; - localSampleRateWasApplied = true; + const resolved = resolveSampleRate(options, samplingContext); + if (!resolved) { + return [false]; } + const [sampleRate, localSampleRateWasApplied] = resolved; // Since this is coming from the user (or from a function provided by the user), who knows what we might get. // (The only valid values are booleans or numbers between 0 and 1.) @@ -96,3 +71,57 @@ export function sampleSpan( return [shouldSample, parsedSampleRate, localSampleRateWasApplied]; } + +/** + * Prefers `tracesSampler`. If it throws, falls back to the parent decision, then `tracesSampleRate`. + * Returns `undefined` when there is nothing to fall back to. + */ +function resolveSampleRate( + options: Pick, + samplingContext: SamplingContext, +): [sampleRate: unknown, localSampleRateWasApplied?: boolean] | undefined { + const { tracesSampler, tracesSampleRate } = options; + + if (typeof tracesSampler === 'function') { + const samplerResult = safeCallback( + DEBUG_BUILD + ? 'The `tracesSampler` callback threw an error, falling back to the parent sampling decision or `tracesSampleRate`:' + : '', + (): [unknown, boolean] => [ + tracesSampler({ + ...samplingContext, + inheritOrSampleWith: fallbackSampleRate => { + // If we have an incoming parent sample rate, we'll just use that one. + // The sampling decision will be inherited because of the sample_rand that was generated when the trace reached the incoming boundaries of the SDK. + if (typeof samplingContext.parentSampleRate === 'number') { + return samplingContext.parentSampleRate; + } + + // Fallback if parent sample rate is not on the incoming trace (e.g. if there is no baggage) + // This is to provide backwards compatibility if there are incoming traces from older SDKs that don't send a parent sample rate or a sample rand. In these cases we just want to force either a sampling decision on the downstream traces via the sample rate. + if (typeof samplingContext.parentSampled === 'boolean') { + return Number(samplingContext.parentSampled); + } + + return fallbackSampleRate; + }, + }), + true, + ], + () => undefined, + ); + if (samplerResult) { + return samplerResult; + } + } + + if (samplingContext.parentSampled !== undefined) { + return [samplingContext.parentSampled]; + } + + if (typeof tracesSampleRate !== 'undefined') { + return [tracesSampleRate, true]; + } + + return undefined; +} diff --git a/packages/core/test/lib/client.test.ts b/packages/core/test/lib/client.test.ts index e3ee176bf638..e43c35d2fe63 100644 --- a/packages/core/test/lib/client.test.ts +++ b/packages/core/test/lib/client.test.ts @@ -412,6 +412,27 @@ describe('Client', () => { expect(isolationScopeBreadcrumbs).toEqual([]); }); + test('calls `beforeBreadcrumb` and discards the breadcrumb when it throws', () => { + const exception = new Error('beforeBreadcrumb failed'); + const beforeBreadcrumb = vi.fn(() => { + throw exception; + }); + const options = getDefaultTestClientOptions({ beforeBreadcrumb }); + const client = new TestClient(options); + setCurrentClient(client); + client.init(); + const debugErrorSpy = vi.spyOn(debugLoggerModule.debug, 'error'); + + expect(() => addBreadcrumb({ message: 'hello' })).not.toThrow(); + + const isolationScopeBreadcrumbs = getIsolationScope().getScopeData().breadcrumbs; + expect(isolationScopeBreadcrumbs).toEqual([]); + expect(debugErrorSpy).toHaveBeenCalledWith( + 'The `beforeBreadcrumb` callback threw an error, dropping the breadcrumb:', + exception, + ); + }); + test('`beforeBreadcrumb` gets an access to a hint as a second argument', () => { const beforeBreadcrumb = vi.fn((breadcrumb, hint) => ({ ...breadcrumb, data: hint.data })); const options = getDefaultTestClientOptions({ beforeBreadcrumb }); @@ -2185,11 +2206,12 @@ describe('Client', () => { }); }); - test('event processor sends an event and logs when it crashes synchronously', () => { + test('drops the event and records a client report when an event processor throws synchronously', () => { const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN }); const client = new TestClient(options); const captureExceptionSpy = vi.spyOn(client, 'captureException'); - const loggerWarnSpy = vi.spyOn(debugLoggerModule.debug, 'warn'); + const recordDroppedEventSpy = vi.spyOn(client, 'recordDroppedEvent'); + const debugErrorSpy = vi.spyOn(debugLoggerModule.debug, 'error'); const scope = new Scope(); const exception = new Error('sorry 1'); scope.addEventProcessor(() => { @@ -2198,71 +2220,46 @@ describe('Client', () => { client.captureEvent({ message: 'hello' }, {}, scope); - expect(TestClient.instance!.event!.exception!.values![0]).toStrictEqual({ - type: 'Error', - value: 'sorry 1', - mechanism: { type: 'internal', handled: false }, - }); - expect(captureExceptionSpy).toBeCalledWith(exception, { - data: { - __sentry__: true, - }, - originalException: exception, - mechanism: { type: 'internal', handled: false }, - }); - expect(loggerWarnSpy).toBeCalledWith( - `Event processing pipeline threw an error, original event will not be sent. Details have been sent as a new event.\nReason: ${exception}`, - ); + expect(TestClient.instance!.event).toBeUndefined(); + expect(captureExceptionSpy).not.toHaveBeenCalled(); + expect(recordDroppedEventSpy).toHaveBeenCalledWith('event_processor', 'error'); + expect(debugErrorSpy).toHaveBeenCalledWith('Event processor "?" threw an error, dropping event:', exception); }); - test('event processor sends an event and logs when it crashes asynchronously', async () => { + test('drops the event and records a client report when an event processor rejects', async () => { vi.useFakeTimers(); const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN }); const client = new TestClient(options); const captureExceptionSpy = vi.spyOn(client, 'captureException'); - const loggerWarnSpy = vi.spyOn(debugLoggerModule.debug, 'warn'); + const recordDroppedEventSpy = vi.spyOn(client, 'recordDroppedEvent'); + const debugErrorSpy = vi.spyOn(debugLoggerModule.debug, 'error'); const scope = new Scope(); const exception = new Error('sorry 2'); - scope.addEventProcessor(() => { - return new Promise((_resolve, reject) => { - reject(exception); - }); - }); + scope.addEventProcessor(() => Promise.reject(exception)); client.captureEvent({ message: 'hello' }, {}, scope); await vi.runOnlyPendingTimersAsync(); - expect(TestClient.instance!.event!.exception!.values![0]).toStrictEqual({ - type: 'Error', - value: 'sorry 2', - mechanism: { type: 'internal', handled: false }, - }); - expect(captureExceptionSpy).toBeCalledWith(exception, { - data: { - __sentry__: true, - }, - originalException: exception, - mechanism: { type: 'internal', handled: false }, - }); - expect(loggerWarnSpy).toBeCalledWith( - `Event processing pipeline threw an error, original event will not be sent. Details have been sent as a new event.\nReason: ${exception}`, - ); + expect(TestClient.instance!.event).toBeUndefined(); + expect(captureExceptionSpy).not.toHaveBeenCalled(); + expect(recordDroppedEventSpy).toHaveBeenCalledWith('event_processor', 'error'); + expect(debugErrorSpy).toHaveBeenCalledWith('Event processor "?" threw an error, dropping event:', exception); }); - test('event processor sends an event and logs when it crashes synchronously in processor chain', () => { + test('a synchronously throwing event processor stops the processor chain', () => { const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN }); const client = new TestClient(options); const captureExceptionSpy = vi.spyOn(client, 'captureException'); + const recordDroppedEventSpy = vi.spyOn(client, 'recordDroppedEvent'); const scope = new Scope(); - const exception = new Error('sorry 3'); const processor1 = vi.fn(event => { return event; }); const processor2 = vi.fn(() => { - throw exception; + throw new Error('sorry 3'); }); const processor3 = vi.fn(event => { return event; @@ -2278,29 +2275,25 @@ describe('Client', () => { expect(processor2).toHaveBeenCalledTimes(1); expect(processor3).toHaveBeenCalledTimes(0); - expect(captureExceptionSpy).toBeCalledWith(exception, { - data: { - __sentry__: true, - }, - originalException: exception, - mechanism: { type: 'internal', handled: false }, - }); + expect(TestClient.instance!.event).toBeUndefined(); + expect(captureExceptionSpy).not.toHaveBeenCalled(); + expect(recordDroppedEventSpy).toHaveBeenCalledWith('event_processor', 'error'); }); - test('event processor sends an event and logs when it crashes asynchronously in processor chain', async () => { + test('a rejecting event processor stops the processor chain', async () => { vi.useFakeTimers(); const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN }); const client = new TestClient(options); const captureExceptionSpy = vi.spyOn(client, 'captureException'); + const recordDroppedEventSpy = vi.spyOn(client, 'recordDroppedEvent'); const scope = new Scope(); - const exception = new Error('sorry 4'); const processor1 = vi.fn(async event => { return event; }); const processor2 = vi.fn(async () => { - throw exception; + throw new Error('sorry 4'); }); const processor3 = vi.fn(event => { return event; @@ -2317,38 +2310,143 @@ describe('Client', () => { expect(processor2).toHaveBeenCalledTimes(1); expect(processor3).toHaveBeenCalledTimes(0); - expect(captureExceptionSpy).toBeCalledWith(exception, { - data: { - __sentry__: true, - }, - originalException: exception, - mechanism: { type: 'internal', handled: false }, - }); + expect(TestClient.instance!.event).toBeUndefined(); + expect(captureExceptionSpy).not.toHaveBeenCalled(); + expect(recordDroppedEventSpy).toHaveBeenCalledWith('event_processor', 'error'); }); - test('client-level event processor that throws on all events does not cause infinite recursion', () => { + test('client-level event processor that throws on all events does not capture a new event', () => { const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN }); const client = new TestClient(options); + const captureExceptionSpy = vi.spyOn(client, 'captureException'); - let processorCallCount = 0; - // Add processor at client level - this runs on ALL events including internal exceptions - client.addEventProcessor(() => { - processorCallCount++; + const processor = vi.fn(() => { throw new Error('Processor always throws'); }); + client.addEventProcessor(processor); client.captureMessage('test message'); - // Should be called once for the original message - // internal exception events skips event processors entirely. - expect(processorCallCount).toBe(1); + expect(processor).toHaveBeenCalledTimes(1); + expect(TestClient.instance!.event).toBeUndefined(); + expect(captureExceptionSpy).not.toHaveBeenCalled(); + }); + + test('drops the event and records a client report when `beforeSend` throws', () => { + const exception = new Error('beforeSend failed'); + const beforeSend = vi.fn(() => { + throw exception; + }); + const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN, beforeSend }); + const client = new TestClient(options); + const captureExceptionSpy = vi.spyOn(client, 'captureException'); + const recordDroppedEventSpy = vi.spyOn(client, 'recordDroppedEvent'); + const debugErrorSpy = vi.spyOn(debugLoggerModule.debug, 'error'); + + client.captureEvent({ message: 'hello' }); + + expect(beforeSend).toHaveBeenCalledTimes(1); + expect(TestClient.instance!.event).toBeUndefined(); + expect(captureExceptionSpy).not.toHaveBeenCalled(); + expect(recordDroppedEventSpy).toHaveBeenCalledWith('before_send', 'error'); + expect(recordDroppedEventSpy).toHaveBeenCalledTimes(1); + expect(debugErrorSpy).toHaveBeenCalledWith( + 'The `beforeSend` callback threw an error, dropping the event:', + exception, + ); + }); + + test('drops the event and records a client report when `beforeSend` rejects', async () => { + vi.useFakeTimers(); + + const exception = new Error('beforeSend failed'); + const beforeSend = vi.fn(() => Promise.reject(exception)); + const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN, beforeSend }); + const client = new TestClient(options); + const captureExceptionSpy = vi.spyOn(client, 'captureException'); + const recordDroppedEventSpy = vi.spyOn(client, 'recordDroppedEvent'); + const debugErrorSpy = vi.spyOn(debugLoggerModule.debug, 'error'); + + client.captureEvent({ message: 'hello' }); + await vi.runOnlyPendingTimersAsync(); + + expect(beforeSend).toHaveBeenCalledTimes(1); + expect(TestClient.instance!.event).toBeUndefined(); + expect(captureExceptionSpy).not.toHaveBeenCalled(); + expect(recordDroppedEventSpy).toHaveBeenCalledWith('before_send', 'error'); + expect(debugErrorSpy).toHaveBeenCalledWith( + 'The `beforeSend` callback threw an error, dropping the event:', + exception, + ); + }); + + test('drops the transaction and its spans when `beforeSendTransaction` throws', () => { + const exception = new Error('beforeSendTransaction failed'); + const beforeSendTransaction = vi.fn(() => { + throw exception; + }); + const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN, beforeSendTransaction }); + const client = new TestClient(options); + const captureExceptionSpy = vi.spyOn(client, 'captureException'); + const recordDroppedEventSpy = vi.spyOn(client, 'recordDroppedEvent'); + const debugErrorSpy = vi.spyOn(debugLoggerModule.debug, 'error'); + + client.captureEvent({ + transaction: '/dogs/are/great', + type: 'transaction', + spans: [ + { + description: 'first span', + span_id: '9e15bf99fbe4bc80', + start_timestamp: 1591603196.637835, + trace_id: '86f39e84263a4de99c326acab3bfe3bd', + data: {}, + status: 'ok', + }, + { + description: 'second span', + span_id: 'aa554c1f506b0783', + start_timestamp: 1591603196.637835, + trace_id: '86f39e84263a4de99c326acab3bfe3bd', + data: {}, + status: 'ok', + }, + ], + }); + + expect(TestClient.instance!.event).toBeUndefined(); + expect(captureExceptionSpy).not.toHaveBeenCalled(); + expect(recordDroppedEventSpy).toHaveBeenCalledWith('before_send', 'transaction'); + expect(recordDroppedEventSpy).toHaveBeenCalledWith('before_send', 'span', 3); + expect(debugErrorSpy).toHaveBeenCalledWith( + 'The `beforeSendTransaction` callback threw an error, dropping the event:', + exception, + ); + }); + + test('captures an internal event when the event processing pipeline itself throws', async () => { + vi.useFakeTimers(); + + const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN }); + const client = new TestClient(options); + const captureExceptionSpy = vi.spyOn(client, 'captureException'); + const loggerWarnSpy = vi.spyOn(debugLoggerModule.debug, 'warn'); + const exception = new Error('sdk bug'); + vi.spyOn(client as any, '_prepareEvent').mockImplementation(() => Promise.reject(exception)); - // Verify the processor error was captured and sent - expect(TestClient.instance!.event!.exception!.values![0]).toStrictEqual({ - type: 'Error', - value: 'Processor always throws', + client.captureEvent({ message: 'hello' }); + await vi.runOnlyPendingTimersAsync(); + + expect(captureExceptionSpy).toBeCalledWith(exception, { + data: { + __sentry__: true, + }, + originalException: exception, mechanism: { type: 'internal', handled: false }, }); + expect(loggerWarnSpy).toBeCalledWith( + `Event processing pipeline threw an error, original event will not be sent. Details have been sent as a new event.\nReason: ${exception}`, + ); }); test('records events dropped due to `sampleRate` option', () => { diff --git a/packages/core/test/lib/eventProcessors.test.ts b/packages/core/test/lib/eventProcessors.test.ts new file mode 100644 index 000000000000..5570788cdcaf --- /dev/null +++ b/packages/core/test/lib/eventProcessors.test.ts @@ -0,0 +1,54 @@ +import { describe, expect, it, vi } from 'vitest'; +import { notifyEventProcessors } from '../../src/eventProcessors'; +import type { EventProcessor } from '../../src/types/eventprocessor'; +import * as debugLoggerModule from '../../src/utils/debug-logger'; + +describe('notifyEventProcessors', () => { + it('passes the event through all processors', async () => { + const processors: EventProcessor[] = [ + event => ({ ...event, tags: { first: 'yes' } }), + async event => ({ ...event, tags: { ...event.tags, second: 'yes' } }), + ]; + + const result = await notifyEventProcessors(processors, { message: 'hello' }, {}); + + expect(result).toEqual({ message: 'hello', tags: { first: 'yes', second: 'yes' } }); + }); + + it('stops when a processor returns null', async () => { + const later = vi.fn(event => event); + + const result = await notifyEventProcessors([() => null, later], { message: 'hello' }, {}); + + expect(result).toBeNull(); + expect(later).not.toHaveBeenCalled(); + }); + + it('drops the event when a processor throws synchronously', async () => { + const debugErrorSpy = vi.spyOn(debugLoggerModule.debug, 'error'); + const error = new Error('boom'); + const throwing: EventProcessor = () => { + throw error; + }; + throwing.id = 'Throwing'; + const later = vi.fn(event => event); + + const result = await notifyEventProcessors([throwing, later], { message: 'hello' }, {}); + + expect(result).toBeNull(); + expect(later).not.toHaveBeenCalled(); + expect(debugErrorSpy).toHaveBeenCalledWith('Event processor "Throwing" threw an error, dropping event:', error); + }); + + it('drops the event when a processor rejects', async () => { + const debugErrorSpy = vi.spyOn(debugLoggerModule.debug, 'error'); + const error = new Error('boom'); + const later = vi.fn(event => event); + + const result = await notifyEventProcessors([() => Promise.reject(error), later], { message: 'hello' }, {}); + + expect(result).toBeNull(); + expect(later).not.toHaveBeenCalled(); + expect(debugErrorSpy).toHaveBeenCalledWith('Event processor "?" threw an error, dropping event:', error); + }); +}); diff --git a/packages/core/test/lib/logs/internal.test.ts b/packages/core/test/lib/logs/internal.test.ts index 61de9ef2a7a8..d34df4ba16e6 100644 --- a/packages/core/test/lib/logs/internal.test.ts +++ b/packages/core/test/lib/logs/internal.test.ts @@ -370,6 +370,35 @@ describe('_INTERNAL_captureLog', () => { ); }); + it('drops logs when beforeSendLog throws', () => { + const exception = new Error('beforeSendLog failed'); + const beforeSendLog = vi.fn(() => { + throw exception; + }); + const recordDroppedEventSpy = vi.spyOn(TestClient.prototype, 'recordDroppedEvent'); + const debugErrorSpy = vi.spyOn(loggerModule.debug, 'error'); + + const options = getDefaultTestClientOptions({ + dsn: PUBLIC_DSN, + beforeSendLog, + }); + const client = new TestClient(options); + const scope = new Scope(); + scope.setClient(client); + + expect(() => _INTERNAL_captureLog({ level: 'info', message: 'test message' }, scope)).not.toThrow(); + + expect(beforeSendLog).toHaveBeenCalled(); + expect(recordDroppedEventSpy).toHaveBeenCalledWith('before_send', 'log_item', 1); + expect(debugErrorSpy).toHaveBeenCalledWith( + 'The `beforeSendLog` callback threw an error, dropping the log:', + exception, + ); + expect(_INTERNAL_getLogBuffer(client)).toBeUndefined(); + + recordDroppedEventSpy.mockRestore(); + }); + it('drops logs when beforeSendLog returns null', () => { const beforeSendLog = vi.fn().mockReturnValue(null); const recordDroppedEventSpy = vi.spyOn(TestClient.prototype, 'recordDroppedEvent'); diff --git a/packages/core/test/lib/metrics/internal.test.ts b/packages/core/test/lib/metrics/internal.test.ts index 2f9b46606857..95e2a4ccea97 100644 --- a/packages/core/test/lib/metrics/internal.test.ts +++ b/packages/core/test/lib/metrics/internal.test.ts @@ -337,6 +337,7 @@ describe('_INTERNAL_captureMetric', () => { it('drops metrics when beforeSendMetric returns null', () => { const beforeSendMetric = vi.fn().mockReturnValue(null); + const recordDroppedEventSpy = vi.spyOn(TestClient.prototype, 'recordDroppedEvent'); const loggerWarnSpy = vi.spyOn(loggerModule.debug, 'log').mockImplementation(() => undefined); const options = getDefaultTestClientOptions({ @@ -357,12 +358,43 @@ describe('_INTERNAL_captureMetric', () => { ); expect(beforeSendMetric).toHaveBeenCalled(); + expect(recordDroppedEventSpy).toHaveBeenCalledWith('before_send', 'metric', 1); expect(loggerWarnSpy).toHaveBeenCalledWith('`beforeSendMetric` returned `null`, will not send metric.'); expect(_INTERNAL_getMetricBuffer(client)).toBeUndefined(); + recordDroppedEventSpy.mockRestore(); loggerWarnSpy.mockRestore(); }); + it('drops metrics when beforeSendMetric throws', () => { + const exception = new Error('beforeSendMetric failed'); + const beforeSendMetric = vi.fn(() => { + throw exception; + }); + const recordDroppedEventSpy = vi.spyOn(TestClient.prototype, 'recordDroppedEvent'); + const debugErrorSpy = vi.spyOn(loggerModule.debug, 'error'); + + const options = getDefaultTestClientOptions({ + dsn: PUBLIC_DSN, + beforeSendMetric, + }); + const client = new TestClient(options); + const scope = new Scope(); + scope.setClient(client); + + expect(() => _INTERNAL_captureMetric({ type: 'counter', name: 'test.metric', value: 1 }, { scope })).not.toThrow(); + + expect(beforeSendMetric).toHaveBeenCalled(); + expect(recordDroppedEventSpy).toHaveBeenCalledWith('before_send', 'metric', 1); + expect(debugErrorSpy).toHaveBeenCalledWith( + 'The `beforeSendMetric` callback threw an error, dropping the metric:', + exception, + ); + expect(_INTERNAL_getMetricBuffer(client)).toBeUndefined(); + + recordDroppedEventSpy.mockRestore(); + }); + it('emits afterCaptureMetric event', () => { const afterCaptureMetricSpy = vi.spyOn(TestClient.prototype, 'emit'); const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN }); diff --git a/packages/core/test/lib/tracing/sampling.test.ts b/packages/core/test/lib/tracing/sampling.test.ts new file mode 100644 index 000000000000..5caa3ea35470 --- /dev/null +++ b/packages/core/test/lib/tracing/sampling.test.ts @@ -0,0 +1,50 @@ +import { describe, expect, it, vi } from 'vitest'; +import { sampleSpan } from '../../../src/tracing/sampling'; +import * as debugLoggerModule from '../../../src/utils/debug-logger'; + +describe('sampleSpan', () => { + describe('when `tracesSampler` throws', () => { + const exception = new Error('tracesSampler failed'); + const tracesSampler = vi.fn(() => { + throw exception; + }); + const expectedMessage = + 'The `tracesSampler` callback threw an error, falling back to the parent sampling decision or `tracesSampleRate`:'; + + it('inherits the parent sampling decision', () => { + const debugErrorSpy = vi.spyOn(debugLoggerModule.debug, 'error'); + + expect(sampleSpan({ tracesSampler }, { name: 'test', attributes: {}, parentSampled: true }, 0.5)).toEqual([ + true, + 1, + undefined, + ]); + expect(sampleSpan({ tracesSampler }, { name: 'test', attributes: {}, parentSampled: false }, 0.5)).toEqual([ + false, + 0, + undefined, + ]); + expect(debugErrorSpy).toHaveBeenCalledWith(expectedMessage, exception); + }); + + it('falls back to `tracesSampleRate` without a parent decision', () => { + const debugErrorSpy = vi.spyOn(debugLoggerModule.debug, 'error'); + + expect(sampleSpan({ tracesSampler, tracesSampleRate: 0.6 }, { name: 'test', attributes: {} }, 0.5)).toEqual([ + true, + 0.6, + true, + ]); + expect(debugErrorSpy).toHaveBeenCalledWith(expectedMessage, exception); + }); + + it('does not sample when there is nothing to fall back to', () => { + const debugErrorSpy = vi.spyOn(debugLoggerModule.debug, 'error'); + const debugWarnSpy = vi.spyOn(debugLoggerModule.debug, 'warn'); + + expect(sampleSpan({ tracesSampler }, { name: 'test', attributes: {} }, 0.5)).toEqual([false]); + expect(debugErrorSpy).toHaveBeenCalledWith(expectedMessage, exception); + expect(debugWarnSpy).not.toHaveBeenCalled(); + }); + }); +}); From 1a1b6436caba597794ebb6117cfba88caf1a4cb7 Mon Sep 17 00:00:00 2001 From: Sigrid <32902192+s1gr1d@users.noreply.github.com> Date: Thu, 3 Sep 2026 12:45:08 +0200 Subject: [PATCH 07/15] fix(nextjs): Add orchestrion bundling regression tests and import.meta.url shim (#23935) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is just adding the tests of the v10 PRs to make sure we don't have a regression. Ported tests of two v10 PRs: - https://github.com/getsentry/sentry-javascript/pull/23910 - https://github.com/getsentry/sentry-javascript/pull/23906 One test caught a real problem: the `@sentry/server-utils` CJS build still replaced `import.meta.url` with a snippet that assumes "a `document` global means a browser", crashing under jsdom. ``` "AssertionError: expected [Function] to not throw an error but 'TypeError [ERR_INVALID_URL_SCHEME]: T…' was thrown" ``` The v10 lazy-loading fix needs no porting: on v11, `withSentryConfig` lives in the separate `@sentry/nextjs/config` export, so importing the SDK never reaches the bundler plugins. Fixes https://github.com/getsentry/sentry-javascript/issues/23789 --- .../tests/worker-bundle.test.ts | 70 +++++++++++++++++++ .../test/serverEntryBundlerGraph.test.ts | 32 +++++++++ packages/server-utils/rollup.npm.config.mjs | 15 +++- .../orchestrion/bundlerBuildOutput.test.ts | 38 ++++++++++ 4 files changed, 154 insertions(+), 1 deletion(-) create mode 100644 dev-packages/e2e-tests/test-applications/nextjs-16-cf-workers/tests/worker-bundle.test.ts create mode 100644 packages/nextjs/test/serverEntryBundlerGraph.test.ts create mode 100644 packages/server-utils/test/orchestrion/bundlerBuildOutput.test.ts diff --git a/dev-packages/e2e-tests/test-applications/nextjs-16-cf-workers/tests/worker-bundle.test.ts b/dev-packages/e2e-tests/test-applications/nextjs-16-cf-workers/tests/worker-bundle.test.ts new file mode 100644 index 000000000000..dbd927e2053b --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/nextjs-16-cf-workers/tests/worker-bundle.test.ts @@ -0,0 +1,70 @@ +import { expect, test } from '@playwright/test'; +import * as fs from 'fs'; +import { createRequire } from 'module'; +import * as path from 'path'; +import { isDevMode } from './isDevMode'; + +/** + * The orchestrion bundler plugins are build-time-only, and their module-scope side effects break + * on Workers (an unawaited `WebAssembly.compile()` crashed every cold start, issue #22794). The + * worker bundle OpenNext produces must therefore never contain them: importing `@sentry/nextjs` + * on the server has to keep the plugin graph out of the deployed artifact. + */ +test('worker bundle does not contain the orchestrion bundler plugins', () => { + test.skip(isDevMode, 'requires the production worker build'); + + const openNextDir = path.resolve(__dirname, '..', '.open-next'); + expect(fs.existsSync(path.join(openNextDir, 'worker.js'))).toBe(true); + + // `assets` holds the static client files; everything else is code the worker can run. + const serverFiles = collectJsFiles(openNextDir).filter( + filePath => !filePath.startsWith(path.join(openNextDir, 'assets')), + ); + expect(serverFiles.length).toBeGreaterThan(0); + + const markers = ['code-transformer-bundler-plugins', '__codeTransformerWebpackDiagnostics']; + + // The markers must still exist in the installed plugin build. + // If upstream renames them, this fails instead of letting the leak check below pass. + const pluginGraphSources = readOrchestrionPluginGraphSources(); + for (const marker of markers) { + expect( + pluginGraphSources.some(source => source.includes(marker)), + `marker "${marker}" is gone from the @sentry/server-utils plugin build — update the markers`, + ).toBe(true); + } + + const leaks = serverFiles.filter(filePath => { + const content = fs.readFileSync(filePath, 'utf8'); + return markers.some(marker => content.includes(marker)); + }); + + expect(leaks.map(filePath => path.relative(openNextDir, filePath))).toEqual([]); +}); + +/** + * Reads the source of the installed `@sentry/server-utils` webpack plugin entry plus the files it + * requires relatively — the graph a leak would drag into the worker bundle. `createRequire` takes + * the `require` export condition, so this resolves the CJS build, whose `require('./…')` calls the + * regex below picks up. + */ +function readOrchestrionPluginGraphSources(): string[] { + const pluginEntry = createRequire(__filename).resolve('@sentry/server-utils/orchestrion/webpack'); + const entrySource = fs.readFileSync(pluginEntry, 'utf8'); + return [ + entrySource, + ...[...entrySource.matchAll(/require\('(\.\.?\/[^']+)'\)/g)].map(([, specifier]) => + fs.readFileSync(path.resolve(path.dirname(pluginEntry), specifier), 'utf8'), + ), + ]; +} + +function collectJsFiles(dir: string): string[] { + return fs.readdirSync(dir, { withFileTypes: true }).flatMap(entry => { + const fullPath = path.join(dir, entry.name); + if (entry.isDirectory()) { + return collectJsFiles(fullPath); + } + return /\.(js|mjs|cjs)$/.test(entry.name) ? [fullPath] : []; + }); +} diff --git a/packages/nextjs/test/serverEntryBundlerGraph.test.ts b/packages/nextjs/test/serverEntryBundlerGraph.test.ts new file mode 100644 index 000000000000..17b5447a66fc --- /dev/null +++ b/packages/nextjs/test/serverEntryBundlerGraph.test.ts @@ -0,0 +1,32 @@ +import { spawnSync } from 'node:child_process'; +import { resolve } from 'node:path'; +import { describe, expect, it } from 'vitest'; + +/** + * Importing the SDK server entry must not load the orchestrion bundler plugins. They are + * build-time-only, and their module-scope side effects break runtimes the build never sees, + * like jsdom/happy-dom test runs (issue #23789) and Cloudflare Workers cold starts (issue #22794). + * Runs in a child process for a clean module cache and real Node resolution. + */ +describe('built CJS server entry', () => { + const serverEntry = resolve(__dirname, '../build/cjs/index.server.js'); + + it('loads under a DOM test environment without pulling in the orchestrion bundler graph', () => { + const script = ` + globalThis.document = { baseURI: 'http://localhost:3000/' }; + require(${JSON.stringify(serverEntry)}); + const toPosix = modulePath => modulePath.split(require('path').sep).join('/'); + const bundlerModules = Object.keys(require.cache).map(toPosix).filter( + modulePath => modulePath.includes('code-transformer-bundler-plugins') || modulePath.includes('orchestrion/bundler'), + ); + if (bundlerModules.length > 0) { + console.error('Bundler-plugin modules loaded at import time:\\n' + bundlerModules.join('\\n')); + process.exit(1); + } + `; + + // On failure, stderr carries either the leaked module list or the import crash itself. + const result = spawnSync(process.execPath, ['-e', script], { encoding: 'utf8' }); + expect(result.status, result.stderr).toBe(0); + }); +}); diff --git a/packages/server-utils/rollup.npm.config.mjs b/packages/server-utils/rollup.npm.config.mjs index 675a81424abe..c313a9f1cc4c 100644 --- a/packages/server-utils/rollup.npm.config.mjs +++ b/packages/server-utils/rollup.npm.config.mjs @@ -48,6 +48,19 @@ const debugNodeAlias = { }, }; +// This package only runs in Node, but rollup's default CJS replacement for `import.meta.url` +// picks browser behavior whenever a `document` global exists, and jsdom/happy-dom define +// `document` while tests run in Node. Always emit the unconditional Node form instead. +const importMetaUrlNodeShim = { + name: 'import-meta-url-node-shim', + resolveImportMeta(property, { format }) { + if (property === 'url' && format === 'cjs') { + return "require('node:url').pathToFileURL(__filename).href"; + } + return null; + }, +}; + // Bundling files from the repo-root `node_modules` moves rollup's common source ancestor up to the // repo root, so `preserveModules` names our own files `packages/server-utils/src/...` — strip that // prefix to keep the `build/cjs/index.js` layout the `exports` map points at. And npm never packs @@ -94,7 +107,7 @@ export default [ 'src/orchestrion/bundler/bun.ts', ], packageSpecificConfig: { - plugins: [debugNodeAlias, commonJSPlugin, thirdPartyLicensePlugin], + plugins: [debugNodeAlias, commonJSPlugin, importMetaUrlNodeShim, thirdPartyLicensePlugin], output: { // set exports to 'named' or 'auto' so that rollup doesn't warn exports: 'named', diff --git a/packages/server-utils/test/orchestrion/bundlerBuildOutput.test.ts b/packages/server-utils/test/orchestrion/bundlerBuildOutput.test.ts new file mode 100644 index 000000000000..04258099b948 --- /dev/null +++ b/packages/server-utils/test/orchestrion/bundlerBuildOutput.test.ts @@ -0,0 +1,38 @@ +import { createRequire } from 'node:module'; +import { resolve } from 'node:path'; +import { afterEach, describe, expect, it } from 'vitest'; + +const nodeRequire = createRequire(import.meta.url); +const BUILD_CJS_DIR = resolve(__dirname, '../../build/cjs'); + +// The five entries share vendored chunks, and the require cache would keep a chunk's module scope +// from running again after the first test. Drop everything under `build/cjs` first, so each test +// really executes the code it claims to. +function requireFresh(entry: string): unknown { + for (const key of Object.keys(nodeRequire.cache)) { + if (key.startsWith(BUILD_CJS_DIR)) { + Reflect.deleteProperty(nodeRequire.cache, key); + } + } + return nodeRequire(resolve(BUILD_CJS_DIR, 'orchestrion/bundler', `${entry}.js`)); +} + +/** + * The bundler entries must load in Node even when a `document` global exists, which is the case + * under jsdom/happy-dom: the vendored code must never treat `document` as proof of a browser. + * Runs against `build/cjs` because that guard lives in the emitted code, not the sources. + * Reference Issue: https://github.com/getsentry/sentry-javascript/issues/23789 + */ +describe('built CJS bundler entries load under DOM test environments', () => { + afterEach(() => { + delete (globalThis as { document?: unknown }).document; + }); + + it.each(['webpack', 'webpack-loader', 'esbuild', 'vite', 'rollup'])( + 'build/cjs/orchestrion/bundler/%s.js loads while a `document` global is defined', + entry => { + (globalThis as { document?: unknown }).document = { baseURI: 'http://localhost:3000/' }; + expect(() => requireFresh(entry)).not.toThrow(); + }, + ); +}); From 27009976d917d9dd1a1c71413d1cdec23f15ab8d Mon Sep 17 00:00:00 2001 From: Charly Gomez Date: Thu, 3 Sep 2026 13:48:00 +0200 Subject: [PATCH 08/15] feat(remix)!: Move the Vite plugin to `@sentry/remix/vite` (#23989) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - `sentryRemixVitePlugin` moves from the main `@sentry/remix` entry to a dedicated `@sentry/remix/vite` subpath, matching `@sentry/sveltekit` and `@sentry/react-router`. - It was exported from `index.server.ts`, the package's CJS `main`, so anything the plugin imports lands in the module graph of every Remix server process. Splitting the entry keeps build-time-only dependencies off the runtime path. - It now returns an array so further plugins can be added without another breaking change — #23988 stacks on this to auto-wire the orchestrion transform. Refs #23986 Co-authored-by: Claude Opus 5 (1M context) --- MIGRATION.md | 12 +++++ .../vite.config.ts | 2 +- .../create-remix-app-express/vite.config.ts | 2 +- .../create-remix-app-v2/vite.config.ts | 2 +- .../remix-hydrogen/vite.config.ts | 2 +- .../remix-server-timing/vite.config.ts | 2 +- packages/remix/package.json | 5 ++ packages/remix/rollup.npm.config.mjs | 1 + packages/remix/src/index.server.ts | 1 - packages/remix/src/vite/index.ts | 32 ++++++++++++ .../vite.ts => vite/routeManifestPlugin.ts} | 37 ++----------- packages/remix/src/vite/types.ts | 11 ++++ .../routeManifestPlugin.test.ts} | 52 +++++++++---------- 13 files changed, 95 insertions(+), 66 deletions(-) create mode 100644 packages/remix/src/vite/index.ts rename packages/remix/src/{config/vite.ts => vite/routeManifestPlugin.ts} (83%) create mode 100644 packages/remix/src/vite/types.ts rename packages/remix/test/{config/vite.test.ts => vite/routeManifestPlugin.test.ts} (92%) diff --git a/MIGRATION.md b/MIGRATION.md index 567e076eeb9a..fa62f96a2033 100644 --- a/MIGRATION.md +++ b/MIGRATION.md @@ -1260,6 +1260,18 @@ Affected SDKs: `@sentry/react-router`. + import { sentryOnBuildEnd } from '@sentry/react-router/vite'; ``` +### Remix: Vite plugin moved to `@sentry/remix/vite` + +Affected SDKs: `@sentry/remix`. + +`sentryRemixVitePlugin` is no longer available from the main `@sentry/remix` entry point. Import it from the dedicated subpath instead: + +```diff +// vite.config.ts +- import { sentryRemixVitePlugin } from '@sentry/remix'; ++ import { sentryRemixVitePlugin } from '@sentry/remix/vite'; +``` + ## 3. Removed APIs ### `@sentry/core` / All SDKs diff --git a/dev-packages/e2e-tests/test-applications/create-remix-app-express-vite-dev/vite.config.ts b/dev-packages/e2e-tests/test-applications/create-remix-app-express-vite-dev/vite.config.ts index 6ebb8eacc6a5..44f475c6714e 100644 --- a/dev-packages/e2e-tests/test-applications/create-remix-app-express-vite-dev/vite.config.ts +++ b/dev-packages/e2e-tests/test-applications/create-remix-app-express-vite-dev/vite.config.ts @@ -1,6 +1,6 @@ import { installGlobals } from '@remix-run/node'; import { vitePlugin as remix } from '@remix-run/dev'; -import { sentryRemixVitePlugin } from '@sentry/remix'; +import { sentryRemixVitePlugin } from '@sentry/remix/vite'; import { defineConfig } from 'vite'; import tsconfigPaths from 'vite-tsconfig-paths'; diff --git a/dev-packages/e2e-tests/test-applications/create-remix-app-express/vite.config.ts b/dev-packages/e2e-tests/test-applications/create-remix-app-express/vite.config.ts index 6ebb8eacc6a5..44f475c6714e 100644 --- a/dev-packages/e2e-tests/test-applications/create-remix-app-express/vite.config.ts +++ b/dev-packages/e2e-tests/test-applications/create-remix-app-express/vite.config.ts @@ -1,6 +1,6 @@ import { installGlobals } from '@remix-run/node'; import { vitePlugin as remix } from '@remix-run/dev'; -import { sentryRemixVitePlugin } from '@sentry/remix'; +import { sentryRemixVitePlugin } from '@sentry/remix/vite'; import { defineConfig } from 'vite'; import tsconfigPaths from 'vite-tsconfig-paths'; diff --git a/dev-packages/e2e-tests/test-applications/create-remix-app-v2/vite.config.ts b/dev-packages/e2e-tests/test-applications/create-remix-app-v2/vite.config.ts index 42372f108ba8..381cc9d13ecc 100644 --- a/dev-packages/e2e-tests/test-applications/create-remix-app-v2/vite.config.ts +++ b/dev-packages/e2e-tests/test-applications/create-remix-app-v2/vite.config.ts @@ -1,5 +1,5 @@ import { vitePlugin as remix } from '@remix-run/dev'; -import { sentryRemixVitePlugin } from '@sentry/remix'; +import { sentryRemixVitePlugin } from '@sentry/remix/vite'; import { sentryOrchestrionPlugin } from '@sentry/server-utils/orchestrion/vite'; import { defineConfig } from 'vite'; import tsconfigPaths from 'vite-tsconfig-paths'; diff --git a/dev-packages/e2e-tests/test-applications/remix-hydrogen/vite.config.ts b/dev-packages/e2e-tests/test-applications/remix-hydrogen/vite.config.ts index b128a147ff52..46dfc4036b9c 100644 --- a/dev-packages/e2e-tests/test-applications/remix-hydrogen/vite.config.ts +++ b/dev-packages/e2e-tests/test-applications/remix-hydrogen/vite.config.ts @@ -1,5 +1,5 @@ import { vitePlugin as remix } from '@remix-run/dev'; -import { sentryRemixVitePlugin } from '@sentry/remix'; +import { sentryRemixVitePlugin } from '@sentry/remix/vite'; import { hydrogen } from '@shopify/hydrogen/vite'; import { oxygen } from '@shopify/mini-oxygen/vite'; import { defineConfig } from 'vite'; diff --git a/dev-packages/e2e-tests/test-applications/remix-server-timing/vite.config.ts b/dev-packages/e2e-tests/test-applications/remix-server-timing/vite.config.ts index d4d7f23895c1..bd4060f4a14b 100644 --- a/dev-packages/e2e-tests/test-applications/remix-server-timing/vite.config.ts +++ b/dev-packages/e2e-tests/test-applications/remix-server-timing/vite.config.ts @@ -1,5 +1,5 @@ import { vitePlugin as remix } from '@remix-run/dev'; -import { sentryRemixVitePlugin } from '@sentry/remix'; +import { sentryRemixVitePlugin } from '@sentry/remix/vite'; import { defineConfig } from 'vite'; import tsconfigPaths from 'vite-tsconfig-paths'; diff --git a/packages/remix/package.json b/packages/remix/package.json index 301894a7deaa..56910a4548d6 100644 --- a/packages/remix/package.json +++ b/packages/remix/package.json @@ -42,6 +42,11 @@ "types": "./build/types/cloudflare/index.d.ts", "default": "./build/esm/cloudflare/index.js" }, + "./vite": { + "types": "./build/types/vite/index.d.ts", + "import": "./build/esm/vite/index.js", + "require": "./build/cjs/vite/index.js" + }, "./import": { "import": { "default": "./build/import-hook.mjs" diff --git a/packages/remix/rollup.npm.config.mjs b/packages/remix/rollup.npm.config.mjs index 380ec3c1bd7e..9bf3f04f3ee3 100644 --- a/packages/remix/rollup.npm.config.mjs +++ b/packages/remix/rollup.npm.config.mjs @@ -13,6 +13,7 @@ export default [ 'src/client/index.ts', 'src/server/index.ts', 'src/cloudflare/index.ts', + 'src/vite/index.ts', ], packageSpecificConfig: { external: ['react-router', 'react-router-dom', 'react', 'react/jsx-runtime'], diff --git a/packages/remix/src/index.server.ts b/packages/remix/src/index.server.ts index e4eb8ad6236e..e40dd4978bb3 100644 --- a/packages/remix/src/index.server.ts +++ b/packages/remix/src/index.server.ts @@ -1,7 +1,6 @@ export * from './server'; export { captureRemixErrorBoundaryError, withSentry, ErrorBoundary, browserTracingIntegration } from './client'; -export { sentryRemixVitePlugin } from './config/vite'; export { createRemixRouteManifest } from './config/createRemixRouteManifest'; export type { CreateRemixRouteManifestOptions } from './config/createRemixRouteManifest'; diff --git a/packages/remix/src/vite/index.ts b/packages/remix/src/vite/index.ts new file mode 100644 index 000000000000..001165f380ab --- /dev/null +++ b/packages/remix/src/vite/index.ts @@ -0,0 +1,32 @@ +import type { Plugin } from 'vite'; +import { makeRouteManifestPlugin } from './routeManifestPlugin'; +import type { SentryRemixVitePluginOptions } from './types'; + +export type { SentryRemixVitePluginOptions }; + +/** + * Sentry Vite plugins for Remix. + * + * Add these to your Vite configuration to inject the Remix route manifest, so client-side + * transactions are parameterized. + * + * @example + * ```typescript + * // vite.config.ts + * import { vitePlugin as remix } from '@remix-run/dev'; + * import { sentryRemixVitePlugin } from '@sentry/remix/vite'; + * import { defineConfig } from 'vite'; + * + * export default defineConfig({ + * plugins: [ + * remix(), + * sentryRemixVitePlugin({ + * appDirPath: './app', + * }), + * ], + * }); + * ``` + */ +export function sentryRemixVitePlugin(options: SentryRemixVitePluginOptions = {}): Plugin[] { + return [makeRouteManifestPlugin(options)]; +} diff --git a/packages/remix/src/config/vite.ts b/packages/remix/src/vite/routeManifestPlugin.ts similarity index 83% rename from packages/remix/src/config/vite.ts rename to packages/remix/src/vite/routeManifestPlugin.ts index 83b033104530..8328af5de060 100644 --- a/packages/remix/src/config/vite.ts +++ b/packages/remix/src/vite/routeManifestPlugin.ts @@ -1,6 +1,7 @@ import * as path from 'path'; import type { Plugin } from 'vite'; -import { createRemixRouteManifest } from './createRemixRouteManifest'; +import { createRemixRouteManifest } from '../config/createRemixRouteManifest'; +import type { SentryRemixVitePluginOptions } from './types'; /** * Escapes a JSON string for safe embedding in HTML script tags. @@ -12,45 +13,13 @@ function escapeJsonForHtml(jsonString: string): string { .replace(/