From eca2923a9b959674dffb339422258227fa23dea5 Mon Sep 17 00:00:00 2001 From: iunanua Date: Wed, 10 Jun 2026 10:27:11 +0200 Subject: [PATCH 1/4] Fix DynamicConfig property name --- libdd-remote-config/src/config/dynamic.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/libdd-remote-config/src/config/dynamic.rs b/libdd-remote-config/src/config/dynamic.rs index c0fa77940c..f845736120 100644 --- a/libdd-remote-config/src/config/dynamic.rs +++ b/libdd-remote-config/src/config/dynamic.rs @@ -80,7 +80,7 @@ pub struct TracingSamplingRule { #[cfg_attr(feature = "test", derive(Default, Serialize))] pub struct DynamicConfig { pub(crate) tracing_header_tags: Option>, - pub(crate) tracing_sample_rate: Option, + pub(crate) tracing_sampling_rate: Option, pub(crate) log_injection_enabled: Option, pub(crate) tracing_tags: Option>, pub(crate) tracing_enabled: Option, @@ -98,8 +98,8 @@ impl From for Vec { tags.into_iter().map(|t| (t.header, t.tag_name)).collect(), )) } - if let Some(sample_rate) = value.tracing_sample_rate { - vec.push(Configs::TracingSampleRate(sample_rate)); + if let Some(sample_rate) = value.tracing_sampling_rate { + vec.push(Configs::TracingSamplingRate(sample_rate)); } if let Some(log_injection) = value.log_injection_enabled { vec.push(Configs::LogInjectionEnabled(log_injection)); @@ -129,7 +129,7 @@ impl From for Vec { #[derive(Clone)] pub enum Configs { TracingHeaderTags(HashMap), - TracingSampleRate(f64), + TracingSamplingRate(f64), LogInjectionEnabled(bool), TracingTags(Vec), // "key:val" format TracingEnabled(bool), From 1e238b0568946797d808741570330548092c1227 Mon Sep 17 00:00:00 2001 From: iunanua Date: Wed, 10 Jun 2026 16:49:25 +0200 Subject: [PATCH 2/4] Preserve absent vs explicit-null for DynamicConfig fields MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Wraps every DynamicConfig field in a new `Patch` newtype that encodes JSON Merge Patch (RFC 7396) semantics: - Patch(None) — field absent on the wire (no change intended) - Patch(Some(None)) — field present as JSON null (clear prior override) - Patch(Some(Some(v))) — field present with value (set to v) `Configs` variants now carry `Option` (Some = set, None = clear), and `From for Vec` only emits a variant for fields that were actually delivered. This fixes the prior collapse where any APM_TRACING payload missing tracing_sampling_rate looked the same to consumers as `tracing_sampling_rate: null`, so a payload that updated unrelated fields (e.g. tracing_tags) would wipe an active remote sample-rate override. The struct-level `#[serde(default)]` keeps the new shape free of per-field `deserialize_with` boilerplate. Test-feature `Serialize` is a small hand-written impl that skips absent fields to keep the `dummy_dynamic_config` round-trip intact for the sidecar tests. Co-Authored-By: Claude Opus 4.7 --- datadog-sidecar/src/shm_remote_config.rs | 4 +- libdd-remote-config/src/config/dynamic.rs | 238 +++++++++++++++++++--- 2 files changed, 207 insertions(+), 35 deletions(-) diff --git a/datadog-sidecar/src/shm_remote_config.rs b/datadog-sidecar/src/shm_remote_config.rs index f0c3022d8f..118a76d65c 100644 --- a/datadog-sidecar/src/shm_remote_config.rs +++ b/datadog-sidecar/src/shm_remote_config.rs @@ -305,7 +305,7 @@ impl MultiTargetHandlers for ConfigFileStora let configs: Vec = config.lib_config.into(); for config in configs { if let Configs::DynamicInstrumentationEnabled(enabled) = config { - writer.dynamic_instrumentation = Some(enabled); + writer.dynamic_instrumentation = enabled; } } } @@ -923,7 +923,7 @@ mod tests { if let Some(cfg) = parsed.downcast::() { assert!(matches!( >::from(cfg.lib_config.clone())[0], - Configs::TracingEnabled(true) + Configs::TracingEnabled(Some(true)) )); } else { unreachable!(); diff --git a/libdd-remote-config/src/config/dynamic.rs b/libdd-remote-config/src/config/dynamic.rs index f845736120..7ff57f2934 100644 --- a/libdd-remote-config/src/config/dynamic.rs +++ b/libdd-remote-config/src/config/dynamic.rs @@ -76,67 +76,171 @@ pub struct TracingSamplingRule { pub sample_rate: f64, } -#[derive(Debug, Clone, Deserialize)] -#[cfg_attr(feature = "test", derive(Default, Serialize))] +/// Three-state field carrying JSON Merge Patch (RFC 7396) semantics: +/// +/// - `Patch(None)` — field was absent on the wire (no change intended). +/// - `Patch(Some(None))` — field was present as JSON `null` (clear any prior remote override). +/// - `Patch(Some(Some(v)))` — field was present with a value (set to `v`). +/// +/// Used by [`DynamicConfig`] for every payload field so the absent / clear / +/// set distinction survives all the way from the wire to consumers reading +/// [`Configs`]. With struct-level `#[serde(default)]` on [`DynamicConfig`], +/// missing fields fall back to [`Patch::default`] (= `Patch(None)`); present +/// fields go through this `Deserialize` impl which resolves null-vs-value via +/// the inner `Option::::deserialize` and wraps the result in `Some` to +/// mark "delivered". +#[derive(Debug, Clone, PartialEq)] +pub struct Patch(pub Option>); + +impl Patch { + /// `true` when the field was not delivered on the wire. + pub fn is_absent(&self) -> bool { + self.0.is_none() + } +} + +impl Default for Patch { + fn default() -> Self { + Patch(None) + } +} + +impl<'de, T: Deserialize<'de>> Deserialize<'de> for Patch { + fn deserialize>(deserializer: D) -> Result { + // `Deserialize::deserialize` only runs when the field WAS present on + // the wire (absent fields go through `Default` via the struct-level + // `#[serde(default)]`). Resolving null-vs-value via the inner + // `Option::::deserialize` lets us encode all three states. + Option::::deserialize(deserializer).map(|inner| Patch(Some(inner))) + } +} + +#[cfg(feature = "test")] +impl Serialize for Patch { + fn serialize(&self, serializer: S) -> Result { + // The struct-level `Serialize` impl for `DynamicConfig` skips absent + // fields before reaching this method, so we only see delivered values: + // `Some(None)` → JSON `null`, `Some(Some(v))` → `v`. + match &self.0 { + None => serializer.serialize_none(), + Some(inner) => inner.serialize(serializer), + } + } +} + +/// Dynamic configuration delivered by the APM_TRACING product. +/// +/// Every field is a [`Patch`] so the absent / clear / set distinction +/// survives the wire. Struct-level `#[serde(default)]` lets missing fields +/// fall back to `Patch::default()` without per-field `deserialize_with` +/// boilerplate. The matching `Serialize` impl (under `feature = "test"`) is +/// hand-written below so absent fields are omitted from the output without +/// needing per-field `skip_serializing_if` annotations. See [`Configs`] for +/// how the three states surface to consumers. +#[derive(Debug, Clone, Default, Deserialize)] +#[serde(default)] pub struct DynamicConfig { - pub(crate) tracing_header_tags: Option>, - pub(crate) tracing_sampling_rate: Option, - pub(crate) log_injection_enabled: Option, - pub(crate) tracing_tags: Option>, - pub(crate) tracing_enabled: Option, - pub(crate) tracing_sampling_rules: Option>, - pub(crate) dynamic_instrumentation_enabled: Option, - pub(crate) exception_replay_enabled: Option, - pub(crate) code_origin_enabled: Option, + pub(crate) tracing_header_tags: Patch>, + pub(crate) tracing_sampling_rate: Patch, + pub(crate) log_injection_enabled: Patch, + pub(crate) tracing_tags: Patch>, + pub(crate) tracing_enabled: Patch, + pub(crate) tracing_sampling_rules: Patch>, + pub(crate) dynamic_instrumentation_enabled: Patch, + pub(crate) exception_replay_enabled: Patch, + pub(crate) code_origin_enabled: Patch, +} + +#[cfg(feature = "test")] +impl Serialize for DynamicConfig { + fn serialize(&self, serializer: S) -> Result { + // Manual impl so absent fields (`Patch(None)`) are omitted from the + // output entirely rather than serializing as JSON `null` — that + // collapse would make round-trips re-deserialize absent fields as + // explicit clears, defeating the three-state semantics. + // + // `serialize_struct`'s `len` argument is only used by formats that + // pre-allocate (bincode, etc.); JSON ignores it. Pass the maximum. + use serde::ser::SerializeStruct; + macro_rules! field { + ($s:expr, $name:ident) => { + if !self.$name.is_absent() { + $s.serialize_field(stringify!($name), &self.$name)?; + } + }; + } + let mut s = serializer.serialize_struct("DynamicConfig", 9)?; + field!(s, tracing_header_tags); + field!(s, tracing_sampling_rate); + field!(s, log_injection_enabled); + field!(s, tracing_tags); + field!(s, tracing_enabled); + field!(s, tracing_sampling_rules); + field!(s, dynamic_instrumentation_enabled); + field!(s, exception_replay_enabled); + field!(s, code_origin_enabled); + s.end() + } } impl From for Vec { fn from(value: DynamicConfig) -> Self { + // A `Configs` variant is emitted whenever the field was present on + // the wire — including the explicit-`null` case, where the inner + // `Option` is `None` to signal "clear". Absent fields produce no + // variant at all, so callers can distinguish all three states. let mut vec = vec![]; - if let Some(tags) = value.tracing_header_tags { - vec.push(Configs::TracingHeaderTags( - tags.into_iter().map(|t| (t.header, t.tag_name)).collect(), - )) + if let Patch(Some(tags)) = value.tracing_header_tags { + vec.push(Configs::TracingHeaderTags(tags.map(|tags| { + tags.into_iter().map(|t| (t.header, t.tag_name)).collect() + }))); } - if let Some(sample_rate) = value.tracing_sampling_rate { + if let Patch(Some(sample_rate)) = value.tracing_sampling_rate { vec.push(Configs::TracingSamplingRate(sample_rate)); } - if let Some(log_injection) = value.log_injection_enabled { + if let Patch(Some(log_injection)) = value.log_injection_enabled { vec.push(Configs::LogInjectionEnabled(log_injection)); } - if let Some(tags) = value.tracing_tags { + if let Patch(Some(tags)) = value.tracing_tags { vec.push(Configs::TracingTags(tags)); } - if let Some(enabled) = value.tracing_enabled { + if let Patch(Some(enabled)) = value.tracing_enabled { vec.push(Configs::TracingEnabled(enabled)); } - if let Some(sampling_rules) = value.tracing_sampling_rules { + if let Patch(Some(sampling_rules)) = value.tracing_sampling_rules { vec.push(Configs::TracingSamplingRules(sampling_rules)); } - if let Some(enabled) = value.dynamic_instrumentation_enabled { + if let Patch(Some(enabled)) = value.dynamic_instrumentation_enabled { vec.push(Configs::DynamicInstrumentationEnabled(enabled)); } - if let Some(enabled) = value.exception_replay_enabled { + if let Patch(Some(enabled)) = value.exception_replay_enabled { vec.push(Configs::ExceptionReplayEnabled(enabled)); } - if let Some(enabled) = value.code_origin_enabled { + if let Patch(Some(enabled)) = value.code_origin_enabled { vec.push(Configs::CodeOriginEnabled(enabled)); } vec } } +/// A single APM_TRACING field that the agent has expressed an opinion about. +/// +/// Each variant carries `Option`: `Some(v)` means "set this field to `v`", +/// `None` means "clear any prior remote override and fall back to local +/// config". A field that was absent on the wire produces no variant at all, +/// so callers can distinguish all three states (absent / clear / set) by +/// matching on presence-in-the-`Vec` and on the inner `Option`. #[derive(Clone)] pub enum Configs { - TracingHeaderTags(HashMap), - TracingSamplingRate(f64), - LogInjectionEnabled(bool), - TracingTags(Vec), // "key:val" format - TracingEnabled(bool), - TracingSamplingRules(Vec), - DynamicInstrumentationEnabled(bool), - ExceptionReplayEnabled(bool), - CodeOriginEnabled(bool), + TracingHeaderTags(Option>), + TracingSamplingRate(Option), + LogInjectionEnabled(Option), + TracingTags(Option>), // inner `Vec` items are "key:val" + TracingEnabled(Option), + TracingSamplingRules(Option>), + DynamicInstrumentationEnabled(Option), + ExceptionReplayEnabled(Option), + CodeOriginEnabled(Option), } pub fn parse_json(data: &[u8]) -> serde_json::error::Result { @@ -152,9 +256,77 @@ pub mod tests { action: "".to_string(), service_target: None, lib_config: DynamicConfig { - tracing_enabled: Some(enabled), + tracing_enabled: Patch(Some(Some(enabled))), ..DynamicConfig::default() }, } } + + #[test] + fn parses_absent_field_as_patch_none() { + let cfg: DynamicConfigFile = parse_json(br#"{"action": "", "lib_config": {}}"#).unwrap(); + assert!(cfg.lib_config.tracing_sampling_rate.is_absent()); + assert!(>::from(cfg.lib_config).is_empty()); + } + + #[test] + fn parses_explicit_null_as_clear_intent() { + let cfg: DynamicConfigFile = + parse_json(br#"{"action": "", "lib_config": {"tracing_sampling_rate": null}}"#) + .unwrap(); + assert_eq!(cfg.lib_config.tracing_sampling_rate, Patch(Some(None))); + let configs: Vec = cfg.lib_config.into(); + assert_eq!(configs.len(), 1); + assert!(matches!(configs[0], Configs::TracingSamplingRate(None))); + } + + #[test] + fn parses_concrete_value_as_set_intent() { + let cfg: DynamicConfigFile = + parse_json(br#"{"action": "", "lib_config": {"tracing_sampling_rate": 0.25}}"#) + .unwrap(); + assert_eq!( + cfg.lib_config.tracing_sampling_rate, + Patch(Some(Some(0.25))) + ); + let configs: Vec = cfg.lib_config.into(); + assert_eq!(configs.len(), 1); + assert!(matches!(configs[0], Configs::TracingSamplingRate(Some(r)) if r == 0.25)); + } + + #[test] + fn unrelated_field_present_does_not_emit_sampling_variants() { + // Regression guard: a payload that updates only `tracing_tags` must + // not surface as a clear for `tracing_sampling_rate` / + // `tracing_sampling_rules` (and vice versa). Each field is + // independently absent / null / set. + let cfg: DynamicConfigFile = + parse_json(br#"{"action": "", "lib_config": {"tracing_tags": ["foo:bar"]}}"#).unwrap(); + let configs: Vec = cfg.lib_config.into(); + assert_eq!(configs.len(), 1); + assert!(matches!(configs[0], Configs::TracingTags(Some(_)))); + assert!(!configs + .iter() + .any(|c| matches!(c, Configs::TracingSamplingRate(_)))); + assert!(!configs + .iter() + .any(|c| matches!(c, Configs::TracingSamplingRules(_)))); + } + + #[test] + fn round_trip_via_test_serialize_preserves_absent_vs_null() { + // The hand-written `Serialize` impl for `DynamicConfig` must skip + // absent fields entirely so the round-trip doesn't collapse + // `Patch(None)` → JSON `null` → `Patch(Some(None))`. + let mut cfg = dummy_dynamic_config(true); + cfg.lib_config.tracing_sampling_rate = Patch(Some(None)); + let json = serde_json::to_string(&cfg).unwrap(); + let parsed: DynamicConfigFile = parse_json(json.as_bytes()).unwrap(); + // absent fields stay absent + assert!(parsed.lib_config.tracing_sampling_rules.is_absent()); + // explicit-null stays explicit-null + assert_eq!(parsed.lib_config.tracing_sampling_rate, Patch(Some(None))); + // set value stays set + assert_eq!(parsed.lib_config.tracing_enabled, Patch(Some(Some(true)))); + } } From 1f9759372698239aadee640a59501b2fed1b7805 Mon Sep 17 00:00:00 2001 From: iunanua Date: Thu, 11 Jun 2026 10:11:53 +0200 Subject: [PATCH 3/4] Add Patch constructors and tracing_sample_rate alias Named constructors (`Patch::absent`/`clear`/`set`) make construction sites self-documenting. The `tracing_sample_rate` alias keeps payloads from legacy producers from being silently dropped as absent after the field rename. Co-Authored-By: Claude Opus 4.7 --- libdd-remote-config/src/config/dynamic.rs | 54 ++++++++++++++--------- 1 file changed, 34 insertions(+), 20 deletions(-) diff --git a/libdd-remote-config/src/config/dynamic.rs b/libdd-remote-config/src/config/dynamic.rs index 7ff57f2934..3d0b3928a3 100644 --- a/libdd-remote-config/src/config/dynamic.rs +++ b/libdd-remote-config/src/config/dynamic.rs @@ -93,6 +93,21 @@ pub struct TracingSamplingRule { pub struct Patch(pub Option>); impl Patch { + /// Field was not delivered on the wire (no change intended). + pub fn absent() -> Self { + Patch(None) + } + + /// Field was delivered as JSON `null` (clear any prior remote override). + pub fn clear() -> Self { + Patch(Some(None)) + } + + /// Field was delivered with a value (set to `v`). + pub fn set(v: T) -> Self { + Patch(Some(Some(v))) + } + /// `true` when the field was not delivered on the wire. pub fn is_absent(&self) -> bool { self.0.is_none() @@ -133,14 +148,14 @@ impl Serialize for Patch { /// Every field is a [`Patch`] so the absent / clear / set distinction /// survives the wire. Struct-level `#[serde(default)]` lets missing fields /// fall back to `Patch::default()` without per-field `deserialize_with` -/// boilerplate. The matching `Serialize` impl (under `feature = "test"`) is -/// hand-written below so absent fields are omitted from the output without -/// needing per-field `skip_serializing_if` annotations. See [`Configs`] for -/// how the three states surface to consumers. +/// boilerplate. #[derive(Debug, Clone, Default, Deserialize)] #[serde(default)] pub struct DynamicConfig { pub(crate) tracing_header_tags: Patch>, + // Accept the legacy `tracing_sample_rate` key so payloads from older + // producers aren't silently treated as absent after the rename. + #[serde(alias = "tracing_sample_rate")] pub(crate) tracing_sampling_rate: Patch, pub(crate) log_injection_enabled: Patch, pub(crate) tracing_tags: Patch>, @@ -158,9 +173,6 @@ impl Serialize for DynamicConfig { // output entirely rather than serializing as JSON `null` — that // collapse would make round-trips re-deserialize absent fields as // explicit clears, defeating the three-state semantics. - // - // `serialize_struct`'s `len` argument is only used by formats that - // pre-allocate (bincode, etc.); JSON ignores it. Pass the maximum. use serde::ser::SerializeStruct; macro_rules! field { ($s:expr, $name:ident) => { @@ -185,10 +197,6 @@ impl Serialize for DynamicConfig { impl From for Vec { fn from(value: DynamicConfig) -> Self { - // A `Configs` variant is emitted whenever the field was present on - // the wire — including the explicit-`null` case, where the inner - // `Option` is `None` to signal "clear". Absent fields produce no - // variant at all, so callers can distinguish all three states. let mut vec = vec![]; if let Patch(Some(tags)) = value.tracing_header_tags { vec.push(Configs::TracingHeaderTags(tags.map(|tags| { @@ -256,7 +264,7 @@ pub mod tests { action: "".to_string(), service_target: None, lib_config: DynamicConfig { - tracing_enabled: Patch(Some(Some(enabled))), + tracing_enabled: Patch::set(enabled), ..DynamicConfig::default() }, } @@ -274,7 +282,7 @@ pub mod tests { let cfg: DynamicConfigFile = parse_json(br#"{"action": "", "lib_config": {"tracing_sampling_rate": null}}"#) .unwrap(); - assert_eq!(cfg.lib_config.tracing_sampling_rate, Patch(Some(None))); + assert_eq!(cfg.lib_config.tracing_sampling_rate, Patch::clear()); let configs: Vec = cfg.lib_config.into(); assert_eq!(configs.len(), 1); assert!(matches!(configs[0], Configs::TracingSamplingRate(None))); @@ -285,10 +293,7 @@ pub mod tests { let cfg: DynamicConfigFile = parse_json(br#"{"action": "", "lib_config": {"tracing_sampling_rate": 0.25}}"#) .unwrap(); - assert_eq!( - cfg.lib_config.tracing_sampling_rate, - Patch(Some(Some(0.25))) - ); + assert_eq!(cfg.lib_config.tracing_sampling_rate, Patch::set(0.25)); let configs: Vec = cfg.lib_config.into(); assert_eq!(configs.len(), 1); assert!(matches!(configs[0], Configs::TracingSamplingRate(Some(r)) if r == 0.25)); @@ -319,14 +324,23 @@ pub mod tests { // absent fields entirely so the round-trip doesn't collapse // `Patch(None)` → JSON `null` → `Patch(Some(None))`. let mut cfg = dummy_dynamic_config(true); - cfg.lib_config.tracing_sampling_rate = Patch(Some(None)); + cfg.lib_config.tracing_sampling_rate = Patch::clear(); let json = serde_json::to_string(&cfg).unwrap(); let parsed: DynamicConfigFile = parse_json(json.as_bytes()).unwrap(); // absent fields stay absent assert!(parsed.lib_config.tracing_sampling_rules.is_absent()); // explicit-null stays explicit-null - assert_eq!(parsed.lib_config.tracing_sampling_rate, Patch(Some(None))); + assert_eq!(parsed.lib_config.tracing_sampling_rate, Patch::clear()); // set value stays set - assert_eq!(parsed.lib_config.tracing_enabled, Patch(Some(Some(true)))); + assert_eq!(parsed.lib_config.tracing_enabled, Patch::set(true)); + } + + #[test] + fn legacy_tracing_sample_rate_alias_still_parses() { + // Older producers may emit the pre-rename key; the alias must keep + // their payloads from being silently treated as absent. + let cfg: DynamicConfigFile = + parse_json(br#"{"action": "", "lib_config": {"tracing_sample_rate": 0.5}}"#).unwrap(); + assert_eq!(cfg.lib_config.tracing_sampling_rate, Patch::set(0.5)); } } From 74dc659b34f756753cc3f60508db785a3bee9d86 Mon Sep 17 00:00:00 2001 From: iunanua Date: Thu, 11 Jun 2026 12:33:22 +0200 Subject: [PATCH 4/4] fix(sidecar): preserve dynamic_instrumentation across non-DI APM_TRACING patches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The shm_remote_config fetched() handler was clearing writer.dynamic_instrumentation up front and rebuilding it only from delivered Configs variants. With JSON Merge Patch semantics, a payload that updates an unrelated field (e.g. tracing_tags) omits dynamic_instrumentation_enabled to mean "no change" — but the take() collapsed that into "clear", triggering a spurious LiveDebugger unforce/force cycle. Keep the prior value and only mutate when a DynamicInstrumentationEnabled variant is actually delivered. Some(_) sets, explicit None (now distinguishable thanks to Patch) clears, absent leaves the value alone. Co-Authored-By: Claude Opus 4.7 --- datadog-sidecar/src/shm_remote_config.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/datadog-sidecar/src/shm_remote_config.rs b/datadog-sidecar/src/shm_remote_config.rs index 118a76d65c..63ceebaa52 100644 --- a/datadog-sidecar/src/shm_remote_config.rs +++ b/datadog-sidecar/src/shm_remote_config.rs @@ -269,7 +269,8 @@ impl MultiTargetHandlers for ConfigFileStora }), }; - let old_dynamic_instrumentation = writer.dynamic_instrumentation.take(); + // Only mutate when a `DynamicInstrumentationEnabled` variant is actually delivered. + let old_dynamic_instrumentation = writer.dynamic_instrumentation; let mut serialized = vec![]; serialized.extend_from_slice(runtime_id.as_bytes());