From cb9a9b555b0c0f051dce50a103bbbc99b79081d8 Mon Sep 17 00:00:00 2001 From: lazerg Date: Thu, 29 Oct 2026 11:53:45 +0500 Subject: [PATCH 1/2] fix(server-utils): Only create Redis db spans when there is a parent span Co-Authored-By: Claude Opus 5.5 --- .../suites/tracing/ioredis-dc/test.ts | 10 +-- .../suites/tracing/redis-cache/test.ts | 12 +--- .../tracing/redis-dc/scenario-no-parent.mjs | 21 ++++++ .../suites/tracing/redis-dc/test.ts | 44 ++++++++++-- .../src/integrations/redis/index.ts | 70 +++++++++++-------- .../integrations/redis/redis-dc-subscriber.ts | 67 ++++++++++-------- 6 files changed, 141 insertions(+), 83 deletions(-) create mode 100644 dev-packages/node-integration-tests/suites/tracing/redis-dc/scenario-no-parent.mjs diff --git a/dev-packages/node-integration-tests/suites/tracing/ioredis-dc/test.ts b/dev-packages/node-integration-tests/suites/tracing/ioredis-dc/test.ts index fe1cc5bb7b09..739787de72da 100644 --- a/dev-packages/node-integration-tests/suites/tracing/ioredis-dc/test.ts +++ b/dev-packages/node-integration-tests/suites/tracing/ioredis-dc/test.ts @@ -101,16 +101,8 @@ describeWithDockerCompose( await createTestRunner() .expect({ span: container => { - // The connect span opens its own segment but shares the trace with the test span, - // so both segments arrive in the same container. - expect(container.items.filter(item => item.is_segment).map(item => item.name)).toEqual([ - 'redis-connect', - SEGMENT_NAME, - ]); + expect(container.items.filter(item => item.is_segment).map(item => item.name)).toEqual([SEGMENT_NAME]); - // ioredis' own handshake commands (`client SETINFO`, `info`) are emitted on the - // channel too, but belong to the connect segment — the test span's children are the - // commands the scenario issues. const spans = container.items.filter( item => !item.is_segment && item.attributes[SENTRY_SEGMENT_NAME]?.value === SEGMENT_NAME, ); diff --git a/dev-packages/node-integration-tests/suites/tracing/redis-cache/test.ts b/dev-packages/node-integration-tests/suites/tracing/redis-cache/test.ts index a6f3271d2356..e51e00265799 100644 --- a/dev-packages/node-integration-tests/suites/tracing/redis-cache/test.ts +++ b/dev-packages/node-integration-tests/suites/tracing/redis-cache/test.ts @@ -323,12 +323,7 @@ describeWithDockerCompose('redis cache auto instrumentation', { workingDirectory await createTestRunner() .expect({ span: container => { - // The connect span opens its own segment, but shares the trace with the test span, - // so both segments arrive in the same container. - expect(container.items.filter(item => item.is_segment).map(item => item.name)).toEqual([ - 'redis-connect', - segmentName, - ]); + expect(container.items.filter(item => item.is_segment).map(item => item.name)).toEqual([segmentName]); expect(childSpans(container)).toEqual([ span('SET localhost:6383', redisSpanOp, { @@ -427,10 +422,7 @@ describeWithDockerCompose('redis cache auto instrumentation', { workingDirectory await createTestRunner() .expect({ span: container => { - expect(container.items.filter(item => item.is_segment).map(item => item.name)).toEqual([ - 'redis-connect', - segmentName, - ]); + expect(container.items.filter(item => item.is_segment).map(item => item.name)).toEqual([segmentName]); expect(childSpans(container)).toEqual([ span('SET localhost:6383', redisSpanOp, { diff --git a/dev-packages/node-integration-tests/suites/tracing/redis-dc/scenario-no-parent.mjs b/dev-packages/node-integration-tests/suites/tracing/redis-dc/scenario-no-parent.mjs new file mode 100644 index 000000000000..572022c9310e --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/redis-dc/scenario-no-parent.mjs @@ -0,0 +1,21 @@ +import * as Sentry from '@sentry/node'; + +async function run() { + const { createClient } = await import('redis-5-tracing'); + const redisClient = await createClient({ socket: { host: '127.0.0.1', port: 6381 } }).connect(); + + await redisClient.set('dc-no-parent:before', 'test-value'); + + const parent = Sentry.startInactiveSpan({ name: 'parent operation' }); + await Sentry.withActiveSpan(parent, () => redisClient.set('dc-no-parent:inside', 'test-value')); + + await redisClient.set('dc-no-parent:after', 'test-value'); + await redisClient.disconnect(); + + // Flush every command before the parent signals that span collection is complete. + await Sentry.flush(); + parent.end(); + await Sentry.flush(); +} + +run(); 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 a9f10789dc48..fb3b37bc542e 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 @@ -1,3 +1,4 @@ +import type { SerializedStreamedSpan } from '@sentry/core'; import { CACHE_HIT, CACHE_ITEM_SIZE, @@ -126,12 +127,7 @@ describeWithDockerCompose( await createTestRunner() .expect({ span: container => { - // The connect span opens its own segment but shares the trace with the test span, - // so both segments arrive in the same container. - expect(container.items.filter(item => item.is_segment).map(item => item.name)).toEqual([ - 'redis-connect', - SEGMENT_NAME, - ]); + expect(container.items.filter(item => item.is_segment).map(item => item.name)).toEqual([SEGMENT_NAME]); const spans = container.items.filter( item => !item.is_segment && item.attributes[SENTRY_SEGMENT_NAME]?.value === SEGMENT_NAME, @@ -204,5 +200,41 @@ describeWithDockerCompose( .completed(); }); }); + + createEsmAndCjsTests(__dirname, 'scenario-no-parent.mjs', 'instrument.mjs', (createTestRunner, test) => { + test('only instruments commands with an active parent span', { timeout: 60_000 }, async () => { + const runner = createTestRunner(); + const dbSpans = new Map(); + const spansPromise = runner.collectStreamedSpans(spans => { + for (const span of spans) { + if (span.attributes[SENTRY_ORIGIN]?.value === ORIGIN) { + dbSpans.set(span.span_id, span); + } + } + return spans.some(span => span.is_segment && span.name === 'parent operation'); + }); + + await runner.unordered().start().completed(); + + const spans = await spansPromise; + const parent = spans.find(span => span.is_segment && span.name === 'parent operation'); + expect(parent).toBeDefined(); + expect( + Array.from(dbSpans.values(), span => ({ + query: span.attributes[DB_QUERY_TEXT]?.value, + parentSpanId: span.parent_span_id, + traceId: span.trace_id, + isSegment: span.is_segment, + })), + ).toEqual([ + { + query: 'SET dc-no-parent:inside ?', + parentSpanId: parent?.span_id, + traceId: parent?.trace_id, + isSegment: false, + }, + ]); + }); + }); }, ); diff --git a/packages/server-utils/src/integrations/redis/index.ts b/packages/server-utils/src/integrations/redis/index.ts index 7176a14b57ec..fd787da25c0a 100644 --- a/packages/server-utils/src/integrations/redis/index.ts +++ b/packages/server-utils/src/integrations/redis/index.ts @@ -141,6 +141,10 @@ function subscribeLegacyRedisCommand(cacheOptions: RedisCacheOptions): void { asyncStart: noop, asyncEnd: noop, start(data) { + const parentSpan = getActiveSpan(); + if (!parentSpan) { + return; + } const command = data.arguments?.[0] as LegacyRedisCommand | undefined; if (!command || typeof command !== 'object') { return; @@ -166,7 +170,6 @@ function subscribeLegacyRedisCommand(cacheOptions: RedisCacheOptions): void { const span = startCommandSpan(command.command, command.args ?? [], attributes, cacheOptions); (data as OrchestrionChannelContext & { _sentrySpan?: Span })._sentrySpan = span; - const parentSpan = getActiveSpan(); command.callback = function (this: unknown, err: Error | null | undefined, reply: unknown) { if (!err) { applyCacheResponseAttributes(span, reply); @@ -174,7 +177,7 @@ function subscribeLegacyRedisCommand(cacheOptions: RedisCacheOptions): void { endSpan(span, err); // eslint-disable-next-line prefer-rest-params const args = arguments as unknown as [Error | null | undefined, unknown]; - return withActiveSpan(parentSpan ?? null, () => originalCallback.apply(this, args)); + return withActiveSpan(parentSpan, () => originalCallback.apply(this, args)); }; }, error(data) { @@ -207,6 +210,7 @@ function bindNodeRedisCommandChannel( return startCommandSpan(commandName, wireArgs.slice(1), nodeRedisAttributes(options), cacheOptions); }, { + requiresParentSpan: true, beforeSpanEnd(span, data) { if (!('error' in data)) { applyCacheResponseAttributes(span, data.result); @@ -241,17 +245,21 @@ function bindNodeRedisConnectChannel(): void { const channel = diagnosticsChannel.tracingChannel( CHANNELS.NODE_REDIS_CONNECT, ); - bindTracingChannelToSpan(channel, data => { - const options = (data.self as NodeRedisClient | undefined)?.options; - return startInactiveSpan({ - name: 'redis-connect', - attributes: { - [SENTRY_KIND]: 'client', - ...nodeRedisAttributes(options), - [SENTRY_OP]: DB, - }, - }); - }); + bindTracingChannelToSpan( + channel, + data => { + const options = (data.self as NodeRedisClient | undefined)?.options; + return startInactiveSpan({ + name: 'redis-connect', + attributes: { + [SENTRY_KIND]: 'client', + ...nodeRedisAttributes(options), + [SENTRY_OP]: DB, + }, + }); + }, + { requiresParentSpan: true }, + ); } // Batch (multi/pipeline): one span per `exec`. Batched commands bypass `sendCommand`, so @@ -262,22 +270,26 @@ function bindNodeRedisBatchChannel( getOperation: (data: OrchestrionChannelContext) => string, ): void { const channel = diagnosticsChannel.tracingChannel(channelName); - bindTracingChannelToSpan(channel, data => { - const commands = data.arguments?.[0]; - const size = Array.isArray(commands) ? commands.length : undefined; - const options = (data.self as NodeRedisClient | undefined)?.options; - const operation = getOperation(data); - return startInactiveSpan({ - name: operation, - attributes: { - [SENTRY_KIND]: 'client', - ...nodeRedisAttributes(options), - [SENTRY_OP]: DB_QUERY, - [DB_OPERATION_NAME]: operation, - ...(size && size > 1 ? { [DB_OPERATION_BATCH_SIZE]: size } : {}), - }, - }); - }); + bindTracingChannelToSpan( + channel, + data => { + const commands = data.arguments?.[0]; + const size = Array.isArray(commands) ? commands.length : undefined; + const options = (data.self as NodeRedisClient | undefined)?.options; + const operation = getOperation(data); + return startInactiveSpan({ + name: operation, + attributes: { + [SENTRY_KIND]: 'client', + ...nodeRedisAttributes(options), + [SENTRY_OP]: DB_QUERY, + [DB_OPERATION_NAME]: operation, + ...(size && size > 1 ? { [DB_OPERATION_BATCH_SIZE]: size } : {}), + }, + }); + }, + { requiresParentSpan: true }, + ); } const _redisIntegration = ((options: RedisIntegrationOptions = {}) => { 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 5db9dc2e5458..cf08da2abc54 100644 --- a/packages/server-utils/src/integrations/redis/redis-dc-subscriber.ts +++ b/packages/server-utils/src/integrations/redis/redis-dc-subscriber.ts @@ -165,6 +165,7 @@ function setupCommandChannel( }); }, { + requiresParentSpan: true, beforeSpanEnd(span, data) { if ('error' in data) return; applyCacheResponseAttributes(span, data.result); @@ -178,36 +179,44 @@ function setupBatchChannel( channelName: string, getOperationName: (data: RedisBatchData) => string, ): void { - bindTracingChannelToSpan(tracingChannel(channelName), data => { - const operation = getOperationName(data); - return startInactiveSpan({ - name: operation, - attributes: { - [SENTRY_ORIGIN]: ORIGIN, - [SENTRY_OP]: DB_QUERY, - [DB_SYSTEM_NAME]: DB_SYSTEM_NAME_VALUE_REDIS, - [DB_OPERATION_NAME]: operation, - // should only include batch size greater than 1, - // or else it isn't properly considered a "batch" - ...(Number(data.batchSize) > 1 ? { [DB_OPERATION_BATCH_SIZE]: data.batchSize } : {}), - ...(data.serverAddress != null ? { [SERVER_ADDRESS]: data.serverAddress } : {}), - ...(data.serverPort != null ? { [SERVER_PORT]: data.serverPort } : {}), - }, - }); - }); + bindTracingChannelToSpan( + tracingChannel(channelName), + data => { + const operation = getOperationName(data); + return startInactiveSpan({ + name: operation, + attributes: { + [SENTRY_ORIGIN]: ORIGIN, + [SENTRY_OP]: DB_QUERY, + [DB_SYSTEM_NAME]: DB_SYSTEM_NAME_VALUE_REDIS, + [DB_OPERATION_NAME]: operation, + // should only include batch size greater than 1, + // or else it isn't properly considered a "batch" + ...(Number(data.batchSize) > 1 ? { [DB_OPERATION_BATCH_SIZE]: data.batchSize } : {}), + ...(data.serverAddress != null ? { [SERVER_ADDRESS]: data.serverAddress } : {}), + ...(data.serverPort != null ? { [SERVER_PORT]: data.serverPort } : {}), + }, + }); + }, + { requiresParentSpan: true }, + ); } function setupConnectChannel(tracingChannel: RedisTracingChannelFactory, channelName: string): void { - bindTracingChannelToSpan(tracingChannel(channelName), data => { - return startInactiveSpan({ - name: 'redis-connect', - attributes: { - [SENTRY_ORIGIN]: ORIGIN, - [SENTRY_OP]: DB, - [DB_SYSTEM_NAME]: DB_SYSTEM_NAME_VALUE_REDIS, - ...(data.serverAddress != null ? { [SERVER_ADDRESS]: data.serverAddress } : {}), - ...(data.serverPort != null ? { [SERVER_PORT]: data.serverPort } : {}), - }, - }); - }); + bindTracingChannelToSpan( + tracingChannel(channelName), + data => { + return startInactiveSpan({ + name: 'redis-connect', + attributes: { + [SENTRY_ORIGIN]: ORIGIN, + [SENTRY_OP]: DB, + [DB_SYSTEM_NAME]: DB_SYSTEM_NAME_VALUE_REDIS, + ...(data.serverAddress != null ? { [SERVER_ADDRESS]: data.serverAddress } : {}), + ...(data.serverPort != null ? { [SERVER_PORT]: data.serverPort } : {}), + }, + }); + }, + { requiresParentSpan: true }, + ); } From 051038131f3fe014710bcf126bdef7e9f012078e Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Thu, 29 Oct 2026 10:58:05 +0100 Subject: [PATCH 2/2] fix(server-utils): Use requiresParentSpan for legacy Redis and test all paths Move the redis v2-v3 subscriber onto bindTracingChannelToSpan so every Redis path checks for a parent span the same way. The wrapped command callback still ends the span; deferSpanEnd keeps the helper from ending it on the sync return. Add no-parent tests for ioredis, node-redis v4/v5 (orchestrion), native batch commands, and a unit test for the legacy path. Co-Authored-By: Claude Opus 5.5 --- .../tracing/ioredis-dc/scenario-no-parent.mjs | 22 +++ .../suites/tracing/ioredis-dc/test.ts | 21 +++ .../scenario-no-parent-redis-4.mjs | 25 +++ .../scenario-no-parent-redis-5.mjs | 25 +++ .../suites/tracing/redis-cache/test.ts | 37 ++++ .../tracing/redis-dc/scenario-no-parent.mjs | 8 +- .../suites/tracing/redis-dc/test.ts | 37 ++-- .../src/integrations/redis/index.ts | 64 +++---- .../integrations/redis/legacy-redis.test.ts | 178 ++++++++++++++++++ 9 files changed, 349 insertions(+), 68 deletions(-) create mode 100644 dev-packages/node-integration-tests/suites/tracing/ioredis-dc/scenario-no-parent.mjs create mode 100644 dev-packages/node-integration-tests/suites/tracing/redis-cache/scenario-no-parent-redis-4.mjs create mode 100644 dev-packages/node-integration-tests/suites/tracing/redis-cache/scenario-no-parent-redis-5.mjs create mode 100644 packages/server-utils/test/integrations/redis/legacy-redis.test.ts diff --git a/dev-packages/node-integration-tests/suites/tracing/ioredis-dc/scenario-no-parent.mjs b/dev-packages/node-integration-tests/suites/tracing/ioredis-dc/scenario-no-parent.mjs new file mode 100644 index 000000000000..2d8e0f8ef0d7 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/ioredis-dc/scenario-no-parent.mjs @@ -0,0 +1,22 @@ +import * as Sentry from '@sentry/node'; + +async function run() { + const { default: Redis } = await import('ioredis-6'); + const redisClient = new Redis({ host: '127.0.0.1', port: 6382, lazyConnect: true }); + + await redisClient.connect(); + await redisClient.set('dc-no-parent:before', 'test-value'); + + const parent = Sentry.startInactiveSpan({ name: 'parent operation' }); + await Sentry.withActiveSpan(parent, () => redisClient.set('dc-no-parent:inside', 'test-value')); + + await redisClient.set('dc-no-parent:after', 'test-value'); + await redisClient.disconnect(); + + // Flush before ending the parent, so a span without a parent would arrive before the parent does. + await Sentry.flush(); + parent.end(); + await Sentry.flush(); +} + +run(); diff --git a/dev-packages/node-integration-tests/suites/tracing/ioredis-dc/test.ts b/dev-packages/node-integration-tests/suites/tracing/ioredis-dc/test.ts index 739787de72da..119e2917d5fe 100644 --- a/dev-packages/node-integration-tests/suites/tracing/ioredis-dc/test.ts +++ b/dev-packages/node-integration-tests/suites/tracing/ioredis-dc/test.ts @@ -153,5 +153,26 @@ describeWithDockerCompose( }); }); } + + createEsmAndCjsTests(__dirname, 'scenario-no-parent.mjs', 'instrument.mjs', (createTestRunner, test) => { + test('only instruments commands with an active parent span', { timeout: 60_000 }, async () => { + const runner = createTestRunner(); + // Spans without a parent share the scope's trace id, so they are collected with this trace. + const spansPromise = runner.collectStreamedSpansUntilSegment('parent operation'); + + await runner.start().completed(); + + const spans = await spansPromise; + const parent = spans.find(span => span.is_segment && span.name === 'parent operation')!; + expect( + spans + .filter(span => span.attributes[SENTRY_ORIGIN]?.value === ORIGIN) + .map(span => ({ + query: span.attributes[DB_QUERY_TEXT]?.value ?? span.name, + parentId: span.parent_span_id, + })), + ).toEqual([{ query: 'set dc-no-parent:inside ?', parentId: parent.span_id }]); + }); + }); }, ); diff --git a/dev-packages/node-integration-tests/suites/tracing/redis-cache/scenario-no-parent-redis-4.mjs b/dev-packages/node-integration-tests/suites/tracing/redis-cache/scenario-no-parent-redis-4.mjs new file mode 100644 index 000000000000..dc13f114e493 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/redis-cache/scenario-no-parent-redis-4.mjs @@ -0,0 +1,25 @@ +import * as Sentry from '@sentry/node'; +import { createClient } from 'redis-4'; + +async function run() { + const redisClient = await createClient({ socket: { port: 6383 } }).connect(); + + await redisClient.set('redis-4-no-parent:before', 'test-value'); + await redisClient.multi().set('redis-4-no-parent:multi-before', 'test-value').exec(); + + const parent = Sentry.startInactiveSpan({ name: 'parent operation' }); + await Sentry.withActiveSpan(parent, async () => { + await redisClient.set('redis-4-no-parent:inside', 'test-value'); + await redisClient.multi().set('redis-4-no-parent:multi-inside', 'test-value').exec(); + }); + + await redisClient.set('redis-4-no-parent:after', 'test-value'); + await redisClient.disconnect(); + + // Flush before ending the parent, so a span without a parent would arrive before the parent does. + await Sentry.flush(); + parent.end(); + await Sentry.flush(); +} + +run(); diff --git a/dev-packages/node-integration-tests/suites/tracing/redis-cache/scenario-no-parent-redis-5.mjs b/dev-packages/node-integration-tests/suites/tracing/redis-cache/scenario-no-parent-redis-5.mjs new file mode 100644 index 000000000000..b731cc1f50bf --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/redis-cache/scenario-no-parent-redis-5.mjs @@ -0,0 +1,25 @@ +import * as Sentry from '@sentry/node'; +import { createClient } from 'redis-5'; + +async function run() { + const redisClient = await createClient({ socket: { port: 6383 } }).connect(); + + await redisClient.set('redis-5-no-parent:before', 'test-value'); + await redisClient.multi().set('redis-5-no-parent:multi-before', 'test-value').exec(); + + const parent = Sentry.startInactiveSpan({ name: 'parent operation' }); + await Sentry.withActiveSpan(parent, async () => { + await redisClient.set('redis-5-no-parent:inside', 'test-value'); + await redisClient.multi().set('redis-5-no-parent:multi-inside', 'test-value').exec(); + }); + + await redisClient.set('redis-5-no-parent:after', 'test-value'); + await redisClient.disconnect(); + + // Flush before ending the parent, so a span without a parent would arrive before the parent does. + await Sentry.flush(); + parent.end(); + await Sentry.flush(); +} + +run(); diff --git a/dev-packages/node-integration-tests/suites/tracing/redis-cache/test.ts b/dev-packages/node-integration-tests/suites/tracing/redis-cache/test.ts index e51e00265799..1926003d57bc 100644 --- a/dev-packages/node-integration-tests/suites/tracing/redis-cache/test.ts +++ b/dev-packages/node-integration-tests/suites/tracing/redis-cache/test.ts @@ -497,4 +497,41 @@ describeWithDockerCompose('redis cache auto instrumentation', { workingDirectory }); }); }); + + describe('no parent span', () => { + for (const version of [4, 5]) { + createEsmAndCjsTests( + __dirname, + `scenario-no-parent-redis-${version}.mjs`, + `instrument-redis-${version}.mjs`, + (createTestRunner, test) => { + test( + `only instruments commands with an active parent span (redis-${version})`, + { timeout: 60_000 }, + async () => { + const runner = createTestRunner(); + // Spans without a parent share the scope's trace id, so they are collected with this trace. + const spansPromise = runner.collectStreamedSpansUntilSegment('parent operation'); + + await runner.start().completed(); + + const spans = await spansPromise; + const parent = spans.find(span => span.is_segment && span.name === 'parent operation')!; + expect( + spans + .filter(span => span.attributes[SENTRY_ORIGIN]?.value === redisOrigin) + .map(span => ({ + query: span.attributes[DB_QUERY_TEXT]?.value ?? span.name, + parentId: span.parent_span_id, + })), + ).toEqual([ + { query: `SET redis-${version}-no-parent:inside [1 other arguments]`, parentId: parent.span_id }, + { query: 'MULTI', parentId: parent.span_id }, + ]); + }, + ); + }, + ); + } + }); }); diff --git a/dev-packages/node-integration-tests/suites/tracing/redis-dc/scenario-no-parent.mjs b/dev-packages/node-integration-tests/suites/tracing/redis-dc/scenario-no-parent.mjs index 572022c9310e..b67c52c4999b 100644 --- a/dev-packages/node-integration-tests/suites/tracing/redis-dc/scenario-no-parent.mjs +++ b/dev-packages/node-integration-tests/suites/tracing/redis-dc/scenario-no-parent.mjs @@ -5,14 +5,18 @@ async function run() { const redisClient = await createClient({ socket: { host: '127.0.0.1', port: 6381 } }).connect(); await redisClient.set('dc-no-parent:before', 'test-value'); + await redisClient.multi().set('dc-no-parent:multi-before', 'test-value').exec(); const parent = Sentry.startInactiveSpan({ name: 'parent operation' }); - await Sentry.withActiveSpan(parent, () => redisClient.set('dc-no-parent:inside', 'test-value')); + await Sentry.withActiveSpan(parent, async () => { + await redisClient.set('dc-no-parent:inside', 'test-value'); + await redisClient.multi().set('dc-no-parent:multi-inside', 'test-value').exec(); + }); await redisClient.set('dc-no-parent:after', 'test-value'); await redisClient.disconnect(); - // Flush every command before the parent signals that span collection is complete. + // Flush before ending the parent, so a span without a parent would arrive before the parent does. await Sentry.flush(); parent.end(); await Sentry.flush(); 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 fb3b37bc542e..61277af75a6b 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 @@ -1,4 +1,3 @@ -import type { SerializedStreamedSpan } from '@sentry/core'; import { CACHE_HIT, CACHE_ITEM_SIZE, @@ -204,35 +203,23 @@ describeWithDockerCompose( createEsmAndCjsTests(__dirname, 'scenario-no-parent.mjs', 'instrument.mjs', (createTestRunner, test) => { test('only instruments commands with an active parent span', { timeout: 60_000 }, async () => { const runner = createTestRunner(); - const dbSpans = new Map(); - const spansPromise = runner.collectStreamedSpans(spans => { - for (const span of spans) { - if (span.attributes[SENTRY_ORIGIN]?.value === ORIGIN) { - dbSpans.set(span.span_id, span); - } - } - return spans.some(span => span.is_segment && span.name === 'parent operation'); - }); + // Spans without a parent share the scope's trace id, so they are collected with this trace. + const spansPromise = runner.collectStreamedSpansUntilSegment('parent operation'); - await runner.unordered().start().completed(); + await runner.start().completed(); const spans = await spansPromise; - const parent = spans.find(span => span.is_segment && span.name === 'parent operation'); - expect(parent).toBeDefined(); + const parent = spans.find(span => span.is_segment && span.name === 'parent operation')!; expect( - Array.from(dbSpans.values(), span => ({ - query: span.attributes[DB_QUERY_TEXT]?.value, - parentSpanId: span.parent_span_id, - traceId: span.trace_id, - isSegment: span.is_segment, - })), + spans + .filter(span => span.attributes[SENTRY_ORIGIN]?.value === ORIGIN) + .map(span => ({ + query: span.attributes[DB_QUERY_TEXT]?.value ?? span.name, + parentId: span.parent_span_id, + })), ).toEqual([ - { - query: 'SET dc-no-parent:inside ?', - parentSpanId: parent?.span_id, - traceId: parent?.trace_id, - isSegment: false, - }, + { query: 'SET dc-no-parent:inside ?', parentId: parent.span_id }, + { query: 'MULTI', parentId: parent.span_id }, ]); }); }); diff --git a/packages/server-utils/src/integrations/redis/index.ts b/packages/server-utils/src/integrations/redis/index.ts index fd787da25c0a..4d9024cf3fce 100644 --- a/packages/server-utils/src/integrations/redis/index.ts +++ b/packages/server-utils/src/integrations/redis/index.ts @@ -131,29 +131,23 @@ function startCommandSpan( // --- redis v2-v3: `RedisClient.prototype.internal_send_command(command_obj)` --- -// Settles via `command_obj.callback`, not the sync return — so instead of -// `bindTracingChannelToSpan` we open the span in `start`, wrap the callback to end it, and end on `error` for sync throws. -function subscribeLegacyRedisCommand(cacheOptions: RedisCacheOptions): void { - const channel = diagnosticsChannel.tracingChannel(CHANNELS.REDIS_COMMAND); - const noop = (): void => {}; - channel.subscribe({ - end: noop, - asyncStart: noop, - asyncEnd: noop, - start(data) { - const parentSpan = getActiveSpan(); - if (!parentSpan) { - return; - } +// Settles via `command_obj.callback`, not the sync return, so the wrapped callback ends the span. +// Only a sync throw, where the callback never fires, leaves ending it to the helper. +function bindLegacyRedisCommandChannel(cacheOptions: RedisCacheOptions): void { + const channel = diagnosticsChannel.tracingChannel( + CHANNELS.REDIS_COMMAND, + ); + bindTracingChannelToSpan( + channel, + data => { const command = data.arguments?.[0] as LegacyRedisCommand | undefined; if (!command || typeof command !== 'object') { - return; + return undefined; } - // The span is ended via the wrapped callback (or the sync-throw `error` path). A - // command with no callback has no completion signal to end it on, so don't open one. + // A command with no callback has no completion signal to end the span on, so don't open one. const originalCallback = command.callback; if (typeof originalCallback !== 'function') { - return; + return undefined; } const client = data.self as LegacyRedisClient | undefined; const attributes: SpanAttributes = { @@ -167,8 +161,8 @@ function subscribeLegacyRedisCommand(cacheOptions: RedisCacheOptions): void { if (client?.connection_options?.port != null) { attributes[SERVER_PORT] = client.connection_options.port; } + const parentSpan = getActiveSpan(); const span = startCommandSpan(command.command, command.args ?? [], attributes, cacheOptions); - (data as OrchestrionChannelContext & { _sentrySpan?: Span })._sentrySpan = span; command.callback = function (this: unknown, err: Error | null | undefined, reply: unknown) { if (!err) { @@ -177,17 +171,15 @@ function subscribeLegacyRedisCommand(cacheOptions: RedisCacheOptions): void { endSpan(span, err); // eslint-disable-next-line prefer-rest-params const args = arguments as unknown as [Error | null | undefined, unknown]; - return withActiveSpan(parentSpan, () => originalCallback.apply(this, args)); + return withActiveSpan(parentSpan ?? null, () => originalCallback.apply(this, args)); }; + return span; }, - error(data) { - // Synchronous throw: the wrapped callback never fires, so end here instead. - const span = (data as OrchestrionChannelContext & { _sentrySpan?: Span })._sentrySpan; - if (span) { - endSpan(span, data.error); - } + { + requiresParentSpan: true, + deferSpanEnd: ({ data }) => !('error' in data), }, - }); + ); } // --- node-redis v4/v5 (`@redis/client`) --- @@ -296,15 +288,8 @@ const _redisIntegration = ((options: RedisIntegrationOptions = {}) => { return { name: INTEGRATION_NAME, setup(client) { - // redis v2-v3 uses a nested callback, not `bindStore`, so it subscribes - // without the async-context binding. Kept separate so a missing binding - // never defers it: on the bundler path the wait would push subscription - // past `Sentry.init()` and early commands would emit with no subscriber - invokeOrchestrionInstrumentation(client, redisModuleNames, instrumentLegacyRedis, [options], { - requiresTracingChannelBinding: false, - }); - // node-redis v4/v5 binds spans into async context via `bindTracingChannelToSpan`. - invokeOrchestrionInstrumentation(client, redisModuleNames, instrumentNodeRedis, [options]); + // redis v2-v3 and node-redis v4/v5 bind spans into async context via `bindTracingChannelToSpan`. + invokeOrchestrionInstrumentation(client, redisModuleNames, instrumentRedis, [options]); // ioredis `<5.11.0` (>=5.11.0 publishes its own `ioredis:*` channel, handled in `setupOnce`). invokeOrchestrionInstrumentation(client, ioredisModuleNames, instrumentIoredis, [options]); }, @@ -320,11 +305,8 @@ const _redisIntegration = ((options: RedisIntegrationOptions = {}) => { }; }) satisfies IntegrationFn; -function instrumentLegacyRedis(options: RedisIntegrationOptions): void { - subscribeLegacyRedisCommand(options); -} - -function instrumentNodeRedis(options: RedisIntegrationOptions): void { +function instrumentRedis(options: RedisIntegrationOptions): void { + bindLegacyRedisCommandChannel(options); bindNodeRedisCommandChannel(CHANNELS.NODE_REDIS_COMMAND, getSendCommandArgs, options); bindNodeRedisCommandChannel(CHANNELS.NODE_REDIS_EXECUTOR, getExecutorArgs, options); bindNodeRedisConnectChannel(); diff --git a/packages/server-utils/test/integrations/redis/legacy-redis.test.ts b/packages/server-utils/test/integrations/redis/legacy-redis.test.ts new file mode 100644 index 000000000000..4bca22f7ec70 --- /dev/null +++ b/packages/server-utils/test/integrations/redis/legacy-redis.test.ts @@ -0,0 +1,178 @@ +import { AsyncLocalStorage } from 'node:async_hooks'; +import { tracingChannel } from 'node:diagnostics_channel'; +import { DB_SYSTEM_NAME, SENTRY_ORIGIN, SERVER_ADDRESS, SERVER_PORT } from '@sentry/conventions/attributes'; +import type { Client, Scope, Span } from '@sentry/core'; +import * as SentryCore from '@sentry/core'; +import { + _INTERNAL_setSpanForScope, + getDefaultCurrentScope, + getDefaultIsolationScope, + GLOBAL_OBJ, + SPAN_STATUS_ERROR, + setAsyncContextStrategy, +} from '@sentry/core'; +import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it, type MockInstance, vi } from 'vitest'; +import { redisIntegration } from '../../../src/integrations/redis'; +import { CHANNELS } from '../../../src/orchestrion/channels'; + +interface TestStore { + scope: Scope; + isolationScope: Scope; +} + +// Same minimal async-context strategy as the pg tests: `bindTracingChannelToSpan` only binds +// when a `getTracingChannelBinding` exists. +function installTestAsyncContextStrategy(): void { + const asyncStorage = new AsyncLocalStorage(); + + function getScopes(): TestStore { + return asyncStorage.getStore() || { scope: getDefaultCurrentScope(), isolationScope: getDefaultIsolationScope() }; + } + + setAsyncContextStrategy({ + withScope: callback => { + const scope = getScopes().scope.clone(); + const isolationScope = getScopes().isolationScope; + return asyncStorage.run({ scope, isolationScope }, () => callback(scope)); + }, + withSetScope: (scope, callback) => { + const isolationScope = getScopes().isolationScope; + return asyncStorage.run({ scope, isolationScope }, () => callback(scope)); + }, + withIsolationScope: callback => { + const scope = getScopes().scope; + const isolationScope = getScopes().isolationScope.clone(); + return asyncStorage.run({ scope, isolationScope }, () => callback(isolationScope)); + }, + withSetIsolationScope: (isolationScope, callback) => { + const scope = getScopes().scope; + return asyncStorage.run({ scope, isolationScope }, () => callback(isolationScope)); + }, + getCurrentScope: () => getScopes().scope, + getIsolationScope: () => getScopes().isolationScope, + getTracingChannelBinding: () => ({ + asyncLocalStorage: asyncStorage, + getStoreWithActiveSpan: (span: Span) => { + const scope = getScopes().scope.clone(); + const isolationScope = getScopes().isolationScope; + _INTERNAL_setSpanForScope(scope, span); + return { scope, isolationScope }; + }, + }), + }); +} + +function makeSpan(): Span { + return { + end: vi.fn(), + setStatus: vi.fn(), + setAttributes: vi.fn(), + spanContext: () => ({ traceId: 'a'.repeat(32), spanId: 'b'.repeat(16), traceFlags: 1 }), + } as unknown as Span; +} + +type Callback = (err: Error | null | undefined, reply: unknown) => unknown; + +interface LegacyCommand { + command: string; + args: string[]; + callback?: Callback; +} + +// What redis v2-v3 passes to `internal_send_command`, published on the orchestrion channel. +function sendCommand(command: LegacyCommand, internalSendCommand: () => unknown = () => true): void { + const ctx = { arguments: [command], self: { connection_options: { host: 'localhost', port: 6379 } } }; + tracingChannel(CHANNELS.REDIS_COMMAND).traceSync(internalSendCommand, ctx); +} + +describe('redisIntegration (redis v2-v3)', () => { + let startInactiveSpanSpy: MockInstance; + let getActiveSpanSpy: MockInstance; + let span: Span; + + beforeAll(() => { + installTestAsyncContextStrategy(); + GLOBAL_OBJ.__SENTRY_ORCHESTRION__ = { runtime: ['redis'] }; + redisIntegration().setup?.({ on: () => () => undefined } as unknown as Client); + }); + + afterAll(() => { + setAsyncContextStrategy(undefined); + delete GLOBAL_OBJ.__SENTRY_ORCHESTRION__; + }); + + beforeEach(() => { + span = makeSpan(); + startInactiveSpanSpy = vi.spyOn(SentryCore, 'startInactiveSpan').mockReturnValue(span); + getActiveSpanSpy = vi.spyOn(SentryCore, 'getActiveSpan').mockReturnValue({} as Span); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + it('starts a span and ends it when the command callback fires', () => { + const originalCallback = vi.fn(); + const command: LegacyCommand = { command: 'set', args: ['key', 'value'], callback: originalCallback }; + + sendCommand(command); + + expect(startInactiveSpanSpy).toHaveBeenCalledWith( + expect.objectContaining({ + attributes: expect.objectContaining({ + [DB_SYSTEM_NAME]: 'redis', + [SENTRY_ORIGIN]: 'auto.db.redis', + [SERVER_ADDRESS]: 'localhost', + [SERVER_PORT]: 6379, + }), + }), + ); + // The sync return must not end the span, the reply has not arrived yet. + expect(span.end).not.toHaveBeenCalled(); + + command.callback?.(null, 'OK'); + + expect(span.end).toHaveBeenCalledTimes(1); + expect(originalCallback).toHaveBeenCalledWith(null, 'OK'); + }); + + it('marks the span as errored when the command callback gets an error', () => { + const command: LegacyCommand = { command: 'incr', args: ['key'], callback: vi.fn() }; + + sendCommand(command); + command.callback?.(new Error('WRONGTYPE'), undefined); + + expect(span.setStatus).toHaveBeenCalledWith({ code: SPAN_STATUS_ERROR, message: 'WRONGTYPE' }); + expect(span.end).toHaveBeenCalledTimes(1); + }); + + it('ends the span when the command throws synchronously', () => { + const command: LegacyCommand = { command: 'set', args: ['key', 'value'], callback: vi.fn() }; + + expect(() => + sendCommand(command, () => { + throw new Error('connection closed'); + }), + ).toThrow('connection closed'); + + expect(span.setStatus).toHaveBeenCalledWith(expect.objectContaining({ code: SPAN_STATUS_ERROR })); + expect(span.end).toHaveBeenCalledTimes(1); + }); + + it('does not start a span without a parent span', () => { + getActiveSpanSpy.mockReturnValue(undefined); + const originalCallback = vi.fn(); + const command: LegacyCommand = { command: 'set', args: ['key', 'value'], callback: originalCallback }; + + sendCommand(command); + + expect(startInactiveSpanSpy).not.toHaveBeenCalled(); + expect(command.callback).toBe(originalCallback); + }); + + it('does not start a span for a command without a callback', () => { + sendCommand({ command: 'set', args: ['key', 'value'] }); + + expect(startInactiveSpanSpy).not.toHaveBeenCalled(); + }); +});