This PR fixes all reported clippy lints. In most cases I have made the
suggested changes. In a few cases (e.g., `blacklisted_name` and
`cognitive_complexity`) I've just silenced the warning.
I can make stylistic changes or discard some of the lint fixes if
preferred.
## Motivation
PR #443 added a `Context::scope` function, as described in issue #427.
In the issue, the desired change was described as follows:
> - The method `FmtContext::visit_spans` should be moved to
> `layer::Context`.
> - The new `layer::Context::visit_spans` should be renamed to
> something along the lines of `parents()`, **and be made to return
> a newtype implementing `std::iter::Iterator`.**
(emphasis mine)
Unfortunately, #443 only implemented the first bullet point — the new
`layer::Context::scope` method was just a copy of the old
`visit_spans`. The branch was merged before a thorough review could
determine that the change made there was incomplete.
## Solution
This branch changes the `Context::scope` function to return an
iterator, as it was supposed to, and rewrites `visit_spans` to consume
that iterator.
Additionally, I've added some new toys:
- `SpanRef::from_root`, which returns an iterator over a span's
parents, starting from the root (rather than from the direct parent
like `SpanRef::parents`)
- `Context::lookup_current`, which returns a `SpanRef` to the current
span's data
- Easier to use lifetimes on a couple of the new `Context` methods
enabled by the "registry" feature (not a breaking change, as these
aren't yet released)
Co-authored-by: David Barsky <me@davidbarsky.com>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
## Motivation
Currently, `Layer::downcast_raw` constructs a pointer with:
```rust
&self as *const _ as *const ()
```
This is wrong. The method's receiver is already `&self`, so this code
constructs a reference _to_ the `&self` reference, and converts _that_
into a pointer. Since this is not the actual memory address of the
layer, this is invalid.
We didn't catch this because the tests for downcasting layers are
also wrong: they never actually dereference the ref created by
`downcast_ref`. Therefore, if an invalid pointer is returned, the
test can still pass, as long as _any_ pointer is returned.
## Solution
This branch changes the pointer to:
```rust
self as *const _ as *const ()
```
Now, the returned pointer is valid.
In addition, it changes the tests to try and access data through the
returned references. The new tests fail with the current master
implementation of `downcast_ref`, which is correct.
Fixes#453
Currently there are some links that 404 because they contain
a reference to a struct 'struct.Layer.html' which presumably
should be the trait 'layer/trait.Layer.html'.
Signed-off-by: ifeanyi <ify1992@yahoo.com>
## Motivation
What I wanted to achieve: lookup data from a span _outside_ of a
`Layer`/`Subscriber` implementation. (i.e. without having a
`Context<S>`)
What I was trying: get the configured subscriber with
`dispatcher::get_default(...)`, and then downcasting to a `Layered<....,
Registry>`. But I (a) cant get access to the inner `Registry` and (b)
don't have an implementation of `LookupSpan` for `Layered`.
## Solution
Since there was already a `LookupMetadata` impl, this just seemed like
an oversight. So I changed the implementation over.
Note that `LookupMetadata` gets implemented via the blanket impl on
`L: LookupSpan`.
This patch moves the current `FmtContext::visit_spans`
implementation into a new method `layer::Context::scope`, leaving
`visit_spans` itself in place in order to maintain compatibility.
Resolves#427
Signed-off-by: ifeanyi <ify1992@yahoo.com>
## Motivation
This branch fixes several bugs in the `Registry` span close logic, and
adds new tests for these issues.
## Solutions
* Spans not being removed at the end of `on_close`
There is currently a bug in the `Registry` that prevents spans from
being removed, even when all `on_close` callbacks are complete. The test
for this behavior ( `span_is_removed_from_registry`) fails to catch the
bug, since there is a bug in the test as well.
The test asserts that a span extension was dropped after the span
closes. Current, the test uses `subscriber::with_default`, and then
made the assertion _outside_ of the `with_default` block. However, this
was incorrect, as even if the `Registry` fails to drop the span, the
span extension will be dropped when the whole `Registry` itself is
dropped, at the end of the `with_default` block.
This branch fixes the test, and the bug in the close logic. I've changed
the test to use an explicit `Dispatch` to keep the registry alive until
the end of the test. This reveals that the span is _not_ being removed
as it should be.
The reason the span fails to be removed is that the `Drop`
implementation for `CloseGuard` drops the span if the value of the
`CLOSE_COUNT` cell is 0. However, the `if` statement that tests for this
compares the value returned by `Cell::get` with 0. The value returned by
`Cell::get` is the _previous_ value, not the current one after dropping
this guard; if the guard being dropped is the final guard, then the
value returned by `Cell::get` will be 1, rather than 0.
I've fixed this logic, and refactored it slightly to hopefully make it
easier to understand in the future.
Thanks to @jtescher for catching this bug!
* Only the first span being removed at the end of `on_close`
In addition, I've fixed a bug where the remove after close logic would
only work for the _first_ span to close on each thread. This is because
dropping a `CloseGuard` does not currently subtract from CLOSE_COUNT
when it is the final guard. This means that when the next span is
closed, the count will start at 1, rather than 0. I've fixed this by
ensuring that the close count is always decremented, and changed the
tests to close multiple spans.
* Spans removed on the first `try_close`
Finally, there is also an issue where the removal logic is run on
_every_ call to `try_close`, regardless of whether or not the subscriber
actually indicates that the span closes. This means that a span will be
removed from the registry even when there are one or more span handles
that reference it.
This is due to the `start_close` method being called _before_
`Subscriber::try_close` is called. When a close guard is dropped, the
span is currently _always_ removed. However, since we call `start_close`
at the beginning of the `Layered::try_close` method, we may be
decrementing a ref count _without_ closing the span, but the close guard
is unaware of this.
I've fixed this bug by updating the `CloseGuard` struct to track whether
the span is closed. It now has a bool that is set only when the
`Subscriber::try_close` call returns true.
Only creating the `CloseGuard` if `try_close` returns true may seem like
a simpler solution, but that won't work, since the call to `try_close`
may call into another `Layered` subscriber. In order to handle
situations where many layers are nested, we need to construct the
`CloseGuard` for each stack frame before calling into the next one, so
it's necessary to set whether the span is closing only after the call to
`try_close` returns.
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
For Registry-backed Layers, this commit introduces a capability where
span deletion is deferred until until all Layers have processed the span
closure notification.
This branch introduces:
- A registry build atop of https://github.com/hawkw/sharded-slab. Layers
are expected to consume this registry through the traits `SpanData`,
`LookupSpan`, and `LookupMetadata`. Layer-specific data, such as
formatted spans and events, are stored in the `Extensions` typemap. Data
is writable via the `ExtensionsMut` view struct of the typemap. This
enables layers to read and write data that they are specifically
interested in.
- The `tracing_subscriber::fmt::Subscriber` has been re-implemented in
terms of `tracing_subscriber::registry::Registry` and
`tracing_subscriber::fmt::Layer`.
- The event/field formatters have been modified (in a non-backwards
compatible way) to accept a `tracing_subscriber::fmt::FmtContext`. A
similar structure existed in `tracing_subscriber::fmt::Subscriber`, but
it was not publicly exposed.
Resolves#135Resolves#157Resolves#391
Signed-off-by: David Barsky <me@davidbarsky.com>
Coauthored-by: Eliza Weisman <eliza@buoyant.io>
## Motivation
Currently, when using `tracing-subscriber`'s `Layer` API, it is
necessary for each layer to manage its own storage for per-span data.
Since subscribers are shared concurrently, this means that `Layer`s must
also manage synchronization on this storage. This is easy to get wrong,
and even when it's implemented correctly, having every layer synchronize
separately adds a lot of overhead. Ideally, it should be possible for
state stored by the subscriber to be exposed to `Layer`s.
## Solution:
This branch *starts* on the project of a general-purpose span registry
by adding a new trait, `LookupMetadata`. Subscribers may implement this
trait to allow looking up a span's `Metadata` by ID. If a subscriber
implements this trait, the `layer::Context` type will then allow any
layers wrapping that subscriber to look up metadata by ID. The
`FmtSubscriber` implements this trait.
Future work will be able to an interface for `Subscriber`s to expose
more span data, such as parents, children, and arbitrarily-typed
extensions, to `Layer`s as well.
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
This overrides the default `Subscriber::current_span` impl
with one that forwards the inner subscribers implementation.
This fixes bugs where the current_span was not fetchable when
using the `FmtSubscriber`.
Signed-off-by: Lucio Franco <luciofranco14@gmail.com>
## Feature Request
### Crates
- `tracing-subscriber`
### Motivation
Currently, the `tracing-subscriber` `FmtSubscriber::default`
implementation defaults to no filtering. This means that all
instrumentation, including extremely verbose `trace`-level diagnostics
from crates like `tokio` are enabled by default.
This is because the subscriber itself does not implement filtering, in
order to allow it to be composed with filters implemented by `Layer`s.
However, defaulting to no filtering at all is definitely a surprising
behavior. I didn't want to conditionally return a different type based
on whether or not filtering was enabled by the `filter` feature flag,
but this is probably not worth the confusion introduced by this
behavior. We should make this more intuitive.
## Solution
This branch changes `tracing-subscriber` to default to enabling
the `INFO` level and above. If the `filter` feature flag is enabled,
users may opt-in to `env_logger`-style filtering. Additionally,
regardless of feature flags, a new `with_max_level` method is
added to the `FmtSubscriber` builder, which takes a `Level` or
`LevelFilter`. `LevelFilter` now implements `Layer` by enabling
any spans and events that are less than or equal to that `Level`.
Fixes: #331Fixes: #332
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
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 <eliza@buoyant.io>
## Motivation
As discussed in #308, there are a large number of crates in this
repository, which can be confusing for users and can increase the
maintainance burden for maintainers. Also, the `tracing-fmt` and
`tracing-subscriber` crates both contain filtering implementations with
similar behaviour and APIs, but `tracing-subscriber`'s filter module
offers more advanced features (filtering on field values), and is usable
with any subscriber implementation. Two separate filter implementations
also has the potential to be confusing for users.
## Solution
This branch moves most of the code from `tracing-fmt` into a module in
`tracing-subscriber`, and changes the `tracing-fmt` builder APIs to use
the `Filter` type in `tracing-subscriber`. The `tracing-subscriber/fmt`
feature flag can be used to disable the formatting subscriber when it is
not used.
The `tracing-fmt` crate has been updated to re-export the APIs from
`tracing-subscriber`, and marked as deprecated. Once we've published a
new version of `tracing-subscriber` with the format APIs, we can publish
a final release of `tracing-fmt` that will update the documentation &
mark all APIs as deprecated, so that users know to move to the
`tracing-subscriber` crate.
Refs: #308
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
## Motivation
In some cases, `Layer` implementations may wish to trigger new events on
the inner subscriber. Potential use cases include layers that aggregate
or roll up events. Now that PR #281 allows events to be constructed
without implicitly dispatching them to a `Subscriber`, we can add
support for this to the `Layer` trait.
## Solution
This commit adds a `Context::event` method to the `layer::Context` type
in `tracing-subscriber`. This takes an `&Event` and calls
`Subscriber::event` on the wrapped subscriber.
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
This branch prepares tracing-subscriber 0.0.1-alpha.2 for release.
I've also added some docs for the new field filtering APIs.
Co-authored-by: Eliza Weisman <eliza@buoyant.io>
## 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 <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
* 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 <eliza@buoyant.io>
* subscriber: remove ref-counting from Layer (#149)
## Motivation
As I mentioned in https://github.com/tokio-rs/tracing/pull/136#discussion_r300158248, 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 <eliza@buoyant.io>
* 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 <eliza@buoyant.io>
