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

deejay1 · 2025-01-20T11:36:35Z

Which problem is this PR solving?

Updates deprecated attributes with current ones and drop deprecated function

Short description of the changes

Uses newer semantic conventions and replaces deprecated SpanAttributes call with Attributes
Syncs attributes with PostgreSQL instrumentation

github-actions · 2025-01-21T06:37:25Z

This package does not have an assigned component owner and is considered unmaintained. As such this package is in feature-freeze and this PR will be closed with 14 days unless a new owner or a sponsor (a member of @open-telemetry/javascript-approvers) for the feature is found. It is the responsibility of the author to find a sponsor for this feature.
Are you familiar with this package? Consider becoming a component owner.

codecov · 2025-02-09T12:49:10Z

Codecov Report

Attention: Patch coverage is 87.50000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 89.67%. Comparing base (55cc256) to head (4e43e12).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...emetry-instrumentation-knex/src/instrumentation.ts	84.61%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2671      +/-   ##
==========================================
+ Coverage   89.63%   89.67%   +0.03%     
==========================================
  Files         184      185       +1     
  Lines        9003     9014      +11     
  Branches     1845     1849       +4     
==========================================
+ Hits         8070     8083      +13     
+ Misses        933      931       -2

Files with missing lines	Coverage Δ
.../opentelemetry-instrumentation-knex/src/semconv.ts	`100.00% <100.00%> (ø)`
...de/opentelemetry-instrumentation-knex/src/utils.ts	`89.74% <100.00%> (+0.26%)`	⬆️
...emetry-instrumentation-knex/src/instrumentation.ts	`96.59% <84.61%> (-2.15%)`	⬇️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

trentm · 2025-03-24T20:34:11Z

I'll sponsor.

trentm · 2025-04-09T22:15:22Z

@maryliag I'd appreciate your guidance here. Part of this PR is updating some db.* semconv attribute usage to more recent forms. E.g. changing db.sql.table to db.collection.name (which was part of semconv 1.25.0).

Is that something we should be doing now? Or is that something we should wait on until after the semconv v1.33.0 release because of the pending stabilization of db semconv: https://github.com/open-telemetry/semantic-conventions/releases/tag/v1.32.0 says:

This release is the second release candidate for the Database Semantic Conventions, with db conventions stability planned to be declared in the subsequent release.

Do you know if there will be a db stability migration thing using OTEL_SEMCONV_STABILITY_OPT_IN as was done for http (https://opentelemetry.io/docs/specs/semconv/non-normative/http-migration/)?

deejay1 · 2025-04-10T09:26:26Z

@trentm as far as I recall knex is already behind other modules regarding the attributes, so I don't see much of an issue making these changes opt-in.

maryliag · 2025-04-10T12:23:34Z

I think it's okay to do them now, because we don't plan on making any other changes to them, at least not in the ones that were updated here. Regarding the opt-in, we do have some guidance on this on semantic convention pages already:

Existing database instrumentations that are using v1.24.0 of this document (or prior):

SHOULD NOT change the version of the database conventions that they emit by default until the database semantic conventions are marked stable. Conventions include, but are not limited to, attributes, metric and span names, and unit of measure.
SHOULD introduce an environment variable OTEL_SEMCONV_STABILITY_OPT_IN in the existing major version which is a comma-separated list of values. The list of values includes:
- database - emit the new, stable database conventions, and stop emitting the old experimental database conventions that the instrumentation emitted previously.
- database/dup - emit both the old and the stable database conventions, allowing for a seamless transition.
- The default behavior (in the absence of one of these values) is to continue emitting whatever version of the old experimental database conventions the instrumentation was emitting previously.
- Note: database/dup has higher precedence than database in case both values are present
SHOULD maintain (security patching at a minimum) the existing major version for at least six months after it starts emitting both sets of conventions.
SHOULD drop the environment variable in the next major version.

So, I would say you can make the changes now, but with an opt-in for the new names and still sending old values, then create an issue to remove the old names on a following version, just to make sure we keep track of this and don't forget

deejay1 · 2025-04-11T12:42:45Z

So, I would say you can make the changes now, but with an opt-in for the new names and still sending old values, then create an issue to remove the old names on a following version, just to make sure we keep track of this and don't forget
Ok, I'll adapt this then. Thanks for the feedback.

deejay1 · 2025-04-24T12:24:39Z

@maryliag can you merge it or should I ping someone?

maryliag · 2025-04-24T12:44:00Z

cc @trentm

trentm · 2025-05-12T20:57:00Z

plugins/node/opentelemetry-instrumentation-knex/README.md

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.

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.

plugins/node/opentelemetry-instrumentation-knex/README.md

plugins/node/opentelemetry-instrumentation-knex/package.json

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

plugins/node/opentelemetry-instrumentation-knex/README.md

Co-authored-by: Trent Mick <[email protected]>

* Bump semantic-conventions to ^1.33.0 * Update used attributes to stabilised ones

trentm · 2025-05-15T23:24:26Z

plugins/node/opentelemetry-instrumentation-knex/src/utils.ts

  DB_SYSTEM_NAME_VALUE_SQLITE,
-} from './semconv';
+} from '@opentelemetry/semantic-conventions';


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.)

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.

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

