-
Notifications
You must be signed in to change notification settings - Fork 544
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.
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
chore(instrumentation-pg): have @opentelemetry/semantic-conventions
package dependency with caret (^
) version to sync with other instrumentation packages
#2664
Conversation
…package dependency with caret (`^`) version to sync with other instrumentation packages, so latest minor versions will be able to used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is pinned on purpose since the incubating
entrypoint may include breaking changes in minor versions.
See https://github.com/open-telemetry/opentelemetry-js/tree/main/semantic-conventions#unstable-semconv for recommendations on how to handle this case.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2664 +/- ##
==========================================
+ 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
|
…telemetry/semantic-conventions/incubating` package
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks 🙂
cc @maryliag (component owner)
tl;dr: I think we should be using the local copy (src/semconv.ts) in tests as well. [The following is copied from a Slack discussion to capture this visibly somewhere.]
I'm changing my mind on this one. Say one is using .../incubating in tests, then the version of the semantic-conventions dep is updated that drops one of the used unstable semconv definitions (say, ATTR_DB_CLIENT_CONNECTION_STATE). The runtime code will be fine, but the tests will fail. We don't want this test failure to be a reason to force the particular instrumentation to change for the new version of Semantic Conventions right then. (Eventually the instrumentations should migrate to newer Semantic Conventions, but it doesn't have to be right when we update the @opentelemetry/semantic-conventions dep for the whole opentelemetry-js-contrib.git repo.) IOW, I think we should be using the src/semconv.ts in tests as well. (Ensuring that the copied defs in src/semconv.ts match what is in the actual .../incubating endpoint should, ideally, be the job of a generator tool or a linting script of some sort.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was some discussion on Slack to refine this PR. It mimics #2599 which was doing the same thing. Credit to @nwalters512 as well for the changes.
@opentelemetry/semantic-conventions
package dependency with caret (^
) version to sync with other instrumentation packages@opentelemetry/semantic-conventions
package dependency with caret (^
) version to sync with other instrumentation packages
… so latest minor versions will be able to used
Which problem is this PR solving?
Currently,
@opentelemetry/instrumentation-pg
package has@opentelemetry/semantic-conventions
dependency with direct version (1.27.0
), but other instrumentation packages have that with caret version (^1.27.0
) of it. And this leads to having multiple copies of the@opentelemetry/semantic-conventions
package with different versions in the AWS Lambda Node.js layer.Short description of the changes
Change
@opentelemetry/semantic-conventions
package version to^1.27.0
to be sync with the other instrumentation packages.