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.

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

refactor: Switch logging from log + env_logger to tracing + tracing_subscriber #298

Merged
merged 1 commit into from
Jan 29, 2022
Merged

Conversation

huitseeker
Copy link
Contributor

@huitseeker huitseeker commented Jan 28, 2022

Why?

This switches the code base to tokio/tracing.

  1. tracing's notion of Spans allows us to instrument logs with context that lets us make sense of the application's state, even in a concurrent or distributed context. The announcement on the tokio blog is a pretty compact intro to the gist of it.
  2. This gives us access to the tracing ecosystem, see refactor: Switch logging from log + env_logger to tracing + tracing_subscriber narwhal#26 for more details

What

More specifically, this PR transforms the "rails" of logging from using log + event_logger to using tracing, and thereby opens the ability to open Spans or use the #[instrument] macro.

It keeps full backward-compatibility with the prior logs as far as Rust is concerned.

Of course, this is not the hard part. The hard part is to use tracing wisely.

What do we do now? (next steps)

Shave yaks!

pub fn shave_the_yak(yak: &mut Yak) {
    // create and enter a span to represent the scope
    let span = span!(Level::TRACE, "shave_the_yak", ?yak);
    let _enter = span.enter();

    // Since the span is annotated with the yak, it is part of the context
    // for everything happening inside the span. Therefore, we don't need
    // to add it to the message for this event, as the `log` crate does.
    info!(target: "yak_events", "Commencing yak shaving");
    loop {
        match find_a_razor() {
            Ok(razor) => {
                // We can add the razor as a field rather than formatting it
                // as part of the message, allowing subscribers to consume it
                // in a more structured manner:
                info!({ %razor }, "Razor located");
                yak.shave(razor);
                break;
            }
            Err(err) => {
                // However, we can also create events with formatted messages,
                // just as we would for log records.
                warn!("Unable to locate a razor: {}, retrying", err);
            }
        }
    }
}

Caveat

It may mess up the fabric benchmarking files, which I'm no longer sure we maintain. If it does, it would be because of different time printing formats, in particular it's a bit difficult to make tracing produce the wonky timestamps of log. As a consequence the python scripts may need massaging to handle RFC 3339 instead of ISO8601. I don't think it matters at this stage, but YMMV.

…ubscriber

This switches the code base to [tokio/tracing](https://github.com/tokio-rs/tracing).

1. tracing's notion of Spans allows us to instrument logs with context that lets us make sense of the application's state, even in a concurrent of distributed context. The [announcement on the tokio blog](https://tokio.rs/blog/2019-08-tracing) is a pretty compact intro to the gist of it.
2. This gives us access to the tracing ecosystem, see MystenLabs/narwhal#26 for more details

This PR transforms the "rails" of logging from using log + event_logger to using tracing, and thereby opens the ability to open Spans or use the `#[instrument]` macro.

It keeps full backward-compatibility with the prior logs. It may mess up the fabric benchmarking files, which I'm no longer sure we maintain. If it does, it would
be because of different time printing formats, in particular it's a bit difficult to make tracing produce the wonky timestamps of log.

As a consequence the python scripts may need massaging to handle RFC 3339 instead of ISO8601.
@huitseeker huitseeker merged commit 244c0dc into MystenLabs:main Jan 29, 2022
@huitseeker huitseeker mentioned this pull request Apr 15, 2022
mwtian pushed a commit to mwtian/sui that referenced this pull request Sep 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants