Add diagnostics_channel TracingChannel support by MiichelleZhou · Pull Request #34 · review-bench/brianc_node-postgres

MiichelleZhou · 2026-09-29T20:58:27Z

This PR introduces tracing channels support to pg-pool and pg querying methods as discussed in #3619

I 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:

Channel	Type	Package	Description
`pg:query`	TracingChannel	`pg`	Query lifecycle (start, end, error with async context)
`pg:connection`	TracingChannel	`pg`	Client connection lifecycle
`pg:pool:connect`	TracingChannel	`pg-pool`	Pool connection acquisition lifecycle
`pg:pool:release`	Channel	`pg-pool`	Client released back to pool
`pg:pool:remove`	Channel	`pg-pool`	Client removed from pool

I 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:

import { tracingChannel } from 'diagnostics_channel'

tracingChannel('pg:query').subscribe({
  start({ query, client }) {
    // start span
    // query: { text, name, rowMode }
    // client: { database, host, port, user, processID, ssl }
  },
  asyncEnd({ query, result }) {
    // end span
    // result: { rowCount, command }
  },
  error({ query, error }) {
    // record error on span
  },
})

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
Hooks wrap hot paths (connect, query, pool checkout/release) via callback substitution; guards limit overhead when unsubscribed, but regressions in async/callback ordering are the main concern.

Overview
Adds Node.js diagnostics_channel instrumentation to pg and pg-pool so APM/tracing tools can subscribe without monkey-patching.

pg exposes pg:query and pg:connection TracingChannels. Client.connect and callback-based Client.query paths wrap work with traceCallback, attaching connection metadata, query text/name/rowMode, and (on success) rowCount/command on async completion.

pg-pool adds pg:pool:connect (TracingChannel with pool counts and client processID / reuse flag), plus pg:pool:release and pg:pool:remove publish channels (including release errors).

Shared diagnostics.js helpers load the built-in module when available, fall back to no-op channels otherwise, and use shouldTrace (hasSubscribers !== false) so Node 18 still traces when the aggregated getter is missing. Unit tests cover channel payloads and skip unstable TracingChannel behavior on older Node versions.

^{Reviewed by Cursor Bugbot for commit b922f0d. Configure here.}

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>

MiichelleZhou · 2026-09-29T20:58:30Z

/devin review

devin-ai-integration · 2026-09-29T20:58:34Z

Starting Devin Review.

qodo-code-review · 2026-09-29T20:59:03Z

PR Summary by Qodo

Add diagnostics channels for pg queries, connections, and pool events

✨ Enhancement 🧪 Tests 🕐 40+ Minutes

AI Description

• Expose query, connection, and pool lifecycle events for instrumentation without monkey-patching.
• Preserve async tracing context while avoiding event construction when channels have no
 subscribers.
• Add lifecycle tests and compatibility handling for older Node versions.

Diagram

graph TD
  App["Application"] --> Pool["pg-pool"] --> Client["pg Client"] --> PgDiag["pg diagnostics"] --> Subscribers["Instrumentation subscribers"]
  Pool --> PoolDiag["pool diagnostics"] --> Subscribers

High-Level Assessment

The following are alternative approaches to this PR:

1. Instrument query completion centrally

➕ Could cover streaming Query objects that have no callback.
➕ Could capture failures before a query is enqueued.
➖ Requires a broader change to Query lifecycle handling.
➖ Makes async context propagation across API entry points harder to preserve.

Recommendation: The callback wrappers are a focused way to add native async tracing for callback and promise calls while retaining the existing APIs. Confirm whether streaming queries and pre-enqueue failures are intentionally outside the channel contract; if they need coverage, consider central lifecycle instrumentation as a follow-up.

Files changed (6) +557 / -11

Enhancement (4) +155 / -11

diagnostics.jsDefine pool diagnostics channels and compatibility guard +35/-0

Define pool diagnostics channels and compatibility guard

• Creates a tracing channel for checkout and plain channels for release and removal. Falls back to inactive channels when diagnostics are unavailable and handles Node 18's undefined tracing subscriber getter.

packages/pg-pool/diagnostics.js

index.jsPublish pool checkout, release, and removal events +34/-0

Publish pool checkout, release, and removal events

• Traces pool checkout callbacks with pool counts and acquired-client details. Publishes client identifiers on release and removal, including release errors where present.

packages/pg-pool/index.js

client.jsTrace client connections and queued queries +55/-11

Trace client connections and queued queries

• Wraps callback and promise connection paths in connection tracing. Traces queries with SQL and client metadata, then adds result details when their callbacks complete.

packages/pg/lib/client.js

diagnostics.jsDefine query and connection tracing channels +31/-0

