feat: add TracingChannel support for native APM instrumentation#4178
feat: add TracingChannel support for native APM instrumentation#4178wellwelwel merged 10 commits intosidorares:masterfrom
Conversation
Add first-class `TracingChannel` (`node:diagnostics_channel`) support so APM tools (OTEL, Datadog, Sentry) can instrument mysql2 without monkey-patching via IITM/RITM. Four tracing channels are implemented: - `mysql2:query` — traces `connection.query()` (callback and event-emitter modes) - `mysql2:execute` — traces `connection.execute()` prepared statements - `mysql2:connect` — traces initial connection handshake - `mysql2:pool:connect` — traces pool connection acquisition Context fields map to OTEL semantic conventions (`db.query.text`, `db.namespace`, `server.address`, `server.port`). Zero-cost when no subscribers are registered. Graceful degradation on Node versions without `TracingChannel` (e.g. Node 18). Follows the pattern established by undici (Node core), node-redis, ioredis, and pg. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Sorry about the Copilot spam. I see you're using Claude, my suggestion would be you to report the errors to it, so it can investigate what caused the current code to break before continuing 🙋🏻♂️ For example: https://github.com/sidorares/node-mysql2/actions/runs/23071036612/job/67024378058?pr=4178 |
|
@logaretm, do you think the Pool Cluster should have its own channel, or could that conflict with the Pool’s existing channel? |
- Short-circuit query()/execute() to original code path when no tracing subscribers exist — zero behavioral change for non-instrumented users - Use origCb.call(cmdQuery, ...) to preserve the command instance as `this` in callbacks, matching mysql2's existing contract - Clean up unused connect/error listener in handshake tracing to prevent listener leaks Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Yea, no worries i directed it to the failures. For the pool cluster, the individual pool connections already get traced via I don't have a strong opinion on this, OTEL didn't instrument clusters but usually the more channels the merrier as if the APM doesn't subscribe to it then no overhead is introduced. What do you think? |
I think the current channels are already good. Thanks for the example. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4178 +/- ##
==========================================
- Coverage 90.44% 90.32% -0.13%
==========================================
Files 86 87 +1
Lines 13989 14280 +291
Branches 1733 1788 +55
==========================================
+ Hits 12653 12898 +245
- Misses 1336 1382 +46
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…ilable poku's describe.skip still executes test bodies, causing failures on Node 18. Use process.exit(0) instead. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…testability - Replace process.exit(0) with poku's skip() for better test reporting - Export dc and hasTracingChannel from tracing.js for testing - Add unit test for tracing module (dc availability, channel setup, getServerContext) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
logaretm
force-pushed
the
feat/tracing-channel-support
branch
from
d294092 to
cd7d6f3
Compare
|
Looks like the skip isn't kicking in, I will check it locally with node 18 |
It seems that the function itself is available, but not all of the necessary implementations are in place. I recommend something like: if (version <= 18) { // from common.test.js
skip('TracingChannel requires Node 19.9+ / 20+');
} |
|
I see what's throwing it off, the specific node version in the tests does have @wellwelwel should we strictly adhere to zero cost here and require |
Since this would be a limitation of Node.js versions rather than of MySQL2 itself, I would choose to make the option unavailable for versions with "incomplete" support. This could also be documented as |
|
The overhead will be minimal anyways since if there are no subscribers then nothing really happens. Even propagation context stores would only run if only there are subscribers and then that's okay. So it's an optimization that exists for newer node versions, but older node won't have it. But happy to stick to all or nothing approach here if you think that's best. |
I'm fine with the current approach, as long as this is documented. Once all the tests pass, I'll take a closer look at the implementation 🙋🏻♂️ |
|
For the documentation, I believe this feature should have its own section, similar to how we handle the use of Cloudflare Worker, for example: |
Node 18.x backported tracingChannel but the aggregated hasSubscribers getter on TracingChannel is always undefined. Only skip tracing when hasSubscribers is explicitly false — undefined means we can't tell, so trace unconditionally (zero-cost optimization only on Node 20+). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
tracePromise wraps operations in promises, which defers callbacks to microtasks and breaks timing for existing tests. traceCallback calls the function synchronously and wraps the callback to emit async lifecycle events — no promise allocation, no timing change. - query/execute callback mode: use traceCallback - query/execute event-emitter mode: keep tracePromise (no callback) - connect handshake: keep tracePromise (event-based) - pool getConnection: use traceCallback - shouldTrace: fall back to channel.start.hasSubscribers on Node 18 where the aggregated getter is missing Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@wellwelwel should the docs be in this PR as well, or in a separate PR? |
Here, please 🙋🏻♂️
|
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Yea I think I see what you want to do there! Sounds Good 🙌
I didn't expect that, thanks for pointing it out! I thought just replacing it with a wrapper would preserve the API.
Yes ofc, LMK if you need any help! |
|
After refactoring the potential breaking change I mentioned about I finished all the minor fixes and will probably release the new version on Monday:
Thanks again, @logaretm 🙋🏻♂️ |
|
Many thanks @wellwelwel ❤️ this goes a long way into reducing the ecosystem reliance on Do you mind using the work done here as an example I can point out for similar libraries? |
Please feel free 🤝 |
|
Did this PR breaks typescript types? I'm trying to bump our mysql2 dep from 3.19.0 to 3.20.0 but it does not build anymore. |
@nymkappa, which version of @types/node are you using? |
We don't explicitly require |
|
Adding the latest version using |
|
@nymkappa @wellwelwel I think it is available in types starting from |
I bumped my local Same result if I stay on |
|
I think the tracing channel import is gone since the errors got reduced. Which version of typescript are you using? there is a known conflict in buffer types related to Uint8Array being a generic in latter TS versions. |
|
@nymkappa, can you create a basic test case that reproduces the error? Everything is working fine for me. I'm testing with a @types/node version before the |
It works with |
I'm just trying to reproduce the error you reported. It seems the problem lies with the TypeScript version itself, not with @types/node: {
"devDependencies": {
"@types/node": "^25.5.0",
"typescript": "~5.2.2"
},
"dependencies": {
"mysql2": "^3.20.0"
}
}Before TypeScript |
Testing in latest @types/node (
|
|
Thanks, I was able to get it working with Okay so this means mysql2 is not compatible with older typescript version then? Starting from |
Within the limits of what I was able to test, it seems more like a conflict between TypeScript and @types/node versions than MySQL2 itself. An example of this is the fact that it worked when reverting the @types/node version to 18 with the old TypeScript version. I think that if the problem were with MySQL2 itself, no matter which @types/node version, it should break, but it would be interesting to have a way to reproduce it to be sure of that. |
Summary
TracingChannel(node:diagnostics_channel) support so APM tools (OTEL, Datadog, Sentry) can instrument mysql2 without monkey-patching via IITM/RITMmysql2:query,mysql2:execute,mysql2:connect,mysql2:pool:connectdb.query.text,db.namespace,server.address,server.port)hasSubscriberschecked before any context allocationTracingChannel(e.g. Node 18)Motivation
Current APM instrumentations use IITM (import-in-the-middle) for ESM and RITM (require-in-the-middle) for CJS to monkey-patch
Connection.prototype.queryandConnection.prototype.execute. This has fragility concerns:With native
TracingChannelsupport, instrumentation libraries become subscribers, not patches. Each tool listens independently with no ordering concerns, no clobbering, and no internal API dependency.Channels
mysql2:queryconnection.query()executionquery,values,database,serverAddress,serverPortmysql2:executeconnection.execute()prepared statementsquery,values,database,serverAddress,serverPortmysql2:connectdatabase,serverAddress,serverPort,usermysql2:pool:connectdatabase,serverAddress,serverPortImplementation
lib/tracing.js+lib/tracing.d.ts— channel setup, context types,tracePromisewrapperslib/base/connection.js— instrumentedquery(),execute(), and constructor (connect)lib/base/pool.js— instrumentedgetConnection()typings/mysql/index.d.ts— re-exports context types for APM consumerstest/integration/tracing-channel.test.mts— integration tests for all 4 channelsUses
tracePromisewith promise wrapping (same pattern as node-redis, ioredis, pg implementations). Promise wrapper inlib/promise/delegates to the base layer, so promise API gets tracing for free.Prior art
Follows the same pattern adopted by:
TracingChannelsince Node 20.12closes #4174