fix(config): fix proxy configuration for nested YAML, standard env vars, and case-insensitive matching#1282
Conversation
d9a7c87 to
fb329b0
Compare
| // Set under both the raw name and the TEST_ prefix: | ||
| // - Raw name: read by StandardProxyEnvProvider (e.g. HTTP_PROXY) | ||
| // - TEST_ prefix: read by from_environment("TEST") (simulates DD_ prefix) | ||
| std::env::set_var(k, v); |
There was a problem hiding this comment.
This is janky, but not sure if making it less janky was worth changing the function signature here. Could do so in a quick follow-up PR if you'd prefer
There was a problem hiding this comment.
Testing out the querying of environment variables is very janky when having to drop down to std::env::set_var. I wouldn't worry about it for right now.
Overall, we just need to spend some time adding a way to mock out environment variables in a scoped / not-process-global fashion.
There was a problem hiding this comment.
Is there any expectation of ADP responding to changes in env vars during the process lifetime? If not we could snapshot it at instantiation and just treat it like a map, I'd be happy to take a look
There was a problem hiding this comment.
No, environment variables changing at runtime isn't a thing that any sane programmer would attempt. It's fraught with peril.
| if let Some(pairs) = env_vars.as_ref() { | ||
| for (k, _) in pairs.iter() { | ||
| std::env::remove_var(k); | ||
| std::env::remove_var(format!("TEST_{}", k)); |
There was a problem hiding this comment.
This is a driveby fix for an existing bug, surfaced by the unit tests in proxy.rs. This flow adds the env vars with the TEST_ prefix but wasn't applying it when it tried to remove them
Binary Size Analysis (Agent Data Plane)Target: 71ad7e9 (baseline) vs e6339cc (comparison) diff
|
| Module | File Size | Symbols |
|---|---|---|
saluki_components::config::DatadogRemapper |
+4.63 KiB | 3 |
saluki_config::ConfigurationLoader::from_yaml |
+3.68 KiB | 2 |
[Unmapped] |
-3.54 KiB | 1 |
core |
+3.45 KiB | 6158 |
serde_json |
+2.83 KiB | 160 |
serde_yaml |
+1.57 KiB | 38 |
[sections] |
+1.32 KiB | 7 |
saluki_config::ConfigurationLoader::add_providers |
+805 B | 1 |
saluki_components::sources::otlp |
-569 B | 164 |
saluki_config::ConfigurationLoader::from_environment |
+370 B | 2 |
agent_data_plane::cli::run |
-351 B | 70 |
saluki_io::net::server |
+189 B | 8 |
std |
-181 B | 141 |
saluki_components::transforms::aggregate |
-160 B | 98 |
saluki_app::api::APIBuilder |
-133 B | 2 |
saluki_components::forwarders::otlp |
-120 B | 60 |
tracing_subscriber |
+120 B | 94 |
tokio |
+120 B | 1790 |
memory_accounting::limiter::MemoryLimiter |
+120 B | 2 |
agent_data_plane::cli::dogstatsd |
+119 B | 34 |
Detailed Symbol Changes
FILE SIZE VM SIZE
-------------- --------------
[NEW] +1.79Mi [NEW] +1.79Mi std::thread::local::LocalKey<T>::with::h890865879671c15c
[NEW] +114Ki [NEW] +114Ki agent_data_plane::cli::run::create_topology::_{{closure}}::hefd7c58d49b1b9de
[NEW] +84.6Ki [NEW] +84.5Ki agent_data_plane::internal::control_plane::spawn_control_plane::_{{closure}}::h7db004459fe32df0
[NEW] +64.2Ki [NEW] +64.0Ki saluki_components::common::datadog::io::run_endpoint_io_loop::_{{closure}}::h0c7bd187fae2ad92
[NEW] +59.7Ki [NEW] +59.6Ki agent_data_plane::cli::run::handle_run_command::_{{closure}}::h194b82be826fc3ff
[NEW] +49.5Ki [NEW] +49.3Ki saluki_app::bootstrap::AppBootstrapper::bootstrap::_{{closure}}::h45ed871c526cc6b0
[NEW] +46.2Ki [NEW] +46.0Ki _<saluki_components::forwarders::otlp::OtlpForwarder as saluki_core::components::forwarders::Forwarder>::run::_{{closure}}::hf5397b05d6f2d1f0
[NEW] +45.7Ki [NEW] +45.5Ki _<saluki_components::destinations::prometheus::Prometheus as saluki_core::components::destinations::Destination>::run::_{{closure}}::h0fd7f099dbec47e8
[NEW] +44.3Ki [NEW] +44.1Ki _<saluki_components::transforms::aggregate::Aggregate as saluki_core::components::transforms::Transform>::run::_{{closure}}::h30cdec13d424d29b
[NEW] +44.1Ki [NEW] +43.9Ki saluki_env::workload::providers::remote_agent::RemoteAgentWorkloadProvider::from_configuration::_{{closure}}::hd0a544098d7f550b
+0.2% +15.3Ki +0.2% +16.7Ki [15509 Others]
[DEL] -44.1Ki [DEL] -43.9Ki saluki_env::workload::providers::remote_agent::RemoteAgentWorkloadProvider::from_configuration::_{{closure}}::h4cefc5ea44474312
[DEL] -44.4Ki [DEL] -44.2Ki _<saluki_components::transforms::aggregate::Aggregate as saluki_core::components::transforms::Transform>::run::_{{closure}}::hbad1a8f295684046
[DEL] -45.7Ki [DEL] -45.5Ki _<saluki_components::destinations::prometheus::Prometheus as saluki_core::components::destinations::Destination>::run::_{{closure}}::h48195e4ce3ad7c44
[DEL] -46.3Ki [DEL] -46.1Ki _<saluki_components::forwarders::otlp::OtlpForwarder as saluki_core::components::forwarders::Forwarder>::run::_{{closure}}::h361c3f26a257b43c
[DEL] -49.5Ki [DEL] -49.3Ki saluki_app::bootstrap::AppBootstrapper::bootstrap::_{{closure}}::h46e462ba2eb46346
[DEL] -59.9Ki [DEL] -59.8Ki agent_data_plane::cli::run::handle_run_command::_{{closure}}::h5f0d26e8067581f3
[DEL] -64.2Ki [DEL] -64.0Ki saluki_components::common::datadog::io::run_endpoint_io_loop::_{{closure}}::hebcc18fe34736c2b
[DEL] -84.6Ki [DEL] -84.5Ki agent_data_plane::internal::control_plane::spawn_control_plane::_{{closure}}::h9fc982c82bb60db5
[DEL] -114Ki [DEL] -114Ki agent_data_plane::cli::run::create_topology::_{{closure}}::h4b4a85d70f231106
[DEL] -1.79Mi [DEL] -1.79Mi std::thread::local::LocalKey<T>::with::h3697124af1157afa
+0.1% +14.1Ki +0.1% +15.5Ki TOTAL
Regression Detector (Agent Data Plane)Regression Detector ResultsRun ID: 3c53bc4c-788d-4be1-ba5a-00ca3c087d6e Baseline: 71ad7e9 Optimization Goals: ✅ No significant changes detected
|
| perf | experiment | goal | Δ mean % | Δ mean % CI | trials | links |
|---|---|---|---|---|---|---|
| ➖ | otlp_ingest_logs_5mb_throughput | ingress throughput | -0.04 | [-0.17, +0.08] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_logs_5mb_cpu | % cpu utilization | -1.75 | [-6.67, +3.17] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_logs_5mb_memory | memory utilization | -2.97 | [-3.51, -2.44] | 1 | (metrics) (profiles) (logs) |
Fine details of change detection per experiment
| perf | experiment | goal | Δ mean % | Δ mean % CI | trials | links |
|---|---|---|---|---|---|---|
| ➖ | dsd_uds_1mb_3k_contexts_cpu | % cpu utilization | +30.07 | [-32.45, +92.59] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_metrics_5mb_memory | memory utilization | +3.42 | [+3.19, +3.66] | 1 | (metrics) (profiles) (logs) |
| ➖ | dsd_uds_10mb_3k_contexts_cpu | % cpu utilization | +1.84 | [-30.13, +33.81] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_traces_ottl_transform_5mb_cpu | % cpu utilization | +1.25 | [-0.90, +3.40] | 1 | (metrics) (profiles) (logs) |
| ➖ | dsd_uds_100mb_3k_contexts_cpu | % cpu utilization | +0.75 | [-5.44, +6.93] | 1 | (metrics) (profiles) (logs) |
| ➖ | dsd_uds_500mb_3k_contexts_cpu | % cpu utilization | +0.53 | [-0.85, +1.92] | 1 | (metrics) (profiles) (logs) |
| ➖ | quality_gates_rss_idle | memory utilization | +0.49 | [+0.47, +0.52] | 1 | (metrics) (profiles) (logs) |
| ➖ | quality_gates_rss_dsd_ultraheavy | memory utilization | +0.35 | [+0.21, +0.48] | 1 | (metrics) (profiles) (logs) |
| ➖ | dsd_uds_10mb_3k_contexts_memory | memory utilization | +0.34 | [+0.16, +0.53] | 1 | (metrics) (profiles) (logs) |
| ➖ | dsd_uds_512kb_3k_contexts_memory | memory utilization | +0.34 | [+0.16, +0.51] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_traces_ottl_transform_5mb_memory | memory utilization | +0.32 | [+0.07, +0.57] | 1 | (metrics) (profiles) (logs) |
| ➖ | dsd_uds_500mb_3k_contexts_memory | memory utilization | +0.26 | [+0.09, +0.43] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_traces_5mb_memory | memory utilization | +0.22 | [-0.03, +0.47] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_traces_ottl_filtering_5mb_memory | memory utilization | +0.17 | [-0.17, +0.51] | 1 | (metrics) (profiles) (logs) |
| ➖ | quality_gates_rss_dsd_low | memory utilization | +0.13 | [-0.07, +0.32] | 1 | (metrics) (profiles) (logs) |
| ➖ | dsd_uds_100mb_3k_contexts_memory | memory utilization | +0.04 | [-0.14, +0.22] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_metrics_5mb_throughput | ingress throughput | +0.03 | [-0.10, +0.16] | 1 | (metrics) (profiles) (logs) |
| ➖ | quality_gates_rss_dsd_medium | memory utilization | +0.02 | [-0.17, +0.22] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_traces_5mb_throughput | ingress throughput | +0.00 | [-0.02, +0.02] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_traces_ottl_transform_5mb_throughput | ingress throughput | +0.00 | [-0.02, +0.02] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_traces_ottl_filtering_5mb_throughput | ingress throughput | -0.00 | [-0.02, +0.02] | 1 | (metrics) (profiles) (logs) |
| ➖ | dsd_uds_1mb_3k_contexts_throughput | ingress throughput | -0.00 | [-0.06, +0.05] | 1 | (metrics) (profiles) (logs) |
| ➖ | dsd_uds_512kb_3k_contexts_throughput | ingress throughput | -0.00 | [-0.05, +0.05] | 1 | (metrics) (profiles) (logs) |
| ➖ | dsd_uds_100mb_3k_contexts_throughput | ingress throughput | -0.01 | [-0.05, +0.03] | 1 | (metrics) (profiles) (logs) |
| ➖ | dsd_uds_10mb_3k_contexts_throughput | ingress throughput | -0.02 | [-0.17, +0.13] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_logs_5mb_throughput | ingress throughput | -0.04 | [-0.17, +0.08] | 1 | (metrics) (profiles) (logs) |
| ➖ | dsd_uds_1mb_3k_contexts_memory | memory utilization | -0.05 | [-0.22, +0.13] | 1 | (metrics) (profiles) (logs) |
| ➖ | dsd_uds_500mb_3k_contexts_throughput | ingress throughput | -0.20 | [-0.33, -0.07] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_traces_ottl_filtering_5mb_cpu | % cpu utilization | -0.34 | [-2.79, +2.10] | 1 | (metrics) (profiles) (logs) |
| ➖ | quality_gates_rss_dsd_heavy | memory utilization | -0.71 | [-0.85, -0.57] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_metrics_5mb_cpu | % cpu utilization | -1.03 | [-8.74, +6.69] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_logs_5mb_cpu | % cpu utilization | -1.75 | [-6.67, +3.17] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_traces_5mb_cpu | % cpu utilization | -2.76 | [-4.84, -0.67] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_logs_5mb_memory | memory utilization | -2.97 | [-3.51, -2.44] | 1 | (metrics) (profiles) (logs) |
| ➖ | dsd_uds_512kb_3k_contexts_cpu | % cpu utilization | -6.44 | [-62.29, +49.40] | 1 | (metrics) (profiles) (logs) |
Bounds Checks: ✅ Passed
| perf | experiment | bounds_check_name | replicates_passed | observed_value | links |
|---|---|---|---|---|---|
| ✅ | quality_gates_rss_dsd_heavy | memory_usage | 10/10 | 112.82MiB ≤ 140MiB | (metrics) (profiles) (logs) |
| ✅ | quality_gates_rss_dsd_low | memory_usage | 10/10 | 33.60MiB ≤ 50MiB | (metrics) (profiles) (logs) |
| ✅ | quality_gates_rss_dsd_medium | memory_usage | 10/10 | 52.87MiB ≤ 75MiB | (metrics) (profiles) (logs) |
| ✅ | quality_gates_rss_dsd_ultraheavy | memory_usage | 10/10 | 167.19MiB ≤ 200MiB | (metrics) (profiles) (logs) |
| ✅ | quality_gates_rss_idle | memory_usage | 10/10 | 20.96MiB ≤ 40MiB | (metrics) (profiles) (logs) |
Explanation
Confidence level: 90.00%
Effect size tolerance: |Δ mean %| ≥ 5.00%
Performance changes are noted in the perf column of each table:
- ✅ = significantly better comparison variant performance
- ❌ = significantly worse comparison variant performance
- ➖ = no significant change in performance
A regression test is an A/B test of target performance in a repeatable rig, where "performance" is measured as "comparison variant minus baseline variant" for an optimization goal (e.g., ingress throughput). Due to intrinsic variability in measuring that goal, we can only estimate its mean value for each experiment; we report uncertainty in that value as a 90.00% confidence interval denoted "Δ mean % CI".
For each experiment, we decide whether a change in performance is a "regression" -- a change worth investigating further -- if all of the following criteria are true:
-
Its estimated |Δ mean %| ≥ 5.00%, indicating the change is big enough to merit a closer look.
-
Its 90.00% confidence interval "Δ mean % CI" does not contain zero, indicating that if our statistical model is accurate, there is at least a 90.00% chance there is a difference in performance between baseline and comparison variants.
-
Its configuration does not mark it "erratic".
…rs, and case-insensitive matching
The Datadog Agent config file uses a nested format for proxy configuration:
proxy:
http: http://proxy.example.com
https: http://proxy.example.com
This produces Figment keys `proxy.http` and `proxy.https`, but `ProxyConfiguration`
deserializes from flat keys `proxy_http` and `proxy_https`. The mismatch caused proxy
configuration set via YAML to be silently ignored, and broke precedence between config
file values and environment variables.
The fix introduces two complementary mechanisms:
1. **Key aliasing in file providers**: `ConfigurationLoader::with_key_aliases` accepts a
table of `(nested_path, flat_key)` pairs. When loading a YAML or JSON file, any value
found at a nested path is also written under the flat key (if not already set), so both
formats produce the same canonical Figment key.
2. **`DatadogRemapper` provider**: A new Figment provider that remaps standard environment
variable names to canonical config keys (e.g. `HTTP_PROXY` → `proxy_http`,
`HTTPS_PROXY` → `proxy_https`). It reads env vars case-insensitively and snapshots
values at construction time.
Together these give the correct precedence order for proxy config:
YAML (proxy.http nested) < HTTP_PROXY env var < DD_PROXY_HTTP env var
`KEY_ALIASES` (the alias table) is exported from `saluki_components::config` alongside
`DatadogRemapper` for callers to pass to `with_key_aliases`.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
534bdeb to
7d048ac
Compare
- Replace redundant closure `|| DatadogRemapper::new()` with function pointer `DatadogRemapper::new` - Apply rustfmt to all changed files - Remove `saluki_components::config::DatadogRemapper` intra-doc link from `saluki-config` (circular dep) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
51dd241 to
80fc7b0
Compare
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
mergequeue build completed successfully, but the github api returned an error while merging the pr.
DetailsError: PUT https://api.github.com/repos/DataDog/saluki/pulls/1282/merge: 405 14 of 14 required status checks are expected. [] (Request ID: 816E:36B79C:10329F0:40C1340:69CD530F) FullStacktrace: |
c227b61
into
main
…rs, and case-insensitive matching (#1282) ## Bug The Datadog Agent config file uses a nested format for proxy configuration: ```yaml proxy: http: http://proxy.example.com https: http://proxy.example.com ``` This produces Figment keys `proxy.http` and `proxy.https`, but `ProxyConfiguration` deserializes from flat keys `proxy_http` and `proxy_https`. The key mismatch caused two bugs: 1. Proxy configuration set via YAML was silently ignored at runtime. 2. Precedence between YAML, `HTTP_PROXY`/`HTTPS_PROXY`, and `DD_PROXY_HTTP`/`DD_PROXY_HTTPS` was broken — the env vars couldn't override the YAML value because they wrote to a different key. ## Fix Two complementary mechanisms restore correct behavior: **Key aliasing in file providers** — `ConfigurationLoader::with_key_aliases` accepts a table of `(nested_path, flat_key)` pairs. When a YAML or JSON file is loaded, any value found at a nested path is also written under the flat key (if not already explicitly set), so both formats produce the same canonical Figment key. **`DatadogRemapper` provider** — a new Figment provider that remaps standard environment variable names to canonical config keys (`HTTP_PROXY` → `proxy_http`, `HTTPS_PROXY` → `proxy_https`). It matches case-insensitively and snapshots values at construction time. Both are exported from `saluki_components::config` and wired into the ADP bootstrap and run configurations. ## Precedence order (proxy example) ``` YAML (proxy.http nested) < HTTP_PROXY env var < DD_PROXY_HTTP env var ``` This matches the Datadog Agent's documented proxy precedence. ## Test plan - [ ] Unit tests in `saluki-config::provider` cover key alias application for both JSON and YAML - [ ] Unit tests in `saluki-components::config` cover `DatadogRemapper` env var remapping - [ ] Integration-style tests in `ForwarderConfiguration` verify the full precedence chain end-to-end 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: travis.thieman <travis.thieman@datadoghq.com> c227b61
…#1306) ## Summary - Fixes proxy configuration for nested YAML, standard env vars, and case-insensitive matching (originally merged in #1282) - Adds `no_proxy` support with host/IP/CIDR matching, `no_proxy_nonexact_match`, and `use_proxy_for_cloud_metadata` fields (originally merged in #1289) This PR brings `thieman/proxy-config-improvements` into `main`. PR #1289 was accidentally targeted at this feature branch instead of `main`. Fixes #806. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: travis.thieman <travis.thieman@datadoghq.com>
…#1306) ## Summary - Fixes proxy configuration for nested YAML, standard env vars, and case-insensitive matching (originally merged in #1282) - Adds `no_proxy` support with host/IP/CIDR matching, `no_proxy_nonexact_match`, and `use_proxy_for_cloud_metadata` fields (originally merged in #1289) This PR brings `thieman/proxy-config-improvements` into `main`. PR #1289 was accidentally targeted at this feature branch instead of `main`. Fixes #806. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: travis.thieman <travis.thieman@datadoghq.com> b7aedc3
Bug
The Datadog Agent config file uses a nested format for proxy configuration:
This produces Figment keys
proxy.httpandproxy.https, butProxyConfigurationdeserializes from flat keysproxy_httpandproxy_https. The key mismatch caused two bugs:HTTP_PROXY/HTTPS_PROXY, andDD_PROXY_HTTP/DD_PROXY_HTTPSwas broken — the env vars couldn't override the YAML value because they wrote to a different key.Fix
Two complementary mechanisms restore correct behavior:
Key aliasing in file providers —
ConfigurationLoader::with_key_aliasesaccepts a table of(nested_path, flat_key)pairs. When a YAML or JSON file is loaded, any value found at a nested path is also written under the flat key (if not already explicitly set), so both formats produce the same canonical Figment key.DatadogRemapperprovider — a new Figment provider that remaps standard environment variable names to canonical config keys (HTTP_PROXY→proxy_http,HTTPS_PROXY→proxy_https). It matches case-insensitively and snapshots values at construction time.Both are exported from
saluki_components::configand wired into the ADP bootstrap and run configurations.Precedence order (proxy example)
This matches the Datadog Agent's documented proxy precedence.
Test plan
saluki-config::providercover key alias application for both JSON and YAMLsaluki-components::configcoverDatadogRemapperenv var remappingForwarderConfigurationverify the full precedence chain end-to-end🤖 Generated with Claude Code