ref(node): Streamline pg instrumentation#21583
Open
logaretm wants to merge 2 commits into
Open
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
Contributor
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}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 936f0c8. Configure here.
| 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), |
Contributor
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.
Comment on lines
-232
to
-234
| tracer: Tracer, | ||
| instrumentationConfig: PgInstrumentationConfig, | ||
| semconvStability: SemconvStability, |
Contributor
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.