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

Sync tracing-opentelemetry and opentelemetry crate versions #170

Open
fabriziosestito opened this issue Oct 3, 2024 · 8 comments
Open

Comments

@fabriziosestito
Copy link

Feature Request

It would be nice if tracing-opentelemetry version was synced with opentelmetry version.
For instance, the fact that tracing-opentelemtry 0.26 depends on opentelemetry 0.25 and not on 0.26 could lead to confusion.

@fabriziosestito fabriziosestito changed the title Sync tracing-opentelemtry and opentelmetry crate versions Sync tracing-opentelemtry and opentelemetry crate versions Oct 3, 2024
@djc
Copy link
Collaborator

djc commented Oct 3, 2024

This is basically impossible until opentelemetry skips some numbers.

@mladedav
Copy link
Contributor

mladedav commented Oct 3, 2024

I'm not advocating for this, but what I think reqwest-tracing does for example is they have a feature per OpenTelemetry version supported by a given release so when a new version comes out, they can just add a feature and do a patch release. Doing a trick like that once would sync the versions but I don't think it's worth the confusion that the migrations would bring.

One good argument against synchronising is that it would prevent this library to make any breaking changes unless OpenTelemetry also releases breaking changes. Not that this is something that happens often, but it's nice that we have the option if needed.

What I think could help would be re-exporting opentelemetry and opentelemetry_sdk and discouraging people from depending on them directly. I'm not sure if we should then also add some of the more often used tracers like otlp behind a feature. And people would still need to depend directly if they needed different features from these than we would provide. Do you think that would be viable and could help?

@collinpa
Copy link

collinpa commented Oct 4, 2024

I was bitten by this today, new to the using these crates. I think a note in the readme would have been useful.

I guess there is already this:

[dependencies]
opentelemetry = "0.21"
opentelemetry_sdk = "0.21"
opentelemetry-stdout = { version = "0.2.0", features = ["trace"] }
tracing = "0.1"
tracing-opentelemetry = "0.22"
tracing-subscriber = "0.3"

but many of the versions didn't match with the current latest so I glossed over it. Updating it to the current latest supported versions might help avoid confusion.

@djc djc changed the title Sync tracing-opentelemtry and opentelemetry crate versions Sync tracing-opentelemetry and opentelemetry crate versions Oct 4, 2024
@djc
Copy link
Collaborator

djc commented Oct 4, 2024

I'm not advocating for this, but what I think reqwest-tracing does for example is they have a feature per OpenTelemetry version supported by a given release so when a new version comes out, they can just add a feature and do a patch release. Doing a trick like that once would sync the versions but I don't think it's worth the confusion that the migrations would bring.

I feel like this crate has more of a 1:1 mapping from the upstream releases to releases of this crate, so I don't think that approach really makes sense here.

One good argument against synchronising is that it would prevent this library to make any breaking changes unless OpenTelemetry also releases breaking changes. Not that this is something that happens often, but it's nice that we have the option if needed.

Yup, I think keeping versions in sync across administrative/organizational boundaries is pretty unlikely. Maybe at some point the functionality of this crate will be subsumed by the upstream project, in which case the calculus here may change.

What I think could help would be re-exporting opentelemetry and opentelemetry_sdk and discouraging people from depending on them directly. I'm not sure if we should then also add some of the more often used tracers like otlp behind a feature. And people would still need to depend directly if they needed different features from these than we would provide. Do you think that would be viable and could help?

I guess it could help but it doesn't feel like the obvious to do thing in this case. That is, I feel like "wrapping" the dependency like that makes sense when a crate is like the main entry point for downstreams to the upstream crates, but not so much when the crate is more like an integration crate matching up two other different (suites of) crates, as is the case here. It feels to me like downstream projects mostly depend on this crate to integrate the opentelemetry and tracing ecosystems, and acting as if tracing-opentelemetry ought to be the focus point for that feels wrong to me.

@jlandahl
Copy link
Contributor

jlandahl commented Oct 7, 2024

I ran into this problem today as well, and it was a quite a challenge to figure out why the compiler was telling me that X doesn't implement trait T when it quite clearly does. It was only when I jumped to the trait source that I (eventually) realized it was for an incompatible version, and the incredible similarity in version numbers meant that I didn't consider that possibility earlier. It's a reasonable assumption for anyone new to the OpenTelemetry ecosystem to think that tracing-opentelemetry 0.26 goes with opentelemetry-* 0.26, so I agree that a note in the README would be really helpful.

I've forked this repo in order to help with this, and was thinking of adding a note early on in the Overview section. Does that seem like a reasonable place for it? The Cargo.toml snippet in the examples is also showing tracing-opentelemetry = "0.22", so that really needs an update, too.

@djc
Copy link
Collaborator

djc commented Oct 8, 2024

Happy to review a PR that adds a note to the README, the Overview section sounds about right. I'd probably prefer to remove the example from the README in favor of keeping it in examples and linking it from the README.

@jlandahl
Copy link
Contributor

jlandahl commented Oct 8, 2024

Happy to review a PR that adds a note to the README, the Overview section sounds about right. I'd probably prefer to remove the example from the README in favor of keeping it in examples and linking it from the README.

Sounds good. I'll have a PR up soon.

jlandahl added a commit to jlandahl/tracing-opentelemetry that referenced this issue Oct 8, 2024
@jlandahl
Copy link
Contributor

jlandahl commented Oct 8, 2024

Happy to review a PR that adds a note to the README, the Overview section sounds about right. I'd probably prefer to remove the example from the README in favor of keeping it in examples and linking it from the README.

PR #171 is up. I decided to add a new section just below the Overview section to make it stand out more. I kept the "basic usage" example, but updated the Cargo.toml section to the latest versions and ensured that it still works.

I also added to the "visualization example" section, including a minimal Cargo.toml example for anyone wanting to use the example code in their own project.

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

No branches or pull requests

5 participants