subscriber: add an abstraction for building visitors #241

hawkw · 2019-08-05T22:22:52Z

Motivation

The tracing-fmt crate crate currently uses a NewVisitor trait to
abstract over constructing field visitors of different types or with
different configurations:

tracing/tracing-fmt/src/lib.rs

Lines 192 to 196 in d51d4d6

    
           pub trait NewVisitor<'a> { 
        
               type Visitor: field::Visit + 'a; 
        
               fn make(&self, writer: &'a mut dyn fmt::Write, is_empty: bool) -> Self::Visitor; 
        
           }

There have been other use-cases where having this abstraction seems
valuable. For example, issue #73 is a proposal for adding utilities for
wrapping a visitor to skip a set of excluded field names. A MakeVisitor
abstraction would allow us to provide much more ergonomic APIs for
composing this kind of visitor behaviour.

Additionally, the current trait in tracing-fmt is tied pretty closely
to `tracing-fmt- specific behaviour. A generalized version of this trait
should be generic over the input type used to build the visitor.

Solution

This PR adds a new MakeVisitor trait in tracing-subscriber.
MakeVisitor generalizes the NewVisitor trait in tracing-fmt to
represent constructing visitors for arbitrary targets. In addition, I've
added some additional traits representing common patterns for visitors,
such as those that format to a fmt::Write instance or write to an IO
stream, or those that produce output once a set of fields have been
visited. I've also added some extension traits & combinators to make
composing visitors easier, and to demonstrate that the trait in this
branch can be used for implementing this kind of wrapper.

I've also rewritten the tracing-fmt crate to use the new
tracing-subscriber::MakeVisitor trait rather than its homegrown
fmt-specifc version.

Closes: #240

Signed-off-by: Eliza Weisman [email protected]

bIgBV · 2019-08-06T00:48:27Z

tracing-subscriber/src/field/delimited.rs

+}
+
+#[derive(Debug)]
+pub struct VisitDelimited<D, V> {


So if I am understanding this correctly, this would mean that you'd simply have to instantiate a VisitDelimited for a new visitor, without having to wrap the Visitor construction in a separate type?

bIgBV · 2019-08-06T00:49:16Z

tracing-subscriber/src/field/delimited.rs

+
+    fn record_debug(&mut self, field: &Field, value: &dyn fmt::Debug) {
+        self.delimit();
+        self.inner.record_debug(field, value);


How are the errors from the delegation to inner's being caught? Am I missing something?

The record_$TYPE functions don't return errors, so if the inner visitor is also fallible, it's expected to capture any errors that might occur. The wrapper's finish method will call finish on the inner visitor, so if the inner visitor also tracks an error, it will be returned if the wrapper hasn't also failed.

hawkw · 2019-08-16T21:31:48Z

I've spent some time playing around with this, and I think it's about ready for review. Please let me know what you think!

KodrAus · 2019-08-22T05:32:24Z

tracing-fmt/src/format.rs

+pub trait FormatFields<'writer> {
+    fn format_fields<R: RecordFields>(
+        &self,
+        writer: &'writer mut dyn fmt::Write,


Is there a reason to be generic over the fields but not over the writer?

Yes, the writer is the MakeVisitor trait's target type, which the trait is generic over. This means that we would have to make the FormatFields trait (rather than the function) generic over a writer, and thus also make the FmtSubscriber itself generic over a writer. This won't work since we will need to pass multiple writer types in (sometimes a String, and sometimes an io stream), so we need to use a trait object to erase the concrete type. On the other hand, the RecordFields is only a parameter of the method, so implementations of FormatFields can't implement the trait only for specific implementors of RecordFields. I thought it was better to avoid the overhead of a trait object or that since it's not strictly required.

We could make the method generic over both a writer and a RecordFields, but the writer will eventually become a trait object if we use it to create a visitor. That might, actually, be better, as implementations that don't use the blanket impl for MakeVisitor could avoid a trait object...

In the near future, the `tracing-subscriber` crate should be ready for a stable 0.1 release. Before we can do that, we should do some pre-release polish, such as denying warnings, updating idioms, and adding missing documentation. This branch performs most of the necessary polish for `tracing-subscriber`. Please note the following: * The next release will not be version 0.1, as PR #241 would be a breaking change, and I'd like to land that first. We'll probably do an alpha.4 first. * I didn't add `deny(warnings)`, as that will also deny deprecation warnings, and I've had bad experiences with deprecations breaking CI in the past. Instead, I've denied most other warnings explicitly. The downside to this is a *very* long list of deny attributes. * The docs added in this branch are pretty rough. It would be good to expand them some more in the future, hopefully before 0.1. Signed-off-by: Eliza Weisman <[email protected]>

bIgBV · 2019-09-10T04:42:05Z

tracing-subscriber/src/field/delimited.rs

+    fn delimit(&mut self)
+    where
+        V: VisitFmt,
+        D: AsRef<str>,


Not really a review comment, but more a Rust question, but why not lift these bounds to the impl block instead of specifying it on the method?

bIgBV

This looks great! I left some general nits. Mostly around things that I felt weren't really clear.

bIgBV · 2019-09-10T04:43:49Z

tracing-subscriber/src/field/delimited.rs

+}
+
+/// A visitor wrapper that inserts a delimiter after the wrapped visitor formats
+/// a field value.


Maybe expand on this comment saying something along the lines of "The delimiter is inserted after it is visited the first time"?

Right now you will have to separately see the impl of the delimit method and where it's being called from to understand how it fits together.

bIgBV · 2019-09-10T04:47:44Z

tracing-subscriber/src/field/display.rs

+where
+    V: Visit,
+{
+    #[inline]


Again more a rust question than anything, but won't llvm automatically inline such small methods when at opt-level=2 ?

bIgBV · 2019-09-10T04:54:42Z

tracing-subscriber/src/field/mod.rs

+}
+/// Extension trait implemented by visitors to indicate that they write to a
+/// `fmt::Write` instance, and allow access to that writer.
+pub trait VisitFmt: VisitOutput<fmt::Result> {


I was initially confused by the VisitFmt name as the trait only has the writer method on it. But goddamn do I love how this abstraction separates the two writers and still brings it all together. It's awesome!

bIgBV · 2019-09-10T05:03:56Z

tracing-subscriber/src/fmt/mod.rs

+    ///     .finish();
+    /// # drop(subscriber)
+    /// ```
+    pub fn fmt_fields<N2>(self, fmt_fields: N2) -> Builder<N2, E, F, W>


