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

Add DebugLevel, TraceLevel, OffLevel #122

Closed
joshka opened this issue Nov 11, 2024 · 4 comments · Fixed by #123
Closed

Add DebugLevel, TraceLevel, OffLevel #122

joshka opened this issue Nov 11, 2024 · 4 comments · Fixed by #123
Labels
enhancement Improve the expected

Comments

@joshka
Copy link
Contributor

joshka commented Nov 11, 2024

Per request on #95 (which seems stalled), I'm creating another issue about DebugLevel, TraceLevel, OffLevel.

In #80, the rationale for not including these was:

  • Debug and trace by definition are below the threshold for normal operations and we should encourage best practices of defaults being for normal operations
  • Abnormal cases where debug and trace become normal operations that people can workaround it the above by providing their own LogLevel impl

Is there a reason we should re-think these assumptions? Otherwise, I lean to closing this with the specified workaround.

I responded to #80 after it was closed, but my response got no response. Here's a simplified version of that (and a longer one at the end.

Yes, I believe that there are sound reasons to re-think the above assumptions due to the following counterexamples:

  • During development / prototyping, I'd like to easily switch my default to use debug / trace / off, so that i can easily run the CLI. Development / prototyping is a normal operation, so the notion that there is a "best practice" there is an incorrect assumption. It might be ok if there was some security related aspect of this, but choosing a log level is an intentional opt-in thing.
  • Having one way for Info/Error and another for Debug/Trace/Off is annoying from a symmetry perspective
  • Actually implementing this a) looking up how to implement the trait, b) actually implementing some throwaway code rather than a one liner which is built-in, c) deleting / disabling / marking the code with clippy lints when the custom log level is not being used.
  • Not having these levels (and other things where this crate has pushed back against what seem like reasonable features) has lead to other crates which do that are named similarly (clap-verbosity, clap-verbosity-flag, clap-verbosity-flag2)

For a specific data point on this. I've used this in 8 different CLIs and this problem has come up in each of them. Often it's things like the data that I care about during prototyping / development is at DEBUG/TRACE level (e.g. the exact details of some web requests)

Given that we both seem to be coming from the perspective of whether this is reasonable, If after reading the above you still feel like this should not be part of the library, I think it would be helpful to share a bit more information about the problems you see with this being a normal part of the lib.

Previous message in response to #80

Hey, @epage I saw this got a little heated above, and while I don't want to stir that up again, I'd like to throw a use case into the mix. If you'd prefer me to open another issue rather than continuing this one, or comment in #95 instead, I'm happy to do that.

TL;DR: This got a little long while writing, but I'm not sure what to cut out. Apologies in advance for the long read:

  • I'd find the extra log levels useful, especially during prototyping and for situations where for a small amount of time setting the default level is an easier approach to debug some problem at a more verbose default log level.
  • Not doing this has caused confusing fragmentation that seems worse than the doing it

During prototyping a cli app, it would be nice and let me fall into the pit of success to have a default level of debug available to me. This would make it easy for me to have http requests show up in my logs and various things like that. Once I'm done prototyping this can certainly be switched back to the default.

If I'm writing a few different CLIs for various things, this additional boilerplate code to set a default that is not available seems heavy. It ends up getting duplicated across a few places, and it's effectively throw away code that I want to delete when it's no longer in use. But I also want the ability to turn that back on if needs be.

So given these concerns:

  • wanting a simple one line setup while prototyping to avoid writing much code for simple things
  • wanting the ability to change this simply in a symmetric way that aligns with changing from ERROR to INFO as the default
  • not wanting to have to dig into the implementation of this to work out which parts to implement

I come to the same conclusion that including the other levels is the right thing based on those assumptions.

Right now there are 3 similarly named crates. The changes aren't documented, but I think they're:

  • clap-verbosity-flag this lib
  • clap-verbosity a fork which adds serde support
  • clap-verbosity-flag2 a fork which adds the extra levels

As a consumer looking to simplify a verbosity flag in a cli app, this is a confusing place to be in. I want both of those extra features, but I also want to generally use software which I know will get regularly maintained rather than being a single issue fork.

