feat: pg orchestrion instrumentation#21826
Conversation
isaacs
requested review from
JPeer264,
andreiborza and
mydea
and removed request for
a team
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
🚲 🏠 API surface choice: In order to opt into orchestrion diagnostics-channel-injection style instrumentations, which swap out the vendored OTel instrumentations, we call This initializes the orchestrion integrations, and uses them instead of the OTel ones. However, this means that the only time that the orchestrion integrations are set up and provided with options, is at that moment of calling Sentry.experimentalUseDiagnosticsChannelInjection({ postgres: { ignoreConnectSpans: true } });I don't love this! It exposes the implementation detail in a way that we will likely regret. I kind of hate it, actually.
It was a simple duct-tape way forward, but it sucks. Another approach would be, instead of actually initializing these integrations at that moment to swap in for the OTel ones, just mark that we're going to use the orchestrion ones when we do initialize, and then when it comes time to initialize them, choose which implementation to use, with the configurations provided. I'm going to try spiking that out here. EDIT: yeah, fixed it. Much nicer. The |
isaacs
force-pushed
the
isaacs/pg-orchestrion
branch
from
fbbe3ae to
334614a
Compare
size-limit report 📦
|
isaacs
force-pushed
the
isaacs/pg-orchestrion
branch
from
9545bf8 to
dfe74f5
Compare
JPeer264
left a comment
JPeer264
left a comment
There was a problem hiding this comment.
Overall LGTM, got minor suggestions. I'll review once more after the tests are green and the merge conflicts got solved
| - name: Set up Deno | ||
| if: | ||
| matrix.test-application == 'deno' || matrix.test-application == 'deno-streamed' || matrix.test-application == | ||
| 'deno-redis' || matrix.test-application == 'hono-4' || matrix.test-application == 'deno-mysql' |
There was a problem hiding this comment.
q: I wonder if using startsWith(matrix.test-application, 'deno-') would be better at this point
| "devDependencies": { | ||
| "mysql": "^2.18.1" | ||
| "mysql": "^2.18.1", | ||
| "pg": "8.16.0" |
There was a problem hiding this comment.
l/m: I think a good time to revisit this comment: #21451 (comment)
|
|
||
| export const denoPostgresIntegration = defineIntegration(_denoPostgresIntegration) as ( | ||
| options?: DenoPostgresIntegrationOptions, | ||
| ) => Integration & { |
There was a problem hiding this comment.
m: Not sure where I've read it, but I think we try to not add a custom type in integrations
|
|
||
| // OpenTelemetry "OLD" db/net semantic-conventions, inlined to keep this | ||
| // integration free of `@opentelemetry/*` deps. | ||
| const ATTR_DB_SYSTEM = 'db.system'; |
There was a problem hiding this comment.
l/m: We could use the new attributes from the @sentry/conventions/attributes already for all that exist. In this case DB_SYSTEM_NAME, then we have already some migrated
| return { | ||
| [ATTR_DB_SYSTEM]: DB_SYSTEM_POSTGRESQL, | ||
| [ATTR_DB_CONNECTION_STRING]: getConnectionString(params), | ||
| ...(params.database ? { [ATTR_DB_NAME]: params.database } : {}), |
There was a problem hiding this comment.
l: AFAIK undefined values will be removed, so maybe we can just write the following (not sure about empty strings though)? (same for other occurrences)
| ...(params.database ? { [ATTR_DB_NAME]: params.database } : {}), | |
| [ATTR_DB_NAME]: params.database, |
`bindTracingChannelToSpan` only bound the span store on `start`, which covers the synchronous frame but not a callback the library dispatches from a detached async context (e.g. a socket data handler or a `setImmediate` drain). There, native async-context propagation no longer reaches the caller, so work issued inside the callback lost its parent. Stash the caller's store at `start` and re-bind it on `asyncStart`, so callback-style channels run their continuation in the caller's context — the same way a promise's `.then` does natively. It's inert for promise channels, which `publish` `asyncStart` rather than `runStores` it. Migrate the lru-memoizer subscriber onto the helper (`getSpan` returns `undefined`, so no span is created — it only needs the context rebind), dropping its hand-rolled callback re-wrapping.
… caller context The asyncStart producer used `data._sentryCallerStore ?? asyncLocalStorage.getStore()`. When the caller had no active store, `_sentryCallerStore` is `undefined` and the fallback bound whatever was ambient at callback time — for a callback dispatched from a pooled socket handler, that can be another request's store. `start` always runs first and sets `_sentryCallerStore`, so return it verbatim (no fallback), matching the synchronous path: no caller context restores to none, not a foreign one.
…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 }`.
Add orchestrion instrumentation for Node, Deno, and Bun, covering the `pg` module. This basically copies exactly what the `mysql` integration does, but for postgres. fix: #20764 fix: JS-2415
This resolves the API surface wart where options for orchestrion integrations had to be passed into the `experimentalUseDiagnosticsChannelInjection` method, and returns that to being an argument-free void-returning opt-in that can be easily no-op'ed in the future, and all options are passed in via `Sentry.init()` as they were in the prior OTel implementation.
isaacs
force-pushed
the
isaacs/pg-orchestrion
branch
from
dfe74f5 to
373c014
Compare
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.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 373c014. Configure here.
| ...(hasSpansEnabled(options) ? getAutoPerformanceIntegrations() : []), | ||
| ]; | ||
|
|
||
| // When the app opted into diagnostics-channel injection (via |
There was a problem hiding this comment.
LruMemoizer orchestrion swap missing
Medium Severity
After moving orchestrion opt-in to per-integration factories, lruMemoizerIntegration still always installs OpenTelemetry instrumentation and never consults getChannelIntegrationFactory, even though experimentalUseDiagnosticsChannelInjection registers LruMemoizer: lruMemoizerChannelIntegration. With orchestrion enabled, lru-memoizer channel hooks are injected but not subscribed, while OTel patching may still run.
Reviewed by Cursor Bugbot for commit 373c014. Configure here.
|
|
||
| if (opts?.captureError) { | ||
| captureException(data.error, getErrorHint(data.error)); | ||
| } | ||
|
|
||
| const { message, attributes } = getErrorInfo(data.error); | ||
| span.setStatus({ code: SPAN_STATUS_ERROR, message }); | ||
| span.setAttributes(attributes); | ||
| annotateSpanError(span, data.error); | ||
| }, | ||
| asyncEnd(data) { | ||
| const span = data._sentrySpan; | ||
| if (span && deferSpanEnd?.({ span, data, end: makeDeferredEnd(span, data) })) { | ||
| return; | ||
| } | ||
| endBoundSpan(data, beforeSpanEnd); | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Bug: Both end and asyncEnd subscribers attach event listeners for promise-based emitter results, leading to duplicate listener registration on the same emitter.
Severity: MEDIUM
Suggested Fix
Prevent the asyncEnd subscriber from calling deferSpanEnd if the end subscriber has already handled it. This can be achieved by storing a flag on the subscriber's data object to indicate that deferral has already occurred, ensuring listeners are attached only once.
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#L144-L171
Potential issue: For promise-based operations that return an event emitter, both the
`end` and `asyncEnd` subscribers in `tracing-channel.ts` are invoked. Each of these
subscribers calls `deferSpanEnd`, which then proceeds to attach `'error'` and `'end'`
listeners to the emitter. This results in two identical pairs of event listeners being
registered on the same emitter. While idempotency checks prevent the span from being
ended multiple times, this redundant listener registration is inefficient, can trigger
`MaxListenersExceededWarning` in Node.js, and may cause any configured `beforeSpanEnd`
hook to execute twice.
Also affects:
packages/server-utils/src/integrations/tracing-channel/postgres.ts:138~155
3 participants
Add orchestrion instrumentation for Node, Deno, and Bun, covering the
pgmodule.This basically copies exactly what the
mysqlintegration does, but for postgres.fix: #20764
fix: JS-2415