Add diagnostics_channel TracingChannel support#3650
Add diagnostics_channel TracingChannel support#3650logaretm wants to merge 7 commits intobrianc:masterfrom
Conversation
Enables instrumentation libraries (OpenTelemetry, etc.) to subscribe to structured events without monkey-patching. Uses TracingChannel for async context propagation and plain channels for simple events. Channels: - pg:query (TracingChannel) — query lifecycle with result enrichment - pg:connection (TracingChannel) — client connect lifecycle - pg:pool:connect (TracingChannel) — pool checkout lifecycle - pg:pool:release (plain) — client released back to pool - pg:pool:remove (plain) — client removed from pool All instrumentation is guarded by hasSubscribers for zero overhead when unused. Gracefully degrades to no-ops on Node < 19.9 or non-Node environments. Closes brianc#3619 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
TracingChannel is not available on Node 18 LTS. Skip the tracing-dependent tests gracefully instead of failing. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Node 18 backported TracingChannel but without the aggregated `hasSubscribers` getter (it returns `undefined` instead of a boolean). Raw truthiness checks treat `undefined` as "no subscribers" which silently disables tracing on Node 18. Replace all `channel.hasSubscribers` guards with `shouldTrace(channel)` which checks `hasSubscribers !== false` — treating `undefined` (Node 18) as "might have subscribers, trace unconditionally" and `false` (Node 20+) as "definitely no subscribers, skip". Also removes the now-unnecessary test skip logic since TracingChannel does exist on Node 18. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Node 18 backported TracingChannel but with a buggy implementation — unsubscribing and resubscribing to the same channel crashes internally (`_subscribers` becomes undefined). Node 16 has no TracingChannel at all. Gate tests on `hasStableTracingChannel` which checks both that `dc.tracingChannel` exists AND that the aggregated `hasSubscribers` getter returns a boolean (only true on Node 19.9+/20.5+). TracingChannel tests: skipped on Node 16/18, run on Node 20+ Plain channel tests (release/remove): run on all versions Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…omise type tracePromise wraps the result in a native Promise, which breaks clients configured with a custom Promise implementation (e.g. bluebird). Switch to traceCallback inside the user's this._Promise constructor so the returned promise type is always correct. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@charmander @brianc I closed the previous PR to clean up a few things. I would love to get feedback on this. I got a few points worth noting/discussing: Argument sanitizationAPMs usually sanitize arguments, the redis implementation for tracing channels actually takes this into account but I don't think this is needed for SQL since parameterized queries already separate the statement from the values. This PR emits query.text but not query.values, which is the same approach taken by OTel. DocumentationI haven't yet documented any of this, would something like this work here? Happy to add a doc covering channel names, payload shapes, and usage examples once the API is agreed on. |
There was a problem hiding this comment.
Pull request overview
Adds first-class diagnostics_channel / TracingChannel instrumentation to pg and pg-pool so query/connection/pool lifecycles can be observed via subscribers instead of monkey-patching.
Changes:
- Introduces
pg:queryandpg:connectionTracingChannels and wires them intoClient#queryandClient#connect. - Introduces
pg:pool:connectTracingChannelpluspg:pool:release/pg:pool:removechannels and wires them into pool lifecycle paths. - Adds unit tests validating emitted events and context payloads (skipping unstable Node versions).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/pg/lib/diagnostics.js | Creates/query/connection tracing channels and shouldTrace() helper. |
| packages/pg/lib/client.js | Wraps connect() / query() callbacks with TracingChannel#traceCallback. |
| packages/pg/test/unit/client/diagnostics-tests.js | Adds unit tests for pg:query and pg:connection tracing. |
| packages/pg-pool/diagnostics.js | Creates pool diagnostics channels and shouldTrace() helper. |
| packages/pg-pool/index.js | Publishes pool connect/release/remove lifecycle events. |
| packages/pg-pool/test/diagnostics.js | Adds tests for pool tracing + publish-only channels. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Check explicitly for `false` rather than truthiness because the aggregated | ||
| // `hasSubscribers` getter on TracingChannel is `undefined` on Node 18 (which | ||
| // backported TracingChannel but not the getter). When `undefined`, we assume | ||
| // there may be subscribers and trace unconditionally. | ||
| function shouldTrace(channel) { | ||
| return channel.hasSubscribers !== false |
There was a problem hiding this comment.
shouldTrace() returns true when channel.hasSubscribers is undefined (e.g. Node 18 TracingChannel), which makes tracing run unconditionally even when there are no subscribers. That introduces overhead on those runtimes and contradicts the stated “no overhead without subscribers” behavior. Consider using a more reliable subscriber check (e.g. per-subchannel start.hasSubscribers/asyncEnd.hasSubscribers when available) or defaulting undefined to false and documenting Node 18 limitations.
| // Check explicitly for `false` rather than truthiness because the aggregated | |
| // `hasSubscribers` getter on TracingChannel is `undefined` on Node 18 (which | |
| // backported TracingChannel but not the getter). When `undefined`, we assume | |
| // there may be subscribers and trace unconditionally. | |
| function shouldTrace(channel) { | |
| return channel.hasSubscribers !== false | |
| // Prefer the aggregated `hasSubscribers` getter when available. On Node 18, | |
| // `TracingChannel` may exist without that getter, so fall back to checking | |
| // the individual tracing subchannels. If subscriber state cannot be | |
| // determined, default to `false` to avoid tracing overhead when there are no | |
| // subscribers. | |
| function shouldTrace(channel) { | |
| if (typeof channel.hasSubscribers === 'boolean') { | |
| return channel.hasSubscribers | |
| } | |
| return ['start', 'end', 'asyncStart', 'asyncEnd', 'error'].some((name) => { | |
| return channel[name] && channel[name].hasSubscribers === true | |
| }) |
There was a problem hiding this comment.
This is fully intended, if the flag is undefined then the platform is tracing-channel capable but doesn't have the skip optimization which is minimal at best.
We can do the suggested .some here but not worth the complexity IMO, happy to address it tho.
| if (shouldTrace(queryChannel) && query.callback) { | ||
| const context = { | ||
| query: { text: query.text, name: query.name, rowMode: query._rowMode }, | ||
| client: { | ||
| database: this.database, | ||
| host: this.host, | ||
| port: this.port, | ||
| user: this.user, | ||
| processID: this.processID, | ||
| ssl: !!this.ssl, | ||
| }, | ||
| } | ||
| const origCb = query.callback | ||
| const enrichedCb = (err, res) => { | ||
| if (res) context.result = { rowCount: res.rowCount, command: res.command } | ||
| return origCb(err, res) | ||
| } | ||
| queryChannel.traceCallback( | ||
| (tracedCb) => { | ||
| query.callback = tracedCb | ||
| enqueue() | ||
| }, | ||
| 0, | ||
| context, | ||
| null, | ||
| enrichedCb | ||
| ) | ||
| } else { | ||
| enqueue() |
There was a problem hiding this comment.
The tracing wrapper is only applied when query.callback is present. Queries executed in EventEmitter mode (user supplies a Query instance and listens for 'row'/'end' without a callback/promise) won’t emit pg:query lifecycle events, so instrumentation coverage is incomplete. Consider tracing those queries by attaching to the query’s 'end'/'error' events (or using a TracingChannel API that supports non-callback lifecycles) so all query styles are covered.
There was a problem hiding this comment.
With OTel instrumentation as the guide here, they don't handle this so this PR also doesn't. I think this is an advanced use-case and users who do it are in a better position to instrument on their own with their APM's API.
| // Check explicitly for `false` rather than truthiness because the aggregated | ||
| // `hasSubscribers` getter on TracingChannel is `undefined` on Node 18 (which | ||
| // backported TracingChannel but not the getter). When `undefined`, we assume | ||
| // there may be subscribers and trace unconditionally. | ||
| function shouldTrace(channel) { | ||
| return channel.hasSubscribers !== false |
There was a problem hiding this comment.
shouldTrace() returns true when channel.hasSubscribers is undefined, which can cause publish()/traceCallback() to run unconditionally on runtimes where hasSubscribers isn’t implemented. If the goal is “no overhead when there are no subscribers”, consider a safer subscriber check (or default undefined to false) and document any Node-version limitations explicitly.
| // Check explicitly for `false` rather than truthiness because the aggregated | |
| // `hasSubscribers` getter on TracingChannel is `undefined` on Node 18 (which | |
| // backported TracingChannel but not the getter). When `undefined`, we assume | |
| // there may be subscribers and trace unconditionally. | |
| function shouldTrace(channel) { | |
| return channel.hasSubscribers !== false | |
| // Only trace when subscriber presence is known. Some runtimes expose | |
| // `TracingChannel` without an aggregated `hasSubscribers` getter, in which case | |
| // `channel.hasSubscribers` is `undefined`. Default that case to `false` so we | |
| // avoid tracing overhead when subscriber state cannot be determined. | |
| function shouldTrace(channel) { | |
| return channel.hasSubscribers === true |
This PR introduces tracing channels support to
pg-poolandpgquerying methods as discussed in #3619I haven't yet run a benchmark, but without active tracing or subscribers the overhead is non-existent so existing users shouldn't be affected. The overhead would come from the consumers of the diag channels published here which is done anyways via monkey patching.
I added the following channels:
pg:querypgpg:connectionpgpg:pool:connectpg-poolpg:pool:releasepg-poolpg:pool:removepg-poolI understand there may be hesitation given that AI tooling was used in the implementation. To be clear, this isn't a drive-by contribution. I designed the channel layout, wrote the proposal document, and iterated through multiple rounds to handle real edge cases (Node 18 compat, preserving custom Promise types, hasSubscribers guards). The AI assisted with the code, but the approach and decisions are mine.
Usage
Instrumentations will only need to subscribe to tracing channels to create traces, logs or metrics:
This is part of a broader initiative to bring TracingChannel support to the most widely used Node.js database and cache libraries. The same pattern has already been merged and shipped in:
I have been directly involved in those prior implementations and I'm happy to own this through to release, address any feedback, and provide whatever support is needed. Would love to hear your thoughts.
Supersedes #3624.