I think much of what I wrote seems to align with the arguments that @SUPERCILEX was making here.

This follows the principle of API design of make the right / safe choice easy while making the exception cases possible

I think this way to define how an API should choose features has some problems, as it overindexes on what is "correct". I don't think there is a correct universally accepted log level that's most useful for all use cases. For some use cases, that level is DEBUG, for some it's ERROR. I don't particularly know any specific use cases where logging at a TRACE level by default makes sense, but I can see that it's reasonable to want that.

Here, there seems like there's a reasonable default (ERROR) that aligns with a default that's used in many logging frameworks, and that works well enough, but not providing the other levels seems just odd and the sort of thing that only has downside.

Perhaps there's a specific use case that you're thinking of that's unstated here where defaulting the verbosity flag level to DEBUG / TRACE would cause known issues? I wonder if you could expand on those a bit more, because those seem like the more theoretical problems to me (but it's likely I don't have the same perspective as yours, so I'm seeking to understand yours a bit more here too).

@epage
Copy link
Member

epage commented Nov 11, 2024

During development / prototyping, I'd like to easily switch my default to use debug / trace / off, so that i can easily run the CLI. Development / prototyping is a normal operation, so the notion that there is a "best practice" there is an incorrect assumption. It might be ok if there was some security related aspect of this, but choosing a log level is an intentional opt-in thing.

So if I'm understanding correctly, this is the only documented use case for this feature, right?

Before anything else, thank you for including a use case!

With a prototype where you want all of the gory details, you can change the levels on the log messages, or change the log level. Understandable to want to change it in as few places as possible.

The workaround for this situation is to pass in the log level flags. I'm assuming the concern there is in always remember and type out the flags? A cargo alias could help with this (cargo trace or cargo trun that runs cargo run -- -vvv).

I suspect I'm missing something about your workflow because I'm having a hard time putting myself in your shoes for wanting to develop this way. For myself, in developing an application, I write at in a production style from the beginning. I can maybe see it more for a prototype but I feel like I could quickly get overwhelmed with the output, masking what I'm looking for on regular runs. This also doesn't scale to new feature development as I would assume you wouldn't want to change the default down to debug and then bring it back up again. As for feature prototyping, again, this makes more sense but with an existing application it would be even more overwhelming to see everything logged on every run.

@joshka
Copy link
Contributor Author

joshka commented Nov 12, 2024

So if I'm understanding correctly, this is the only documented use case for this feature, right?

Yeah, but then Debug / Trace logs really are something that you'd usually only need for development purposes, so it's kind of a tautology that these would be only used while developing / prototyping / bug bashing. My comment about security was alluding to config options which allow self signed certs. You definitely don't want that on in prod, but you also definitely want to be able to use it when you don't want the overhead of doing the full TLS cert thing for dev. This feels similar (but without any security impact).

I suspect I'm missing something about your workflow because I'm having a hard time putting myself in your shoes for wanting to develop this way

I suspect that the part you're missing is that often command line projects are a useful tool as a discovery / diagnosis mechanism. Their purpose is not to live forever in a production state, but are instead used as throwaway code, which serves as:

  • spiking a new feature isolated from the rest of the code
  • learning how a new library acts for implementing a feature in a larger app
  • isolating a minimal repro of a bug
  • running "one off" batch jobs that need details about what went on
  • etc.

Often the ability to discover the internals of how some crate works is only surfaced through logging / tracing. I have many throwaway clis for various things like this.

There's also the power of defaults. I can hit Cmd+R, Enter from anywhere in a cli project, and rust-analyzer will run the main command without parameters. I implemented this change in rust-lang/rust-analyzer#16972. AFAIK, there's not a way to override the args sent to just the cargo run at a workspace level.

This is my general workflow

cargo new --bin foo
code foo
cargo add clap clap-verbosity-flag --features clap/derive
cargo add tracing tracing-subscriber
cargo add some-other-lib 
  • write a main, derive an args parser
  • run the app a bit, realize that for some period of time I need debug level logs while I'm iterating on some feature because some library I'm testing doesn't do what I'd anticipated, or there's some distributed weirdness to debug

