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

fix: Update defect of of wrong resource attribute of "container.id" #1682

Merged
merged 6 commits into from Sep 19, 2023
Merged

Conversation

liurui-1
Copy link
Contributor

Which problem is this PR solving?

As described in "#1677", the way to get resource attribute of "container.id" is wrong.

Short description of the changes

I made the changes following the code of OTel Java SDK which works well.
Here is the code I followed:
https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/resources/library/src/main/java/io/opentelemetry/instrumentation/resources/ContainerResource.java#L54-L60

I have tested the code working well in my Kubernetes environments.

@liurui-1 liurui-1 requested a review from a team as a code owner September 14, 2023 05:44
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Sep 14, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@liurui-1 liurui-1 changed the title Update defect of of wrong resource attribute of "container.id" fix: Update defect of of wrong resource attribute of "container.id" Sep 14, 2023
@codecov
Copy link

codecov bot commented Sep 14, 2023

Codecov Report

Merging #1682 (f54e732) into main (deb9aa4) will decrease coverage by 0.02%.
The diff coverage is 87.50%.

@@            Coverage Diff             @@
##             main    #1682      +/-   ##
==========================================
- Coverage   91.77%   91.76%   -0.02%     
==========================================
  Files         139      139              
  Lines        7112     7125      +13     
  Branches     1427     1431       +4     
==========================================
+ Hits         6527     6538      +11     
- Misses        585      587       +2     
Files Changed Coverage
...ector-container/src/detectors/ContainerDetector.ts 87.50%

@pichlermarc
Copy link
Member

Thanks for fixing this! 🙂
Looks like the tests still need some attention as their current test data is not very realistic, so the new logic drops them.

Feel free to take the test-data from https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/0f1199620db05a3739cd3bae7e0c408a9bc0d65b/instrumentation/resources/library/src/test/java/io/opentelemetry/instrumentation/resources/CgroupV1ContainerIdExtractorTest.java#L82-L113 and adapt the tests here to use those. 🙂

@liurui-1
Copy link
Contributor Author

liurui-1 commented Sep 16, 2023

Hello @pichlermarc , I updated the UT code. Please review again.

@liurui-1
Copy link
Contributor Author

Hello, I updated the UT code. Please review again.

@pichlermarc pichlermarc linked an issue Sep 18, 2023 that may be closed by this pull request
Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

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

Looks good, just missing some lint fixes it seems 🙂

@liurui-1
Copy link
Contributor Author

@pichlermarc, I updated code according to the lint message. It seems the lint result does not show up automatically. Please help review.

Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

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

Thanks again for fixing this 🙂

@pichlermarc pichlermarc merged commit 5675c49 into open-telemetry:main Sep 19, 2023
14 checks passed
@dyladan dyladan mentioned this pull request Sep 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"container.id" resource attribute is wrong
2 participants