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

ddtrace/tracer: fix UDS/http url configuration and error messages #1604

Merged
merged 16 commits into from
Feb 1, 2023

Conversation

knusbaum
Copy link
Contributor

@knusbaum knusbaum commented Dec 6, 2022

This commit cleans up the configuration of the agent URL so that it reflects the actual URL being used by the Tracer. Previously it would be left as localhost:8162 if the tracer was using UDS, and the error messages would mention localhost:8126 even though it was sending to the agent over unix sockets.

In addition, it cleans up/fixes a few tests associated with this change.

What does this PR do?

Motivation

Describe how to test/QA your changes

Reviewer's Checklist

  • 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.

Verified

This commit was signed with the committer’s verified signature.
rarguelloF Rodrigo Argüello
This commit cleans up the configuration of the agent URL so that it
reflects the actual URL being used by the Tracer. Previously it would be
left as localhost:8162 if the tracer was using UDS, and the error
messages would mention localhost:8126 even though it was sending to the
agent over unix sockets.

In addition, it cleans up/fixes a few tests associated with this change.
@knusbaum knusbaum requested a review from a team December 6, 2022 15:59
@knusbaum knusbaum added this to the Triage milestone Dec 6, 2022
@mackjmr
Copy link
Member

mackjmr commented Dec 7, 2022

@knusbaum should we also update the agent URL when WithUDS is used (https://github.com/DataDog/dd-trace-go/blob/v1.44.0/ddtrace/tracer/option.go#L631-L634) ? Currently this is also left as localhost:8162.

Verified

This commit was signed with the committer’s verified signature.
rarguelloF Rodrigo Argüello
@pr-commenter
Copy link

pr-commenter bot commented Dec 13, 2022

Benchmarks

Comparing candidate commit 0011d93 in PR branch knusbaum/fix-uds-http with baseline commit 648c900 in branch main.

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

Copy link
Contributor

@ajgajg1134 ajgajg1134 left a comment

Choose a reason for hiding this comment

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

Looks good, good 👀 spotting those flipped test assertions.
One question about the logic in newConfig

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@knusbaum
Copy link
Contributor Author

Nice catch, @mackjmr. That should be fixed now.

Verified

This commit was signed with the committer’s verified signature.
rarguelloF Rodrigo Argüello
ajgajg1134
ajgajg1134 previously approved these changes Jan 17, 2023
knusbaum and others added 4 commits January 23, 2023 08:55

Verified

This commit was signed with the committer’s verified signature.
rarguelloF Rodrigo Argüello

Verified

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

Verified

This commit was signed with the committer’s verified signature.
rarguelloF Rodrigo Argüello

Verified

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

Unverified

No user is associated with the committer email.
ajgajg1134
ajgajg1134 previously approved these changes Jan 25, 2023
Copy link
Contributor

@ajgajg1134 ajgajg1134 left a comment

Choose a reason for hiding this comment

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

Approved! with one optional nit

@knusbaum knusbaum removed this from the v1.47.0 milestone Jan 25, 2023
@knusbaum knusbaum added this to the v1.48.0 milestone Jan 25, 2023
ajgajg1134
ajgajg1134 previously approved these changes Jan 25, 2023
@alexstojda
Copy link

Hi! Thanks for this @knusbaum, how soon can we get this merged? :D

@ajgajg1134

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