feat(node): Rewrite knex instrumentation to orchestrion tracing channels#22237
feat(node): Rewrite knex instrumentation to orchestrion tracing channels#22237logaretm wants to merge 1 commit into
Conversation
size-limit report 📦
|
logaretm
force-pushed
the
awad/js-2408-rewrite-opentelemetryinstrumentation-knex-to-orchestrion
branch
3 times, most recently
from
fea4e2e to
f04f9e0
Compare
logaretm
requested review from
andreiborza,
isaacs and
mydea
and removed request for
a team
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Migrate the knex integration off the vendored `InstrumentationBase` monkey-patch onto a `node:diagnostics_channel` subscriber whose channels are injected by the orchestrion code transform. The OTel path stays as the fallback when orchestrion isn't injected. knex is opt-in (not a default performance integration), so the `@sentry/node` factory picks the path itself in `setupOnce` via `isOrchestrionInjected()` rather than the central `channelIntegrations` swap. `Runner.query` produces the span; `Client.queryBuilder`/`schemaBuilder`/`raw` are wrapped for bookkeeping so the span active at builder-creation time is reused as the parent when the query runs later. Fixes #20757
logaretm
force-pushed
the
awad/js-2408-rewrite-opentelemetryinstrumentation-knex-to-orchestrion
branch
from
f04f9e0 to
189751c
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 189751c. Configure here.
| // fall back to the vendored OTel instrumentation. `isOrchestrionInjected()` is only reliable by | ||
| // `setupOnce` (the runtime injection runs during `Sentry.init()`, after integrations are built). | ||
| if (isOrchestrionInjected()) { | ||
| knexChannelIntegration().setupOnce?.(); |
There was a problem hiding this comment.
Orchestrion import breaks tree-shaking
Medium Severity
knexIntegration now statically imports @sentry/server-utils/orchestrion, and that barrel eagerly loads every channel subscriber. Because knexIntegration is re-exported from @sentry/node's public entry, ordinary import * as Sentry from '@sentry/node' pulls in the full orchestrion subscriber set even when knex and diagnostics-channel injection are unused. That undercuts the tree-shaking boundary documented for experimentalUseDiagnosticsChannelInjection. Flagged because it was mentioned in this rules file.
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit 189751c. Configure here.
| op: 'db', | ||
| parentSpan, | ||
| attributes, | ||
| }); |
There was a problem hiding this comment.
Span name omits SQL statement
Medium Severity
Query spans are started with a getName(...) title such as select tests.User instead of the SQL text. Sibling orchestrion DB integrations set the span name from the statement because applyOtelSpanData remapping only runs on the Node OTel path. Without that remapping, exported knex span descriptions stay as the synthetic name rather than the query.
Reviewed by Cursor Bugbot for commit 189751c. Configure here.
| // Prefer the diagnostics-channel subscriber when orchestrion injected its channels; otherwise | ||
| // fall back to the vendored OTel instrumentation. `isOrchestrionInjected()` is only reliable by | ||
| // `setupOnce` (the runtime injection runs during `Sentry.init()`, after integrations are built). | ||
| if (isOrchestrionInjected()) { |
There was a problem hiding this comment.
is this needed, I think other integrations (at least the ones I migrated) did not use this pattern?
2 participants
Migrates the knex integration off the vendored
InstrumentationBasemonkey-patch onto anode:diagnostics_channelsubscriber whose channels are injected by the orchestrion code transform. The vendored OTel path stays as the fallback when orchestrion isn't injected.closes #20757