Uh oh!
There was an error while loading. Please reload this page.
ref(redis): expose diagnostics-channel subscription as an integration - #21963
Conversation
Stop exporting the raw subscribeRedisDiagnosticChannels from @sentry/server-utils and expose a redisIntegration plugin instead, mirroring the mysql2 and graphql subscribers. The node and deno integrations now extend that base plugin rather than calling subscribe directly (the raw export predated the extendIntegration API). Drop the redundant subscribed guard, the activeUnbinds bookkeeping, the module-level response hook, and the _resetRedisDiagnosticChannelsForTesting export. The subscriber has a single deduped setupOnce caller, so the guard protected a path that can't occur; the response hook is captured via closure. Tests subscribe once in beforeAll and rely on process isolation instead of unbind/reset, matching the graphql subscriber tests.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit aa0e0d1. Configure here.
| // `bindTracingChannelToSpan` (inside the subscriber) makes the span the active | ||
| // OTel context via `bindStore`, which needs the Sentry OTel context manager to | ||
| // be registered — `initOpenTelemetry()` does that after integration `setupOnce`, | ||
| // so defer to the next tick. |
There was a problem hiding this comment.
Preload skips Redis channel subscribe
Medium Severity
Diagnostics-channel Redis subscription moved into redisIntegrationsetupOnce, but instrumentRedis (still invoked from preloadOpenTelemetry) no longer registers those handlers. Preload-only or preload-before-init flows 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)
Reviewed by Cursor Bugbot for commit aa0e0d1. Configure here.
size-limit report 📦
|
JPeer264
left a comment
There was a problem hiding this comment.
Implementation LGTM. I added couple of concerns.
| }); | ||
| describe('idempotency', () => { | ||
| it('does not re-subscribe on a second call, but updates the response hook', async () => { |
There was a problem hiding this comment.
setupOnce guarantees that, this was a unit test that tested the subscription only happening once which wouldn't pass now since the "install once" is handled by the integration wrapper not the sub function.
| */ | ||
| export { mongooseIntegration } from './mongoose'; | ||
| export { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
semver guarantees on previously internal was never promised
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
There was a problem hiding this comment.
Just double checked. We set this in our README, so I think we're good here
There was a problem hiding this comment.
Great, thanks! We also said this here, but no one reads that i think.
| if (subscribed) return; | ||
| subscribed = true; | ||
| try { |
There was a problem hiding this comment.
q/l: Any reason why the try/catch block got removed? I think keeping it wouldn't harm
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ya I cross-checked true. Thanks for confirming
Uh oh!
There was an error while loading. Please reload this page.
…instrumentation-redis-to-orchestrion Resolve conflict in redis integration: keep this branch's skip-both-OTel-patches logic (branch adds orchestrion redisConfig for node-redis <5.12.0), adopt develop's extendIntegration structure where the diagnostics-channel subscription is handled by the shared server-utils redisChannelIntegration (#21963). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The redis diagnostics-channel work exported a raw
subscribeRedisDiagnosticChannelsfrom@sentry/server-utilsthat node and deno called directly. That predated theextendIntegrationAPI, so this brings it in line with the other integrations.I don't think this is a breaking change because
server-utilsis a private-ish package for us.While in here I dropped a few things that were only there to work around the raw export:
subscribedguard, since there's now a single dedupedsetupOncecaller, so it guarded a path that can't happenactiveUnbindsarray and_resetRedisDiagnosticChannelsForTestingexport, which only existed so tests could detach handlers between runs. The subscriber tests now subscribe once inbeforeAlland lean on process isolation, same as the graphql testsI left the enclosing-span behavior alone for now (redis still creates spans without an active parent, unlike mysql2). Happy to argue that separately.