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(deps): Widen dependency ranges for non-SDK packages #5165

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mydea
Copy link
Contributor

@mydea mydea commented Nov 15, 2024

Related to #4815

I noticed that e.g. @opentelemetry/instrumentation-http or @opentelemetry/instrumentation-fetch all hard-depend on specific versions of e.g. @opentelemetry/instrumentation, core, semantic-conventions etc. Is this really desired?

Based on https://github.com/open-telemetry/opentelemetry-specification/blob/a03382ada8afa9415266a84dafac0510ec8c160f/specification/upgrading.md?plain=1#L97-L122, if I understand correctly, instrumentation should continue to work for following minor releases of core packages. By relaxing e.g. @opentelemetry/core dependencies to e.g. ^1.26.0 etc. we could make compatibility much easier. As of now, it can be pretty hard to update instrumentation.

FWIW all the instrumentation from opentelemetry-js-contrib seems to already do it that way.

Closes #4975

@mydea mydea requested a review from a team as a code owner November 15, 2024 12:52
Copy link

codecov bot commented Nov 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.17%. Comparing base (6096f72) to head (e7c86ed).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5165      +/-   ##
==========================================
- Coverage   93.18%   93.17%   -0.02%     
==========================================
  Files         314      314              
  Lines        8076     8076              
  Branches     1622     1622              
==========================================
- Hits         7526     7525       -1     
- Misses        550      551       +1     

see 1 file with indirect coverage changes

@pichlermarc
Copy link
Member

At the moment, these being pinned is fully intentional, but not optimal - a few reasons for this:

  • this change will likely be reverted by release automation because it strict-updates dependencies from this monorepo
  • we don't have proper documentation on @opentelemetry/core when exports were added, which makes it difficult to ensure that we depend on the correct minimum, which will drastically increase the time needed to review changes that involve @opentelemetry/core
  • we don't have any way to automate checking if we depend on the correct minimum version in this repo and always the latest version from this repo will be linked, which introduces significant potential for human error during reviews

Given these problems, I don't think we can merge this PR as-is. I completely agree that this is annoying the way it is and I'd love to see this changed in the future, but that change has to be sustainable in the sense of how much effort we need to put in to maintain it long-term.

We do have challenges with regards to maintainability of the project already, and I think this change would not exactly improve the situation at this point in time. I'd much rather do other things like #5148 first and then re-visit this at a later point in time when we've reached a point where this is sustainable.

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.

Widen dependency ranges for related packages for instrumentation
2 participants