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

[system detector] Fallback to os.Hostname when FQDN is not available #3099

Merged

Conversation

mx-psi
Copy link
Member

@mx-psi mx-psi commented Apr 14, 2021

Description:

  • Fall back to os.Hostname when FQDN is not available.

Link to tracking Issue: Fixes #3092

Testing: Updated unit tests

@mx-psi mx-psi marked this pull request as ready for review April 14, 2021 09:01
@mx-psi mx-psi requested a review from a team April 14, 2021 09:01
@codecov
Copy link

codecov bot commented Apr 14, 2021

Codecov Report

Merging #3099 (1030b04) into main (e9f085b) will increase coverage by 0.01%.
The diff coverage is 87.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3099      +/-   ##
==========================================
+ Coverage   91.64%   91.65%   +0.01%     
==========================================
  Files         482      482              
  Lines       23381    23385       +4     
==========================================
+ Hits        21427    21434       +7     
+ Misses       1454     1449       -5     
- Partials      500      502       +2     
Flag Coverage Δ
integration 63.30% <ø> (ø)
unit 90.65% <87.50%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...urcedetectionprocessor/internal/system/metadata.go 0.00% <0.00%> (ø)
...sourcedetectionprocessor/internal/system/system.go 100.00% <100.00%> (ø)
receiver/k8sclusterreceiver/watcher.go 95.29% <0.00%> (-2.36%) ⬇️
processor/groupbytraceprocessor/event.go 95.96% <0.00%> (-0.81%) ⬇️
exporter/datadogexporter/metadata/metadata.go 90.14% <0.00%> (+5.63%) ⬆️
internal/stanza/receiver.go 100.00% <0.00%> (+5.88%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e9f085b...1030b04. Read the comment docs.

@pmcollins
Copy link
Member

This change is fine for a short-term fix IMO, but longer term, what do you think about moving away from the fqdn third party library we're using, and coding the fqdn resolution ourselves? I say this because the library is small, but it doesn't seem to have the fallback behavior we want.

@mx-psi
Copy link
Member Author

mx-psi commented Apr 14, 2021

what do you think about moving away from the fqdn third party library we're using, and coding the fqdn resolution ourselves? I say this because the library is small, but it doesn't seem to have the fallback behavior we want.

What is the fallback behavior we want? I think we don't want to fall back to os.Hostname when we can't get a FQDN (through the library or otherwise) on the FQDN method. The FQDN and the OS hostname are different concepts and if e.g. we introduce a host.fqdn attribute we would want to be able to distinguish between them. Maybe you are talking about a different fallback behavior?

@pmcollins
Copy link
Member

pmcollins commented Apr 14, 2021

what do you think about moving away from the fqdn third party library we're using, and coding the fqdn resolution ourselves? I say this because the library is small, but it doesn't seem to have the fallback behavior we want.

What is the fallback behavior we want? I think we don't want to fall back to os.Hostname when we can't get a FQDN (through the library or otherwise) on the FQDN method. The FQDN and the OS hostname are different concepts and if e.g. we introduce a host.fqdn attribute we would want to be able to distinguish between them. Maybe you are talking about a different fallback behavior?

Yeah, that's the fallback behavior I was thinking of. The fact that the library gets the hostname then tries to get the fqdn, and if that fails, we get the hostname seemed a little redundant. But your explanation makes sense. Thanks.

Copy link
Member

@dmitryax dmitryax left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for fixing it!

@tigrannajaryan tigrannajaryan merged commit a067bf9 into open-telemetry:main Apr 14, 2021
pmatyjasek-sumo pushed a commit to pmatyjasek-sumo/opentelemetry-collector-contrib that referenced this pull request Apr 28, 2021
open-telemetry#3099)

- Fall back to `os.Hostname` when FQDN is not available.

**Link to tracking Issue:** Fixes open-telemetry#3092
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.

Collector fails to start when system detector cannot obtain FQDN of the host
5 participants