feat(node): use diagnostics_channel for redis >= 5.12.0#20573
feat(node): use diagnostics_channel for redis >= 5.12.0#20573
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Adds diagnostics_channel-based Redis tracing for upcoming node-redis (>= 5.12.0) while narrowing the existing IITM patching to avoid double instrumentation once diagnostics events are available.
Changes:
- Restrict vendored node-redis IITM patcher to
>=5.0.0 <5.12.0. - Add a diagnostics_channel subscriber which creates Sentry spans from
node-redis:*tracing channels. - Ensure
@sentry/opentelemetry/*subpath imports stay external in the Node rollup build.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| packages/node/src/integrations/tracing/redis/vendored/redis-instrumentation.ts | Narrows the v5 IITM instrumentation version range to stop before 5.12.0. |
| packages/node/src/integrations/tracing/redis/redis-dc-subscriber.ts | New diagnostics_channel subscriber which creates/ends spans for command/batch/connect events and runs the responseHook. |
| packages/node/src/integrations/tracing/redis/index.ts | Wires the subscriber into Redis instrumentation (deferred to next tick). |
| packages/node/rollup.npm.config.mjs | Externalizes @sentry/opentelemetry/* imports so subpath entrypoints aren’t bundled. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // 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)); |
There was a problem hiding this comment.
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.
| // 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); |
| // 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)); |
There was a problem hiding this comment.
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.
size-limit report 📦
|
isaacs
force-pushed
the
isaacs/vendor-redis
branch
3 times, most recently
from
d150996 to
de731ed
Compare
node-redis 5.12.0 publishes command, batch, and connect events via node:diagnostics_channel. Subscribe to those channels via @sentry/opentelemetry/tracing-channel to produce spans without IITM- based monkey-patching, which lets the redis integration work on runtimes that don't support IITM (Bun, Deno, Cloudflare Workers). The existing OTel patcher is narrowed to '<5.12.0' so it does not double-instrument when both paths are present. The DC subscription is deferred to the next microtask so it runs after initOpenTelemetry() sets up the Sentry context manager (required for bindStore).
isaacs
force-pushed
the
feat/redis-diagnostic-channels
branch
2 times, most recently
from
fdbf2b6 to
75744f4
Compare
isaacs
force-pushed
the
feat/redis-diagnostic-channels
branch
from
75744f4 to
22acdcd
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
isaacs
force-pushed
the
feat/redis-diagnostic-channels
branch
from
22acdcd to
38623e1
Compare
isaacs
force-pushed
the
feat/redis-diagnostic-channels
branch
from
38623e1 to
f88ffb0
Compare
| error?: Error; | ||
| } | ||
|
|
||
| interface BatchData { |
There was a problem hiding this comment.
l/q: Are these types also vendored? If so it might be nicer if they live in the vendored folder
There was a problem hiding this comment.
Not vendored from OTEL. the those are exported by redis, shame we can't use it as a dev dep here.
| "prisma": "6.15.0", | ||
| "proxy": "^2.1.1", | ||
| "redis-4": "npm:redis@^4.6.14", | ||
| "redis-5": "npm:redis@^5.12.0", |
There was a problem hiding this comment.
m: I think it would make sense to add 2 tests. One for redis 5 with tracing channels and another without them. Then we would know if we break something.
There was a problem hiding this comment.
There are subtle differences between span counts with 5.12 and <5.12 as a result of some decisions in the redis tracing channel PR.
But yea we should have both to make sure we capture correctly.
| // `@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. | ||
| void Promise.resolve().then(() => subscribeRedisDiagnosticChannels(cacheResponseHook)); |
There was a problem hiding this comment.
q: Would this work the same with CJS and ESM in Node? I wonder if there is a better way, than defer it to the next tick, like a "afterSetup" hook or so
There was a problem hiding this comment.
Yea, it works the same. We have an afterAllSetup but that could be much later than we like, or too early. I have not tried tbh.
Let me give afterAllSetup a go.
node-redis sanitizeArgs replaces MGET key arguments with '?' in the diagnostics_channel payload, so cache prefix detection cannot match — MGET remains a plain db.redis span.
| if (!span) return; | ||
| // Same slice: strip command name from args before passing to the response hook. | ||
| runResponseHook(span, data.command, data.args.slice(1), data.result); | ||
| span.end(); |
There was a problem hiding this comment.
Command error causes response hook on ended span
Medium Severity
Per the Node.js TracingChannel contract, when a traced promise rejects, both the error and asyncEnd events fire (in that order). The error handler sets the error status and ends the span, but then asyncEnd still runs runResponseHook on the already-ended span with a potentially undefined data.result, and calls span.end() again. The asyncEnd handler lacks a guard against the error-already-handled case, so cacheResponseHook may incorrectly set cache.hit: false (from calculateCacheItemSize(undefined) returning 0) on error spans.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 992feac. Configure here.
The hook unconditionally set origin to 'auto.db.otel.redis', clobbering the diagnostic_channel origin on all DC-sourced spans. Remove the override so each instrumentation path keeps its own origin.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a57c3bf. Configure here.
| response: unknown, | ||
| ) => { | ||
| span.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, 'auto.db.otel.redis'); | ||
|
|
There was a problem hiding this comment.
IITM-path redis spans lose sentry.origin attribute
High Severity
The removal of sentry.origin from cacheResponseHook means ioredis and redis module spans (versions <5.12.0) no longer set sentry.origin. These auto-instrumented spans now incorrectly default to origin: 'manual' instead of auto.db.otel.redis, causing integration tests to fail.
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit a57c3bf. Configure here.
There was a problem hiding this comment.
We do this already in the integration inside the IITM path, so this was affecting both while being redundant for IITM.
Tests will prove that I think...
Builds on #20510 and adds tracing channels subscribers via
node:diagnostics_channel.The module patchers are narrowed to
>=5.0.0 <5.12.0while the subscriber path runs unconditionally. it will be inert in older releases anyways and will activate when version 5.12 publishes the events.Verified that it works on Node and Bun equally as well, while IITM fails on Bun.