-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
ref(redis): expose diagnostics-channel subscription as an integration #21963
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,22 +5,8 @@ | |
| */ | ||
|
|
||
| export { mongooseIntegration } from './mongoose'; | ||
| export { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. m: I know you meant that this is private-ish, still I would vote to keep these here as it is a public package (even though it is highly unlikely that someone used it. I think deprecating these exports would be a better way IMO
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the opposite, semver guarantees on previously internal was never promised. So I'm exercising that here to align with the other integrations. WDYT? Maybe we can sample more opinions?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That is correct, do we set this promise somewhere for this package now? It'd be good to clarify in general what counts as "internal" now
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just double checked. We set this in our README, so I think we're good here
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great, thanks! We also said this here, but no one reads that i think. |
||
| IOREDIS_DC_CHANNEL_COMMAND, | ||
| IOREDIS_DC_CHANNEL_CONNECT, | ||
| REDIS_DC_CHANNEL_BATCH, | ||
| REDIS_DC_CHANNEL_COMMAND, | ||
| REDIS_DC_CHANNEL_CONNECT, | ||
| subscribeRedisDiagnosticChannels, | ||
| } from './redis/redis-dc-subscriber'; | ||
| export type { | ||
| IORedisCommandData, | ||
| RedisBatchData, | ||
| RedisCommandData, | ||
| RedisConnectData, | ||
| RedisDiagnosticChannelResponseHook, | ||
| RedisTracingChannelFactory, | ||
| } from './redis/redis-dc-subscriber'; | ||
| export { redisIntegration, type RedisDiagnosticChannelsOptions } from './redis'; | ||
| export type { RedisDiagnosticChannelResponseHook } from './redis/redis-dc-subscriber'; | ||
| export { defaultDbStatementSerializer } from './redis/redis-statement-serializer'; | ||
| export { bindTracingChannelToSpan } from './tracing-channel'; | ||
| export type { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| import { defineIntegration, type IntegrationFn, waitForTracingChannelBinding } from '@sentry/core'; | ||
| import * as dc from 'node:diagnostics_channel'; | ||
| import { type RedisDiagnosticChannelResponseHook, subscribeRedisDiagnosticChannels } from './redis-dc-subscriber'; | ||
|
|
||
| /** Options controlling the redis diagnostics-channel subscription. */ | ||
| export interface RedisDiagnosticChannelsOptions { | ||
| /** | ||
| * Optional hook invoked once the redis command response arrives. Useful for attaching | ||
| * response-derived attributes (e.g. cache hit/miss, payload size). | ||
| */ | ||
| responseHook?: RedisDiagnosticChannelResponseHook; | ||
| } | ||
|
|
||
| const _redisIntegration = ((options: RedisDiagnosticChannelsOptions = {}) => { | ||
| return { | ||
| name: 'Redis', | ||
| setupOnce() { | ||
| // Bail on runtimes without `tracingChannel` (Node <= 18.18.0). | ||
| if (!dc.tracingChannel) { | ||
| return; | ||
| } | ||
|
|
||
| waitForTracingChannelBinding(() => { | ||
| subscribeRedisDiagnosticChannels(dc.tracingChannel, options.responseHook); | ||
| }); | ||
| }, | ||
| }; | ||
| }) satisfies IntegrationFn; | ||
|
|
||
| /** | ||
| * Auto-instrument the [redis](https://www.npmjs.com/package/redis) and | ||
| * [ioredis](https://www.npmjs.com/package/ioredis) libraries via their native | ||
| * `node:diagnostics_channel` tracing channels (node-redis >= 5.12.0, ioredis >= 5.11.0). | ||
| */ | ||
| export const redisIntegration = defineIntegration(_redisIntegration); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,8 +7,7 @@ import { | |
| SERVER_PORT, | ||
| } from '@sentry/conventions/attributes'; | ||
| import type { Span } from '@sentry/core'; | ||
| import { debug, SEMANTIC_ATTRIBUTE_SENTRY_OP, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, startInactiveSpan } from '@sentry/core'; | ||
| import { DEBUG_BUILD } from '../debug-build'; | ||
| import { SEMANTIC_ATTRIBUTE_SENTRY_OP, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, startInactiveSpan } from '@sentry/core'; | ||
| import { bindTracingChannelToSpan } from '../tracing-channel'; | ||
|
|
||
| // Channel names published by node-redis >= 5.12.0 and ioredis >= 5.11.0. | ||
|
|
@@ -106,10 +105,6 @@ export type RedisDiagnosticChannelResponseHook = ( | |
| */ | ||
| export type RedisTracingChannelFactory = <T extends object>(name: string) => TracingChannel<T, T>; | ||
|
|
||
| let subscribed = false; | ||
| let currentResponseHook: RedisDiagnosticChannelResponseHook | undefined; | ||
| let activeUnbinds: Array<() => void> = []; | ||
|
|
||
| /** | ||
| * Subscribe Sentry span handlers to node-redis and ioredis diagnostics-channel | ||
| * events: `node-redis:command`/`:batch`/`:connect` (published by node-redis | ||
|
|
@@ -118,46 +113,37 @@ let activeUnbinds: Array<() => void> = []; | |
| * On older client versions the channels are never published to, so subscribers | ||
| * are inert — there is no double-instrumentation against any IITM-based | ||
| * patcher gated to those older versions. | ||
| * | ||
| * Idempotent: subsequent calls update the response hook but do not | ||
| * re-subscribe. | ||
| */ | ||
| export function subscribeRedisDiagnosticChannels( | ||
| tracingChannel: RedisTracingChannelFactory, | ||
| responseHook?: RedisDiagnosticChannelResponseHook, | ||
| ): void { | ||
| currentResponseHook = responseHook; | ||
| if (subscribed) return; | ||
| subscribed = true; | ||
|
|
||
| try { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. q/l: Any reason why the try/catch block got removed? I think keeping it wouldn't harm
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These functions never throw, so a reset/re-install behavior was never going to happen, so it is redundant. isaacs thought so too in other PRs so I'm applying them here. I don't mind having a try/catch tbh just in case but I think we need to consistently do or don't do them.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ya I cross-checked true. Thanks for confirming |
||
| // node-redis: command name appears as args[0] in the channel payload, so | ||
| // strip it before the statement and response hook see it. | ||
| activeUnbinds.push( | ||
| setupCommandChannel<RedisCommandData>(tracingChannel, REDIS_DC_CHANNEL_COMMAND, data => data.args.slice(1)), | ||
| setupBatchChannel(tracingChannel, REDIS_DC_CHANNEL_BATCH, data => | ||
| data.batchMode === 'PIPELINE' ? 'PIPELINE' : 'MULTI', | ||
| ), | ||
| setupConnectChannel(tracingChannel, REDIS_DC_CHANNEL_CONNECT), | ||
| // ioredis: args already exclude the command name; no slicing needed. And | ||
| // ioredis has no separate batch channel — pipeline/MULTI metadata rides | ||
| // on the per-command payload via `batchMode`/`batchSize`. | ||
| setupCommandChannel<IORedisCommandData>(tracingChannel, IOREDIS_DC_CHANNEL_COMMAND, data => data.args), | ||
| setupConnectChannel(tracingChannel, IOREDIS_DC_CHANNEL_CONNECT), | ||
| ); | ||
| } catch { | ||
| // The factory may rely on `node:diagnostics_channel`, which isn't always | ||
| // available. Fail closed; the SDK simply won't emit redis spans here. | ||
| DEBUG_BUILD && debug.log('Redis node:diagnostics_channel subscription failed.'); | ||
| } | ||
| // node-redis: command name appears as args[0] in the channel payload, so | ||
| // strip it before the statement and response hook see it. | ||
| setupCommandChannel<RedisCommandData>( | ||
| tracingChannel, | ||
| REDIS_DC_CHANNEL_COMMAND, | ||
| data => data.args.slice(1), | ||
| responseHook, | ||
| ); | ||
| setupBatchChannel(tracingChannel, REDIS_DC_CHANNEL_BATCH, data => | ||
| data.batchMode === 'PIPELINE' ? 'PIPELINE' : 'MULTI', | ||
| ); | ||
| setupConnectChannel(tracingChannel, REDIS_DC_CHANNEL_CONNECT); | ||
| // ioredis: args already exclude the command name; no slicing needed. And | ||
| // ioredis has no separate batch channel — pipeline/MULTI metadata rides | ||
| // on the per-command payload via `batchMode`/`batchSize`. | ||
| setupCommandChannel<IORedisCommandData>(tracingChannel, IOREDIS_DC_CHANNEL_COMMAND, data => data.args, responseHook); | ||
| setupConnectChannel(tracingChannel, IOREDIS_DC_CHANNEL_CONNECT); | ||
| } | ||
|
|
||
| function setupCommandChannel<T extends RedisCommandData | IORedisCommandData>( | ||
| tracingChannel: RedisTracingChannelFactory, | ||
| channelName: string, | ||
| getCommandArgs: (data: T) => string[], | ||
| ): () => void { | ||
| return bindTracingChannelToSpan( | ||
| responseHook?: RedisDiagnosticChannelResponseHook, | ||
| ): void { | ||
| bindTracingChannelToSpan( | ||
| tracingChannel<T>(channelName), | ||
| data => { | ||
| // `args` is already sanitized by the publishing library (node-redis / | ||
|
|
@@ -183,18 +169,18 @@ function setupCommandChannel<T extends RedisCommandData | IORedisCommandData>( | |
| captureError: false, | ||
| beforeSpanEnd(span, data) { | ||
| if ('error' in data) return; | ||
| runResponseHook(span, data.command, getCommandArgs(data), data.result); | ||
| runResponseHook(responseHook, span, data.command, getCommandArgs(data), data.result); | ||
| }, | ||
| }, | ||
| ).unbind; | ||
| ); | ||
| } | ||
|
|
||
| function setupBatchChannel( | ||
| tracingChannel: RedisTracingChannelFactory, | ||
| channelName: string, | ||
| getOperationName: (data: RedisBatchData) => string, | ||
| ): () => void { | ||
| return bindTracingChannelToSpan( | ||
| ): void { | ||
| bindTracingChannelToSpan( | ||
| tracingChannel<RedisBatchData>(channelName), | ||
| data => { | ||
| return startInactiveSpan({ | ||
|
|
@@ -212,11 +198,11 @@ function setupBatchChannel( | |
| }); | ||
| }, | ||
| { captureError: false }, | ||
| ).unbind; | ||
| ); | ||
| } | ||
|
|
||
| function setupConnectChannel(tracingChannel: RedisTracingChannelFactory, channelName: string): () => void { | ||
| return bindTracingChannelToSpan( | ||
| function setupConnectChannel(tracingChannel: RedisTracingChannelFactory, channelName: string): void { | ||
| bindTracingChannelToSpan( | ||
| tracingChannel<RedisConnectData>(channelName), | ||
| data => { | ||
| return startInactiveSpan({ | ||
|
|
@@ -231,23 +217,20 @@ function setupConnectChannel(tracingChannel: RedisTracingChannelFactory, channel | |
| }); | ||
| }, | ||
| { captureError: false }, | ||
| ).unbind; | ||
| ); | ||
| } | ||
|
|
||
| function runResponseHook(span: Span, command: string, args: string[], result: unknown): void { | ||
| const hook = currentResponseHook; | ||
| function runResponseHook( | ||
| hook: RedisDiagnosticChannelResponseHook | undefined, | ||
| span: Span, | ||
| command: string, | ||
| args: string[], | ||
| result: unknown, | ||
| ): void { | ||
| if (!hook) return; | ||
| try { | ||
| hook(span, command, args, result); | ||
| } catch { | ||
| // never let user hooks break instrumentation | ||
| } | ||
| } | ||
|
|
||
| /** Test-only: detach all channel bindings and reset module-local subscribe state. */ | ||
| export function _resetRedisDiagnosticChannelsForTesting(): void { | ||
| activeUnbinds.forEach(unbind => unbind()); | ||
| activeUnbinds = []; | ||
| subscribed = false; | ||
| currentResponseHook = undefined; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,9 +17,8 @@ import { | |
| spanToJSON, | ||
| startSpan, | ||
| } from '@sentry/core'; | ||
| import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; | ||
| import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'; | ||
| import { | ||
| _resetRedisDiagnosticChannelsForTesting, | ||
| IOREDIS_DC_CHANNEL_COMMAND, | ||
| IOREDIS_DC_CHANNEL_CONNECT, | ||
| REDIS_DC_CHANNEL_BATCH, | ||
|
|
@@ -127,23 +126,31 @@ async function traceCommand( | |
|
|
||
| const factory = tracingChannel as RedisTracingChannelFactory; | ||
|
|
||
| // The response hook is bound into the channel handlers when we subscribe, so it must be a stable | ||
| // reference for the whole file; `vi.clearAllMocks` in `afterEach` resets its recorded calls per test. | ||
| const responseHook = vi.fn(); | ||
|
|
||
| describe('subscribeRedisDiagnosticChannels', () => { | ||
| let responseHook: ReturnType<typeof vi.fn>; | ||
| let captureExceptionSpy: ReturnType<typeof vi.spyOn>; | ||
|
|
||
| // `node:diagnostics_channel` channels are process-global. `_reset…` calls each binding's `unbind`, | ||
| // so we can subscribe and fully detach per test without handlers leaking across tests. | ||
| beforeEach(() => { | ||
| // The subscriber binds handlers onto process-global channels with no teardown, so subscribe once | ||
| // here, mirroring production where `setupOnce` subscribes a single time. Per-test we only reset the | ||
| // client, scopes, and mock call history (in `afterEach`), so nothing leaks between tests. | ||
| beforeAll(() => { | ||
| installTestAsyncContextStrategy(); | ||
| subscribeRedisDiagnosticChannels(factory, responseHook); | ||
| }); | ||
|
|
||
| afterAll(() => { | ||
| setAsyncContextStrategy(undefined); | ||
| }); | ||
|
|
||
| beforeEach(() => { | ||
| initTestClient(); | ||
| responseHook = vi.fn(); | ||
| captureExceptionSpy = vi.spyOn(SentryCore, 'captureException').mockReturnValue('event-id'); | ||
| subscribeRedisDiagnosticChannels(factory, responseHook); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| _resetRedisDiagnosticChannelsForTesting(); | ||
| setAsyncContextStrategy(undefined); | ||
| getCurrentScope().clear(); | ||
| getCurrentScope().setClient(undefined); | ||
| getGlobalScope().clear(); | ||
|
|
@@ -261,21 +268,4 @@ describe('subscribeRedisDiagnosticChannels', () => { | |
| expect(spanToJSON(span!).timestamp).toBeDefined(); | ||
| }); | ||
| }); | ||
|
|
||
| describe('idempotency', () => { | ||
| it('does not re-subscribe on a second call, but updates the response hook', async () => { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. q: Why was this removed?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| const secondHook = vi.fn(); | ||
| subscribeRedisDiagnosticChannels(factory, secondHook); | ||
|
|
||
| const { span } = await traceCommand( | ||
| REDIS_DC_CHANNEL_COMMAND, | ||
| { command: 'GET', args: ['GET', 'k'] }, | ||
| { result: 'v' }, | ||
| ); | ||
|
|
||
| expect(secondHook).toHaveBeenCalledTimes(1); | ||
| expect(secondHook).toHaveBeenCalledWith(span, 'GET', ['k'], 'v'); | ||
| expect(responseHook).not.toHaveBeenCalled(); | ||
| }); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Preload skips Redis channel subscribe
Medium Severity
Diagnostics-channel Redis subscription moved into
redisIntegrationsetupOnce, butinstrumentRedis(still invoked frompreloadOpenTelemetry) no longer registers those handlers. Preload-only or preload-before-initflows can run Redis against node-redis/ioredis ≥5.11 without ever subscribing, so native channel tracing stays off until full SDK init.Additional Locations (1)
packages/server-utils/src/redis/index.ts#L16-L27Reviewed by Cursor Bugbot for commit aa0e0d1. Configure here.