trentm · 2025-05-15T23:25:44Z

plugins/node/opentelemetry-instrumentation-knex/README.md

-This package uses `@opentelemetry/semantic-conventions` version `1.22+`, 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.33.0](https://github.com/open-telemetry/semantic-conventions/blob/v1.33.0/docs/database/database-metrics.md)


Did you intend to link the database metrics doc page?

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

plugins/node/opentelemetry-instrumentation-knex/package.json

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

Use semconvStabilityFromStr from @opentelemetry/instrumentation, to avoid duplicating code.

deejay1 · 2025-05-16T13:51:28Z

@trentm frankly, I'm stumped why it started failing after the change
Edit: looks like https://ncjamieson.com/dont-export-const-enums/

trentm · 2025-05-16T19:09:00Z

Edit: looks like https://ncjamieson.com/dont-export-const-enums/

I think you are right. Good find!
Sorry this PR has become the guinea pig for the SemconvStability addition. I'll start digging in. Currently I'm thinking we'll need a new @opentelemetry/[email protected] that drops the const from that enum.

Lint

The lint step is failing because you need to now do:

npm uninstall @opentelemetry/core

That is assuming you'd rather not go back to a local/internal semconvStability implementation while @opentelemetry/instrumentation sorts out its const enum issue.

deejay1 · 2025-05-16T19:16:17Z

Sorry this PR has become the guinea pig for the SemconvStability addition

No problem, I'll wait - someone has to test things ;)

As for the linter - I didn't even look at this being occupied with the previous error.

…um` to `enum` to allow single-file transpilation tooling to work with code that uses `SemconvStability` Refs: open-telemetry#5691 Refs: open-telemetry/opentelemetry-js-contrib#2671 (comment)

trentm · 2025-05-16T19:41:00Z

I'll start digging in.

My write-up: open-telemetry/opentelemetry-js#5691
and PR: open-telemetry/opentelemetry-js#5692

It being Friday afternoon, this will likely sit until Monday.

# Conflicts: # package-lock.json

deejay1 · 2025-05-21T09:48:32Z

It being Friday afternoon, this will likely sit until Monday.

Ok, I've updated everything now.

trentm

LGTM after the suggested package-lock edits.

trentm · 2025-05-21T23:07:39Z

package-lock.json

+        "@opentelemetry/api-logs": {
+          "version": "0.201.1",
+          "resolved": "https://registry.npmjs.org/@opentelemetry/api-logs/-/api-logs-0.201.1.tgz",
+          "integrity": "sha512-IxcFDP1IGMDemVFG2by/AMK+/o6EuBQ8idUq3xZ6MxgQGeumYZuX5OwR0h9HuvcUc/JPjQGfU5OHKIKYDJcXeA==",
+          "requires": {
+            "@opentelemetry/api": "^1.3.0"
+          }
+        },
+        "@opentelemetry/instrumentation": {
+          "version": "0.201.1",
+          "resolved": "https://registry.npmjs.org/@opentelemetry/instrumentation/-/instrumentation-0.201.1.tgz",
+          "integrity": "sha512-6EOSoT2zcyBM3VryAzn35ytjRrOMeaWZyzQ/PHVfxoXp5rMf7UUgVToqxOhQffKOHtC7Dma4bHt+DuwIBBZyZA==",
+          "requires": {
+            "@opentelemetry/api-logs": "0.201.1",
+            "@types/shimmer": "^1.2.0",
+            "import-in-the-middle": "^1.8.1",
+            "require-in-the-middle": "^7.1.1",
+            "shimmer": "^1.2.1"
+          }
+        },


Suggested change

"@opentelemetry/api-logs": {

"version": "0.201.1",

"resolved": "https://registry.npmjs.org/@opentelemetry/api-logs/-/api-logs-0.201.1.tgz",

"integrity": "sha512-IxcFDP1IGMDemVFG2by/AMK+/o6EuBQ8idUq3xZ6MxgQGeumYZuX5OwR0h9HuvcUc/JPjQGfU5OHKIKYDJcXeA==",

"requires": {

"@opentelemetry/api": "^1.3.0"

}

},

"@opentelemetry/instrumentation": {

"version": "0.201.1",

"resolved": "https://registry.npmjs.org/@opentelemetry/instrumentation/-/instrumentation-0.201.1.tgz",

"integrity": "sha512-6EOSoT2zcyBM3VryAzn35ytjRrOMeaWZyzQ/PHVfxoXp5rMf7UUgVToqxOhQffKOHtC7Dma4bHt+DuwIBBZyZA==",

"requires": {

"@opentelemetry/api-logs": "0.201.1",

"@types/shimmer": "^1.2.0",

"import-in-the-middle": "^1.8.1",

"require-in-the-middle": "^7.1.1",

"shimmer": "^1.2.1"

}

},

package-lock.json updating sucks. There are multiple layouts of deps that satisfy a given set of package.json files. The one you currently have on this PR duplicates some packages to plugins/node/opentelemetry-instrumentation-knex/node_modules that don't need to be there. This happened because you added a dep on @opentelemetry/[email protected] in this subdir when the top-level still depended on version 0.201.0. Then #2835 updated the dep to version 0.201.1 at the top-level.

A subsequent npm install does not clean this up. npm dedupe does clean this up -- at least this case -- but running that on the current repo state also changes the install location of a few other packages, which doesn't belong on this PR.

Instead, I'll suggest a few manual deletions to package-lock.json to clean it up manually.

(The alternative is to revert package-lock.json to the state on main, then run:

cd plugins/node/opentelemetry-instrumentation-knex npm install

Yes, I'm obsessing. :) It would be fine to merge this as is and later (or not) run npm dedupe.

In fact, let's just do that.

trentm · 2025-05-21T23:08:01Z

package-lock.json

+        "@types/shimmer": {
+          "version": "1.2.0",
+          "resolved": "https://registry.npmjs.org/@types/shimmer/-/shimmer-1.2.0.tgz",
+          "integrity": "sha512-UE7oxhQLLd9gub6JKIAhDq06T0F6FnztwMNRvYgjeQSBeMc1ZG/tA47EwfduvkuQS8apbkM/lpLpWsaCeYsXVg=="
+        },


Suggested change

"@types/shimmer": {

"version": "1.2.0",

"resolved": "https://registry.npmjs.org/@types/shimmer/-/shimmer-1.2.0.tgz",

"integrity": "sha512-UE7oxhQLLd9gub6JKIAhDq06T0F6FnztwMNRvYgjeQSBeMc1ZG/tA47EwfduvkuQS8apbkM/lpLpWsaCeYsXVg=="

},

trentm · 2025-05-21T23:08:21Z

package-lock.json

+    "plugins/node/opentelemetry-instrumentation-knex/node_modules/@types/shimmer": {
+      "version": "1.2.0",
+      "resolved": "https://registry.npmjs.org/@types/shimmer/-/shimmer-1.2.0.tgz",
+      "integrity": "sha512-UE7oxhQLLd9gub6JKIAhDq06T0F6FnztwMNRvYgjeQSBeMc1ZG/tA47EwfduvkuQS8apbkM/lpLpWsaCeYsXVg==",
+      "license": "MIT"
+    },


Suggested change

"plugins/node/opentelemetry-instrumentation-knex/node_modules/@types/shimmer": {

"version": "1.2.0",

"resolved": "https://registry.npmjs.org/@types/shimmer/-/shimmer-1.2.0.tgz",

"integrity": "sha512-UE7oxhQLLd9gub6JKIAhDq06T0F6FnztwMNRvYgjeQSBeMc1ZG/tA47EwfduvkuQS8apbkM/lpLpWsaCeYsXVg==",

"license": "MIT"

},

trentm · 2025-05-21T23:08:45Z

package-lock.json

+    "plugins/node/opentelemetry-instrumentation-knex/node_modules/@opentelemetry/api-logs": {
+      "version": "0.201.1",
+      "resolved": "https://registry.npmjs.org/@opentelemetry/api-logs/-/api-logs-0.201.1.tgz",
+      "integrity": "sha512-IxcFDP1IGMDemVFG2by/AMK+/o6EuBQ8idUq3xZ6MxgQGeumYZuX5OwR0h9HuvcUc/JPjQGfU5OHKIKYDJcXeA==",
+      "license": "Apache-2.0",
+      "dependencies": {
+        "@opentelemetry/api": "^1.3.0"
+      },
+      "engines": {
+        "node": ">=8.0.0"
+      }
+    },
+    "plugins/node/opentelemetry-instrumentation-knex/node_modules/@opentelemetry/instrumentation": {
+      "version": "0.201.1",
+      "resolved": "https://registry.npmjs.org/@opentelemetry/instrumentation/-/instrumentation-0.201.1.tgz",
+      "integrity": "sha512-6EOSoT2zcyBM3VryAzn35ytjRrOMeaWZyzQ/PHVfxoXp5rMf7UUgVToqxOhQffKOHtC7Dma4bHt+DuwIBBZyZA==",
+      "license": "Apache-2.0",
+      "dependencies": {
+        "@opentelemetry/api-logs": "0.201.1",
+        "@types/shimmer": "^1.2.0",
+        "import-in-the-middle": "^1.8.1",
+        "require-in-the-middle": "^7.1.1",
+        "shimmer": "^1.2.1"
+      },
+      "engines": {
+        "node": "^18.19.0 || >=20.6.0"
+      },
+      "peerDependencies": {
+        "@opentelemetry/api": "^1.3.0"
+      }
+    },


Suggested change

"plugins/node/opentelemetry-instrumentation-knex/node_modules/@opentelemetry/api-logs": {

"version": "0.201.1",

"resolved": "https://registry.npmjs.org/@opentelemetry/api-logs/-/api-logs-0.201.1.tgz",

"integrity": "sha512-IxcFDP1IGMDemVFG2by/AMK+/o6EuBQ8idUq3xZ6MxgQGeumYZuX5OwR0h9HuvcUc/JPjQGfU5OHKIKYDJcXeA==",

"license": "Apache-2.0",

"dependencies": {

"@opentelemetry/api": "^1.3.0"

},

"engines": {

"node": ">=8.0.0"

}

},

"plugins/node/opentelemetry-instrumentation-knex/node_modules/@opentelemetry/instrumentation": {

"version": "0.201.1",

"resolved": "https://registry.npmjs.org/@opentelemetry/instrumentation/-/instrumentation-0.201.1.tgz",

"integrity": "sha512-6EOSoT2zcyBM3VryAzn35ytjRrOMeaWZyzQ/PHVfxoXp5rMf7UUgVToqxOhQffKOHtC7Dma4bHt+DuwIBBZyZA==",

"license": "Apache-2.0",

"dependencies": {

"@opentelemetry/api-logs": "0.201.1",

"@types/shimmer": "^1.2.0",

"import-in-the-middle": "^1.8.1",

"require-in-the-middle": "^7.1.1",

"shimmer": "^1.2.1"

},

"engines": {

"node": "^18.19.0 || >=20.6.0"

},

"peerDependencies": {

"@opentelemetry/api": "^1.3.0"

}

},

trentm

LGTM.
I'll follow up with a separate npm dedupe PR.

Thanks for doing this @deejay1!

deejay1 requested a review from a team as a code owner January 20, 2025 11:36

github-actions bot added pkg:instrumentation-knex pkg-status:unmaintained This package is unmaintained. Only bugfixes may be acceped until a new owner has been found. labels Jan 20, 2025

github-actions bot added the pkg-status:unmaintained:autoclose-scheduled label Jan 21, 2025

deejay1 force-pushed the knex-semantic-conventions branch from 7b91bdc to 3a70ffe Compare February 9, 2025 12:40

trentm added the has:sponsor This package or feature has a sponsor that has volunteered to review PRs and respond to questions label Mar 24, 2025

deejay1 force-pushed the knex-semantic-conventions branch from 5184085 to 0fe75a2 Compare April 14, 2025 14:14

github-actions bot assigned aabmass, abhee11, blumamir, d4nyll, david-luna, dyladan, hectorhdzg, JacksonWeber, JamieDanielson and jj22ee Apr 14, 2025

github-actions bot added pkg:instrumentation-ioredis pkg:instrumentation-graphql pkg:instrumentation-mysql pkg:instrumentation-mysql2 pkg:instrumentation-fs pkg:instrumentation-restify labels Apr 14, 2025

Merge branch 'main' into knex-semantic-conventions

53585cb

trentm reviewed May 12, 2025

View reviewed changes

plugins/node/opentelemetry-instrumentation-knex/README.md Outdated Show resolved Hide resolved

deejay1 and others added 3 commits May 13, 2025 10:56

Apply suggestions from code review

ff6e0bf

Co-authored-by: Trent Mick <[email protected]>

feat(instrumentation-knex): Implement fixes after review

b40ba4d

* Bump semantic-conventions to ^1.33.0 * Update used attributes to stabilised ones

Merge branch 'main' into knex-semantic-conventions-

88fb85c

trentm reviewed May 15, 2025

View reviewed changes

deejay1 added 2 commits May 16, 2025 14:25

Merge branch 'main' into knex-semantic-conventions-

8a78e1f

feat(instrumentation-knex): Use semconvStabilityFromStr

f48f01e

Use semconvStabilityFromStr from @opentelemetry/instrumentation, to avoid duplicating code.

feat(instrumentation-knex): Drop core from dependencies

2a85075

trentm mentioned this pull request May 16, 2025

export const enum SemconvStability from @opentelemetry/instrumentation is problematic open-telemetry/opentelemetry-js#5691

Closed

4 tasks

trentm mentioned this pull request May 16, 2025

fix(instrumentation): change SemconvStability export from const enum to enum open-telemetry/opentelemetry-js#5692

Merged

deejay1 added 3 commits May 19, 2025 17:21

feat(instrumentation-knex): Update instrumentation version

a8d4c05

feat(instrumentation-knex): Fix semconv versions

4810f90

Merge branch 'main' into knex-semantic-conventions-

5499bd9

# Conflicts: # package-lock.json

feat(instrumentation-knex): Fix import

4e43e12

pichlermarc mentioned this pull request May 21, 2025

fix(knex): connection attrs missing if connectionString used #1932

Open

trentm reviewed May 21, 2025

View reviewed changes

trentm approved these changes May 21, 2025

View reviewed changes

trentm merged commit 825441a into open-telemetry:main May 21, 2025
29 of 30 checks passed

dyladan mentioned this pull request May 21, 2025

chore: release main #2834

Open

trentm mentioned this pull request May 21, 2025

chore: reduce duplicate dep installs with 'npm dedupe' #2840

Merged

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

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

Uh oh!

Conversation

deejay1 commented Jan 20, 2025

Which problem is this PR solving?

Short description of the changes

Uh oh!

github-actions bot commented Jan 21, 2025

Uh oh!

codecov bot commented Feb 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

trentm commented Mar 24, 2025

Uh oh!

trentm commented Apr 9, 2025

Uh oh!

deejay1 commented Apr 10, 2025

Uh oh!

maryliag commented Apr 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deejay1 commented Apr 11, 2025

Uh oh!

deejay1 commented Apr 24, 2025

Uh oh!

maryliag commented Apr 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

deejay1 commented May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

trentm commented May 16, 2025

Uh oh!

deejay1 commented May 16, 2025

Uh oh!

trentm commented May 16, 2025

Uh oh!

deejay1 commented May 21, 2025

Uh oh!

trentm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

codecov bot commented Feb 9, 2025 •

edited

Loading

maryliag commented Apr 10, 2025 •

edited

Loading

deejay1 commented May 16, 2025 •

edited

Loading