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

docs: Remove misleading comment about RUST_LOG #2994

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

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 hawkw, davidbarsky and a team 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.

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
3 participants