-
Notifications
You must be signed in to change notification settings - Fork 24
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 optional support for tracing
besides log
#37
Conversation
As an optional feature, add support for getting the verbosity level as a `tracing::Level` to pass to Tokio's `tracing` library, very similar to the existing support for getting the verbosity level as a `log::Level`. A `tracing` equivalent of the existing `Verbosity::log_level_filter` method easily could be added as well: ```rust /// Get the log level filter, as defined by the `tracing` crate. pub fn tracing_level_filter(&self) -> tracing::level_filters::LevelFilter { self.tracing_level().into() } ``` However, *that* method's body is so simple that I question whether adding it would be worth your potentially needing to update it if `tracing`, for example, moves the `LevelFilter` type out of that `level_filters` module.
@@ -30,6 +30,7 @@ codecov = { repository = "rust-cli/clap-verbosity-flag" } | |||
[dependencies] | |||
log = "0.4.1" | |||
clap = { version = "3.0", default-features = false, features = ["std", "derive"] } | |||
tracing = { version = "0.1", default-features = false, optional = true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How mature / stable is tracing for being a 0.1
crate? We just went 1.0 and I don't want to repeatedly bump by taking on a potentially volatile dependency
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How mature / stable is tracing for being a
0.1
crate?
I'm only a very new user of tracing
, so I don't know. On one hand, I would guess that tracing::Level
is too basic to be likely to disappear. On the other hand, in hindsight, considering the crates' very different version numbers, I do think it would be more appropriate for me to ask tracing
to add the necessary conversion code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On one hand, I would guess that tracing::Level is too basic to be likely to disappear.
My concern is not with it disappearing but that when a tracing 0.2 comes out, to support it, we'd have to release clap-verbosity-flag 2.0. Ditto for each breaking release of tracing, regardless of whether the type looks the same. Rust will consider tracing::Level
from 0.1, 0.2, 0.3, etc all to be different, incompatible types.
@@ -30,6 +30,7 @@ codecov = { repository = "rust-cli/clap-verbosity-flag" } | |||
[dependencies] | |||
log = "0.4.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do tracing users generally feel about still requiring log
? If a breaking change is involved in the future, should we allow disabling it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do tracing users generally feel about still requiring
log
?
I'm only a very new user of tracing
, so I don't know, but I note that the main tracing
README suggests that applications use the crate tracing_subscriber
, which already, as a default feature, depends on log
and consumes log
messages, and the main tracing
crate also interfaces with log
as a non-default feature.
I'm sure some users would consider the cost in compilation time of log
to be too much, but would they be using clap-derive
?
/// | ||
/// `None` means all output is disabled. | ||
#[cfg(feature = "tracing")] | ||
pub fn tracing_level(&self) -> Option<tracing::Level> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For tracing, is it more appropriate to return Level
or LevelFilter
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be clear, I'm not saying "also" but "instead of"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They're very easily convertible to each other, and the equivalent of filter_level
accepts both, so I don't think it matters much. I chose tracing::Level
over tracing::level_filters::LevelFilter
because I guessed that, if either's path were to change in the future, it more likely would be the latter's.
I hadn't thought of that. That thought convinces me against proposing to add this support in this library, rather than proposing to add a conversion from |
FYI tokio/tracing maintain an (unstable) crate called tracing-log for the conversion between |
#39 at least added an example of integrating with |
Issue raised for further discussion: #121 |
As an optional feature, add support for getting the verbosity level as
a
tracing::Level
to pass to Tokio'stracing
library, very similarto the existing support for getting the verbosity level as a
log::Level
.A
tracing
equivalent of the existingVerbosity::log_level_filter
method easily could be added as well:
However, that method's body is so simple that I question whether
adding it would be worth your potentially needing to update it if
tracing
, for example, moves theLevelFilter
type out of thatlevel_filters
module.