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: detect host id for non-cloud environments #3575

Merged
merged 5 commits into from
Mar 1, 2023

Conversation

mwear
Copy link
Member

@mwear mwear commented Jan 27, 2023

Which problem is this PR solving?

This PR adds host.id resource attribute collection to the HostDetector. The host.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:

Unique host ID. For Cloud, this must be the instance_id assigned by the cloud provider. For non-containerized Linux systems, the machine-id located in /etc/machine-id or /var/lib/dbus/machine-id may be used.

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:

OS Primary Fallback
Linux contents of /etc/machine-id contents of /var/lib/dbus/machine-id
BSD contents of /etc/hostid output of kenv -q smbios.system.uuid
MacOS IOPlatformUUID line from the output of ioreg -rd1 -c "IOPlatformExpertDevice" -
Windows MachineGuid from registry HKEY_LOCAL_MACHINE\\SOFTWARE\\Microsoft\\Cryptography -

Fixes #3574

Short description of the changes

  • Adds ability to collect host-id from non-cloud resources to the HostDetector
  • Provides platform specific host.id collection

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

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

  • Unit tests
  • Manual tests

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added
  • Documentation has been updated

@mwear mwear requested a review from a team as a code owner January 27, 2023 20:30
@codecov
Copy link

codecov bot commented Jan 27, 2023

Codecov Report

Merging #3575 (75322de) into main (79b06a4) will increase coverage by 1.98%.
The diff coverage is 83.75%.

❗ Current head 75322de differs from pull request most recent head 38bd2e6. Consider uploading reports for the commit 38bd2e6 to get more accurate results

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     
Impacted Files Coverage Δ
...urces/src/platform/node/machine-id/getMachineId.ts 35.29% <35.29%> (ø)
...rc/platform/node/machine-id/getMachineId-darwin.ts 92.85% <92.85%> (ø)
...s/src/platform/node/machine-id/getMachineId-win.ts 93.33% <93.33%> (ø)
...ry-resources/src/platform/node/HostDetectorSync.ts 100.00% <100.00%> (ø)
...esources/src/platform/node/machine-id/execAsync.ts 100.00% <100.00%> (ø)
...s/src/platform/node/machine-id/getMachineId-bsd.ts 100.00% <100.00%> (ø)
...src/platform/node/machine-id/getMachineId-linux.ts 100.00% <100.00%> (ø)
.../packages/api-logs/src/platform/node/globalThis.ts 0.00% <0.00%> (-100.00%) ⬇️
...-trace-base/src/platform/node/RandomIdGenerator.ts 87.50% <0.00%> (-6.25%) ⬇️
...ckages/opentelemetry-browser-detector/src/types.ts
... and 121 more

@dyladan
Copy link
Member

dyladan commented Jan 30, 2023

It seems ok, but I have reservations about the dependency:

  • will get pulled in by every package which relies on the semconv package. the implementation appears to be small so maybe this isn't a problem
  • we are relying on an external package to implement the behavior we have specified. it appears to work the same way now, but will it always match our spec?
  • 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?

I would say i'm not strongly against the dependency, but want to make sure we consider it thoroughly

@dyladan
Copy link
Member

dyladan commented Jan 30, 2023

it is also worth mentioning that we have been very hesitant to take any external deps in the past

@mwear
Copy link
Member Author

mwear commented Jan 31, 2023

I have reservations about the dependency

We can write our own (I'll take a stab it in this PR)

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?

We should clarify what the desired behavior is for Windows in the spec. I'll start that effort as well.

@mwear mwear marked this pull request as draft February 1, 2023 23:01
@mwear
Copy link
Member Author

mwear commented Feb 1, 2023

Converting to a draft until the dependency is removed and spec work is underway.

@mwear mwear force-pushed the host-id-resource-attribute branch 2 times, most recently from 2329e8d to c604338 Compare February 10, 2023 20:12
@mwear mwear marked this pull request as ready for review February 10, 2023 21:25
@mwear
Copy link
Member Author

mwear commented Feb 10, 2023

I'm bringing this out of draft as the dependency has been removed and the spec work is in progress. There will be some slight refactoring needed after #3460 merges.

'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;
Copy link
Contributor

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

@legendecas legendecas linked an issue Feb 13, 2023 that may be closed by this pull request
Copy link
Member

@svrnm svrnm left a 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 :-)

@mwear
Copy link
Member Author

mwear commented Feb 15, 2023

I went ahead and rebased to pick up the changes in #3460. HostDetectorSync now has both sync and async attributes.

Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

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

Still LGTM

@mwear
Copy link
Member Author

mwear commented Mar 1, 2023

@pichlermarc pichlermarc merged commit 0d373bd into open-telemetry:main Mar 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consistently record host.id as a resource attribute Add host.id detection to HostDetector
6 participants