-
Notifications
You must be signed in to change notification settings - Fork 819
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: detect host id for non-cloud environments #3575
feat: detect host id for non-cloud environments #3575
Conversation
27d86aa
to
89544cb
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #3575 +/- ##
==========================================
+ Coverage 91.74% 93.73% +1.98%
==========================================
Files 161 250 +89
Lines 3915 6206 +2291
Branches 789 1242 +453
==========================================
+ Hits 3592 5817 +2225
- Misses 323 389 +66
|
It seems ok, but I have reservations about the dependency:
I would say i'm not strongly against the dependency, but want to make sure we consider it thoroughly |
it is also worth mentioning that we have been very hesitant to take any external deps in the past |
I have reservations about the dependency
Win32/64 uses key MachineGuid in registry HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Cryptography which is not specified by OTel. what if OTel specs a different windows host id method?
|
Converting to a draft until the dependency is removed and spec work is underway. |
2329e8d
to
c604338
Compare
'QUERY HKEY_LOCAL_MACHINE\\SOFTWARE\\Microsoft\\Cryptography /v MachineGuid'; | ||
let command = '%windir%\\System32\\REG.exe'; | ||
if (process.arch === 'ia32' && 'PROCESSOR_ARCHITEW6432' in process.env) { | ||
command = '%windir%\\sysnative\\cmd.exe /c ' + command; |
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.
tagging @hectorhdzg @JacksonWeber for visibility and any comments
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.
great work, love it :-)
c604338
to
3b0e865
Compare
I went ahead and rebased to pick up the changes in #3460. |
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.
Still LGTM
The spec PR has merged: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/resource/semantic_conventions/host.md#collecting-hostid-from-non-containerized-systems. Let me know if there is anything outstanding. |
Which problem is this PR solving?
This PR adds
host.id
resource attribute collection to the HostDetector. Thehost.id
is currently collected by the cloud provider Detectors (AWS EC2, GCP, etc), but not collected for non-cloud environments. See the spec and description (copied) below:This PR includes code to collect the
host.id
based on this pending spec work: open-telemetry/opentelemetry-specification#3173.The tl;dr is that the following sources should be used to collect the
host.id
based on platform:/etc/machine-id
/var/lib/dbus/machine-id
/etc/hostid
kenv -q smbios.system.uuid
IOPlatformUUID
line from the output ofioreg -rd1 -c "IOPlatformExpertDevice"
MachineGuid
from registryHKEY_LOCAL_MACHINE\\SOFTWARE\\Microsoft\\Cryptography
Fixes #3574
Short description of the changes
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Checklist: