[NO-TICKET] Add environment variable for most configs

[vpellan]

What does this PR do?

Add environment variable for all configurations that are not proc or objects

Motivation:

This way, these configs could be set through env var, and stable config.

Change log entry

Yes. (Needs to be written as the list of introduced env vars is long)

Additional Notes:

How to test the change?

[github-actions]

Typing analysis

Note: Ignored files are excluded from the next sections.

steep:ignore comments

This PR introduces 1 steep:ignore comment, and clears 1 steep:ignore comment.

steep:ignore comments (+1-1)

❌ Introduced:

lib/datadog/core/configuration/option.rb:189

✅ Cleared:

lib/datadog/core/configuration/option.rb:181

[ivoanjo]

Left some notes. While I think the objective of allowing consistent settings is correct, I think we should be careful about just "ADD MOAR THINGS" -- there's a lot of settings that should probably not even exist or not be exposed so I recommend extreme caution with introducing more things that then in the future will require a API break to get rid of.

[ivoanjo]

For profiling settings under advanced, the ones that don't have env vars are deliberate -- they are things that are exposed only for very specific situations and almost always shouldn't/needn't be touched.

I'd like to keep them like that for now.

[ivoanjo]

This + a few others below are marked as "private" -- I believe the intention is not to expose these settings directly?

[ivoanjo]

I think we actually don't need this one, and should probably deprecate it, rather than introduce a new environment variable.

[ivoanjo]

This one is also for internal use, so I don't think this is correct?

[datadog-official]

⚠️ Tests

✨ Fix all issues with BitsAI or with Cursor

⚠️ Other Violations

🧪 3 Tests failed

Datadog::Core::Telemetry::Event::AppStarted.payload reports instrumented Rails configuration from rspec     (Fix with Cursor)

expected [{name: "DD_GIT_REPOSITORY_URL", origin: "default", seq_id: 1, value: nil}, {name: "DD_GIT_COMMIT_SHA...lue: true}, {name: "DD_TRACE_ACTIVE_SUPPORT_CACHE_STORE", origin: "default", seq_id: 1, value: nil}] to include {name: "tracing.rails.middleware_names", origin: "code", seq_id: 5, value: true}
Diff:
@@ -1,4 +1,635 @@
-:name => "tracing.rails.middleware_names",
-:origin => "code",
-:seq_id => 5,
-:value => true,
+[{name: "DD_GIT_REPOSITORY_URL", origin: "default", seq_id: 1, value: nil},
+ {name: "DD_GIT_COMMIT_SHA", origin: "default", seq_id: 1, value: nil},
+ {name: "DD_AGENT_TRANSPORT", origin: "unknown", seq_id: 7, value: "TCP"},
...

Datadog::Core::Telemetry::Event::AppStarted.payload with default configuration reports default configuration from rspec     (Fix with Cursor)

expected [{:name => "DD_GIT_REPOSITORY_URL", :origin => "default", :seq_id => 1, :value => nil}, {:name => "DD...me => "DD_ERROR_TRACKING_HANDLED_ERRORS_INCLUDE", :origin => "default", :seq_id => 1, :value => ""}] to include {name: "tracing.partial_flush.min_spans_threshold", origin: "default", seq_id: 1, value: 500}
Diff:



@@ -1,13 +1,66 @@
-[{:name => "agent.host", :origin => "default", :seq_id => 1, :value => nil},
- {:name => "DD_ENV", :origin => "default", :seq_id => 1, :value => nil},
- {:name => "DD_TRACE_SAMPLE_RATE",
+[{:name => "DD_GIT_REPOSITORY_URL",
...

Datadog::Core::Telemetry::Event::AppStarted.payload with set configuration reports set configuration from rspec     (Fix with Cursor)

expected [{:name => "DD_GIT_REPOSITORY_URL", :origin => "default", :seq_id => 1, :value => nil}, {:name => "DD...me => "DD_ERROR_TRACKING_HANDLED_ERRORS_INCLUDE", :origin => "default", :seq_id => 1, :value => ""}] to include {name: "logger.level", origin: "code", seq_id: 5, value: 0}
Diff:








...

ℹ️ Info

No other issues found (see more)

❄️ No new flaky tests detected

_{This comment will be updated automatically if new data arrives.
🔗 Commit SHA: be6e782 | Docs | Datadog PR Page | Was this helpful? React with 👍/👎 or give us feedback!}

[vpellan]

I recommend extreme caution with introducing more things that then in the future will require a API break to get rid of

Isn't it already the case even with the config DSL? I'd say it's even worse because setting a non-existing config in the Datadog.configure block would crash the app, while env vars can be non existent but still set by the customer without crashing the app. I'm unsure how adding env vars to existing config could lead to bigger breaking change than currently. But of course the first step of this PR was to add env vars to all configs, then with the review process we can remove some, e.g. internal-only ones, although there is an internal category in the config registry that would fit that purpose

[ivoanjo]

But of course the first step of this PR was to add env vars to all configs, then with the review process we can remove some, e.g. internal-only ones, although there is an internal category in the config registry that would fit that purpose

I guess this is indeed possible path, but... to me is a bit surprising, to be honest? 👀 😅

Adding all env vars before deciding if we need the config at all is kinda in the reverse order of what I'd expect: I'd kinda expect us to go "we don't need this one, let's start deprecating, and we'll keep that one, let's add the env var" and so on... 🤔