Use tracing/tracing_subscriber instead of env_logger #1187
Conversation
…and coloring are now specified via an env variable
… parallel; appease clippy; a TODO was added
ImplOfAnImpl
force-pushed
the
use_tracing
branch
from
1d8a081
to
66ddabb
Compare
ImplOfAnImpl
requested review from
muursh,
erubboli,
TheQuantumPhysicist and
azarovh
as code owners
| Action::Ref(call) => { | ||
| worker_tasks.spawn(async move { | ||
| let subsys = subsys.read().await; | ||
| call(&*subsys).await | ||
| }); | ||
| }.in_current_span()); |
There was a problem hiding this comment.
I can see a future where we gradually move away from task::spawn in favour of various task pools and scoped tasks and similar. That means either invoking in_current_span manually like here or coming up with a wrapper to make it convenient (similar to the top-level spawn_in_current_span).
Does it make sense to address it now or should we leave it and deal with it as the need arises?
There was a problem hiding this comment.
I'm not sure how to address this now, because it's not clear to me what those spawn replacements will look like.
Do you have any particular suggestions?
There was a problem hiding this comment.
I'm fine doing it as the need arises and some patterns emerge
| log.workspace = true | ||
| thiserror.workspace = true | ||
| tokio = { workspace = true, default-features = false, features = ["rt", "sync"] } |
There was a problem hiding this comment.
Tokio with logging... what could go wrong...
Just for the record, I hate this... but I think I can't provide a case against it to stop it. I hope I'm wrong.
This supersedes https://git.mintlayer.org/mintlayer/mintlayer-core/-/merge_requests/1149
The usage of
env_loggerwas replaced withtracing/tracing_subscriber.Logging in p2p tests is now initialized globally via the
ctorcrate, instead of doing so explicitly for certain tests only (this selectivity didn't work properly anyway).Calls of the
#[tracing::instrument]macro were put on every test on p2p, so that the test's name is included in the corresponding log messages.For this to work properly, some futures had to be "instrumented" via the call to the
instrumentmethod.The type of the log (json/text) and its coloring can now be selected via a separate env var
ML_LOG_STYLE.The unused "file path" parameter of
init_loggingwas removed.Update: I've moved all trivial changes into a separate PR #1191. Also, I've applied Lukas's comment about wrapping the
.instrumentcalls in a function. For now this function (spawn_in_current_span) is called only in selected places, so that the tracing span info is printed in p2p tests. I'm not sure whether I should use it everywhere instead of the normaltokio::spawncall (for now it won't make any difference, because neither tests of other components nor the production code uses tracing spans, but it still looks a bit inconsistent).