feat: add detector for container ID in cgroup v1 #515
Conversation
rauno56
changed the title
Codecov Report
@@ Coverage Diff @@
## main #515 +/- ##
==========================================
- Coverage 88.30% 87.83% -0.48%
==========================================
Files 26 27 +1
Lines 804 830 +26
Branches 183 187 +4
==========================================
+ Hits 710 729 +19
- Misses 94 101 +7
Continue to review full report at Codecov.
|
|
@seemk, this is ready for another look. Matches Java(synced with Profiling PM about this) - best option for now. The test failure seems unrelated. Let's see if that's just flaky - rerunning. |
| } catch (e) { | ||
| if (e instanceof Error) { | ||
| const errorMessage = e.message; | ||
| diag.info( |
There was a problem hiding this comment.
Wouldn't this mean logging this error on other platforms besides Linux?
There was a problem hiding this comment.
Yes, with the current implementation it would. Your suggestion?
How about we detect the platform and only call _getContainerId() if it's linux?
There was a problem hiding this comment.
I guess the whole detector could check for process.platform === 'linux'
There was a problem hiding this comment.
Not sure what you mean by "whole". Since it's currently the only platform-specific detector, added it into the detector itself. Once we have more, we could do it in resource.ts to optimize.
|
@seemk, PTAL. |
| identifier for differentiating between containers. | ||
| It also matches Java: https://github.com/open-telemetry/opentelemetry-java/commit/2cb461d4aef16f1ac1c5e67edc2fb41f90ed96a3#diff-ad68bc34d4da31a50709591d4b7735f88c008be7ed1fc325c6367dd9df033452 | ||
| */ | ||
| protected _parseFile(contents: string): string | null { |
There was a problem hiding this comment.
Not important / nit, but _parseFile does not actually parse a file, but a container id? 💭
Description
This only works when cgroup v1 is used.
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Checklist:
npm run change:new)