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 Tokio to v1 #421

Merged
merged 15 commits into from
Jan 18, 2021
Merged

Upgrade Tokio to v1 #421

merged 15 commits into from
Jan 18, 2021

Conversation

mraerino
Copy link
Contributor

@mraerino mraerino commented Jan 12, 2021

Fixes #420

I also upgraded these dependencies:

  • reqwest
  • hyper
  • tonic
  • prost

@mraerino mraerino requested a review from a team January 12, 2021 12:48
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jan 12, 2021

CLA Signed

The committers are authorized under a signed CLA.

@mraerino mraerino mentioned this pull request Jan 12, 2021
@mraerino
Copy link
Contributor Author

mraerino commented Jan 12, 2021

the minimum supported rust version for tokio is 1.45: https://github.com/tokio-rs/tokio#supported-rust-versions

do you think we could raise the version here as well? 1.42 seems pretty old

@codecov
Copy link

codecov bot commented Jan 12, 2021

Codecov Report

Merging #421 (0f562c3) into master (777fdb9) will decrease coverage by 2.80%.
The diff coverage is 58.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #421      +/-   ##
==========================================
- Coverage   51.55%   48.74%   -2.81%     
==========================================
  Files          65       66       +1     
  Lines        5117     5438     +321     
==========================================
+ Hits         2638     2651      +13     
- Misses       2479     2787     +308     
Impacted Files Coverage Δ
opentelemetry-otlp/src/span.rs 3.26% <0.00%> (-30.08%) ⬇️
opentelemetry/src/lib.rs 100.00% <ø> (ø)
opentelemetry/src/sdk/trace/provider.rs 75.51% <ø> (ø)
opentelemetry/src/util.rs 18.30% <42.85%> (ø)
opentelemetry/src/sdk/trace/span_processor.rs 81.97% <100.00%> (ø)
opentelemetry/src/context.rs 32.02% <0.00%> (-0.22%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 777fdb9...0f562c3. Read the comment docs.

@TommyCpp
Copy link
Contributor

do you think we could raise the version here as well? 1.42 seems pretty old

I think we can. We promised three versions before current stable version, which is 1.49. So we can bump our minimum supported version to 1.45 or 1.46

@mraerino
Copy link
Contributor Author

This is ready for a review now.
I expect this to need a major version bump since a feature flag changed and people can't update automatically via semver since they need to upgrade their tokio version too

@jtescher
Copy link
Member

@mraerino thanks for working on this, as far as MSRV is concerned we could go either way, for example the async-std feature currently doesn't support 1.42 either. Maybe we should just remove the tokio support from the "supported" features for 1.42 and the associated ci workflow and bump it in a subsequent PR? Nice to support folks on older versions in the more stable core even if opt-in features can't support them.

Also pre-1.0 minor changes often include breaking changes, so not as much of a problem there until we hit GA

@mraerino
Copy link
Contributor Author

Maybe we should just remove the tokio support from the "supported" features for 1.42 and the associated ci workflow and bump it in a subsequent PR?

tokio is used in the tests even if zero features are enabled. i don't think what you propose is possible with the current CI flow

@djc
Copy link
Contributor

djc commented Jan 13, 2021

Maybe we should just remove the tokio support from the "supported" features for 1.42 and the associated ci workflow and bump it in a subsequent PR? Nice to support folks on older versions in the more stable core even if opt-in features can't support them.

Alternatives:

The reason I hadn't done the upgrade here is there are also some tonic dependencies, and tonic support for tokio 1 has not yet been released. So if we want to prevent spawning multiple runtimes in the background, this PR should (a) wait until tonic 0.4 has been released -- judging from chat activity, this should come pretty soon, or (b) change crates depending on tonic to depend on a released version of opentelemetry, sticking to tokio 0.2 for now.

(Otherwise the changes here look good to me; my one nit is the underscore in the tokio_support feature flag, where I feel hyphens are probably more common.)

@mraerino
Copy link
Contributor Author

thanks for pointing out tonic! totally did not see that.
the PR is merged so once they release it we can move forward: hyperium/tonic#530

@mraerino
Copy link
Contributor Author

mraerino commented Jan 13, 2021

i made #424 to test the otlp exporter closer to reality. With that test, CI would be red here right now since tonic fails to run in a Tokio v1 executor.

@djc
Copy link
Contributor

djc commented Jan 15, 2021

FYI, tonic 0.4 has been released.

@mraerino
Copy link
Contributor Author

yup, i saw. merging master and upgrading tonic

@mraerino

This comment has been minimized.

Copy link
Contributor

@djc djc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than the nit below, this is looking good to me!

Co-authored-by: Dirkjan Ochtman <[email protected]>
@jtescher jtescher merged commit 6728799 into open-telemetry:master Jan 18, 2021
@mraerino
Copy link
Contributor Author

@djc @jtescher is there a timeline for releasing this?

@jtescher
Copy link
Member

ASAP

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.

Upgrade to Tokio 1.0
4 participants