diff --git a/Cargo.lock b/Cargo.lock index fe802c45..f3ea07cf 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2582,6 +2582,7 @@ dependencies = [ "secrecy", "sentry", "serde", + "serde_ignored", "serde_json", "stresstest", "tempfile", @@ -3933,6 +3934,16 @@ dependencies = [ "syn", ] +[[package]] +name = "serde_ignored" +version = "0.1.14" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "115dffd5f3853e06e746965a20dcbae6ee747ae30b543d91b0e089668bb07798" +dependencies = [ + "serde", + "serde_core", +] + [[package]] name = "serde_json" version = "1.0.149" diff --git a/objectstore-server/Cargo.toml b/objectstore-server/Cargo.toml index 89782d98..3d52848f 100644 --- a/objectstore-server/Cargo.toml +++ b/objectstore-server/Cargo.toml @@ -43,6 +43,7 @@ sentry = { workspace = true, features = [ ] } secrecy = { version = "0.10.3", features = ["serde"] } serde = { workspace = true } +serde_ignored = "0.1.14" serde_json = { workspace = true } thiserror = { workspace = true } tokio = { workspace = true, features = ["full"] } diff --git a/objectstore-server/docs/architecture.md b/objectstore-server/docs/architecture.md index eddb38c6..884842b0 100644 --- a/objectstore-server/docs/architecture.md +++ b/objectstore-server/docs/architecture.md @@ -101,6 +101,9 @@ this precedence (highest wins): 3. **Defaults** — sensible development defaults (local filesystem backend, auth disabled) +Unknown merged config keys are ignored and logged as startup warnings after +tracing initialization completes. + Key configuration sections: - `storage` — backend type and connection parameters; use `type: tiered` for two-tier routing with `high_volume` and `long_term` sub-backends diff --git a/objectstore-server/src/config.rs b/objectstore-server/src/config.rs index 11f7338f..fb9d805c 100644 --- a/objectstore-server/src/config.rs +++ b/objectstore-server/src/config.rs @@ -49,6 +49,7 @@ use tracing::level_filters::LevelFilter; pub use objectstore_service::backend::StorageConfig; use crate::killswitches::Killswitches; +use crate::observability; use crate::rate_limits::RateLimits; /// Environment variable prefix for all configuration options. @@ -729,9 +730,16 @@ impl Config { if let Some(path) = path { figment = figment.merge(Yaml::file(path)); } - let config = figment - .merge(Env::prefixed(ENV_PREFIX).split("__")) - .extract()?; + let figment = figment.merge(Env::prefixed(ENV_PREFIX).split("__")); + let config = figment.extract()?; + + let value: figment::value::Value = figment.extract()?; + serde_ignored::deserialize::<_, _, Self>(&value, |unknown_path| { + observability::log_or_buffer( + tracing::Level::WARN, + format!("Ignoring unknown config key `{unknown_path}`"), + ); + })?; Ok(config) } @@ -747,8 +755,10 @@ mod tests { use objectstore_service::backend::HighVolumeStorageConfig; use secrecy::ExposeSecret; + use tracing::Level; use crate::killswitches::Killswitch; + use crate::observability::{TEST_LOG_STATE_LOCK, buffered_logs, reset_buffered_logs}; use crate::rate_limits::{BandwidthLimits, RateLimits, ThroughputLimits, ThroughputRule}; use super::*; @@ -1159,4 +1169,101 @@ mod tests { Ok(()) }); } + + #[test] + fn unknown_yaml_key_buffers_warning() { + let _guard = TEST_LOG_STATE_LOCK.lock().unwrap(); + reset_buffered_logs(); + + let mut tempfile = tempfile::NamedTempFile::new().unwrap(); + tempfile + .write_all( + br#" + http_addr: 127.0.0.1:9001 + http: + max_requests: 123 + typo: ignored + logging: + level: debug + invalid: true + "#, + ) + .unwrap(); + + figment::Jail::expect_with(|_jail| { + let config = Config::load(Some(tempfile.path())).unwrap(); + + assert_eq!(config.http_addr, "127.0.0.1:9001".parse().unwrap()); + assert_eq!(config.http.max_requests, 123); + assert_eq!(config.logging.level, LevelFilter::DEBUG); + + assert_eq!( + buffered_logs(), + vec![ + ( + Level::WARN, + "Ignoring unknown config key `http.typo`".to_string(), + ), + ( + Level::WARN, + "Ignoring unknown config key `logging.invalid`".to_string(), + ), + ] + ); + + Ok(()) + }); + } + + #[test] + fn unknown_env_key_buffers_warning() { + let _guard = TEST_LOG_STATE_LOCK.lock().unwrap(); + reset_buffered_logs(); + + figment::Jail::expect_with(|jail| { + jail.set_env("OS__HTTP_ADDR", "127.0.0.1:9010"); + jail.set_env("OS__HTTP__MAX_REQUESTS", "321"); + jail.set_env("OS__HTTP__UNKNOWN_KEY", "1"); + jail.set_env("OS__LOGGING__LEVEL", "debug"); + jail.set_env("OS__LOGGING__UNKNOWN_KEY", "2"); + + let config = Config::load(None).unwrap(); + + assert_eq!(config.http_addr, "127.0.0.1:9010".parse().unwrap()); + assert_eq!(config.http.max_requests, 321); + assert_eq!(config.logging.level, LevelFilter::DEBUG); + + assert_eq!( + buffered_logs(), + vec![ + ( + Level::WARN, + "Ignoring unknown config key `http.unknown_key`".to_string(), + ), + ( + Level::WARN, + "Ignoring unknown config key `logging.unknown_key`".to_string(), + ), + ] + ); + + Ok(()) + }); + } + + #[test] + fn known_keys_do_not_buffer_warnings() { + let _guard = TEST_LOG_STATE_LOCK.lock().unwrap(); + reset_buffered_logs(); + + figment::Jail::expect_with(|jail| { + jail.set_env("OS__HTTP__MAX_REQUESTS", "123"); + + Config::load(None).unwrap(); + + assert!(buffered_logs().is_empty()); + + Ok(()) + }); + } } diff --git a/objectstore-server/src/observability.rs b/objectstore-server/src/observability.rs index d2cdfb2b..5364675e 100644 --- a/objectstore-server/src/observability.rs +++ b/objectstore-server/src/observability.rs @@ -4,6 +4,9 @@ //! Sentry must be initialized before the Tokio runtime is created so it can //! instrument async tasks from the start. +use std::collections::VecDeque; +use std::sync::{Mutex, OnceLock}; + use secrecy::ExposeSecret; use sentry::integrations::tracing as sentry_tracing; use tracing::Level; @@ -15,6 +18,37 @@ use crate::config::{Config, LogFormat}; /// The full release name including the objectstore version and SHA. const RELEASE: &str = std::env!("OBJECTSTORE_RELEASE"); +#[derive(Clone, Debug, PartialEq, Eq)] +struct BufferedLog { + level: Level, + message: String, +} + +#[derive(Debug, Default)] +struct BufferedLogsState { + tracing_ready: bool, + logs: VecDeque, +} + +static BUFFERED_LOGS_STATE: OnceLock> = OnceLock::new(); + +fn get_buffered_logs_state() -> std::sync::MutexGuard<'static, BufferedLogsState> { + BUFFERED_LOGS_STATE + .get_or_init(|| Mutex::new(BufferedLogsState::default())) + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()) +} + +fn emit_log(level: Level, message: &str) { + match level { + Level::ERROR => tracing::error!("{message}"), + Level::WARN => tracing::warn!("{message}"), + Level::INFO => tracing::info!("{message}"), + Level::DEBUG => tracing::debug!("{message}"), + Level::TRACE => tracing::trace!("{message}"), + } +} + /// Initializes the Sentry error-reporting client, if a DSN is configured. /// /// Returns `None` when `config.sentry.dsn` is not set. The returned @@ -111,6 +145,42 @@ pub fn init_tracing(config: &Config) { .with(sentry_layer) .with(env_filter) .init(); + + flush_buffered_logs(); +} + +/// Emits a log through tracing when ready, or buffers it until tracing is initialized. +pub fn log_or_buffer(level: Level, message: impl Into) { + let buffered_log = BufferedLog { + level, + message: message.into(), + }; + + let ready = { + let mut state = get_buffered_logs_state(); + if state.tracing_ready { + true + } else { + state.logs.push_back(buffered_log.clone()); + false + } + }; + + if ready { + emit_log(buffered_log.level, &buffered_log.message); + } +} + +fn flush_buffered_logs() { + let buffered_logs = { + let mut state = get_buffered_logs_state(); + state.tracing_ready = true; + state.logs.drain(..).collect::>() + }; + + for log in buffered_logs { + emit_log(log.level, &log.message); + } } /// Logs an error to the configured logger or `stderr` if not yet configured. @@ -123,3 +193,21 @@ pub fn ensure_log_error(error: &anyhow::Error) { eprintln!("{error:?}"); } } + +#[cfg(test)] +pub(crate) static TEST_LOG_STATE_LOCK: Mutex<()> = Mutex::new(()); + +#[cfg(test)] +pub(crate) fn reset_buffered_logs() { + let mut state = get_buffered_logs_state(); + *state = BufferedLogsState::default(); +} + +#[cfg(test)] +pub(crate) fn buffered_logs() -> Vec<(Level, String)> { + get_buffered_logs_state() + .logs + .iter() + .map(|log| (log.level, log.message.clone())) + .collect() +}