From 936f0c84f3a70759d03b29bc728590d8ec456cea Mon Sep 17 00:00:00 2001 From: Abdelrahman Awad Date: Tue, 16 Jun 2026 15:17:44 -0400 Subject: [PATCH 1/4] ref(node): Streamline pg instrumentation 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) --- .../postgres/scenario-connect-error.mjs | 22 + .../tracing/postgres/scenario-no-parent.mjs | 26 ++ .../suites/tracing/postgres/scenario-pool.mjs | 30 ++ .../suites/tracing/postgres/scenario.mjs | 12 + .../suites/tracing/postgres/test.ts | 145 +++++++ .../integrations/tracing/postgres/index.ts | 6 +- .../postgres/vendored/enums/AttributeNames.ts | 2 - .../postgres/vendored/enums/SpanNames.ts | 1 - .../postgres/vendored/instrumentation.ts | 386 +++--------------- .../postgres/vendored/internal-types.ts | 4 - .../postgres/vendored/pg-pool-types.ts | 1 - .../tracing/postgres/vendored/pg-types.ts | 1 - .../tracing/postgres/vendored/semconv.ts | 58 +-- .../tracing/postgres/vendored/types.ts | 70 +--- .../tracing/postgres/vendored/utils.ts | 328 ++++----------- .../integrations/tracing/postgres.test.ts | 93 ----- 16 files changed, 364 insertions(+), 821 deletions(-) create mode 100644 dev-packages/node-integration-tests/suites/tracing/postgres/scenario-connect-error.mjs create mode 100644 dev-packages/node-integration-tests/suites/tracing/postgres/scenario-no-parent.mjs create mode 100644 dev-packages/node-integration-tests/suites/tracing/postgres/scenario-pool.mjs delete mode 100644 packages/node/test/integrations/tracing/postgres.test.ts diff --git a/dev-packages/node-integration-tests/suites/tracing/postgres/scenario-connect-error.mjs b/dev-packages/node-integration-tests/suites/tracing/postgres/scenario-connect-error.mjs new file mode 100644 index 000000000000..1eb5469145c5 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/postgres/scenario-connect-error.mjs @@ -0,0 +1,22 @@ +import * as Sentry from '@sentry/node'; +import { Client } from 'pg'; + +// Nothing is listening on this port, so `connect()` fails and the connect span +// must be recorded with an errored status. +const client = new Client({ port: 5499, user: 'test', password: 'test', database: 'tests' }); + +async function run() { + await Sentry.startSpan( + { + name: 'Test Transaction', + op: 'transaction', + }, + async () => { + await client.connect().catch(() => { + // expected: connection refused + }); + }, + ); +} + +run(); diff --git a/dev-packages/node-integration-tests/suites/tracing/postgres/scenario-no-parent.mjs b/dev-packages/node-integration-tests/suites/tracing/postgres/scenario-no-parent.mjs new file mode 100644 index 000000000000..67c91affdc45 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/postgres/scenario-no-parent.mjs @@ -0,0 +1,26 @@ +import * as Sentry from '@sentry/node'; +import { Client } from 'pg'; + +const client = new Client({ port: 5494, user: 'test', password: 'test', database: 'tests' }); + +async function run() { + // No active span here: `requireParentSpan` means the connect and this query + // must NOT produce spans. + await client.connect(); + await client.query('SELECT 1 AS unparented'); + + // With an active span, the query is instrumented as a child span. + await Sentry.startSpan( + { + name: 'Test Transaction', + op: 'transaction', + }, + async () => { + await client.query('SELECT 2 AS parented'); + }, + ); + + await client.end(); +} + +run(); diff --git a/dev-packages/node-integration-tests/suites/tracing/postgres/scenario-pool.mjs b/dev-packages/node-integration-tests/suites/tracing/postgres/scenario-pool.mjs new file mode 100644 index 000000000000..b45d41086ed5 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/postgres/scenario-pool.mjs @@ -0,0 +1,30 @@ +import * as Sentry from '@sentry/node'; +import pg from 'pg'; + +const { Pool } = pg; + +// The connection string carries credentials so we can assert they are masked +// out of the `db.connection_string` span attribute. +const pool = new Pool({ connectionString: 'postgresql://test:test@localhost:5494/tests' }); + +async function run() { + await Sentry.startSpan( + { + name: 'Test Transaction', + op: 'transaction', + }, + async () => { + try { + // Callback-style query exercises the callback-patching path (the + // promise-based scenarios never hit it). + await new Promise((resolve, reject) => { + pool.query('SELECT 1 AS foo', (err, res) => (err ? reject(err) : resolve(res))); + }); + } finally { + await pool.end(); + } + }, + ); +} + +run(); diff --git a/dev-packages/node-integration-tests/suites/tracing/postgres/scenario.mjs b/dev-packages/node-integration-tests/suites/tracing/postgres/scenario.mjs index 351309e05010..2c9142eaf1bb 100644 --- a/dev-packages/node-integration-tests/suites/tracing/postgres/scenario.mjs +++ b/dev-packages/node-integration-tests/suites/tracing/postgres/scenario.mjs @@ -23,6 +23,18 @@ async function run() { await client.query('INSERT INTO "User" ("email", "name") VALUES ($1, $2)', ['tim', 'tim@domain.com']); await client.query('SELECT * FROM "User"'); + + // A named (prepared) query records its name as the `db.postgresql.plan` attribute + await client.query({ + name: 'select-user-by-email', + text: 'SELECT * FROM "User" WHERE "email" = $1', + values: ['tim'], + }); + + // A failing query should still produce an errored span + await client.query('SELECT * FROM "does_not_exist_table"').catch(() => { + // swallow: we only care about the span it produces + }); } finally { await client.end(); } diff --git a/dev-packages/node-integration-tests/suites/tracing/postgres/test.ts b/dev-packages/node-integration-tests/suites/tracing/postgres/test.ts index e9ed24371d8c..0a801aab4046 100644 --- a/dev-packages/node-integration-tests/suites/tracing/postgres/test.ts +++ b/dev-packages/node-integration-tests/suites/tracing/postgres/test.ts @@ -48,6 +48,33 @@ describe('postgres auto instrumentation', () => { status: 'ok', origin: 'auto.db.otel.postgres', }), + expect.objectContaining({ + data: expect.objectContaining({ + 'db.system': 'postgresql', + 'db.name': 'tests', + 'db.statement': 'SELECT * FROM "User" WHERE "email" = $1', + 'db.postgresql.plan': 'select-user-by-email', + 'sentry.origin': 'auto.db.otel.postgres', + 'sentry.op': 'db', + }), + description: 'SELECT * FROM "User" WHERE "email" = $1', + op: 'db', + status: 'ok', + origin: 'auto.db.otel.postgres', + }), + expect.objectContaining({ + data: expect.objectContaining({ + 'db.system': 'postgresql', + 'db.name': 'tests', + 'db.statement': 'SELECT * FROM "does_not_exist_table"', + 'sentry.origin': 'auto.db.otel.postgres', + 'sentry.op': 'db', + }), + description: 'SELECT * FROM "does_not_exist_table"', + op: 'db', + status: 'internal_error', + origin: 'auto.db.otel.postgres', + }), ]), }; @@ -114,6 +141,124 @@ describe('postgres auto instrumentation', () => { }); }); + describe('pool', () => { + const EXPECTED_TRANSACTION = { + transaction: 'Test Transaction', + spans: expect.arrayContaining([ + // Pool connect span: no origin is set on connect spans, so it defaults + // to 'manual', and the connection-string credentials are masked out. + expect.objectContaining({ + data: expect.objectContaining({ + 'db.system': 'postgresql', + 'db.name': 'tests', + 'db.connection_string': 'postgresql://localhost:5494/tests', + 'sentry.op': 'db', + }), + description: 'pg-pool.connect', + op: 'db', + status: 'ok', + origin: 'manual', + }), + // Callback-style query (no awaited promise returned to the caller). + expect.objectContaining({ + data: expect.objectContaining({ + 'db.system': 'postgresql', + 'db.name': 'tests', + 'db.statement': 'SELECT 1 AS foo', + 'sentry.origin': 'auto.db.otel.postgres', + 'sentry.op': 'db', + }), + description: 'SELECT 1 AS foo', + op: 'db', + status: 'ok', + origin: 'auto.db.otel.postgres', + }), + ]), + }; + + createEsmAndCjsTests(__dirname, 'scenario-pool.mjs', 'instrument.mjs', (createTestRunner, test) => { + test( + 'auto-instruments `pg.Pool`, masks connection-string credentials, and handles callback-style queries', + { timeout: 90_000 }, + async () => { + await createTestRunner() + .withDockerCompose({ + workingDirectory: [__dirname], + }) + .expect({ transaction: EXPECTED_TRANSACTION }) + .start() + .completed(); + }, + ); + }); + }); + + describe('connect error', () => { + const EXPECTED_TRANSACTION = { + transaction: 'Test Transaction', + spans: expect.arrayContaining([ + expect.objectContaining({ + data: expect.objectContaining({ + 'db.system': 'postgresql', + 'db.name': 'tests', + 'sentry.op': 'db', + }), + description: 'pg.connect', + op: 'db', + status: 'internal_error', + origin: 'manual', + }), + ]), + }; + + // No DB needed: the scenario connects to a port where nothing is listening. + createEsmAndCjsTests(__dirname, 'scenario-connect-error.mjs', 'instrument.mjs', (createTestRunner, test) => { + test('records an errored connect span when the connection fails', { timeout: 90_000 }, async () => { + await createTestRunner().expect({ transaction: EXPECTED_TRANSACTION }).start().completed(); + }); + }); + }); + + describe('requireParentSpan', () => { + createEsmAndCjsTests(__dirname, 'scenario-no-parent.mjs', 'instrument.mjs', (createTestRunner, test) => { + test('does not instrument queries or connects without an active parent span', { timeout: 90_000 }, async () => { + await createTestRunner() + .withDockerCompose({ + workingDirectory: [__dirname], + }) + .expect({ + transaction: txn => { + const descriptions = txn.spans?.map(span => span.description) ?? []; + // The unparented connect + query must not have produced spans + expect(descriptions).not.toContain('SELECT 1 AS unparented'); + expect(descriptions.find(name => name?.includes('connect'))).toBeUndefined(); + // Only the parented query is instrumented + expect(txn).toMatchObject({ + transaction: 'Test Transaction', + spans: expect.arrayContaining([ + expect.objectContaining({ + data: expect.objectContaining({ + 'db.system': 'postgresql', + 'db.name': 'tests', + 'db.statement': 'SELECT 2 AS parented', + 'sentry.origin': 'auto.db.otel.postgres', + 'sentry.op': 'db', + }), + description: 'SELECT 2 AS parented', + op: 'db', + status: 'ok', + origin: 'auto.db.otel.postgres', + }), + ]), + }); + }, + }) + .start() + .completed(); + }); + }); + }); + conditionalTest({ max: 25 })('pg-native', () => { const EXPECTED_TRANSACTION = { transaction: 'Test Transaction', diff --git a/packages/node/src/integrations/tracing/postgres/index.ts b/packages/node/src/integrations/tracing/postgres/index.ts index 18a8c823b161..a4581c33e2b9 100644 --- a/packages/node/src/integrations/tracing/postgres/index.ts +++ b/packages/node/src/integrations/tracing/postgres/index.ts @@ -1,7 +1,7 @@ import { PgInstrumentation } from './vendored/instrumentation'; import type { IntegrationFn } from '@sentry/core'; import { defineIntegration } from '@sentry/core'; -import { addOriginToSpan, generateInstrumentOnce } from '@sentry/node-core'; +import { generateInstrumentOnce } from '@sentry/node-core'; interface PostgresIntegrationOptions { ignoreConnectSpans?: boolean; @@ -13,10 +13,6 @@ export const instrumentPostgres = generateInstrumentOnce( INTEGRATION_NAME, PgInstrumentation, (options?: PostgresIntegrationOptions) => ({ - requireParentSpan: true, - requestHook(span) { - addOriginToSpan(span, 'auto.db.otel.postgres'); - }, ignoreConnectSpans: options?.ignoreConnectSpans ?? false, }), ); diff --git a/packages/node/src/integrations/tracing/postgres/vendored/enums/AttributeNames.ts b/packages/node/src/integrations/tracing/postgres/vendored/enums/AttributeNames.ts index 753e7d328808..3796cc23ff03 100644 --- a/packages/node/src/integrations/tracing/postgres/vendored/enums/AttributeNames.ts +++ b/packages/node/src/integrations/tracing/postgres/vendored/enums/AttributeNames.ts @@ -17,11 +17,9 @@ * - Vendored from: https://github.com/open-telemetry/opentelemetry-js-contrib/tree/15ef7506553f631ea4181391e0c5725a56f0d082/packages/instrumentation-pg * - Upstream version: @opentelemetry/instrumentation-pg@0.70.0 */ -/* eslint-disable */ // Postgresql specific attributes not covered by semantic conventions export enum AttributeNames { - PG_VALUES = 'db.postgresql.values', PG_PLAN = 'db.postgresql.plan', IDLE_TIMEOUT_MILLIS = 'db.postgresql.idle.timeout.millis', MAX_CLIENT = 'db.postgresql.max.client', diff --git a/packages/node/src/integrations/tracing/postgres/vendored/enums/SpanNames.ts b/packages/node/src/integrations/tracing/postgres/vendored/enums/SpanNames.ts index c7d61b383d57..8d3de763c17d 100644 --- a/packages/node/src/integrations/tracing/postgres/vendored/enums/SpanNames.ts +++ b/packages/node/src/integrations/tracing/postgres/vendored/enums/SpanNames.ts @@ -17,7 +17,6 @@ * - Vendored from: https://github.com/open-telemetry/opentelemetry-js-contrib/tree/15ef7506553f631ea4181391e0c5725a56f0d082/packages/instrumentation-pg * - Upstream version: @opentelemetry/instrumentation-pg@0.70.0 */ -/* eslint-disable */ // Contains span names produced by instrumentation export enum SpanNames { diff --git a/packages/node/src/integrations/tracing/postgres/vendored/instrumentation.ts b/packages/node/src/integrations/tracing/postgres/vendored/instrumentation.ts index 96b8b4a24dad..635ec0c2d26e 100644 --- a/packages/node/src/integrations/tracing/postgres/vendored/instrumentation.ts +++ b/packages/node/src/integrations/tracing/postgres/vendored/instrumentation.ts @@ -17,114 +17,42 @@ * - Vendored from: https://github.com/open-telemetry/opentelemetry-js-contrib/tree/15ef7506553f631ea4181391e0c5725a56f0d082/packages/instrumentation-pg * - Upstream version: @opentelemetry/instrumentation-pg@0.70.0 * - Types from `pg` and `pg-pool` packages inlined as simplified interfaces - * - Minor TypeScript strictness adjustments + * - Refactored to use Sentry's span APIs instead of OpenTelemetry tracing APIs + * - Dropped the OTel metrics (no MeterProvider is wired up), the `SemconvStability` + * dual-emission, and config the SDK never passes (request/response hooks, + * enhancedDatabaseReporting, addSqlCommenterCommentToQueries) */ -/* eslint-disable */ - -import { - isWrapped, - InstrumentationBase, - InstrumentationNodeModuleDefinition, - safeExecuteInTheMiddle, - SemconvStability, - semconvStabilityFromStr, -} from '@opentelemetry/instrumentation'; + +import { context, SpanKind, trace } from '@opentelemetry/api'; +import { InstrumentationBase, InstrumentationNodeModuleDefinition, isWrapped } from '@opentelemetry/instrumentation'; +import type { Span } from '@sentry/core'; +import { SDK_VERSION, SPAN_STATUS_ERROR, startInactiveSpan, withActiveSpan } from '@sentry/core'; import { InstrumentationNodeModuleFile } from '../../InstrumentationNodeModuleFile'; -import { - context, - trace, - Span, - SpanStatusCode, - SpanKind, - Histogram, - ValueType, - Attributes, - UpDownCounter, -} from '@opentelemetry/api'; -import type { PgClient } from './pg-types'; -import { +import { SpanNames } from './enums/SpanNames'; +import type { PgClientConnect, PgClientExtended, - PostgresCallback, - PgPoolExtended, PgPoolCallback, - EVENT_LISTENERS_SET, + PgPoolExtended, + PostgresCallback, } from './internal-types'; -import { PgInstrumentationConfig } from './types'; +import type { PgInstrumentationConfig } from './types'; import * as utils from './utils'; -import { addSqlCommenterComment } from '../../utils/sql-common'; -import { SDK_VERSION, timestampInSeconds } from '@sentry/core'; + const PACKAGE_NAME = '@sentry/instrumentation-pg'; -import { SpanNames } from './enums/SpanNames'; -import { - ATTR_ERROR_TYPE, - ATTR_SERVER_PORT, - ATTR_SERVER_ADDRESS, - ATTR_DB_NAMESPACE, - ATTR_DB_OPERATION_NAME, - ATTR_DB_SYSTEM_NAME, - METRIC_DB_CLIENT_OPERATION_DURATION, -} from '@opentelemetry/semantic-conventions'; -import { - METRIC_DB_CLIENT_CONNECTION_COUNT, - METRIC_DB_CLIENT_CONNECTION_PENDING_REQUESTS, - ATTR_DB_SYSTEM, - DB_SYSTEM_VALUE_POSTGRESQL, -} from './semconv'; - -function extractModuleExports(module: any) { + +function extractModuleExports(module: any): any { return module[Symbol.toStringTag] === 'Module' ? module.default // ESM : module; // CommonJS } export class PgInstrumentation extends InstrumentationBase { - declare private _operationDuration: Histogram; - declare private _connectionsCount: UpDownCounter; - declare private _connectionPendingRequests: UpDownCounter; - // Pool events connect, acquire, release and remove can be called - // multiple times without changing the values of total, idle and waiting - // connections. The _connectionsCounter is used to keep track of latest - // values and only update the metrics _connectionsCount and _connectionPendingRequests - // when the value change. - private _connectionsCounter: utils.poolConnectionsCounter = { - used: 0, - idle: 0, - pending: 0, - }; - private _semconvStability: SemconvStability; - - constructor(config: PgInstrumentationConfig = {}) { + public constructor(config: PgInstrumentationConfig = {}) { super(PACKAGE_NAME, SDK_VERSION, config); - this._semconvStability = semconvStabilityFromStr('database', process.env.OTEL_SEMCONV_STABILITY_OPT_IN); } - override _updateMetricInstruments() { - this._operationDuration = this.meter.createHistogram(METRIC_DB_CLIENT_OPERATION_DURATION, { - description: 'Duration of database client operations.', - unit: 's', - valueType: ValueType.DOUBLE, - advice: { - explicitBucketBoundaries: [0.001, 0.005, 0.01, 0.05, 0.1, 0.5, 1, 5, 10], - }, - }); - - this._connectionsCounter = { - idle: 0, - pending: 0, - used: 0, - }; - this._connectionsCount = this.meter.createUpDownCounter(METRIC_DB_CLIENT_CONNECTION_COUNT, { - description: 'The number of connections that are currently in state described by the state attribute.', - unit: '{connection}', - }); - this._connectionPendingRequests = this.meter.createUpDownCounter(METRIC_DB_CLIENT_CONNECTION_PENDING_REQUESTS, { - description: 'The number of current pending requests for an open connection.', - unit: '{connection}', - }); - } - - protected init() { + protected init(): InstrumentationNodeModuleDefinition[] { const SUPPORTED_PG_VERSIONS = ['>=8.0.3 <9']; const SUPPORTED_PG_POOL_VERSIONS = ['>=2.0.0 <4']; @@ -182,7 +110,7 @@ export class PgInstrumentation extends InstrumentationBase { - return function connect(this: PgClient, callback?: Function) { - const config = plugin.getConfig(); - - if (utils.shouldSkipInstrumentation(config) || config.ignoreConnectSpans) { + return function connect(this: PgClientExtended, callback?: (...args: unknown[]) => void) { + if (utils.shouldSkipInstrumentation() || plugin.getConfig().ignoreConnectSpans) { return original.call(this, callback); } - const span = plugin.tracer.startSpan(SpanNames.CONNECT, { + const span = startInactiveSpan({ + name: SpanNames.CONNECT, kind: SpanKind.CLIENT, - attributes: utils.getSemanticAttributesFromConnection(this, plugin._semconvStability), + attributes: utils.getSemanticAttributesFromConnection(this), }); - if (callback) { + let cb = callback; + if (cb) { const parentSpan = trace.getSpan(context.active()); - callback = utils.patchClientConnectCallback(span, callback); + cb = utils.patchClientConnectCallback(span, cb); if (parentSpan) { - callback = context.bind(context.active(), callback); + cb = context.bind(context.active(), cb); } } - const connectResult: unknown = context.with(trace.setSpan(context.active(), span), () => { - return original.call(this, callback); + const connectResult: unknown = withActiveSpan(span, () => { + return original.call(this, cb); }); return handleConnectResult(span, connectResult); @@ -250,41 +178,13 @@ export class PgInstrumentation extends InstrumentationBase { - if (key in attributes) { - metricsAttributes[key] = attributes[key]; - } - }); - - const durationSeconds = timestampInSeconds() - startTime; - this._operationDuration.record(durationSeconds, metricsAttributes); - } - private _getClientQueryPatch() { - const plugin = this; return (original: (...args: any[]) => any) => { this._diag.debug('Patching pg.Client.prototype.query'); return function query(this: PgClientExtended, ...args: unknown[]) { - if (utils.shouldSkipInstrumentation(plugin.getConfig())) { + if (utils.shouldSkipInstrumentation()) { return original.apply(this, args as never); } - const startTime = timestampInSeconds(); // client.query(text, cb?), client.query(text, values, cb?), and // client.query(configObj, cb?) are all valid signatures. We construct @@ -297,12 +197,9 @@ export class PgInstrumentation extends InstrumentationBase { - plugin.recordOperationDuration(attributes, startTime); - }; - - const instrumentationConfig = plugin.getConfig(); - - const span = utils.handleConfigQuery.call( - this, - plugin.tracer, - instrumentationConfig, - plugin._semconvStability, - queryConfig, - ); - - // Modify query text w/ a tracing comment before invoking original for - // tracing, but only if args[0] has one of our expected shapes. - if (instrumentationConfig.addSqlCommenterCommentToQueries) { - if (firstArgIsString) { - args[0] = addSqlCommenterComment(span, arg0); - } else if (firstArgIsQueryObjectWithText && !('name' in arg0)) { - // In the case of a query object, we need to ensure there's no name field - // as this indicates a prepared query, where the comment would remain the same - // for every invocation and contain an outdated trace context. - args[0] = { - ...arg0, - text: addSqlCommenterComment(span, arg0.text), - }; - } - } + const span = utils.handleConfigQuery.call(this, queryConfig); // Bind callback (if any) to parent span (if any) if (args.length > 0) { const parentSpan = trace.getSpan(context.active()); if (typeof args[args.length - 1] === 'function') { // Patch ParameterQuery callback - args[args.length - 1] = utils.patchCallback( - instrumentationConfig, - span, - args[args.length - 1] as PostgresCallback, // nb: not type safe. - attributes, - recordDuration, - ); + args[args.length - 1] = utils.patchCallback(span, args[args.length - 1] as PostgresCallback); // If a parent span exists, bind the callback if (parentSpan) { @@ -374,13 +226,7 @@ export class PgInstrumentation extends InstrumentationBase { - // pick keys to expose explicitly, so we're not leaking pg package - // internals that are subject to change - const { database, host, port, user } = this.connectionParameters; - const connection = { database, host, port, user }; - - requestHook(span, { - connection, - query: { - text: queryConfig.text, - // nb: if `client.query` is called with illegal arguments - // (e.g., if `queryConfig.values` is passed explicitly, but a - // non-array is given), then the type casts will be wrong. But - // we leave it up to the queryHook to handle that, and we - // catch and swallow any errors it throws. The other options - // are all worse. E.g., we could leave `queryConfig.values` - // and `queryConfig.name` as `unknown`, but then the hook body - // would be forced to validate (or cast) them before using - // them, which seems incredibly cumbersome given that these - // casts will be correct 99.9% of the time -- and pg.query - // will immediately throw during development in the other .1% - // of cases. Alternatively, we could simply skip calling the - // hook when `values` or `name` don't have the expected type, - // but that would add unnecessary validation overhead to every - // hook invocation and possibly be even more confusing/unexpected. - values: queryConfig.values as unknown[], - name: queryConfig.name as string | undefined, - }, - }); - }, - err => { - if (err) { - plugin._diag.error('Error running query hook', err); - } - }, - true, - ); - } - let result: unknown; try { result = original.apply(this, args as never); } catch (e: unknown) { - if (e instanceof Error) { - span.recordException(utils.sanitizedErrorMessage(e)); - } - span.setStatus({ - code: SpanStatusCode.ERROR, - message: utils.getErrorMessage(e), - }); + span.setStatus({ code: SPAN_STATUS_ERROR, message: utils.getErrorMessage(e) }); span.end(); throw e; } @@ -452,27 +250,13 @@ export class PgInstrumentation extends InstrumentationBase { - // Return a pass-along promise which ends the span and then goes to user's orig resolvers - return new Promise(resolve => { - utils.handleExecutionResult(plugin.getConfig(), span, result); - recordDuration(); - span.end(); - resolve(result); - }); + span.end(); + return result; }) .catch((error: Error) => { - return new Promise((_, reject) => { - if (error instanceof Error) { - span.recordException(utils.sanitizedErrorMessage(error)); - } - span.setStatus({ - code: SpanStatusCode.ERROR, - message: error.message, - }); - recordDuration(); - span.end(); - reject(error); - }); + span.setStatus({ code: SPAN_STATUS_ERROR, message: error.message }); + span.end(); + return Promise.reject(error); }); } @@ -482,86 +266,32 @@ export class PgInstrumentation extends InstrumentationBase { - this._connectionsCounter = utils.updateCounter( - poolName, - pgPool, - this._connectionsCount, - this._connectionPendingRequests, - this._connectionsCounter, - ); - }); - - pgPool.on('acquire', () => { - this._connectionsCounter = utils.updateCounter( - poolName, - pgPool, - this._connectionsCount, - this._connectionPendingRequests, - this._connectionsCounter, - ); - }); - - pgPool.on('remove', () => { - this._connectionsCounter = utils.updateCounter( - poolName, - pgPool, - this._connectionsCount, - this._connectionPendingRequests, - this._connectionsCounter, - ); - }); - - pgPool.on('release' as any, () => { - this._connectionsCounter = utils.updateCounter( - poolName, - pgPool, - this._connectionsCount, - this._connectionPendingRequests, - this._connectionsCounter, - ); - }); - pgPool[EVENT_LISTENERS_SET] = true; - } - private _getPoolConnectPatch() { const plugin = this; return (originalConnect: (...args: any[]) => any) => { return function connect(this: PgPoolExtended, callback?: PgPoolCallback) { - const config = plugin.getConfig(); - - if (utils.shouldSkipInstrumentation(config)) { - return originalConnect.call(this, callback as any); - } - - // Still set up event listeners for metrics even when skipping spans - plugin._setPoolConnectEventListeners(this); - - if (config.ignoreConnectSpans) { + if (utils.shouldSkipInstrumentation() || plugin.getConfig().ignoreConnectSpans) { return originalConnect.call(this, callback as any); } - // setup span - const span = plugin.tracer.startSpan(SpanNames.POOL_CONNECT, { + const span = startInactiveSpan({ + name: SpanNames.POOL_CONNECT, kind: SpanKind.CLIENT, - attributes: utils.getSemanticAttributesFromPoolConnection(this.options, plugin._semconvStability), + attributes: utils.getSemanticAttributesFromPoolConnection(this.options), }); - if (callback) { + let cb = callback; + if (cb) { const parentSpan = trace.getSpan(context.active()); - callback = utils.patchCallbackPGPool(span, callback) as PgPoolCallback; + cb = utils.patchCallbackPGPool(span, cb); // If a parent span exists, bind the callback if (parentSpan) { - callback = context.bind(context.active(), callback); + cb = context.bind(context.active(), cb); } } - const connectResult: unknown = context.with(trace.setSpan(context.active(), span), () => { - return originalConnect.call(this, callback as any); + const connectResult: unknown = withActiveSpan(span, () => { + return originalConnect.call(this, cb as any); }); return handleConnectResult(span, connectResult); @@ -570,7 +300,7 @@ export class PgInstrumentation extends InstrumentationBase { - if (error instanceof Error) { - span.recordException(utils.sanitizedErrorMessage(error)); - } - span.setStatus({ - code: SpanStatusCode.ERROR, - message: utils.getErrorMessage(error), - }); + span.setStatus({ code: SPAN_STATUS_ERROR, message: utils.getErrorMessage(error) }); span.end(); return Promise.reject(error); }), diff --git a/packages/node/src/integrations/tracing/postgres/vendored/internal-types.ts b/packages/node/src/integrations/tracing/postgres/vendored/internal-types.ts index 181c85052555..c8d640c69d0f 100644 --- a/packages/node/src/integrations/tracing/postgres/vendored/internal-types.ts +++ b/packages/node/src/integrations/tracing/postgres/vendored/internal-types.ts @@ -18,7 +18,6 @@ * - Upstream version: @opentelemetry/instrumentation-pg@0.70.0 * - Types from `pg` and `pg-pool` packages inlined as simplified interfaces */ -/* eslint-disable */ import type { PgClient } from './pg-types'; import type { PgPool } from './pg-pool-types'; @@ -64,11 +63,8 @@ export interface PgPoolOptionsParams { user: string; } -export const EVENT_LISTENERS_SET = Symbol('opentelemetry.instrumentation.pg.eventListenersSet'); - export interface PgPoolExtended extends PgPool { options: PgPoolOptionsParams; - [EVENT_LISTENERS_SET]?: boolean; // flag to identify if the event listeners for instrumentation have been set } export type PgClientConnect = (callback?: Function) => Promise | void; diff --git a/packages/node/src/integrations/tracing/postgres/vendored/pg-pool-types.ts b/packages/node/src/integrations/tracing/postgres/vendored/pg-pool-types.ts index d388d823c41d..4bf1ea6a112a 100644 --- a/packages/node/src/integrations/tracing/postgres/vendored/pg-pool-types.ts +++ b/packages/node/src/integrations/tracing/postgres/vendored/pg-pool-types.ts @@ -2,7 +2,6 @@ * Simplified type definitions inlined from the `pg-pool` package. * PoolClient and PoolConfig are replaced with structural equivalents. */ -/* eslint-disable */ import type { PgPoolClient } from './pg-types'; diff --git a/packages/node/src/integrations/tracing/postgres/vendored/pg-types.ts b/packages/node/src/integrations/tracing/postgres/vendored/pg-types.ts index 0d51e09e4f81..181449405947 100644 --- a/packages/node/src/integrations/tracing/postgres/vendored/pg-types.ts +++ b/packages/node/src/integrations/tracing/postgres/vendored/pg-types.ts @@ -3,7 +3,6 @@ * Deep type trees (Connection, Submittable, QueryConfig generics, FieldDef, * NoticeMessage, ClientConfig) are replaced with structural equivalents. */ -/* eslint-disable */ export interface FieldDef { name: string; diff --git a/packages/node/src/integrations/tracing/postgres/vendored/semconv.ts b/packages/node/src/integrations/tracing/postgres/vendored/semconv.ts index 0c4434f078a9..e8f672a0f9e0 100644 --- a/packages/node/src/integrations/tracing/postgres/vendored/semconv.ts +++ b/packages/node/src/integrations/tracing/postgres/vendored/semconv.ts @@ -16,8 +16,8 @@ * NOTICE from the Sentry authors: * - Vendored from: https://github.com/open-telemetry/opentelemetry-js-contrib/tree/15ef7506553f631ea4181391e0c5725a56f0d082/packages/instrumentation-pg * - Upstream version: @opentelemetry/instrumentation-pg@0.70.0 + * - Trimmed to the (old) semantic conventions the SDK actually emits */ -/* eslint-disable */ /* * This file contains a copy of unstable semantic convention definitions @@ -25,31 +25,11 @@ * @see https://github.com/open-telemetry/opentelemetry-js/tree/main/semantic-conventions#unstable-semconv */ -/** - * The name of the connection pool; unique within the instrumented application. In case the connection pool implementation doesn't provide a name, instrumentation **SHOULD** use a combination of parameters that would make the name unique, for example, combining attributes `server.address`, `server.port`, and `db.namespace`, formatted as `server.address:server.port/db.namespace`. Instrumentations that generate connection pool name following different patterns **SHOULD** document it. - * - * @example myDataSource - * - * @experimental This attribute is experimental and is subject to breaking changes in minor releases of `@opentelemetry/semantic-conventions`. - */ -export const ATTR_DB_CLIENT_CONNECTION_POOL_NAME = 'db.client.connection.pool.name' as const; - -/** - * The state of a connection in the pool - * - * @example idle - * - * @experimental This attribute is experimental and is subject to breaking changes in minor releases of `@opentelemetry/semantic-conventions`. - */ -export const ATTR_DB_CLIENT_CONNECTION_STATE = 'db.client.connection.state' as const; - /** * Deprecated, use `server.address`, `server.port` attributes instead. * * @example "Server=(localdb)\\v11.0;Integrated Security=true;" * - * @experimental This attribute is experimental and is subject to breaking changes in minor releases of `@opentelemetry/semantic-conventions`. - * * @deprecated Replaced by `server.address` and `server.port`. */ export const ATTR_DB_CONNECTION_STRING = 'db.connection_string' as const; @@ -60,8 +40,6 @@ export const ATTR_DB_CONNECTION_STRING = 'db.connection_string' as const; * @example customers * @example main * - * @experimental This attribute is experimental and is subject to breaking changes in minor releases of `@opentelemetry/semantic-conventions`. - * * @deprecated Replaced by `db.namespace`. */ export const ATTR_DB_NAME = 'db.name' as const; @@ -72,8 +50,6 @@ export const ATTR_DB_NAME = 'db.name' as const; * @example SELECT * FROM wuser_table * @example SET mykey "WuValue" * - * @experimental This attribute is experimental and is subject to breaking changes in minor releases of `@opentelemetry/semantic-conventions`. - * * @deprecated Replaced by `db.query.text`. */ export const ATTR_DB_STATEMENT = 'db.statement' as const; @@ -81,8 +57,6 @@ export const ATTR_DB_STATEMENT = 'db.statement' as const; /** * Deprecated, use `db.system.name` instead. * - * @experimental This attribute is experimental and is subject to breaking changes in minor releases of `@opentelemetry/semantic-conventions`. - * * @deprecated Replaced by `db.system.name`. */ export const ATTR_DB_SYSTEM = 'db.system' as const; @@ -93,8 +67,6 @@ export const ATTR_DB_SYSTEM = 'db.system' as const; * @example readonly_user * @example reporting_user * - * @experimental This attribute is experimental and is subject to breaking changes in minor releases of `@opentelemetry/semantic-conventions`. - * * @deprecated Removed, no replacement at this time. */ export const ATTR_DB_USER = 'db.user' as const; @@ -104,8 +76,6 @@ export const ATTR_DB_USER = 'db.user' as const; * * @example example.com * - * @experimental This attribute is experimental and is subject to breaking changes in minor releases of `@opentelemetry/semantic-conventions`. - * * @deprecated Replaced by `server.address` on client spans and `client.address` on server spans. */ export const ATTR_NET_PEER_NAME = 'net.peer.name' as const; @@ -115,37 +85,11 @@ export const ATTR_NET_PEER_NAME = 'net.peer.name' as const; * * @example 8080 * - * @experimental This attribute is experimental and is subject to breaking changes in minor releases of `@opentelemetry/semantic-conventions`. - * * @deprecated Replaced by `server.port` on client spans and `client.port` on server spans. */ export const ATTR_NET_PEER_PORT = 'net.peer.port' as const; -/** - * Enum value "idle" for attribute {@link ATTR_DB_CLIENT_CONNECTION_STATE}. - */ -export const DB_CLIENT_CONNECTION_STATE_VALUE_IDLE = 'idle' as const; - -/** - * Enum value "used" for attribute {@link ATTR_DB_CLIENT_CONNECTION_STATE}. - */ -export const DB_CLIENT_CONNECTION_STATE_VALUE_USED = 'used' as const; - /** * Enum value "postgresql" for attribute {@link ATTR_DB_SYSTEM}. */ export const DB_SYSTEM_VALUE_POSTGRESQL = 'postgresql' as const; - -/** - * The number of connections that are currently in state described by the `state` attribute - * - * @experimental This metric is experimental and is subject to breaking changes in minor releases of `@opentelemetry/semantic-conventions`. - */ -export const METRIC_DB_CLIENT_CONNECTION_COUNT = 'db.client.connection.count' as const; - -/** - * The number of current pending requests for an open connection - * - * @experimental This metric is experimental and is subject to breaking changes in minor releases of `@opentelemetry/semantic-conventions`. - */ -export const METRIC_DB_CLIENT_CONNECTION_PENDING_REQUESTS = 'db.client.connection.pending_requests' as const; diff --git a/packages/node/src/integrations/tracing/postgres/vendored/types.ts b/packages/node/src/integrations/tracing/postgres/vendored/types.ts index 05fffa56336b..d5fdba6084d5 100644 --- a/packages/node/src/integrations/tracing/postgres/vendored/types.ts +++ b/packages/node/src/integrations/tracing/postgres/vendored/types.ts @@ -16,79 +16,15 @@ * NOTICE from the Sentry authors: * - Vendored from: https://github.com/open-telemetry/opentelemetry-js-contrib/tree/15ef7506553f631ea4181391e0c5725a56f0d082/packages/instrumentation-pg * - Upstream version: @opentelemetry/instrumentation-pg@0.70.0 - * - Types from `pg` package inlined as simplified interfaces + * - Trimmed to the config the SDK actually passes */ -/* eslint-disable */ -import type { QueryResult, QueryArrayResult } from './pg-types'; -import type * as api from '@opentelemetry/api'; -import { InstrumentationConfig } from '@opentelemetry/instrumentation'; - -export interface PgResponseHookInformation { - data: QueryResult | QueryArrayResult; -} - -export interface PgInstrumentationExecutionResponseHook { - (span: api.Span, responseInfo: PgResponseHookInformation): void; -} - -export interface PgRequestHookInformation { - query: { - text: string; - name?: string; - values?: unknown[]; - }; - connection: { - database?: string; - host?: string; - port?: number; - user?: string; - }; -} - -export interface PgInstrumentationExecutionRequestHook { - (span: api.Span, queryInfo: PgRequestHookInformation): void; -} +import type { InstrumentationConfig } from '@opentelemetry/instrumentation'; export interface PgInstrumentationConfig extends InstrumentationConfig { - /** - * If true, an attribute containing the query's parameters will be attached - * the spans generated to represent the query. - */ - enhancedDatabaseReporting?: boolean; - - /** - * Hook that allows adding custom span attributes or updating the - * span's name based on the data about the query to execute. - * - * @default undefined - */ - requestHook?: PgInstrumentationExecutionRequestHook; - - /** - * Hook that allows adding custom span attributes based on the data - * returned from "query" Pg actions. - * - * @default undefined - */ - responseHook?: PgInstrumentationExecutionResponseHook; - - /** - * If true, requires a parent span to create new spans. - * - * @default false - */ - requireParentSpan?: boolean; - - /** - * If true, queries are modified to also include a comment with - * the tracing context, following the {@link https://github.com/open-telemetry/opentelemetry-sqlcommenter sqlcommenter} format - */ - addSqlCommenterCommentToQueries?: boolean; - /** * If true, `pg.connect` and `pg-pool.connect` spans will not be created. - * Query spans and pool metrics are still recorded. + * Query spans are still recorded. * * @default false */ diff --git a/packages/node/src/integrations/tracing/postgres/vendored/utils.ts b/packages/node/src/integrations/tracing/postgres/vendored/utils.ts index a22036674243..12976d2011e7 100644 --- a/packages/node/src/integrations/tracing/postgres/vendored/utils.ts +++ b/packages/node/src/integrations/tracing/postgres/vendored/utils.ts @@ -17,57 +17,35 @@ * - Vendored from: https://github.com/open-telemetry/opentelemetry-js-contrib/tree/15ef7506553f631ea4181391e0c5725a56f0d082/packages/instrumentation-pg * - Upstream version: @opentelemetry/instrumentation-pg@0.70.0 * - Types from `pg` package inlined as simplified interfaces - * - Minor TypeScript strictness adjustments + * - Refactored to use Sentry's span APIs instead of OpenTelemetry tracing APIs */ -/* eslint-disable */ -import { - context, - trace, - Span, - SpanStatusCode, - Tracer, - SpanKind, - diag, - UpDownCounter, - Attributes, -} from '@opentelemetry/api'; +import { context, SpanKind, trace } from '@opentelemetry/api'; +import type { Span, SpanAttributes } from '@sentry/core'; +import { SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, SPAN_STATUS_ERROR, startInactiveSpan } from '@sentry/core'; import { AttributeNames } from './enums/AttributeNames'; -import { - ATTR_ERROR_TYPE, - ATTR_DB_SYSTEM_NAME, - ATTR_DB_NAMESPACE, - ATTR_SERVER_ADDRESS, - ATTR_SERVER_PORT, - ATTR_DB_QUERY_TEXT, - DB_SYSTEM_NAME_VALUE_POSTGRESQL, -} from '@opentelemetry/semantic-conventions'; -import { - ATTR_DB_CLIENT_CONNECTION_POOL_NAME, - ATTR_DB_CLIENT_CONNECTION_STATE, - DB_CLIENT_CONNECTION_STATE_VALUE_USED, - DB_CLIENT_CONNECTION_STATE_VALUE_IDLE, - ATTR_DB_SYSTEM, - ATTR_DB_NAME, - ATTR_DB_USER, - DB_SYSTEM_VALUE_POSTGRESQL, - ATTR_DB_CONNECTION_STRING, - ATTR_NET_PEER_PORT, - ATTR_NET_PEER_NAME, - ATTR_DB_STATEMENT, -} from './semconv'; -import { +import { SpanNames } from './enums/SpanNames'; +import type { PgClientExtended, - PostgresCallback, + PgParsedConnectionParams, PgPoolCallback, PgPoolExtended, - PgParsedConnectionParams, PgPoolOptionsParams, + PostgresCallback, } from './internal-types'; -import { PgInstrumentationConfig } from './types'; -import type { PgClient, QueryResult, QueryArrayResult } from './pg-types'; -import { safeExecuteInTheMiddle, SemconvStability } from '@opentelemetry/instrumentation'; -import { SpanNames } from './enums/SpanNames'; +import type { PgClient } from './pg-types'; +import { + ATTR_DB_CONNECTION_STRING, + ATTR_DB_NAME, + ATTR_DB_STATEMENT, + ATTR_DB_SYSTEM, + ATTR_DB_USER, + ATTR_NET_PEER_NAME, + ATTR_NET_PEER_PORT, + DB_SYSTEM_VALUE_POSTGRESQL, +} from './semconv'; + +export const ORIGIN = 'auto.db.otel.postgres'; /** * Helper function to get a low cardinality span name from whatever info we have @@ -88,7 +66,7 @@ import { SpanNames } from './enums/SpanNames'; * `client.query()`. This will be undefined if `client.query()` was called * with invalid arguments. */ -export function getQuerySpanName(dbName: string | undefined, queryConfig?: { text: string; name?: unknown }) { +export function getQuerySpanName(dbName: string | undefined, queryConfig?: { text: string; name?: unknown }): string { // NB: when the query config is invalid, we omit the dbName too, so that // someone (or some tool) reading the span name doesn't misinterpret the // dbName as being a prepared statement or sql commit name. @@ -104,7 +82,7 @@ export function getQuerySpanName(dbName: string | undefined, queryConfig?: { tex return `${SpanNames.QUERY_PREFIX}:${command}${dbName ? ` ${dbName}` : ''}`; } -export function parseNormalizedOperationName(queryText: string) { +export function parseNormalizedOperationName(queryText: string): string { // Trim the query text to handle leading/trailing whitespace const trimmedQuery = queryText.trim(); const indexOfFirstSpace = trimmedQuery.indexOf(' '); @@ -125,13 +103,13 @@ export function parseAndMaskConnectionString(connectionString: string): string { url.password = ''; return url.toString(); - } catch (e) { + } catch { // If parsing fails, return a generic connection string return 'postgresql://localhost:5432/'; } } -export function getConnectionString(params: PgParsedConnectionParams | PgPoolOptionsParams) { +export function getConnectionString(params: PgParsedConnectionParams | PgPoolOptionsParams): string { if ('connectionString' in params && params.connectionString) { return parseAndMaskConnectionString(params.connectionString); } @@ -152,96 +130,63 @@ function getPort(port: number | undefined): number | undefined { return undefined; } -export function getSemanticAttributesFromConnection( - params: PgParsedConnectionParams, - semconvStability: SemconvStability, -) { - let attributes: Attributes = {}; - - if (semconvStability & SemconvStability.OLD) { - attributes = { - ...attributes, - [ATTR_DB_SYSTEM]: DB_SYSTEM_VALUE_POSTGRESQL, - [ATTR_DB_NAME]: params.database, - [ATTR_DB_CONNECTION_STRING]: getConnectionString(params), - [ATTR_DB_USER]: params.user, - [ATTR_NET_PEER_NAME]: params.host, // required - [ATTR_NET_PEER_PORT]: getPort(params.port), - }; - } - if (semconvStability & SemconvStability.STABLE) { - attributes = { - ...attributes, - [ATTR_DB_SYSTEM_NAME]: DB_SYSTEM_NAME_VALUE_POSTGRESQL, - [ATTR_DB_NAMESPACE]: params.namespace, - [ATTR_SERVER_ADDRESS]: params.host, - [ATTR_SERVER_PORT]: getPort(params.port), - }; - } - - return attributes; +export function getSemanticAttributesFromConnection(params: PgParsedConnectionParams): SpanAttributes { + return { + [ATTR_DB_SYSTEM]: DB_SYSTEM_VALUE_POSTGRESQL, + [ATTR_DB_NAME]: params.database, + [ATTR_DB_CONNECTION_STRING]: getConnectionString(params), + [ATTR_DB_USER]: params.user, + [ATTR_NET_PEER_NAME]: params.host, // required + [ATTR_NET_PEER_PORT]: getPort(params.port), + }; } -export function getSemanticAttributesFromPoolConnection( - params: PgPoolOptionsParams, - semconvStability: SemconvStability, -) { +export function getSemanticAttributesFromPoolConnection(params: PgPoolOptionsParams): SpanAttributes { let url: URL | undefined; try { url = params.connectionString ? new URL(params.connectionString) : undefined; - } catch (e) { + } catch { url = undefined; } - let attributes: Attributes = { + + return { [AttributeNames.IDLE_TIMEOUT_MILLIS]: params.idleTimeoutMillis, [AttributeNames.MAX_CLIENT]: params.maxClient, + [ATTR_DB_SYSTEM]: DB_SYSTEM_VALUE_POSTGRESQL, + [ATTR_DB_NAME]: url?.pathname.slice(1) ?? params.database, + [ATTR_DB_CONNECTION_STRING]: getConnectionString(params), + [ATTR_NET_PEER_NAME]: url?.hostname ?? params.host, + [ATTR_NET_PEER_PORT]: Number(url?.port) || getPort(params.port), + [ATTR_DB_USER]: url?.username ?? params.user, }; - - if (semconvStability & SemconvStability.OLD) { - attributes = { - ...attributes, - [ATTR_DB_SYSTEM]: DB_SYSTEM_VALUE_POSTGRESQL, - [ATTR_DB_NAME]: url?.pathname.slice(1) ?? params.database, - [ATTR_DB_CONNECTION_STRING]: getConnectionString(params), - [ATTR_NET_PEER_NAME]: url?.hostname ?? params.host, - [ATTR_NET_PEER_PORT]: Number(url?.port) || getPort(params.port), - [ATTR_DB_USER]: url?.username ?? params.user, - }; - } - if (semconvStability & SemconvStability.STABLE) { - attributes = { - ...attributes, - [ATTR_DB_SYSTEM_NAME]: DB_SYSTEM_NAME_VALUE_POSTGRESQL, - [ATTR_DB_NAMESPACE]: params.namespace, - [ATTR_SERVER_ADDRESS]: url?.hostname ?? params.host, - [ATTR_SERVER_PORT]: Number(url?.port) || getPort(params.port), - }; - } - - return attributes; } -export function shouldSkipInstrumentation(instrumentationConfig: PgInstrumentationConfig) { - return instrumentationConfig.requireParentSpan === true && trace.getSpan(context.active()) === undefined; +/** + * The SDK always requires a parent span (it sets `requireParentSpan: true`), so + * we only instrument when there is an active span to parent the new span under. + */ +export function shouldSkipInstrumentation(): boolean { + return trace.getSpan(context.active()) === undefined; } -// Create a span from our normalized queryConfig object, +// Create an (inactive) span from our normalized queryConfig object, // or return a basic span if no queryConfig was given/could be created. export function handleConfigQuery( this: PgClientExtended, - tracer: Tracer, - instrumentationConfig: PgInstrumentationConfig, - semconvStability: SemconvStability, queryConfig?: { text: string; values?: unknown; name?: unknown }, -) { +): Span { // Create child span. const { connectionParameters } = this; const dbName = connectionParameters.database; const spanName = getQuerySpanName(dbName, queryConfig); - const span = tracer.startSpan(spanName, { + const span = startInactiveSpan({ + name: spanName, kind: SpanKind.CLIENT, - attributes: getSemanticAttributesFromConnection(connectionParameters, semconvStability), + attributes: { + ...getSemanticAttributesFromConnection(connectionParameters), + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: ORIGIN, + }, }); if (!queryConfig) { @@ -250,35 +195,7 @@ export function handleConfigQuery( // Set attributes if (queryConfig.text) { - if (semconvStability & SemconvStability.OLD) { - span.setAttribute(ATTR_DB_STATEMENT, queryConfig.text); - } - if (semconvStability & SemconvStability.STABLE) { - span.setAttribute(ATTR_DB_QUERY_TEXT, queryConfig.text); - } - } - - if (instrumentationConfig.enhancedDatabaseReporting && Array.isArray(queryConfig.values)) { - try { - const convertedValues = queryConfig.values.map(value => { - if (value == null) { - return 'null'; - } else if (value instanceof Buffer) { - return value.toString(); - } else if (typeof value === 'object') { - if (typeof value.toPostgres === 'function') { - return value.toPostgres(); - } - return JSON.stringify(value); - } else { - //string, number - return value.toString(); - } - }); - span.setAttribute(AttributeNames.PG_VALUES, convertedValues); - } catch (e) { - diag.error('failed to stringify ', queryConfig.values, e); - } + span.setAttribute(ATTR_DB_STATEMENT, queryConfig.text); } // Set plan name attribute, if present @@ -289,130 +206,34 @@ export function handleConfigQuery( return span; } -export function handleExecutionResult( - config: PgInstrumentationConfig, - span: Span, - pgResult: QueryResult | QueryArrayResult | unknown, -) { - if (typeof config.responseHook === 'function') { - safeExecuteInTheMiddle( - () => { - config.responseHook!(span, { - data: pgResult as QueryResult | QueryArrayResult, - }); - }, - err => { - if (err) { - diag.error('Error running response hook', err); - } - }, - true, - ); - } -} - -export function patchCallback( - instrumentationConfig: PgInstrumentationConfig, - span: Span, - cb: PostgresCallback, - attributes: Attributes, - recordDuration: { (): void }, -): PostgresCallback { +export function patchCallback(span: Span, cb: PostgresCallback): PostgresCallback { return function patchedCallback(this: PgClientExtended, err: Error, res: object) { if (err) { - if (Object.prototype.hasOwnProperty.call(err, 'code')) { - attributes[ATTR_ERROR_TYPE] = (err as any)['code']; - } - if (err instanceof Error) { - span.recordException(sanitizedErrorMessage(err)); - } - span.setStatus({ - code: SpanStatusCode.ERROR, - message: err.message, - }); - } else { - handleExecutionResult(instrumentationConfig, span, res); + span.setStatus({ code: SPAN_STATUS_ERROR, message: err.message }); } - - recordDuration(); span.end(); cb.call(this, err, res); }; } -export function getPoolName(pool: PgPoolOptionsParams): string { - let poolName = ''; - poolName += (pool?.host ? `${pool.host}` : 'unknown_host') + ':'; - poolName += (pool?.port ? `${pool.port}` : 'unknown_port') + '/'; - poolName += pool?.database ? `${pool.database}` : 'unknown_database'; - - return poolName.trim(); -} - -export interface poolConnectionsCounter { - used: number; - idle: number; - pending: number; -} - -export function updateCounter( - poolName: string, - pool: PgPoolExtended, - connectionCount: UpDownCounter, - connectionPendingRequests: UpDownCounter, - latestCounter: poolConnectionsCounter, -): poolConnectionsCounter { - const all = pool.totalCount; - const pending = pool.waitingCount; - const idle = pool.idleCount; - const used = all - idle; - - connectionCount.add(used - latestCounter.used, { - [ATTR_DB_CLIENT_CONNECTION_STATE]: DB_CLIENT_CONNECTION_STATE_VALUE_USED, - [ATTR_DB_CLIENT_CONNECTION_POOL_NAME]: poolName, - }); - - connectionCount.add(idle - latestCounter.idle, { - [ATTR_DB_CLIENT_CONNECTION_STATE]: DB_CLIENT_CONNECTION_STATE_VALUE_IDLE, - [ATTR_DB_CLIENT_CONNECTION_POOL_NAME]: poolName, - }); - - connectionPendingRequests.add(pending - latestCounter.pending, { - [ATTR_DB_CLIENT_CONNECTION_POOL_NAME]: poolName, - }); - - return { used: used, idle: idle, pending: pending }; -} - export function patchCallbackPGPool(span: Span, cb: PgPoolCallback): PgPoolCallback { return function patchedCallback(this: PgPoolExtended, err: Error, res: object, done: any) { if (err) { - if (err instanceof Error) { - span.recordException(sanitizedErrorMessage(err)); - } - span.setStatus({ - code: SpanStatusCode.ERROR, - message: err.message, - }); + span.setStatus({ code: SPAN_STATUS_ERROR, message: err.message }); } span.end(); cb.call(this, err, res, done); }; } -export function patchClientConnectCallback(span: Span, cb: Function): Function { - return function patchedClientConnectCallback(this: PgClient, err: Error) { - if (err) { - if (err instanceof Error) { - span.recordException(sanitizedErrorMessage(err)); - } - span.setStatus({ - code: SpanStatusCode.ERROR, - message: err.message, - }); +export function patchClientConnectCallback(span: Span, cb: (...args: unknown[]) => void): (...args: unknown[]) => void { + return function patchedClientConnectCallback(this: PgClient, ...args: unknown[]) { + const err = args[0]; + if (err instanceof Error) { + span.setStatus({ code: SPAN_STATUS_ERROR, message: err.message }); } span.end(); - cb.apply(this, arguments); + cb.apply(this, args); }; } @@ -421,7 +242,7 @@ export function patchClientConnectCallback(span: Span, cb: Function): Function { * defensive, to recognize the fact that, in JS, any kind of value (even * primitives) can be thrown. */ -export function getErrorMessage(e: unknown) { +export function getErrorMessage(e: unknown): string | undefined { return typeof e === 'object' && e !== null && 'message' in e ? String((e as { message?: unknown }).message) : undefined; @@ -435,14 +256,3 @@ export type ObjectWithText = { text: string; [k: string]: unknown; }; - -/** - * Generates a sanitized message for the error. - * Only includes the error type and PostgreSQL error code, omitting any sensitive details. - */ -export function sanitizedErrorMessage(error: unknown): string { - const name = (error as any)?.name ?? 'PostgreSQLError'; - const code = (error as any)?.code ?? 'UNKNOWN'; - - return `PostgreSQL error of type '${name}' occurred (code: ${code})`; -} diff --git a/packages/node/test/integrations/tracing/postgres.test.ts b/packages/node/test/integrations/tracing/postgres.test.ts deleted file mode 100644 index bdb9a07a6594..000000000000 --- a/packages/node/test/integrations/tracing/postgres.test.ts +++ /dev/null @@ -1,93 +0,0 @@ -import { PgInstrumentation } from '../../../src/integrations/tracing/postgres/vendored/instrumentation'; -import { INSTRUMENTED } from '@sentry/node-core'; -import { beforeEach, describe, expect, it, type MockInstance, vi } from 'vitest'; -import { instrumentPostgres, postgresIntegration } from '../../../src/integrations/tracing/postgres'; - -vi.mock('../../../src/integrations/tracing/postgres/vendored/instrumentation'); - -describe('postgres integration', () => { - beforeEach(() => { - vi.clearAllMocks(); - delete INSTRUMENTED.Postgres; - - (PgInstrumentation as unknown as MockInstance).mockImplementation(() => ({ - setTracerProvider: () => undefined, - setMeterProvider: () => undefined, - getConfig: () => ({}), - setConfig: () => ({}), - enable: () => undefined, - })); - }); - - it('has a name and setupOnce method', () => { - const integration = postgresIntegration(); - expect(integration.name).toBe('Postgres'); - expect(typeof integration.setupOnce).toBe('function'); - }); - - it('passes ignoreConnectSpans: true to PgInstrumentation when set on integration', () => { - postgresIntegration({ ignoreConnectSpans: true }).setupOnce!(); - - expect(PgInstrumentation).toHaveBeenCalledTimes(1); - expect(PgInstrumentation).toHaveBeenCalledWith({ - requireParentSpan: true, - requestHook: expect.any(Function), - ignoreConnectSpans: true, - }); - }); - - it('passes ignoreConnectSpans: false to PgInstrumentation by default', () => { - postgresIntegration().setupOnce!(); - - expect(PgInstrumentation).toHaveBeenCalledTimes(1); - expect(PgInstrumentation).toHaveBeenCalledWith({ - requireParentSpan: true, - requestHook: expect.any(Function), - ignoreConnectSpans: false, - }); - }); - - it('instrumentPostgres receives ignoreConnectSpans option', () => { - instrumentPostgres({ ignoreConnectSpans: true }); - - expect(PgInstrumentation).toHaveBeenCalledTimes(1); - expect(PgInstrumentation).toHaveBeenCalledWith({ - requireParentSpan: true, - requestHook: expect.any(Function), - ignoreConnectSpans: true, - }); - }); - - it('second call to instrumentPostgres passes full config to setConfig, not raw user options', () => { - const mockSetConfig = vi.fn(); - (PgInstrumentation as unknown as MockInstance).mockImplementation(() => ({ - setTracerProvider: () => undefined, - setMeterProvider: () => undefined, - getConfig: () => ({}), - setConfig: mockSetConfig, - enable: () => undefined, - })); - - instrumentPostgres({ ignoreConnectSpans: true }); - expect(PgInstrumentation).toHaveBeenCalledWith( - expect.objectContaining({ - requireParentSpan: true, - ignoreConnectSpans: true, - requestHook: expect.any(Function), - }), - ); - - mockSetConfig.mockClear(); - instrumentPostgres({ ignoreConnectSpans: false }); - - expect(PgInstrumentation).toHaveBeenCalledTimes(1); - expect(mockSetConfig).toHaveBeenCalledTimes(1); - expect(mockSetConfig).toHaveBeenCalledWith( - expect.objectContaining({ - requireParentSpan: true, - ignoreConnectSpans: false, - requestHook: expect.any(Function), - }), - ); - }); -}); From f5227f9c3ddba695085ebb97d31fbfd6fa4707e5 Mon Sep 17 00:00:00 2001 From: Abdelrahman Awad Date: Wed, 17 Jun 2026 10:44:13 -0400 Subject: [PATCH 2/4] ref(node): Remove remaining OTel tracing APIs from pg instrumentation 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. --- .../postgres/vendored/instrumentation.ts | 55 +++++++++++-------- .../tracing/postgres/vendored/utils.ts | 6 +- 2 files changed, 36 insertions(+), 25 deletions(-) diff --git a/packages/node/src/integrations/tracing/postgres/vendored/instrumentation.ts b/packages/node/src/integrations/tracing/postgres/vendored/instrumentation.ts index 635ec0c2d26e..6b3181d501bd 100644 --- a/packages/node/src/integrations/tracing/postgres/vendored/instrumentation.ts +++ b/packages/node/src/integrations/tracing/postgres/vendored/instrumentation.ts @@ -23,10 +23,10 @@ * enhancedDatabaseReporting, addSqlCommenterCommentToQueries) */ -import { context, SpanKind, trace } from '@opentelemetry/api'; +import { SpanKind } from '@opentelemetry/api'; import { InstrumentationBase, InstrumentationNodeModuleDefinition, isWrapped } from '@opentelemetry/instrumentation'; import type { Span } from '@sentry/core'; -import { SDK_VERSION, SPAN_STATUS_ERROR, startInactiveSpan, withActiveSpan } from '@sentry/core'; +import { getActiveSpan, SDK_VERSION, SPAN_STATUS_ERROR, startInactiveSpan, withActiveSpan } from '@sentry/core'; import { InstrumentationNodeModuleFile } from '../../InstrumentationNodeModuleFile'; import { SpanNames } from './enums/SpanNames'; import type { @@ -47,6 +47,18 @@ function extractModuleExports(module: any): any { : module; // CommonJS } +/** + * Binds `callback` to `parentSpan`, so that the span is active again whenever the + * (deferred) callback runs. This mirrors the upstream `context.bind(context.active(), callback)`: + * `pg` invokes callbacks outside of the original async scope (e.g. for pooled connections), so we + * re-establish the trace context to keep spans created inside the callback correctly parented. + */ +function bindCallbackToSpan any>(parentSpan: Span, callback: T): T { + return function (this: unknown, ...args: any[]): unknown { + return withActiveSpan(parentSpan, () => callback.apply(this, args)); + } as T; +} + export class PgInstrumentation extends InstrumentationBase { public constructor(config: PgInstrumentationConfig = {}) { super(PACKAGE_NAME, SDK_VERSION, config); @@ -162,10 +174,10 @@ export class PgInstrumentation extends InstrumentationBase 0) { - const parentSpan = trace.getSpan(context.active()); + const parentSpan = getActiveSpan(); if (typeof args[args.length - 1] === 'function') { // Patch ParameterQuery callback args[args.length - 1] = utils.patchCallback(span, args[args.length - 1] as PostgresCallback); // If a parent span exists, bind the callback if (parentSpan) { - args[args.length - 1] = context.bind(context.active(), args[args.length - 1]); + args[args.length - 1] = bindCallbackToSpan(parentSpan, args[args.length - 1] as PostgresCallback); } } else if (typeof queryConfig?.callback === 'function') { // Patch ConfigQuery callback @@ -230,7 +242,7 @@ export class PgInstrumentation extends InstrumentationBase; - return context.bind( - context.active(), - connectResultPromise - .then(result => { - span.end(); - return result; - }) - .catch((error: unknown) => { - span.setStatus({ code: SPAN_STATUS_ERROR, message: utils.getErrorMessage(error) }); - span.end(); - return Promise.reject(error); - }), - ); + // The caller's continuation after `await client.connect()` keeps its trace context via the + // SDK's async context propagation, so we don't need to re-bind the returned promise. + return connectResultPromise + .then(result => { + span.end(); + return result; + }) + .catch((error: unknown) => { + span.setStatus({ code: SPAN_STATUS_ERROR, message: utils.getErrorMessage(error) }); + span.end(); + return Promise.reject(error); + }); } diff --git a/packages/node/src/integrations/tracing/postgres/vendored/utils.ts b/packages/node/src/integrations/tracing/postgres/vendored/utils.ts index 12976d2011e7..203da8fb2e07 100644 --- a/packages/node/src/integrations/tracing/postgres/vendored/utils.ts +++ b/packages/node/src/integrations/tracing/postgres/vendored/utils.ts @@ -20,9 +20,9 @@ * - Refactored to use Sentry's span APIs instead of OpenTelemetry tracing APIs */ -import { context, SpanKind, trace } from '@opentelemetry/api'; +import { SpanKind } from '@opentelemetry/api'; import type { Span, SpanAttributes } from '@sentry/core'; -import { SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, SPAN_STATUS_ERROR, startInactiveSpan } from '@sentry/core'; +import { getActiveSpan, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, SPAN_STATUS_ERROR, startInactiveSpan } from '@sentry/core'; import { AttributeNames } from './enums/AttributeNames'; import { SpanNames } from './enums/SpanNames'; import type { @@ -166,7 +166,7 @@ export function getSemanticAttributesFromPoolConnection(params: PgPoolOptionsPar * we only instrument when there is an active span to parent the new span under. */ export function shouldSkipInstrumentation(): boolean { - return trace.getSpan(context.active()) === undefined; + return getActiveSpan() === undefined; } // Create an (inactive) span from our normalized queryConfig object, From 321e80df9f721863a5a29c85b907482ef1a71eb7 Mon Sep 17 00:00:00 2001 From: Abdelrahman Awad Date: Wed, 17 Jun 2026 11:34:00 -0400 Subject: [PATCH 3/4] test(node): Cover pg connect-promise continuation parenting 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. --- .../postgres/scenario-connect-then.mjs | 25 +++++++++++++ .../suites/tracing/postgres/test.ts | 37 +++++++++++++++++++ 2 files changed, 62 insertions(+) create mode 100644 dev-packages/node-integration-tests/suites/tracing/postgres/scenario-connect-then.mjs diff --git a/dev-packages/node-integration-tests/suites/tracing/postgres/scenario-connect-then.mjs b/dev-packages/node-integration-tests/suites/tracing/postgres/scenario-connect-then.mjs new file mode 100644 index 000000000000..7c10d0e6f1f1 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/postgres/scenario-connect-then.mjs @@ -0,0 +1,25 @@ +import * as Sentry from '@sentry/node'; +import { Client } from 'pg'; + +async function run() { + await Sentry.startSpan( + { + name: 'Test Transaction', + op: 'transaction', + }, + () => + // Chain off `connect()` with `.then()` instead of awaiting it: the query + // issued from the continuation must still be parented to the active + // transaction, proving the trace context survives the connect promise. + new Promise((resolve, reject) => { + const client = new Client({ port: 5494, user: 'test', password: 'test', database: 'tests' }); + client + .connect() + .then(() => client.query('SELECT 1 AS connect_then')) + .then(() => client.end()) + .then(resolve, reject); + }), + ); +} + +run(); diff --git a/dev-packages/node-integration-tests/suites/tracing/postgres/test.ts b/dev-packages/node-integration-tests/suites/tracing/postgres/test.ts index 0a801aab4046..a24b11efee42 100644 --- a/dev-packages/node-integration-tests/suites/tracing/postgres/test.ts +++ b/dev-packages/node-integration-tests/suites/tracing/postgres/test.ts @@ -219,6 +219,43 @@ describe('postgres auto instrumentation', () => { }); }); + // A query chained off `connect()` with `.then()` (rather than awaited) must still + // be parented to the active transaction. Since the instrumentation requires a + // parent span, the query only produces a span if the trace context survives the + // connect promise's continuation. + describe('connect promise continuation', () => { + const EXPECTED_TRANSACTION = { + transaction: 'Test Transaction', + spans: expect.arrayContaining([ + expect.objectContaining({ + data: expect.objectContaining({ + 'db.system': 'postgresql', + 'db.name': 'tests', + 'db.statement': 'SELECT 1 AS connect_then', + 'sentry.origin': 'auto.db.otel.postgres', + 'sentry.op': 'db', + }), + description: 'SELECT 1 AS connect_then', + op: 'db', + status: 'ok', + origin: 'auto.db.otel.postgres', + }), + ]), + }; + + createEsmAndCjsTests(__dirname, 'scenario-connect-then.mjs', 'instrument.mjs', (createTestRunner, test) => { + test('parents a query chained off connect() to the active transaction', { timeout: 90_000 }, async () => { + await createTestRunner() + .withDockerCompose({ + workingDirectory: [__dirname], + }) + .expect({ transaction: EXPECTED_TRANSACTION }) + .start() + .completed(); + }); + }); + }); + describe('requireParentSpan', () => { createEsmAndCjsTests(__dirname, 'scenario-no-parent.mjs', 'instrument.mjs', (createTestRunner, test) => { test('does not instrument queries or connects without an active parent span', { timeout: 90_000 }, async () => { From 94658175ac0a58e20a346198726985e489f5e6eb Mon Sep 17 00:00:00 2001 From: Abdelrahman Awad Date: Wed, 17 Jun 2026 12:21:02 -0400 Subject: [PATCH 4/4] ci: Warn (not error) on no-deprecated in vendored instrumentations 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. --- .oxlintrc.base.json | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.oxlintrc.base.json b/.oxlintrc.base.json index bf47e2e7a51d..db897a6e5805 100644 --- a/.oxlintrc.base.json +++ b/.oxlintrc.base.json @@ -160,7 +160,8 @@ "rules": { "typescript/no-explicit-any": "off", "no-unsafe-member-access": "off", - "no-this-alias": "off" + "no-this-alias": "off", + "typescript/no-deprecated": "warn" } }, {