Uh oh!
There was an error while loading. Please reload this page.
ref(node): Streamline mysql2 instrumentation#21509
Uh oh!
There was an error while loading. Please reload this page.
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -12,8 +12,10 @@ describe('mysql2 auto instrumentation', () => { | ||
| expect.objectContaining({ | ||
| description: 'SELECT 1 + 1 AS solution', | ||
| op: 'db', | ||
| origin: 'auto.db.otel.mysql2', | ||
| data: expect.objectContaining({ | ||
| 'db.system': 'mysql', | ||
| 'db.statement': 'SELECT 1 + 1 AS solution', | ||
| 'net.peer.name': 'localhost', | ||
| 'net.peer.port': 3306, | ||
| 'db.user': 'root', | ||
| @@ -22,13 +24,36 @@ describe('mysql2 auto instrumentation', () => { | ||
| expect.objectContaining({ | ||
| description: 'SELECT NOW()', | ||
| op: 'db', | ||
| origin: 'auto.db.otel.mysql2', | ||
| data: expect.objectContaining({ | ||
| 'db.system': 'mysql', | ||
| 'db.statement': 'SELECT NOW()', | ||
| 'net.peer.name': 'localhost', | ||
| 'net.peer.port': 3306, | ||
| 'db.user': 'root', | ||
| }), | ||
| }), | ||
| // `execute` is instrumented the same way as `query` | ||
| expect.objectContaining({ | ||
| description: 'SELECT 42 AS answer', | ||
| op: 'db', | ||
| origin: 'auto.db.otel.mysql2', | ||
| data: expect.objectContaining({ | ||
| 'db.system': 'mysql', | ||
| 'db.statement': 'SELECT 42 AS answer', | ||
| }), | ||
| }), | ||
| // a failing query produces a span with an error status | ||
| expect.objectContaining({ | ||
| 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 assertionMedium Severity The failing-query expectation uses   Reviewed by Cursor Bugbot for commit 33dea13. Configure here. | ||
| data: expect.objectContaining({ | ||
| 'db.system': 'mysql', | ||
| 'db.statement': 'SELECT * FROM does_not_exist', | ||
| }), | ||
| }), | ||
| ]), | ||
| }; | ||
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
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
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -18,75 +18,59 @@ | ||||||
| * - Upstream version: @opentelemetry/instrumentation-mysql2@0.64.0 | ||||||
| * - Types from 'mysql2' inlined as simplified interfaces | ||||||
| * - Minor TypeScript strictness adjustments for this repository's compiler settings | ||||||
| * - Refactored to use Sentry's span APIs instead of OpenTelemetry tracing APIs | ||||||
| */ | ||||||
| import { SpanKind } from '@opentelemetry/api'; | ||||||
| import type { InstrumentationConfig } from '@opentelemetry/instrumentation'; | ||||||
| import { InstrumentationBase, InstrumentationNodeModuleDefinition, isWrapped } from '@opentelemetry/instrumentation'; | ||||||
| import type { SpanAttributes } from '@sentry/core'; | ||||||
| import { SDK_VERSION, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, SPAN_STATUS_ERROR, startInactiveSpan } from '@sentry/core'; | ||||||
| import { InstrumentationNodeModuleFile } from '../../InstrumentationNodeModuleFile'; | ||||||
| import type { Connection, FormatFunction, Query, QueryError, QueryOptions } from './mysql2-types'; | ||||||
| import { ATTR_DB_STATEMENT, ATTR_DB_SYSTEM, DB_SYSTEM_VALUE_MYSQL } from './semconv'; | ||||||
| import { getConnectionAttributes, getConnectionPrototypeToInstrument, getQueryText, getSpanName, once } from './utils'; | ||||||
| const PACKAGE_NAME = '@sentry/instrumentation-mysql2'; | ||||||
| const ORIGIN = 'auto.db.otel.mysql2'; | ||||||
Member There was a problem hiding this comment. l: I don't think that we should add
Suggested change
Member 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) MemberAuthor 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. | ||||||
| const supportedVersions = ['>=1.4.2 <4']; | ||||||
| // The raw imported `mysql2` module exposes the `format` helper used to render | ||||||
| // parameterized queries. Typed shallowly since it is only read internally. | ||||||
| type MySQL2Module = { format?: FormatFunction; [key: string]: unknown }; | ||||||
| export class MySQL2Instrumentation extends InstrumentationBase<InstrumentationConfig> { | ||||||
| public constructor(config: InstrumentationConfig = {}) { | ||||||
| super(PACKAGE_NAME, SDK_VERSION, config); | ||||||
| } | ||||||
| protected init(): InstrumentationNodeModuleDefinition[] { | ||||||
| let format: FormatFunction | undefined; | ||||||
| function setFormatFunction(moduleExports: MySQL2Module): void { | ||||||
| if (!format && moduleExports.format) { | ||||||
| format = moduleExports.format; | ||||||
| } | ||||||
| } | ||||||
| const patch = (ConnectionPrototype: Connection): void => { | ||||||
| if (isWrapped(ConnectionPrototype.query)) { | ||||||
| this._unwrap(ConnectionPrototype, 'query'); | ||||||
| } | ||||||
| this._wrap(ConnectionPrototype, 'query', this._patchQuery(format) as any); | ||||||
| if (isWrapped(ConnectionPrototype.execute)) { | ||||||
| this._unwrap(ConnectionPrototype, 'execute'); | ||||||
| } | ||||||
| this._wrap(ConnectionPrototype, 'execute', this._patchQuery(format) as any); | ||||||
| }; | ||||||
| const unpatch = (ConnectionPrototype: Connection): void => { | ||||||
| this._unwrap(ConnectionPrototype, 'query'); | ||||||
| this._unwrap(ConnectionPrototype, 'execute'); | ||||||
| }; | ||||||
| return [ | ||||||
| new InstrumentationNodeModuleDefinition( | ||||||
| 'mysql2', | ||||||
| supportedVersions, | ||||||
| (moduleExports: MySQL2Module) => { | ||||||
| setFormatFunction(moduleExports); | ||||||
| return moduleExports; | ||||||
| }, | ||||||
| @@ -95,7 +79,7 @@ export class MySQL2Instrumentation extends InstrumentationBase<MySQL2Instrumenta | ||||||
| new InstrumentationNodeModuleFile( | ||||||
| 'mysql2/promise.js', | ||||||
| supportedVersions, | ||||||
| (moduleExports: MySQL2Module) => { | ||||||
| setFormatFunction(moduleExports); | ||||||
| return moduleExports; | ||||||
| }, | ||||||
| @@ -120,9 +104,9 @@ export class MySQL2Instrumentation extends InstrumentationBase<MySQL2Instrumenta | ||||||
| ]; | ||||||
| } | ||||||
| private _patchQuery(format: FormatFunction | undefined) { | ||||||
| const thisPlugin = this; | ||||||
| return (originalQuery: Function): Function => { | ||||||
| return function query( | ||||||
| this: Connection, | ||||||
| query: string | Query | QueryOptions, | ||||||
| @@ -135,61 +119,24 @@ export class MySQL2Instrumentation extends InstrumentationBase<MySQL2Instrumenta | ||||||
| } else if (arguments[2]) { | ||||||
| values = [_valuesOrCallback]; | ||||||
| } | ||||||
There was a problem hiding this comment. Error span status mismatches testMedium Severity Failed queries set the span status Additional Locations (1)Reviewed by Cursor Bugbot for commit fd777c9. Configure here. | ||||||
| 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, | ||||||
logaretm marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||||||
| attributes, | ||||||
| }); | ||||||
Comment on lines
-151
to
158
Contributor There was a problem hiding this comment. Bug: The Suggested FixTo correctly measure the full duration of the streaming query, the event listener should be bound to the Prompt for AI AgentDid we get this right? 👍 / 👎 to inform future reviews. MemberAuthor There was a problem hiding this comment. While this looks technically correct, we need to preserve the upstream instrumentation and track this individually later. | ||||||
| const endSpan = once((err?: QueryError | null) => { | ||||||
| if (err) { | ||||||
| span.setStatus({ code: SPAN_STATUS_ERROR, message: err.message }); | ||||||
| } | ||||||
| span.end(); | ||||||
| }); | ||||||
| @@ -204,8 +151,8 @@ export class MySQL2Instrumentation extends InstrumentationBase<MySQL2Instrumenta | ||||||
| .once('error', (err: any) => { | ||||||
| endSpan(err); | ||||||
| }) | ||||||
| .once('result', () => { | ||||||
| endSpan(); | ||||||
| }); | ||||||
| return streamableQuery; | ||||||
| @@ -222,11 +169,11 @@ export class MySQL2Instrumentation extends InstrumentationBase<MySQL2Instrumenta | ||||||
| }; | ||||||
| } | ||||||
| private _patchCallbackQuery(endSpan: (err?: QueryError | null) => void) { | ||||||
| return (originalCallback: Function) => { | ||||||
| return function (...args: [err: QueryError | null, ...rest: unknown[]]) { | ||||||
| endSpan(args[0]); | ||||||
| return originalCallback(...args); | ||||||
| }; | ||||||
| }; | ||||||
| } | ||||||
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
Oops, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Oops, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Oops, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
l: Is it necessary to add these (especially for all vendored integrations)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was added in #21481 so depending on whichever gets merged first the other one will drop, this is a one time change only.