ZTC-1545: Log a warning when parsing settings if yaml contains unused…

… keys

Cargo.toml

@@ -54,6 +54,7 @@ routerify = "3"
 socket2 = { version = "0.5.3", features = [ "all" ] }
 syn = "1"
 serde = "1"
+serde_ignored = "0.1.10"
 serde_path_to_error = "0.1.15"
 serde_yaml = "0.8.26"
 serde_with = "3.3.0"

foundations/Cargo.toml

@@ -64,6 +64,7 @@ settings = [
     "dep:serde_path_to_error",
     "dep:serde_yaml",
     "dep:serde",
+    "dep:serde_ignored",
     "dep:yaml-merge-keys",
     "dep:indexmap",
 ]
@@ -172,6 +173,7 @@ rand = { workspace = true, optional = true }
 rustracing = { workspace = true, optional = true }
 rustracing_jaeger = { workspace = true, optional = true }
 serde = { workspace = true, optional = true, features = ["derive", "rc"] }
+serde_ignored = { workspace = true, optional = true }
 serde_path_to_error = { workspace = true, optional = true }
 serde_yaml = { workspace = true, optional = true }
 serde_with = { workspace = true, optional = true }

foundations/src/settings/mod.rs

@@ -154,6 +154,8 @@ use std::fs::File;
 use std::io;
 use std::path::Path;
 
+pub use serde_ignored::Path as IgnoredPath;
+
 /// A macro that implements the [`Settings`] trait for a structure or an enum
 /// and turns Rust doc comments into serializable documentation.
 ///
@@ -313,6 +315,28 @@ use std::path::Path;
 /// [`Settings`]: crate::settings::Settings
 pub use foundations_macros::settings;
 
+/// If included at the end of a YAML key and the default [SettingsDeserializationOptions] are used,
+/// any YAML keys ending with this suffix will not be logged if unused.
+///
+/// This can be useful for [YAML key references]
+///
+/// # Examples
+/// When the following YAML is deserialized into a settings struct that has no field corresponding
+/// to `MERGE_ME_ALLOW_UNUSED`, no warning will be logged for `MERGE_ME_ALLOW_UNUSED`.
+/// ```yml
+/// ---
+/// MERGE_ME_ALLOW_UNUSED: &MERGE_ME
+///   a: 4
+///   b: 5
+///
+/// inner:
+///   << : *MERGE_ME
+/// x: 0
+/// ```
+///
+/// [YAML key references]: https://yaml.org/type/merge.html
+pub const YAML_KEY_ALLOW_UNUSED_SUFFIX: &str = "ALLOW_UNUSED";
+
 /// A trait for a YAML-serializable settings with documentation.
 ///
 /// In most cases the trait don't need to be manually implemented and can be generated by the
@@ -461,24 +485,117 @@ pub fn to_yaml_file(settings: &impl Settings, path: impl AsRef<Path>) -> Bootstr
     )?)
 }
 
-/// Parse settings from YAML string.
+/// Builds [SettingsDeserializationOptions] with specified options.
+/// Any left unspecified will use defaults.
+pub struct SettingsDeserializationOptionsBuilder {
+    ignored_key_callback: Option<Box<dyn FnMut(IgnoredPath)>>,
+}
+
+impl SettingsDeserializationOptionsBuilder {
+    /// Initialize the builder without any options specified
+    pub fn new() -> Self {
+        SettingsDeserializationOptionsBuilder {
+            ignored_key_callback: None,
+        }
+    }
+
+    /// Set the callback that's called when YAML keys are not used during deserialization
+    /// into settings.
+    ///
+    /// If unspecified, the default will log each key as a warning, as long as it does not
+    /// end with [YAML_KEY_ALLOW_UNUSED_SUFFIX]. In that case, nothing will be logged.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// use foundations::settings::{IgnoredPath, SettingsDeserializationOptionsBuilder};
+    ///
+    /// let builder = SettingsDeserializationOptionsBuilder::new();
+    /// builder.with_ignored_key_callback(Box::new(|path| {
+    ///     // Do something
+    /// }));
+    ///
+    /// ```
+    pub fn with_ignored_key_callback(mut self, callback: Box<dyn FnMut(IgnoredPath)>) -> Self {
+        self.ignored_key_callback = Some(callback);
+        self
+    }
+
+    /// Creates the configured [SettingsDeserializationOptions]
+    ///
+    /// Any values left unspecified will use default values.
+    pub fn build(self) -> SettingsDeserializationOptions {
+        SettingsDeserializationOptions {
+            ignored_key_callback: self.ignored_key_callback.unwrap_or_else(|| {
+                Box::new(|path| {
+                    #[cfg(feature = "logging")]
+                    if let IgnoredPath::Map { parent: _, key } = &path {
+                        if !key.ends_with(YAML_KEY_ALLOW_UNUSED_SUFFIX) {
+                            crate::telemetry::log::warn!("Unused key in settings"; "path" => %path);
+                        }
+                    }
+                })
+            }),
+        }
+    }
+}
+
+/// Options that modify how YAML is deserialized into settings
+///
+/// Created using a [SettingsDeserializationOptionsBuilder],
+/// which can be obtained with [SettingsDeserializationOptions::builder()]
+pub struct SettingsDeserializationOptions {
+    ignored_key_callback: Box<dyn FnMut(IgnoredPath)>,
+}
+
+impl SettingsDeserializationOptions {
+    /// Returns a default [SettingsDeserializationOptionsBuilder]
+    pub fn builder() -> SettingsDeserializationOptionsBuilder {
+        SettingsDeserializationOptionsBuilder::new()
+    }
+}
+
+/// Parse settings from YAML string with options customizing deserialization behavior.
 ///
 /// Note: [YAML key references] will be merged during parsing.
 ///
