Skip to content
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

subscriber: Add a Layer type for composing subscribers #136

Merged
merged 35 commits into from
Jul 17, 2019
Merged

Conversation

hawkw
Copy link
Member

@hawkw hawkw commented Jul 2, 2019

Motivation

In many cases, it is valuable to compose multiple consumers for trace
events. However, certain aspects of tracing's design --- in
particular, that span IDs are assigned by the subscriber, and that a
single subscriber collects events from multiple threads --- make it
difficult to compose the Subscriber trait directly.

Solution

This branch introduces a new trait, Layer, in tracing-subscriber.
Layer represents the composable subset of the Subscriber
functionality --- it recieves notifications of spans and events, and can
filter callsites, but it does not assign IDs or manage reference counts,
instead being notified on span closure by the underlying subscriber. A
Layer can thus be wrapped around another subscriber to add additional
functionality. In addition, this branch adds functionality for composing
layers, and a filter module that provides Layer implementations for
simple filters.

Future Work

To have a complete story for multi-subscriber fanout, we will also want
to implement a performant concurrent span store, as I described in #157.
To better support the use of subscriber layers, may wish to consider
having a "registry" subscriber that implements only span storage, ID
generation, and lifecycle management, for composing layers on top of.
Finally, we should consider adding Layer implementations to other
crates that currently only expose Subscriber impls.

Refs: #135

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

@hawkw hawkw added the crate/subscriber Related to the `tracing-subscriber` crate label Jul 2, 2019
@hawkw hawkw requested review from davidbarsky and LucioFranco July 2, 2019 18:23
@hawkw hawkw self-assigned this Jul 2, 2019
@hawkw hawkw added this to the tracing-subscriber 0.1 milestone Jul 2, 2019
Copy link
Member

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks pretty good as a first approach, Im gonna have to see how this fits in with my subscribers.

tracing-subscriber/src/layer.rs Outdated Show resolved Hide resolved
tracing-subscriber/src/lib.rs Outdated Show resolved Hide resolved
tracing-subscriber/src/layer.rs Outdated Show resolved Hide resolved
tracing-subscriber/src/layer.rs Outdated Show resolved Hide resolved
hawkw added a commit that referenced this pull request Jul 9, 2019
## Motivation

As discussed in #136, a proposed `Layer` trait for composing subscribers
required should probably _not_ be responsible for managing ref-counts
and determining span closures. Instead, the root subscriber should be
responsible for this, and `Layer`s should be able to opt into a callback
that's notified _when_ a span is closed. 

Adding a callback that's called on closure to `Layer` requires modifying
the `Subscriber` trait to allow indicating when a span closes.

## Solution

This branch attempts to do this without a breaking change, by adding a
new `try_close` trait method to `Subscriber`. `try_close` is identical
to `drop_span` except that it returns a boolean value, set to `true` if
the span has closed. 

The `try_close` default implementation simply calls
`self.drop_span(...)` and returns `false`, so that if subscribers don't
care about tracking ref counts, close notifications will never be
triggered. 

The `drop_span` documentation now indicates that it is "soft deprecated"
similarly to `std::error::Error`'s `description` method. This encourages
subscribers to implement the new API, but isn't a breaking change.
Existing _callers_ of `drop_span` are still able to call it without
causing issues. A subsequent PR will mark `drop_span` as deprecated
in favour of `try_close`.

Signed-off-by: Eliza Weisman <[email protected]>
@hawkw hawkw force-pushed the eliza/wrapper branch 2 times, most recently from 11afa5c to ccaf143 Compare July 9, 2019 19:50
hawkw added a commit that referenced this pull request Jul 9, 2019
## Motivation

As I mentioned in #136 (comment), the
`Layer` trait proposed in #136 should probably _not_ be responsible for
managing ref-counts and determining span closures. Instead, the root
subscriber should be responsible for this, and `Layer`s should be able
to opt into a callback that's notified _when_ a span is closed. 

## Solution

Adding a callback that's called on closure to `Layer` required modifying
the `Subscriber` trait to allow indicating when a span closes. This
branch attempts to do this without a breaking change, by adding a new
`try_close` trait method to `Subscriber`. `try_close` is identical to
`drop_span` except that it returns a boolean value, set to `true` if the
span has closed. This function was added in #153. 

This branch updates `tracing-subscriber::Layer` to use `try_close`. 
Rather than having `clone_span`/`drop_span` callbacks on `Layer`, we
now have a `close` function, which is called when the inner subscriber's
`try_close` returns `true`.

Signed-off-by: Eliza Weisman <[email protected]>
@hawkw
Copy link
Member Author

hawkw commented Jul 9, 2019

7bd71e5 adds a Ctx type to the Layer module to allow exposing functions on the inner subscriber such as current_span to the layer. It's wrapped as a new type to restrict the layer from calling any arbitrary method on the inner subscriber, and to allow us to add new methods to it in the future without breaking changes.

@hawkw
Copy link
Member Author

hawkw commented Jul 9, 2019

Some potential points to bikeshed (@davidbarsky, @jonhoo, @LucioFranco):

  • What should the name of the trait implemented by layerable subscribers be? Is Layer the most descriptive, or should it be Consumer or Observer?
  • Should the Layer::enter, Layer::exit, and Layer::event methods be on_enter, on_exit, and on_event, to make it clearer that they're just callbacks & disambiguate them from the Subscriber methods?

