fix(agent-data-plane): correct JSON parsing for /config response, body truncation, and scrubber quoting#1548
Conversation
Binary Size Analysis (Agent Data Plane)Target: a47b224 (baseline) vs 2e4e7c6 (comparison) diff
|
| Module | File Size | Symbols |
|---|---|---|
core |
+59.71 KiB | 3942 |
unsafe_libyaml |
-38.76 KiB | 29 |
hyper |
+21.73 KiB | 202 |
alloc |
+20.02 KiB | 507 |
hyper_util |
-16.78 KiB | 29 |
serde_core |
-15.33 KiB | 229 |
serde_yaml |
-13.82 KiB | 49 |
serde_json |
+10.00 KiB | 87 |
tokio_rustls |
-9.51 KiB | 22 |
rustls |
+8.92 KiB | 19 |
agent_data_plane::components::apm_onboarding |
-8.75 KiB | 35 |
saluki_core::state::reflector |
+6.38 KiB | 6 |
saluki_core::topology::blueprint |
-6.33 KiB | 43 |
agent_data_plane::run_inner::_{{closure}} |
+6.16 KiB | 18 |
std |
-6.11 KiB | 125 |
&mut serde_json |
+5.85 KiB | 23 |
[Unmapped] |
+5.57 KiB | 1 |
ottl::parser::arena |
-5.30 KiB | 20 |
saluki_config::GenericConfiguration::try_get_typed |
-5.28 KiB | 22 |
http_body_util |
+5.26 KiB | 84 |
Detailed Symbol Changes
FILE SIZE VM SIZE
-------------- --------------
[NEW] +146Ki [NEW] +145Ki agent_data_plane::cli::run::handle_run_command::_{{closure}}::haa9813ed4ca23f28
[NEW] +69.5Ki [NEW] +69.3Ki agent_data_plane::run_inner::_{{closure}}::h723aacf938f9d030
[NEW] +65.2Ki [NEW] +65.0Ki saluki_core::topology::built::BuiltTopology::spawn::_{{closure}}::h46a002518f3986e6
[NEW] +64.0Ki [NEW] +63.8Ki agent_data_plane::cli::run::create_topology::_{{closure}}::hc5a0308291d23682
[NEW] +57.7Ki [NEW] +57.6Ki saluki_core::topology::blueprint::TopologyBlueprint::build::_{{closure}}::h91fe1d0fd4947006
[NEW] +57.6Ki [NEW] +57.4Ki saluki_env::workload::providers::remote_agent::RemoteAgentWorkloadProvider::from_configuration::_{{closure}}::hdbe5f6b3baf1e3f5
[NEW] +57.4Ki [NEW] +57.3Ki agent_data_plane::cli::debug::handle_debug_command::_{{closure}}::h11fa1baf8ca2126e
[NEW] +43.5Ki [NEW] +43.4Ki _<figment::value::de::ConfiguredValueDe<I> as serde_core::de::Deserializer>::deserialize_struct::h16064f15ac126db1
[NEW] +36.4Ki [NEW] +36.2Ki saluki_env::helpers::remote_agent::client::RemoteAgentClient::from_configuration::_{{closure}}::hc18bfcb42b1927e3
+0.4% +27.7Ki -0.1% -5.67Ki [11703 Others]
[NEW] +24.2Ki [NEW] +24.0Ki _<figment::value::de::ConfiguredValueDe<I> as serde_core::de::Deserializer>::deserialize_struct::h927322bb51f58c94
[DEL] -24.5Ki [DEL] -24.3Ki agent_data_plane::internal::remote_agent::run_remote_agent_registration_loop::_{{closure}}::h5c28facf299da4b1
[DEL] -37.0Ki [DEL] -36.8Ki saluki_env::helpers::remote_agent::client::RemoteAgentClient::from_configuration::_{{closure}}::h425c60e55962f65f
[DEL] -45.0Ki [DEL] -44.9Ki _<figment::value::de::ConfiguredValueDe<I> as serde_core::de::Deserializer>::deserialize_struct::h41bbdfb398ffc402
[DEL] -57.3Ki [DEL] -57.1Ki saluki_env::workload::providers::remote_agent::RemoteAgentWorkloadProvider::from_configuration::_{{closure}}::h4c13754b6ce4085d
[DEL] -57.5Ki [DEL] -57.4Ki saluki_core::topology::blueprint::TopologyBlueprint::build::_{{closure}}::h92652bb731b7a6ce
[DEL] -57.7Ki [DEL] -57.5Ki agent_data_plane::cli::debug::handle_debug_command::_{{closure}}::h965c6db136660c6c
[DEL] -63.3Ki [DEL] -63.2Ki agent_data_plane::run_inner::_{{closure}}::h64914a9c7c1b7789
[DEL] -63.3Ki [DEL] -63.2Ki agent_data_plane::cli::run::create_topology::_{{closure}}::h07a486be4c2edd20
[DEL] -65.1Ki [DEL] -64.9Ki saluki_core::topology::built::BuiltTopology::spawn::_{{closure}}::h1f7106bedd18ac02
[DEL] -146Ki [DEL] -146Ki agent_data_plane::cli::run::handle_run_command::_{{closure}}::h1eac5b7f7595aab9
+0.1% +31.8Ki -0.0% -1.57Ki TOTAL
…ing; fix body allocation and add precision note
|
@codex review |
Regression Detector (Agent Data Plane)Regression Detector ResultsRun ID: 19fa5594-1c1c-4834-8a77-5260154b46d9 Baseline: a47b224 Optimization Goals: ✅ No significant changes detected
|
| perf | experiment | goal | Δ mean % | Δ mean % CI | trials | links |
|---|---|---|---|---|---|---|
| ➖ | otlp_ingest_logs_5mb_memory | memory utilization | +1.23 | [+0.65, +1.80] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_logs_5mb_throughput | ingress throughput | +0.01 | [-0.12, +0.13] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_logs_5mb_cpu | % cpu utilization | -1.86 | [-6.75, +3.03] | 1 | (metrics) (profiles) (logs) |
Fine details of change detection per experiment
| perf | experiment | goal | Δ mean % | Δ mean % CI | trials | links |
|---|---|---|---|---|---|---|
| ➖ | otlp_ingest_metrics_5mb_memory | memory utilization | +1.44 | [+1.23, +1.65] | 1 | (metrics) (profiles) (logs) |
| ➖ | dsd_uds_500mb_3k_contexts_throughput | ingress throughput | +1.29 | [+1.17, +1.41] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_traces_ottl_filtering_5mb_cpu | % cpu utilization | +1.26 | [-0.81, +3.34] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_logs_5mb_memory | memory utilization | +1.23 | [+0.65, +1.80] | 1 | (metrics) (profiles) (logs) |
| ➖ | dsd_uds_100mb_3k_contexts_cpu | % cpu utilization | +0.87 | [-4.80, +6.55] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_metrics_5mb_cpu | % cpu utilization | +0.42 | [-5.75, +6.60] | 1 | (metrics) (profiles) (logs) |
| ➖ | dsd_uds_1mb_3k_contexts_memory | memory utilization | +0.36 | [+0.22, +0.50] | 1 | (metrics) (profiles) (logs) |
| ➖ | dsd_uds_10mb_3k_contexts_memory | memory utilization | +0.32 | [+0.16, +0.47] | 1 | (metrics) (profiles) (logs) |
| ➖ | dsd_uds_100mb_3k_contexts_memory | memory utilization | +0.18 | [+0.02, +0.33] | 1 | (metrics) (profiles) (logs) |
| ➖ | dsd_uds_500mb_3k_contexts_memory | memory utilization | +0.16 | [+0.01, +0.30] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_traces_ottl_transform_5mb_memory | memory utilization | +0.05 | [-0.11, +0.21] | 1 | (metrics) (profiles) (logs) |
| ➖ | dsd_uds_500mb_3k_contexts_cpu | % cpu utilization | +0.04 | [-1.41, +1.49] | 1 | (metrics) (profiles) (logs) |
| ➖ | dsd_uds_100mb_3k_contexts_throughput | ingress throughput | +0.02 | [-0.02, +0.05] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_metrics_5mb_throughput | ingress throughput | +0.01 | [-0.14, +0.16] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_logs_5mb_throughput | ingress throughput | +0.01 | [-0.12, +0.13] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_traces_5mb_cpu | % cpu utilization | +0.00 | [-1.97, +1.98] | 1 | (metrics) (profiles) (logs) |
| ➖ | dsd_uds_1mb_3k_contexts_throughput | ingress throughput | +0.00 | [-0.06, +0.06] | 1 | (metrics) (profiles) (logs) |
| ➖ | dsd_uds_512kb_3k_contexts_throughput | ingress throughput | -0.00 | [-0.06, +0.05] | 1 | (metrics) (profiles) (logs) |
| ➖ | dsd_uds_10mb_3k_contexts_throughput | ingress throughput | -0.00 | [-0.19, +0.19] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_traces_ottl_filtering_5mb_memory | memory utilization | -0.01 | [-0.25, +0.23] | 1 | (metrics) (profiles) (logs) |
| ➖ | dsd_uds_10mb_3k_contexts_cpu | % cpu utilization | -0.05 | [-28.74, +28.63] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_traces_5mb_throughput | ingress throughput | -0.06 | [-0.13, +0.02] | 1 | (metrics) (profiles) (logs) |
| ➖ | quality_gates_rss_dsd_ultraheavy | memory utilization | -0.06 | [-0.19, +0.06] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_traces_5mb_memory | memory utilization | -0.10 | [-0.26, +0.06] | 1 | (metrics) (profiles) (logs) |
| ➖ | quality_gates_rss_dsd_low | memory utilization | -0.11 | [-0.27, +0.05] | 1 | (metrics) (profiles) (logs) |
| ➖ | dsd_uds_512kb_3k_contexts_cpu | % cpu utilization | -0.13 | [-56.81, +56.56] | 1 | (metrics) (profiles) (logs) |
| ➖ | quality_gates_rss_dsd_medium | memory utilization | -0.31 | [-0.48, -0.14] | 1 | (metrics) (profiles) (logs) |
| ➖ | dsd_uds_512kb_3k_contexts_memory | memory utilization | -0.32 | [-0.46, -0.17] | 1 | (metrics) (profiles) (logs) |
| ➖ | quality_gates_rss_idle | memory utilization | -0.38 | [-0.43, -0.33] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_traces_ottl_transform_5mb_cpu | % cpu utilization | -0.47 | [-2.50, +1.56] | 1 | (metrics) (profiles) (logs) |
| ➖ | quality_gates_rss_dsd_heavy | memory utilization | -0.66 | [-0.78, -0.53] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_traces_ottl_filtering_5mb_throughput | ingress throughput | -0.83 | [-0.92, -0.75] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_traces_ottl_transform_5mb_throughput | ingress throughput | -1.30 | [-1.41, -1.18] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_logs_5mb_cpu | % cpu utilization | -1.86 | [-6.75, +3.03] | 1 | (metrics) (profiles) (logs) |
| ➖ | dsd_uds_1mb_3k_contexts_cpu | % cpu utilization | -2.31 | [-54.65, +50.04] | 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 | 121.13MiB ≤ 140MiB | (metrics) (profiles) (logs) |
| ✅ | quality_gates_rss_dsd_low | memory_usage | 10/10 | 40.27MiB ≤ 50MiB | (metrics) (profiles) (logs) |
| ✅ | quality_gates_rss_dsd_medium | memory_usage | 10/10 | 60.46MiB ≤ 75MiB | (metrics) (profiles) (logs) |
| ✅ | quality_gates_rss_dsd_ultraheavy | memory_usage | 10/10 | 177.33MiB ≤ 200MiB | (metrics) (profiles) (logs) |
| ✅ | quality_gates_rss_idle | memory_usage | 10/10 | 27.58MiB ≤ 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".
|
Codex Review: Didn't find any major issues. Nice work! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
lucastemb
left a comment
There was a problem hiding this comment.
Solid work! Just two small comments. 👍
tobz
left a comment
There was a problem hiding this comment.
All three of the bug fixes mentioned make sense here. What I'm not seeing is the changes to saluki-config.
The stated purpose makes sense, but we don't actually want callers to have to drop down to raw maps, messing around with them... that defeats the purpose of the configuration object being effectively read-only and unidirectional: delivered from Core Agent and used as-is by remote agents.
Further, I don't actually see us using it here which makes me skeptical of its value. I think we should remove this portion of the PR for right now and, if anything, open a new PR with it by itself, showing how it's intended to be used, so we can talk about the design and whether or not we want to move forward with something like that.
|
We can bypass for now since the |
Summary
Three related bugs that all manifested when running
target/devel/agent-data-plane config(after having runDD_DATA_PLANE_USE_NEW_CONFIG_STREAM_ENDPOINT=true make run-adp) against a live agent that is using config streaming:1. Wrong parser for
/configresponse (cli/config.rs)The privileged
/configendpoint (ConfigAPIHandler) returns JSON, but the CLI was deserializing it throughserde_yaml. YAML is a superset of JSON so small payloads worked accidentally, but the output was re-serialized as YAML which loses structure (e.g. collapses object keys). Fixed by parsing withserde_jsonand pretty-printing the result.2. Response body silently truncated for large payloads (
cli/utils.rs)collect_bodywas callingBuf::chunk().to_vec()on the aggregated body.chunk()returns only the first contiguous slice of aBuf(typically ~16 KiB), so responses larger than one internal chunk were silently truncated — exactly the case for a full agent config dump. Fixed by callingCollected::to_bytes(), which merges all frames before converting.3. Scrubber strips closing
"from JSON string values (scrubber.rs)The
password_replacerregex did not capture the trailing"delimiter, so"password":"secret"became"password":"********— missing the closing quote — which made the scrubbed JSON unparseable. Fixed by adding(")as capture group$4and including$4in the replacement, so the delimiter is preserved. Added unit tests to assert scrubbed output is still valid JSON.Change Type
How did you test this PR?
scrubber.rscovering JSON password scrubbing, API keyscrubbing on large payloads, and YAML double-quoted values
lib/saluki-config/tests/config_duplicate_keys.rsfor both new
GenericConfigurationmethodssaluki-commontest suites pass (cargo test -p saluki-common -p saluki-config)-D warningsacross all affected crates