Skip to content

Conversation

abatilo
Copy link

@abatilo abatilo commented Jun 2, 2024

This comment describes a behavior that's not true. Unless you configure
an EnvFilter, the global collector will not respect RUST_LOG.

I've opted to remove the comment since the surrounding context is only
the minimal intro of the crate, instead of adding more information that
requires more context right at the introduction of the crate.

Closes #2993

This comment describes a behavior that's not true. Unless you configure
an `EnvFilter`, the global collector will not respect `RUST_LOG`.

I've opted to remove the comment since the surrounding context is only
the minimal intro of the crate, instead of adding more information that
requires more context right at the introduction of the crate.

Closes tokio-rs#2993
@abatilo abatilo requested review from a team, davidbarsky and hawkw as code owners June 2, 2024 23:25
@abatilo abatilo changed the title docs: Remove misleading comment docs: Remove misleading comment about RUST_LOG Jun 2, 2024
@davidbarsky
Copy link
Member

Thanks for the PR. The issue underlying issue is that tracing_subscriber::fmt::init() is not equivalent to tracing_subscriber::fmt().init(). The former does RUST_LOG-based filtering while the latter does not. This is pretty confusing and we should remove the latter.

@abatilo
Copy link
Author

abatilo commented Jun 3, 2024

Oh goodness. 🤦 Thank you for pointing that out. With the former syntax, is there a way to still instantiate things like the json formatter while keeping the init one "line"?

@davidbarsky
Copy link
Member

Before anything else, I should emphasize that I'm sorry that this was your early/first experience with Rust: it took me several minutes of fiddling in a local repo to figure out what was happening and my name is in the authors array of this crate!

Oh goodness. 🤦 Thank you for pointing that out. With the former syntax, is there a way to still instantiate things like the json formatter while keeping the init one "line"?

The short answer is "not really"; the longer answer is "depends on if you're okay with chaining some method calls". To expand on the latter, both EnvFilter or Targets will work, but using Targets will make you do the same sort of environment variable reading/parsing that the free functions init/try_init do under the hood.

Stepping back, I think we'd want to:

  • keep the free functions that the README.md references, but remove init/try_init() that come from SubscriberInitExt and/or remove init/try_init from SubscriberBuilder. Removing SubscriberBuilder` entirely is also an option!
  • Ultimately, while I think we provided some nice one-liners for getting started with tracing-subscriber, we failed at providing a reasonable/understandable dialectical ratchet for users of this crate. I think that we should be more focused on centering this more low-level interface, but it's an interface that is substantially more clear about what's happening under the hood and more scalable to support other layers.
    • As an example as to why I'm considering this option, I'd like to move the JSON formatter out of tracing-subscriber into its own crate because there isn't one JSON format: there are many, and I think we're better off creating a "JSON logger kit" with a few pre-built options rather than overloading tracing-subscriber as we do now.

@abatilo As a person who just ran into this issue/new to Rust, what's your initial gut feeling? Is removing some of these .init() methods/combinators a step too far? Is writing tracing_subscriber::registry().with(fmt::layer()).with(filter).init() annoying/fraying your attention span when you just want something to be logged?

@abatilo
Copy link
Author

abatilo commented Jun 3, 2024

I appreciate the warm welcome and extremely fast responses from contributors to this repo! The frustration at least turned into a great learning experience.

My recent background is almost entirely in Go where JSON structured logging seems more common, relatively speaking.

For what it's worth, here's how I instantiate my favorite logging library with a bare minimum:

log := zerolog.New(os.Stdout).With().Timestamp().Logger()

There is no default/built in ability to source a logging level from an environment variable as far as I know. We're left to implement that ourselves.

With my biases in consideration, I actually think I was just more confused than anything that there appears to be so many ways to setup a logger. I don't personally mind one bit if the instantiation is very verbose, but having more of a golden path would be quite nice.

Let me know if that doesn't quite answer your questions though, but if I'm understanding you correctly, I would be in favor of removing the init methods and keep the longer approach if it means more uniformity.

@davidbarsky
Copy link
Member

My recent background is almost entirely in Go where JSON structured logging seems more common, relatively speaking.

👍

For what it's worth, here's how I instantiate my favorite logging library with a bare minimum:

log := zerolog.New(os.Stdout).With().Timestamp().Logger()

There is no default/built in ability to source a logging level from an environment variable as far as I know. We're left to implement that ourselves.

With my biases in consideration, I actually think I was just more confused than anything that there appears to be so many ways to setup a logger. I don't personally mind one bit if the instantiation is very verbose, but having more of a golden path would be quite nice.

That did answer my question, thank you! I too would prefer a single, golden path for setting up a subscriber/logger: we've let this garden of options grow to be a little untended and confusing. I just wasn't sure if there's a point at which you (or any other user!) would find the code to be way too noisy and bounce off entirely. It seems to me that we're not at that threshold yet.

@asvrada
Copy link

asvrada commented Jun 4, 2024

Is removing some of these .init() methods/combinators a step too far? Is writing tracing_subscriber::registry().with(fmt::layer()).with(filter).init() annoying/fraying your attention span when you just want something to be logged?

I want to throw in my 2 cents after using this crate. (BTW thanks for the quality work).

I would prefer if there were two options to initialize tracing

  1. Have something like ::registry().with(fmt::layer()).with(filter).init() that provides functionalities to configure however you want
  2. Provide a SINGLE default configuration with the most commonly used features, like EnvFilter, and output in normal format with ANSI. Ideally, this is implemented using option 1 so it's merely a shortcut, not an entirely new way of doing things.

So normal non-power users or new users, who just want to get things started, could use Option 2 to quickly get it working. Later if requirements change, they could replace with option 1 with more control over the process.

@kaffarell
Copy link
Contributor

Anyway we could start by maybe deprecating tracing_subscriber::fmt().init() in the next version. We should also add a note that one should use the fmt::init() function and that it checks RUST_LOG. What do you guys think?

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.

Leveled logging is ignoring RUST_LOG

4 participants