Define query and connection tracing channels

• Initializes native tracing channels when available and otherwise uses inactive fallbacks. Provides a subscriber guard that accommodates Node 18's undefined aggregated getter.

packages/pg/lib/diagnostics.js

Tests (2) +402 / -0

diagnostics.jsTest pool diagnostics events +197/-0

Test pool diagnostics events

• Covers checkout context and client enrichment, release events with and without errors, and removal events. Skips tracing-specific cases where TracingChannel lacks stable subscriber behavior.

packages/pg-pool/test/diagnostics.js

diagnostics-tests.jsTest query and connection tracing +205/-0

Test query and connection tracing

• Checks successful and failed query events, promise-query tracing, context metadata, and callback connection tracing. Skips tracing cases on runtimes without stable TracingChannel support.

packages/pg/test/unit/client/diagnostics-tests.js

devin-ai-integration

Devin Review found 6 potential issues.

devin-ai-integration · 2026-09-29T21:01:33Z

files allowlist

@@ -1,5 +1,6 @@
 'use strict'
 const EventEmitter = require('events').EventEmitter
+const { poolConnectChannel, poolReleaseChannel, poolRemoveChannel, shouldTrace } = require('./diagnostics')


🔴 Published pool package fails to load

Published installations cannot resolve require('./diagnostics') because the package excludes the new module. Importing pg-pool fails before any pool can run.

Learn more

The entry point imports a separate diagnostics module, but the files allowlist contains only index.js and esm. npm pack --dry-run --json ./packages/pg-pool confirms that diagnostics.js is omitted. Importing a published copy of the pool therefore fails before the application can create a client.

Example: An application installs the published tarball and runs require('pg-pool'); the entry point fails to resolve ./diagnostics.

Recommended fix: Add diagnostics.js to the pool package's files allowlist, then verify the packed tarball includes it.

Was this helpful? React with 👍 or 👎 to provide feedback.

devin-ai-integration · 2026-09-29T21:01:34Z

the timeout handler

