spetz commented on code in PR #4354:
URL: https://github.com/apache/iggy/pull/4354#discussion_r4147377027
##########
core/integration/tests/server/scenarios/log_rotation_scenario.rs:
##########
@@ -126,6 +129,94 @@ async fn log_rotation_should_be_valid(present_log_config:
LogRotationTestConfig)
run(&harness, &log_dir, present_log_config).await;
}
+#[tokio::test]
+#[parallel]
+async fn
given_log_path_existing_in_working_directory_when_server_starts_should_log_under_data_path()
+ {
+ // The server inherits this working directory, so this directory has the
+ // same relative path as the configured log directory.
+ let decoy = tempfile::Builder::new()
+ .prefix("iggy-logs-")
+ .tempdir_in(".")
Review Comment:
The decoy directory is created in the working directory of the test process,
which is `core/integration/` inside the checkout, and `iggy-logs-*` is not
covered by `.gitignore`. `TempDir` cleans up only on drop, so a run that
nextest terminates after its slow timeout, or one stopped with Ctrl+C, leaves
an untracked directory in the source tree.
##########
core/server_common/src/log/settings.rs:
##########
@@ -39,6 +41,67 @@ pub struct LoggingSettings {
pub retention: IggyDuration,
}
+/// `logging.level` in the `RUST_LOG` syntax, parsed more strictly than
+/// `EnvFilter::new` does. `EnvFilter` reads a bare word that is not a level
+/// as a target name and enables only that target, so a typo such as `inof`
+/// turned off every log line, errors included, without a warning.
+#[derive(Debug)]
+pub struct LogFilter(EnvFilter);
+
+#[derive(Debug, thiserror::Error)]
+pub enum LogFilterError {
+ #[error("no filter directive")]
+ Empty,
+
+ #[error(
+ "`{0}` is not a log level (trace, debug, info, warn, error or off);
write `{0}=<level>` to filter one target"
+ )]
+ NotALevel(String),
+
+ #[error(transparent)]
+ InvalidDirective(#[from] ParseError),
+}
+
+/// The 0.8.0 and 0.9.0 `config.toml` listed `none` as a level, but `tracing`
+/// has no such level.
+const NONE_LEVEL_ALIAS: &str = "none";
+const OFF_LEVEL: &str = "off";
+
+impl FromStr for LogFilter {
+ type Err = LogFilterError;
+
+ fn from_str(level: &str) -> Result<Self, Self::Err> {
+ // `EnvFilter` mis-parses a directive with spaces around it.
+ let directives: Vec<&str> = level
+ .split(',')
+ .map(str::trim)
+ .filter(|directive| !directive.is_empty())
+ .map(|directive| {
+ if directive.eq_ignore_ascii_case(NONE_LEVEL_ALIAS) {
+ OFF_LEVEL
+ } else {
+ directive
+ }
+ })
+ .collect();
+ if directives.is_empty() {
+ return Err(LogFilterError::Empty);
+ }
+ if let Some(word) = directives.iter().copied().find(|directive| {
+ !directive.contains(['=', '[']) &&
directive.parse::<LevelFilter>().is_err()
+ }) {
+ return Err(LogFilterError::NotALevel(word.to_owned()));
+ }
+ Ok(Self(EnvFilter::builder().parse(directives.join(","))?))
Review Comment:
This strict parse makes every malformed directive fail the boot, but the
upgrade notes in the commit message list a bare target name as the only value
that no longer boots. `EnvFilter::new` parsed lossily, so a config such as
`level = "info,server=verbose"` printed an "ignoring" line to stderr and
started at `info`. `EnvFilter::builder().parse` returns `InvalidDirective` for
the same value, and the validator in `core/configs/src/common/validators.rs:97`
turns that into a failed boot.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
