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

internal/hostname: Add tracer side hostname detection #1712

Merged
merged 39 commits into from
Feb 21, 2023

Conversation

ajgajg1134
Copy link
Contributor

@ajgajg1134 ajgajg1134 commented Feb 2, 2023

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:

  • GCP
  • Azure
  • EC2
  • Fargate
  • Any Windows machine

Reviewer's Checklist

NOTE! This is a big PR, many files are copied from the datadog agent, primarily with changes made to:

  1. Remove any agent dependencies like config, logging etc.
  2. Remove legacy options / configuration that in new installations doesn't make sense (Similar to what OTEL hostname detection work did)

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 the ddtrace/tracer folder to integrate this new information.

  • If known, an appropriate milestone has been selected; otherwise the Triage milestone is set.
  • Changed code has unit tests for its functionality.
  • If this interacts with the agent in a new way, a system test has been added.

Sorry, something went wrong.

Unverified

This user has not yet uploaded their public signing key.
WIP

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@ajgajg1134 ajgajg1134 added this to the Triage milestone Feb 2, 2023
@pr-commenter
Copy link

pr-commenter bot commented Feb 2, 2023

Benchmarks

Comparing candidate commit e146954 in PR branch andrew.glaude/HostNameResolution with baseline commit 0ef937c in branch main.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 6 cases.

@ajgajg1134 ajgajg1134 force-pushed the andrew.glaude/HostNameResolution branch from a72b8c6 to 7050c51 Compare February 2, 2023 19:13
Copy link
Contributor

@katiehockman katiehockman left a 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 👍

@ajgajg1134 ajgajg1134 force-pushed the andrew.glaude/HostNameResolution branch from 010acfe to 8b00c96 Compare February 3, 2023 20:32
@ajgajg1134 ajgajg1134 marked this pull request as ready for review February 8, 2023 18:21
@ajgajg1134 ajgajg1134 requested a review from a team February 8, 2023 18:21
@ajgajg1134 ajgajg1134 requested a review from a team as a code owner February 8, 2023 18:21
ajgajg1134 and others added 2 commits February 10, 2023 09:56
Co-authored-by: Ahmed Mezghani <38987709+ahmed-mez@users.noreply.github.com>
Comment on lines 629 to 632
if t.config.disableHostnameDetection {
return hostname.Get()
}
return ""
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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.

ahmed-mez
ahmed-mez previously approved these changes Feb 14, 2023
ahmed-mez
ahmed-mez previously approved these changes Feb 15, 2023
@ajgajg1134 ajgajg1134 modified the milestones: Triage, v1.48.0 Feb 15, 2023
@ajgajg1134 ajgajg1134 merged commit 3ccf65d into main Feb 21, 2023
@ajgajg1134 ajgajg1134 deleted the andrew.glaude/HostNameResolution branch February 21, 2023 18:05
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.

None yet

4 participants