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

Report detected/configured_hostname and fix tests #1891

Merged
merged 7 commits into from Aug 28, 2023

Conversation

basepi
Copy link
Contributor

@basepi basepi commented Aug 24, 2023

What does this pull request do?

Removes the deprecated hostname, instead splitting it out into detected_hostname and configured_hostname. Ensure that detected_hostname is the FQDN wherever possible.

Related issues

Closes #1824

@basepi
Copy link
Contributor Author

basepi commented Aug 25, 2023

@elasticmachine, run elasticsearch-ci/docs

elasticapm/base.py Outdated Show resolved Hide resolved
elasticapm/base.py Outdated Show resolved Hide resolved
"""
global fqdn
if not fqdn:
fqdn = socket.getfqdn()
Copy link
Member

Choose a reason for hiding this comment

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

For comparison, here is the Node.js APM agent impl: https://github.com/elastic/apm-agent-nodejs/blob/main/lib/apm-client/http-apm-client/detect-hostname.js#L28-L77
which is almost, but not quite what is spec'd at https://github.com/elastic/apm/blob/main/specs/agents/metadata.md#hostname (I didn't fallback to the HOSTNAME and HOST envvars).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, interesting, you actually shell out as spec'd. I'll have to think on that. It's not very pythonic. 🤔

Copy link
Member

Choose a reason for hiding this comment

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

I don't love shelling out, but all the reading and poking around I did suggested that hostname -f is the authoritative way to get the FQDN on non-Windows systems, and re-implementing hostname -f functionality in Node.js at least wasn't feasible. Then on Windows I followed the spec to exec the powershell thing.

Given that Python has the FQDN-related function, I can understand your wanting to just use that.

Note that I do occasionally get this on my macOS:

>>> import socket
>>> socket.gethostname()
'pink.local'
>>> socket.getfqdn()
'1.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.ip6.arpa'

which is something related to the IPv6 localhost addr and perhaps the entry in /etc/hosts. That I get it only sometimes is weird:

% while true; do sleep 1; python3 -c 'import socket; print(socket.getfqdn())'; done
1.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.ip6.arpa
1.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.ip6.arpa
1.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.ip6.arpa
1.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.ip6.arpa
1.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.ip6.arpa
1.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.ip6.arpa
1.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.ip6.arpa
1.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.ip6.arpa
pink.local
pink.local
pink.local
pink.local
pink.local
pink.local
pink.local
pink.local
pink.local

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a good thing to know. If we run into issues we can re-visit. But for now, I have limited python agent time so I'm going to call it "good enough" :)

elasticapm/conf/__init__.py Outdated Show resolved Hide resolved
@basepi basepi enabled auto-merge (squash) August 28, 2023 21:22
@basepi basepi merged commit e29f762 into elastic:main Aug 28, 2023
96 checks passed
APM-Agents (OLD) automation moved this from In Progress to Done Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

[META 793] Report FQDN for detected_hostname
2 participants