-
Notifications
You must be signed in to change notification settings - Fork 506
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
feat: added prefix to look for containerid #2341
feat: added prefix to look for containerid #2341
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2341 +/- ##
==========================================
- Coverage 90.97% 90.44% -0.54%
==========================================
Files 146 150 +4
Lines 7492 7397 -95
Branches 1502 1535 +33
==========================================
- Hits 6816 6690 -126
- Misses 676 707 +31
|
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 have very little knowledge about how containers are stored and formatted in files. added few general suggestions / concerns.
The new tests seems not to cover some of the flows introduced by this PR. Please consider adding more tests.
detectors/node/opentelemetry-resource-detector-container/src/detectors/ContainerDetector.ts
Outdated
Show resolved
Hide resolved
detectors/node/opentelemetry-resource-detector-container/src/detectors/ContainerDetector.ts
Outdated
Show resolved
Hide resolved
detectors/node/opentelemetry-resource-detector-container/src/detectors/ContainerDetector.ts
Outdated
Show resolved
Hide resolved
detectors/node/opentelemetry-resource-detector-container/src/detectors/ContainerDetector.ts
Outdated
Show resolved
Hide resolved
detectors/node/opentelemetry-resource-detector-container/src/detectors/ContainerDetector.ts
Outdated
Show resolved
Hide resolved
detectors/node/opentelemetry-resource-detector-container/src/detectors/ContainerDetector.ts
Show resolved
Hide resolved
detectors/node/opentelemetry-resource-detector-container/src/detectors/ContainerDetector.ts
Outdated
Show resolved
Hide resolved
detectors/node/opentelemetry-resource-detector-container/src/detectors/ContainerDetector.ts
Outdated
Show resolved
Hide resolved
detectors/node/opentelemetry-resource-detector-container/src/detectors/ContainerDetector.ts
Show resolved
Hide resolved
detectors/node/opentelemetry-resource-detector-container/test/ContainerDetector.test.ts
Outdated
Show resolved
Hide resolved
@blumamir Thank you for the review - I have added some comments and added more test cases in utils.test.ts This PR
|
…hee11/opentelemetry-js-contrib into fix/container-detector-prefix
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.
Thank you for working on this and addressing all the comments.
LGTM
Which problem is this PR solving?
-#2339
Short description of the changes