Skip to content
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: unpin semantic-conventions package dep #5439

Merged
merged 2 commits into from
Feb 11, 2025

Conversation

trentm
Copy link
Contributor

@trentm trentm commented Feb 7, 2025

There is no need to pin deps for the semantic-conventions package.
Our recommendation is to not pin, to allow for de-duplication of installs
of the package in node_modules trees to reduce install size.
The concerns that motivate pinning other otel deps, described in
#5283, do
not apply to the semantic-conventions package.

This also removes the updating of the semconv deps when the semconv
package is released. There should be no need to do so. A particular
package can bump its minimum semconv dep if/when it uses exports
in a newer version.

There is no need to pin deps for the semantic-conventions package.
Our recommendation is to not pin, to allow for de-duplication of installs
of the package in node_modules trees to reduce install size.
The concerns that motivate pinning other otel deps, described in
open-telemetry#5283, do
not apply to the semantic-conventions package.

This also removes the updating of the semconv deps when the semconv
package is released. There should be no need to do so. A particular
package can bump its minimum semconv dep if/when it uses exports
in a newer version.
@trentm trentm self-assigned this Feb 7, 2025
@trentm trentm requested a review from a team as a code owner February 7, 2025 22:48
Copy link

codecov bot commented Feb 7, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.79%. Comparing base (b9d5aee) to head (045801c).
Report is 5 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5439   +/-   ##
=======================================
  Coverage   94.79%   94.79%           
=======================================
  Files         310      310           
  Lines        7974     7974           
  Branches     1682     1682           
=======================================
  Hits         7559     7559           
  Misses        415      415           

Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

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

Looks good. 👍

@pichlermarc
Copy link
Member

Note for anyone looking at this who's thinking of un-pinning other packages:

  • @opentelemetry/semantic-conventions is an exception for now
  • we're aware that other packages should be un-pinned as well
    • It looks easy and safe to do on the surface but it is not (see [all] limitations of our type exports and proposed ways forward #5283). We're working on that and we've already done a lot of work to make that happen, please be patient.
    • un-pinning packages will be initiated by maintainers or approvers of the repository - PRs opened by anyone else will be closed.

@pichlermarc pichlermarc added this pull request to the merge queue Feb 11, 2025
Merged via the queue into open-telemetry:main with commit 824814f Feb 11, 2025
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants