Unable to dynamically configure with builder

[leshow]

Feature Request

Crates

none

Motivation

Doing anything dynamic can get pretty hairy:

let builder = SubscriberBuilder::builder();
if something {
   builder = builder.json();
} else {
   builder = builder.compact();
}

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 for Format 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.

[hawkw]

Is there a reason why we can't do this? Why consume self for each step in the build.

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 json and compact methods in your example both consume the builder and return a new builder with different values for some of its type parameters, neither of them would work through a mutable reference.

[leshow]

Ah yeah, it looks like some of the configuration is lifted to the type level.

How do you feel about adding a with_format option or something like that? I'd be happy to do it.

[hawkw]

How do you feel about adding a with_format option or something like that? I'd be happy to do it.

https://docs.rs/tracing-subscriber/0.2.0/tracing_subscriber/fmt/struct.SubscriberBuilder.html#method.event_format :)

[leshow]

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();
}

  ^^^^^^^^ expected struct `tracing_subscriber::fmt::format::DefaultFields`, found struct `tracing_subscriber::fmt::format::json::JsonFields`
    |
    = note: expected struct `tracing_subscriber::fmt::SubscriberBuilder<tracing_subscriber::fmt::format::DefaultFields, tracing_subscriber::fmt::format::Format<tracing_subscriber::fmt::format::Full>>`
               found struct `tracing_subscriber::fmt::SubscriberBuilder<tracing_subscriber::fmt::format::json::JsonFields, tracing_subscriber::fmt::format::Format<tracing_subscriber::fmt::format::json::Json>>`

Perhaps I'm misunderstanding, but I don't see how event_format helps, for example:

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 event_format return a different type.

[hawkw]

I would really like to figure out a nicer story for this, especially since there is probably an 0.3 release of tracing-subscriber on the horizon & we will have the opportunity to make breaking changes.

However, there are a few issues that complicate this. First, there is the issue that neither the FormatEvent trait nor the FormatFields trait are currently object-safe, because they are generic. Unfortunately, I think the generics cannot easily be removed. Parameterizing FormatEvent over a Subscriber type is a necessary part of the interface between the Registry and the format layer, as it allows the layer to require that the underlying subscriber provide access to span data. We can't easily pass the subscriber as a trait object for that reason.

Second, there are few places where other code relies on having concrete named types for fmt components. In particular, they are used to allow downcasting from a fmt::Subscriber or fmt::Layer to access the event formatter, writer, or field formatter, here:

tracing/tracing-subscriber/src/fmt/fmt_layer.rs

Lines 495 to 505 in bcb7326

	// This `downcast_raw` impl allows downcasting a `fmt` layer to any of
	// its components (event formatter, field formatter, and `MakeWriter`)
	// as well as to the layer's type itself. The potential use-cases for
	// this *may* be somewhat niche, though...
	match () {
	_ if id == TypeId::of::<Self>() => Some(self as *const Self as *const ()),
	_ if id == TypeId::of::<E>() => Some(&self.fmt_event as *const E as *const ()),
	_ if id == TypeId::of::<N>() => Some(&self.fmt_fields as *const N as *const ()),
	_ if id == TypeId::of::<W>() => Some(&self.make_writer as *const W as *const ()),
	_ => None,
	}

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

	/// A formatted representation of a span's fields stored in its [extensions].
	///
	/// Because `FormattedFields` is generic over the type of the formatter that
	/// produced it, multiple versions of a span's formatted fields can be stored in
	/// the [`Extensions`][extensions] type-map. This means that when multiple
	/// formatters are in use, each can store its own formatted representation
	/// without conflicting.
	///
	/// [extensions]: ../registry/extensions/index.html
	#[derive(Default)]
	pub struct FormattedFields<E> {
	_format_event: PhantomData<fn(E)>,
	/// The formatted fields of a span.
	pub fields: String,
	}

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 fmt::Layer. For example, this makes the tracing-error crate's ErrorLayer much cheaper, because it only needs to format fields if a fmt::Layer doesn't already exist, or if different formatters are in use by the fmt::Layer and the ErrorLayer.

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.

[hawkw]

Actually...while I was writing all that up, I think I came up with a fairly simple and obvious solution. Stay tuned!

[leshow]

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 Subscriber or something. I don't think I fully grasp all of the design decisions in the crate but I'm happy to be your rubber duck and help you come up with something!

[hawkw]

I was under the impression that you had lifted configuration to the type level solely so that you couldn't misconfigure a Subscriber or something.

@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 Layer to be generic over Subscribers, so that Layers can place more trait bounds on the Subscriber as needed if they require additional functionality. Everything being generic rather than trait-object based kind of falls naturally from there.

I'm happy to be your rubber duck and help you come up with something!

The solution I've come up with is adding an erased combinator to Subscribers that allow wrapping them in a Box to get back a type-erased subscriber. So, you could hypothetically write code like this

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 erased type, such as LookupSpan, which is necessary to use an erased::Subscriber with layers that require LookupSpan. I've found a couple of solutions, but they both have downsides: one is a breaking change, and would have to wait for tracing-subscriber v0.3 to be released, and the other would only work with the tracing_subscriber::Registry type (and not other subscribers implementing LookupSpan).

We could probably land the non-breaking version in the near future, and then consider the more general option in 0.3.

[hawkw]

A note on the above: this won't work with cases where you want to continue calling SubscriberBuilder methods on a builder, as in this case that @leshow mentioned in #575 (comment):

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();
}

This is because an erased::Subscriber type would be a subscriber trait object, and not a SubscriberBuilder.

However, this specific example could be fixed fairly easily by just moving all the shared configuration (in this case, the call to with_max_level) before the dynamic configuration (which formatter to use). For example, this code should compile:

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()
    };
    // ...
}

[leshow]

Would that work similarly to how erased-serde works? something like:

trait ErasedSubscriber {
fn erased(&self, sub: &mut dyn Subscriber) -> Result<Ok, Error>;
}

My particular use case is a bunch of configuration values come in at runtime, this is what we have right now to support this:

 fn configure_tracing(&self) -> Result<()> {
        let json_builder = Subscriber::builder();
        let full_builder = Subscriber::builder();
        let log_lvl = self.log_lvl.clone();
        match self.log_frmt.as_str() {
            "json" => {
                let json_builder = json_builder
                    .with_max_level(log_lvl)
                    .event_format(format::Format::default().json());
                tracing::subscriber::set_global_default(json_builder.finish())
            }
            _ => {
                let full_builder = full_builder
                    .with_max_level(log_lvl)
                    .event_format(format::Format::default());
                tracing::subscriber::set_global_default(full_builder.finish())
            }
        }
        .expect("setting default tracing subscriber failed");
        Ok(())
    }

Simplifying to what you've got above would be a big improvement