Repository navigation
Add diagnostics_channel TracingChannel support - #39
MiichelleZhou 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>
|
@cubic-dev-ai review this PR |
@MiichelleZhou I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
4 issues found across 6 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/pg/lib/diagnostics.js">
<violation number="1" location="packages/pg/lib/diagnostics.js:10">
P3: Node names this API `process.getBuiltinModule` (lowercase `i`), so this check never selects the built-in-module path on supported Node versions. Correct the spelling on both lines.</violation>
<violation number="2" location="packages/pg/lib/diagnostics.js:28">
P2: On Node 16.19+ and 18.x, `TracingChannel.prototype.hasSubscribers` is `undefined`, so `shouldTrace` returns `true` for `queryChannel`/`connectionChannel` even when nothing is subscribed. Every `connect()`, every callback/promise `query()`, and (via `packages/pg-pool/diagnostics.js`, which uses the identical function) every `pool.connect()` then allocates a context object, closures, and invokes `traceCallback` (which eagerly wraps the callback) — for all users on those Node versions, not just instrumented ones. That defeats the PR's stated guard: "Tracing is guarded when there are no subscribers to avoid affecting existing users." pg declares `engines.node >= 16`, so 18.2–18.18 (where tracing is unconditionally enabled) is a large supported LTS population. The sub-channels (`start`/`end`/`asyncStart`/`asyncEnd`/`error`) each expose a proper boolean `hasSubscribers` on those versions, so the aggregated check can be reconstructed. Note the new tests in `packages/pg/test/unit/client/diagnostics-tests.js`/`packages/pg-pool/test/diagnostics.js` are skipped unless `hasSubscribers` is a boolean — the traced path runs unconditionally on exactly the versions where it is untested.</violation>
</file>
<file name="packages/pg/lib/client.js">
<violation number="1" location="packages/pg/lib/client.js:709">
P2: Callback-free `Submittable` queries, including `QueryStream`, skip `pg:query` tracing entirely because this guard requires `query.callback`. Trace their `end`/`error` lifecycle too so stream-based queries are observable.</violation>
<violation number="2" location="packages/pg/lib/client.js:728">
P2: `query_timeout` bypasses this traced callback, leaving `pg:query` spans without an `error` or `asyncEnd` event when a query times out. Ensure the timeout path completes tracing as well as notifying the original callback.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| return channel.hasSubscribers !== false | ||
| } | ||
|
|
There was a problem hiding this comment.
P2: On Node 16.19+ and 18.x, TracingChannel.prototype.hasSubscribers is undefined, so shouldTrace returns true for queryChannel/connectionChannel even when nothing is subscribed. Every connect(), every callback/promise query(), and (via packages/pg-pool/diagnostics.js, which uses the identical function) every pool.connect() then allocates a context object, closures, and invokes traceCallback (which eagerly wraps the callback) — for all users on those Node versions, not just instrumented ones. That defeats the PR's stated guard: "Tracing is guarded when there are no subscribers to avoid affecting existing users." pg declares engines.node >= 16, so 18.2–18.18 (where tracing is unconditionally enabled) is a large supported LTS population. The sub-channels (start/end/asyncStart/asyncEnd/error) each expose a proper boolean hasSubscribers on those versions, so the aggregated check can be reconstructed. Note the new tests in packages/pg/test/unit/client/diagnostics-tests.js/packages/pg-pool/test/diagnostics.js are skipped unless hasSubscribers is a boolean — the traced path runs unconditionally on exactly the versions where it is untested.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/pg/lib/diagnostics.js, line 28:
<comment>On Node 16.19+ and 18.x, `TracingChannel.prototype.hasSubscribers` is `undefined`, so `shouldTrace` returns `true` for `queryChannel`/`connectionChannel` even when nothing is subscribed. Every `connect()`, every callback/promise `query()`, and (via `packages/pg-pool/diagnostics.js`, which uses the identical function) every `pool.connect()` then allocates a context object, closures, and invokes `traceCallback` (which eagerly wraps the callback) — for all users on those Node versions, not just instrumented ones. That defeats the PR's stated guard: "Tracing is guarded when there are no subscribers to avoid affecting existing users." pg declares `engines.node >= 16`, so 18.2–18.18 (where tracing is unconditionally enabled) is a large supported LTS population. The sub-channels (`start`/`end`/`asyncStart`/`asyncEnd`/`error`) each expose a proper boolean `hasSubscribers` on those versions, so the aggregated check can be reconstructed. Note the new tests in `packages/pg/test/unit/client/diagnostics-tests.js`/`packages/pg-pool/test/diagnostics.js` are skipped unless `hasSubscribers` is a boolean — the traced path runs unconditionally on exactly the versions where it is untested.</comment>
<file context>
@@ -0,0 +1,31 @@
+// backported TracingChannel but not the getter). When `undefined`, we assume
+// there may be subscribers and trace unconditionally.
+function shouldTrace(channel) {
+ return channel.hasSubscribers !== false
+}
+
</file context>
| return channel.hasSubscribers !== false | |
| } | |
| function shouldTrace(channel) { | |
| if (channel.hasSubscribers !== undefined) { | |
| return channel.hasSubscribers | |
| } | |
| // Node 16.19/18.x lack the aggregated getter; fall back to the individual | |
| // channels so tracing stays off when nothing is subscribed. | |
| return ( | |
| channel.start.hasSubscribers || | |
| channel.asyncStart.hasSubscribers || | |
| channel.error.hasSubscribers || | |
| channel.asyncEnd.hasSubscribers || | |
| channel.end.hasSubscribers | |
| ) | |
| } |
| } | ||
| queryChannel.traceCallback( | ||
| (tracedCb) => { | ||
| query.callback = tracedCb |
There was a problem hiding this comment.
P2: query_timeout bypasses this traced callback, leaving pg:query spans without an error or asyncEnd event when a query times out. Ensure the timeout path completes tracing as well as notifying the original callback.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/pg/lib/client.js, line 728:
<comment>`query_timeout` bypasses this traced callback, leaving `pg:query` spans without an `error` or `asyncEnd` event when a query times out. Ensure the timeout path completes tracing as well as notifying the original callback.</comment>
<file context>
@@ -687,11 +700,42 @@ class Client extends EventEmitter {
+ }
+ queryChannel.traceCallback(
+ (tracedCb) => {
+ query.callback = tracedCb
+ enqueue()
+ },
</file context>
| this._pulseQueryQueue() | ||
| } | ||
|
|
||
| if (shouldTrace(queryChannel) && query.callback) { |
There was a problem hiding this comment.
P2: Callback-free Submittable queries, including QueryStream, skip pg:query tracing entirely because this guard requires query.callback. Trace their end/error lifecycle too so stream-based queries are observable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/pg/lib/client.js, line 709:
<comment>Callback-free `Submittable` queries, including `QueryStream`, skip `pg:query` tracing entirely because this guard requires `query.callback`. Trace their `end`/`error` lifecycle too so stream-based queries are observable.</comment>
<file context>
@@ -687,11 +700,42 @@ class Client extends EventEmitter {
+ this._pulseQueryQueue()
+ }
+
+ if (shouldTrace(queryChannel) && query.callback) {
+ const context = {
+ query: { text: query.text, name: query.name, rowMode: query._rowMode },
</file context>
| if (typeof process.getBuiltInModule === 'function') { | ||
| dc = process.getBuiltInModule('diagnostics_channel') |
There was a problem hiding this comment.
P3: Node names this API process.getBuiltinModule (lowercase i), so this check never selects the built-in-module path on supported Node versions. Correct the spelling on both lines.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/pg/lib/diagnostics.js, line 10:
<comment>Node names this API `process.getBuiltinModule` (lowercase `i`), so this check never selects the built-in-module path on supported Node versions. Correct the spelling on both lines.</comment>
<file context>
@@ -0,0 +1,31 @@
+
+try {
+ let dc
+ if (typeof process.getBuiltInModule === 'function') {
+ dc = process.getBuiltInModule('diagnostics_channel')
+ } else {
</file context>
| if (typeof process.getBuiltInModule === 'function') { | |
| dc = process.getBuiltInModule('diagnostics_channel') | |
| if (typeof process.getBuiltinModule === 'function') { | |
| dc = process.getBuiltinModule('diagnostics_channel') |
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.
Summary by cubic
Adds
diagnostics_channelTracingChannel support topgandpg-poolso instrumentation libraries can subscribe to structured query and pool events instead of monkey-patching. No behavior change for existing users: all tracing is gated behind subscriber checks, so there is no overhead when nothing is subscribed.New channels
pg:queryandpg:connectiontracing channels onClient, carrying query text/name/rowMode and client connection details with async context.pg:pool:connecttracing channel andpg:pool:release/pg:pool:removeplain channels onPool, carrying pool statistics and client IDs.Compatibility
hasSubscribersgetter, so the guard treatsundefinedas "may have subscribers" to keep tracing working there.tracePromise.Written for commit b922f0d. Summary will update on new commits.