+/// Note: If the `logging` feature is enabled, this will log warnings for unused keys by default.
+/// This can be customized by using [from_yaml_str_with_opts] instead.
+///
 /// [YAML key references]: https://yaml.org/type/merge.html
-pub fn from_yaml_str<T: Settings>(data: impl AsRef<str>) -> BootstrapResult<T> {
+pub fn from_yaml_str_with_opts<T: Settings>(
+    data: impl AsRef<str>,
+    mut opts: SettingsDeserializationOptions,
+) -> BootstrapResult<T> {
     let de = serde_yaml::Deserializer::from_str(data.as_ref());
     let value: serde_yaml::Value = serde_path_to_error::deserialize(de)?;
+
     // NOTE: merge dict key refs: https://yaml.org/type/merge.html
     let value = yaml_merge_keys::merge_keys_serde(value)?;
 
-    Ok(serde_path_to_error::deserialize(value)?)
+    let de = serde_ignored::Deserializer::new(value, &mut opts.ignored_key_callback);
+    Ok(serde_path_to_error::deserialize(de)?)
+}
+
+/// Parse settings from YAML string.
+///
+/// Note: [YAML key references] will be merged during parsing.
+///
+/// Note: If the `logging` feature is enabled, this will log warnings for unused keys by default.
+/// This can be customized using [from_yaml_str_with_opts] instead.
+///
+/// [YAML key references]: https://yaml.org/type/merge.html
+pub fn from_yaml_str<T: Settings>(data: impl AsRef<str>) -> BootstrapResult<T> {
+    let options = SettingsDeserializationOptions::builder().build();
+    from_yaml_str_with_opts(data, options)
 }
 
 /// Parse settings from YAML file.
 ///
 /// Note: [YAML key references] will be merged during parsing.
 ///
+/// Note: If the `logging` feature is enabled, this will log warnings for unused keys by default.
+///
 /// [YAML key references]: https://yaml.org/type/merge.html
 pub fn from_file<T: Settings>(path: impl AsRef<Path>) -> BootstrapResult<T> {
     let data = std::fs::read_to_string(path)?;

foundations/src/telemetry/log/init.rs

@@ -29,13 +29,15 @@ type FilteredDrain<D> = LevelFilter<
 
 static HARNESS: OnceCell<LogHarness> = OnceCell::new();
 
-static NOOP_HARNESS: Lazy<LogHarness> = Lazy::new(|| {
-    let root_drain = Arc::new(Discard);
-    let noop_log = Logger::root(Arc::clone(&root_drain), slog::o!());
+static PRE_INIT_HARNESS: Lazy<LogHarness> = Lazy::new(|| {
+    let base_drain = get_base_drain(&LoggingSettings::default())
+        .unwrap_or_else(|_| AsyncDrain::new(Discard).build());
+    let root_drain = Arc::new(base_drain.fuse());
+    let pre_init_log = Logger::root(Arc::clone(&root_drain), slog::o!());
 
     LogHarness {
         root_drain,
-        root_log: Arc::new(parking_lot::RwLock::new(noop_log)),
+        root_log: Arc::new(parking_lot::RwLock::new(pre_init_log)),
         settings: Default::default(),
         log_scope_stack: Default::default(),
     }
@@ -50,7 +52,7 @@ pub(crate) struct LogHarness {
 
 impl LogHarness {
     pub(crate) fn get() -> &'static Self {
-        HARNESS.get().unwrap_or(&NOOP_HARNESS)
+        HARNESS.get().unwrap_or(&PRE_INIT_HARNESS)
     }
 
     #[cfg(any(test, feature = "testing"))]
@@ -66,10 +68,33 @@ pub(crate) fn init(service_info: &ServiceInfo, settings: &LoggingSettings) -> Bo
         return Ok(());
     }
 
+    let base_drain = get_base_drain(settings)?;
+    let root_drain = get_root_drain(settings, Arc::new(base_drain.fuse()));
+    let root_kv = slog::o!(
+        "module" => FnValue(|record| {
+            format!("{}:{}", record.module(), record.line())
+        }),
+        "version" => service_info.version,
+        "pid" => std::process::id(),
+    );
+
+    let root_log = build_log_with_drain(settings, root_kv, Arc::clone(&root_drain));
+    let harness = LogHarness {
+        root_drain,
+        root_log: Arc::new(parking_lot::RwLock::new(root_log)),
+        settings: settings.clone(),
+        log_scope_stack: Default::default(),
+    };
+
+    let _ = HARNESS.set(harness);
+
+    Ok(())
+}
+
+fn get_base_drain(settings: &LoggingSettings) -> Result<AsyncDrain, anyhow::Error> {
     // NOTE: OXY-178, default is 128 (https://docs.rs/slog-async/2.7.0/src/slog_async/lib.rs.html#251)
     const CHANNEL_SIZE: usize = 1024;
-
-    let base_drain = match (&settings.output, &settings.format) {
+    Ok(match (&settings.output, &settings.format) {
         (LogOutput::Terminal, LogFormat::Text) => {
             let drain = TextDrain::new(TermDecorator::new().stdout().build())
                 .build()
@@ -90,28 +115,7 @@ pub(crate) fn init(service_info: &ServiceInfo, settings: &LoggingSettings) -> Bo
             let drain = build_json_log_drain(File::create(file)?);
             AsyncDrain::new(drain).chan_size(CHANNEL_SIZE).build()
         }
-    };
-
-    let root_drain = get_root_drain(settings, Arc::new(base_drain.fuse()));
-    let root_kv = slog::o!(
-        "module" => FnValue(|record| {
-            format!("{}:{}", record.module(), record.line())
-        }),
-        "version" => service_info.version,
-        "pid" => std::process::id(),
-    );
-
-    let root_log = build_log_with_drain(settings, root_kv, Arc::clone(&root_drain));
-    let harness = LogHarness {
-        root_drain,
-        root_log: Arc::new(parking_lot::RwLock::new(root_log)),
-        settings: settings.clone(),
-        log_scope_stack: Default::default(),
-    };
-
-    let _ = HARNESS.set(harness);
-
-    Ok(())
+    })
 }
 
 fn get_root_drain(

foundations/tests/data/settings_nested_struct_using_merge_keys.yaml

@@ -0,0 +1,10 @@
+---
+MERGE_ME_ALLOW_UNUSED: &MERGE_ME
+  a: 4
+  b: 5
+  c: 6
+  d: "extraneous"
+
+inner:
+  << : *MERGE_ME
+x: 0

foundations/tests/settings.rs

@@ -1,8 +1,13 @@
 use foundations::settings::collections::Map;
 use foundations::settings::net::SocketAddr;
-use foundations::settings::{settings, to_yaml_string};
+use foundations::settings::{from_yaml_str, settings, to_yaml_string};
+use foundations::telemetry::log::TestLogRecord;
+use foundations::telemetry::TestTelemetryContext;
+use foundations_macros::with_test_telemetry;
+use slog::Level;
 
 #[settings]
+#[derive(PartialEq)]
 struct NestedStruct {
     /// A field, which is named the same as another field.
     a: usize,
@@ -14,13 +19,20 @@ struct NestedStruct {
     c: u32,
 }
 
+#[settings]
+struct NestedStructMissingB {
+    a: usize,
+    c: u32,
+}
+
 impl NestedStruct {
     fn default_b() -> u32 {
         0xb
     }
 }
 
 #[settings]
+#[derive(PartialEq)]
 struct SimpleStruct {
     /// The documentation of NestedStruct
     /// will be added to the keys of `inner`
@@ -29,6 +41,12 @@ struct SimpleStruct {
     x: u32,
 }
 
+#[settings]
+struct SimpleStructWithInnerMissingB {
+    inner: NestedStructMissingB,
+    x: u32,
+}
+
 #[settings]
 struct NestedDup {
     inner: NestedStruct,
@@ -256,3 +274,49 @@ fn vec() {
 
     assert_ser_eq!(s, "data/with_vec.yaml");
 }
+
+#[test]
+fn deserialize_from_yaml_str() {
+    let deserialized: SimpleStruct =
+        from_yaml_str(include_str!("data/settings_nested_struct.yaml")).unwrap();
+
+    assert_eq!(
+        SimpleStruct {
+            inner: NestedStruct { a: 0, b: 11, c: 0 },
+            x: 0,
+        },
+        deserialized
+    );
+}
+
+#[with_test_telemetry(test)]
+fn warns_on_unused_key(ctx: TestTelemetryContext) {
+    let _: SimpleStructWithInnerMissingB =
+        from_yaml_str(include_str!("data/settings_nested_struct.yaml")).unwrap();
+
+    assert_eq!(
+        *ctx.log_records(),
+        vec![TestLogRecord {
+            level: Level::Warning,
+            message: "Unused key in settings".into(),
+            fields: vec![("path".into(), "inner.b".into())]
+        }]
+    );
+}
+
+#[with_test_telemetry(test)]
+fn warns_on_unused_key_inside_merge_key(ctx: TestTelemetryContext) {
+    let _: SimpleStruct = from_yaml_str(include_str!(
+        "data/settings_nested_struct_using_merge_keys.yaml"
+    ))
+    .unwrap();
+
+    assert_eq!(
+        *ctx.log_records(),
+        vec![TestLogRecord {
+            level: Level::Warning,
+            message: "Unused key in settings".into(),
+            fields: vec![("path".into(), "inner.d".into())]
+        }]
+    )
+}