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

feat: added prefix to look for containerid #2341

Merged
merged 8 commits into from
Aug 7, 2024

Conversation

abhee11
Copy link
Contributor

@abhee11 abhee11 commented Jul 17, 2024

Which problem is this PR solving?

-#2339

Short description of the changes

  • adding some prefix check - if they exist to truncate them from the container id string -

@abhee11 abhee11 changed the title feature: added prefix to look for containerid feat: added prefix to look for containerid Jul 17, 2024
Copy link

codecov bot commented Jul 17, 2024

Codecov Report

Attention: Patch coverage is 94.91525% with 3 lines in your changes missing coverage. Please review.

Project coverage is 90.44%. Comparing base (dfb2dff) to head (8216f74).
Report is 203 commits behind head on main.

Files Patch % Lines
...resource-detector-container/src/detectors/utils.ts 93.93% 2 Missing ⚠️
...ector-container/src/detectors/ContainerDetector.ts 96.15% 1 Missing ⚠️
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     
Files Coverage Δ
...ector-container/src/detectors/ContainerDetector.ts 94.91% <96.15%> (+2.05%) ⬆️
...resource-detector-container/src/detectors/utils.ts 93.93% <93.93%> (ø)

... and 67 files with indirect coverage changes

@abhee11
Copy link
Contributor Author

abhee11 commented Jul 18, 2024

@blumamir @rauno56 @dyladan Can I get a review or you can assign it to a relevant person

Copy link
Member

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

@abhee11
Copy link
Contributor Author

abhee11 commented Jul 31, 2024

@blumamir Thank you for the review - I have added some comments and added more test cases in utils.test.ts This PR

  1. Covers more prefixes ( previous was only docker ) this has more coverage.
  2. Added additional hexadecimal checks.
  3. Added suffix check and improved the start and end consideration of what might be containerID
  4. Added more tests to cover new functionality .

Copy link
Member

@blumamir blumamir left a 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

@blumamir blumamir merged commit 1991aed into open-telemetry:main Aug 7, 2024
21 checks passed
@dyladan dyladan mentioned this pull request Aug 7, 2024
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.

2 participants