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

Add a host resource detector #5399

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

pyohannes
Copy link
Contributor

Fixes #5398

This PR adds a resource detector for physical hosts which sets values according to semantic conventions for host resources:

  • host.arch
  • host.id
  • host.name
  • host.ip
  • host.mac

@pyohannes pyohannes requested a review from a team April 16, 2024 16:06
Copy link

codecov bot commented Apr 16, 2024

Codecov Report

Attention: Patch coverage is 93.84615% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 62.5%. Comparing base (30ed923) to head (f585990).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #5399     +/-   ##
=======================================
+ Coverage   62.3%   62.5%   +0.1%     
=======================================
  Files        189     191      +2     
  Lines      11575   11640     +65     
=======================================
+ Hits        7221    7282     +61     
- Misses      4145    4147      +2     
- Partials     209     211      +2     
Files Coverage Δ
detectors/host/host.go 96.6% <96.6%> (ø)
detectors/host/host_id_linux.go 60.0% <60.0%> (ø)

@MrAlias
Copy link
Contributor

MrAlias commented Apr 16, 2024

This seems duplicative of WithHost. Why not update that option?

@pyohannes
Copy link
Contributor Author

This seems duplicative of WithHost. Why not update that option?

Indeed it is duplicative.

@MrAlias Do you think it a feasible option to populate host.arch via WithHost, and add additional WithHostIP and WithHostMAC to opt into host.ip and host.mac (all implemented in the SDK)?

@MrAlias
Copy link
Contributor

MrAlias commented Apr 17, 2024

Taking a look at the host semantic conventions, it looks like they are all experimental (even host.name 😬).

I think in the long term we want to make the WithHost option include all the recommended/required stable semantic conventions for a host. But in the interim, having a detector here is probably the correct approach.

I think this PR is the correct approach. I'll plan to review.

Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

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

.github/dependabot.yml Outdated Show resolved Hide resolved
detectors/host/host.go Outdated Show resolved Hide resolved
detectors/host/host.go Outdated Show resolved Hide resolved
detectors/host/host.go Outdated Show resolved Hide resolved
detectors/host/host.go Outdated Show resolved Hide resolved
detectors/host/host_test.go Outdated Show resolved Hide resolved
detectors/host/host_test.go Outdated Show resolved Hide resolved
detectors/host/host_test.go Outdated Show resolved Hide resolved

assert.True(t, err == nil)

hostName, _ := os.Hostname()
Copy link
Contributor

Choose a reason for hiding this comment

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

This error should not be ignored. The test should be failed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I adapted the tests so that the host.name is only added to the list of expected attributes when os.Hostname() doesn't return an error.

This covers cases where os.Hostname() returns an error, but a host.id might still be obtained.

I'm happy to both make the tests and detector logic fail when os.Hostname() fails, if folks think that's better.

detectors/host/host_id_windows.go Outdated Show resolved Hide resolved
pyohannes and others added 14 commits April 22, 2024 13:35
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
@pyohannes
Copy link
Contributor Author

Missing support for BSD: https://github.com/open-telemetry/opentelemetry-go/blob/main/sdk/resource/host_id_bsd.go
Missing fallback when the host system is not supported for the host ID: https://github.com/open-telemetry/opentelemetry-go/blob/main/sdk/resource/host_id_unsupported.go

Thanks @MrAlias, this should be ready for another review.

@dashpole
Copy link
Contributor

But in the interim, having a detector here is probably the correct approach.

@MrAlias Should we use feature environment variables instead of a new detector module?

@MrAlias
Copy link
Contributor

MrAlias commented May 1, 2024

But in the interim, having a detector here is probably the correct approach.

@MrAlias Should we use feature environment variables instead of a new detector module?

That's a good idea.

@MrAlias
Copy link
Contributor

MrAlias commented May 1, 2024

But in the interim, having a detector here is probably the correct approach.

@MrAlias Should we use feature environment variables instead of a new detector module?

That's a good idea.

I've added this to the SIG meeting agenda to discuss.

@MrAlias
Copy link
Contributor

MrAlias commented Jun 26, 2024

@pyohannes can this be closed? We plan to move this into this (behind the experimental envar), right?

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.

Add a resource detector for physical hosts
3 participants