opentracing / opentracing-rust

OpenTracing API for Rust
http://opentracing.io
Apache License 2.0
76 stars 7 forks source link

API: SpanContext #6

Closed daschl closed 6 years ago

daschl commented 6 years ago

This ticket is meant to track the API for the SpanContext.

One proposal:

/// The `SpanContext` represents Span state that must propagate to descendant
/// Spans and across process boundaries.
///
/// SpanContext is logically divided into two pieces: (1) the user-level "Baggage"
/// that propagates across Span boundaries and (2) any Tracer-implementation-specific
/// fields that are needed to identify or otherwise contextualize
/// the associated Span instance (e.g., a `(trace_id, span_id, sampled)` tuple).
pub trait SpanContext {
    /// Allows to iterate over the baggage items propagating along with
    /// the associated Span.
    fn baggage_items<I>(&self) -> I where I: Iterator<Item = (String, String)>;
}
daschl commented 6 years ago

As per #2, @stefano-pogliani will start working on this

stefano-pogliani commented 6 years ago

As SpanContexts are propagated through code and attached to references should they implement Clone?

I would say so but we can also have the Span::context method return a SpanContext (which is not clonable) and if the user needs more then one SpanContext they can call the Span::context method multiple times?

PS: For reference, link to the java implementation https://github.com/opentracing/opentracing-java/blob/master/opentracing-api/src/main/java/io/opentracing/SpanContext.java

daschl commented 6 years ago

@stefano-pogliani wait, why would you need more than one SpanContext per span? I wonder if the Span should own the context and only hand out a mutable or immutable borrow so you can attach baggage?

stefano-pogliani commented 6 years ago

@daschl each Span will always have exactly one SpanContext.

I was more thinking of what the equivalent of java's addReference would look like.

Can we pass a reference? If so, what should the reference lifetime be? Would references be clashing with follows-from style (span) references (where the partent span can be dropped before the child span is)?

If we have the Span take ownership of the SpanContext it references lots of complexity goes away. But for that to work a SpanContext needs to be ... cloned? Extracted multiple times from a Span?

In my implementation I:

So the using code with Clone would look something like:

let span = tracer.span(...);
let context: &SpanContext = span.context();
// ... snip ...

for _ in 1..9 {
    let mut child_span = tracer.span(...);
    child_span.follows_from(context.clone());
}

Or another option I can think of is to return SpanContexts and not require clones:

let span = tracer.span(...);
// ... snip ...

for _ in 1..9 {
    let mut child_span = tracer.span(...);
    let context: SpanContext = span.context();
    child_span.follows_from(context.clone());
}
stefano-pogliani commented 6 years ago

Hi @daschl, I have been trying to implement the SpanContext as below and I keep getting issues with lifetimes or template parameters:

Version 1: https://play.rust-lang.org/?gist=7b1e387c2800925f17810c0fa00e9d00&version=stable This fails because the concrete type in the implementation (HashMap's Iter type) does not match I from the trait.

Version 2: https://play.rust-lang.org/?gist=4a6def2cf2329e53a44e2e938220b555&version=stable I am not sure how to tell the compiler that the references in this iterator are bound to the struct.

I also tried to use Iterator<Item = (String, String)> to avoid dealing with references for now but I still get an error when I try to use a concrete iterator type. Version 3 shows this: https://play.rust-lang.org/?gist=cb81aa90de84dfc11cb350d5e6be0fb3&version=stable

Could you help me understand what am I doing wrong?

stefano-pogliani commented 6 years ago

I figured out what I was doing wrong, ignore me :smile: I should have a PR for this soon.

daschl commented 6 years ago

Awesome, sorry for being slow - still need to update the other PR as well with enhancements. Looking forward to the PR

stefano-pogliani commented 6 years ago

Hi @daschl ,

I was able to come up with some code that defines the SpanContext trait.

I also started the util crate with a module so that traits can have a boxed equivalent. I hit a wall with lifetimes on this and wanted a second opinion so I tried to push the code for a PR. Github denied my push command, could I have push rights?

daschl commented 6 years ago

@stefano-pogliani great news!

Actually I don't think you need push access. Just fork on github and push to your fork, then do a PR. I'll do the same going forward as well since its the easiest way to collaborate (the first branch I pushed because I was lazy, but I shouldn't do that). Does that work?

stefano-pogliani commented 6 years ago

@daschl see PR #10

daschl commented 6 years ago

Merged! thanks much