Skip to content

ref(node): Streamline knex instrumentation #21561

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
logaretm wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

ref(node): Streamline knex instrumentation #21561

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a32abea
ref(node): Streamline knex instrumentation
logaretm Jun 15, 2026
e3cc97a
ref(node): Remove remaining OTel tracing APIs from knex instrumentation
logaretm Jun 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions dev-packages/node-integration-tests/suites/tracing/knex/pg/scenario.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,11 @@ async function run() {

await pgClient('User').insert({ name: 'bob', email: 'bob@domain.com' });
await pgClient('User').select('*');

// Trigger a failing query to capture the error span (table does not exist).
await pgClient('DoesNotExist')
.select('*')
.catch(() => {});
} finally {
await pgClient.destroy();
}
Expand Down
16 changes: 16 additions & 0 deletions dev-packages/node-integration-tests/suites/tracing/knex/pg/test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,22 @@ describe('knex auto instrumentation', () => {
description: 'select * from "User"',
origin: 'auto.db.otel.knex',
}),

expect.objectContaining({
data: expect.objectContaining({
'knex.version': KNEX_VERSION,
'db.operation': 'select',
'db.sql.table': 'DoesNotExist',
'db.system': 'postgresql',
'db.name': 'tests',
'db.statement': 'select * from "DoesNotExist"',
'sentry.origin': 'auto.db.otel.knex',
'sentry.op': 'db',
}),
status: 'internal_error',
Comment thread
logaretm marked this conversation as resolved.
description: 'select * from "DoesNotExist"',
origin: 'auto.db.otel.knex',
}),
]),
};

Expand Down
27 changes: 4 additions & 23 deletions packages/node/src/integrations/tracing/knex/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,36 +1,17 @@
import { KnexInstrumentation } from './vendored/instrumentation';
import type { IntegrationFn } from '@sentry/core';
import { defineIntegration, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, spanToJSON } from '@sentry/core';
import { generateInstrumentOnce, instrumentWhenWrapped } from '@sentry/node-core';
import { defineIntegration } from '@sentry/core';
import { generateInstrumentOnce } from '@sentry/node-core';

const INTEGRATION_NAME = 'Knex';

export const instrumentKnex = generateInstrumentOnce(
INTEGRATION_NAME,
() => new KnexInstrumentation({ requireParentSpan: true }),
);
export const instrumentKnex = generateInstrumentOnce(INTEGRATION_NAME, () => new KnexInstrumentation());

const _knexIntegration = (() => {
let instrumentationWrappedCallback: undefined | ((callback: () => void) => void);

return {
name: INTEGRATION_NAME,
setupOnce() {
const instrumentation = instrumentKnex();
instrumentationWrappedCallback = instrumentWhenWrapped(instrumentation);
},

setup(client) {
instrumentationWrappedCallback?.(() =>
client.on('spanStart', span => {
const { data } = spanToJSON(span);
// knex.version is always set in the span data
// https://github.com/open-telemetry/opentelemetry-js-contrib/blob/0309caeafc44ac9cb13a3345b790b01b76d0497d/plugins/node/opentelemetry-instrumentation-knex/src/instrumentation.ts#L138
if ('knex.version' in data) {
span.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, 'auto.db.otel.knex');
}
}),
);
instrumentKnex();
},
};
}) satisfies IntegrationFn;
Expand Down
31 changes: 0 additions & 31 deletions packages/node/src/integrations/tracing/knex/vendored/constants.ts
View file
Open in desktop

This file was deleted.

215 changes: 88 additions & 127 deletions packages/node/src/integrations/tracing/knex/vendored/instrumentation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,31 +17,21 @@
* - Vendored from: https://github.com/open-telemetry/opentelemetry-js-contrib/tree/15ef7506553f631ea4181391e0c5725a56f0d082/packages/instrumentation-knex
* - Upstream version: @opentelemetry/instrumentation-knex@0.62.0
* - Minor TypeScript strictness adjustments for this repository's compiler settings
* - Refactored to use Sentry's span APIs instead of OpenTelemetry tracing APIs
*/
/* eslint-disable */

