subscriber: For registry-backed layers, delay deletion of span until all layers have processed the event #432
Conversation
davidbarsky
changed the title
|
The same benchmarks running on a significantly less noisy c3.8xlarge: https://gist.github.com/davidbarsky/111814dcc55e89580e5ee283b9900bbf. I first ran benchmarks on the |
There was a problem hiding this comment.
This looks great, and I'm glad that we were able to use the approach that doesn't involve additional unsafe code without significant performance impact. I left some notes, primarily on style and naming.
I also think it would be really good to have some tests for this behavior, since it is relatively complicated. Some things we should test:
- a
Layerthat expects a span to be closed, and asserts that looking up the span is successful inside it'son_closecallback - the same test with two or more
Layers — both layers should be able to access the closed span's data during theiron_closecalls - a test that ensures that the span data is removed from the registry once all the
on_closemethods have completed. We could test this by having a type that sets a shared atomic boolean when it's dropped, insert that type into a span's extensions, and drop the span. After the close completes, we can assert that the boolean indicating that the extension was dropped is set.
On Discord, we discussed how these benchmarks might potentially be inaccurate (perhaps due to a noisy benchmarking environment). Did you have a chance to run them again in a more isolated benchmark environment? If so, it would be great to see those results as well. |
hawkw
added
crate/subscriber
Following the suggested fix on #365: #365 (comment) tracing-core's use of `spin` is feature-gated. `lazy_static` is vendored for either feature but I've modified it to respect our feature flags. I've also removed doc comments that no longer compile and suppressed a couple of warnings with the lints that are now being applied. At this point * Including this in another project with std `lazy_static` works * Tracing unit tests pass both with and without `--no-default-features` Fixes #365 * core: vendor lazy_static and spin for no_std support * core: fmt vendored code * core: trim down vendored spin, reduce visibility and addresss warnings * core: limit vendored lazy_static for use only with no_std, fix warnings * core: update paths to now-vendored no_std libs * tracing: use spin::Once re-exported from tracing-core instead of crate * core: remove ineffectual doc(hidden) * core: refmt stdlib module * core: bump to 0.1.8 to expose export of vendored spin::Once to tracing
`fmt::Layer::on_record` was looking up extensions using `FormattedFields<Self>>` when it should have been using `FormattedFields<N>>`. Signed-off-by: David Barsky [email protected]
Great, thank you — I must've missed this earlier. Seems like the performance difference is very slight, probably not significant enough to justify the more complex unsafe solution. We can revisit this if it turns out to have an impact. |
…cing into david/downcast_ref-registry
|
@hawkw I think I've addressed all of the comments you had, including adding tests. I'd appreciate a re-review whenever you have the time! |
Co-Authored-By: Eliza Weisman <[email protected]>
|
the test failures on nightly (https://github.com/tokio-rs/tracing/pull/432/checks?check_run_id=306313856) seem unrelated, but it would be good if they could be addressed |
|
@hawkw Sorry—just saw your comment about nightly failures. I'll address that in a separate branch. |
0.2.0 (February 4, 2020) Breaking Changes: - **fmt**: Renamed `Context` to `FmtContext` (#420, #425) - **fmt**: Renamed `Builder` to `SubscriberBuilder` (#420) - **filter**: Removed `Filter`. Use `EnvFilter` instead (#434) Added: - **registry**: `Registry`, a reusable span store that `Layer`s can use a high-performance, in-memory store. (#420, #425, #432, #433, #435) - **registry**: Added `LookupSpan` trait, implemented by `Subscriber`s to expose stored span data to `Layer`s (#420) - **fmt**: Added `fmt::Layer`, to allow composing log formatting with other `Layer`s (#420) - **fmt**: Added support for JSON field and event formatting (#377, #415) - **filter**: Documentation for filtering directives (#554) Changed: - **fmt**: Renamed `Context` to `FmtContext` (#420, #425) (BREAKING) - **fmt**: Renamed `Builder` to `SubscriberBuilder` (#420) (BREAKING) - **fmt**: Reimplemented `fmt::Subscriber` in terms of the `Registry` and `Layer`s (#420) Removed: - **filter**: Removed `Filter`. Use `EnvFilter` instead (#434) (BREAKING) Fixed: - **fmt**: Fixed memory leaks in the slab used to store per-span data (3c35048) - **fmt**: `fmt::SubscriberBuilder::init` not setting up `log` compatibility (#489) - **fmt**: Spans closed by a child span closing not also closing _their_ parents (#514) - **Layer**: Fixed `Layered` subscribers failing to downcast to their own type (#549) - **Layer**: Fixed `Layer::downcast_ref` returning invalid references (#454) Signed-off-by: Eliza Weisman <[email protected]>
# 0.2.0 (February 4, 2020) ### Breaking Changes - **fmt**: Renamed `Context` to `FmtContext` (#420, #425) - **fmt**: Renamed `Builder` to `SubscriberBuilder` (#420) - **filter**: Removed `Filter`. Use `EnvFilter` instead (#434) ### Added - **registry**: `Registry`, a `Subscriber` implementation that `Layer`s can use as a high-performance, in-memory span store. (#420, #425, #432, #433, #435) - **registry**: Added `LookupSpan` trait, implemented by `Subscriber`s to expose stored span data to `Layer`s (#420) - **fmt**: Added `fmt::Layer`, to allow composing log formatting with other `Layer`s - **fmt**: Added support for JSON field and event formatting (#377, #415) - **filter**: Documentation for filtering directives (#554) ### Changed - **fmt**: Renamed `Context` to `FmtContext` (#420, #425) (BREAKING) - **fmt**: Renamed `Builder` to `SubscriberBuilder` (#420) (BREAKING) - **fmt**: Reimplemented `fmt::Subscriber` in terms of the `Registry` and `Layer`s (#420) ### Removed - **filter**: Removed `Filter`. Use `EnvFilter` instead (#434) (BREAKING) ### Fixed - **fmt**: Fixed memory leaks in the slab used to store per-span data (3c35048) - **fmt**: `fmt::SubscriberBuilder::init` not setting up `log` compatibility (#489) - **fmt**: Spans closed by a child span closing not also closing _their_ parents (#514) - **Layer**: Fixed `Layered` subscribers failing to downcast to their own type (#549) - **Layer**: Fixed `Layer::downcast_ref` returning invalid references (#454) Fixes #515 Fixes #458 Signed-off-by: Eliza Weisman <[email protected]>
This PR implements the second option ("The One With The
downcast_ref") in @hawkw's comment. The benchmarks of this change, relative to the master branch, are available here.Edit: The same benchmarks running on a significantly less noisy c3.8xlarge: https://gist.github.com/davidbarsky/111814dcc55e89580e5ee283b9900bbf.
Resolves #429