configstream: add a ConfigUnset event so layer removals cross the wire - #55609
configstream: add a ConfigUnset event so layer removals cross the wire#55609rahulkaukuntla wants to merge 4 commits into
Conversation
Snapshots only ever carry the core agent's merged view -- one entry per key, tagged with the winning source -- so a subscriber has no lower layer of its own to fall back to when a layer is cleared. A ConfigUpdate cannot express a removal: it wrote the shadowed value back into the layer being cleared, leaving the subscriber diverged from the core agent. Reporting the resolved source instead would not help either, since the stale higher-priority entry would keep winning the merge. Add a ConfigUnset event carrying the cleared layer plus what the key resolves to without it, and a model.Config OnUnset hook to observe layer removals, which NotificationReceiver cannot describe. The consumer seeds the fallback value before applying the removal so local receivers see the final value once rather than transiently observing the default.
… one Set compared previousValue against its own newValue argument and passed that argument to receivers. A write to a layer that loses the merge therefore notified with a value that Get never returns, and named the layer written to rather than the one the setting resolves from. Read back from the merged tree instead, so a losing write notifies nobody and a winning one reports what the setting now is. The write is still recorded, and surfaces correctly if the layer above it is later cleared.
There was a problem hiding this comment.
AI review by Codex (OpenAI) - workflow run
Patch is incorrect: a failed fallback-value encoding is converted into a valid-looking removal event, causing subscribers to permanently diverge.
Emitting the unset without Resolved after a failed encode reads as "nothing remains" on the subscriber, which drops the key and advances its sequence ID -- diverging permanently with nothing left to signal it. Drop the event instead, matching what the update path already does. The consumed sequence ID leaves a gap, and handleConfigUpdate resynchronizes the affected subscriber with a fresh snapshot.
|
/dd-review |
There was a problem hiding this comment.
AI review by Codex (OpenAI) - workflow run
Verdict
Patch is incorrect. The new event is sent without backward-compatible negotiation, so mixed-version deployments can break config streaming. The protocol documentation is also stale.
|
🎯 Code Coverage (details) 🔗 Commit SHA: 7d3ffec | Docs | View more details | Give us feedback! |
…clients The README described a ConfigEvent with only snapshots and updates. Add the new arm along with the removal semantics: why the fallback value has to travel with the removal, and why an unrecognized event has to resynchronize rather than be skipped. config-stream-client and internal/remote-agent switch on the event kind with no default case, so both silently swallowed unsets -- the former being the tool reached for to debug exactly this. Both now report them, and the debug client reports unknown kinds instead of hiding them.
Files inventory check summaryFile checks results against ancestor ac3e2c30: Results for datadog-agent_7.84.0~devel.git.585.7d3ffec.pipeline.134020131-1_amd64.deb:No change detected Results for datadog-iot-agent_7.84.0~devel.git.585.7d3ffec.pipeline.134020131-1_amd64.deb:No change detected |
Static quality checks✅ Please find below the results from static quality gates Successful checksInfo
5 successful checks with minimal change (< 2 KiB)
|
Regression DetectorRegression Detector ResultsMetrics dashboard Baseline: ac3e2c3 Optimization Goals: ✅ No significant changes detected
|
| perf | experiment | goal | Δ mean % | Δ mean % CI | trials | links |
|---|---|---|---|---|---|---|
| ➖ | dsd_uds_10mb_3k_timestamped_contexts_cpu | % cpu utilization | +0.84 | [+0.58, +1.10] | 1 | Logs |
| ➖ | quality_gate_security_idle | memory utilization | +0.21 | [+0.16, +0.26] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_security_mean_fs_load | memory utilization | +0.06 | [+0.02, +0.09] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_idle | memory utilization | -0.04 | [-0.08, +0.01] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_metrics_logs | memory utilization | -0.06 | [-0.29, +0.17] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_security_no_fs_load | memory utilization | -0.27 | [-0.34, -0.19] | 1 | Logs bounds checks dashboard |
| ➖ | dsd_uds_10mb_3k_timestamped_contexts_memory | memory utilization | -0.28 | [-0.49, -0.06] | 1 | Logs |
| ➖ | quality_gate_idle_all_features | memory utilization | -0.38 | [-0.42, -0.34] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_private_action_runner | memory utilization | -0.88 | [-1.00, -0.75] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_logs | % cpu utilization | -4.21 | [-5.07, -3.35] | 1 | Logs bounds checks dashboard |
Bounds Checks: ✅ Passed
| perf | experiment | bounds_check_name | replicates_passed | observed_value | links |
|---|---|---|---|---|---|
| ✅ | quality_gate_idle | intake_connections | 10/10 | 4 = 4 | bounds checks dashboard |
| ✅ | quality_gate_idle | memory_usage | 10/10 | 173.41MiB ≤ 179MiB | bounds checks dashboard |
| ✅ | quality_gate_idle | total_bytes_received | 10/10 | 749.04KiB ≤ 819.20KiB | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | intake_connections | 10/10 | 4 = 4 | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | memory_usage | 10/10 | 518.33MiB ≤ 537MiB | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | total_bytes_received | 10/10 | 1.14MiB ≤ 1.25MiB | bounds checks dashboard |
| ✅ | quality_gate_logs | intake_connections | 10/10 | 18 ≤ 40 | bounds checks dashboard |
| ✅ | quality_gate_logs | memory_usage | 10/10 | 211.58MiB ≤ 220MiB | bounds checks dashboard |
| ✅ | quality_gate_logs | missed_bytes | 10/10 | 0B = 0B | bounds checks dashboard |
| ✅ | quality_gate_logs | total_bytes_received | 10/10 | 263.09MiB ≤ 292MiB | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | cpu_usage | 10/10 | 378.27 ≤ 2000 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | intake_connections | 10/10 | 19 ≤ 40 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | memory_usage | 10/10 | 438.49MiB ≤ 455MiB | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | missed_bytes | 10/10 | 0B = 0B | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | total_bytes_received | 10/10 | 0.94GiB ≤ 1.04GiB | bounds checks dashboard |
| ✅ | quality_gate_private_action_runner | memory_usage | 10/10 | 71.60MiB ≤ 75MiB | bounds checks dashboard |
| ✅ | quality_gate_security_idle | cpu_usage | 10/10 | 29.96 ≤ 100 | bounds checks dashboard |
| ✅ | quality_gate_security_idle | memory_usage | 10/10 | 326.53MiB ≤ 355MiB | bounds checks dashboard |
| ✅ | quality_gate_security_mean_fs_load | cpu_usage | 10/10 | 61.28 ≤ 200 | bounds checks dashboard |
| ✅ | quality_gate_security_mean_fs_load | memory_usage | 10/10 | 307.20MiB ≤ 335MiB | bounds checks dashboard |
| ✅ | quality_gate_security_no_fs_load | cpu_usage | 10/10 | 23.50 ≤ 100 | bounds checks dashboard |
| ✅ | quality_gate_security_no_fs_load | memory_usage | 10/10 | 314.04MiB ≤ 345MiB | bounds checks dashboard |
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".
CI Pass/Fail Decision
✅ Passed. All Quality Gates passed.
- quality_gate_security_no_fs_load, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_no_fs_load, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check missed_bytes: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_security_mean_fs_load, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_mean_fs_load, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check missed_bytes: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_idle_all_features, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_idle_all_features, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_idle_all_features, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_security_idle, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_idle, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_private_action_runner, bounds check memory_usage: 10/10 replicas passed. Gate passed.
There was a problem hiding this comment.
AI review by Codex (OpenAI) - workflow run
patch is correct — the unset protocol, producer sequencing, consumer application, and coverage are consistent, with no actionable defects found.
What does this PR do?
Adds a
ConfigUnsetevent to the config stream protocol so that clearing a source layer on the core agent is reproduced on subscribers, and fixesSetto notify receivers with the resolved value rather than the one written.Motivation
Snapshots only carry the core agent's merged view — one entry per key, tagged with the winning source — so a subscriber has no lower layer of its own to fall back to when a layer is cleared. A
ConfigUpdatecannot express a removal: it wrote the shadowed value back into the layer being emptied, leaving the subscriber diverged. Reporting the resolved source instead wouldn't help, since the subscriber's stale higher-priority entry would keep winning the merge. So the removal has to cross the wire, and it has to carry what the key resolves to without the cleared layer.The second commit fixes a related bug on the producer side:
SetcomparedpreviousValueagainst its ownnewValueargument, so a write to a layer that loses the merge notified receivers with a valueGetnever returns, under the layer written to rather than the one it resolves from. It now reads back from the merged tree.Describe how you validated your changes
New unit tests in
pkg/config/nodetreemodel, producer tests incomp/core/configstream/impl, and a consumer integration test covering the env-var-shadowed-by-agent-runtime case. Both fixes were verified to fail against the unpatched code. Also, live-tested end to end with the real agent and trace-agent binaries streaming over gRPC.