ekump/add config gaps from transport proc #3350

ekump · 2023-12-27T21:05:04Z

2.0 Upgrade Guide notes
The c.tracing.transport_options option 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 the Datadog.configure block, or environment variables.

The following options have been added to replace configuration options previously only available via transport_options:

c.agent.timeout_seconds or DD_TRACE_AGENT_TIMEOUT_SECONDS
c.agent.uds_path or ~~DD_TRACE_AGENT_UDS_PATH~~
c.agent.use_ssl or ~~DD_AGENT_USE_SSL~~

Setting 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 = true

What 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 without transport_proc.

uds_path
use_ssl (HTTP only)
timeout_seconds

Additionally, product-specific AgentSettingsResolvers have 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:

If this PR touches code that signs or publishes builds or packages, or handles
credentials of any kind, I've requested a review from @DataDog/security-design-and-guidance.
This PR doesn't touch any of that.

Unsure? Have a question? Request a review!

codecov-commenter · 2023-12-27T21:31:13Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (83a4ec0) 98.09% compared to head (83a4ec0) 98.09%.

❗ Current head 83a4ec0 differs from pull request most recent head 48afca8. Consider uploading reports for the commit 48afca8 to get more accurate results

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.
📢 Have feedback on the report? Share it here.

alai97

Looks good, two copy suggestions!

docs/GettingStarted.md

ivoanjo · 2024-01-04T08:52:21Z

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

Do you mind holding off on merging before I can give a review?

ekump · 2024-01-04T14:22:37Z

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

Do you mind holding off on merging before I can give a review?

@ivoanjo No problem, this doesn't need to be merged ASAP.

marcotc · 2024-01-04T22:53:08Z

lib/datadog/core/configuration/agent_settings_resolver.rb

            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


IS THIS MODERN RUBY???

marcotc · 2024-01-04T22:54:42Z

lib/datadog/core/configuration/ext.rb

+          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'


Out of curiosity, as these cross-language, or created by us?

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)

@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_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?

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_PATH is 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.)

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

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

Thank you so much for the improvements, @ekump!

marcotc

Very nice clean up!

Co-authored-by: Austin Lai <[email protected]>

ekump added the do-not-merge/WIP Not ready for merge label Dec 27, 2023

ekump requested a review from a team as a code owner December 27, 2023 21:05

github-actions bot added the core Involves Datadog core libraries label Dec 27, 2023

ekump marked this pull request as draft December 27, 2023 21:05

ekump removed the request for review from a team December 27, 2023 21:06

github-actions bot added profiling Involves Datadog profiling tracing labels Dec 29, 2023

ekump marked this pull request as ready for review January 3, 2024 19:45

ekump requested review from a team as code owners January 3, 2024 19:45

ekump requested review from GustavoCaso and a team and removed request for a team and GustavoCaso January 3, 2024 19:45

ekump removed the do-not-merge/WIP Not ready for merge label Jan 3, 2024

alai97 approved these changes Jan 3, 2024

View reviewed changes

docs/GettingStarted.md Outdated Show resolved Hide resolved

docs/GettingStarted.md Outdated Show resolved Hide resolved

github-actions bot added the integrations Involves tracing integrations label Jan 4, 2024

ekump force-pushed the ekump/add-config-gaps-from-transport-proc branch from e73198f to 70f0a42 Compare January 4, 2024 17:32

github-actions bot removed the integrations Involves tracing integrations label Jan 4, 2024

marcotc reviewed Jan 4, 2024

View reviewed changes

marcotc approved these changes Jan 4, 2024

View reviewed changes

marcotc approved these changes Jan 5, 2024

View reviewed changes

ekump and others added 14 commits January 12, 2024 16:02

WIP: allow agent timeout to be configurable without transport_proc

ccb17a1

WIP: allow uds_path to be configurable without transport_proc

fba2227

WIP: remove deprecated_for_removal_transport_configuration_proc

03a169a

WIP: fix lint issues

f94499d

WIP: remove product specific agent settings resolvers

56cf5d7

wip remove transport_options proc

39125f7

align env vars to global standards and include "Trace"

f664f8b

update docs for transport_proc removal

43016b7

Update docs/GettingStarted.md

d115769

Co-authored-by: Austin Lai <[email protected]>

Update docs/GettingStarted.md

fe3f753

Co-authored-by: Austin Lai <[email protected]>

remove env vars for uds and ssl

f5f38f0

addressing PR feedback

5398304

update ReadMe to remove references to agent adapters

a4c34d2

wip: remove ext.rb file added back during merge

48afca8

ekump force-pushed the ekump/add-config-gaps-from-transport-proc branch from 314a1bc to 48afca8 Compare January 12, 2024 21:02

github-actions bot removed the integrations Involves tracing integrations label Jan 12, 2024

Update gemfiles/*

5bb619a

ekump merged commit ab5e35e into 2.0 Jan 12, 2024
9 of 19 checks passed

ekump deleted the ekump/add-config-gaps-from-transport-proc branch January 12, 2024 21:39

ivoanjo mentioned this pull request Jan 26, 2024

HTTP adapter renamed to :http #3406

Closed

2 tasks

TonyCTHsu added this to the 2.0 milestone Feb 20, 2024

ivoanjo added the 2.0 label Mar 14, 2024

TonyCTHsu modified the milestones: 2.0, 2.0.0.beta1 Mar 22, 2024

marcotc mentioned this pull request Apr 2, 2024

Rails: Remove circular require #3567

Merged

2 tasks

This was referenced Apr 12, 2024

Bump to version 2.0.0 TonyCTHsu/dd-trace-rb#1

Closed

Bump to version 2.0.0 TonyCTHsu/dd-trace-rb#2

Closed

Update changelog with release 2.0.0.beta2 #3599

Merged

Bump to version 2.0.0.beta2 #3600

Merged

TonyCTHsu mentioned this pull request Jun 6, 2024

Bump to version 2.0.0 #3687

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ekump/add config gaps from transport proc #3350

ekump/add config gaps from transport proc #3350

ekump commented Dec 27, 2023 •

edited

Loading

codecov-commenter commented Dec 27, 2023 •

edited

Loading

alai97 left a comment

ivoanjo commented Jan 4, 2024

ekump commented Jan 4, 2024

marcotc Jan 4, 2024

marcotc Jan 4, 2024

ivoanjo Jan 5, 2024

ekump Jan 5, 2024 •

edited

Loading

ivoanjo Jan 5, 2024

ekump Jan 5, 2024

marcotc Jan 5, 2024

marcotc left a comment

ekump/add config gaps from transport proc #3350

ekump/add config gaps from transport proc #3350

Conversation

ekump commented Dec 27, 2023 • edited Loading

codecov-commenter commented Dec 27, 2023 • edited Loading

Codecov Report

alai97 left a comment

Choose a reason for hiding this comment

ivoanjo commented Jan 4, 2024

ekump commented Jan 4, 2024

marcotc Jan 4, 2024

Choose a reason for hiding this comment

marcotc Jan 4, 2024

Choose a reason for hiding this comment

ivoanjo Jan 5, 2024

Choose a reason for hiding this comment

ekump Jan 5, 2024 • edited Loading

Choose a reason for hiding this comment

ivoanjo Jan 5, 2024

Choose a reason for hiding this comment

ekump Jan 5, 2024

Choose a reason for hiding this comment

marcotc Jan 5, 2024

Choose a reason for hiding this comment

marcotc left a comment

Choose a reason for hiding this comment

ekump commented Dec 27, 2023 •

edited

Loading

codecov-commenter commented Dec 27, 2023 •

edited

Loading

ekump Jan 5, 2024 •

edited

Loading