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

Upgrade to hyper/http v1.0 #1674

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open

Conversation

aumetra
Copy link

@aumetra aumetra commented Apr 22, 2024

Fixes #1427

Blocked by:

Changes

  • Update all hyper and http dependencies to v1.x
  • Update reqwest to v0.12
  • Remove isahc since it's still stuck on http v0.2

Merge requirement checklist

  • CONTRIBUTING guidelines followed
  • Unit tests added/updated (if applicable)
  • Appropriate CHANGELOG.md files updated for non-trivial, user-facing changes
  • Changes in public API reviewed (if applicable)

Copy link

linux-foundation-easycla bot commented Apr 22, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@aumetra
Copy link
Author

aumetra commented Apr 22, 2024

Lint needs to have a feature enabled that's transitively enabled if the full workspace is checked, will do that later.
The external types check is broken because the nightly used is too old and axum unconditionally enables the diagnostic attribute on nightly without the feature flag because they assume everyone is at least on a nightly that is 1.68

Copy link

@mladedav mladedav left a 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 http@0.2 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.

examples/tracing-http-propagator/src/server.rs Outdated Show resolved Hide resolved
opentelemetry-http/Cargo.toml Outdated Show resolved Hide resolved
opentelemetry-http/src/lib.rs Outdated Show resolved Hide resolved
opentelemetry-jaeger/Cargo.toml Outdated Show resolved Hide resolved
opentelemetry-zipkin/src/lib.rs Outdated Show resolved Hide resolved
@aumetra
Copy link
Author

aumetra commented May 3, 2024

Sidenote: The some tests in this repository seem flakey? Locally I sometimes have to rerun the tests a few times before they pass. Weird.

Cargo.toml Show resolved Hide resolved
@TommyCpp
Copy link
Contributor

TommyCpp commented May 8, 2024

The some tests in this repository seem flakey

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

Cargo.toml Show resolved Hide resolved
Copy link

codecov bot commented May 9, 2024

Codecov Report

Attention: Patch coverage is 28.57143% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 74.2%. Comparing base (595ad05) to head (40fb924).

Files Patch % Lines
opentelemetry-http/src/lib.rs 0.0% 5 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff          @@
##            main   #1674   +/-   ##
=====================================
  Coverage   74.1%   74.2%           
=====================================
  Files        125     125           
  Lines      19481   19469   -12     
=====================================
  Hits       14452   14452           
+ Misses      5029    5017   -12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@aumetra
Copy link
Author

aumetra commented May 9, 2024

(missed that one during the last rebase)

@NOBLES5E
Copy link

I was wondering if this is planned to be merged anytime soon?

@aumetra
Copy link
Author

aumetra commented May 17, 2024

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

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.

[Feature]: Port opentelemetry-http to hyper 1.x
6 participants