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

Add support for OpenTelemetry 0.21 #118

Merged
merged 6 commits into from
Nov 10, 2023
Merged

Add support for OpenTelemetry 0.21 #118

merged 6 commits into from
Nov 10, 2023

Conversation

vriesk
Copy link
Contributor

@vriesk vriesk commented Nov 8, 2023

Issue #, if available:

Description of changes:

Adds support for OpenTelemetry 0.21. Nothing particularly interesting. While here, getting rid of the Fibonacci-exploding number of compile guards and use a single const-evaluated expression.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

src/otel.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated

#[cfg(all(feature = "opentelemetry_0_19", feature = "opentelemetry_0_20"))]
compile_error!("feature \"opentelemetry_0_19\" and feature \"opentelemetry_0_20\" cannot be enabled at the same time");
const _OTEL_GUARD: () = assert!({
Copy link
Contributor

@omid omid Nov 9, 2023

Choose a reason for hiding this comment

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

It breaks it.
I don't see the error while building

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's because compiler fails earlier than running/replacing consts.

Copy link
Contributor Author

@vriesk vriesk Nov 9, 2023

Choose a reason for hiding this comment

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

No, it's not runtime, const _: () forces compile-time evaluation of a const block here.

It might be a later compiler pass than checking import name clashes, so errors look different, but this will prevent compilation from working.

See https://internals.rust-lang.org/t/nicer-static-assertions/15986

Copy link
Contributor

Choose a reason for hiding this comment

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

Anyway, compilation will fail, even if you remove this bit.
Having assert is as useful as not having it. But compile_error will be triggered earlier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I can revert this if that's your preference, however I don't think maintaining an ever-expanding and quite a bit error prone Cartesian cross-product of possible flag combinations is a good approach.

Ideally, there could be some macro that would take a feature list and then construct the cross product for us, but I'm unaware of such a macro and then whether it's at all possible as cfg! and #[cfg(..)] expression are a bit magical and might be evaluated earlier than any possible macro could kick in.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Owner

Choose a reason for hiding this comment

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

Happy to add the dependency!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If @LukeMathWalker agrees, we can use https://crates.io/crates/mutually_exclusive_features

Cool, done. Is the @recurs [params: ...] a common convention for recursive declarative macros BTW? Haven't seen it yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure how common it is. I learned that from diesel macros, eg.

It's not just for recursion, they can there to make it rarely possible to being called from outside, and make internal calls in macros.

Like here, I used it, to name and extract a functionality of making comma separated list of items, with the comma_sep name. (And yes, in this case we also have a recursion and an extra rule for the last element.)

And about the new changes, I think we need exactly one of the opentelemetry_* features, no? I'm not sure. If so, then better to use exactly_one_of macro.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's also written here and here and railroad also flags them as internal rules
image
vs
image

Copy link
Owner

@LukeMathWalker LukeMathWalker left a comment

Choose a reason for hiding this comment

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

Thanks!

@LukeMathWalker LukeMathWalker merged commit fe9d543 into LukeMathWalker:main Nov 10, 2023
12 checks passed
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.

None yet

3 participants