ref(node): Streamline knex instrumentation#21561
Open
logaretm wants to merge 2 commits into
Open
Conversation
Refactors the vendored knex instrumentation off the OpenTelemetry tracing APIs onto Sentry's span APIs, mirroring the mongoose (#21481) and mysql2 (#21509) streamlines. - Replace `tracer.startSpan` + manual `context.with`/`.then`/`.catch` with `startSpan`. `Runner.query` returns a real, already-executing Promise, so `startSpan` can safely await it and auto-end the span while keeping it active so the underlying `pg`/`mysql2` driver spans nest correctly. - Preserve the build-time `contextSymbol` parent + require-parent-span behavior (only instrument queries that run within an existing trace). - Bake the `auto.db.otel.knex` origin into the span attributes and drop the `spanStart` hook from `index.ts`. - Drop the env-gated `OTEL_SEMCONV_STABILITY_OPT_IN` dual-emission; only the OLD semantic conventions (`db.system`, `db.statement`, ...) were ever emitted. Behavior change: the unsupported stable-semconv opt-in is no longer honored. - Hardcode `requireParentSpan`/`maxQueryLength` (the integration only ever used the defaults), delete the now-dead `types.ts`/`constants.ts` and `otelExceptionFromKnexError`, drop OTel `recordException`, and remove the blanket `eslint-disable`. - Extend the `pg` integration suite with a failing query to cover the error path (`status: internal_error`).
logaretm
force-pushed
the
awad/js-2385-streamline-opentelemetryinstrumentation-knex
branch
from
125fe11 to
a32abea
Compare
logaretm
requested review from
JPeer264,
andreiborza,
mydea and
nicohrubec
and removed request for
a team
![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 a32abea. Configure here.
nicohrubec
reviewed
Jun 17, 2026
nicohrubec
left a comment
nicohrubec
left a comment
Member
There was a problem hiding this comment.
if we can easily already replace the context.with would be good to get that in here as well I think, but else lgtm
| }, | ||
| parentContext, | ||
| const args = arguments; | ||
| return api.context.with(parentContext, () => |
Member
There was a problem hiding this comment.
l: it would be great if we could already rewrite this with Sentry APIs as well
Member
Author
There was a problem hiding this comment.
Nice catch, I did not see that
Replace api.context/api.trace parent detection with getActiveSpan +
startSpan({ parentSpan, onlyIfParent }), and inline the postgresql
semconv constant locally.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Comment on lines
+149
to
159
| const parentSpan: Span | undefined = this.builder[parentSpanSymbol] || getActiveSpan(); | ||
|
|
||
| const args = arguments; | ||
| return startSpan( | ||
| { | ||
| kind: api.SpanKind.CLIENT, | ||
| name: utils.getName(name, operation, table), | ||
| kind: SpanKind.CLIENT, | ||
| attributes, | ||
| parentSpan, | ||
| onlyIfParent: true, | ||
| }, |
Contributor
There was a problem hiding this comment.
Bug: Passing an ended span as parentSpan to startSpan bypasses the onlyIfParent: true guard, because the ended span is set as active before the guard is checked.
Severity: MEDIUM
Suggested Fix
Before calling startSpan, validate that the parentSpan is still valid. Reintroduce a check similar to the old implementation, using api.trace.isSpanContextValid(parentSpan.spanContext()) to ensure the span from this.builder[parentSpanSymbol] has not ended before using it as a parent.
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/node/src/integrations/tracing/knex/vendored/instrumentation.ts#L149-L159
Potential issue: The `knex` instrumentation stores a parent span when a query builder is
created. This span can be ended by the time the query executes later in a different
async context. This ended span is passed as the `parentSpan` option to `startSpan` with
`onlyIfParent: true`. However, the `startSpan` implementation sets this explicit
`parentSpan` as the active span *before* checking the `onlyIfParent` condition. As a
result, the check finds the (ended) span and incorrectly proceeds, creating a new span
as a child of an ended one, which breaks the trace hierarchy. This replaces a previous
implementation that correctly validated the span context with `isSpanContextValid`.
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.
Streamlines the vendored knex instrumentation by moving it off the OpenTelemetry tracing APIs onto Sentry's span APIs.
In this integration it was possible to use
startSpandirectly as all APIs seem to be promise based and there is no risk of lazy thenables as far as I can tell.