feat(server-utils): Migrate @opentelemetry/instrumentation-ioredis to orchestrion#21849
Open
chargome wants to merge 6 commits into
Open
feat(server-utils): Migrate @opentelemetry/instrumentation-ioredis to orchestrion#21849chargome wants to merge 6 commits into
@opentelemetry/instrumentation-ioredis to orchestrion#21849chargome wants to merge 6 commits into
Conversation
Resolve conflict in experimentalUseDiagnosticsChannelInjection.ts: keep develop's derive-from-names for the 1:1 channel replacements (mysql, lru-memoizer) and add ioredisChannelIntegration separately, kept out of replacedOtelIntegrationNames since it only supersedes the ioredis sub-part of the composite OTel 'Redis'. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…rvives bundling ioredis' connect() calls standard-as-callback's CJS default export; left external it resolves to a non-function in the nitro/Rollup bundle. Inline it alongside ioredis so the interop links consistently. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Member
Author
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
- Keep the OTel ioredis monkey-patch on Node without tracingChannel (<18.19) even when injection is opted in, since orchestrion can't run there — otherwise ioredis <5.11.0 would lose tracing entirely. - Defer the orchestrion ioredis bindTracingChannelToSpan calls via waitForTracingChannelBinding, matching the native redis diagnostics-channel subscriber, so the async-context binding registered by initOpenTelemetry() is available before binding. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Member
Author
|
bugbot run |
…nstrumentation The orchestrion opt-in imports cacheResponseHook; importing it from redis/index transitively pulled the vendored OTel IORedisInstrumentation/RedisInstrumentation into the opt-in module graph. Move cacheResponseHook + _redisOptions into redis/cache.ts (no OTel instrumentation imports) so apps using injection only for mysql/lru-memoizer don't bundle the redis OTel instrumentation. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Member
Author
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit da258e2. Configure here.
chargome
requested review from
JPeer264,
andreiborza,
isaacs,
mydea and
nicohrubec
and removed request for
a team,
JPeer264 and
mydea
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 da258e2. Configure here.
mydea
reviewed
Jul 1, 2026
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Comment on lines
+61
to
+68
| function connectionAttributes(host: string | undefined, port: number | undefined): Record<string, unknown> { | ||
| return { | ||
| [DB_SYSTEM]: 'redis', | ||
| [ATTR_DB_CONNECTION_STRING]: `redis://${host}:${port}`, | ||
| [NET_PEER_NAME]: host, | ||
| [NET_PEER_PORT]: port, | ||
| [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: ORIGIN, | ||
| }; |
Contributor
There was a problem hiding this comment.
Bug: The connectionAttributes function creates a malformed db.connection_string like "redis://undefined:undefined" when host or port are undefined.
Severity: MEDIUM
Suggested Fix
Conditionally add the ATTR_DB_CONNECTION_STRING, NET_PEER_NAME, and NET_PEER_PORT attributes to the returned object only if host and port are defined. This can be done using spread syntax, similar to the pattern in the MySQL and native Redis integrations.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: packages/server-utils/src/integrations/tracing-channel/ioredis.ts#L61-L68
Potential issue: When an `ioredis` client is initialized without an explicit `host` or
`port` (e.g., when using a UNIX socket), the `connectionAttributes` function is called
with `undefined` values. The string interpolation `redis://${host}:${port}` then
produces a literal string `"redis://undefined:undefined"` for the `db.connection_string`
attribute. This results in incorrect and misleading data in Sentry spans. Other
integrations in the codebase handle this by conditionally adding attributes only when
their values are defined, a pattern which is missing here.
Did we get this right? 👍 / 👎 to inform future reviews.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Migrates ioredis instrumentation from the otel to orchestrion diagnostics-channel injection.
<5.11.0;>=5.11.0keeps using ioredis' ownioredis:*diagnostics_channel (unchanged).@sentry/server-utilsproduces the same db spans as before with raw args redacted via the shareddefaultDbStatementSerializer.Redisintegration stays in place (it still handles node-redis and ioredis ≥5.11) — only its ioredis monkey-patch is turned off when injection is on, except on Node<18.19, where orchestrion isn't available.closes #20755