[NO-TICKET] Convert as much direct env var access to config DSL as possible#5559
[NO-TICKET] Convert as much direct env var access to config DSL as possible#5559
Conversation
Typing analysisIgnored filesThis PR clears 5 ignored files. It increases the percentage of typed files from 45.99% to 46.48% (+0.49%). Ignored files (+0-5)✅ Cleared:Note: Ignored files are excluded from the next sections.
|
BenchmarksBenchmark execution time: 2026-04-08 14:30:17 Comparing candidate commit a8bf43b in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 45 metrics, 1 unstable metrics.
|
|
✅ Tests 🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage (details) 🔗 Commit SHA: a8bf43b | Docs | Datadog PR Page | Was this helpful? React with 👍/👎 or give us feedback! |
| # DEV 3.0: In dd-trace-rs, DD_TRACE_AGENT_URL takes precedence over DD_AGENT_HOST and DD_TRACE_AGENT_PORT | ||
| # Before releasing dd-trace-rb 3.0, we should consider following that logic. | ||
| # (Add the env vars to c.agent.host and c.agent.port, and prioritize parsed_http_url) | ||
| return @configured_hostname if defined?(@configured_hostname) |
There was a problem hiding this comment.
For this part, is there an internal reference for it?
I was hoping that, if we change this, we'd align on a direction that all libraries are actively planning to move towards, rather than "before we were consistent with java, now we're consistent with python" kinda vibe (in particular, I want to avoid ever needing to change this again in the future).
There was a problem hiding this comment.
I've added the reference in the comment. It is in the public documentation here : https://docs.datadoghq.com/tracing/trace_collection/library_config/#agent . There's also internal doc for that but it basically state the same thing.
There was a problem hiding this comment.
Perfect, public docs is the best source for this, ty!
…use the DSL for the env var if needed
d172bf5 to
cb9f2d6
Compare
| options[:statsd] = settings.health_metrics.statsd unless settings.health_metrics.statsd.nil? | ||
|
|
||
| Core::Diagnostics::Health::Metrics.new(telemetry: telemetry, logger: logger, **options) | ||
| Core::Diagnostics::Health::Metrics.new(telemetry: telemetry, port: settings.agent.statsd.port, logger: logger, **options) |
There was a problem hiding this comment.
not sure if this change is related to config DSL; also could settings.agent.statsd be nil?
| settings :statsd do | ||
| # Agent Statsd UDP port. | ||
| # @configure_with {Datadog::Statsd} | ||
| # @default `DD_METRIC_AGENT_PORT` environment variable, otherwise `8125` | ||
| # @return [Integer,nil] | ||
| option :port do |o| | ||
| o.type :int | ||
| o.env Datadog::Core::Configuration::Ext::Metrics::ENV_DEFAULT_PORT | ||
| o.default Datadog::Core::Configuration::Ext::Metrics::DEFAULT_PORT | ||
| end | ||
| end |
There was a problem hiding this comment.
I think this deserves a mention in the changelog
| settings :git do | ||
| # The URL of the git repository. | ||
| # @default `DD_GIT_REPOSITORY_URL` environment variable, otherwise `nil` | ||
| # @return [String,nil] | ||
| option :repository_url do |o| | ||
| o.type :string, nilable: true | ||
| o.env Core::Git::Ext::ENV_REPOSITORY_URL | ||
| # Sensitive informations are filtered before being manually sent through telemetry | ||
| # in core/telemetry/event/app_started.rb | ||
| o.skip_telemetry true | ||
| end | ||
|
|
||
| # The SHA of the git commit. | ||
| # @default `DD_GIT_COMMIT_SHA` environment variable, otherwise `nil` | ||
| # @return [String,nil] | ||
| option :commit_sha do |o| | ||
| o.type :string, nilable: true | ||
| o.env Core::Git::Ext::ENV_COMMIT_SHA | ||
| end | ||
| end |
There was a problem hiding this comment.
I think this also should be mentioned in the changelog? I would probably even add a separate entry for every new config there
|
|
||
| @git_commit_sha = DATADOG_ENV[Datadog::Core::Git::Ext::ENV_COMMIT_SHA] | ||
| def self.git_repository_url(settings) | ||
| Utils::Url.filter_basic_auth(settings.git.repository_url) |
There was a problem hiding this comment.
why is the memoization removed here?
| @git_repository_url = Utils::Url.filter_basic_auth(DATADOG_ENV[Datadog::Core::Git::Ext::ENV_REPOSITORY_URL]) | ||
| end | ||
|
|
||
| def self.git_commit_sha |
There was a problem hiding this comment.
it kinda feels odd that we will be accessing repo url differently from commit sha, just because of this basic auth filter. Why don't we change this method to read commit sha from settings instead?
| attr_reader :statsd, :logger, :telemetry | ||
|
|
||
| def initialize(telemetry:, logger: Datadog.logger, statsd: nil, enabled: true, **_) | ||
| def initialize(telemetry:, port:, logger: Datadog.logger, statsd: nil, enabled: true, **_) |
There was a problem hiding this comment.
this definitely feels like a big breaking change here, even though this method is not marked as public. Is this really necessary to add new mandatory keyword argument here?
| return @git_commit_sha if defined?(@git_commit_sha) | ||
|
|
||
| @git_commit_sha = DATADOG_ENV[Datadog::Core::Git::Ext::ENV_COMMIT_SHA] | ||
| def self.git_repository_url(settings) |
There was a problem hiding this comment.
this method name doesn't seem right now - why do we even need it, when we could call filter_basic_auth directly where needed?
| conf_value( | ||
| 'DD_INJECT_FORCE', | ||
| Core::Environment::VariableHelpers.env_to_bool('DD_INJECT_FORCE', false), | ||
| %w[1 true].include?(DATADOG_ENV.fetch('DD_INJECT_FORCE', 'false').strip.downcase), |
There was a problem hiding this comment.
why are we not using env_to_bool here anymore? could we still have a helper for converting string to boolean?
p-datadog
left a comment
There was a problem hiding this comment.
Two things caught my eye — both with apply-able suggestions.
| Datadog::Core::Diagnostics::Health::Metrics.new( | ||
| logger: logger, | ||
| telemetry: telemetry, | ||
| port: Datadog::Core::Configuration::Ext::Metrics::ENV_DEFAULT_PORT, |
There was a problem hiding this comment.
question (non-blocking): Is this passing the env var name string ('DD_METRIC_AGENT_PORT') instead of the port number?
ENV_DEFAULT_PORT resolves to the string 'DD_METRIC_AGENT_PORT', while DEFAULT_PORT is 8125. It looks like CI doesn't catch it because the test injects a stubbed statsd client, so default_statsd_client (the path that uses @port) is never reached — but I might be reading the test wrong. Worth a look?
| port: Datadog::Core::Configuration::Ext::Metrics::ENV_DEFAULT_PORT, | |
| port: Datadog::Core::Configuration::Ext::Metrics::DEFAULT_PORT, |
| Core.log_deprecation(key: :disable_rails_patching) do | ||
| 'The tracing.contrib.disable_rails_patching option (`DD_DISABLE_DATADOG_RAILS`) is deprecated.' \ | ||
| 'Please remove it from your Datadog.configure block or remove the environment variable' \ | ||
| 'And use `DD_TRACE_RAILS_ENABLED=false` instead.' |
There was a problem hiding this comment.
suggestion (non-blocking): The \ continuation concatenates without spaces, so this produces "...deprecated.Please..." and "...environment variableAnd use...". A small spacing fix would go a long way here.
| 'And use `DD_TRACE_RAILS_ENABLED=false` instead.' | |
| 'The tracing.contrib.disable_rails_patching option (`DD_DISABLE_DATADOG_RAILS`) is deprecated. ' \ | |
| 'Please remove it from your Datadog.configure block or remove the environment variable. ' \ | |
| 'Use `DD_TRACE_RAILS_ENABLED=false` instead.' |
|
|
||
| @git_commit_sha = DATADOG_ENV[Datadog::Core::Git::Ext::ENV_COMMIT_SHA] | ||
| def self.git_repository_url(settings) | ||
| Utils::Url.filter_basic_auth(settings.git.repository_url) |
There was a problem hiding this comment.
question (non-blocking): Does Environment::Git still belong in the Environment namespace?
The Environment modules discover facts about the runtime — env vars, platform, process info — and feed into configuration. After this change, git_repository_url reads from settings rather than from the environment, which reverses that dependency. Environment::Git is now a config consumer rather than a config input.
It works fine as-is, but the name feels like it's doing something different from what the code actually does. Worth thinking about whether this method still lives in the right place?
p-datadog
left a comment
There was a problem hiding this comment.
question (non-blocking): DI probe file loader changes
DD_DYNAMIC_INSTRUMENTATION_PROBE_FILE is an undocumented internal/development env var — it loads probe definitions from a local JSON file instead of remote configuration. It's not something customers would configure, so the Stable Config and Datadog.configure benefits don't really apply here.
The original code deliberately reads the env var directly because the probe file loader runs at boot time, before the normal component lifecycle. The reordering means Datadog::DI.component (which triggers DI component initialization) now runs before checking whether a probe file path is even set. Today this is gated by the env var check in boot.rb:42, but the coupling feels more fragile than the original.
Would it make sense to leave this file as-is and skip the probe_file DSL option? The telemetry visibility benefit feels minimal for an internal-only env var. Happy to be wrong on this — curious if there's a reason I'm not seeing for including it.
buraizu
left a comment
There was a problem hiding this comment.
Approving with one update request for description consistency (and also subject-verb agreement).
|
|
||
| This allows you to keep important spans when trace-level sampling is applied. Is is not possible to drop spans using Single Span Sampling. | ||
|
|
||
| You can provide single span sampling rules inline with `tracing.sampling.span_rules`, or configure `tracing.sampling.span_rules_file` with a path to a file containing the rules. When both are configured, `tracing.sampling.span_rules` takes precedence. |
There was a problem hiding this comment.
| You can provide single span sampling rules inline with `tracing.sampling.span_rules`, or configure `tracing.sampling.span_rules_file` with a path to a file containing the rules. When both are configured, `tracing.sampling.span_rules` takes precedence. | |
| You can provide single span sampling rules inline with `tracing.sampling.span_rules`, or configure `tracing.sampling.span_rules_file` with a path to a file containing the rules. When both are configured, inline rules take precedence. |
| 'Both DD_SPAN_SAMPLING_RULES and DD_SPAN_SAMPLING_RULES_FILE were provided: only ' \ | ||
| 'DD_SPAN_SAMPLING_RULES will be used. Please do not provide DD_SPAN_SAMPLING_RULES_FILE when ' \ | ||
| 'also providing DD_SPAN_SAMPLING_RULES as their configuration conflicts. ' \ | ||
| "DD_SPAN_SAMPLING_RULES_FILE=#{span_rules_file} DD_SPAN_SAMPLING_RULES=#{value}" |
There was a problem hiding this comment.
span_rules_file references the contents of the file (and not the file name). So this log message will not show the value set in DD_SPAN_SAMPLING_RULES_FILE. Is this expected?
What does this PR do?
This PR converts as much configurations that are accessed directly through environment variable to our config DSL as possible. Some cannot be converted (env vars accessed before initializing the config DSL, or when booting a single piece of the tracer such as DI or AppSec)
Motivation:
By doing this, customers will not only be able to configure them through
Datadog.configureblocks, but also through Stable Config. These configs will also be correctly reported through telemetry thanks to the work done for config visibility.Change log entry
Yes. Enable the configuration of
DD_GIT_REPOSITORY_URL,DD_GIT_COMMIT_SHA,DD_TRACE_AGENT_URL,DD_TRACE_AGENT_TIMEOUT_SECONDS,DD_METRIC_AGENT_PORTandDD_DYNAMIC_INSTRUMENTATION_PROBE_FILEthroughDatadog.configureblocks and in the Datadog UIAdditional Notes:
I'll open a PR on datadog-ci-rb so that
DD_GIT_REPOSITORY_URL,DD_GIT_COMMIT_SHAare supported.How to test the change?