-
Notifications
You must be signed in to change notification settings - Fork 468
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
Upgrade to hyper/http v1.0 #1674
Conversation
|
Lint needs to have a feature enabled that's transitively enabled if the full workspace is checked, will do that later. |
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 have a few minor points. Feel free to ignore it if you don't care about the features as this should never break anyone unless a downstream crate or cargo make breaking changes and at that point all bets are off.
I think this could also be merged without waiting on tonic
by adding a dependency on [email protected]
to opentelemetry-otlp
and using that for tonic
-specific errors since that's the only place where these types figure as far as I could find.
Sidenote: The some tests in this repository seem flakey? Locally I sometimes have to rerun the tests a few times before they pass. Weird. |
Yeah feel free to report those test in #1559. Most of them are because global setup / env vars and we are working on addressing them as they pop up |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1674 +/- ##
=======================================
- Coverage 75.0% 75.0% -0.1%
=======================================
Files 122 122
Lines 20276 20290 +14
=======================================
Hits 15222 15222
- Misses 5054 5068 +14 ☔ View full report in Codecov by Sentry. |
(missed that one during the last rebase) |
I was wondering if this is planned to be merged anytime soon? |
I'm intending to get this over the finish line eventually, issue is just that the tonic PR is still pending |
As soon as it's released, sure! |
Seems like tonic is close to releasing v0.12 with hyper v1 support. |
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.
(Drive-by feedback:)
This is looking pretty good, looking forward to getting this released! Have some nits for your consideration.
Not intended. Probably mistyped somewhere when attempting to reset the git submodule. Gonna revert that |
opentelemetry-proto/src/proto/tonic/opentelemetry.proto.collector.logs.v1.rs
Show resolved
Hide resolved
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. Thanks for the work @aumetra. Will keep this open for another day to give other approvers a chance to review before merging.
@lalitb are you planning a release? Need help with that? |
@djc yes I believe @cijothomas plans to do a release today if there are no issues. |
Great, looking forward to it! |
Fixes #1427
Blocked by:
Changes
Merge requirement checklist
CHANGELOG.md
files updated for non-trivial, user-facing changes