ref(node): Streamline mysql2 instrumentation#21509
Conversation
size-limit report 📦
|
logaretm
requested review from
JPeer264,
andreiborza,
mydea and
nicohrubec
and removed request for
a team
| "rules": { | ||
| "typescript/no-explicit-any": "off" | ||
| "typescript/no-explicit-any": "off", | ||
| "no-unsafe-member-access": "off", |
There was a problem hiding this comment.
l: Is it necessary to add these (especially for all vendored integrations)?
There was a problem hiding this comment.
It was added in #21481 so depending on whichever gets merged first the other one will drop, this is a one time change only.
| } from '@opentelemetry/semantic-conventions'; | ||
|
|
||
| const PACKAGE_NAME = '@sentry/instrumentation-mysql2'; | ||
| const ORIGIN = 'auto.db.otel.mysql2'; |
There was a problem hiding this comment.
l: I don't think that we should add otel in it, as it comes from us - but it was here before - so it might be better to keep it as is and change it later?
| const ORIGIN = 'auto.db.otel.mysql2'; | |
| const ORIGIN = 'auto.db.mysql2'; |
There was a problem hiding this comment.
I think I'd opt to change all of these to something non-otel in v11, for now this indicates that it comes from otel instrumentation, which it still is (just our own)
There was a problem hiding this comment.
Yea, my goal was not to change anything in terms of output. So I try to keep all attrs intact.
| @@ -0,0 +1,260 @@ | |||
| /* | |||
| * The upstream @opentelemetry/instrumentation-mysql2 suite runs against a real | |||
There was a problem hiding this comment.
m: should we move the tests requiring a fake to real integration tests?
There was a problem hiding this comment.
I think in this instrumentation case we can move most of the test cases to a real one. Will do that.
Refactors the vendored mysql2 instrumentation to use Sentry's span APIs instead of the OpenTelemetry tracing API, and removes the config paths that are dead in Sentry's context. - Replace `tracer.startSpan` with `startInactiveSpan`. mysql2's `query`/ `execute` complete via stream events or a patched callback that fire after the synchronous wrapper returns, so the span must outlive a sync scope and is ended manually. `startSpan`/`startSpanManual` are unusable here: the returned `Query` is thenable-but-not-a-Promise, and its core `.then` is a trap that throws, so the promise-probing in `handleCallbackErrors` would throw on every query. - Bake the origin in via `SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN` instead of an `index.ts` responseHook. - Drop the `OTEL_SEMCONV_STABILITY_OPT_IN` dual-emission and the unused `responseHook`, query masking, and SQL-commenter config. The integration only ever passed the origin responseHook, so these were unreachable. mysql2 spans now always emit the legacy semconv attributes (`db.system`, `db.statement`, `net.peer.*`); op is derived downstream. - Drop the blanket eslint-disable, type the module exports shallowly. - Add unit tests against a fake connection and assert origin/db.statement in the integration suite. Fixes #20739
… test The fake-connection unit test used a plain EventEmitter/callback, which is not thenable and so masked mysql2's real thenable `Query` behavior. Move that coverage to the real-package integration suite instead: - scenario now also runs `execute` and a failing query - integration test asserts the execute span and the errored span's `internal_error` status Replace the fake patch test with a focused pure-function unit test for `getConnectionPrototypeToInstrument`, the only version-sensitive logic (own-prototype vs. inherited base-class layout across mysql2 majors).
logaretm
force-pushed
the
awad/js-2391-streamline-opentelemetryinstrumentation-mysql2
branch
from
7da4a80 to
f03121f
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
DB span descriptions are set to db.statement by parseSpanDescription, not the getSpanName value. The new execute/error matchers asserted 'SELECT'; correct them to the full SQL like the existing matchers.
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 33dea13. Configure here.
| description: 'SELECT * FROM does_not_exist', | ||
| op: 'db', | ||
| status: 'internal_error', | ||
| origin: 'auto.db.otel.mysql2', |
There was a problem hiding this comment.
Wrong error status assertion
Medium Severity
The failing-query expectation uses status: 'internal_error', but instrumentation sets span status with message: err.message. For a missing table, MySQL supplies a concrete message, so getStatusMessage returns that text rather than internal_error.
Reviewed by Cursor Bugbot for commit 33dea13. Configure here.
4 participants
Streamlines the vendored
mysql2instrumentation to use Sentry's span APIs instead of the OpenTelemetry tracing API, and removes the code paths that are dead in Sentry's context.Mirrors the approach in #21481 (mongoose).
Notes
Blockers for
startSpan*mysql2 ends its span manually since completion fires via stream events/callbacks after the sync wrapper returns, which
startSpan's auto-end misses. Worse, the returnedQueryis thenable but not a real Promise with a.thenthat throws, sostartSpan/startSpanManualwould throw on every query.startInactiveSpanleaves theQueryuntouched.Semantic Conventions Attributes
Dropping the
OTEL_SEMCONV_STABILITY_OPT_INpath means mysql2 spans now always emit the legacy semantic-convention attributes (db.system,db.statement,db.connection_string,db.name,db.user,net.peer.*).Modernizing the semconv is deferred as a separate, breaking change.
Fixes #20739