-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
k8sclusterreceiver: migrate to latest semconv version #35238
base: main
Are you sure you want to change the base?
Conversation
8218f28
to
4558879
Compare
4558879
to
8d7f4ad
Compare
Did you check that k8scluster receiver actually implements semconv 1.27? Are there any code changes needed to follow newest version of semconv? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Upgrading semconv is risky. Please confirm that no metrics or attributes emitted by this component change bc of this upgrade.
I think that it actually has 0 risk. The semconv attributes' value have been compared using go-otel-semconv-comparator resulting in 0 diffs. |
hey guys, can you please take a look ? @dmitryax @povilasv @TylerHelmuth |
Could you elaborate what your tools does? What did you compare? Where did you found no differences? |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
did my last reply answered your question ? are there any concerns on this ? @dmitryax @TylerHelmuth @povilasv |
I am not really sure what you are comparing, and if it's good enought? As #35238 (review) was written previously, you should check what this component emits vs what is specified in the latest semconv. Are there any attributes that need changing? |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
Description: The version of semconv is upgraded from to v1.18.0 to v1.27.0
Link to tracking Issue: #22095
Testing: Tests passed