Repository navigation
Add diagnostics_channel TracingChannel support - #25
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>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
There was a problem hiding this comment.
🟡 Changes recommended
Critical callback and packaging defects, plus unresolved tracing behavior issues, block approval.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds diagnostics_channel tracing for pg queries/connections and pg-pool lifecycle events.
Changes:
- Adds PostgreSQL query and connection tracing channels.
- Adds pool connection, release, and removal diagnostics.
- Adds diagnostic behavior tests.
File summaries
| File | Summary and final findings |
|---|---|
packages/pg/test/unit/client/diagnostics-tests.js |
Tests query and connection diagnostics. |
packages/pg/lib/diagnostics.js |
Defines pg channels. Moderate (2 votes): subscriber detection defeats the no-overhead path. Moderate (1 vote): misspelled getBuiltinModule fallback. |
packages/pg/lib/client.js |
Instruments client connections and queries. Critical (1 vote): incorrect tracing callback arguments. Moderate (2 votes): native client is not instrumented. Moderate (1 vote): array query results lose metadata. Moderate (1 vote): query timeouts bypass tracing. |
packages/pg-pool/test/diagnostics.js |
Tests pool diagnostics. |
packages/pg-pool/index.js |
Publishes pool lifecycle events. Critical (1 vote): incorrect tracing callback arguments. Critical (3 votes): diagnostics.js is omitted from the package tarball. |
packages/pg-pool/diagnostics.js |
Defines pool channels. Moderate (2 votes): subscriber detection defeats the no-overhead path. Moderate (1 vote): misspelled getBuiltinModule fallback. |
Review details
Suppressed comments (4)
packages/pg-pool/diagnostics.js:12
process.getBuiltInModuleis misspelled; Node exposesprocess.getBuiltinModule(without the capitalI). This branch is therefore never taken on Node, so the implementation always falls back torequire('diagnostics_channel'), defeating the loader-safe built-in-module path intended for bundlers and alternate runtimes.
if (typeof process.getBuiltInModule === 'function') {
dc = process.getBuiltInModule('diagnostics_channel')
packages/pg/lib/client.js:724
Client#querycan return an array for a valid multi-statement simple query, but this branch treats every truthy result as one result object. The resultingasyncEndpayload is{ rowCount: undefined, command: undefined }for those queries, so handle array results explicitly (or define an aggregate shape) instead of silently dropping their metadata.
const enrichedCb = (err, res) => {
if (res) context.result = { rowCount: res.rowCount, command: res.command }
return origCb(err, res)
packages/pg/lib/client.js:724
- When
query_timeoutis enabled, the timeout handler closes overqueryCallbackand calls it directly, while this code only replacesquery.callback. As a result, timed-out queries invoke the user's callback but bypass the tracing wrapper, sopg:querynever publishes the timeout'serror/asyncEndlifecycle; route the timeout callback through the traced callback while preserving the timeout timer cleanup on normal completion.
const origCb = query.callback
const enrichedCb = (err, res) => {
if (res) context.result = { rowCount: res.rowCount, command: res.command }
return origCb(err, res)
packages/pg/lib/diagnostics.js:11
process.getBuiltInModuleis misspelled; Node exposesprocess.getBuiltinModule(without the capitalI). This branch is therefore never taken on Node, so the implementation always falls back torequire('diagnostics_channel'), defeating the loader-safe built-in-module path intended for bundlers and alternate runtimes.
if (typeof process.getBuiltInModule === 'function') {
dc = process.getBuiltInModule('diagnostics_channel')
- Files reviewed: 6/6 changed files
- Comments generated: 6
- 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') | |||
| 0, | ||
| context, | ||
| null, | ||
| enrichedCb |
| const context = { | ||
| connection: { database: this.database, host: this.host, port: this.port, user: this.user, ssl: !!this.ssl }, | ||
| } | ||
| connectionChannel.traceCallback((tracedCb) => this._connect(tracedCb), 0, context, null, callback) |
| function shouldTrace(channel) { | ||
| return channel.hasSubscribers !== false | ||
| } |
| const defaults = require('./defaults') | ||
| const Connection = require('./connection') | ||
| const crypto = require('./crypto/utils') | ||
| const { queryChannel, connectionChannel, shouldTrace } = require('./diagnostics') |
| function shouldTrace(channel) { | ||
| return channel.hasSubscribers !== false | ||
| } |
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.