diff --git a/datadog-sidecar/src/shm_remote_config.rs b/datadog-sidecar/src/shm_remote_config.rs index f0c3022d8f..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()); @@ -305,7 +306,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 +924,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 c0fa77940c..3d0b3928a3 100644 --- a/libdd-remote-config/src/config/dynamic.rs +++ b/libdd-remote-config/src/config/dynamic.rs @@ -76,67 +76,179 @@ 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 { + /// 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() + } +} + +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. +#[derive(Debug, Clone, Default, Deserialize)] +#[serde(default)] pub struct DynamicConfig { - pub(crate) tracing_header_tags: Option>, - pub(crate) tracing_sample_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>, + // 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>, + 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. + 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 { 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_sample_rate { - vec.push(Configs::TracingSampleRate(sample_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), - TracingSampleRate(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 +264,83 @@ pub mod tests { action: "".to_string(), service_target: None, lib_config: DynamicConfig { - tracing_enabled: Some(enabled), + tracing_enabled: Patch::set(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::clear()); + 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::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)); + } + + #[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::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::clear()); + // set value stays set + 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)); + } }