+      const origCb = query.callback
+      const enrichedCb = (err, res) => {
+        if (res) context.result = { rowCount: res.rowCount, command: res.command }
+        return origCb(err, res)
+      }
+      queryChannel.traceCallback(
+        (tracedCb) => {
+          query.callback = tracedCb
+          enqueue()


🟡 Timed-out queries leave traces unfinished

With query_timeout, the timer invokes queryCallback directly instead of the tracing wrapper. Timed-out queries never publish their trace error or completion.

Learn more

A query timeout is set up in the timeout handler before the new tracing wrapper replaces query.callback. The timer calls its saved queryCallback directly, then replaces query.callback with a noop. Neither the timeout nor a later query completion reaches the trace callback, so the pg:query operation remains open to diagnostics subscribers.

Example: With query_timeout: 10, a stalled SELECT pg_sleep(1) rejects after ten milliseconds but its pg:query subscriber receives only the start and end events, not the timeout error or async end.

Recommended fix: Route timeout completion through the tracing wrapper exactly once, and keep the later query callback suppressed to avoid duplicate completion. Test a traced query that times out.

Was this helpful? React with 👍 or 👎 to provide feedback.

devin-ai-integration · 2026-09-29T21:01:35Z

Query results

+      const enrichedCb = (err, res) => {
+        if (res) context.result = { rowCount: res.rowCount, command: res.command }
+        return origCb(err, res)


🟡 Multiple statement result metadata disappears

When a SQL string returns several results, enrichedCb reads metadata from their array instead of each result. The trace reports undefined command and row count.

Learn more

The Query results become an array when one simple-query string contains multiple statements. Query.handleReadyForQuery passes this array as res. Arrays have no rowCount or command, so diagnostics subscribers receive undefined metadata for successful multi-statement queries.

Example: client.query('SELECT 1; SELECT 2') resolves with two results, but pg:query reports { rowCount: undefined, command: undefined }.

Recommended fix: Handle array results explicitly when building the tracing context, preserving per-statement command and row count, and cover multi-statement queries in the tests.

Was this helpful? React with 👍 or 👎 to provide feedback.

devin-ai-integration · 2026-09-29T21:01:37Z

+      this._pulseQueryQueue()
+    }
+
+    if (shouldTrace(queryChannel) && query.callback) {


🔍 Callback-free queries remain untraced

query.callback gates tracing, so event-driven queries and callback-free cursors never appear on pg:query. Confirm whether tracing is meant to cover these public query forms.

Was this helpful? React with 👍 or 👎 to provide feedback.

devin-ai-integration · 2026-09-29T21:01:37Z

connect and query methods

 const defaults = require('./defaults')
 const Connection = require('./connection')
 const crypto = require('./crypto/utils')
+const { queryChannel, connectionChannel, shouldTrace } = require('./diagnostics')


🔍 Native client has no matching traces

The native client has separate connect and query methods, so pg.native emits neither new lifecycle trace. Confirm the intended coverage.

Was this helpful? React with 👍 or 👎 to provide feedback.

devin-ai-integration · 2026-09-29T21:01:39Z

+function shouldTrace(channel) {
+  return channel.hasSubscribers !== false
+}


🔍 Node 18 tracing fallback needs measurement

When hasSubscribers is undefined, shouldTrace enters tracing on every call, even without subscribers. Benchmark this supported runtime before relying on the no-overhead claim.

Was this helpful? React with 👍 or 👎 to provide feedback.

cursor

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.}

cursor · 2026-09-29T21:04:47Z

@@ -1,5 +1,6 @@
 'use strict'
 const EventEmitter = require('events').EventEmitter
+const { poolConnectChannel, poolReleaseChannel, poolRemoveChannel, shouldTrace } = require('./diagnostics')


Unpublished pool diagnostics module

High Severity

index.js now requires ./diagnostics, but that file is not in the pg-pool package files list (index.js and esm only). The published tarball omits it, so installing pg-pool from npm throws MODULE_NOT_FOUND and the pool cannot load.

Additional Locations (1)

packages/pg-pool/diagnostics.js#L1-L35

^{Reviewed by Cursor Bugbot for commit b922f0d. Configure here.}

cursor · 2026-09-29T21:04:47Z

+        context,
+        null,
+        enrichedCb
+      )


Query timeout leaves traces unfinished

Medium Severity

When query_timeout fires, the timeout handler calls the original callback and replaces query.callback with a no-op, so the traceCallback wrapper never runs. Subscribers get start but never asyncEnd or error, leaving APM spans open for the queries that most need them.

^{Reviewed by Cursor Bugbot for commit b922f0d. Configure here.}

logaretm and others added 7 commits March 5, 2026 12:40

Skip TracingChannel tests on Node < 19.9

91ace72

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>

fix: format

5b50561

fix: format

3957e3c

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

devin-ai-integration Bot reviewed Sep 29, 2026

View reviewed changes

MiichelleZhou closed this Sep 29, 2026

MiichelleZhou deleted the eval/3650/devinai/round-7 branch September 29, 2026 21:02

cursor Bot reviewed Sep 29, 2026

View reviewed changes

Navigation Menu

Repository navigation

Add diagnostics_channel TracingChannel support - #34

Add diagnostics_channel TracingChannel support#34
MiichelleZhou wants to merge 7 commits into
eval/3650/basefrom
eval/3650/devinai/round-7

MiichelleZhou commented Sep 29, 2026 •

edited by cursor Bot

Loading

Uh oh!

MiichelleZhou commented Sep 29, 2026

Uh oh!

devin-ai-integration Bot commented Sep 29, 2026

Uh oh!

qodo-code-review Bot commented Sep 29, 2026

Uh oh!

devin-ai-integration Bot left a comment

Uh oh!

devin-ai-integration Bot Sep 29, 2026

Uh oh!

devin-ai-integration Bot Sep 29, 2026

Uh oh!

devin-ai-integration Bot Sep 29, 2026

Uh oh!

devin-ai-integration Bot Sep 29, 2026

Uh oh!

devin-ai-integration Bot Sep 29, 2026

Uh oh!

devin-ai-integration Bot Sep 29, 2026

Uh oh!

cursor Bot left a comment

Uh oh!

cursor Bot Sep 29, 2026

Uh oh!

cursor Bot Sep 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

MiichelleZhou commented Sep 29, 2026 • edited by cursor Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Usage

Uh oh!

MiichelleZhou commented Sep 29, 2026

Uh oh!

devin-ai-integration Bot commented Sep 29, 2026

Uh oh!

qodo-code-review Bot commented Sep 29, 2026

PR Summary by Qodo

Uh oh!

devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot Sep 29, 2026

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot Sep 29, 2026

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot Sep 29, 2026

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot Sep 29, 2026

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot Sep 29, 2026

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot Sep 29, 2026

Choose a reason for hiding this comment

Uh oh!

cursor Bot left a comment

Choose a reason for hiding this comment

Uh oh!

cursor Bot Sep 29, 2026

Choose a reason for hiding this comment

Unpublished pool diagnostics module

Uh oh!

cursor Bot Sep 29, 2026

Choose a reason for hiding this comment

Query timeout leaves traces unfinished

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

MiichelleZhou commented Sep 29, 2026 •

edited by cursor Bot

Loading