Add support for Unix Domain Socket (UDS) configuration via DD_TRACE_AGENT_URL
#2806
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…AGENT_URL` **What does this PR do?**: This PR extends the `AgentSettingsResolver` to allow the `DD_TRACE_AGENT_URL` to contain a Unix Domain Socket (UDS) configuration string, for instance `unix:///some/path/to/the/apm.socket`. It also adds a warning if there's configuration for both http and unix domain socket (and in that case it will prioritize the http variant). **Motivation**: This was missing for feature parity with other Datadog client libraries, and it was annoying a lot of customers in #1967 (thanks for the patience, folks!). **Additional Notes**: The change is quite small because dd-trace-rb already supported using unix domain sockets -- you just couldn't configure them via this environment variable. Thus, the only thing that changed was parsing it correctly. **How to test the change?**: Configure an agent with a uds socket and report data through it. Fixes #1967
TonyCTHsu
approved these changes
Apr 24, 2023
marcotc
approved these changes
Apr 24, 2023
|
Hey! I found a tiny tiny bug that wouldn't even impact customers at this point but I want to fix it and add test coverage for it before merging this PR, so I've marked this as do not merge :) |
|
Tiny bug fixed in e7680ed |
marcotc
approved these changes
Apr 25, 2023
|
Thanks y'all for the reviews, merging away! |
ivoanjo
added a commit
that referenced
this pull request
Aug 9, 2023
…figuration **What does this PR do?**: This PR fixes a corner case bug introduced in #2806 when we added support for configuration unix domain socket (UDS) via the `DD_TRACE_AGENT_URL` environment variable. Specifically, when a mix of http and uds configuration was specified, we printed a warning and used the http configuration instead. But, if only the hostname or only the port + uds configuration was provided, we did not correctly use the default hostname and default port as a replacement. That is: * `DD_AGENT_HOST=10.128.186.61 DD_TRACE_AGENT_URL=unix:///var/run/datadog/apm.socket` resulted in `hostname="10.128.186.61", port=nil` * `DD_TRACE_AGENT_PORT=1234 DD_TRACE_AGENT_URL=unix:///var/run/datadog/apm.socket` resulted in `hostname=nil, port=1234` This led to a failure to communicate with the Datadog agent in these situations. **Motivation**: Fix issue when communicating with the agent. **Additional Notes**: We caught this issue while testing internal apps. On the plus side, we did not get any customer reports for this issue, so hopefully nobody was bitten by it. **How to test the change?**: Change includes test coverage. You can also compare the output of the `AgentSettingsResolver` before/after the issue was fixed: ``` # Before $ DD_AGENT_HOST=10.128.186.61 DD_TRACE_AGENT_URL=unix:///var/run/datadog/apm.socket bundle exec ruby -e "require 'ddtrace'; pp(Datadog::Core::Configuration::AgentSettingsResolver.call(Datadog.configuration))" #<struct Datadog::Core::Configuration::AgentSettingsResolver::AgentSettings adapter=:net_http, ssl=false, hostname="10.128.186.61", port=nil, uds_path=nil, timeout_seconds=nil, deprecated_for_removal_transport_configuration_proc=nil> $ DD_TRACE_AGENT_PORT=1234 DD_TRACE_AGENT_URL=unix:///var/run/datadog/apm.socket bundle exec ruby -e "require 'ddtrace'; pp(Datadog::Core::Configuration::AgentSettingsResolver.call(Datadog.configuration))" #<struct Datadog::Core::Configuration::AgentSettingsResolver::AgentSettings adapter=:net_http, ssl=false, hostname=nil, port=1234, uds_path=nil, timeout_seconds=nil, deprecated_for_removal_transport_configuration_proc=nil> # After $ DD_AGENT_HOST=10.128.186.61 DD_TRACE_AGENT_URL=unix:///var/run/datadog/apm.socket bundle exec ruby -e "require 'ddtrace'; pp(Datadog::Core::Configuration::AgentSettingsResolver.call(Datadog.configuration))" #<struct Datadog::Core::Configuration::AgentSettingsResolver::AgentSettings adapter=:net_http, ssl=false, hostname="10.128.186.61", port=8126, uds_path=nil, timeout_seconds=nil, deprecated_for_removal_transport_configuration_proc=nil> $ DD_TRACE_AGENT_PORT=1234 DD_TRACE_AGENT_URL=unix:///var/run/datadog/apm.socket bundle exec ruby -e "require 'ddtrace'; pp(Datadog::Core::Configuration::AgentSettingsResolver.call(Datadog.configuration))" #<struct Datadog::Core::Configuration::AgentSettingsResolver::AgentSettings adapter=:net_http, ssl=false, hostname="127.0.0.1", port=1234, uds_path=nil, timeout_seconds=nil, deprecated_for_removal_transport_configuration_proc=nil> ```
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?:
This PR extends the
AgentSettingsResolverto allow theDD_TRACE_AGENT_URLto contain a Unix Domain Socket (UDS) configuration string, for instanceunix:///some/path/to/the/apm.socket.It also adds a warning if there's configuration for both http and unix domain socket (and in that case it will prioritize the http variant).
Motivation:
This was missing for feature parity with other Datadog client libraries, and it was annoying a lot of customers in #1967 (thanks for the patience, folks!).
Additional Notes:
The change is quite small because dd-trace-rb already supported using unix domain sockets -- you just couldn't configure them via this environment variable. Thus, the only thing that changed was parsing it correctly.
How to test the change?:
Configure an agent with a uds socket and report data through it.
Fixes #1967