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

Inline incubating semconv attributes #1604

Merged
merged 1 commit into from
Dec 19, 2024

Conversation

trask
Copy link
Member

@trask trask commented Dec 13, 2024

Follow-up to @laurit's #1585 (comment)

@trask trask requested a review from a team as a code owner December 13, 2024 21:38
@github-actions github-actions bot requested review from srprash and wangzlei December 13, 2024 21:38
Copy link
Contributor

@breedx-splk breedx-splk left a comment

Choose a reason for hiding this comment

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

I think I must have missed something in the comment...why is it desirable to copy/duplicate these constants?

@trask
Copy link
Member Author

trask commented Dec 14, 2024

https://github.com/open-telemetry/semantic-conventions-java?tab=readme-ov-file#published-releases

This artifact has the -alpha and comes with no compatibility guarantees. Libraries can use this for testing, but should make copies of the attributes to avoid possible runtime errors from version conflicts.

@breedx-splk
Copy link
Contributor

https://github.com/open-telemetry/semantic-conventions-java?tab=readme-ov-file#published-releases

This artifact has the -alpha and comes with no compatibility guarantees. Libraries can use this for testing, but should make copies of the attributes to avoid possible runtime errors from version conflicts.

So the entire purpose here is to avoid taking a dependency on an artifact with -alpha in the suffix? I'd rather do that than have copies of the values that could deviate from the semconv over time....

Maybe we figure that the tests (which use the -alpha) give us enough confidence?

@trask
Copy link
Member Author

trask commented Dec 19, 2024

we're worried about diamond dependency problem where some users will end up depending (transitively) on different irreconcilable versions of the semconv alpha artifact

Maybe we figure that the tests (which use the -alpha) give us enough confidence?

yeah, though it would be interesting to validate this

@laurit
Copy link
Contributor

laurit commented Dec 19, 2024

The guidance given in https://github.com/open-telemetry/semantic-conventions-java/blob/main/README.md states that

Libraries can use this for testing, but should make copies of the attributes to avoid possible runtime errors from version conflicts.

@trask
Copy link
Member Author

trask commented Dec 19, 2024

@breedx-splk I'm going to merge since this is the (current) convention in this repo, but we can revisit

@trask trask merged commit 16222fe into open-telemetry:main Dec 19, 2024
14 checks passed
@trask trask deleted the inline-incubating-attributes branch December 19, 2024 15:22
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.

5 participants