@hawkw hawkw changed the title [RFC] subscriber: proposal for composing subscribers subscriber: Add a Layer type for composing subscribers Jul 9, 2019
@hawkw hawkw requested review from davidbarsky and LucioFranco July 9, 2019 23:29
@hawkw hawkw marked this pull request as ready for review July 9, 2019 23:29
@hawkw
Copy link
Member Author

hawkw commented Jul 9, 2019

I've marked this as ready for review, though I'd like to finish writing docs once we agree on the API.

@LucioFranco
Copy link
Member

@hawkw, in this case, I actually like Observer I think its fits a lot better.

@hawkw
Copy link
Member Author

hawkw commented Jul 9, 2019

@hawkw, in this case, I actually like Observer I think its fits a lot better.

Worth noting that a Layer can also be e.g. a filter, though.

@davidbarsky
Copy link
Member

What should the name of the trait implemented by layerable subscribers be? Is Layer the most descriptive, or should it be Consumer or Observer?

Getting some déjà vu here! Layer seems to be the best fit, as Consumer implies a finality that Layer doesn't have. I'd be happy to be a swing vote if there's a clear split.

Should the Layer::enter, Layer::exit, and Layer::event methods be on_enter, on_exit, and on_event, to make it clearer that they're just callbacks & disambiguate them from the Subscriber methods?

I'm very strongly in favor of this change—this was one of the biggest bits of confusion to me, and the on_* naming scheme would communicate the intent much more clearly, I think.

hawkw added a commit that referenced this pull request Jul 11, 2019
## Motivation

As I mentioned in #136 (comment), the
`Layer` trait proposed in #136 should probably _not_ be responsible for
managing ref-counts and determining span closures. Instead, the root
subscriber should be responsible for this, and `Layer`s should be able
to opt into a callback that's notified _when_ a span is closed. 

## Solution

Adding a callback that's called on closure to `Layer` required modifying
the `Subscriber` trait to allow indicating when a span closes. This
branch attempts to do this without a breaking change, by adding a new
`try_close` trait method to `Subscriber`. `try_close` is identical to
`drop_span` except that it returns a boolean value, set to `true` if the
span has closed. This function was added in #153. 

This branch updates `tracing-subscriber::Layer` to use `try_close`. 
Rather than having `clone_span`/`drop_span` callbacks on `Layer`, we
now have a `close` function, which is called when the inner subscriber's
`try_close` returns `true`.

Signed-off-by: Eliza Weisman <[email protected]>
hawkw and others added 23 commits July 17, 2019 13:20
Signed-off-by: Eliza Weisman <[email protected]>
Signed-off-by: Eliza Weisman <[email protected]>
Signed-off-by: Eliza Weisman <[email protected]>
Signed-off-by: Eliza Weisman <[email protected]>
Signed-off-by: Eliza Weisman <[email protected]>
Signed-off-by: Eliza Weisman <[email protected]>
Signed-off-by: Eliza Weisman <[email protected]>
This reworks layer composition a bit, and fixes an issue where
`Context`s were not actually guaranteed to wrap a `Subscriber`, and thus
implemented no methods. Layers now guarantee that `S` implements
`Subscriber`.

Signed-off-by: Eliza Weisman <[email protected]>
Signed-off-by: Eliza Weisman <[email protected]>
it needs to be reworked & will be added separately

Signed-off-by: Eliza Weisman <[email protected]>
Signed-off-by: Eliza Weisman <[email protected]>
Signed-off-by: Eliza Weisman <[email protected]>
@hawkw hawkw requested a review from davidbarsky July 17, 2019 20:22
@hawkw hawkw merged commit 8ad3e8e into master Jul 17, 2019
hawkw added a commit that referenced this pull request Aug 1, 2019
## Motivation

Currently, `tracing-fmt` provides an implementation of `env_logger`-like
filtering directives. However, there are two issues:

1. The implementation is specific to `tracing-fmt` and does not work 
   with other subscribers.
2. Filtering dynamically based on field values is not supported.

Now that the `Layer` type has been added to `tracing-subscriber` (see 
#136), we can implement filtering generically, as a `Layer` that can
wrap other `Subscriber`s to provide a particular filtering strategy.

## Solution

This branch re-implements the `env_logger` style filtering in 
`tracing-fmt` as a `tracing-subscriber::Layer`. The new `Layer` type
implements dynamic filtering on span fields, in addition to the 
functionality of the `tracing-fmt` implementation. I've also added a 
wrapper type to support runtime reloading of a `Layer`, similarly to 
the `Reload` type in `tracing-fmt::filter`, but more general.

Finally, I've added some tests and an interactive demo for the new 
filtering. The example runs a simple web service with a load generator,
and allows users to explore the `tracing-fmt` output from the example 
service by dynamically reloading the filter settings.

## Notes

This is admittedly a pretty large branch, but I think it makes the 
most sense to merge everything together, since the example requires
both the filter implementation *and* the reload layer. I've tried to 
make sure the most complex bits of the filtering code has comments 
describing the implementation, but please let me know if anything 
is unclear.

Also, there is a lot of room for potential performance improvements
in the current filter implementation. I've left comments on some code
that I think could probably be made more efficient. Ideally, though, 
any future optimization work ought to be guided by benchmarks as well.

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

Signed-off-by: Eliza Weisman <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crate/subscriber Related to the `tracing-subscriber` crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants