Skip to content
This repository has been archived by the owner on Aug 30, 2019. It is now read-only.

config: shell out to dd-agent to retrieve hostname #242

Merged
merged 1 commit into from Mar 9, 2017
Merged

Conversation

talwai
Copy link

@talwai talwai commented Mar 9, 2017

The infrastructure agent implements more refined logic for
inferring hostname of the machine this process is running on
https://github.com/DataDog/dd-agent/blob/7cd0986e137683195f129b2a988dd60ca9ccf0ea/utils/hostname.py#L48

Prior to this change, the trace-agent would identify as a different host
than the infra agent in subtle cases (EC2 / GCE for example). This
created phantom hosts and general inconsistency in the UI

We shell out to the infra agent, via the following command, falling back
to running os.Hostname() when it fails

PYTHONPATH=/opt/datadog-agent/agent /opt/datadog-agent/embedded/bin/python -c "from utils.hostname import get_hostname; print get_hostname()"

The infrastructure agent implements more refined logic for
inferring hostname of the machine this process is running on
https://github.com/DataDog/dd-agent/blob/7cd0986e137683195f129b2a988dd60ca9ccf0ea/utils/hostname.py#L48

Prior to this change, the trace-agent would identify as a different host
than the infra agent in subtle cases (EC2 / GCE for example). This
created phantom hosts and general inconsistency in the UI

We shell out to the infra agent, via the following command, falling back
to running `os.Hostname()` when it fails

```
PYTHONPATH=/opt/datadog-agent/agent /opt/datadog-agent/embedded/bin/python -c "from utils.hostname import get_hostname; print get_hostname()"
```
@talwai talwai requested a review from ufoot March 9, 2017 16:54
@LotharSee
Copy link

Can we check with Croissant to know which platform is compatible with that?
And check that it still works well on the others? (including Windows)

Copy link
Member

@ufoot ufoot left a comment

Choose a reason for hiding this comment

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

LGTM !

@@ -149,3 +149,9 @@ func TestConfigNewIfExists(t *testing.T) {
assert.Nil(t, conf)
os.Remove(filename)
}

func TestGetHostname(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

This test is very fine but will probably fall back on os.Hostname in most cases. OTOH I can't see any way to really trigger the "real test" without either black-box test the thing (so, non-Go solution, complex...) or changing the func prototype and passing python and command as args (sounds like over engineering for the sake of testability...). So well, good to go, but will keep in mind this is slightly under tested.

return os.Hostname()
}

hostname := strings.TrimSpace(stdout.String())
Copy link
Member

Choose a reason for hiding this comment

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

👍

@talwai
Copy link
Author

talwai commented Mar 9, 2017

@LotharSee - confirmed that utils.get_hostname() is the canonical source of hostname for all platforms supported by the infra agent, including windows. However the path to the embedded agent Python will differ on Windows so getHostname() will end up defaulting to os.Hostname() anyway.

I've convinced myself this works on the Linux systems we support, and i think pursuing windows support is out of the scope of this PR (since the hostname logic is not the only thing that will need tweaking)

@talwai talwai merged commit d2e71c5 into master Mar 9, 2017
@dtilghman dtilghman deleted the talwai/hostname branch March 10, 2017 05:14
@ufoot ufoot added this to the 5.12 milestone Mar 10, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants