subscriber: Introduce Registry; Refactor fmt::Subscriber in terms of Registry + Layers #420
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
hawkw
requested changes
Nov 10, 2019
davidbarsky
changed the title
davidbarsky
changed the title
|
@davidbarsky did you ever get to run the subscriber benchmarks for this branch? I'd love to see results. |
|
@hawkw My computer died yesterday, and then I ended up watching Watchmen. Here's the output of |
|
These are the benchmarks on the master branch: |
|
Just focusing on
|
hawkw
added a commit
that referenced
this pull request
Nov 11, 2019
These benchmarks were originally written for assessing before/after performance on a branch that was not merged. However, they are probably generally useful for other fmt subscriber changes (e.g. #420). Signed-off-by: Eliza Weisman <eliza@buoyant.io>
|
@davidbarsky I don't think any of those benchmarks actually measure |
davidbarsky
pushed a commit
that referenced
this pull request
Nov 11, 2019
These benchmarks were originally written for assessing before/after performance on a branch that was not merged. However, they are probably generally useful for other fmt subscriber changes (e.g. #420). Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Co-authored-by: David Barsky <dbarsky@amazon.com> Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Co-authored-by: David Barsky <dbarsky@amazon.com> Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw
reviewed
Nov 13, 2019
Comment on lines
66
to
79
| /// If a extension of this type already exists, it will | ||
| /// be returned. Note that extensions are _not_ | ||
| /// `Layer`-specific—they are _span_-specific. This means that | ||
| /// other layers can access and mutate extensions that | ||
| /// a different Layer recorded. For example, an application might | ||
| /// have a layer that records execution timings alongside a layer | ||
| /// that reports spans and events to a distributed | ||
| /// tracing system that requires timestamps for spans. | ||
| /// Ideally, if one layer records a timestamp _x_, the other layer | ||
| /// should be able to reuse timestamp _x_. | ||
| /// | ||
| /// Therefore, extensions should generally be newtypes, rather than common | ||
| /// types like [`String`](std::string::String), to avoid accidnental | ||
| /// cross-`Layer` clobbering. |
There was a problem hiding this comment.
i think this maybe could be clearer, but i'm not immediately sure how. i'm also not convinced that discussing this on the method docs for insert is the right place; maybe it should be part of a higher-level description of extensions...but i'm fine with editing the docs later.
There was a problem hiding this comment.
This can be clearer, and yeah—this should go somewhere else. A module-level discussion, maybe?
hawkw
approved these changes
Nov 13, 2019
There was a problem hiding this comment.
This looks good to me (modulo a couple last minute nits). Thanks so much for all your work on this, @davidbarsky!
@jonhoo do you want to give this another review before it merges?
Comment on lines
+13
to
+14
| /// A "separate current span" for each thread is a semantic choice, as each span | ||
| /// can be executing in a different thread. |
There was a problem hiding this comment.
i'm not sure if i would say that this is a semantic choice we're making here in particular as much as it's the generally expected semantics for spans...not going to block on a comment on an internal api, though.
|
Followup issues to this PR:
(These all need to be separate issues, but I wanted to place this somewhere until I get the chance to create these) |
davidbarsky
changed the title
|
@davidbarsky, if you're collecting follow ups here. |
|
@SamScott that was definitely meant to be re-exported, I thought I added a re-export in |
|
Yep, that’s a bug. We should fix that. If it’s blocking you, feel free to cut a PR and I’ll review it. |
|
@samscott89 I opened a pull request to address this: #425. Thanks for catching this! |
davidbarsky
added a commit
that referenced
this pull request
Nov 14, 2019
With #420, we introduced FmtContext and made it a required parameter on FormatEvent's primary methods. However, we forgot to export it, making it impossible to write custom FormatEvent implementations. This PR fixes that oversight. Thanks to @samscott89 for noticing this! Signed-off-by: David Barsky me@davidbarsky.com
This was referenced Nov 14, 2019
|
@davidbarsky, woah! Thanks for getting that fixed super fast. It wasn't blocking anything, actually the new API meant I could even throw away the code which needed the format event bits :) I have been using this PR to start writing some PoC/MVP style tracing to honeycomb and zipkin... Works so easily, it's wonderful. |
|
@samscott89: Glad to hear! You’ll probably need #429 to export Honeycomb-style spans on close; I’ve started work on the second option that Eliza mentioned in her comment. PR forthcoming. |
@samscott89 BTW, @pkinsky is working on a Honeycomb/ |
|
👋 @samscott89 I just updated the master branch with some changes I had sitting in a branch (thanks for the code review, @hawkw!), I believe it's close to being ready to publish a v0.0.1 on crates.io (I just need to finish documentation, some CI, etc). I'd love to collaborate on refactoring it to use the new APIs! |
|
Hey @pkinsky! That looks cool. There's definitely some similarities to the quick mvp version of this I wrote locally to make it happen. I'll move this to an issue on your repo to keep things in a more sensible place (hope thats okay!). |
hawkw
added a commit
that referenced
this pull request
Feb 4, 2020
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 <eliza@buoyant.io>
hawkw
added a commit
that referenced
this pull request
Feb 4, 2020
# 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 <eliza@buoyant.io>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
(Note: this is a clone of #412 in order to make reviewing the changes as a cohesive whole easier.)
This branch introduces:
SpanData,LookupSpan, andLookupMetadata. Layer-specific data—such as formatted spans and events—are stored in theExtensionstypemap. They are writable via theExtensionsMutview struct of the typemap. This enables layers to read and write data that they are interested in.tracing_subscriber::fmt::Subscriberhas been re-implemented in terms oftracing_subscriber::registry::Registry.tracing_subscriber::fmt::FmtContext. A similar structure existed intracing_subscriber::fmt::Subscriber, but it was not previously publicly exposed.Examples of composing layers to mimic the
tracing_subscriber::fmt::Subscriberare forthcoming, most likely in a followup PR.Resolves #135
Resolves #157
Resolves #391