added flag for clearing log files at restart #3203
Conversation
Signed-off-by: Aminu 'Seun Joshua <seun.aminujoshua@gmail.com>
crates/trigger/src/cli.rs
Outdated
| @@ -54,6 +55,14 @@ pub struct FactorsTriggerCommand<T: Trigger<B::Factors>, B: RuntimeFactorsBuilde | |||
| )] | |||
| pub log: Option<PathBuf>, | |||
|
|
|||
| /// Whether to refresh logs when restarted. | |||
There was a problem hiding this comment.
I found the word "refresh" a bit odd here, although I can see the connection when I squint. At least in help text, I think we should be more explicit about what the flag does, e.g.
| /// Whether to refresh logs when restarted. | |
| /// If set, Spin deletes the log files before starting the application. |
(or 'clears the log files'? Not sure which is better)
crates/trigger/src/cli.rs
Outdated
| /// Whether to refresh logs when restarted. | ||
| #[clap( | ||
| name = REFRESH_LOGS, | ||
| short = 'r', |
There was a problem hiding this comment.
I wouldn't spend a valuable shortcode on this.
crates/runtime-config/src/lib.rs
Outdated
| @@ -192,6 +198,8 @@ pub struct TomlResolver<'a> { | |||
| state_dir: UserProvidedPath, | |||
| /// Explicitly provided log directory. | |||
| log_dir: UserProvidedPath, | |||
| /// Whether to refresh logs when restarted. | |||
| refresh_logs: bool, | |||
There was a problem hiding this comment.
A bit strange to see this in a struct called TomlResolver - it's not like it has anything to do with resolving TOML. But I guess maybe the log directory is already non-TOML-related and the struct has just grown beyond its original name? Not sure.
There was a problem hiding this comment.
Somewhat, yes.
All but refresh_logs still source the default value from table: TomlKeyTracker<'a>, so the initial name makes sense. However, with other elements added, a change of name would be better, something like RuntimeConfigurationResolver. Would that be fine, or what would you suggest?
There was a problem hiding this comment.
I should have come back to this comment after writing the one about the location of the delete code - sorry.
We should first figure out a good place for the delete code to live, whether in this struct impl or elsewhere. Then that will guide us as to whether this flag needs to live here or not.
crates/runtime-config/src/lib.rs
Outdated
| @@ -268,7 +278,17 @@ impl<'a> TomlResolver<'a> { | |||
| } | |||
|
|
|||
| match log_dir { | |||
| UserProvidedPath::Provided(p) if self.refresh_logs => Ok(Some({ | |||
| let _ = std::fs::remove_dir_all(&p); | |||
There was a problem hiding this comment.
This seems to end up duplicating the check and the deletion code. It seems like something you could do outside the match, if the match successfully resolves a path.
But I see this is in a function whose job is to "get the configured log directory." Getting path to a directory should not have a potential side effect of deleting that directory - that's super surprising behaviour.
(Similarly, notice that this function does not create the directory. It just gets the path.)
So I think this code needs to move somewhere else, at which point hopefully we will know where the log directory is (if we have one) and can clear it without resorting to repeated guards or code.
crates/trigger/src/cli.rs
Outdated
| @@ -54,6 +55,14 @@ pub struct FactorsTriggerCommand<T: Trigger<B::Factors>, B: RuntimeFactorsBuilde | |||
| )] | |||
| pub log: Option<PathBuf>, | |||
|
|
|||
| /// Whether to refresh logs when restarted. | |||
| #[clap( | |||
| name = REFRESH_LOGS, | |||
There was a problem hiding this comment.
Maybe consider a SPIN_ prefix for this? Going by the original issue, some folks will be treating this as a user preference and setting it in .profile rather than on the spin up command line, so it would be good to have it make sense out of the Spin context perhaps?
(sorry forgot to think of this while reviewing)
Signed-off-by: Aminu 'Seun Joshua <seun.aminujoshua@gmail.com>
| @@ -147,6 +143,11 @@ where | |||
|
|
|||
| let toml = toml_resolver.toml(); | |||
| let log_dir = toml_resolver.log_dir()?; | |||
|
|
|||
| if refresh_logs { | |||
| let _ = Self::resolve_log_dir(&log_dir); | |||
There was a problem hiding this comment.
I'm sorry to be a nag but once again this is kind of jamming the delete code in somewhere that yes, it's convenient right now because this code knows what the log directory is, but will be surprising to anyone debugging in a few months. This is the constructor for the runtime configuration. It has nothing to do with creating or deleting directories.
What I really think we should do before diving into writing code is some design:
- What is the desired behaviour of the feature?
- What considerations do we have to think about when implementing the feature?
- What code object(s) are responsible for the behaviour?
Let me give some examples.
- The current behaviour is to delete the logs directory. Suppose, for some reason known only to myself, I set the logs directory to be my application directory (
spin up --log-dir . --refresh-logs). Oops, Spin just deleted my application directory. Should the behaviour be to delete individual log files rather than the whole directory? - The current implementation deletes the directory as part of trigger startup. That means that if I have a multi-trigger application, each trigger will perform a directory delete as it starts. Suppose one trigger fails early, and another trigger starts slowly. The second trigger blew away my logs for the failure.
- The existing log file creation happens in
ComponentStdioWriter, in thespin_trigger::cli::stdiomodule. Does deleting existing logs fit nicely with the existing responsibilities of that type or module? I'm not saying it does, I'm just giving it as an example of a place I'd look at before the runtime-config subsystem.
I don't know if there are other factors to consider because I've not done any design on this, I just feel like this might run smoother if you were gathering feedback on a holistic proposal rather than trying to make local tweaks in response to a stream of comments. That said, this is your project and if you feel more comfortable working at the code level then no worries!
Aims to fix #3130