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

Configure async runtime in Rust code instead of cargo feature #481

Merged
merged 5 commits into from
Mar 21, 2021

Conversation

frigus02
Copy link
Member

@frigus02 frigus02 commented Mar 14, 2021

Extracted from #469.

Configure the async runtime (Tokio, async-std, ...) in Rust using install_batch(runtime) on the trace pipeline builders instead of using cargo features.

The main advantages of using Rust code is that cargo doesn't have a way of describing mutually exclusive features at the moment. We currently fall back to Tokio if multiple runtime features are enabled. This can be confusing, especially if a runtime feature is enabled by another crate.

Choosing the runtime in Rust code means the entire trace pipeline configuration is in one place.

With this:

  • The rt-* cargo features can be enabled at the same time. The only downside of enabling more features is a longer build time.
  • Users have to pick a runtime by providing an object implemeting a Runtime trait or use install_simple() to use the simple span processor.

Examples:

let tracer = opentelemetry_jaeger::new_pipeline()
    .install_simple()?;
let tracer = opentelemetry_jaeger::new_pipeline()
    .install_batch(opentelemetry::runtime::Tokio)?;

@frigus02 frigus02 marked this pull request as ready for review March 15, 2021 07:16
@frigus02 frigus02 requested a review from a team March 15, 2021 07:16
@frigus02 frigus02 force-pushed the with-runtime branch 2 times, most recently from a01e3bb to 3c9c1d8 Compare March 17, 2021 19:01
@TommyCpp
Copy link
Contributor

Might need an empty commit to nudge the EasyCLA

Copy link
Contributor

@TommyCpp TommyCpp left a comment

Choose a reason for hiding this comment

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

Nice job 👍

Copy link
Member

@jtescher jtescher left a comment

Choose a reason for hiding this comment

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

LGTM @djc additional comments?

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.

Nope, LGTM!

@frigus02
Copy link
Member Author

I'm going to test the examples now to make sure they all still work

@frigus02
Copy link
Member Author

Result:

  • No examples broken because of this PR
  • Some examples had issues. I added commits to fix them in this PR
  • I didn't manage to fix the grpc example. The client span doesn't seem to be sent.

From my side this is ready to be merged.

Configure the async runtime (Tokio, async-std, ...) in Rust using
`with_runtime(runtime)` on the trace pipeline builders instead of using
cargo features.

The main advantages of using Rust code is that cargo doesn't have a way
of describing mutually exclusive features at the moment. We currently
fall back to Tokio if multiple runtime features are enabled. This can be
confusing, especially if a runtime feature is enabled by another crate.

Choosing the runtime in Rust code means the entire trace pipeline
configuration is in one place.
- Remove `()` runtime implementation
- Replace `install` function with `install_simple` and `install_batch`
@jtescher jtescher merged commit cb419bb into open-telemetry:main Mar 21, 2021
@frigus02 frigus02 deleted the with-runtime branch April 5, 2021 15:28
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.

4 participants