chore(instrumentation-pg): Inline incubating constants from @opentelemetry/semantic-conventions #2599

nwalters512 · 2024-12-10T21:24:55Z

Which problem is this PR solving?

No other instrumentation packages pin @opentelemetry/semantic-conventions, and from what I can tell, there's no good reason that @opentelemetry/instumentation-pg should be pinning it. When the dependency is pinned, npm can't dedupe it to a higher version, which results in multiple versions of this package being installed.

As decided in open-telemetry/opentelemetry-js#5182 (comment), incubating constants from @opentelemetry/semantic-conventions should be inlined instead of pinning @opentelemetry/semantic-conventions to a particular version.

Short description of the changes

~~In @opentelemetry/instrumentation-pg, @opentelemetry/semantic-conventions now uses a ^ semver range to allow newer minor versions to be installed.~~

This PR updates @opentelemetry/instrumentation-pg to include inlined copies of the experimental constants it needs. This, in turn, allows us to unpin @opentelemetry/semantic-conventions.

codecov · 2024-12-10T21:34:41Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.80%. Comparing base (e57f3e4) to head (b8aa803).
Report is 4 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2599      +/-   ##
==========================================
+ Coverage   90.79%   90.80%   +0.01%     
==========================================
  Files         169      170       +1     
  Lines        8061     8070       +9     
  Branches     1646     1646              
==========================================
+ Hits         7319     7328       +9     
  Misses        742      742

Files with missing lines	Coverage Δ
...elemetry-instrumentation-pg/src/instrumentation.ts	`90.21% <ø> (ø)`
...try-instrumentation-pg/src/semantic-conventions.ts	`100.00% <100.00%> (ø)`
...node/opentelemetry-instrumentation-pg/src/utils.ts	`97.65% <ø> (ø)`

maryliag · 2024-12-10T22:19:00Z

The reason for this versioning being pinned, it's because this package is using the incubating entrypoint, which may break on minor bumps.
This was added on purpose here: #2473
Once it's no longer using the incubating, it can be unpinned.

I see other packages that use the incubating entrypoint are not pinning the value, like this one. @pichlermarc do you know if this was just a mistake or we can now unpin for these cases?

nwalters512 · 2024-12-10T22:22:30Z

Ah! Apologies, I didn't go digging deep enough. In that case, can I just change this PR to pin to 1.28.0 so that it will be deduped by package managers (at least until 1.29.0 comes out)?

pichlermarc · 2024-12-11T16:35:51Z

I see other packages that use the incubating entrypoint are not pinning the value, like this one. @pichlermarc do you know if this was just a mistake or we can now unpin for these cases?

Hi 🙂 I'd say it's a mistake. When using the incubating entrypoint, we should always pin for now until we have a decision on open-telemetry/opentelemetry-js#5182. Sorry for the inconvenience.

maryliag · 2024-12-11T17:04:23Z

Hi @nwalters512 , looks like we will go with a different approach of copying those specific values from the incubation, see more here: open-telemetry/opentelemetry-js#5182 (comment)

I would be happy to review if that is something you want to work on.

nwalters512 · 2024-12-11T18:56:51Z

@maryliag I'd be happy to open some PRs for this! I'll work on instrumentation-pg to start, and I can follow up with other packages that are using the incubating entrypoint.

maryliag · 2024-12-11T19:02:55Z

thank you @nwalters512!

nwalters512

@maryliag I repurposed this PR to include the inlining of the relevant constants.

nwalters512 · 2024-12-11T19:10:20Z

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

@@ -65,7 +65,7 @@ import {
  METRIC_DB_CLIENT_OPERATION_DURATION,
  ATTR_DB_NAMESPACE,
  ATTR_DB_OPERATION_NAME,
-} from '@opentelemetry/semantic-conventions/incubating';
+} from './semantic-conventions';


open-telemetry/opentelemetry-js#5182 (comment) suggested using semconv.ts; I went with semantic-conventions.ts since it directly matches the package name. Happy to use the other suggestion if that's preferred.

