feat(server-utils): Migrate @opentelemetry/instrumentation-mysql2 to orchestrion#22229
Conversation
mydea
requested review from
JPeer264,
andreiborza and
isaacs
and removed request for
a team
size-limit report 📦
|
|
|
||
| const INTEGRATION_NAME = 'Mysql2' as const; | ||
| const ORIGIN = 'auto.db.orchestrion.mysql2'; | ||
| const DB_SYSTEM_VALUE_MYSQL = 'mysql'; |
There was a problem hiding this comment.
Note: I decided to drop db.connection_string as we just build this from other attributes, so it is redundant information. we have this in some other places, e.g. mongo, postgres, ioredis, we can either drop those while migrating or also drop it in v11, both are fine IMHO.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| ...(host ? { [NET_PEER_NAME]: host } : {}), | ||
| // oxlint-disable-next-line typescript/no-deprecated | ||
| ...(portIsNumber ? { [NET_PEER_PORT]: portNumber } : {}), | ||
| }; |
There was a problem hiding this comment.
Missing JDBC connection string attribute
Medium Severity
The orchestrion mysql2 span attributes omit db.connection_string, while the vendored @opentelemetry/instrumentation-mysql2 patcher and the orchestrion mysql integration always emit a JDBC-style connection string. Opt-in orchestrion users lose that attribute on query and execute spans for mysql2 versions below 3.20.0.
Reviewed by Cursor Bugbot for commit be9e15d. Configure here.
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 2eb8253. Configure here.
| origin: ORIGIN, | ||
| data: expect.objectContaining({ | ||
| 'db.system': 'mysql', | ||
| 'db.statement': 'SELECT 42 AS scalar_value', |
There was a problem hiding this comment.
Just a general question (out of scope for this PR): But db.statement/db.query.text should ideally be sanitized. However, this statement includes the actual value 42. Is it possible to sanitize/parametrize this?
Same for description
There was a problem hiding this comment.
this is actually parametrized when it is parametrized in the code - here the test was wrong/failing, I just fixed this ^^
| }; | ||
| } | ||
|
|
||
| const _mysql2ChannelIntegration = (() => { |
There was a problem hiding this comment.
This integration probably also needs to be added to Deno: https://github.com/getsentry/sentry-javascript/tree/develop/packages/deno/src/integrations
(if mysql2 is expected to work on Deno)
There was a problem hiding this comment.
And if this needs a Deno wrapper: this should have a guard (usually done with a closure like in redis) so it's not running twice.
There was a problem hiding this comment.
I would keep this in a separate/follow up PR IMHO, we do not need to do this at the same time :)
There was a problem hiding this comment.
I wonder if rather than one-off each Deno integration, there's a way to have the Deno SDK automatically slurp these all up. They're all pretty much identical, after all, and exported by server-utils. 🤔
mydea
force-pushed
the
francesconovy/js-2414-rewrite-opentelemetryinstrumentation-mysql2-to-orchestrion
branch
from
2eb8253 to
bf47d65
Compare
|
|
||
| return { | ||
| // oxlint-disable-next-line typescript/no-deprecated | ||
| ...(database ? { [DB_NAME]: database } : {}), |
There was a problem hiding this comment.
super-l: Following is better to read (same for the ones below:
| ...(database ? { [DB_NAME]: database } : {}), | |
| [DB_NAME]: database || undefined, |
or the following if we don't care about falsy values:
| ...(database ? { [DB_NAME]: database } : {}), | |
| [DB_NAME]: database, |
| // bind values are left as `?` placeholders in `db.statement` (not inlined) | ||
| expect.objectContaining({ | ||
| description: 'SELECT NOW()', | ||
| description: 'SELECT ? as a, ? as b, NOW() as c', |
| }; | ||
| } | ||
|
|
||
| const _mysql2ChannelIntegration = (() => { |
There was a problem hiding this comment.
I wonder if rather than one-off each Deno integration, there's a way to have the Deno SDK automatically slurp these all up. They're all pretty much identical, after all, and exported by server-utils. 🤔
…o orchestrion Rewrites the mysql2 instrumentation from an OTel `InstrumentationBase` patcher to a diagnostics-channel listener with orchestrion injecting the channel into the instrumented module, mirroring the mysql/kafkajs migrations. Adds an opt-in `mysql2ChannelIntegration` (registered in `channelIntegrations`, so `@sentry/node`/`@sentry/bun` swap it in for the OTel `Mysql2` integration when diagnostics-channel injection is enabled). It subscribes to the injected `orchestrion:mysql2:query`/`:execute` channels for mysql2 < 3.20.0 and to mysql2's native tracing channels for >= 3.20.0, so the two ranges never overlap. The orchestrion config uses `kind: 'Callback'` rather than `'Auto'`: `Auto`'s no-callback branch probes the return value for `.then`, but mysql2's callback-less `query(sql)` returns a `Query` whose `.then()` throws, which would crash streamed queries. `Callback` traces the callback and promise forms (the common case) and leaves the rarely-used row-streaming form untouched. Fixes #20763 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mydea
force-pushed
the
francesconovy/js-2414-rewrite-opentelemetryinstrumentation-mysql2-to-orchestrion
branch
from
bf47d65 to
fbe6507
Compare
mydea
deleted the
francesconovy/js-2414-rewrite-opentelemetryinstrumentation-mysql2-to-orchestrion
branch
5 participants
Rewrites the
mysql2instrumentation from an OpenTelemetryInstrumentationBasepatcher to anode:diagnostics_channellistener, with orchestrion injecting the channel into the instrumented module ..(#21923) migrations.Connectioninlib/connection.jsfor< 3.11.5;BaseConnectioninlib/base/connection.jsfor>= 3.11.5).Fixes #20763
🤖 Generated with Claude Code