import * as api from '@opentelemetry/api';
import { SDK_VERSION } from '@sentry/core';
import * as constants from './constants';
import { SpanKind } from '@opentelemetry/api';
import type { InstrumentationConfig } from '@opentelemetry/instrumentation';
import { InstrumentationBase, InstrumentationNodeModuleDefinition, isWrapped } from '@opentelemetry/instrumentation';
import type { Span, SpanAttributes } from '@sentry/core';
import {
InstrumentationBase,
InstrumentationNodeModuleDefinition,
isWrapped,
SemconvStability,
semconvStabilityFromStr,
} from '@opentelemetry/instrumentation';
getActiveSpan,
SDK_VERSION,
SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN,
SPAN_STATUS_ERROR,
startSpan,
} from '@sentry/core';
import { InstrumentationNodeModuleFile } from '../../InstrumentationNodeModuleFile';
import * as utils from './utils';
import { KnexInstrumentationConfig } from './types';
import {
ATTR_DB_COLLECTION_NAME,
ATTR_DB_NAMESPACE,
ATTR_DB_OPERATION_NAME,
ATTR_DB_QUERY_TEXT,
ATTR_DB_SYSTEM_NAME,
ATTR_SERVER_ADDRESS,
ATTR_SERVER_PORT,
} from '@opentelemetry/semantic-conventions';
import {
ATTR_DB_NAME,
ATTR_DB_OPERATION,
Expand All @@ -53,65 +43,69 @@
ATTR_NET_PEER_PORT,
ATTR_NET_TRANSPORT,
} from './semconv';
import * as utils from './utils';

const PACKAGE_NAME = '@sentry/instrumentation-knex';

const contextSymbol = Symbol('opentelemetry.instrumentation-knex.context');
const DEFAULT_CONFIG: KnexInstrumentationConfig = {
maxQueryLength: 1022,
requireParentSpan: false,
};