nwalters512 · 2024-12-11T19:10:50Z

plugins/node/opentelemetry-instrumentation-pg/src/semantic-conventions.ts

+/**
+ * These constants are considered experimental exports of `@opentelemetry/semantic-conventions`.
+ * They're being inlined until they're officially exported by `@opentelemetry/semantic-conventions`.
+ */


This little comment will appear at the top of this file in other packages; let's workshop it if needed before it proliferates.

FWIW, in a third-party instrumentation I have at work that is doing this, I have this block comment:
https://github.com/elastic/elastic-otel-node/blob/690896cb2995a349313b37ae6e609a741676946c/packages/instrumentation-openai/src/semconv.ts#L20-L27

That's perhaps a little wordier than necessary, though.

Agreed that it's perhaps a bit wordy. I'd be happy to apply any recommendations to the message I have here!

nwalters512 · 2024-12-11T19:11:32Z

plugins/node/opentelemetry-instrumentation-pg/src/semantic-conventions.ts

+/**
+ * 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`.


I copied these all in full, including the "experimental" warning. Happy to edit this to remove it desired.

I think either way is fine.

(Ideally eventually there is some handy tooling for this to help with generation and maintenance of these. I'd played with a start at something, but it is far from ready for others to use:
https://github.com/elastic/elastic-otel-node/blob/main/packages/instrumentation-openai/scripts/semconv-gen.js)

nwalters512 · 2024-12-11T19:30:31Z

Sorry about the failing tests, I'll get those fixed up.

maryliag · 2024-12-12T13:50:53Z

Thank you for working on this!

@trentm would you mind confirming if that is what you had in mind?

trentm

@trentm would you mind confirming if that is what you had in mind?

Yes, this is what I had in mind. Thanks!

I added a couple comments and a nit. Good to go as is, however.

trentm · 2025-01-16T23:28:59Z

plugins/node/opentelemetry-instrumentation-pg/src/semantic-conventions.ts

+/**
+ * These constants are considered experimental exports of `@opentelemetry/semantic-conventions`.
+ * They're being inlined until they're officially exported by `@opentelemetry/semantic-conventions`.
+ */


FWIW, in a third-party instrumentation I have at work that is doing this, I have this block comment:
https://github.com/elastic/elastic-otel-node/blob/690896cb2995a349313b37ae6e609a741676946c/packages/instrumentation-openai/src/semconv.ts#L20-L27

That's perhaps a little wordier than necessary, though.

trentm · 2025-01-16T23:30:50Z

plugins/node/opentelemetry-instrumentation-pg/src/semantic-conventions.ts

