Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add random 6-character suffix to log file names #1041

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Add random 6-character suffix to log file names #1041

wants to merge 3 commits into from
+84 −48

Conversation

dannycjones
Copy link
Contributor

@dannycjones dannycjones commented Sep 30, 2024 •
edited
Loading

Description of change

Before this change, Mountpoint can write to the same log file if multiple Mountpoint processes spawn within the same second. This can cause log entries to be interleaved and reduces the effectiveness of debug logging since its impossible to tell which log entries relate to which Mountpoint process.

This change updates the log file creation logic to now include a six-character suffix between the timestamp and the .log suffix. An example of a new log file name: mountpoint-s3-2024-10-02T10-26-57Z.961grx.log.

We update the log config logic so that it can be defined before the fork, and then persists after fork. We know this is safe as it contains no state unsafe to share over forks.

This change also updates the logging documentation to indicate that log file names could always change in the future and shouldn't be considered stable. It also clarifies this behavior, since customers may only infrequently run into this scenario.

Relevant issues: N/A

Alternatives considered

  • Include PID in log entries.
    • This would add additional bytes/noise to all logs to cover a case which for most customers will not occur.
    • In background mode, what is 'the PID of Mountpoint'? It should probably be the child process ID.
  • Include subseconds in the file name.
    • This one doesn't really have much advantage over always include the PID, and still leaves us open to the (hopefully far less likely) possibility of two Mountpoints spawning in the same subsecond.

Does this change impact existing behavior?

Yes. We now include a random suffix in the name of log files.

Before: mountpoint-s3-2024-10-02T10-26-57Z.log
After: mountpoint-s3-2024-10-02T10-26-57Z.961grx.log

Does this change need a changelog entry in any of the crates?

Yes, the main Mountpoint change log has been updated.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and I agree to the terms of the Developer Certificate of Origin (DCO).

@dannycjones dannycjones marked this pull request as draft September 30, 2024 20:37
@dannycjones
Copy link
Contributor Author

I was just testing this, it doesn't cover running in the background yet.

@dannycjones dannycjones changed the title Add PID to log file names if log file already exists Add random 6-character suffix to log file names Oct 2, 2024
dannycjones
dannycjones commented Oct 2, 2024
View reviewed changes
mountpoint-s3/src/cli.rs
Comment on lines +480 to +509
/// Generates a logging configuration based on the CLI arguments.
///
/// This includes random string generation which can change with each invocation,
/// so once created the [LoggingConfig] should be cloned if another owned copy is required.
fn logging_config(cli_args: &CliArgs) -> LoggingConfig {
let default_filter = if cli_args.no_log {
String::from("off")
} else {
let mut filter = if cli_args.debug {
String::from("debug")
} else {
String::from("warn")
};
let crt_verbosity = if cli_args.debug_crt { "debug" } else { "off" };
filter.push_str(&format!(",{}={}", AWSCRT_LOG_TARGET, crt_verbosity));
if cli_args.log_metrics {
filter.push_str(&format!(",{}=info", metrics::TARGET_NAME));
}
filter
};

let log_file = cli_args.log_directory.as_ref().map(|dir| prepare_log_file_name(dir));

LoggingConfig {
log_file,
log_to_stdout: cli_args.foreground,
default_filter,
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've moved this out of the impl CliArgs because it produces a different return value each time (due to randomness). I hope moving it out makes this clearer (along with RustDoc).

@dannycjones dannycjones marked this pull request as ready for review October 2, 2024 10:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@dannycjones