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

k8sclusterreceiver: migrate to latest semconv version #35238

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

narcis96
Copy link
Contributor

Description: The version of semconv is upgraded from to v1.18.0 to v1.27.0

Link to tracking Issue: #22095

Testing: Tests passed

@narcis96 narcis96 requested a review from a team September 17, 2024 10:38
@github-actions github-actions bot requested a review from povilasv September 17, 2024 10:38
@narcis96 narcis96 force-pushed the narcis/k8sclusterreceiver branch 2 times, most recently from 4558879 to 8d7f4ad Compare September 17, 2024 11:18
@povilasv
Copy link
Contributor

Did you check that k8scluster receiver actually implements semconv 1.27? Are there any code changes needed to follow newest version of semconv?

Copy link
Member

@TylerHelmuth TylerHelmuth left a 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.

@narcis96
Copy link
Contributor Author

narcis96 commented Sep 24, 2024

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.

@narcis96 narcis96 requested a review from a team as a code owner October 2, 2024 09:33
@narcis96
Copy link
Contributor Author

narcis96 commented Oct 2, 2024

hey guys, can you please take a look ? @dmitryax @povilasv @TylerHelmuth

@povilasv
Copy link
Contributor

povilasv commented Oct 2, 2024

Could you elaborate what your tools does? What did you compare? Where did you found no differences?

@narcis96
Copy link
Contributor Author

narcis96 commented Oct 4, 2024

Could you elaborate what your tools does? What did you compare? Where did you found no differences?

@povilasv

The tool checks that all constants from the older semconv version have the same value inside the newer semconv version.
You can check the existing upgrades: #22095

Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Oct 19, 2024
@narcis96
Copy link
Contributor Author

narcis96 commented Oct 24, 2024

did my last reply answered your question ? are there any concerns on this ? @dmitryax @TylerHelmuth @povilasv

@github-actions github-actions bot removed the Stale label Oct 25, 2024
@povilasv
Copy link
Contributor

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?

Copy link
Contributor

github-actions bot commented Nov 9, 2024

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Nov 9, 2024
@ChrsMark
Copy link
Member

Looked for which conventions are used in the component: https://github.com/search?q=repo%3Aopen-telemetry%2Fopentelemetry-collector-contrib+path%3Areceiver%2Fk8sclusterreceiver+conventions.&type=code&ref=advsearch

It appears that resource names and uids are used which I believe have not changed. @povilasv could you double check?

@github-actions github-actions bot removed the Stale label Nov 16, 2024
Copy link
Contributor

github-actions bot commented Dec 1, 2024

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Dec 1, 2024
Copy link
Member

@dmitryax dmitryax left a comment

Choose a reason for hiding this comment

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

There should be no backward incompatible changes for this receiver afaik

@github-actions github-actions bot removed the Stale label Dec 2, 2024
@ChrsMark
Copy link
Member

ChrsMark commented Dec 2, 2024

@narcis96 could you rebase?

@narcis96 narcis96 force-pushed the narcis/k8sclusterreceiver branch from 39a68c2 to 23618c5 Compare December 11, 2024 11:45
@narcis96 narcis96 requested a review from ChrsMark as a code owner December 11, 2024 11:45
@narcis96
Copy link
Contributor Author

@ChrsMark done. PTAL

@TylerHelmuth TylerHelmuth added Skip Changelog PRs that do not require a CHANGELOG.md entry ready to merge Code review completed; ready to merge by maintainers labels Dec 16, 2024
Copy link

codecov bot commented Dec 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.72%. Comparing base (8be3693) to head (8c0e465).

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #35238      +/-   ##
==========================================
- Coverage   79.72%   79.72%   -0.01%     
==========================================
  Files        2218     2218              
  Lines      209150   209150              
==========================================
- Hits       166751   166736      -15     
- Misses      36826    36836      +10     
- Partials     5573     5578       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@TylerHelmuth TylerHelmuth removed the ready to merge Code review completed; ready to merge by maintainers label Dec 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
receiver/k8scluster Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants