Skip to content

feat(instrumentation-knex): Use newer semantic conventions #2671

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

Merged
merged 13 commits into from
May 21, 2025
Merged

feat(instrumentation-knex): Use newer semantic conventions #2671

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
f6f9e4c
feat(instrumentation-knex): Use newer semantic conventions
deejay1 Jan 20, 2025
aef24d6
feat(instrumentation-knex): Use getStringListFromEnv from core
deejay1 Apr 15, 2025
53585cb
Merge branch 'main' into knex-semantic-conventions
deejay1 May 8, 2025
ff6e0bf
Apply suggestions from code review
deejay1 May 13, 2025
b40ba4d
feat(instrumentation-knex): Implement fixes after review
deejay1 May 14, 2025
88fb85c
Merge branch 'main' into knex-semantic-conventions-
deejay1 May 14, 2025
8a78e1f
Merge branch 'main' into knex-semantic-conventions-
deejay1 May 16, 2025
f48f01e
feat(instrumentation-knex): Use semconvStabilityFromStr
deejay1 May 16, 2025
2a85075
feat(instrumentation-knex): Drop core from dependencies
deejay1 May 16, 2025
a8d4c05
feat(instrumentation-knex): Update instrumentation version
deejay1 May 19, 2025
4810f90
feat(instrumentation-knex): Fix semconv versions
deejay1 May 21, 2025
5499bd9
Merge branch 'main' into knex-semantic-conventions-
deejay1 May 21, 2025
4e43e12
feat(instrumentation-knex): Fix import
deejay1 May 21, 2025
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
28 changes: 18 additions & 10 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

49 changes: 13 additions & 36 deletions plugins/node/opentelemetry-instrumentation-knex/README.md
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

General note on semconv docs in instrumentation READMEs. Jamie and I have been recently working through some OTEL_SEMCONV_STABILITY_OPT_IN support for http-related instrumentation. If you like there is some boilerplate here that you could copy:

https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/packages/opentelemetry-instrumentation-http/README.md?plain=1#L73

For example, it merges the attributes for "old" and "stable" into a single table.
However, what you have here is fine, so no need to adjust.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll take another look at it tomorrow, as I'm somehow not satisfied with the current fixed version, yet I can't tell what bothers me there.

