Skip to content
Draft
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/node/rollup.npm.config.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ export default [
makeBaseNPMConfig({
entrypoints: ['src/index.ts', 'src/init.ts', 'src/preload.ts'],
packageSpecificConfig: {
external: [/^@sentry\/opentelemetry/],
output: {
// set exports to 'named' or 'auto' so that rollup doesn't warn
exports: 'named',
Expand Down
6 changes: 6 additions & 0 deletions packages/node/src/integrations/tracing/redis/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import type { IORedisInstrumentationConfig } from './vendored/types';
import { IORedisInstrumentation } from './vendored/ioredis-instrumentation';
import { RedisInstrumentation } from './vendored/redis-instrumentation';
import { subscribeRedisDiagnosticChannels } from './redis-dc-subscriber';

interface RedisOptions {
/**
Expand Down Expand Up @@ -116,6 +117,11 @@
(): void => {
instrumentIORedis();
instrumentRedisModule();
// node-redis >= 5.12.0 publishes via diagnostics_channel. The subscriber uses
// `@sentry/opentelemetry/tracing-channel`, which needs the Sentry OTel context manager
// to be registered before it can `bindStore`. `initOpenTelemetry()` runs after integration
// `setupOnce`, so defer to the next tick.
Promise.resolve().then(() => subscribeRedisDiagnosticChannels(cacheResponseHook));

Check failure on line 124 in packages/node/src/integrations/tracing/redis/index.ts

View workflow job for this annotation

GitHub Actions / Lint

typescript-eslint(no-floating-promises)

Promises must be awaited, add void operator to ignore.
Copy link

Copilot AI Apr 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The diagnostics_channel subscriber is deferred via a microtask. This means node-redis >=5.12.0 operations performed immediately after Sentry.init() (in the same synchronous tick) can run before the subscriber is attached and will not be traced. Consider hooking this up from the OpenTelemetry init path (after the context manager is registered) or otherwise ensuring subscription happens synchronously before user code continues.

Suggested change
// node-redis >= 5.12.0 publishes via diagnostics_channel. The subscriber uses
// `@sentry/opentelemetry/tracing-channel`, which needs the Sentry OTel context manager
// to be registered before it can `bindStore`. `initOpenTelemetry()` runs after integration
// `setupOnce`, so defer to the next tick.
Promise.resolve().then(() => subscribeRedisDiagnosticChannels(cacheResponseHook));
// node-redis >= 5.12.0 publishes via diagnostics_channel. Subscribe synchronously so
// operations performed immediately after `Sentry.init()` are traced without a race window.
subscribeRedisDiagnosticChannels(cacheResponseHook);

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Apr 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR adds a new diagnostics_channel-based instrumentation path, but there are no unit tests covering span creation/end, error handling, or that the responseHook is invoked for command events. Since this package already has Redis tracing tests under packages/node/test/integrations/tracing/redis, it would be good to add a focused test suite for subscribeRedisDiagnosticChannels() using node:diagnostics_channel tracing channels.

Copilot uses AI. Check for mistakes.

// todo: implement them gradually
// new LegacyRedisInstrumentation({}),
Expand Down
231 changes: 231 additions & 0 deletions packages/node/src/integrations/tracing/redis/redis-dc-subscriber.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,231 @@
import type { Span } from '@opentelemetry/api';
import {
SEMANTIC_ATTRIBUTE_SENTRY_OP,
SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN,
SPAN_STATUS_ERROR,
startSpanManual,
} from '@sentry/core';
import { tracingChannel, type TracingChannelContextWithSpan } from '@sentry/opentelemetry/tracing-channel';
import { defaultDbStatementSerializer } from './vendored/redis-common';
import {
ATTR_DB_STATEMENT,
ATTR_DB_SYSTEM,
ATTR_NET_PEER_NAME,
ATTR_NET_PEER_PORT,
DB_SYSTEM_VALUE_REDIS,
} from './vendored/semconv';
import type { IORedisInstrumentationConfig } from './vendored/types';

// Channel names as published by node-redis >= 5.12.0.
// Hardcoded so we don't import `redis` at module-load time.
const CHANNEL_COMMAND = 'node-redis:command';
const CHANNEL_BATCH = 'node-redis:batch';
const CHANNEL_CONNECT = 'node-redis:connect';

const ORIGIN = 'auto.db.redis.diagnostic-channel';
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Origin value contains invalid hyphen character

Medium Severity

The ORIGIN constant 'auto.db.redis.diagnostic-channel' contains a hyphen (-), which is not permitted in span origins. Per the trace origin specification, origin parts may only contain [a-z], [A-Z], [0-9], and _ characters. The hyphen in diagnostic-channel makes this non-conforming — it likely needs to be diagnostic_channel instead.

Fix in Cursor Fix in Web

Triggered by project rule: PR Review Guidelines for Cursor Bot

Reviewed by Cursor Bugbot for commit 291507c. Configure here.


interface CommandData {
command: string;
args: Array<string | Buffer>;
database?: number;
serverAddress?: string;
serverPort?: number;
result?: unknown;
error?: Error;
}

interface BatchData {
batchMode?: 'MULTI' | 'PIPELINE';
batchSize?: number;
database?: number;
clientId?: string | number;
serverAddress?: string;
serverPort?: number;
result?: unknown[];
error?: Error;
}

interface ConnectData {
serverAddress?: string;
serverPort?: number;
url?: string;
error?: Error;
}

const NOOP = (): void => {};

let subscribed = false;
let currentResponseHook: IORedisInstrumentationConfig['responseHook'] | undefined;

/**
* Subscribe Sentry handlers to node-redis diagnostics_channel events (>= 5.12.0).
*
* Uses `@sentry/opentelemetry/tracing-channel` so OTel AsyncLocalStorage context propagates
* automatically via `bindStore` — without it, spans created in `start` would not become
* the active context for subsequent operations.
*
* Safe on every runtime that exposes `node:diagnostics_channel` (Node, Bun, Deno, Workers).
Copy link

Copilot AI Apr 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The JSDoc claims this is "Safe on every runtime" including Workers/Deno, but this is in @sentry/node (Node >=18) and relies on node:diagnostics_channel via @sentry/opentelemetry/tracing-channel. Consider tightening the wording to avoid implying support for runtimes where Node builtins (Buffer/diagnostics_channel) may not exist.

Suggested change
* Safe on every runtime that exposes `node:diagnostics_channel` (Node, Bun, Deno, Workers).
* Intended for Node environments where `node:diagnostics_channel` is available.
* On runtimes without the required Node builtins, setup fails closed and no subscribers are registered.

Copilot uses AI. Check for mistakes.
* In node-redis < 5.12.0 the channels are never published to, so subscribers are inert and
* there is no double-instrumentation against the IITM-based patcher (gated to < 5.12.0).
*/
export function subscribeRedisDiagnosticChannels(
responseHook?: IORedisInstrumentationConfig['responseHook'],
): void {
currentResponseHook = responseHook;
if (subscribed) return;

try {
setupCommandChannel();
setupBatchChannel();
setupConnectChannel();
subscribed = true;
} catch {
// tracingChannel from @sentry/opentelemetry requires `node:diagnostics_channel`.
// On runtimes where it isn't available, fail closed.
}
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No integration or E2E test for new feature

Low Severity

This is a feat PR adding a new diagnostics_channel-based Redis tracing path, but there doesn't appear to be any integration or E2E test covering the new subscribeRedisDiagnosticChannels functionality. The review rules recommend that feat PRs include at least one integration or E2E test.

Fix in Cursor Fix in Web

Triggered by project rule: PR Review Guidelines for Cursor Bot

Reviewed by Cursor Bugbot for commit 291507c. Configure here.


function setupCommandChannel(): void {
const channel = tracingChannel<CommandData>(CHANNEL_COMMAND, data => {
const statement = safeSerialize(data.command, data.args);
return startSpanManual(
{
name: `redis-${data.command}`,
attributes: {
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: ORIGIN,
[SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'db.redis',
[ATTR_DB_SYSTEM]: DB_SYSTEM_VALUE_REDIS,
...(statement != null ? { [ATTR_DB_STATEMENT]: statement } : {}),
...(data.serverAddress != null ? { [ATTR_NET_PEER_NAME]: data.serverAddress } : {}),
...(data.serverPort != null ? { [ATTR_NET_PEER_PORT]: data.serverPort } : {}),
},
},
span => span,
) as Span;
});

channel.subscribe({
start: NOOP,
asyncStart: NOOP,
end: NOOP,
asyncEnd: data => {
const span = data._sentrySpan;
if (!span) return;
runResponseHook(span, data.command, data.args, data.result);
span.end();
},
error: data => {
const span = data._sentrySpan;
if (!span) return;
if (data.error) {
span.setStatus({ code: SPAN_STATUS_ERROR, message: data.error.message });
}
span.end();
},
});
}

function setupBatchChannel(): void {
const channel = tracingChannel<BatchData>(CHANNEL_BATCH, data => {
const operationName = data.batchMode === 'PIPELINE' ? 'PIPELINE' : 'MULTI';

return startSpanManual(
{
name: operationName,
attributes: {
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: ORIGIN,
[SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'db.redis',
[ATTR_DB_SYSTEM]: DB_SYSTEM_VALUE_REDIS,
...(data.batchSize != null ? { 'db.redis.batch_size': data.batchSize } : {}),
...(data.serverAddress != null ? { [ATTR_NET_PEER_NAME]: data.serverAddress } : {}),
...(data.serverPort != null ? { [ATTR_NET_PEER_PORT]: data.serverPort } : {}),
},
},
span => span,
) as Span;
});

channel.subscribe({
start: NOOP,
asyncStart: NOOP,
end: NOOP,
asyncEnd: data => {
data._sentrySpan?.end();
},
error: data => {
const span = data._sentrySpan;
if (!span) return;
if (data.error) {
span.setStatus({ code: SPAN_STATUS_ERROR, message: data.error.message });
}
span.end();
},
});
}

function setupConnectChannel(): void {
const channel = tracingChannel<ConnectData>(CHANNEL_CONNECT, data => {
return startSpanManual(
{
name: 'redis-connect',
attributes: {
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: ORIGIN,
[SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'db.redis.connect',
[ATTR_DB_SYSTEM]: DB_SYSTEM_VALUE_REDIS,
...(data.serverAddress != null ? { [ATTR_NET_PEER_NAME]: data.serverAddress } : {}),
...(data.serverPort != null ? { [ATTR_NET_PEER_PORT]: data.serverPort } : {}),
},
},
span => span,
) as Span;
});

channel.subscribe({
start: NOOP,
asyncStart: NOOP,
end: NOOP,
asyncEnd: data => {
data._sentrySpan?.end();
},
error: data => {
const span = data._sentrySpan;
if (!span) return;
if (data.error) {
span.setStatus({ code: SPAN_STATUS_ERROR, message: data.error.message });
}
span.end();
},
});
}

function runResponseHook(
span: Span,
command: string,
args: Array<string | Buffer>,
result: unknown,
): void {
const hook = currentResponseHook;
if (!hook) return;
try {
hook(span, command, args as unknown as Parameters<typeof hook>[2], result);
Copy link

Copilot AI Apr 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

runResponseHook currently needs an unsafe cast for args to match the hook signature. Since CommandArgs already includes string | Buffer elements, this should be type-compatible without a cast (or you can type currentResponseHook as the redis response hook type). Removing the cast avoids masking real type mismatches.

Suggested change
hook(span, command, args as unknown as Parameters<typeof hook>[2], result);
hook(span, command, args, result);

Copilot uses AI. Check for mistakes.
} catch {
// never let user hooks break instrumentation
}
}

function safeSerialize(command: string, args: Array<string | Buffer>): string | undefined {
try {
return defaultDbStatementSerializer(command, args);
} catch {
return undefined;
}
}

// Test-only helper.
export function _resetRedisDiagnosticChannelsForTesting(): void {
subscribed = false;
currentResponseHook = undefined;
}
Comment on lines +217 to +221
Copy link

Copilot AI Apr 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file exposes _resetRedisDiagnosticChannelsForTesting(), but it only flips flags and does not unsubscribe any handlers already registered with diagnostics_channel. If tests call subscribeRedisDiagnosticChannels() again after a reset, you'll end up with duplicate subscribers (and duplicated spans). Consider keeping references to the subscriber objects (or channel instances) and calling channel.unsubscribe(...) during reset.

Copilot uses AI. Check for mistakes.

// Suppress unused-import lint when only used in types.
export type { TracingChannelContextWithSpan };
Original file line number Diff line number Diff line change
Expand Up @@ -368,7 +368,7 @@ class RedisInstrumentationV4_V5 extends InstrumentationBase<RedisInstrumentation

const multiCommanderModule = new InstrumentationNodeModuleFile(
`${basePackageName}/dist/lib/client/multi-command.js`,
['^1.0.0', '^5.0.0'],
['^1.0.0', '>=5.0.0 <5.12.0'],
(moduleExports: any) => {
const redisClientMultiCommandPrototype = moduleExports?.default?.prototype;
if (isWrapped(redisClientMultiCommandPrototype?.exec)) {
Expand Down Expand Up @@ -398,7 +398,7 @@ class RedisInstrumentationV4_V5 extends InstrumentationBase<RedisInstrumentation

const clientIndexModule = new InstrumentationNodeModuleFile(
`${basePackageName}/dist/lib/client/index.js`,
['^1.0.0', '^5.0.0'],
['^1.0.0', '>=5.0.0 <5.12.0'],
(moduleExports: any) => {
const redisClientPrototype = moduleExports?.default?.prototype;
if (redisClientPrototype?.multi) {
Expand Down Expand Up @@ -436,7 +436,7 @@ class RedisInstrumentationV4_V5 extends InstrumentationBase<RedisInstrumentation

return new InstrumentationNodeModuleDefinition(
basePackageName,
['^1.0.0', '^5.0.0'],
['^1.0.0', '>=5.0.0 <5.12.0'],
(moduleExports: any) => moduleExports,
() => {},
[commanderModuleFile, multiCommanderModule, clientIndexModule],
Expand Down
Loading