ekump/add config gaps from transport proc #3350
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 2.0 #3350 +/- ##
=======================================
Coverage 98.09% 98.09%
=======================================
Files 1250 1250
Lines 72381 72381
Branches 3382 3382
=======================================
Hits 71000 71000
Misses 1381 1381 ☔ View full report in Codecov by Sentry. |
ekump
requested review from
GustavoCaso and
a team
and removed request for
a team and
GustavoCaso
|
Hey! I'm a bit behind on a few other PRs and tasks, but would like to give a careful pass on this one, since I've written most of the Do you mind holding off on merging before I can give a review? |
@ivoanjo No problem, this doesn't need to be merged ASAP. |
ekump
force-pushed
the
ekump/add-config-gaps-from-transport-proc
branch
from
e73198f
to
70f0a42
Compare
| DetectedConfiguration.new( | ||
| friendly_name: "'c.agent.host'", | ||
| value: settings.agent.host | ||
| ), | ||
| DetectedConfiguration.new( | ||
| friendly_name: "#{Datadog::Core::Configuration::Ext::Agent::ENV_DEFAULT_URL} environment variable", | ||
| value: parsed_http_url && parsed_http_url.hostname | ||
| value: parsed_http_url&.hostname |
| ENV_DEFAULT_USE_SSL = 'DD_TRACE_AGENT_USE_SSL' | ||
| ENV_DEFAULT_TIMEOUT_SECONDS = 'DD_TRACE_AGENT_TIMEOUT_SECONDS' | ||
| ENV_DEFAULT_UDS_PATH = 'DD_TRACE_AGENT_UDS_PATH' |
There was a problem hiding this comment.
Out of curiosity, as these cross-language, or created by us?
There was a problem hiding this comment.
I was going to comment on something similar -- if these are cross language, no concerns at all.
But if they're new and specific to Ruby, I would perhaps hesitate to add them as they're subsets of DD_TRACE_AGENT_URL, which is already supported by all libraries.
In my view, part of the problem with the transport proc started was it wasn't possible to set these values via code, exactly because it wasn't possible to set DD_TRACE_AGENT_URL via code. If you wanted to, you could already set them via env vars.
Thus, I think the new options via code look reasonable, but it's weird to add the new env vars because:
a) It creates inconsistency between client libraries. These env vars look very generic, so I can imagine a customer copy-pasting their existing Ruby config to another service, and then get confused when it doesn't work. (This has happened in the past with other env vars -- customers open an issue reporting "hey I use DD_FOO with python/java etc but it doesn't work for ruby -- help?)
b) It creates more duplication of settings -- having DD_TRACE_AGENT_URL and DD_TRACE_AGENT_UDS_PATH means introducing precedence, etc. And as you can kinda tell from the AgentSettingsResolver, there's already quite a lot of complexity in all this business.
Note that all of this comment is about the env vars -- I think it's fine to have the new settings for code (e.g. I think it's ok to not have a 1:1 correspondence between every code setting and env var; it's definitely not new to our settings)
There was a problem hiding this comment.
@marcotc These are not cross-language, I made them up. Searching through github it looks like I should probably replace DD_TRACE_AGENT_UDS_PATH with DD_TRACE_AGENT_UNIX_DOMAIN_SOCKET. For timeout, Python uses DD_TRACE_AGENT_TIMEOUT_SECONDS but PHP uses DD_TRACE_AGENT_TIMEOUT_VAL.
@ivoanjo I think your points are valid and I'm in favor of simplifying the resolver as much as possible. I also agree that having multiple ways to set options via env var is a bit weird. But I think we may need the SSL and timeout env vars.
- It is possible to set your host via the cross-language vars
DD_TRACE_AGENT_HOSTandDD_TRACE_AGENT_PORTfor an http connection. If a user does this, shouldn't they also be able to set the use of SSL via env var? - You can't set timeout via
DD_TRACE_AGENT_URL. I don't think we want to go down the path of query strings? - The only justification for
DD_TRACE_AGENT_UDS_PATHis consistency with the http functionality. I don't feel strongly, but if we do keep the timeout and ssl env vars, I don't see the incremental added complexity of a UDS path var as a big deal. If we do decide to drop the SSL var and leave only the timeout var, then I think we should drop UDS path.
If we want to adopt a policy of "If you want to configure via ENV vars and you want to either use SSL for http connections or you want to use UDS then you must use DD_TRACE_AGENT_URL" I don't object. I would just want to run it by TEE first to make sure we aren't missing an edge case. (The fact that you can't set parameters today via env var and only through the proc or DD_TRACE_AGENT_URL implies we wouldn't be removing functionality here.)
There was a problem hiding this comment.
It is possible to set your host via the cross-language vars DD_TRACE_AGENT_HOST and DD_TRACE_AGENT_PORT for an http connection. If a user does this, shouldn't they also be able to set the use of SSL via env var?
It's a very good point! I think this is all the result of very organic evolution. Having said that, using ssl is a quite advanced configuration: Afaik the datadog agent doesn't have built-in support for serving ssl. So if you're using ssl, it already means you have some proxying http server in front the agent, which probably means you know what you're doing.
For that reason, I would kinda lean towards letting DD_TRACE_AGENT_URL take care of ssl (and uds).
As for timeout: one one side, we never actually got asked to support it via env, which probably suggests it's not often used. So I would kinda wait for someone to ask.
But, I think it's reasonable if you want to preeempt that and go ahead. In that case, I'd probably suggest following the python convention, just because the PHP one is awful (cmon -- every setting is a value! it's like suffixing things with _ENV_VAR lol).
There was a problem hiding this comment.
@ivoanjo I think that's reasonable. I removed DD_TRACE_AGENT_UDS_PATH and DD_TRACE_AGENT_USE_SSL but left DD_TRACE_AGENT_TIMEOUT_SECONDS as is.
There was a problem hiding this comment.
Thank you so much for the improvements, @ekump!
Co-authored-by: Austin Lai <76412946+alai97@users.noreply.github.com>
Co-authored-by: Austin Lai <76412946+alai97@users.noreply.github.com>
ekump
force-pushed
the
ekump/add-config-gaps-from-transport-proc
branch
from
314a1bc
to
48afca8
Compare
2.0 Upgrade Guide notes
The
c.tracing.transport_optionsoption has been removed. The library can no longer be configured with a custom transport adapter. The default, or test adapter must be used. All agent transport configuration options can be set via theDatadog.configureblock, or environment variables.The following options have been added to replace configuration options previously only available via transport_options:
c.agent.timeout_secondsorDD_TRACE_AGENT_TIMEOUT_SECONDSc.agent.uds_pathorDD_TRACE_AGENT_UDS_PATHc.agent.use_sslorDD_AGENT_USE_SSLSetting uds path or the use of SSL via DD_TRACE_AGENT_URL continues to work as it did in 1.x.
To use the test adapter set
c.agent.tracing.test_mode.enabled = trueWhat does this PR do?
This PR removes the
transport_proc. Users can no longer define custom transporters. All configuration must be done via either env vars, or programmatic configuration. The following options can now be set withouttransport_proc.uds_pathuse_ssl(HTTP only)timeout_secondsAdditionally, product-specific
AgentSettingsResolvershave been removed as there is no added functionality, and the concept of non-core resolvers doesn't really make sense at this time.Motivation:
Simplification of agent communication configuration. No reasonable justification presented to support custom transport procs.
Additional Notes:
How to test the change?
Configure agent communication via newly provided options. Library should function as normal.
For Datadog employees:
credentials of any kind, I've requested a review from
@DataDog/security-design-and-guidance.Unsure? Have a question? Request a review!