export class KnexInstrumentation extends InstrumentationBase<KnexInstrumentationConfig> {
private _semconvStability: SemconvStability;

constructor(config: KnexInstrumentationConfig = {}) {
super(PACKAGE_NAME, SDK_VERSION, { ...DEFAULT_CONFIG, ...config });

this._semconvStability = semconvStabilityFromStr('database', process.env.OTEL_SEMCONV_STABILITY_OPT_IN);
}

override setConfig(config: KnexInstrumentationConfig = {}) {
super.setConfig({ ...DEFAULT_CONFIG, ...config });
const ORIGIN = 'auto.db.otel.knex';

const MODULE_NAME = 'knex';
const SUPPORTED_VERSIONS = [
// use "lib/execution" for runner.js, "lib" for client.js as basepath, latest tested 0.95.6
'>=0.22.0 <4',
// use "lib" as basepath
'>=0.10.0 <0.18.0',
'>=0.19.0 <0.22.0',
// use "src" as basepath
'>=0.18.0 <0.19.0',
];

// Max length of the query text captured in the `db.statement` attribute; ".." is appended when truncated.
const MAX_QUERY_LENGTH = 1022;

const parentSpanSymbol = Symbol('sentry.instrumentation-knex.parent-span');

export class KnexInstrumentation extends InstrumentationBase<InstrumentationConfig> {
public constructor(config: InstrumentationConfig = {}) {
super(PACKAGE_NAME, SDK_VERSION, config);
}

init() {
const module = new InstrumentationNodeModuleDefinition(constants.MODULE_NAME, constants.SUPPORTED_VERSIONS);
public init(): InstrumentationNodeModuleDefinition {
const module = new InstrumentationNodeModuleDefinition(MODULE_NAME, SUPPORTED_VERSIONS);

module.files.push(
this.getClientNodeModuleFileInstrumentation('src'),
this.getClientNodeModuleFileInstrumentation('lib'),
this.getRunnerNodeModuleFileInstrumentation('src'),
this.getRunnerNodeModuleFileInstrumentation('lib'),
this.getRunnerNodeModuleFileInstrumentation('lib/execution'),
this._getClientNodeModuleFileInstrumentation('src'),
this._getClientNodeModuleFileInstrumentation('lib'),
this._getRunnerNodeModuleFileInstrumentation('src'),
this._getRunnerNodeModuleFileInstrumentation('lib'),
this._getRunnerNodeModuleFileInstrumentation('lib/execution'),
);

return module;
}

private getRunnerNodeModuleFileInstrumentation(basePath: string) {
private _getRunnerNodeModuleFileInstrumentation(basePath: string): InstrumentationNodeModuleFile {
return new InstrumentationNodeModuleFile(
`knex/${basePath}/runner.js`,
constants.SUPPORTED_VERSIONS,
SUPPORTED_VERSIONS,
(Runner: any, moduleVersion?: string) => {
this.ensureWrapped(Runner.prototype, 'query', this.createQueryWrapper(moduleVersion));
this._ensureWrapped(Runner.prototype, 'query', this._createQueryWrapper(moduleVersion));
return Runner;
},
(Runner: any, _moduleVersion?: string) => {
(Runner: any) => {
this._unwrap(Runner.prototype, 'query');
return Runner;
},
);
}

private getClientNodeModuleFileInstrumentation(basePath: string) {
private _getClientNodeModuleFileInstrumentation(basePath: string): InstrumentationNodeModuleFile {
return new InstrumentationNodeModuleFile(
`knex/${basePath}/client.js`,
constants.SUPPORTED_VERSIONS,
SUPPORTED_VERSIONS,
(Client: any) => {
this.ensureWrapped(Client.prototype, 'queryBuilder', this.storeContext.bind(this));
this.ensureWrapped(Client.prototype, 'schemaBuilder', this.storeContext.bind(this));
this.ensureWrapped(Client.prototype, 'raw', this.storeContext.bind(this));
this._ensureWrapped(Client.prototype, 'queryBuilder', this._storeContext.bind(this));
this._ensureWrapped(Client.prototype, 'schemaBuilder', this._storeContext.bind(this));
this._ensureWrapped(Client.prototype, 'raw', this._storeContext.bind(this));
return Client;
},
(Client: any) => {
Expand All @@ -123,9 +117,7 @@
);
}

private createQueryWrapper(moduleVersion?: string) {
const instrumentation = this;

private _createQueryWrapper(moduleVersion?: string) {
return function wrapQuery(original: (...args: any[]) => any) {
return function wrapped_logging_method(this: any, query: any) {
const config = this.client.config;
Expand All @@ -137,93 +129,62 @@
config?.connection?.filename ||
config?.connection?.database ||
utils.extractDatabaseFromConnectionString(connectionString);
const { maxQueryLength } = instrumentation.getConfig();

const attributes: api.Attributes = {
const attributes: SpanAttributes = {
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: ORIGIN,
'knex.version': moduleVersion,
[ATTR_DB_SYSTEM]: utils.mapSystem(this.client.driverName),

Check failure on line 136 in packages/node/src/integrations/tracing/knex/vendored/instrumentation.ts

View workflow job for this annotation

GitHub Actions / Lint

typescript-eslint(no-deprecated) 

`ATTR_DB_SYSTEM` is deprecated.
[ATTR_DB_SQL_TABLE]: table,

Check failure on line 137 in packages/node/src/integrations/tracing/knex/vendored/instrumentation.ts

View workflow job for this annotation

GitHub Actions / Lint

typescript-eslint(no-deprecated) 

`ATTR_DB_SQL_TABLE` is deprecated.
[ATTR_DB_OPERATION]: operation,

Check failure on line 138 in packages/node/src/integrations/tracing/knex/vendored/instrumentation.ts

View workflow job for this annotation

GitHub Actions / Lint

typescript-eslint(no-deprecated) 

`ATTR_DB_OPERATION` is deprecated.
[ATTR_DB_USER]: config?.connection?.user,

Check failure on line 139 in packages/node/src/integrations/tracing/knex/vendored/instrumentation.ts

View workflow job for this annotation

GitHub Actions / Lint

typescript-eslint(no-deprecated) 

`ATTR_DB_USER` is deprecated.
[ATTR_DB_NAME]: name,

Check failure on line 140 in packages/node/src/integrations/tracing/knex/vendored/instrumentation.ts

View workflow job for this annotation

GitHub Actions / Lint

typescript-eslint(no-deprecated) 

`ATTR_DB_NAME` is deprecated.
[ATTR_NET_PEER_NAME]: config?.connection?.host ?? utils.extractHostFromConnectionString(connectionString),

Check failure on line 141 in packages/node/src/integrations/tracing/knex/vendored/instrumentation.ts

View workflow job for this annotation

GitHub Actions / Lint

typescript-eslint(no-deprecated) 

`ATTR_NET_PEER_NAME` is deprecated.
[ATTR_NET_PEER_PORT]: config?.connection?.port ?? utils.extractPortFromConnectionString(connectionString),

Check failure on line 142 in packages/node/src/integrations/tracing/knex/vendored/instrumentation.ts

View workflow job for this annotation

GitHub Actions / Lint

typescript-eslint(no-deprecated) 

`ATTR_NET_PEER_PORT` is deprecated.
[ATTR_NET_TRANSPORT]: config?.connection?.filename === ':memory:' ? 'inproc' : undefined,

Check failure on line 143 in packages/node/src/integrations/tracing/knex/vendored/instrumentation.ts

View workflow job for this annotation

GitHub Actions / Lint

typescript-eslint(no-deprecated) 

`ATTR_NET_TRANSPORT` is deprecated.
[ATTR_DB_STATEMENT]: utils.limitLength(query?.sql, MAX_QUERY_LENGTH),

Check failure on line 144 in packages/node/src/integrations/tracing/knex/vendored/instrumentation.ts

View workflow job for this annotation

GitHub Actions / Lint

typescript-eslint(no-deprecated) 

`ATTR_DB_STATEMENT` is deprecated.
};
const transport = config?.connection?.filename === ':memory:' ? 'inproc' : undefined;

if (instrumentation._semconvStability & SemconvStability.OLD) {
Object.assign(attributes, {
[ATTR_DB_SYSTEM]: utils.mapSystem(this.client.driverName),
[ATTR_DB_SQL_TABLE]: table,
[ATTR_DB_OPERATION]: operation,
[ATTR_DB_USER]: config?.connection?.user,
[ATTR_DB_NAME]: name,
[ATTR_NET_PEER_NAME]: config?.connection?.host ?? utils.extractHostFromConnectionString(connectionString),
[ATTR_NET_PEER_PORT]: config?.connection?.port ?? utils.extractPortFromConnectionString(connectionString),
[ATTR_NET_TRANSPORT]: transport,
});
}
if (instrumentation._semconvStability & SemconvStability.STABLE) {
Object.assign(attributes, {
[ATTR_DB_SYSTEM_NAME]: utils.mapSystem(this.client.driverName),
[ATTR_DB_COLLECTION_NAME]: table,
[ATTR_DB_OPERATION_NAME]: operation,
[ATTR_DB_NAMESPACE]: name,
[ATTR_SERVER_ADDRESS]: config?.connection?.host ?? utils.extractHostFromConnectionString(connectionString),
[ATTR_SERVER_PORT]: config?.connection?.port ?? utils.extractPortFromConnectionString(connectionString),
});
}
if (maxQueryLength) {
const queryText = utils.limitLength(query?.sql, maxQueryLength);
if (instrumentation._semconvStability & SemconvStability.STABLE) {
attributes[ATTR_DB_QUERY_TEXT] = queryText;
}
if (instrumentation._semconvStability & SemconvStability.OLD) {
attributes[ATTR_DB_STATEMENT] = queryText;
}
}

const parentContext = this.builder[contextSymbol] || api.context.active();
const parentSpan = api.trace.getSpan(parentContext);
const hasActiveParent = parentSpan && api.trace.isSpanContextValid(parentSpan.spanContext());
if (instrumentation._config.requireParentSpan && !hasActiveParent) {
return original.bind(this)(...arguments);
}

const span = instrumentation.tracer.startSpan(
utils.getName(name, operation, table),

// The query builder captures the span active when it was created (see `_storeContext`).
// `onlyIfParent` ensures we only instrument queries that run as part of an existing trace.
const parentSpan: Span | undefined = this.builder[parentSpanSymbol] || getActiveSpan();

const args = arguments;
return startSpan(
{
kind: api.SpanKind.CLIENT,
name: utils.getName(name, operation, table),
kind: SpanKind.CLIENT,
attributes,
parentSpan,
onlyIfParent: true,
},
Comment on lines +149 to 159

@sentry sentry Bot Jun 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Passing an ended span as parentSpan to startSpan bypasses the onlyIfParent: true guard, because the ended span is set as active before the guard is checked.
Severity: MEDIUM

Suggested Fix

Before calling startSpan, validate that the parentSpan is still valid. Reintroduce a check similar to the old implementation, using api.trace.isSpanContextValid(parentSpan.spanContext()) to ensure the span from this.builder[parentSpanSymbol] has not ended before using it as a parent.

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/knex/vendored/instrumentation.ts#L149-L159

Potential issue: The `knex` instrumentation stores a parent span when a query builder is
created. This span can be ended by the time the query executes later in a different
async context. This ended span is passed as the `parentSpan` option to `startSpan` with
`onlyIfParent: true`. However, the `startSpan` implementation sets this explicit
`parentSpan` as the active span *before* checking the `onlyIfParent` condition. As a
result, the check finds the (ended) span and incorrectly proceeds, creating a new span
as a child of an ended one, which breaks the trace hierarchy. This replaces a previous
implementation that correctly validated the span context with `isSpanContextValid`.

Did we get this right? 👍 / 👎 to inform future reviews.

parentContext,
span =>
// `Runner.query` returns a real, already-executing Promise, so it is safe to let
// `startSpan` await it and auto-end the span.
original.apply(this, args).catch((err: any) => {
const formatter = utils.getFormatter(this);
const fullQuery = formatter(query.sql, query.bindings || []);
const message = err.message.replace(`${fullQuery} - `, '');
span.setStatus({ code: SPAN_STATUS_ERROR, message });
throw err;
}),
);
const spanContext = api.trace.setSpan(api.context.active(), span);

return api.context
.with(spanContext, original, this, ...arguments)
.then((result: unknown) => {
span.end();
return result;
})
.catch((err: any) => {
const formatter = utils.getFormatter(this);
const fullQuery = formatter(query.sql, query.bindings || []);
const message = err.message.replace(fullQuery + ' - ', '');
const exc = utils.otelExceptionFromKnexError(err, message);
span.recordException(exc);
span.setStatus({ code: api.SpanStatusCode.ERROR, message });
span.end();
throw err;
});
};
};
}

private storeContext(original: Function) {
private _storeContext(original: (...args: any[]) => any) {
return function wrapped_logging_method(this: any) {
const builder = original.apply(this, arguments);
Object.defineProperty(builder, contextSymbol, {
value: api.context.active(),
// Capture the span that is active when the query builder is created. The query often executes
// in a different async context, so we reuse this span as the parent when the query runs.
Object.defineProperty(builder, parentSpanSymbol, {
value: getActiveSpan(),
});
return builder;
};
}

ensureWrapped(obj: any, methodName: string, wrapper: (original: any) => any) {
private _ensureWrapped(obj: any, methodName: string, wrapper: (original: any) => any): void {
if (isWrapped(obj[methodName])) {
this._unwrap(obj, methodName);
}
Expand Down
Loading
Loading