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

MiichelleZhou · 2026-09-28T19:45:02Z

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
Instrumentation wraps connect, query, and pool checkout/release hot paths; behavior should be unchanged without subscribers, but callback wrapping on those paths warrants careful review when tracing is active.

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, wired through new diagnostics.js helpers that no-op when diagnostics_channel is missing and use shouldTrace (hasSubscribers !== false) for Node 18. Client.connect() and callback-based Client.query() paths wrap work with traceCallback and attach connection/query/client (and result) metadata on completion.

pg-pool adds pg:pool:connect (TracingChannel with pool stats and client reuse info), plus pg:pool:release and pg:pool:remove channels that publish on release and removal (including release errors).

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-28T19:45:06Z

/devin review

devin-ai-integration · 2026-09-28T19:45:09Z

Starting Devin Review.

qodo-code-review · 2026-09-28T19:45:33Z

PR Summary by Qodo

Add diagnostics channels for pg queries and pool lifecycles

✨ Enhancement 🧪 Tests 🕐 40+ Minutes

AI Description

• Expose query, connection, and pool lifecycle events for instrumentation without monkey-patching.
• Preserve asynchronous tracing context and include query results, connection details, and pool
 metrics.
• Add lifecycle tests and compatibility guards for runtimes with differing diagnostics support.

Diagram

sequenceDiagram
    actor App as Application
    participant Pool as pg-pool
    participant Client as pg Client
    participant PoolDiag as Pool diagnostics
    participant PgDiag as pg diagnostics
    participant DC as diagnostics_channel
    actor Subscriber as Instrumentation
    App->>Pool: Check out client
    Pool->>PoolDiag: Trace checkout
    Pool->>Client: Connect client
    App->>Client: Execute query
    Client->>PgDiag: Trace connection and query
    PoolDiag->>DC: Publish pool lifecycle
    PgDiag->>DC: Publish client lifecycle
    DC-->>Subscriber: Deliver lifecycle events
    App->>Pool: Release client
    Pool->>PoolDiag: Publish release or removal

High-Level Assessment

The following are alternative approaches to this PR:

1. Extend existing EventEmitter hooks

➕ Reuses an established pool notification mechanism.
➕ Does not depend on TracingChannel availability.
➖ Does not provide consistent query and connection lifecycle coverage.
➖ Does not provide TracingChannel asynchronous context propagation.

Recommendation: Use diagnostics_channel as proposed: it provides subscribable lifecycle tracing across both packages without requiring consumers to wrap methods. Before merging, ensure pg-pool includes its new diagnostics.js helper in its published files; its current package.json files list excludes it.

Files changed (6) +557 / -11

Enhancement (4) +155 / -11

diagnostics.jsInitialize pool diagnostics channels with runtime guards +35/-0

Initialize pool diagnostics channels with runtime guards
• Defines a tracing channel for checkout and plain channels for release and removal. Falls back when diagnostics_channel is unavailable and handles Node 18's missing aggregated subscriber getter.
packages/pg-pool/diagnostics.js

index.jsTrace pool checkout and publish release and removal +34/-0

Trace pool checkout and publish release and removal

• Wraps pool checkout callbacks with tracing and adds pool and acquired-client context. Publishes guarded release and removal messages containing client identifiers and, for release, any error.

packages/pg-pool/index.js

client.jsTrace client connections and callback-backed queries +55/-11

Trace client connections and callback-backed queries

• Wraps callback and promise connection paths and callback-backed queries in tracing channels. Adds connection and query context, enriching successful queries with command and row count.

packages/pg/lib/client.js

diagnostics.jsInitialize client diagnostics tracing channels +31/-0

Initialize client diagnostics tracing channels

• Defines query and connection TracingChannels with a no-op fallback when unavailable. Uses an explicit false subscriber check to accommodate Node 18's missing aggregated getter.

packages/pg/lib/diagnostics.js

Tests (2) +402 / -0

diagnostics.jsTest pool diagnostics lifecycle messages +197/-0

Test pool diagnostics lifecycle messages

• Tests checkout context enrichment, release messages and errors, and removal messages using mock clients. Skips TracingChannel-specific cases where the runtime lacks the expected stable behavior.

packages/pg-pool/test/diagnostics.js

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

Test query and connection tracing events
• Tests query success, errors, result and client context, promise queries, and callback connection tracing. Skips tracing tests on runtimes without the expected TracingChannel behavior.
packages/pg/test/unit/client/diagnostics-tests.js

devin-ai-integration

Devin Review found 5 potential issues.

devin-ai-integration · 2026-09-28T19:47:33Z

published file list

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


🔴 Published pools fail to load

Installing the published pool leaves ./diagnostics out of the package, so require('./diagnostics') fails at startup. The published file list includes only index.js and esm.

Learn more

The package manager packs only files selected by the pool package's files field. That field lists index.js and esm but omits the new diagnostics module. A published install thus has the importing file but not its required module, and consumers cannot load the pool.

Example: After publishing the current pool package, an application importing pg reaches the pool import and gets Cannot find module './diagnostics' instead of a usable pool.

Recommended fix: Add diagnostics.js to the files array in packages/pg-pool/package.json, then verify the packed tarball contains it with a package dry run.

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

devin-ai-integration · 2026-09-28T19:47:35Z

queryCallback

+      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

When a query times out, queryCallback bypasses traceCallback and the timer replaces its wrapped callback with a no-op. Subscribers receive neither the timeout error nor completion, leaving the query trace open.

Learn more

The query timeout setup saves the original callback in queryCallback before this tracing wrapper replaces query.callback. On timeout, the timer calls the saved callback directly and then sets query.callback to a no-op. The wrapper installed here never runs, even if the backend later responds, so the tracing channel emits a start without an error or asyncEnd.

Example: A query_timeout of 10 milliseconds fires while SELECT pg_sleep(1) runs. The application receives Query read timeout, but a tracing subscriber still holds the query as active.

Recommended fix: Route timeout completion through the traced callback exactly once, while preserving timer cancellation and suppression of any later backend callback. Cover both queued and active query timeouts.

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

devin-ai-integration · 2026-09-28T19:47:36Z

Query events

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


🟡 Evented queries disappear from tracing

Calling client.query(new Query(...)) without a callback skips traceCallback. These queries finish through Query events, so pg:query subscribers receive no lifecycle events.

Learn more

A submittable query object can be returned from client.query with no callback. The Query implementation reports its outcome through error and end events instead. The new guard skips all instrumentation for that supported API, even when pg:query has subscribers.

Example: const q = client.query(new pg.Query('SELECT 1')); q.on('end', done) completes normally, but subscribers receive no pg:query:start or terminal event.

Recommended fix: Instrument queries without callbacks through their event lifecycle, without adding a callback that changes row accumulation or error-event behavior. Cover successful and failed evented queries.

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

devin-ai-integration · 2026-09-28T19:47:37Z

Query

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


🟡 Multi-statement traces lose result fields

For multi-statement queries, traceCallback replaces context.result with the array returned by Query. Subscribers cannot read result.command or result.rowCount from the trace.

Learn more

The result returned for multiple statements is an array of results, as Query records. Although enrichedCb assigns a structured context.result, Node's traceCallback then writes its second callback argument to context.result after enrichedCb returns. With multiple statements, the published context therefore has an array rather than the exposed command and rowCount properties.

Example: Running SELECT 1; SELECT 2 returns two result objects. The asyncEnd subscriber sees context.result as an array, and context.result.command is undefined.

Recommended fix: Define a result shape for multi-statement queries and emit it at a point that Node's traceCallback does not overwrite, or use a separate context property for stable metadata. Test the actual asyncEnd payload.

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

devin-ai-integration · 2026-09-28T19:47:38Z

+
+    if (shouldTrace(queryChannel) && query.callback) {
+      const context = {
+        query: { text: query.text, name: query.name, rowMode: query._rowMode },


🟨 Query traces can expose SQL literals

When applications embed sensitive values in SQL text, pg:query delivers those values to every tracing subscriber. Instrumentation that exports spans or logs can persist the values outside the database.

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-28T19:51:12Z

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


Pool diagnostics file not published

High Severity

index.js now requires ./diagnostics, but pg-pool's package files list still only includes index.js and esm. Published installs will throw Cannot find module './diagnostics' as soon as the pool is loaded.

Additional Locations (1)

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

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

cursor · 2026-09-28T19:51:12Z

+        context,
+        null,
+        enrichedCb
+      )


Query timeout skips tracing end

Medium Severity

When query_timeout fires, the original callback is invoked and query.callback is replaced with a noop, so the traceCallback wrapper never runs. Subscribers get start without error or asyncEnd, which leaves APM spans open.

Additional Locations (1)

packages/pg/lib/client.js#L649-L664

^{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 28, 2026

View reviewed changes

MiichelleZhou closed this Sep 28, 2026

MiichelleZhou deleted the eval/3650/devinai/round-5 branch September 28, 2026 19:48

cursor Bot reviewed Sep 28, 2026

View reviewed changes

Navigation Menu

Repository navigation

Add diagnostics_channel TracingChannel support - #31

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

MiichelleZhou commented Sep 28, 2026 •

edited by cursor Bot

Loading

Uh oh!

MiichelleZhou commented Sep 28, 2026

Uh oh!

devin-ai-integration Bot commented Sep 28, 2026

Uh oh!

qodo-code-review Bot commented Sep 28, 2026

Uh oh!

devin-ai-integration Bot left a comment

Uh oh!

devin-ai-integration Bot Sep 28, 2026

Uh oh!

devin-ai-integration Bot Sep 28, 2026

Uh oh!

devin-ai-integration Bot Sep 28, 2026

Uh oh!

devin-ai-integration Bot Sep 28, 2026

Uh oh!

devin-ai-integration Bot Sep 28, 2026

Uh oh!

cursor Bot left a comment

Uh oh!

cursor Bot Sep 28, 2026

Uh oh!

cursor Bot Sep 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

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

Conversation

MiichelleZhou commented Sep 28, 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 28, 2026

Uh oh!

devin-ai-integration Bot commented Sep 28, 2026

Uh oh!

qodo-code-review Bot commented Sep 28, 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 28, 2026

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot Sep 28, 2026

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot Sep 28, 2026

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot Sep 28, 2026

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot Sep 28, 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 28, 2026

Choose a reason for hiding this comment

Pool diagnostics file not published

Uh oh!

cursor Bot Sep 28, 2026

Choose a reason for hiding this comment

Query timeout skips tracing end

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

MiichelleZhou commented Sep 28, 2026 •

edited by cursor Bot

Loading