-
Notifications
You must be signed in to change notification settings - Fork 458
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
internal/hostname: Add tracer side hostname detection #1712
Conversation
a72b8c6
to
7050c51
Compare
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'll just leave two small driveby comments for the first couple files as I looked at it. It's nice that most of this is in internal/hostname so it's easy to rip out if we end up not wanting to go this route. And just generally helpful for readability 👍
010acfe
to
8b00c96
Compare
Co-authored-by: Katie Hockman <katie@hockman.dev>
Co-authored-by: Ahmed Mezghani <38987709+ahmed-mez@users.noreply.github.com>
ddtrace/tracer/tracer.go
Outdated
if t.config.disableHostnameDetection { | ||
return hostname.Get() | ||
} | ||
return "" |
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.
if t.config.disableHostnameDetection { | |
return hostname.Get() | |
} | |
return "" | |
if !t.config.disableHostnameDetection { | |
return hostname.Get() | |
} | |
return "" |
It's often easier and less error prone to have this kind of booleans describe enabling the feature. Any reason why we'd prefer disableHostnameDetection
instead of enableHostnameDetection
here ?
Even for consistency, all the existing booleans in the config
struct describe enabling features/options.
What does this PR do?
This adds a new
_dd.tracer_hostname
tag which is filled with the tracer detected hostname when the tracer is NOT connected over UDS. Note that a significant amount of this code is a lightly modified version of the code in the datadog-agent's hostname detection logic. Most of the modifications are to reduce complexity / pick "preferred" defaults that can't exist in the agent for legacy reasons. Additionally, this hostname detector can not detect docker/k8s hostnames as most traced apps do not provide access to those APIs (and they aren't provided by default). The "fromContainer" provider is kept so that if this feature is desired for customers who do manually provide access it can later be implemented in the correct place.Motivation
This will help determine the feasibility of more tracer-level hostname detection for better support of deployments where traced applications do not live on the same host as their agent.
Describe how to test/QA your changes
Manual Testing to be completed in:
Reviewer's Checklist
NOTE! This is a big PR, many files are copied from the datadog agent, primarily with changes made to:
Where this is true, files have a comment at the top indicating where they were taken from and what was changed. Feel free to focus your attention on the ones that are new / more heavily modified. In particular I would focus on the
providers.go
file and the changes in theddtrace/tracer
folder to integrate this new information.Triage
milestone is set.