Repository navigation
Add diagnostics_channel TracingChannel support - #22
Closed
zhanglongtu wants to merge 7 commits into
Closed
zhanglongtu wants to merge 7 commits into
zhanglongtu wants to merge 7 commits into
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 #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>
There was a problem hiding this comment.
🟡 Changes recommended
Packaging, tracing correctness, compatibility, and coverage issues remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds diagnostics_channel tracing support for pg queries, connections, and pg-pool lifecycle events.
Changes:
- Adds query, connection, and pool diagnostic channels.
- Instruments callback and promise-based operations.
- Adds diagnostics unit tests.
File summaries
| File | Description |
|---|---|
packages/pg/test/unit/client/diagnostics-tests.js |
Tests client tracing behavior. |
packages/pg/lib/diagnostics.js |
Initializes PostgreSQL tracing channels. |
packages/pg/lib/client.js |
Instruments client connections and queries. |
packages/pg-pool/test/diagnostics.js |
Tests pool diagnostics. |
packages/pg-pool/index.js |
Publishes pool lifecycle events. |
packages/pg-pool/diagnostics.js |
Initializes pool diagnostic channels. |
Review details
Suppressed comments (9)
packages/pg-pool/diagnostics.js:12
- The Node API is named
process.getBuiltinModule, notprocess.getBuiltInModule, so this branch is never taken on Node versions that provide the built-in module accessor. In runtimes where therequirefallback is unavailable, diagnostics are then silently disabled; use the actual API name.
if (typeof process.getBuiltInModule === 'function') {
dc = process.getBuiltInModule('diagnostics_channel')
packages/pg-pool/diagnostics.js:32
- The same Node 18 fallback makes
shouldTrace(poolConnectChannel)true even with no subscribers, so every pool connection allocates tracing context and callback wrappers. This adds avoidable overhead to a hot pool path; use a version-safe check of the underlying channels rather than treating an undefined aggregate as subscribed.
return channel.hasSubscribers !== false
packages/pg-pool/index.js:218
- The pool connect tracing call uses the same invalid argument order, making
0the published context and preventing subscribers from receiving the pool/client context. Passcontextas the second argument,nullasthisArg, andenrichedCbas the operation callback.
poolConnectChannel.traceCallback(
packages/pg/lib/client.js:709
- Checking
query.callbackexcludes the supportedclient.query(new Query(...))event-emitter form: that path intentionally has no callback and completes throughquery.emit('end')/error, so it produces nopg:querystart, asyncEnd, or error events. Trace that lifecycle too, or document that this channel is callback/promise-only.
if (shouldTrace(queryChannel) && query.callback) {
packages/pg/lib/client.js:215
TracingChannel.traceCallbacktakes(fn, context, thisArg, ...args), but this passes0as the context and the context object asthisArg. Subscribers will receive0instead of the connection context, and the callback factory's arguments are shifted; passcontextsecond,nullthird, andcallbackas the operation argument.
connectionChannel.traceCallback((tracedCb) => this._connect(tracedCb), 0, context, null, callback)
packages/pg/lib/client.js:231
- The promise overload repeats the incorrect
traceCallbackargument order:0becomes the tracing context, sopg:connectionsubscribers do not receive the object built above and the callback wrapper is misapplied. Use the context as the second argument and the callback as the only operation argument.
connectionChannel.traceCallback((tracedCb) => this._connect(tracedCb), 0, context, null, callback)
packages/pg/lib/client.js:726
- This
traceCallbackcall has the same shifted arguments:0is published as the query context and the callback factory receives the wrong arguments, so the new query lifecycle notifications cannot carry the context/result data. Passcontext, null, enrichedCbafter the factory.
queryChannel.traceCallback(
packages/pg/lib/client.js:723
Query.handleReadyForQuerycan pass an array of results for multi-statement queries, but this always readsrowCountandcommanddirectly fromres. Those traces consequently publish undefined result fields and lose the per-statement data; preserve the array or map each result before assigning the context.
if (res) context.result = { rowCount: res.rowCount, command: res.command }
packages/pg/lib/diagnostics.js:11
- The Node API is named
process.getBuiltinModule, notprocess.getBuiltInModule, so this branch is never taken on Node versions that provide the built-in module accessor. In runtimes where therequirefallback is unavailable, diagnostics are then silently disabled; use the actual API name.
if (typeof process.getBuiltInModule === 'function') {
dc = process.getBuiltInModule('diagnostics_channel')
- Files reviewed: 6/6 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| @@ -1,5 +1,6 @@ | |||
| 'use strict' | |||
| const EventEmitter = require('events').EventEmitter | |||
| const { poolConnectChannel, poolReleaseChannel, poolRemoveChannel, shouldTrace } = require('./diagnostics') | |||
| const defaults = require('./defaults') | ||
| const Connection = require('./connection') | ||
| const crypto = require('./crypto/utils') | ||
| const { queryChannel, connectionChannel, shouldTrace } = require('./diagnostics') |
| this._pulseQueryQueue() | ||
| } | ||
|
|
||
| if (shouldTrace(queryChannel) && query.callback) { |
| // backported TracingChannel but not the getter). When `undefined`, we assume | ||
| // there may be subscribers and trace unconditionally. | ||
| function shouldTrace(channel) { | ||
| return channel.hasSubscribers !== false |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
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.