I know naming is a hard problem but maybe have a different variable for the generic? N and N2 are a little confusing as the NewVisitor type has been removed right?

hawkw · 2019-09-12T17:22:56Z

tracing-subscriber/src/field/mod.rs

+}
+/// Extension trait implemented by visitors to indicate that they write to a
+/// `fmt::Write` instance, and allow access to that writer.
+pub trait VisitFmt: VisitOutput<fmt::Result> {


Something I'm a bit on the fence about that I'd love to get some opinions on: We could make the VisitFmt traits trait aliases for VisitOutput<fmt::Result> + AsMut<dyn fmt::Write>; that way, any type implementing AsMut could also get a blanket implementation for free. On the other hand, it means introducing a trait alias.

It seems to me that the trait alias for VisitOutput<fmt::Result> + AsMut<dyn fmt::Write> could be added in non-backwards incompatible change, so I wouldn't gate on this.

davidbarsky · 2019-09-25T16:29:46Z

This looks good to me; the build is failing on missing docs + missing debug impls: https://dev.azure.com/tracing/tracing/_build/results?buildId=721&view=logs&j=84c7964f-8fe4-5ae5-6e5b-8951524b2943&t=a6204e26-b351-5262-402a-ed02abe9e163&l=443.

I think with that + resolved conflicts, this could merged?

Signed-off-by: Eliza Weisman <[email protected]>

hawkw · 2019-10-08T20:56:38Z

@davidbarsky @LucioFranco this is just about ready to merge, if either of you wanted to take a final look first.

davidbarsky · 2019-10-08T20:58:14Z

@hawkw no issues with merging this as is.

bIgBV reviewed Aug 6, 2019

View reviewed changes

hawkw force-pushed the eliza/new-visitor branch from 7c7f741 to 81acb32 Compare August 15, 2019 20:53

hawkw changed the title ~~[WIP] first pass on MakeVisitor abstraction~~ subscriber: add an abstraction for building visitors Aug 15, 2019

hawkw marked this pull request as ready for review August 16, 2019 21:26

hawkw requested review from LucioFranco, davidbarsky and KodrAus August 16, 2019 21:26

hawkw added crate/subscriber Related to the `tracing-subscriber` crate kind/feature New feature or request labels Aug 16, 2019

KodrAus reviewed Aug 22, 2019

View reviewed changes

hawkw mentioned this pull request Aug 25, 2019

fmt: support customizing output stream #299

Merged

6 tasks

hawkw requested a review from a team August 30, 2019 20:53

hawkw mentioned this pull request Sep 2, 2019

subscriber: pre-0.1 polish pass #312

Merged

hawkw added this to the tracing-subscriber 0.1 milestone Sep 2, 2019

hawkw removed this from the tracing-subscriber 0.1 milestone Sep 4, 2019

hawkw force-pushed the eliza/new-visitor branch from 780898d to 5000d31 Compare September 5, 2019 00:12

bIgBV reviewed Sep 10, 2019

View reviewed changes

hawkw commented Sep 12, 2019

View reviewed changes

davidbarsky approved these changes Sep 25, 2019

View reviewed changes

hawkw force-pushed the eliza/new-visitor branch 2 times, most recently from ed365c3 to 3d2ff32 Compare October 7, 2019 16:44

hawkw added 4 commits October 8, 2019 10:07

first pass on MakeVisitor abstraction

f914ad1

Signed-off-by: Eliza Weisman <[email protected]>

properly seal recordfields

c756e23

Signed-off-by: Eliza Weisman <[email protected]>

add some docs, cleanup

f5eb605

Signed-off-by: Eliza Weisman <[email protected]>

docs wip

3e0acd5

Signed-off-by: Eliza Weisman <[email protected]>

hawkw added 11 commits October 8, 2019 10:07

docs/cleanup

136544f

Signed-off-by: Eliza Weisman <[email protected]>

WIP rewrite tracing-fmt to use MakeVisitor

4bbf867

Signed-off-by: Eliza Weisman <[email protected]>

rewrite fmt to use new interfaces

edb386a

Signed-off-by: Eliza Weisman <[email protected]>

fn adapter that works a little nicer

e3aef8b

Signed-off-by: Eliza Weisman <[email protected]>

examples 'n' stuff

50b6248

Signed-off-by: Eliza Weisman <[email protected]>

fix imports in tests

c93f1bf

Signed-off-by: Eliza Weisman <[email protected]>

subscriber: add debug alt formatting wrapper

81646ea

Signed-off-by: Eliza Weisman <[email protected]>

post rebase fixup

b3d30f4

Signed-off-by: Eliza Weisman <[email protected]>

(wip) start fixing lints

3a1fe43

Signed-off-by: Eliza Weisman <[email protected]>

post-rebase fixy

7b92080

Signed-off-by: Eliza Weisman <[email protected]>

add missing Debug impls

6bf2d24

Signed-off-by: Eliza Weisman <[email protected]>

hawkw mentioned this pull request Oct 8, 2019

Ignore rate_limit_secs tracing attribute for rate limited logs vectordotdev/vector#989

Closed

hawkw force-pushed the eliza/new-visitor branch from a30fc0f to 6bf2d24 Compare October 8, 2019 17:19

hawkw and others added 8 commits October 8, 2019 12:01

add docs to debug_alt

167d174

Signed-off-by: Eliza Weisman <[email protected]>

fix failing doc test

395a679

fix fmt issue

a2be200

add missing docs

b97af4c

Signed-off-by: Eliza Weisman <[email protected]>

rename default visitor

a9f5576

Signed-off-by: Eliza Weisman <[email protected]>

fix broken docs links

dc8bd81

Signed-off-by: Eliza Weisman <[email protected]>

tweaks/fixes

55ac42b

Signed-off-by: Eliza Weisman <[email protected]>

rustfmt

95a889e

Signed-off-by: Eliza Weisman <[email protected]>

hawkw merged commit 06daa95 into master Oct 8, 2019

This was referenced Oct 8, 2019

tracing-subscriber: JSON formatter #218

Closed

Utility for filtering fields at the visitor level #73

Open

davidbarsky mentioned this pull request Oct 10, 2019

Provide access to tracing-subscriber::fmt::spans or similar API. #355

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

subscriber: add an abstraction for building visitors #241

subscriber: add an abstraction for building visitors #241

hawkw commented Aug 5, 2019 •

edited

Loading

bIgBV Aug 6, 2019

bIgBV Aug 6, 2019

hawkw Aug 6, 2019

hawkw commented Aug 16, 2019

KodrAus Aug 22, 2019

hawkw Aug 25, 2019

hawkw Aug 25, 2019

bIgBV Sep 10, 2019

bIgBV left a comment

bIgBV Sep 10, 2019

bIgBV Sep 10, 2019

bIgBV Sep 10, 2019

bIgBV Sep 10, 2019

hawkw Sep 12, 2019

davidbarsky Sep 25, 2019

davidbarsky commented Sep 25, 2019

hawkw commented Oct 8, 2019

davidbarsky commented Oct 8, 2019

	pub trait NewVisitor<'a> {
	type Visitor: field::Visit + 'a;

	fn make(&self, writer: &'a mut dyn fmt::Write, is_empty: bool) -> Self::Visitor;
	}

subscriber: add an abstraction for building visitors #241

subscriber: add an abstraction for building visitors #241

Conversation

hawkw commented Aug 5, 2019 • edited Loading

Motivation

Solution

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hawkw commented Aug 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bIgBV left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidbarsky commented Sep 25, 2019

hawkw commented Oct 8, 2019

davidbarsky commented Oct 8, 2019

hawkw commented Aug 5, 2019 •

edited

Loading