-
Notifications
You must be signed in to change notification settings - Fork 821
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: bump otlp trace exporters to v1 #2626
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2626 +/- ##
=======================================
Coverage 92.52% 92.52%
=======================================
Files 144 144
Lines 5177 5177
Branches 1102 1102
=======================================
Hits 4790 4790
Misses 387 387 |
19a849f
to
446d9be
Compare
@@ -1,6 +1,6 @@ | |||
{ | |||
"name": "@opentelemetry/exporter-trace-otlp-grpc", | |||
"version": "0.27.0", | |||
"version": "1.0.0", |
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.
Version must match the lerna.json version
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. Changed that. I did think that we version the packages independently though... 🤔
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.
No we only have 2 versions, the experimental and the stable versions are different. You may be thinking of contrib where the versions are completely independent.
Tests pass with the package linked. |
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.
lgtm, however i don't understand why experimentals are failling even after removing tsconfig refs
Sorry for such a late reply here. I found this reference https://github.com/Rauno56/opentelemetry-js/blob/chore/stable-otlp/experimental/packages/opentelemetry-exporter-metrics-otlp-http/tsconfig.esm.json#L13 |
Tests are still failing but at least the build passes now. I haven't looked into the test failure yet but do you know what might be causing it? |
It might be because of some unpublished changes to the metrics exporter - it linked the packages before so that the tests could be in sync with the dev version of the implementation. Now when the packages are separated it relies on the published versions. |
I think we already dicussed skippign tests for metrics exporter while we are updating the SDK @dyladan ? i don't see a problem WDYT ? |
The published version should be the same as the last dev version. we published right before removing it. i'm not sure why that would cause the test to fail |
595b8d9
to
208ab8e
Compare
Redid the move to resolve conflicts... |
208ab8e
to
c979d59
Compare
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.
lgtm, just need to decide what to do with tests
Yeah... I'm a bit lost here. After doing a clean install on just metrics grpc exporter, the tests pass locally. Until I update to
I'm following the CI process step-by-step. I'm afraid the issue is caused by some lerna-magic. |
Alright... as you can see... 🎉 ee61e1e |
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.
LGTM
If you want to remove the opentelemetry-
prefix i'm fine with it.
👍 Will do. |
Which problem is this PR solving?
The trace exporters are currently experimental even though we don't really consider them to be as such.
Short description of the changes
This moves the code from experimental folder to the main one changing as little on the way as possible.
opentelemetry-
prefix from the folder. Are we open to doing so?