+/**
+ * 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`.


I think either way is fine.

(Ideally eventually there is some handy tooling for this to help with generation and maintenance of these. I'd played with a start at something, but it is far from ready for others to use:
https://github.com/elastic/elastic-otel-node/blob/main/packages/instrumentation-openai/scripts/semconv-gen.js)

trentm · 2025-01-16T23:34:44Z

plugins/node/opentelemetry-instrumentation-pg/src/semantic-conventions.ts

nit: I have a small preference for "semconv.ts" if others think that is reasonable as the filename. (It is the name I mentioned as an example at https://github.com/open-telemetry/opentelemetry-js/blob/main/semantic-conventions/README.md#unstable-semconv)

I'm happy to be overridden by others' opinions on this. The current naming of .ts files in this repo does tend to use this-style.ts rather than using abbreviations in filenames.

I commented on this here too: #2599 (comment)

I'll defer to whatever you or the other maintainers think is best 🙂

… package dependency with caret (`^`) version to sync with other instrumentation packages (#2664) Refs: #2599 (similar work)

trentm · 2025-01-17T18:17:46Z

@nwalters512 #2664 is doing the same thing. There was discussion on Slack to refine that one. I'm merging that one and closing this one. Thanks for the changes!

chore(instrumentation-pg): Unpin @opentelemetry/semantic-conventions

a27311d

nwalters512 requested a review from a team as a code owner December 10, 2024 21:24

github-actions bot added the pkg:instrumentation-pg label Dec 10, 2024

github-actions bot assigned maryliag Dec 10, 2024

github-actions bot requested a review from maryliag December 10, 2024 21:25

Inline incubating constants from semantic-conventions

3a42a6a

nwalters512 changed the title ~~chore(instrumentation-pg): Unpin @opentelemetry/semantic-conventions~~ chore(instrumentation-pg): Inline incubating constants from @opentelemetry/semantic-conventions Dec 11, 2024

nwalters512 commented Dec 11, 2024

View reviewed changes

Add missing header

514a1d5

Inline more constants; fix failing tests

6a07e55

Merge branch 'main' into pg-unpin-semantic-conventions

ee5803f

trentm approved these changes Jan 16, 2025

View reviewed changes

Merge branch 'main' into pg-unpin-semantic-conventions

b8aa803

trentm mentioned this pull request Jan 17, 2025

chore(instrumentation-pg): have @opentelemetry/semantic-conventions package dependency with caret (^) version to sync with other instrumentation packages #2664

Merged

trentm pushed a commit that referenced this pull request Jan 17, 2025

chore(instrumentation-pg): have @opentelemetry/semantic-conventions…

4fb610d

… package dependency with caret (`^`) version to sync with other instrumentation packages (#2664) Refs: #2599 (similar work)

trentm closed this Jan 17, 2025

nwalters512 deleted the pg-unpin-semantic-conventions branch January 17, 2025 20:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(instrumentation-pg): Inline incubating constants from @opentelemetry/semantic-conventions #2599

chore(instrumentation-pg): Inline incubating constants from @opentelemetry/semantic-conventions #2599

nwalters512 commented Dec 10, 2024 •

edited

Loading

codecov bot commented Dec 10, 2024 •

edited

Loading

maryliag commented Dec 10, 2024

nwalters512 commented Dec 10, 2024

pichlermarc commented Dec 11, 2024 •

edited

Loading

maryliag commented Dec 11, 2024

nwalters512 commented Dec 11, 2024

maryliag commented Dec 11, 2024

nwalters512 left a comment

nwalters512 Dec 11, 2024 •

edited

Loading

nwalters512 Dec 11, 2024

trentm Jan 16, 2025

nwalters512 Jan 16, 2025

nwalters512 Dec 11, 2024

trentm Jan 16, 2025

nwalters512 commented Dec 11, 2024

maryliag commented Dec 12, 2024

trentm left a comment

trentm Jan 16, 2025

trentm Jan 16, 2025

trentm Jan 16, 2025

nwalters512 Jan 16, 2025

trentm commented Jan 17, 2025

chore(instrumentation-pg): Inline incubating constants from @opentelemetry/semantic-conventions #2599

chore(instrumentation-pg): Inline incubating constants from @opentelemetry/semantic-conventions #2599

Conversation

nwalters512 commented Dec 10, 2024 • edited Loading

Which problem is this PR solving?

Short description of the changes

codecov bot commented Dec 10, 2024 • edited Loading

Codecov Report

maryliag commented Dec 10, 2024

nwalters512 commented Dec 10, 2024

pichlermarc commented Dec 11, 2024 • edited Loading

maryliag commented Dec 11, 2024

nwalters512 commented Dec 11, 2024

maryliag commented Dec 11, 2024

nwalters512 left a comment

Choose a reason for hiding this comment

nwalters512 Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nwalters512 commented Dec 11, 2024

maryliag commented Dec 12, 2024

trentm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trentm commented Jan 17, 2025

nwalters512 commented Dec 10, 2024 •

edited

Loading

codecov bot commented Dec 10, 2024 •

edited

Loading

pichlermarc commented Dec 11, 2024 •

edited

Loading

nwalters512 Dec 11, 2024 •

edited

Loading