-
Notifications
You must be signed in to change notification settings - Fork 3.3k
[processor/k8sattributes] Fix k8s.node.uid extraction when node.name is disabled #45328
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
base: main
Are you sure you want to change the base?
[processor/k8sattributes] Fix k8s.node.uid extraction when node.name is disabled #45328
Conversation
|
Welcome, contributor! Thank you for your contribution to opentelemetry-collector-contrib. Important reminders:
A maintainer will review your pull request soon. Thank you for helping make OpenTelemetry better! |
7da0e28 to
0941c3c
Compare
4677ddd to
e5ad2c6
Compare
5e4dce6 to
b41072a
Compare
ChrsMark
left a comment
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.
Please remove the unnecessary additions. Otherwise it looks good.
9f25840 to
224c042
Compare
4f2481b to
231e614
Compare
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.
Sth looks weird.
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.
Thanks for the catch @ChrsMark. I realized the issue was that I was initializing the client (c) outside the loop, which caused shared state between the test cases.
I’ve moved the client initialization inside t.Run so each test case starts with a clean state. I also removed the commented-out code and ran make fmt. Ready for another look!
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.
I don't see why we need to change the existing tests for this patch.
The current state of the PR is incomplete since the actual fix has been reverted since I approved the PR.
Please ensure that the PR is actually ready for review with only the required/necessary changes implemented.
231e614 to
27bdfdb
Compare
Description:
Fixes #43865. Previously,
k8s.node.uidwas only added ifk8s.node.namewas also requested in the configuration. This PR decouples the logic so UID can be fetched independently.Testing:
Added a unit test case validating
k8s.node.uidis present even whenk8s.node.nameis omitted.Credit:
Logic based on investigation by @LeonLow97.