subscriber: add lifetime parameter to MakeWriter

Motivation ---------- Currently, the `tracing-subscriber` crate has the `MakeWriter` trait for customizing the io writer used by `fmt`. This trait is necessary (rather than simply using a `Write` instance) because the default implementation performs the IO on the thread where an event was recorded, meaning that a separate writer needs to be acquired by each thread (either by calling a function like `io::stdout`, by locking a shared `Write` instance, etc). Right now there is a blanket impl for `Fn() -> T where T: Write`. This works fine with functions like `io::stdout`. However, the _other_ common case for this trait is locking a shared writer. Therefore, it makes sense to see an implementation like this: ``` rust impl<'a, W: io::Write> MakeWriter for Mutex<W> where W: io::Write, { type Writer = MutexWriter<'a, W>; fn make_writer(&self) -> Self::Writer { MutexWriter(self.lock().unwrap()) } } pub struct MutexWriter<'a, W>(MutexGuard<'a, W>); impl<W: io::Write> io::Write for MutexWriter<'_, W> { // write to the shared writer in the `MutexGuard`... } ``` Unfortunately, it's impossible to write this. Since `MakeWriter` always takes an `&self` parameter and returns `Self::Writer`, the generic parameter is unbounded: ``` Checking tracing-subscriber v0.2.4 (/home/eliza/code/tracing/tracing-subscriber) error[E0207]: the lifetime parameter `'a` is not constrained by the impl trait, self type, or predicates --> tracing-subscriber/src/fmt/writer.rs:61:6 | 61 | impl<'a, W: io::Write> MakeWriter for Mutex<W> | ^^ unconstrained lifetime parameter error: aborting due to previous error ``` This essentially precludes any `MakeWriter` impl where the writer is borrowed from the type implementing `MakeWriter`. This is a significant blow to the usefulness of the trait. For example, it prevented the use of `MakeWriter` in `tracing-flame` as suggested in #631 (comment). Proposal -------- This PR changes `MakeWriter` to be generic over a lifetime `'a`: ```rust pub trait MakeWriter<'a> { type Writer: io::Write; fn make_writer(&'a self) -> Self::Writer; } ``` The `self` parameter is now borrowed for the `&'a` lifetime, so it is okay to return a writer borrowed from `self`, such as in the `Mutex` case. I've also added an impl of `MakeWriter` for `Mutex<T> where T: Writer`. Unfortunately, this is a breaking change and will need to wait until we release `tracing-subscriber` 0.3. Fixes #675. Signed-off-by: Eliza Weisman <[email protected]>
tokio-rs · Jul 6, 2020 · 7e3c0ab · 7e3c0ab
1 parent 71b4efd
commit 7e3c0ab
Show file tree

Hide file tree

Showing 13 changed files with 311 additions and 287 deletions.
diff --git a/examples/Cargo.toml b/examples/Cargo.toml
@@ -15,7 +15,7 @@ tracing-core = { path = "../tracing-core", version = "0.1"}
 tracing-error = { path = "../tracing-error" }
 tracing-flame = { path = "../tracing-flame" }
 tracing-tower = { version = "0.1.0", path = "../tracing-tower" }
-tracing-subscriber = { path = "../tracing-subscriber", version = "0.2.7", features = ["json", "chrono"] }
+tracing-subscriber = { path = "../tracing-subscriber", version = "0.3", features = ["json", "chrono"] }
 tracing-futures = { version = "0.2.1", path = "../tracing-futures", features = ["futures-01"] }
 tracing-attributes =  { path = "../tracing-attributes", version = "0.1.2"}
 tracing-log = { path = "../tracing-log", version = "0.1.1", features = ["env_logger"] }

diff --git a/tracing-appender/Cargo.toml b/tracing-appender/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name = "tracing-appender"
-version = "0.1.0"
+version = "0.2.0"
 authors = [
     "Zeki Sherif <[email protected]>",
     "Tokio Contributors <[email protected]>"
@@ -25,7 +25,7 @@ chrono = "0.4.11"
 
 [dependencies.tracing-subscriber]
 path = "../tracing-subscriber"
-version = "0.2.7"
+version = "0.3"
 default-features = false
 features = ["fmt"]
 

diff --git a/tracing-appender/src/non_blocking.rs b/tracing-appender/src/non_blocking.rs
@@ -236,10 +236,10 @@ impl std::io::Write for NonBlocking {
     }
 }
 
-impl MakeWriter for NonBlocking {
+impl<'a> MakeWriter<'a> for NonBlocking {
     type Writer = NonBlocking;
 
-    fn make_writer(&self) -> Self::Writer {
+    fn make_writer(&'a self) -> Self::Writer {
         self.clone()
     }
 }

diff --git a/tracing-error/Cargo.toml b/tracing-error/Cargo.toml
@@ -38,7 +38,7 @@ default = ["traced-error"]
 traced-error = []
 
 [dependencies]
-tracing-subscriber = { path = "../tracing-subscriber", version = "0.2.7", default-features = false, features = ["registry", "fmt"] }
+tracing-subscriber = { path = "../tracing-subscriber", version = ">= 0.2.4, < 0.4", default-features = false, features = ["registry", "fmt"] }
 tracing = { path = "../tracing", version = "0.1.12"}
 
 [badges]

diff --git a/tracing-flame/Cargo.toml b/tracing-flame/Cargo.toml
@@ -25,7 +25,7 @@ default = ["smallvec"]
 smallvec = ["tracing-subscriber/smallvec"]
 
 [dependencies]
-tracing-subscriber = { path = "../tracing-subscriber", version = "0.2.3", default-features = false, features = ["registry", "fmt"] }
+tracing-subscriber = { path = "../tracing-subscriber", version = "0.3", default-features = false, features = ["registry", "fmt"] }
 tracing = { path = "../tracing", version = "0.1.12"}
 lazy_static = "1.3.0"
 

diff --git a/tracing-journald/Cargo.toml b/tracing-journald/Cargo.toml
@@ -16,4 +16,4 @@ keywords = ["tracing", "journald"]
 
 [dependencies]
 tracing-core = { path = "../tracing-core", version = "0.1.10" }
-tracing-subscriber = { path = "../tracing-subscriber", version = "0.2.5" }
+tracing-subscriber = { path = "../tracing-subscriber", version = ">= 0.2.5, < 0.4" }
diff --git a/tracing-opentelemetry/Cargo.toml b/tracing-opentelemetry/Cargo.toml
@@ -26,7 +26,7 @@ opentelemetry = { version = "0.6", default-features = false, features = ["trace"
 rand = "0.7"
 tracing = { path = "../tracing", version = "0.1" }
 tracing-core = { path = "../tracing-core", version = "0.1" }
-tracing-subscriber = { path = "../tracing-subscriber", version = "0.2", default-features = false, features = ["registry"] }
+tracing-subscriber = { path = "../tracing-subscriber", version = ">= 0.2, < 0.4", default-features = false, features = ["registry"] }
 tracing-log = { path = "../tracing-log", version = "0.1", default-features = false, optional = true }
 
 [dev-dependencies]

diff --git a/tracing-subscriber/Cargo.toml b/tracing-subscriber/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name = "tracing-subscriber"
-version = "0.2.7"
+version = "0.3.0"
 authors = [
     "Eliza Weisman <[email protected]>",
     "David Barsky <[email protected]>",

diff --git a/tracing-subscriber/src/fmt/fmt_layer.rs b/tracing-subscriber/src/fmt/fmt_layer.rs
@@ -110,7 +110,7 @@ impl<S, N, E, W> Layer<S, N, E, W>
 where
     S: Subscriber + for<'a> LookupSpan<'a>,
     N: for<'writer> FormatFields<'writer> + 'static,
-    W: MakeWriter + 'static,
+    W: for<'writer> MakeWriter<'writer> + 'static,
 {
     /// Sets the [event formatter][`FormatEvent`] that the layer will use to
     /// format events.
@@ -150,7 +150,7 @@ where
 
 // This needs to be a seperate impl block because they place different bounds on the type paramaters.
 impl<S, N, E, W> Layer<S, N, E, W> {
-    /// Sets the [`MakeWriter`] that the [`Layer`] being built will use to write events.
+    /// Sets the [`for<'writer> MakeWriter<'writer>`] that the [`Layer`] being built will use to write events.
     ///
     /// # Examples
     ///
@@ -167,11 +167,11 @@ impl<S, N, E, W> Layer<S, N, E, W> {
     /// # let _ = layer.with_subscriber(tracing_subscriber::registry::Registry::default());
     /// ```
     ///
-    /// [`MakeWriter`]: ../fmt/trait.MakeWriter.html
+    /// [`for<'writer> MakeWriter<'writer>`]: ../fmt/trait.for<'writer> MakeWriter<'writer>.html
     /// [`Layer`]: ../layer/trait.Layer.html
     pub fn with_writer<W2>(self, make_writer: W2) -> Layer<S, N, E, W2>
     where
-        W2: MakeWriter + 'static,
+        W2: for<'writer> MakeWriter<'writer> + 'static,
     {
         Layer {
             fmt_fields: self.fmt_fields,
@@ -376,7 +376,7 @@ where
     S: Subscriber + for<'a> LookupSpan<'a>,
     N: for<'writer> FormatFields<'writer> + 'static,
     E: FormatEvent<S, N> + 'static,
-    W: MakeWriter + 'static,
+    W: for<'writer> MakeWriter<'writer> + 'static,
 {
     /// Builds a [`Layer`] with the provided configuration.
     ///
@@ -407,7 +407,7 @@ where
     S: Subscriber + for<'a> LookupSpan<'a>,
     N: for<'writer> FormatFields<'writer> + 'static,
     E: FormatEvent<S, N> + 'static,
-    W: MakeWriter + 'static,
+    W: for<'writer> MakeWriter<'writer> + 'static,
 {
     #[inline]
     fn make_ctx<'a>(&'a self, ctx: Context<'a, S>) -> FmtContext<'a, S, N> {
@@ -488,7 +488,7 @@ where
     S: Subscriber + for<'a> LookupSpan<'a>,
     N: for<'writer> FormatFields<'writer> + 'static,
     E: FormatEvent<S, N> + 'static,
-    W: MakeWriter + 'static,
+    W: for<'writer> MakeWriter<'writer> + 'static,
 {
     fn new_span(&self, attrs: &Attributes<'_>, id: &Id, ctx: Context<'_, S>) {
         let span = ctx.span(id).expect("Span not found, this is a bug");
@@ -649,7 +649,7 @@ where
 
     unsafe fn downcast_raw(&self, id: TypeId) -> Option<*const ()> {
         // This `downcast_raw` impl allows downcasting a `fmt` layer to any of
-        // its components (event formatter, field formatter, and `MakeWriter`)
+        // its components (event formatter, field formatter, and `for<'writer> MakeWriter<'writer>`)
         // as well as to the layer's type itself. The potential use-cases for
         // this *may* be somewhat niche, though...
         match () {
@@ -732,15 +732,12 @@ mod test {
         self,
         format::{self, test::MockTime, Format},
         layer::Layer as _,
-        test::MockWriter,
+        test::MockMakeWriter,
         time,
     };
     use crate::Registry;
     use format::FmtSpan;
-    use lazy_static::lazy_static;
     use regex::Regex;
-    use std::sync::Mutex;
-    use tracing::subscriber::with_default;
     use tracing_core::dispatcher::Dispatch;
 
     #[test]
@@ -794,135 +791,87 @@ mod test {
         re.replace_all(s.as_str(), "timing").to_string()
     }
 
-    #[test]
-    fn synthesize_span_none() {
-        lazy_static! {
-            static ref BUF: Mutex<Vec<u8>> = Mutex::new(vec![]);
-        }
-
-        let make_writer = || MockWriter::new(&BUF);
+    fn test_synthesize(which: FmtSpan, expected: &str) {
+        let make_writer = MockMakeWriter::default();
         let subscriber = crate::fmt::Subscriber::builder()
-            .with_writer(make_writer)
+            .with_writer(make_writer.clone())
             .with_level(false)
             .with_ansi(false)
             .with_timer(MockTime)
-            // check that FmtSpan::NONE is the default
+            .with_span_events(which)
             .finish();
+        test_synthesize_subscriber(subscriber, make_writer, expected)
+    }
 
-        with_default(subscriber, || {
+    fn test_synthesize_subscriber(
+        subscriber: impl Into<Dispatch>,
+        buf: MockMakeWriter,
+        expected: &str,
+    ) {
+        tracing::dispatcher::with_default(&subscriber.into(), || {
             let span1 = tracing::info_span!("span1", x = 42);
             let _e = span1.enter();
         });
-        let actual = sanitize_timings(String::from_utf8(BUF.try_lock().unwrap().to_vec()).unwrap());
-        assert_eq!("", actual.as_str());
+        let actual = sanitize_timings(String::from_utf8(buf.buf().to_vec()).unwrap());
+        assert_eq!(expected, actual.as_str());
     }
 
     #[test]
-    fn synthesize_span_active() {
-        lazy_static! {
-            static ref BUF: Mutex<Vec<u8>> = Mutex::new(vec![]);
-        }
-
-        let make_writer = || MockWriter::new(&BUF);
+    fn synthesize_span_none() {
+        let make_writer = MockMakeWriter::default();
         let subscriber = crate::fmt::Subscriber::builder()
-            .with_writer(make_writer)
+            .with_writer(make_writer.clone())
             .with_level(false)
             .with_ansi(false)
             .with_timer(MockTime)
-            .with_span_events(FmtSpan::ACTIVE)
+            // check that FmtSpan::NONE is the default
             .finish();
 
-        with_default(subscriber, || {
-            let span1 = tracing::info_span!("span1", x = 42);
-            let _e = span1.enter();
-        });
-        let actual = sanitize_timings(String::from_utf8(BUF.try_lock().unwrap().to_vec()).unwrap());
-        assert_eq!(
+        test_synthesize_subscriber(subscriber, make_writer, "")
+    }
+
+    #[test]
+    fn synthesize_span_active() {
+        test_synthesize(
+            FmtSpan::ACTIVE,
             "fake time span1{x=42}: tracing_subscriber::fmt::fmt_layer::test: enter\n\
              fake time span1{x=42}: tracing_subscriber::fmt::fmt_layer::test: exit\n",
-            actual.as_str()
         );
     }
 
     #[test]
     fn synthesize_span_close() {
-        lazy_static! {
-            static ref BUF: Mutex<Vec<u8>> = Mutex::new(vec![]);
-        }
-
-        let make_writer = || MockWriter::new(&BUF);
-        let subscriber = crate::fmt::Subscriber::builder()
-            .with_writer(make_writer)
-            .with_level(false)
-            .with_ansi(false)
-            .with_timer(MockTime)
-            .with_span_events(FmtSpan::CLOSE)
-            .finish();
-
-        with_default(subscriber, || {
-            let span1 = tracing::info_span!("span1", x = 42);
-            let _e = span1.enter();
-        });
-        let actual = sanitize_timings(String::from_utf8(BUF.try_lock().unwrap().to_vec()).unwrap());
-        assert_eq!(
+        test_synthesize(FmtSpan::CLOSE,
             "fake time span1{x=42}: tracing_subscriber::fmt::fmt_layer::test: close timing timing\n",
-            actual.as_str()
         );
     }
 
     #[test]
     fn synthesize_span_close_no_timing() {
-        lazy_static! {
-            static ref BUF: Mutex<Vec<u8>> = Mutex::new(vec![]);
-        }
-
-        let make_writer = || MockWriter::new(&BUF);
+        let make_writer = MockMakeWriter::default();
         let subscriber = crate::fmt::Subscriber::builder()
-            .with_writer(make_writer)
+            .with_writer(make_writer.clone())
             .with_level(false)
             .with_ansi(false)
             .with_timer(MockTime)
             .without_time()
             .with_span_events(FmtSpan::CLOSE)
             .finish();
 
-        with_default(subscriber, || {
-            let span1 = tracing::info_span!("span1", x = 42);
-            let _e = span1.enter();
-        });
-        let actual = sanitize_timings(String::from_utf8(BUF.try_lock().unwrap().to_vec()).unwrap());
-        assert_eq!(
+        test_synthesize_subscriber(
+            subscriber,
+            make_writer,
             " span1{x=42}: tracing_subscriber::fmt::fmt_layer::test: close\n",
-            actual.as_str()
         );
     }
 
     #[test]
     fn synthesize_span_full() {
-        lazy_static! {
-            static ref BUF: Mutex<Vec<u8>> = Mutex::new(vec![]);
-        }
-
-        let make_writer = || MockWriter::new(&BUF);
-        let subscriber = crate::fmt::Subscriber::builder()
-            .with_writer(make_writer)
-            .with_level(false)
-            .with_ansi(false)
-            .with_timer(MockTime)
-            .with_span_events(FmtSpan::FULL)
-            .finish();
-
-        with_default(subscriber, || {
-            let span1 = tracing::info_span!("span1", x = 42);
-            let _e = span1.enter();
-        });
-        let actual = sanitize_timings(String::from_utf8(BUF.try_lock().unwrap().to_vec()).unwrap());
-        assert_eq!(
+        test_synthesize(FmtSpan::FULL,
             "fake time span1{x=42}: tracing_subscriber::fmt::fmt_layer::test: new\n\
              fake time span1{x=42}: tracing_subscriber::fmt::fmt_layer::test: enter\n\
              fake time span1{x=42}: tracing_subscriber::fmt::fmt_layer::test: exit\n\
              fake time span1{x=42}: tracing_subscriber::fmt::fmt_layer::test: close timing timing\n",
-            actual.as_str()
         );
     }
 }