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

Updating the metadata hostname specification #517

Merged
merged 8 commits into from
Oct 24, 2021
Merged

Conversation

eyalkoren
Copy link
Contributor

@eyalkoren eyalkoren commented Sep 30, 2021

@elastic/apm-agent-devs we just got a report that the link from APM traces to host metrics may be broken if the hostname discovery is not done as specified hereby, so please take a quick look to see whether such a fix is required in your agent

Agent agent uses always the simple hostname value (no domain) Agent uses the new (not deprecated) intake fields Issue
Java Fixed with elastic/apm-agent-java#2161
.NET elastic/apm-agent-dotnet#1504
Node.js ✅ (os.hostname(), which uses the gethostname syscall)
Python ✅ : (socket.gethostname(), which uses the gethostname syscall)
Ruby ✅ (Socket.gethostname)
Go
PHP ✅ Agent uses PHP function gethostname which is a trivial wrapper around gethostname syscall)

@elastic/apm-agent-devs we just got a report that the link from APM traces to host metrics may be broken if the hostname discovery is not done as specified hereby, so please take a quick look to see whether such a fix is required in your agent
@eyalkoren eyalkoren requested review from a team as code owners September 30, 2021 07:55
@apmmachine
Copy link

apmmachine commented Sep 30, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2021-10-22T04:04:00.405+0000

  • Duration: 3 min 18 sec

  • Commit: 6dc72c8

specs/agents/metadata.md Outdated Show resolved Hide resolved
specs/agents/metadata.md Outdated Show resolved Hide resolved
specs/agents/metadata.md Outdated Show resolved Hide resolved
specs/agents/metadata.md Outdated Show resolved Hide resolved
@eyalkoren
Copy link
Contributor Author

eyalkoren commented Oct 12, 2021

Following the requirement to provide a more comprehensive hostname detection algorithm and make it as compatible as possible with beats, I did some digging with the beats team and changed the spec accordingly.

I don't have enough resources to asses how generic and comprehensive it is, other than seeing it working wherever I tried it. It is certainly not 100% bulletproof, but our goals are more modest for now. Any comments, proposals and enhancements are very welcome.

@elastic/apm-agent-devs - please take a look, even if you did already.

@trentm
Copy link
Member

trentm commented Oct 12, 2021

Is using a language's standard library method for getting the hostname (e.g. socket.gethostname() for Python, os.hostname() for Node.js) acceptable? On unix-y systems these use the gethostname syscall. On Windows (sigh), Node.js and Python use different Windows APIs for this:

Executing uname and possibly hostname will add some cold-startup time for serverless (e.g. Lambda) envs.

@basepi
Copy link
Contributor

basepi commented Oct 12, 2021

@trentm I think the consensus is that the gethostname syscall is acceptable, and doesn't use DNS.

On the Windows end of things, I've been banging the drum for our python agent to drop Windows support anyway because we have no confirmed Windows users. So I'm not stressing about that side, at least for Python.

@eyalkoren
Copy link
Contributor Author

eyalkoren commented Oct 13, 2021

@trentm I think the consensus is that the gethostname syscall is acceptable, and doesn't use DNS.

👍
Avoiding any API that involves NDS lookup is indeed the main point here.

This is tricky indeed. Eventually, what we need is a language-independent spec. That's the only reason I chose to put it this way, but everything that can be done without invoking an external process is of course preferable, as long as it yields the same result.
I will add inline comments saying that equivalent language-specific APIs can be used.
Anyone that wishes to extend that to include concrete examples is very welcome. We may eventually "backport" the spec we come up here to ECS, so any input you put here would be useful.

Lastly, I just learned that the currently proposed algorithm can either return the simple hostname OR FQDN, so I will probably extend the algorithm a little bit to handle that.

@eyalkoren
Copy link
Contributor Author

Added some clarification on what can be used to replace invocation of external commands and extended the algorithm to clear domain parts if the discovered value is a FQDN.

Copy link
Member

@felixbarny felixbarny left a comment

Choose a reason for hiding this comment

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

LGTM

@eyalkoren eyalkoren merged commit 71f6281 into master Oct 24, 2021
@eyalkoren eyalkoren deleted the hostname-spec branch October 24, 2021 10:03
@trentm trentm mentioned this pull request Jun 16, 2023
12 tasks
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.

10 participants