opentelemetry-rust icon indicating copy to clipboard operation
opentelemetry-rust copied to clipboard

Upgrade to hyper/http v1.0

Open aumetra opened this issue 1 year ago • 11 comments
trafficstars

Fixes #1427

Blocked by:

  • https://github.com/hyperium/tonic/pull/1670
  • https://github.com/tokio-rs/axum/pull/2693

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

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

aumetra avatar Apr 22 '24 15:04 aumetra

CLA Signed

The committers listed above are authorized under a signed CLA.

  • :white_check_mark: login: aumetra / name: Aumetra Weisman (c93e43f9ae12868a01ebbb7dda2c958b733209b4, 25f9213ab87eeb080df88242aba333c37bbc82ec, 1a3d711b4d983c9d9b17ff7c5547043d0b5069a5, e22bb0b063fc42fa0813f537df907bd84b3eecba, f5c73eb240aec3934af0e2fe62a41ce1ca18ee22, 75b325927004ccc9f1d0dbba5be98620a12ef000, 7d1c3a1635d95a01fd966fb99e7ef14f3d9b377c, 95608c5759d34fa825767e375fc66738b0007b56, 318bb62b530cdb043191d24998eb6a4d369ee5ff, 88ad41698a4e36269fa8668006c9e661d7c9dcfa, 19ddbd8ae6066feb41205f9918dee185d9d99ebd, 8ad764ac9ea5fef69788f083405560ad0d8b1149, 6f34ee354a31ea1822470c70480047c0283c80f2, 40fa5766e3cc2786c1010764e7b841c96cd60c6a, adda11d6d3a55144c74ab850e45a02b77b99c2b2, 70a9bc7c6cb215e191f9a57577fdfdd8b937c841, a87360bb186130e87eb44f37a1baa93fd6a5131a, 6c75e7c5d8c98ede77c14bece26bb33810ddadc6, 45903d803b529eaed84cf2bfdae676ec6b0d846a, e52ba2c29618767a0616bdb8c0d3660e2483f816, af97e0ed5a6de7748c78b99f0e59c110934e2a0b, eb2ae3295d3f8c5f1670d21a4c09edb71650bae9, 650e688f7f4f6e59e37aa7e83d2b4d5e104da00b, 8e6c5d2a9e7c8839213832b4c62ee7ca9c0b73db, 0664b648d9dda61739f86dd433c66f3751b42da3, 23c09c781e478dbad5992c3df309b8997e980d54, 5944e90d64cd9fc397ded0b18f13b6550bebb7db, 16783065f157ba507670057325938f3a4ff5b1b9, 832b1cb3496be28fc303671f1c9375327464aeab, c8409787d4923d1efc557a82fd458adcdaecafaa, 1d1cef9c6f1d481830ea1448300080abe4e63b56, ab7a3a4154bae41780362678e1f697f00ffb6f63)
  • :white_check_mark: login: lalitb / name: Lalit Kumar Bhasin (7d9670f2c19a0f6250e2e5f494cc84459717805b)

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

aumetra avatar Apr 22 '24 21:04 aumetra

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

aumetra avatar May 03 '24 15:05 aumetra

The some tests in this repository seem flakey

Yeah feel free to report those test in https://github.com/open-telemetry/opentelemetry-rust/issues/1559. Most of them are because global setup / env vars and we are working on addressing them as they pop up

TommyCpp avatar May 08 '24 03:05 TommyCpp

Codecov Report

Attention: Patch coverage is 10.00000% with 18 lines in your changes missing coverage. Please review.

Project coverage is 75.0%. Comparing base (df815bd) to head (7d9670f). Report is 1 commits behind head on main.

Files Patch % Lines
opentelemetry-http/src/lib.rs 0.0% 18 Missing :warning:
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     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar May 09 '24 19:05 codecov[bot]

(missed that one during the last rebase)

aumetra avatar May 09 '24 22:05 aumetra

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

