-
Notifications
You must be signed in to change notification settings - Fork 715
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
Unable to dynamically configure with builder #575
Comments
Many of the builder methods change a type parameter on the builder; we can't do this through a mutable reference, since these functions return a value of a different type. For example, the |
Ah yeah, it looks like some of the configuration is lifted to the type level. How do you feel about adding a |
|
I made a mistake in my first example you can't actually have this: use tracing::{info, Level};
use tracing_subscriber::fmt::{format, Subscriber};
fn other(cond: bool) {
let b = Subscriber::builder();
if cond {
b = b.json();
} else {
b = b.compact();
}
let b = b.with_max_level(Level::TRACE).finish();
}
Perhaps I'm misunderstanding, but I don't see how fn other(cond: bool) {
let b = Subscriber::builder();
let b = b.event_format(if cond {
format::Format::default().json()
} else {
format::Format::default().compact()
});
let b = b.with_max_level(Level::TRACE).finish();
} This still produces an error, because the branches passed to |
I would really like to figure out a nicer story for this, especially since there is probably an 0.3 release of However, there are a few issues that complicate this. First, there is the issue that neither the Second, there are few places where other code relies on having concrete named types for tracing/tracing-subscriber/src/fmt/fmt_layer.rs Lines 495 to 505 in bcb7326
and by the FormattedFields type, which remembers the type of the formatter that created it, to allow multiple formatters to store formatted fields for the same span without clobbering one-another:tracing/tracing-subscriber/src/fmt/fmt_layer.rs Lines 370 to 384 in 62e0cc0
The second is particularly important, as it allows multiple formatted representations of a span's fields to co-exist, and allows other layers which also need formatted representations of fields to reuse the formatted fields produced by the So, I think it's theoretically possible to fix this, but it might require a significant amount of internal refactoring. I'll give it some more thought. |
Actually...while I was writing all that up, I think I came up with a fairly simple and obvious solution. Stay tuned! |
Just saw this, I was under the impression that you had lifted configuration to the type level solely so that you couldn't misconfigure a |
@leshow that's definitely part of the motivation (e.g. it would be nice if the JSON event formatter makes sure that you are also using the JSON field formatter) but the main motivation is really that we want
The solution I've come up with is adding an use tracing_subscriber::{SubscriberExt, erased};
/// Returns a new `Subscriber` configured to output events in human-readable
/// text format, or as JSON, depending on the provided config.
fn new_subscriber(cfg: &Config) -> erased::Subscriber {
// Shared configuration regardless of whether JSON formatting is used.
let subscriber = tracing_subscriber::fmt()
.with_env_filter(cfg.filter())
.with_ansi(cfg.is_ansi());
// The `erased` combinator allows each branch of this `if` statement
// (and the function itself) to have the same type.
if cfg.is_json() {
subscriber.json().erased()
} else if cfg.is_compact() {
subscriber.compact().erased()
} else {
subscriber.erased()
}
} The downside to this is that it is rather difficult to forward additional trait impls through the We could probably land the non-breaking version in the near future, and then consider the more general option in 0.3. |
A note on the above: this won't work with cases where you want to continue calling
This is because an However, this specific example could be fixed fairly easily by just moving all the shared configuration (in this case, the call to fn other(cond: bool) {
let b = Subscriber::builder()
.with_max_level(Level::TRACE);
let subscriber = if cond {
b.json().finish().erased()
} else {
b.compact().finish().erased()
};
// ...
} |
Would that work similarly to how erased-serde works? something like:
My particular use case is a bunch of configuration values come in at runtime, this is what we have right now to support this:
Simplifying to what you've got above would be a big improvement |
same issue |
Got a ping on this from the last message, thought I would add some more context. There are obvious workarounds to this, but if you want to see how I currently solve it in a real project: https://github.com/leshow/nailgun/blob/master/bin/src/logs.rs#L27 One use case (as above) is if you've got a cli that wants to output json/pretty formatted/etc output depending on some input. I'm actually totally OK with the status quo, because this is the kind of setup that you write once and don't often touch. But at the same time I could see how it could get much worse if you wanted to expose more options at runtime, you could end up with many repetitive branches |
I think my question #2261 has something to do with this problem. Any news on this? |
Your question in #2261 does, in fact, have to do with this issue. I think that the |
Circling back on this, I think
after
so long as you don't need to dynamically create a different number of layers. I think it's safe to mark this as resolved. Thank you! |
Not sure this is exactly the same issue, but I ran into something similar trying to config layers with cargo features and Registry.
Also #2499 may be related |
Feature Request
Crates
none
Motivation
Doing anything dynamic can get pretty hairy:
Proposal
Make the various builders use functions of the form:
fn foo(&mut self, arg: T) -> &mut Self
Alternative
Could accept a
with_x
for every option so at least re-assignment on conditionals could be worked around (there's nothing forFormat
currently)Is there a reason why we can't do this? Why consume
self
for each step in the build.I'm happy to implement this change if it's something you're interested in.
The text was updated successfully, but these errors were encountered: