ref(node): Streamline pg instrumentation#21583
Conversation
logaretm
force-pushed
the
awad/js-2392-streamline-opentelemetryinstrumentation-pg
branch
from
207f6ec to
72548f7
Compare
logaretm
changed the base branch from
awad/js-2393-streamline-opentelemetryinstrumentation-redis
to
develop
size-limit report 📦
|
logaretm
force-pushed
the
awad/js-2392-streamline-opentelemetryinstrumentation-pg
branch
from
72548f7 to
56a88b9
Compare
Streamlines the vendored `pg` (node-postgres) instrumentation to use Sentry's span APIs instead of the OpenTelemetry tracing APIs, following the redis/ioredis precedent. - Replace `tracer.startSpan` + `context.with(trace.setSpan(...))` with `startInactiveSpan`/`withActiveSpan`, `SpanStatusCode.ERROR` with `SPAN_STATUS_ERROR`, and drop `recordException` (a no-op in Sentry's pipeline) across the client query, client connect and pool connect paths. - Drop the OTel metrics (operation duration + pool connection counters): the SDK wires up no `MeterProvider`, so `this.meter` is the no-op meter and every `record`/`add` was dead. Also removes the pool event-listener plumbing and the `db.client.connection.*` semconv. - Drop the `SemconvStability` dual-emission and keep the OLD semconv attributes only (the STABLE path was env-gated behind `OTEL_SEMCONV_STABILITY_OPT_IN` and never enabled by the SDK). - Remove config the SDK never passes (`requestHook`, `responseHook`, `enhancedDatabaseReporting`, `addSqlCommenterCommentToQueries`) and hardcode the always-on `requireParentSpan` behaviour; bake the `auto.db.otel.postgres` origin into the query span attributes (connect spans keep their `manual` origin, matching prior output). - Drop the blanket eslint-disable and rely on the consolidated path entry. Removes the config-passing `Postgres` unit test (its only pg-specific behaviour, `ignoreConnectSpans` forwarding, is covered end-to-end) and expands the real integration suite to cover every code path, matching the redis/mysql2 precedent: - error paths: a failing query and a refused connect assert `status: 'internal_error'`; - `Pool`: a new scenario covers `pg-pool.connect` spans, callback-style queries, and connection-string credential masking on `db.connection_string`; - prepared statements: a named query asserts the `db.postgresql.plan` attribute; - `requireParentSpan`: a new scenario asserts queries/connects without an active parent span are not instrumented. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
logaretm
force-pushed
the
awad/js-2392-streamline-opentelemetryinstrumentation-pg
branch
from
56a88b9 to
936f0c8
Compare
logaretm
requested review from
JPeer264 and
mydea
and removed request for
a team
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| this: PgClientExtended, | ||
| tracer: Tracer, | ||
| instrumentationConfig: PgInstrumentationConfig, | ||
| semconvStability: SemconvStability, |
There was a problem hiding this comment.
Connect callback omits non-Error failures
Low Severity
patchClientConnectCallback only marks the connect span as failed when the first callback argument is an Error. Query and pool connect callbacks still treat any truthy error value as a failure. A truthy non-Error connect failure ends the span without error status, so it can appear successful in traces.
Reviewed by Cursor Bugbot for commit 936f0c8. Configure here.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Replace trace.getSpan(context.active()) with getActiveSpan() for the requireParentSpan check and parent detection, and translate the context.bind callback binding to a withActiveSpan-based helper. The connect-result promise no longer needs explicit binding since the SDK's async context propagates across the caller's await.
| name: SpanNames.CONNECT, | ||
| kind: SpanKind.CLIENT, | ||
| attributes: utils.getSemanticAttributesFromConnection(this, plugin._semconvStability), | ||
| attributes: utils.getSemanticAttributesFromConnection(this), |
There was a problem hiding this comment.
Bug: The _getClientConnectPatch function passes this instead of this.connectionParameters to getSemanticAttributesFromConnection, causing incorrect db.name, db.user, and db.connection_string span attributes.
Severity: MEDIUM
Suggested Fix
In _getClientConnectPatch, change the call from utils.getSemanticAttributesFromConnection(this) to utils.getSemanticAttributesFromConnection(this.connectionParameters) to pass the correct connection parameters object.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location:
packages/node/src/integrations/tracing/postgres/vendored/instrumentation.ts#L172
Potential issue: In `_getClientConnectPatch`, the `getSemanticAttributesFromConnection`
function is called with `this` (a `PgClientExtended` instance) instead of the expected
`this.connectionParameters` object. Because the `PgClientExtended` instance does not
have `database` and `user` properties at the top level, the resulting `pg.connect` span
will have missing `db.name` and `db.user` attributes. The `db.connection_string`
attribute will also be incorrect, defaulting to `'postgresql://localhost:5432/'`. This
degrades observability by providing incomplete and misleading data for database
connection spans.
| tracer: Tracer, | ||
| instrumentationConfig: PgInstrumentationConfig, | ||
| semconvStability: SemconvStability, |
There was a problem hiding this comment.
Bug: The error check in patchedClientConnectCallback was changed to if (err instanceof Error), which is inconsistent with other callback patchers and less defensive than the standard if (err) check.
Severity: LOW
Suggested Fix
Revert the error check in patchedClientConnectCallback from if (err instanceof Error) back to if (err) to align with other callback patchers and follow standard Node.js conventions.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: packages/node/src/integrations/tracing/postgres/vendored/utils.ts#L232-L234
Potential issue: In `patchedClientConnectCallback`, error handling was changed from a
general truthiness check (`if (err)`) to a stricter type check (`if (err instanceof
Error)`). This creates an inconsistency with other callback patchers like
`patchCallback` and `patchCallbackPGPool`, which still use `if (err)`. While the `pg`
library is expected to pass an `Error` object, this change makes the error handling less
defensive and deviates from the standard Node.js callback pattern. If the library ever
passes a truthy, non-Error value, the span's error status will not be set.
Add a scenario that chains a query off connect() with .then() instead of awaiting it, asserting the query span is still parented to the active transaction. This pins that the trace context survives the connect promise continuation after dropping the explicit OTel context.bind.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 3 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 321e80d. Configure here.
| attributes: { | ||
| ...getSemanticAttributesFromConnection(connectionParameters), | ||
| [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: ORIGIN, | ||
| }, |
There was a problem hiding this comment.
Postgres spans lack db op
Medium Severity
Query and connect spans created via startInactiveSpan no longer get sentry.op / op db. Previously, OpenTelemetry spans were exported through inference on db.system; native SentrySpan children use spanToJSON only, so missing op or SEMANTIC_ATTRIBUTE_SENTRY_OP stays unset unless set at creation.
Additional Locations (2)
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit 321e80d. Configure here.
| attributes: { | ||
| ...getSemanticAttributesFromConnection(connectionParameters), | ||
| [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: ORIGIN, | ||
| }, |
There was a problem hiding this comment.
Query span names use prefix
Medium Severity
Query spans are named with getQuerySpanName (for example pg.query:SELECT tests) while exported descriptions previously came from db.statement (the full SQL). After switching to native SentrySpan, getSpanJSON uses _name as description, so UI and tests expecting the statement text will see the prefixed operation name instead.
Reviewed by Cursor Bugbot for commit 321e80d. Configure here.
Vendored OTel instrumentations intentionally emit the old db.*/net.* semantic conventions, whose local semconv constants carry the upstream @deprecated JSDoc. The type-aware no-deprecated rule flagged every usage as an error and failed lint. Downgrade it to a warning for the vendored tracing paths so the intentional usage is surfaced without breaking CI.
1 participant
Streamlines the vendored pg (node-postgres) instrumentation onto Sentry's span APIs.
I found that some spans (
pg.connect,pg-pool.connect) still use manual origin, this was the case prior so I kept them as-is but I think they should be changed in the future.I added a bunch of real tests for various scenarios we were lacking.