NOBLES5E avatar May 17 '24 01:05 NOBLES5E

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

aumetra avatar May 17 '24 20:05 aumetra

@aumetra The tonic PR is merged. do you think we can now remove the patch from this PR, and make it ready to review/merge.

Edit - Or probably better to wait for tonic release with merged changes.

lalitb avatar Jun 13 '24 20:06 lalitb

As soon as it's released, sure!

aumetra avatar Jun 13 '24 20:06 aumetra

Seems like tonic is close to releasing v0.12 with hyper v1 support.
After they publish it, I'll rebase and remove the patches

https://github.com/hyperium/tonic/pull/1740

aumetra avatar Jun 21 '24 06:06 aumetra

@aumetra as https://github.com/hyperium/tonic/pull/1740 is merged, can you resolve the conflicts in this PR, and fixing the patch dependency, so that it can be reviewed and merged ?

lalitb avatar Jul 08 '24 22:07 lalitb

Sorry, had to set my PC up again after moving :P Gonna rebase it real quick and then address the review comments.

aumetra avatar Jul 09 '24 10:07 aumetra

This ::prost::Enumeration issue (https://github.com/tokio-rs/prost/issues/1097) is a bit annoying. I silenced it for now because it doesn't seem like there is an actual solution to the problem yet.

aumetra avatar Jul 09 '24 11:07 aumetra

prost v0.13.0 got yanked so we gotta deal with the broken CI for a little bit

aumetra avatar Jul 09 '24 13:07 aumetra

0.13.1 has been released.

djc avatar Jul 09 '24 13:07 djc

Well then, let's see how far it gets now.

aumetra avatar Jul 09 '24 13:07 aumetra

Looking pretty good. Just a question to the code owners if they'd prefer to split the isahc removal into a separate PR. I'm fine with doing either.

aumetra avatar Jul 09 '24 14:07 aumetra

Looking pretty good. Just a question to the code owners if they'd prefer to split the isahc removal into a separate PR. I'm fine with doing either.

Thanks @aumetra . As @djc suggested, good to split as separate PR.

lalitb avatar Jul 09 '24 14:07 lalitb

Looking pretty good. Just a question to the code owners if they'd prefer to split the isahc removal into a separate PR. I'm fine with doing either.

Thanks @aumetra . As @djc suggested, good to split as separate PR.

Unless you think it would be difficult to split, as isahc client depends on http 0.2.1.

lalitb avatar Jul 09 '24 15:07 lalitb

I would split it up like this:

  • create PR to delete that client
  • get that merged
  • rebase on main after merge
  • merge this

aumetra avatar Jul 09 '24 15:07 aumetra

+1 on split the client clean up to another PR to control the scope

TommyCpp avatar Jul 09 '24 15:07 TommyCpp

Removed isahc support in https://github.com/open-telemetry/opentelemetry-rust/pull/1924, gonna rebase this on that branch so we can move ahead.

aumetra avatar Jul 10 '24 05:07 aumetra

Just rebased it real quick to make the eventual rebase on main based on https://github.com/open-telemetry/opentelemetry-rust/pull/1924 easier. Waiting for that PR to merge for now.

aumetra avatar Jul 10 '24 05:07 aumetra

@aumetra Is the upgrade to opentelemetry-proto submodule intended/required? If not, can we remove it from this PR, to be handled separately.

lalitb avatar Jul 10 '24 21:07 lalitb

@aumetra Is the upgrade to opentelemetry-proto submodule intended/required? If not, can we remove it from this PR, to be handled separately.

Not intended. Probably mistyped somewhere when attempting to reset the git submodule. Gonna revert that

aumetra avatar Jul 11 '24 07:07 aumetra

@lalitb are you planning a release? Need help with that?

djc avatar Jul 12 '24 14:07 djc

@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.

lalitb avatar Jul 12 '24 15:07 lalitb

Great, looking forward to it!

djc avatar Jul 12 '24 16:07 djc