Add trait methods for `follows_from` #49

hawkw · 2018-10-24T23:54:05Z

This branch adds Subscriber and Span methods for annotating spans
with prior spans from which they follow. For the most part, this branch
doesn't add implementations of this interface for now, besides proxying
to it in composite subscribers.

It would be good to get a review of the interface before getting too deep
into implementation.

Signed-off-by: Eliza Weisman eliza@buoyant.io

hawkw · 2018-10-24T23:55:45Z

tokio-trace/src/span.rs

@@ -124,6 +124,10 @@ pub struct Data {
 #[derive(Clone, Debug, PartialEq, Eq, Hash)]
 pub struct Id(u64);

+pub trait AsId {
+    fn as_id(&self) -> Option<Id>;


Abstracts over anything that may have a span ID, so we can add a follows-from annotation from a Span handle, an Id, and so on. I was hoping there was a trait in the standard library that could be used here, but this seemed simpler.

Yeah, I guess this depends on rust-lang/rust#33417 landing.

hawkw · 2018-10-24T23:56:19Z

tokio-trace/src/subscriber.rs

@@ -38,6 +38,8 @@ pub trait Subscriber {
        value: &dyn IntoValue,
    ) -> Result<(), AddValueError>;

+    fn add_follows_from(&self, span: &span::Id, follows: span::Id);


Should this return an error if the span corresponding to that ID doesn't exist, like how add_value does?

Yes, I think so. I can imagine two types of subscribers:

Subscribers that do care whether a span corresponds to that ID doesn't exist. An example might be a profiler like pprof or hawktracer.

Subscribers that don't care whether a span corresponds to that ID doesn't exist. An example might be a metrics emission system.

A metrics emission system could just ignore the error, but an implementer of a profiler might be frustrated if they don't get an error when calling this method. Introducing an error down the line would be a breaking error, so I'd opt for an error that can be ignored now.

Yeah, that's reasonable. Good point.

hawkw · 2018-10-24T23:56:52Z

tokio-trace/src/subscriber.rs

@@ -38,6 +38,8 @@ pub trait Subscriber {
        value: &dyn IntoValue,
    ) -> Result<(), AddValueError>;

+    fn add_follows_from(&self, span: &span::Id, follows: span::Id);


Taking the ID of the span to annotate by-reference may not really be necessary...

hawkw · 2018-10-24T23:57:32Z

tokio-trace-subscriber/src/registry.rs

+
+    /// Queries the registry for an iterator over the IDs of the spans that
+    /// `span` follows from.
+    fn get_follows_from(&self, span: &Id) -> Self::FollowsFrom;


Are there other ways of querying these relationships that ought to be part of the registry API?

Should the subscriber interface also have a similar function? I kept it out because I didn't want to add too many more methods to that trait...

Are there other ways of querying these relationships that ought to be part of the registry API?

I'm thinking the follows_from family of methods should be named succeeds/proceeds to more clearly delineate ordering. Alternatively—and this might be a infeasible—but a PartialOrd implementation could be helpful.

Should the subscriber interface also have a similar function? I kept it out because I didn't want to add too many more methods to that trait.

No, probably not, but I can be convinced otherwise.

@davidbarsky

I'm thinking the follows_from family of methods should be named succeeds/proceeds to more clearly delineate ordering.

That's reasonable --- this naming is per tokio-rs/tokio#561 (comment), but I'm open to renaming it. I'm not sure how I feel about succeeds here, though, since that seems easy to confuse with a notion of success/failure...

Alternatively—and this might be a infeasible—but a PartialOrd implementation could be helpful.

Having something like this would certainly be ergonomic, but I don't think it's immediately possible in the current scheme --- it's the subscriber that knows these relationships, not the spans themselves. That said, I'll keep thinking about the potential of doing that.

That's reasonable --- this naming is per tokio-rs/tokio#561 (comment), but I'm open to renaming it. I'm not sure how I feel about succeeds here, though, since that seems easy to confuse with a notion of success/failure...

True. What about “prior/subsequent?”

Having something like this would certainly be ergonomic, but I don't think it's immediately possible in the current scheme --- it's the subscriber that knows these relationships, not the spans themselves. That said, I'll keep thinking about the potential of doing that.

Good point, didn't consider that that part.

That's reasonable --- this naming is per tokio-rs/tokio#561 (comment), but I'm open to renaming it. I'm not sure how I feel about succeeds here, though, since that seems easy to confuse with a notion of success/failure...

True. What about “prior/subsequent?”

That works for me!

[...] I don't think it's immediately possible in the current scheme --- it's the subscriber that knows these relationships, not the spans themselves.

Good point, didn't consider that that part.

I am considering changing the SpanRef type in the tokio-trace-subscriber crate to have a reference back to the registry for querying it to get span data, so it may be possible to implement something like that for SpanRefs in the more batteries-included crate...but that'll be its own branch.

davidbarsky

Left some comments that aren't blocking, but might be useful directions to explore.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

hawkw · 2018-10-25T21:28:23Z

There is one other potential concern that comes to mind here: namely, how should the similar follows-from annotations for events be handled? Do we want each one to correspond to an event_follows_from method invocation on the subscriber, or is passing them as an array on the Event struct itself okay?

The former would be more consistent with spans, but it also introduces some additional complexity --- currently, there isn't a way to refer to an event that's already occurred (there's no event ID type), and the event! macro doesn't return a persistent handle for calling methods on. I'd rather not introduce a lot of complexity to Events, but I'd also prefer both APIs to be consistent...

hawkw · 2018-10-25T21:34:49Z

@davidbarsky I think I've addressed all your feedback --- any other concerns?

davidbarsky · 2018-10-25T21:38:56Z

No other concerns.

…

On Thu, Oct 25, 2018 at 5:34 PM Eliza Weisman ***@***.***> wrote: @davidbarsky <https://github.com/davidbarsky> I think I've addressed all your feedback --- any other concerns? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#49 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB-NPjsCa9Ywc4emJ3Sx5seLlh23cmsOks5uoi55gaJpZM4X5QVc> .

hawkw added the kind/enhancement New feature or request label Oct 24, 2018

hawkw self-assigned this Oct 24, 2018

hawkw requested a review from davidbarsky October 24, 2018 23:54

hawkw commented Oct 24, 2018

View reviewed changes

davidbarsky reviewed Oct 25, 2018

View reviewed changes

hawkw added 6 commits October 25, 2018 11:01

Basic skeleton for 'follows_from' in core

098a40e

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

Add follows_from APIs to subscriber crate

c317a4a

disembreak log crate, examples

c2c78f0

Add method for looking up follows-from

bcc8179

Make add_follows_from falliable

ea6b26e

rustfmt

620924a

hawkw force-pushed the eliza/follows-from-sketch branch from 40edea6 to 620924a Compare October 25, 2018 18:02

name bikeshed

a356d47

hawkw merged commit ca00836 into master Oct 25, 2018

hawkw deleted the eliza/follows-from-sketch branch October 25, 2018 21:54

hawkw added this to Done in core 0.1 Oct 30, 2018

hawkw mentioned this pull request Oct 30, 2018

bikeshed: add_prior_span #59

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add trait methods for `follows_from` #49

Add trait methods for `follows_from` #49

hawkw commented Oct 24, 2018

hawkw Oct 24, 2018

davidbarsky Oct 25, 2018

hawkw Oct 24, 2018

davidbarsky Oct 25, 2018

hawkw Oct 25, 2018

hawkw Oct 24, 2018

hawkw Oct 24, 2018

hawkw Oct 24, 2018

davidbarsky Oct 25, 2018

hawkw Oct 25, 2018 •

edited

Loading

davidbarsky Oct 25, 2018

hawkw Oct 25, 2018 •

edited

Loading

davidbarsky left a comment

hawkw commented Oct 25, 2018

hawkw commented Oct 25, 2018

davidbarsky commented Oct 25, 2018 via email

Add trait methods for follows_from #49

Add trait methods for follows_from #49

Conversation

hawkw commented Oct 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hawkw Oct 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hawkw Oct 25, 2018 • edited Loading

Choose a reason for hiding this comment

davidbarsky left a comment

Choose a reason for hiding this comment

hawkw commented Oct 25, 2018

hawkw commented Oct 25, 2018

davidbarsky commented Oct 25, 2018 via email

Add trait methods for `follows_from` #49

Add trait methods for `follows_from` #49

hawkw Oct 25, 2018 •

edited

Loading

hawkw Oct 25, 2018 •

edited

Loading