Now my options are:

  • just skip clap-verbosity filter in the logging setup
  • Add an EnvFilter to my logging setup and change my run command to RUST_LOG=trace
  • pass in a -v param every time I re-run the app after making changes
  • Add a new temporary clap_verbosity_flag::LogLevel implementation
  • A one line temporary change Verbosity<TraceLevel> which is what I'm advocating for here.

I feel like I could quickly get overwhelmed with the output, masking what I'm looking for on regular runs.

Taking a drink from the firehose isn't something I do for very long either, but it's something that often I want to do before winding back the information that is coming my way (e.g. turning of things in hyper / h2 / etc. on my tracing config).

Some concrete examples:

  • a client / server cli / tui for a recent Ratatui lab that was presented at a conf, where it was useful to show the debug while developing some of the the tools, but once released Info was more useful level
  • a throwaway tool I was using to diagnose and understand where various http libs hit overload and why
  • an experiment with building some tooling around axum / askama / sqlx to generically solve some common scaffolding stuff
  • a prototype where I was interested to understand how exactly certain things mapped onto sqlite calls underneath a library because I was working at the edge of the supported feature set

All of these were cases of use debug / trace level for some time, but in the end leave it as some higher level.

@epage
Copy link
Member

epage commented Nov 12, 2024

I suspect that the part you're missing is that often command line projects are a useful tool as a discovery / diagnosis mechanism. Their purpose is not to live forever in a production state, but are instead used as throwaway code, which serves as:

So to put a different way, this is less for development of a production application but true prototypes / exploratory programming.

There's also the power of defaults. I can hit Cmd+R, Enter from anywhere in a cli project, and rust-analyzer will run the main command without parameters. I implemented this change in rust-lang/rust-analyzer#16972. AFAIK, there's not a way to override the args sent to just the cargo run at a workspace level.

I always overlook these as I don't "invest" in the use of LSPs.

The cases you listed seem reasonable. I'm ok with this moving forward. Thank you for taking the time to help me understand! If nothing else, something that came from this is me better understanding how people are using my crates so I can make better decisions in the future!

@epage epage added the enhancement Improve the expected label Nov 12, 2024
@joshka
Copy link
Contributor Author

joshka commented Nov 12, 2024

So to put a different way, this is less for development of a production application but true prototypes / exploratory programming.

Yeah, most of my Rust programming is non-production related, so the lens I look at these things has quite a different tint.

The cases you listed seem reasonable. I'm ok with this moving forward. Thank you for taking the time to help me understand! If nothing else, something that came from this is me better understanding how people are using my crates so I can make better decisions in the future!

Thanks for making all the crates and investing the time to understand use cases that are a bit different from the norm. I appreciate your time in both. I'll do up a PR soon.

joshka added a commit to joshka/clap-verbosity-flag that referenced this issue Nov 13, 2024
This adds `OffLevel`, `DebugLevel`, and `TraceLevel` to the list of log
levels that can be set as the default log level for the logger.

A new example, `log_level.rs`, demonstrates how to set the default log
level for Verbosity to something other than the default. It can be run
with `cargo run --example=log_level [default level] [flags]`. E.g.,:
`cargo run --example=log_level off -vvvv` will set the default log level
to `OffLevel` and then apply the verbosity flags to set the log level to
`Debug`.

Fixes: clap-rs#122
joshka added a commit to joshka/clap-verbosity-flag that referenced this issue Nov 14, 2024
This adds `OffLevel`, `DebugLevel`, and `TraceLevel` to the list of log
levels that can be set as the default log level for the logger.

Fixes: clap-rs#122
joshka added a commit to joshka/clap-verbosity-flag that referenced this issue Nov 14, 2024
joshka added a commit to joshka/clap-verbosity-flag that referenced this issue Nov 14, 2024
joshka added a commit to joshka/clap-verbosity-flag that referenced this issue Nov 14, 2024
@epage epage closed this as completed in 6e5fc6a Nov 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improve the expected
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants