Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Capture FQDN in APM agents #805
Capture FQDN in APM agents #805
Changes from all commits
35aa204
368b60f
ee28eb0
f3a61ac
8fb6ddf
1d69c53
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
@SylvainJuge In my testing while implementing for .NET, it seems that this command may return with a period suffix (e.g.
DESKTOP-NCGCCT0.
) or even the suffix.localdomain
. Do we send as-is, or should we consider trimming in either case?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.
@stevejgordon Where did you see an example like
DESKTOP-NCGCCT0.
? Is that an OS other than Windows?I'm not sure if it is equivalent, but on macOS the domain is commonly ".local", e.g. on my laptop:
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.
So I'm testing using WSL2 on Windows with Ubuntu. On my Windows laptop, where no domain is set, it reports the period suffix when I run
hostname -f
from Ubuntu (using WSL2). I also set up a Windows 11 VM to test adding the DNS suffix and again added WSL Ubuntu to test the code and picked up the FQDN, which it did. Once I removed the DNS suffix,.localdomain
started appearing.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.
For .NET, as the primary mechanism, I'm planning to use the
Dns.GetHostEntry(string.Empty).HostName
API, which is cross-platform aware and seems to potentially include the suffix, at least when testing on Ubuntu via WSL2.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.
I think here that's clearly a case where using the API seems to be more consistent than the equivalent with
hostname
.Does it also returns a consistent value when invoked outside of WSL ?
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.
@stevejgordon Assuming I followed the code correctly, Beats are currently using https://github.com/elastic/go-sysinfo/blob/9ea2eba5301c77b511c0475d7137ea3437a55a9a/providers/windows/host_windows.go#L87-L94
(https://learn.microsoft.com/en-us/windows/win32/api/sysinfoapi/nf-sysinfoapi-getcomputernameexa)
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.
@trentm The .NET API eventually seems to call into the native Windows Sockets 2, Winsock API
gethostname
function and on Linux thegethostname
system call.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.
So, if I understand this correctly, you are suggesting to just remove the extra
.
that is appended when calling this API through WSL ?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.
@SylvainJuge Originally, yes, I thought perhaps we should. However, as it's consistent with
hostname -f
and therefore, presumably what other agents on the same host will send, perhaps best to leave it unchanged.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.
@stevejgordon do you think we need to add a clarification about this in the spec or could we leave it as-is ?
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.
As a final nit, I am not implementing this fallback of using the
HOSTNAME
andHOST
envvars in the Node.js APM agent.I couldn't quickly find any references for the
HOST
envvar being set by anything authoritative. TheHOSTNAME
var is set by Bash, which doesn't strike me as an authority we should use. FWIW, this SO post points out thatHOSTNAME
is not an envvar defined by POSIX (https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap08.html).This earlier spec comment was the only discussion around having the
HOSTNAME
andHOST
fallbacks that I could see. I don't know if it originated from what Beats were doing. My understanding is that now beats are using an algorithm that does not use these vars (https://github.com/elastic/beats/blob/c7c18b37eb34f0665f6d9a3b2c7ea4543018ba6a/libbeat/cmd/instance/beat.go#L789-L803 https://github.com/elastic/go-sysinfo/blob/9ea2eba5301c77b511c0475d7137ea3437a55a9a/providers/shared/fqdn.go#L29-L43)Regarding current usage, it looks to me like only the Java and .NET agents are currently looking at the HOSTNAME and HOST envvars. (FWIW. the .NET agent looks at all of HOSTNAME, HOST, and COMPUTERNAME on all platforms). None of the other agents that I saw are using those envvars.
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.
Thanks for the review and feedback @trentm !
I agree with you that those variables might definitely not standard nor available to the runtime as they are usually set by shell and are unlikely to be set on all OSes and distributions. I am assuming (slight speculation on my end) the intent to have them was to provide a "fallback just in case", and that they might not be available most of the time.
However, I don't see any major downside to having them in the spec and using them as fallback, even if it's rarely used in practice. So for now I would suggest we keep them as-is and revisit this decision later if that proves to be an issue or a source of extra complexity.