Original file line number Diff line number Diff line change
Expand Up @@ -54,9 +54,9 @@ registerInstrumentations({

## Semantic Conventions

This package uses `@opentelemetry/semantic-conventions` version `1.27+`, which implements Semantic Convention [Version 1.7.0](https://github.com/open-telemetry/opentelemetry-specification/blob/v1.7.0/semantic_conventions/README.md)
This package uses `@opentelemetry/semantic-conventions` version `1.33+`, which implements Semantic Convention [Version 1.7.0](https://github.com/open-telemetry/opentelemetry-specification/blob/v1.7.0/semantic_conventions/README.md)

This package is capable of emitting both Semantic Convention [Version 1.7.0](https://github.com/open-telemetry/opentelemetry-specification/blob/v1.7.0/semantic_conventions/README.md) and [Version 1.32.0](https://github.com/open-telemetry/semantic-conventions/blob/v1.32.0/docs/database/database-metrics.md)
This package is capable of emitting both Semantic Convention [Version 1.7.0](https://github.com/open-telemetry/opentelemetry-specification/blob/v1.7.0/semantic_conventions/README.md) and [Version 1.33.0](https://github.com/open-telemetry/semantic-conventions/blob/v1.33.0/docs/database/database-metrics.md)
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you intend to link the database metrics doc page?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, both have the tags ;) I'll link to database-spans.md when 1.33.1 comes out

It is controlled using the environment variable `OTEL_SEMCONV_STABILITY_OPT_IN`, which is a comma separated list of values.
The values `database` and `database/dup` control this instrumentation.
See details for the behavior of each of these values below.
Expand All @@ -73,42 +73,19 @@ When upgrading to the new semantic conventions, it is recommended to do so in th

This will cause both the old and new semantic conventions to be emitted during the transition period.

### ### Legacy Behavior (default)

Enabled when `OTEL_SEMCONV_STABILITY_OPT_IN` contains `database/dup` or DOES NOT CONTAIN `database`.

Attributes collected:

| Attribute | Short Description |
| ----------------------- | ------------------------------------------------------------------------------ |
| `db.name` | This attribute is used to report the name of the database being accessed. |
| `db.operation` | The name of the operation being executed. |
| `db.sql.table` | The name of the primary table that the operation is acting upon. |
| `db.statement` | The database statement being executed. |
| `db.system` | An identifier for the database management system (DBMS) product being used. |
| `db.user` | Username for accessing the database. |
| `net.peer.name` | Remote hostname or similar. |
| `net.peer.port` | Remote port number. |
| `net.transport` | Transport protocol used. |

### RC Semantic Conventions 1.32

Enabled when `OTEL_SEMCONV_STABILITY_OPT_IN` contains `database` OR `database/dup`.
This is the recommended configuration, and will soon become the default behavior.

Attributes collected:

| Attribute | Short Description |
|----------------------|-----------------------------------------------------------------------------|
| `db.namespace` | This attribute is used to report the name of the database being accessed. |
| `db.operation.name` | The name of the operation being executed. |
| `db.collection.name` | The name of the primary table that the operation is acting upon. |
| `db.query.text` | The database statement being executed. |
| `db.system` | An identifier for the database management system (DBMS) product being used. |
| `db.user` | Username for accessing the database. |
| `server.address` | Remote hostname or similar. |
| `server.port` | Remote port number. |
| `network.transport` | Transport protocol used. |
| v1.7.0 semconv | v1.23.0 semconv | Short Description |
|-----------------|----------------------|-----------------------------------------------------------------------------|
| `db.name` | `db.namespace` | This attribute is used to report the name of the database being accessed. |
| `db.operation` | `db.operation.name` | The name of the operation being executed. |
| `db.sql.table` | `db.collection.name` | The name of the primary table that the operation is acting upon. |
| `db.statement` | `db.query.text` | The database statement being executed. |
| `db.system` | `db.system.name` | An identifier for the database management system (DBMS) product being used. |
| `db.user` | (not included) | Username for accessing the database. |
| `net.peer.name` | `server.address` | Remote hostname or similar. |
| `net.peer.port` | `server.port` | Remote port number. |
| `net.transport` | (not included) | Transport protocol used. |

## Useful links

Expand Down
6 changes: 2 additions & 4 deletions plugins/node/opentelemetry-instrumentation-knex/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,7 @@
"files": [
"build/src/**/*.js",
"build/src/**/*.js.map",
"build/src/**/*.d.ts",
"LICENSE",
"README.md"
"build/src/**/*.d.ts"
],
"publishConfig": {
"access": "public"
Expand All @@ -60,7 +58,7 @@
"dependencies": {
"@opentelemetry/core": "^2.0.0",
"@opentelemetry/instrumentation": "^0.200.0",
"@opentelemetry/semantic-conventions": "^1.27.0"
"@opentelemetry/semantic-conventions": "^1.33.0"
},
"homepage": "https://github.com/open-telemetry/opentelemetry-js-contrib/tree/main/plugins/node/opentelemetry-instrumentation-knex#readme"
}
29 changes: 10 additions & 19 deletions plugins/node/opentelemetry-instrumentation-knex/src/instrumentation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,16 @@
isWrapped,
} from '@opentelemetry/instrumentation';
import * as utils from './utils';
import { mapSystem } from './utils';
import { KnexInstrumentationConfig } from './types';
import { SemconvStability } from './internal-types';
import { getStringListFromEnv } from '@opentelemetry/core';
import {
ATTR_NETWORK_TRANSPORT,
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,
SEMATTRS_DB_NAME,
Expand All @@ -42,15 +47,6 @@
SEMATTRS_NET_PEER_PORT,
SEMATTRS_NET_TRANSPORT,
} from '@opentelemetry/semantic-conventions';
import {
ATTR_DB_COLLECTION_NAME,
ATTR_DB_NAMESPACE,
ATTR_DB_OPERATION_NAME,
ATTR_DB_QUERY_TEXT,
ATTR_DB_SYSTEM_NAME,
ATTR_DB_USER,
} from './semconv';
import { mapSystem } from './utils';

const contextSymbol = Symbol('opentelemetry.instrumentation-knex.context');
const DEFAULT_CONFIG: KnexInstrumentationConfig = {
Expand All @@ -66,12 +62,12 @@

for (const entry of getStringListFromEnv('OTEL_SEMCONV_STABILITY_OPT_IN') ??
[]) {
if (entry.toLowerCase() === 'database/dup') {

Check warning on line 65 in plugins/node/opentelemetry-instrumentation-knex/src/instrumentation.ts

View check run for this annotation

Codecov / codecov/patch

plugins/node/opentelemetry-instrumentation-knex/src/instrumentation.ts#L65 

Added line #L65 was not covered by tests
// database/dup takes highest precedence. If it is found, there is no need to read the rest of the list
this._semconvStability = SemconvStability.DUPLICATE;
break;
} else if (entry.toLowerCase() === 'database') {
this._semconvStability = SemconvStability.STABLE;

Check warning on line 70 in plugins/node/opentelemetry-instrumentation-knex/src/instrumentation.ts

View check run for this annotation

Codecov / codecov/patch

plugins/node/opentelemetry-instrumentation-knex/src/instrumentation.ts#L67-L70 

Added lines #L67 - L70 were not covered by tests
}
}
}
Expand Down Expand Up @@ -150,9 +146,6 @@
private createQueryWrapper(moduleVersion?: string) {
const instrumentation = this;

// We need to bind it here, because `this` is not the same in the wrapper
const semConv = this._semconvStability;

return function wrapQuery(original: (...args: any[]) => any) {
return function wrapped_logging_method(this: any, query: any) {
const config = this.client.config;
Expand All @@ -171,7 +164,7 @@
const transport =
config?.connection?.filename === ':memory:' ? 'inproc' : undefined;

if ((semConv & SemconvStability.OLD) === SemconvStability.OLD) {
if (instrumentation._semconvStability & SemconvStability.OLD) {
Object.assign(attributes, {
[SEMATTRS_DB_SYSTEM]: mapSystem(config.client),
[SEMATTRS_DB_SQL_TABLE]: table,
Expand All @@ -183,25 +176,23 @@
[SEMATTRS_NET_TRANSPORT]: transport,
});
}
if ((semConv & SemconvStability.STABLE) === SemconvStability.STABLE) {
if (instrumentation._semconvStability & SemconvStability.STABLE) {
Object.assign(attributes, {

Check warning on line 180 in plugins/node/opentelemetry-instrumentation-knex/src/instrumentation.ts

View check run for this annotation

Codecov / codecov/patch

plugins/node/opentelemetry-instrumentation-knex/src/instrumentation.ts#L180 

Added line #L180 was not covered by tests
[ATTR_DB_SYSTEM_NAME]: mapSystem(config.client),
[ATTR_DB_COLLECTION_NAME]: table,
[ATTR_DB_OPERATION_NAME]: operation,
[ATTR_DB_USER]: config?.connection?.user,
[ATTR_DB_NAMESPACE]: name,
[ATTR_SERVER_ADDRESS]: config?.connection?.host,
[ATTR_SERVER_PORT]: config?.connection?.port,
[ATTR_NETWORK_TRANSPORT]: transport,
});
}
if (maxQueryLength) {
// filters both undefined and 0
const queryText = utils.limitLength(query?.sql, maxQueryLength);
if (semConv & SemconvStability.STABLE) {
if (instrumentation._semconvStability & SemconvStability.STABLE) {
attributes[ATTR_DB_QUERY_TEXT] = queryText;

Check warning on line 193 in plugins/node/opentelemetry-instrumentation-knex/src/instrumentation.ts

View check run for this annotation

Codecov / codecov/patch

plugins/node/opentelemetry-instrumentation-knex/src/instrumentation.ts#L193 

Added line #L193 was not covered by tests
}
if (semConv & SemconvStability.OLD) {
if (instrumentation._semconvStability & SemconvStability.OLD) {
attributes[SEMATTRS_DB_STATEMENT] = queryText;
}
}
Expand Down
30 changes: 0 additions & 30 deletions plugins/node/opentelemetry-instrumentation-knex/src/semconv.ts
View file
Open in desktop

This file was deleted.

2 changes: 1 addition & 1 deletion plugins/node/opentelemetry-instrumentation-knex/src/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import { Exception } from '@opentelemetry/api';
import {
DB_SYSTEM_NAME_VALUE_POSTGRESQL,
DB_SYSTEM_NAME_VALUE_SQLITE,
} from './semconv';
} from '@opentelemetry/semantic-conventions';
Copy link
Contributor

Choose a reason for hiding this comment

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

To use DB_SYSTEM_NAME_VALUE_SQLITE will require using a local src/semconv.ts soon. See:
open-telemetry/opentelemetry-js#5690 (comment)

tl;dr: DB_SYSTEM_NAME_VALUE_SQLITE is actually unstable and should not have been included in the stable @opentelemetry/semantic-conventions entry point.

(Eventually semconv-related things will settle down and this won't be so hard/obtuse/subtle.)

Copy link
Contributor

Choose a reason for hiding this comment

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

To use DB_SYSTEM_NAME_VALUE_SQLITE will require using a local src/semconv.ts soon.

Perhaps easiest to wait for a v1.33.1 release of the semconv package, then create the src/semconv.ts again (perhaps using the https://github.com/open-telemetry/opentelemetry-js-contrib/blob/main/scripts/gen-semconv-ts.js tool). I'm happy to help with that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i'll wait for the release, anyway I have no idea why they are still experimental as it's pretty much a given that sqlite is sqlite


type KnexError = Error & {
code?: string;
Expand Down