Fix default hostname/port not being used when mixing http and uds configuration + improve warning message

[ivoanjo]

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.

Also included are a few tweaks to the warning message generated + tests for it. See individual commits for detail.

I've tested the fix in the KTG and can confirm data is showing up again for "candidate" releases.

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>

[marcotc]

From technical point of view, this PR improves greatly the internal consistent of the agent resolver.
We are discussing internally if DD_TRACE_AGENT_URL should override both DD_AGENT_HOST and DD_TRACE_AGENT_PORT instead though.

[TonyCTHsu]

Thanks for catching this!

[ivoanjo]

Replying to the DD_TRACE_AGENT_URL concern, the current behavior is to prioritize it, but yeah that doesn't apply in this corner case.

Specifically, it's only in this bizarre corner case where conflicting configuration gets added (supplying both http and uds configuration) that there's a divergence.

The divergence right now is a bit more subtle, because it's not about DD_TRACE_AGENT_URL being downgraded vs DD_AGENT_HOST/DD_TRACE_AGENT_PORT. Instead, currently any configuration for http (including via code) mixed with uds via DD_TRACE_AGENT_URL is prioritized.

Thus, setting Datadog.configure { |c| c.agent.host = 'foo' } and DD_TRACE_AGENT_URL with uds means giving priority to the via-code configuration.

Usually, so far, we've been giving priority to any settings via code. So for this case, we'd either deprioritize settings-via-code vs a uds DD_TRACE_AGENT_URL env (:cry:) or we need to add extra complexity in this case to understand if the conflicting hostname/port is coming via env var or via code (as if our logic for this wasn't kinda complex :cry: ).

Since we've talked about this a bit via slack, and Tony approved this, and this PR is preventing us from seeing data on some of the test apps, I'm going to go ahead and press the merge button.

If we do decide to go in a separate direction, I'm perfectly happy with a revert -- my intention is getting it to work, so any way we can do it is fine by me :)