Uh oh!
There was an error while loading. Please reload this page.
ref(server-utils): Move mysql orchestrion integration onto bindTracingChannelToSpan - #21865
Merged
Conversation
logaretm
changed the base branch from
feat/tracing-channel-callback-context
to
developJune 30, 2026 15:15
logaretm
changed the base branch from
develop
to
feat/tracing-channel-callback-contextJune 30, 2026 15:15
Contributor
size-limit report 📦
|
logaretmforce-pushed
the
feat/mysql-tracing-channel-helper
branch
from
June 30, 2026 19:10
5b73ec5 to
a70f7f4Comparelogaretm
marked this pull request as ready for review
June 30, 2026 19:13
logaretm
requested review from
JPeer264, andreiborza, isaacs and mydea
and removed request for
a teamJune 30, 2026 19:13
Uh oh!
There was an error while loading. Please reload this page.
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 a70f7f4. Configure here.
Uh oh!
There was an error while loading. Please reload this page.
logaretmforce-pushed
the
feat/mysql-tracing-channel-helper
branch
3 times, most recently
from
June 30, 2026 19:24
020a61d to
972275cCompareisaacs
approved these changes
Jun 30, 2026
isaacs
left a comment
Member
There was a problem hiding this comment.
Looks good! Significant improvement. I'll update my pg PR once this lands.
| const span = spans.get(rawCtx); | ||
| if (!span) return; | ||
| // `tracingChannel` is unavailable before Node 18.19 so do nothing in that case. | ||
| if (!diagnosticsChannel.tracingChannel) { |
Member
There was a problem hiding this comment.
It seems like this should be a guard at a higher level (not objecting to it here, and can definitely be a follow-up PR).
Like, if we don't have dc.tracingChannel, then we kind of have to use the OTel integration, or nothing, right?
MemberAuthor
There was a problem hiding this comment.
Okay I think I can follow up with another PR for this.
Uh oh!
There was an error while loading. Please reload this page.
…ngChannelToSpan The mysql subscriber hand-rolled the whole channel lifecycle: a span WeakMap, a parent-scope WeakMap, manual callback re-wrapping to restore the caller's context, and per-path span ending. Most of that is what the helper already does — now that it restores caller context on `asyncStart`, the manual re-wrap can go. Add a `deferSpanEnd` option to the helper: return `true` to take ownership of ending the span so it isn't ended on `end`/`asyncEnd`. mysql uses it for the one path the helper can't model — `query(sql)` with no callback returns a streamed `Query` emitter that settles via its own `'end'`/`'error'` events, not the channel. That path keeps `bindScopeToEmitter` so user listeners nest correctly. Net: the callback and sync-throw paths are fully the helper's; only the streamed-emitter wiring stays mysql-specific.
The synthetic test published `start`/`asyncStart` with bare `publish`, which doesn't run bound stores — fine for the old subscriber that opened the span on `start`, but the span is now opened in the `start.bindStore` transform (only run via `runStores`). Drive it the way orchestrion's `wrapCallback` actually does so the bound store activates and the span opens.
…le parity
`deferSpanEnd` handed the caller the span and left them to end it, so the
streamed-mysql path reimplemented a thinner version of the lifecycle — bare
`setStatus`, no `error.type`, no `captureError`, no `beforeSpanEnd`.
Pass an `end(error?)` into `deferSpanEnd` instead: it owns *how* the span ends
(error status + attributes, captureError, beforeSpanEnd) and is idempotent, so
the deferred path can't drift from the channel lifecycle. The error annotation
is factored into a shared helper both paths use. mysql just wires the emitter:
`on('error', end)` / `on('end', end)`.
`deferSpanEnd` now takes a single object arg `{ span, data, end }`.logaretmforce-pushed
the
feat/mysql-tracing-channel-helper
branch
from
July 1, 2026 00:36
972275c to
4a057c8Compare
Comment on lines
+147
to
+149
| const span = data._sentrySpan; | ||
| if (span && deferSpanEnd?.({ span, data, end: makeDeferredEnd(span, data) })) { | ||
| return; |
Contributor
There was a problem hiding this comment.
Bug: For MySQL queries with callbacks, deferSpanEnd is called in both end and asyncEnd handlers, attaching duplicate listeners and causing span.end() to be called twice.
Severity: MEDIUM
Suggested Fix
Ensure that span deferral logic is only executed once per operation. This can be achieved by setting a flag on the data object after the first successful call to deferSpanEnd and checking for this flag in subsequent handlers to prevent re-registering listeners. For example, set data.span_deferred = true in the end handler and skip the deferSpanEnd call in the asyncEnd handler if this flag is present.
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/tracing-channel.ts#L147-L149
Potential issue: For callback-based MySQL queries, the `deferSpanEnd` function is
invoked in both the `end` and `asyncEnd` handlers. Each invocation creates a new
`makeDeferredEnd` closure with its own independent idempotency flag. Because the MySQL
driver returns a `Query` emitter synchronously which is stored on `ctx.result`, both
handlers successfully defer the span's end. This results in two separate sets of event
listeners being attached to the same `Query` emitter. When the query completes or
errors, both sets of listeners execute, leading to `span.end()` being called twice on
the same span, which can cause malformed trace data.
Also affects:
packages/server-utils/src/tracing-channel.ts:165~167
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 freeto 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.
Stacked on #21863.
For
mysqlit returns an event emitter, that signals the op end which doesn't fit inside a tracing channel lifecycle, For that I addeddeferSpanEndoption, which hands ownership back to the caller. mysql uses it to attach the emitter listeners and keepbindScopeToEmitterso user listeners on the stream still nest under the caller.There had to be a change to one test because it wasn't simulating a channel publish correctly, it needed to run the channel stores first before asserting.