[DRAFT] registry part 2 #412
Conversation
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>
|
I can't directly respond, but:
I'm as puzzled as you are, but yes—I've moved the
removed it! I wanted to merge my imports in this module, forgot to remove it. Sorry!
Agreed; a lot of this is just patching over the differences and attempting to get something working. |
There was a problem hiding this comment.
Applied most of your suggestions!
As for:
right, what I was getting at is that Registry::drop_span is only ever called by this method, why does there need to be a separate method? Why not just put the body of drop_span here?
gotcha! I mostly copying the structure of fmt as I familiarized myself with this codebase.
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
| /// | ||
| /// [`Layer`]: ../trait.Layer.html | ||
| #[derive(Debug)] | ||
| pub struct FmtLayer< |
There was a problem hiding this comment.
I think it would be ideal if this was called fmt::Layer rather than fmt::Layer, so that it's more consistent with fmt::Subscriber...but then we would need to use the layer trait as layer::Layer...
| N: for<'writer> FormatFields<'writer> + 'static, | ||
| W: MakeWriter + 'static, | ||
| { | ||
| /// Sets a [FormatEvent<S, N>]. |
There was a problem hiding this comment.
link needs to be fixed — also i think the subscriber builder has a doc comment with examples for the equivalent method
| // this needs to be a seperate impl block because we're re-assigning the the W2 (make_writer) | ||
| // type paramater from the default. | ||
| impl<S, N, E, W> Builder<S, N, E, W> { | ||
| /// Sets a [MakeWriter] for spans and events. |
There was a problem hiding this comment.
link needs to be fixed, and can we copy the doc comment from the subscriber builder?
| E: FormatEvent<S, N> + 'static, | ||
| W: MakeWriter + 'static, | ||
| { | ||
| /// Builds a [FmtLayer] infalliably. |
| } | ||
| } | ||
|
|
||
| impl<S> Default for Builder<S> { |
There was a problem hiding this comment.
it's weird how impls on the layer and on the builder are interleaved here, can we sort them by type?
| /// | ||
| /// By storing [FormattedFields] instead of a [String] directly, | ||
| /// [FmtLayer] is able to be more defensive about other layers | ||
| /// accidentally a span's extensions. |
There was a problem hiding this comment.
these links need to be fixed...also, i think i left a comment on this earlier about a possible rewording? maybe it was lost in the move
| N: for<'writer> FormatFields<'writer> + 'static, | ||
| { | ||
| /// Visits parent spans. Used to visit parent spans when formatting spans | ||
| /// and events |
There was a problem hiding this comment.
again, i think i left a comment on this that may have vanished?
| let parents = span.parents().collect::<SpanRefVec<'_, _>>(); | ||
| let mut iter = parents.iter().rev(); | ||
| // visit all the parent spans... | ||
| while let Some(parent) = iter.next() { |
There was a problem hiding this comment.
nit: it might be more idiomatic to say
| while let Some(parent) = iter.next() { | |
| for parent in iter { |
or even
| while let Some(parent) = iter.next() { | |
| for parent in parents.iter().rev() { |
and get rid of the let iter =
| // spans path. however, that requires passing the store to `visit_spans` | ||
| // with a different lifetime, and i'm too lazy to sort that out now. this | ||
| // workaround shouldn't remaining in the final shipping version _unless_ | ||
| // benchmarks show that small-vector optimization is preferable to not-very-deep |
There was a problem hiding this comment.
probably remove the comment that says we shouldn't ship this, i think this implementation is probably fine (benches would still be good though)
| W: MakeWriter + 'static, | ||
| { | ||
| /// Sets a [FormatEvent<S, N>]. | ||
| pub fn with_event_formatter<E2>(self, e: E2) -> Builder<S, N, E2, W> |
There was a problem hiding this comment.
The equivalent method on SubscriberBuilder is just called
| pub fn with_event_formatter<E2>(self, e: E2) -> Builder<S, N, E2, W> | |
| pub fn fmt_event<E2>(self, e: E2) -> Builder<S, N, E2, W> |
let's make these more consistent?
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
|
Closed in favor of #420. |
Opening a draft PR to make this easier to comment on, please ignore for now