ref(node): Streamline knex instrumentation#21561
Conversation
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}
nicohrubec
left a comment
nicohrubec
left a comment
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, () => |
There was a problem hiding this comment.
l: it would be great if we could already rewrite this with Sentry APIs as well
There was a problem hiding this comment.
Nice catch, I did not see that
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
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`).
Replace api.context/api.trace parent detection with getActiveSpan +
startSpan({ parentSpan, onlyIfParent }), and inline the postgresql
semconv constant locally.
logaretm
force-pushed
the
awad/js-2385-streamline-opentelemetryinstrumentation-knex
branch
from
e3cc97a to
63c3a48
Compare
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 216196d. Configure here.
| return startSpan( | ||
| { | ||
| kind: api.SpanKind.CLIENT, | ||
| name: utils.getName(name, operation, table), |
There was a problem hiding this comment.
Span description no longer SQL
Medium Severity
Knex spans now use utils.getName as the span name, which becomes the exported description. The prior OTEL exporter derived descriptions from db.statement, so UI and tests expecting full SQL (for example select * from "User") no longer match.
Reviewed by Cursor Bugbot for commit 216196d. Configure here.
| const fullQuery = formatter(query.sql, query.bindings || []); | ||
| const message = err.message.replace(`${fullQuery} - `, ''); | ||
| span.setStatus({ code: SPAN_STATUS_ERROR, message }); | ||
| throw err; |
There was a problem hiding this comment.
Error status not normalized
Medium Severity
Failed queries set the span status message to the trimmed Knex/Postgres error text. OTEL export previously mapped non-canonical error messages to internal_error, but native spans keep the raw message, diverging from tests and prior telemetry.
Reviewed by Cursor Bugbot for commit 216196d. Configure here.
2 participants
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.