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

core: add Subscriber::try_close #153

Merged
merged 4 commits into from
Jul 9, 2019
Merged

core: add Subscriber::try_close #153

merged 4 commits into from
Jul 9, 2019

Conversation

hawkw
Copy link
Member

@hawkw hawkw commented Jul 6, 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 Layers 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 returns false, so that
if subscribers don't care about tracking ref counts, close notifications
will never be triggered. The default implementation of drop_span is
changed to call self.try_close(...) and ignore the return value.

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, as the Layered::drop_span will always call try_close
on the inner subscriber, in order to forward the closure notification to
the layer if the span closes.

Signed-off-by: Eliza Weisman <[email protected]>
@hawkw hawkw added the crate/log Related to the `tracing-log` crate label Jul 6, 2019
@hawkw hawkw self-assigned this Jul 6, 2019
@hawkw hawkw added kind/feature New feature or request crate/core Related to the `tracing-core` crate and removed crate/log Related to the `tracing-log` crate labels Jul 6, 2019
@hawkw hawkw added this to the tracing-subscriber 0.1 milestone Jul 7, 2019
@hawkw
Copy link
Member Author

hawkw commented Jul 9, 2019

@jonhoo I've changed try_close to call drop_span, as you suggested in #153 (comment), and updated the documentation a bit. It would be great to get your approval on this branch, as I'd like to move forward with this.

@jonhoo
Copy link
Contributor

jonhoo commented Jul 9, 2019

@hawkw happy to approve subject to the one comment above.

Signed-off-by: Eliza Weisman <[email protected]>
@hawkw hawkw merged commit 4173844 into master Jul 9, 2019
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 added a commit that referenced this pull request Jul 10, 2019
# 0.1.2 (July 10, 2019)

### Deprecated

- `Subscriber::drop_span` in favor of new `Subscriber::try_close` (#168)

### Added

- `Into<Option<&Id>>`, `Into<Option<Id>>`, and
  `Into<Option<&'static Metadata<'static>>>` impls for `span::Current` (#170)
- `Subscriber::try_close` method (#153)
- Improved documentation for `dispatcher` (#171)

* core: update changelog for 0.1.2
* core: update version to 0.1.2

Signed-off-by: Eliza Weisman <[email protected]>
hawkw added a commit that referenced this pull request Jul 11, 2019
## Motivation

The `log` compatibility feature in the `tracing` logs when spans are
entered or exited, but does not track the full span lifecycle. This is
largely because there was not previously any way for the `tracing` crate
to be made aware of span closes.

## Solution

#153 added the `try_close` method to `Subscriber`, allowing subscribers
to communicate when spans are closed. This commit changes the `log`
compatibility feature in `tracing` to log when spans are closed, if a
subscriber is active and informs it of span closures.

I've also added tests for this.

Signed-off-by: Eliza Weisman <[email protected]>
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 added a commit that referenced this pull request Jul 12, 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 added a commit that referenced this pull request Jul 15, 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 added a commit that referenced this pull request Jul 17, 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 added a commit that referenced this pull request Jul 17, 2019
* add `Layer` trait for composing subscribers

### 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]>

* subscriber: remove ref-counting from Layer (#149)

## 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]>

* ensure a Context always refers to a Subscriber

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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crate/core Related to the `tracing-core` crate kind/feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants