Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: depend on API ^1.0.0 also in devDependencies #1012
chore: depend on API ^1.0.0 also in devDependencies #1012
Changes from 6 commits
65528ad4b68445c103355dcfe03235baaca9b58ab04a245d6File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
pin dependencies within the same learna monorep, forgot this one in a previous PR #984
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.
Please help me understand why are we supposed to pin "monorepo-local" packages / why should we make those follow an different rule than external pacakges.
There are more:
@opentelemetry/propagator-aws-xray@^1.0.1in@opentelemetry/instrumentation-aws-lambda@opentelemetry/instrumentation-express@^0.28.0inexpress-example@opentelemetry/instrumentation-document-load@^0.27.1in@opentelemetry/browser-extension-autoinjection@opentelemetry/auto-instrumentations-*Are these also supposed to be pinned? Why not?
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.
We do it in core repo and I see no reason to do it different here. They are released in one shot and user should update all of them or nothing. So it's always consistent as during tests here.
At least this is my understanding based on the setup in core repo.
Whenever I had a mixup of versions it ended up in a nightmare.
And yes, you are right, there are a few dependencies I haven't seen yet. In general I have the feeling that dependency management in this (and core) repo should be more automated but till now I found no time to get familiar with renovate bot.
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.
I'm still not sure what is the best way to setup the dependencies. It seems that every release made has it's own followup problems regarding dependencies. Not sure if we are able to get to a setup which just works.
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.
I'm all for consistency, just trying to understand the reasoning behind it. So the main reason is to mimic the repository setup since they are linked in CI. That makes sense.
I'm also totally for automation, but currently I personally don't even always understand the rules to automate them. Hence me bombarding you with questions. :)
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.
What about the examples I listed above tho of other places that do not follow the same rule?
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.
I haven't touched the examples (except that on part of the lerna project) as they have no tests and I don't want to run all of them manually to verify that they still work.
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.
By "examples I listed above" I meant:
Which by the rule of "monorepo local deps should be pinned" are currently inconsistent.
What comes to
@opentelemetry/instrumentationpackage, I'd leave that to^0.27for now and specify http and grpc instrumentations accordingly to a version that uses^0.27- we can then update that separately. What do you think?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.
I was involved in pinning the dependencies in core repo and support doing this here as well.
At aspecto, we craft our own distribution of OpenTelemetry and publish it to our users. To guarantee stability, we pin every otel package dependency to exact version. When a new otel version is released, we first test it with our distro and backend and when we verify there are no issues, we release a new version of our distro to clients.
Now, imagine that someone accidentally introduces a breaking change or bug into a patch version. If the dependencies are defined with a caret, then the new patch version will be picked up automatically, break the installation, and there is nothing the user can do to resolve this issue.
This is the original issue that was followed with pinning transitive same-mono-repo dependencies
For example:
User pinned "@opentelemetry/[email protected]", and it depends on "@opentelemetry/propagation-utils@^0.27.0". Now if "0.27.1" is published and it's broken - user's app is broken until a patch fix is published :(
User pinned "@opentelemetry/[email protected]", and it depends on "@opentelemetry/[email protected]". Now if "0.27.1" is published, it will go along with "@opentelemetry/[email protected]" and user has the freedom to test 0.6.1 and upgrade when he wants :)
User does not pin - it uses "@opentelemetry/instrumentation-aws-sdk@^0.6.0". Now if "@opentelemetry/[email protected]" is published lerna will also publish "@opentelemetry/[email protected]" which means the user that asked to depend on caret will automatically get the new transitive version as he asked :)
If I am not missing anything, I believe this strategy covers both needs and makes sense
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.
If we depend on old http instrumentation this will pull in old SDK components and likely to new problems.
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.
correct SDK/core deps here - forgot this one in #984
As this example is part of the lerna project it needs to be kept in sync as it participates in hoisting. Other examples can be updated independent.
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.
#984 has some of the same dependencies pinned for the browser extension package but not others, afaict. Why is that?
The extension pacakge also participates in hoisting.
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.
I pinned dev-dependencies from other repos as this seems to be the standard setup here and in core.
Not pinning dependencies to local repos was not my focus that time but I think it should be done. But I guess for dev-deps it's anyway don't care as they have no influence to end user and lerna links pinned and non pinned dependencies.
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.
So should we achieve consistency between this here and browser extension 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.
Yes, I think we should be consistent.