Repository navigation
Add diagnostics_channel TracingChannel support - #32
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, connections, and pool lifecycle
AI Description
Diagram
High-Level Assessment
Files changed (6)
|
Code Review by Qodo
1. Published 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. Published pools fail to load 🐞 Bug ≡ Correctness
pg-pool/index.js now requires ./diagnostics, but the package's files list excludes diagnostics.js. When pg-pool is installed from a published package, both its CommonJS entry and ESM wrapper reach that missing module.
Agent Prompt
## Issue description
The published pg-pool package omits the new module required by its entry point, so imports fail.
## Fix Focus Areas
- packages/pg-pool/package.json[46-49]
- packages/pg-pool/index.js[3-3]
## Recommended Fix
Add `diagnostics.js` to the package's `files` list and verify it appears in a package archive.
ⓘ 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.
2. Event-based queries escape tracing 🐞 Bug ◔ Observability
Client.query() starts a trace only when query.callback exists, leaving callback-free Query instances outside the new channel. The supported event-based query API submits those instances and reports completion through end or error events, so subscribers receive no lifecycle events for them.
Agent Prompt
## Issue description
Event-based Query instances execute without a callback and therefore produce no query diagnostics.
## Fix Focus Areas
- packages/pg/lib/client.js[622-635]
- packages/pg/lib/client.js[703-738]
- packages/pg/lib/query.js[128-149]
## Recommended Fix
Add tracing for callback-free Query instances using their completion and error events, without changing their event-based API.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| const enrichedCb = (err, res) => { | ||
| if (res) context.result = { rowCount: res.rowCount, command: res.command } | ||
| return origCb(err, res) |
There was a problem hiding this comment.
3. Multi-statement traces lose results 🐞 Bug ◔ Observability
enrichedCb reads rowCount and command from res as though it were always one result object. For a successful multi-statement query, Query passes an array of results, so the published result metadata has neither statement's command nor row count.
Agent Prompt
## Issue description
Multi-statement queries return an array, but tracing extracts fields as if the response were one result.
## Fix Focus Areas
- packages/pg/lib/client.js[721-724]
- packages/pg/lib/query.js[59-68]
## Recommended Fix
Handle array results explicitly and publish metadata for each statement while retaining the single-result shape for ordinary queries.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| const origCb = query.callback | ||
| const enrichedCb = (err, res) => { | ||
| if (res) context.result = { rowCount: res.rowCount, command: res.command } | ||
| return origCb(err, res) |
There was a problem hiding this comment.
4. Query callbacks lose their receiver 🐞 Bug ≡ Correctness
enrichedCb invokes the saved callback as origCb(err, res), discarding the query instance previously supplied as its receiver. When tracing is subscribed and a query callback uses this, it no longer sees the Query instance on either success or error.
Agent Prompt
## Issue description
The tracing wrapper changes the receiver observed by existing query callbacks.
## Fix Focus Areas
- packages/pg/lib/client.js[721-735]
- packages/pg/lib/query.js[128-149]
## Recommended Fix
Use a non-arrow enrichment callback and invoke the saved callback with the receiver supplied to the traced callback, preserving its arguments and return value.
ⓘ 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
Installed copies of pg-pool fail to load because ./diagnostics is absent from the published package. The package allowlist includes the entry point but excludes the new module.
Learn more
The pool entry point now requires a sibling module at load time. The files allowlist in package metadata excludes that module. An npm pack --dry-run --json listing confirms the tarball contains index.js but no diagnostics.js. Every installation from this tarball fails before creating a pool.
Example: A consumer installs the published pg-pool tarball and calls require('pg-pool'). Node resolves index.js but cannot resolve ./diagnostics, so it throws MODULE_NOT_FOUND.
Recommended fix: Add diagnostics.js to the files allowlist in packages/pg-pool/package.json, and verify it appears in npm pack --dry-run --json.
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 unfinished
When query_timeout fires, traceCallback never receives the error because the timer calls the saved queryCallback directly. The span emits start but never emits error or asyncEnd.
Learn more
The timeout handler in Client.query saves queryCallback before tracing is installed. On timeout it invokes that original callback directly, then replaces query.callback with a no-op. The tracing callback installed here never runs. A tracing subscriber sees an open query indefinitely despite the application receiving a timeout error.
Example: A SELECT pg_sleep(10) query with query_timeout: 10 emits pg:query:start. After 10 ms its promise rejects, but pg:query:error and pg:query:asyncEnd never fire.
Recommended fix: Route the timeout error through the traced callback exactly once, while preserving the existing suppression of any subsequent database completion. Add a timeout-specific tracing test.
Was this helpful? React with 👍 or 👎 to provide feedback.
| this._pulseQueryQueue() | ||
| } | ||
|
|
||
| if (shouldTrace(queryChannel) && query.callback) { |
There was a problem hiding this comment.
🟡 Event-driven queries disappear from tracing
When an event-driven Query has no callback, pg:query emits no events despite executing the query. The query.callback guard skips tracing, leaving these queries invisible to subscribers.
Learn more
A caller can pass a Query object to Client.query and consume row, end, and error events without setting Query.callback. Query.handleReadyForQuery emits end even without a callback, and Query.handleError emits error. This guard traces only callback-backed queries, so event-driven operations never appear on the new query channel.
Example: client.query(new Query('SELECT 1')).on('end', handler) completes normally, but a subscriber to pg:query sees no start or completion event.
Recommended fix: Instrument Query event completion/error as well as callback completion, or otherwise supply a completion hook for callback-free Submittables without changing their event-based API.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const enrichedCb = (err, res) => { | ||
| if (res) context.result = { rowCount: res.rowCount, command: res.command } | ||
| return origCb(err, res) |
There was a problem hiding this comment.
🟡 Multi-statement traces lose result metadata
For multi-statement queries, enrichedCb reads rowCount and command from an array instead of its results. Subscribers receive neither command nor row count for those queries.
Learn more
Query._checkForMultirow converts multi-statement results into an array. Query.handleReadyForQuery passes that array to the callback. This enrichment reads properties directly from the array and records both as undefined, while single-statement queries receive valid metadata.
Example: client.query('SELECT 1; SELECT 2') returns two result objects, but pg:query:asyncEnd has result: { rowCount: undefined, command: undefined }.
Recommended fix: Detect an array of results and publish metadata for each statement, while retaining the current single-result shape for ordinary queries.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const enqueue = () => { | ||
| if (this._queryQueue.length > 0) queryQueueLengthDeprecationNotice() | ||
| this._queryQueue.push(query) | ||
| this._pulseQueryQueue() | ||
| } | ||
|
|
||
| if (shouldTrace(queryChannel) && query.callback) { |
There was a problem hiding this comment.
🔍 Measure the no-subscriber query path
Even without subscribers, Client.query creates an enqueue closure and checks the channel. Benchmark the hot path before relying on the no-overhead claim.
Was this helpful? React with 👍 or 👎 to provide feedback.
| // TracingChannel exists on Node 18+ but the aggregated hasSubscribers getter | ||
| // and stable unsubscribe behavior require Node 19.9+/20.5+. Skip tests on | ||
| // older versions where TracingChannel is missing or has internal bugs. | ||
| 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 coverage
The tests skip versions without an aggregated hasSubscribers getter, while shouldTrace traces unconditionally there. Add coverage for that distinct execution path.
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 2 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.
Published package missing diagnostics module
High Severity
index.js now requires ./diagnostics, but the pg-pool files list still only ships index.js and esm. Installing the published package fails at load time with a missing-module error, which also breaks pg because it depends on pg-pool.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit b922f0d. Configure here.
| context, | ||
| null, | ||
| enrichedCb | ||
| ) |
There was a problem hiding this comment.
Query timeout skips tracing completion
Medium Severity
When query_timeout fires, the timeout path invokes the pre-trace callback and then replaces query.callback with a noop. Tracing already published start around enqueue, so asyncEnd and error never run and APM spans stay open for timed-out queries.
Additional Locations (1)
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 core
connectandquerycheckout paths; behavior should be unchanged without subscribers, but callback wrapping and Node 18 tracing fallbacks warrant careful review.Overview
Adds first-class Node.js
diagnostics_channelinstrumentation topgandpg-pool, so APM/tracing tools can subscribe without monkey-patching.pgexposesTracingChannelhooks onpg:connection(Client.connect) andpg:query(callback-basedClient.query), publishing connection/query metadata and enriching async context with results (rowCount,command).pg-pooladdspg:pool:connect(checkout lifecycle with pool stats and clientprocessID/ reuse), pluspg:pool:releaseandpg:pool:removepublish channels on release and removal.Shared
diagnostics.jshelpers load channels viagetBuiltInModule/require, fall back to no-op channels when unavailable, and useshouldTrace(hasSubscribers !== false) so Node 18 still traces when the aggregated getter is missing. Unit tests cover channel payloads and skip unstableTracingChannelbehavior on older Node versions.Reviewed by Cursor Bugbot for commit b922f0d. Configure here.