Repository navigation
Add diagnostics_channel TracingChannel support - #30
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>
|
/review |
PR Summary by QodoAdd diagnostics channels for pg queries and pool lifecycles
AI Description
Diagram
High-Level Assessment
Files changed (6)
|
Code Review by Qodo
1. Installed pools fail to load
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| @@ -1,5 +1,6 @@ | |||
| 'use strict' | |||
| const EventEmitter = require('events').EventEmitter | |||
| const { poolConnectChannel, poolReleaseChannel, poolRemoveChannel, shouldTrace } = require('./diagnostics') | |||
There was a problem hiding this comment.
1. Installed pools fail to load 🐞 Bug ≡ Correctness
The new top-level require('./diagnostics') imports a file omitted by pg-pool's package files
allowlist. In an npm-installed package, both the CommonJS entry point and the ESM wrapper reach that
import and fail with MODULE_NOT_FOUND.
Agent Prompt
## Issue description
The pool entry point requires a new file that the package does not publish.
## Fix Focus Areas
- packages/pg-pool/index.js[1-3]
- packages/pg-pool/package.json[43-50]
## Recommended Fix
Add `diagnostics.js` to the package `files` allowlist and verify that the packed package loads through both entry points.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| queryChannel.traceCallback( | ||
| (tracedCb) => { | ||
| query.callback = tracedCb | ||
| enqueue() |
There was a problem hiding this comment.
2. Timed-out queries leave open traces 🐞 Bug ◔ Observability
Client.query() installs the traced callback after its timeout handler has captured the original query.callback. When the timer fires, it calls that original callback and then replaces the traced callback with a no-op, so subscribers see a start but no traced completion or error.
Agent Prompt
## Issue description
Query timeouts bypass the newly installed tracing callback.
## Fix Focus Areas
- packages/pg/lib/client.js[650-664]
- packages/pg/lib/client.js[721-735]
## Recommended Fix
Have the timeout path invoke the current traced callback exactly once before disabling later completion, while retaining timer cleanup and existing caller behavior.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| poolConnectChannel.traceCallback( | ||
| (tracedCb) => { | ||
| response.callback = tracedCb | ||
| }, |
There was a problem hiding this comment.
3. Pool connections lose trace context 🐞 Bug ≡ Correctness
Pool.connect() uses traceCallback() only to assign response.callback, ending the traced synchronous scope before acquisition begins. On a new-client checkout, newClient() and its nested client.connect() run afterward, so a subscriber's pool-connect context is not active during connection startup.
Agent Prompt
## Issue description
The pool trace wraps callback assignment rather than connection acquisition, losing context during nested connection startup.
## Fix Focus Areas
- packages/pg-pool/index.js[218-227]
- packages/pg-pool/index.js[229-278]
## Recommended Fix
Move the acquisition and queue-setup branches into the function passed to `traceCallback`, while continuing to use its wrapped callback for asynchronous completion and preserving `connect()`'s return value.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| this._pulseQueryQueue() | ||
| } | ||
|
|
||
| if (shouldTrace(queryChannel) && query.callback) { |
There was a problem hiding this comment.
4. Event-based queries produce no trace 🐞 Bug ◔ Observability
The new query tracing branch requires query.callback, excluding supplied Query objects that use events instead of a callback. When such a query completes through its supported end event, it never enters the added tracing branch.
Agent Prompt
## Issue description
Queries supplied as event-based `Query` objects bypass the new diagnostics channel.
## Fix Focus Areas
- packages/pg/lib/client.js[625-637]
- packages/pg/lib/client.js[709-739]
- packages/pg/lib/query.js[111-143]
## Recommended Fix
Instrument callback-less `Query` objects through their completion and error events, preserving their existing event and unhandled-error behavior.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| function shouldTrace(channel) { | ||
| return channel.hasSubscribers !== false |
There was a problem hiding this comment.
5. Node 18 queries pay tracing costs 🐞 Bug ➹ Performance
shouldTrace() treats Node 18's missing aggregate hasSubscribers getter as permission to trace every operation. On Node 18 versions with TracingChannel, even with no subscribers, each callback-based query and connection builds context and callback wrappers, and pool checkouts do the same.
Agent Prompt
## Issue description
The absent aggregate subscriber getter makes Node 18 enter every tracing branch without subscribers.
## Fix Focus Areas
- packages/pg/lib/diagnostics.js[23-29]
- packages/pg-pool/diagnostics.js[27-33]
## Recommended Fix
When the aggregate getter is unavailable, check the constituent tracing channels' subscriber flags instead of assuming subscribers exist; retain the noop-channel behavior.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| if (shouldTrace(queryChannel) && query.callback) { | ||
| const context = { |
There was a problem hiding this comment.
6. Rejected requests leave no trace 🐞 Bug ◔ Observability
Both new tracing setups sit after existing early-return guards: Client.query() skips tracing for an unqueryable or closing client, and Pool.connect() skips it after pool shutdown. When those guards reject callback or promise requests, callers receive errors but subscribers receive no corresponding start or error events.
Agent Prompt
## Issue description
Early rejection paths return before the new tracing wrappers are installed.
## Fix Focus Areas
- packages/pg/lib/client.js[685-735]
- packages/pg-pool/index.js[195-227]
## Recommended Fix
Include the early failure paths within each operation's tracing setup so their existing callback or promise rejection also publishes a trace error.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| queryChannel.traceCallback( | ||
| (tracedCb) => { | ||
| query.callback = tracedCb | ||
| enqueue() | ||
| }, |
There was a problem hiding this comment.
7. Cancelled queries leave open traces 🐞 Bug ◔ Observability
The added query wrapper starts a trace when it enqueues a query, but queued-query cancellation removes that query without calling its traced callback. If a caller cancels a query while another is active, subscribers retain a start event with no completion or error event.
Agent Prompt
## Issue description
Cancelling a queued query removes it without completing the trace begun at enqueue time.
## Fix Focus Areas
- packages/pg/lib/client.js[542-560]
- packages/pg/lib/client.js[703-739]
## Recommended Fix
Give queued queries a trace-completion path on cancellation, or defer starting their trace until execution begins, without changing the existing caller cancellation behavior.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| @@ -1,5 +1,6 @@ | |||
| 'use strict' | |||
| const EventEmitter = require('events').EventEmitter | |||
| const { poolConnectChannel, poolReleaseChannel, poolRemoveChannel, shouldTrace } = require('./diagnostics') | |||
There was a problem hiding this comment.
🔴 Published pool package cannot load
Packed installs of pg-pool omit the required diagnostics module. The package allowlist includes only index.js and esm, so importing the published pool fails.
Learn more
The pool package uses a files allowlist to choose which files enter the published tarball. It currently includes index.js and esm, but not the new module required here. A checkout works because the module exists locally; an installed tarball lacks it and cannot load the pool.
Example: A service installs the newly published pg-pool and runs require('pg-pool'). Module resolution reaches require('./diagnostics'), cannot find the file, and throws before the service creates a pool.
Recommended fix: Add diagnostics.js to the files array in the package manifest, then inspect the packed tarball and load the installed package in a packaging test.
Was this helpful? React with 👍 or 👎 to provide feedback.
| queryChannel.traceCallback( | ||
| (tracedCb) => { | ||
| query.callback = tracedCb | ||
| enqueue() | ||
| }, | ||
| 0, | ||
| context, | ||
| null, | ||
| enrichedCb |
There was a problem hiding this comment.
🟡 Timed-out queries leave tracing spans open
When query_timeout fires, query.callback is replaced before the tracing wrapper receives the error. The timeout handler calls an earlier callback directly, so subscribers never receive an error or asyncEnd.
Learn more
Query timeouts save a callback before tracing wraps query.callback. When the timeout fires, the timer calls the saved callback directly and replaces query.callback with a no-op. TracingChannel observes completion only through its wrapped callback, so the timeout leaves the trace without an error or asyncEnd event, even though the caller gets an error.
Example: A client uses { text: 'SELECT pg_sleep(10)', query_timeout: 10 } with a query-channel subscriber. The subscriber sees start/end, the caller receives Query read timeout, but no error or asyncEnd arrives for that query.
Recommended fix: Route the timeout's completion through the traced callback exactly once. Coordinate the timer's saved callback and the later query.callback replacement in the timeout handler so a later server reply does not complete the trace twice.
Was this helpful? React with 👍 or 👎 to provide feedback.
| this._pulseQueryQueue() | ||
| } | ||
|
|
||
| if (shouldTrace(queryChannel) && query.callback) { |
There was a problem hiding this comment.
🟡 Event-based queries disappear from query tracing
When a submittable query has no callback, query.callback is empty and tracing is skipped. Query events still complete the query, leaving its execution invisible to subscribers.
Learn more
client.query(new Query('SELECT 1')) returns a query object that reports rows and completion through events rather than a callback. The tracing gate requires a callback, so this supported query form bypasses the query tracing channel entirely. Its end and error events still fire through Query completion.
Example: An application attaches an end listener to client.query(new Query('SELECT 1')) while an instrumenter subscribes to pg:query. The listener runs, but the instrumenter receives no start or completion event.
Recommended fix: Trace the event-based query lifecycle separately, without assigning a callback to queries that did not have one; Query.handleRowDescription uses callback presence to decide whether to buffer rows.
Was this helpful? React with 👍 or 👎 to provide feedback.
| } | ||
| const origCb = query.callback | ||
| const enrichedCb = (err, res) => { | ||
| if (res) context.result = { rowCount: res.rowCount, command: res.command } |
There was a problem hiding this comment.
🟡 Multi-statement traces lose result metadata
For multi-statement queries, res is an array, so context.result gets undefined rowCount and command. Query results contain those fields on each array element instead.
Learn more
A simple query can execute more than one statement. Query stores each statement's result separately and passes an array to the callback. This trace mapping reads metadata from the array itself, where neither rowCount nor command exists, so a successful multi-statement trace contains no useful result metadata.
Example: For SELECT 1; SELECT 2, the callback receives two Result objects, but the diagnostic context contains { rowCount: undefined, command: undefined }.
Recommended fix: Handle an array of results explicitly in the enrichment callback, preserving the per-statement command and row count while retaining the current shape for single statements.
Was this helpful? React with 👍 or 👎 to provide feedback.
| // 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.
🔍 Unsubscribed Node 18 tracing has overhead
On Node 18, an undefined hasSubscribers makes shouldTrace enter tracing paths without subscribers. Benchmark this compatibility path before claiming subscriber-free overhead is negligible.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const hasStableTracingChannel = | ||
| typeof dc.tracingChannel === 'function' && typeof dc.tracingChannel('pg:test:probe').hasSubscribers === 'boolean' | ||
|
|
||
| const suite = new helper.Suite() | ||
| const test = suite.test.bind(suite) | ||
| // pass undefined as callback to skip when TracingChannel is unavailable/unstable | ||
| const testTracing = (name, cb) => test(name, hasStableTracingChannel ? cb : undefined) |
There was a problem hiding this comment.
🔍 Node 18 compatibility path lacks tests
The tracing tests skip when hasSubscribers is undefined, while production deliberately traces in that case. Consider isolated Node 18 coverage that avoids unstable unsubscribe behavior.
Was this helpful? React with 👍 or 👎 to provide feedback.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 3 potential issues.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit b922f0d. Configure here.
| @@ -1,5 +1,6 @@ | |||
| 'use strict' | |||
| const EventEmitter = require('events').EventEmitter | |||
| const { poolConnectChannel, poolReleaseChannel, poolRemoveChannel, shouldTrace } = require('./diagnostics') | |||
There was a problem hiding this comment.
Pool diagnostics file not published
High Severity
index.js now requires ./diagnostics, but diagnostics.js is not in the pg-pool files allowlist. After publish, loading the pool throws a missing-module error for every pg and pg-pool consumer.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit b922f0d. Configure here.
| context, | ||
| null, | ||
| enrichedCb | ||
| ) |
There was a problem hiding this comment.
Timeout skips query tracing callback
Medium Severity
A query_timeout invokes the original callback and then replaces query.callback with a no-op, so the pg:query wrapper never runs. Subscribers receive start without error or asyncEnd, leaving traces open.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit b922f0d. Configure here.
| context, | ||
| null, | ||
| enrichedCb | ||
| ) |
There was a problem hiding this comment.
Failed queries skip diagnostic events
Medium Severity
pg:query wrapping happens only after the !_queryable and _ending checks return. Those failed query() calls never publish channel events, so subscribers miss a common class of errors. connect() wraps first and does not have this gap.
Reviewed by Cursor Bugbot for commit b922f0d. Configure here.
2 participants
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.
Note
Medium Risk
Instrumentation wraps connect, query, and pool checkout/release hot paths; behavior should be unchanged without subscribers, but callback wrapping adds complexity in core client/pool flows.
Overview
Adds Node
diagnostics_channelinstrumentation so APM and custom observers can subscribe without monkey-patchingpgorpg-pool.pgexposespg:queryandpg:connectionasTracingChannels aroundClient.query(callback-based queries) andClient.connect, with context for query text, connection metadata, and resultrowCount/command.pg-pooladdspg:pool:connect(TracingChannelwith pool stats and client reuse), pluspg:pool:releaseandpg:pool:removepublish channels on release and removal.Shared
diagnostics.jshelpers load the built-in module when available, fall back safely in non-Node environments, and useshouldTraceso work is skipped when there are no subscribers—while still tracing on Node 18 wherehasSubscribersis missing. Unit tests cover channel payloads and skip unstableTracingChannelbehavior on older Node versions.Reviewed by Cursor Bugbot for commit b922f0d. Configure here.