-
Notifications
You must be signed in to change notification settings - Fork 843
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
Is it OK for stable trace SDK to have an implementation dependency on alpha artifacts (metrics, semconv)? #2914
Comments
I was going to say it's ok since we require the bom but realized the bom doesn't help with alpha modules. I suggested removing them before but @jkwatson and @bogdandrutu definitely wanted to keep them and I do understand they're pretty useful metrics. |
Tangential question: If someone uses only the release BOM but not the alpha BOM, how will gradle resolve the version of the metrics API for the tracing SDK's dependency? |
It'll be what's in the pom like any transitive dependency. But since it's not dependency management if another dependency also uses metrics transitively it can end up a different version. |
I think this is 100% acceptable. We're not exposing anything related to metrics on the APIs of the SDK. This is just like if we had a dependency on any library that didn't have the same stability guarantees as our own SDK. changing the internal usage of metrics will not break ABI or API compatibility with anything in the SDK. It is only a potential dependency management issue, which is fine, IMO. |
Since it's only an implementation dependency, I think I agree. Would it still be worth a comment in the gradle file? Like |
I think this ship has sailed. closing. |
The trace SDK is stable but depends on the metric API which is in alpha: https://github.com/open-telemetry/opentelemetry-java/blob/v0.17.1/sdk/trace/build.gradle.kts#L17
It does this to support some internal metrics for the BatchSpanProcessor.
Note that the exposure of internal metrics is of course also not specified yet, see open-telemetry/opentelemetry-specification#959.
I would vote for removing any metric dependencies from the tracing SDK for now. But if not, I think at least a comment is warranted in the gradle file.
The text was updated successfully, but these errors were encountered: