fix(logging): add startup and shutdown lifecycle events#2359
Conversation
Mirko-von-Leipzig
left a comment
There was a problem hiding this comment.
I'm unsure how I feel about this tbh. It achieves the goal, but I think we're wading into the territory of the telemetry and logging overshadowing the actual code that I want to see.
Or put differently, I want the majority of the LoC in a function to be actual logic. Not logging or helper functions for it.
Some of this is dictated by instrument! scoping for field recordings, but I feel this is going to be a problem no matter what given tracing API.
We can go ahead; I don't have a better solution.
| tracing::info!( | ||
| target: crate::LOG_TARGET, | ||
| { | ||
| genesis.commitment = %genesis_commitment, | ||
| data.directory = %self.data_directory.display(), | ||
| }, | ||
| "Node bootstrap complete", | ||
| ); |
There was a problem hiding this comment.
I wonder if it would be beneficial to create our own macro; given that we don't want the full tracing feature set.
If we re-arrange the macro order, we can put the fields at the end and remove the need for { } encapsulation?
There was a problem hiding this comment.
Yeah, but then we'd lose the format string capability of the tracing event macros (ie. you can use it like format!). Or maybe that's actually a good thing?
There was a problem hiding this comment.
Perhaps in addition to re-ordering, we could do:
pub trait TracingDisplay {
fn to_string(&self) -> String;
}
// Usage:
miden_tracing::info!(
crate::LOG_TARGET,
"Node bootstrap complete",
genesis.commitment = genesis_commitment,
data.directory = self.data_directory,
);
// Generates
tracing::info!(
target: crate::LOG_TARGET,
{
genesis.commitment = %TracingDisplay::to_string(&genesis_commitment),
data.directory = %TracingDisplayer::to_string(&self.data_directory),
},
"Node bootstrap complete",
);ef2ab3d to
06268f5
Compare
Summary
Partially resolves part 5 of issue #2330
Changelog