Do not assume hostname binary is available #179
Conversation
There was a problem hiding this comment.
Thanks for this contribution @pschipitsch. This is awesome 😄
See my comment regarding your changes.
TLDR; This change doesn't seem to be backward compatible.
| @@ -90,7 +90,7 @@ def record_hostname(hostname) | |||
| end | |||
|
|
|||
| def report() | |||
| hostname = %x[hostname -f].strip | |||
| hostname = Dogapi.find_localhost | |||
lib/dogapi/common.rb
Outdated
| @@ -164,10 +164,9 @@ def Dogapi.find_datadog_host | |||
|
|
|||
| def Dogapi.find_localhost | |||
| begin | |||
| # prefer hostname -f over Socket.gethostname | |||
| @@hostname ||= %x[hostname -f].strip | |||
| @@hostname ||= Addrinfo.getaddrinfo(Socket.gethostname, nil, nil, nil, nil, Socket::AI_CANONNAME).first.canonname | |||
There was a problem hiding this comment.
From https://github.com/DataDog/dogapi-rb it seems that Socket.gethostname is not recommended (apparently it only returns the short name). However, when calling %x[hostname -f].strip and Socket.gethostname on my box, I get the same result. Addrinfo.getaddrinfo(Socket.gethostname, nil, nil, nil, nil, Socket::AI_CANONNAME).first.canonname returns the same value on my box but with upper cases here and there instead. This will introduce a regression since hostnames in Datadog are case sensitive [1]. Datadog is less permissive than the spec [2].
So It seems that using Socket.gethostname is not possible and it seems that your solution would not be backward compatible.
Another option I see is to let the user choose between %x[hostname -f].strip and your solution.
Finally, I am not Ruby expert and I am assuming your solution is the proper way to get the canonical hostname. Still, I have to ask you, is there a shorter/cleaner syntax?
[1] https://docs.datadoghq.com/tagging/assigning_tags/?tab=agentv6#hostname.
[2] https://tools.ietf.org/html/rfc952
There was a problem hiding this comment.
The hostname binary is available in my case. My hostnames are always unique, as well.
It does return the short name, however. Is DogAPI RB expecting the FQDN, including the TLD?
If so, we should allow users to set that themselves.
There was a problem hiding this comment.
Hey, I think the main idea here being that the hostname should be a unique value and avoid clashing with other hosts. For some context, there is a doc about how the Datadog Agent detects the hostname - https://docs.datadoghq.com/agent/faq/how-datadog-agent-determines-the-hostname/?tab=agentv6
To the other point, about staying backwards compatible, one idea could be to leave the hostname -f as the default, and fallback to this kind of approach in the rescue should that error for whatever reason.
There was a problem hiding this comment.
@gzussa Thanks for the feedback! I've updated to try hostname -f and use Addrinfo.getaddrinfo if hostname is not found.
lib/dogapi/common.rb
Outdated
| rescue | ||
| raise 'Cannot determine local hostname via hostname -f' | ||
| raise 'Cannot determine local hostname via Socket.gethostname' |
There was a problem hiding this comment.
While we're here, I think it may be helpful to also provide the message of the original error that caused this to occur. What do you think?
|
@pschipitsch Thanks for making the suggested changes. |
|
@dabcoder The latest error message is kept in the global variable |
|
Thanks @zippolyte. How do we go about building a new gem with this change? |
We run this code in AWS Lambda. AWS recently removed the
hostnamebinary from the execution environment.