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.
| 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.
| responseHook?: MySQL2InstrumentationExecutionResponseHook; | ||
| addSqlCommenterCommentToQueries?: boolean; | ||
| } | ||
| export type MySQL2InstrumentationConfig = InstrumentationConfig; |
There was a problem hiding this comment.
m: maybe let's just get rid of this file since it's now basically empty? 😅
The vendored types.ts had collapsed to a single re-export alias (MySQL2InstrumentationConfig = InstrumentationConfig). Use InstrumentationConfig directly in the instrumentation and delete the file.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit fd777c9. Configure here.
| values = [_valuesOrCallback]; | ||
| } | ||
| const { maskStatement, maskStatementHook, responseHook } = thisPlugin.getConfig(); | ||
|
|
There was a problem hiding this comment.
Error span status mismatches test
Medium Severity
Failed queries set the span status message to the raw MySQL err.message, while the new integration test expects status: 'internal_error'. Serialized span status uses that message verbatim, so the assertion and runtime behavior introduced in this change do not align.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit fd777c9. Configure here.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| attributes[ATTR_DB_SYSTEM_NAME] = DB_SYSTEM_NAME_VALUE_MYSQL; | ||
| attributes[ATTR_DB_QUERY_TEXT] = dbQueryText; | ||
| } | ||
|
|
||
| const span = thisPlugin.tracer.startSpan(getSpanName(query), { | ||
| kind: api.SpanKind.CLIENT, | ||
| const attributes: SpanAttributes = { | ||
| ...getConnectionAttributes(this.config), | ||
| [ATTR_DB_SYSTEM]: DB_SYSTEM_VALUE_MYSQL, | ||
| [ATTR_DB_STATEMENT]: getQueryText(query, format, values), | ||
| [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: ORIGIN, | ||
| }; | ||
|
|
||
| const span = startInactiveSpan({ | ||
| name: getSpanName(query), | ||
| kind: SpanKind.CLIENT, | ||
| attributes, | ||
| }); |
There was a problem hiding this comment.
Bug: The mysql2 instrumentation incorrectly uses the 'result' event to end spans for streaming queries, causing premature span closure for multi-row queries and span leaks for zero-row queries.
Severity: HIGH
Suggested Fix
To correctly measure the full duration of the streaming query, the event listener should be bound to the 'end' event instead of the 'result' event. Change .once('result', () => { endSpan(); }); to .once('end', () => { endSpan(); });. This ensures the span is only closed after all rows have been received.
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/mysql2/vendored/instrumentation.ts#L151-L158
Potential issue: The `mysql2` instrumentation for streaming queries incorrectly uses the
`'result'` event to end a trace span. The `'result'` event fires for each row returned
by the query. For queries that return multiple rows, this causes the span to end
prematurely after the first row, resulting in an inaccurate duration measurement. For
queries that return zero rows, the `'result'` event never fires, causing the span to
never be closed and leading to a resource leak.
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
While this looks technically correct, we need to preserve the upstream instrumentation and track this individually later.
logaretm
deleted the
awad/js-2391-streamline-opentelemetryinstrumentation-mysql2
branch
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