Span context interface

[stefano-pogliani]

@daschl as promised, the SpanContext interface :smile: As mentioned I also added the opentracing-util crate.

So far it only includes the code to wrap a SpanContext into boxes. There is a problem though: if the BoxedSpanContext::baggage_items is called the compiler complaints that the BoxedSpanContext instance may not leave long enough. The error is visible even if I wrap the entire call into a nested block (so that the BoxedSpanContext instance would be guaranteed to have a longer lifetime).

I am sure I have failed to properly declare my lifetimes and so now the compiler does not realise the borrow is dropped when the result of BoxedSpanContext::baggage_items is dropped but I can't see where I went wrong.

It would be nice to have another pair of eyes on this. The error from rust (rustc 1.24.0 (4d90ac38c 2018-02-12)):

error[E0597]: `context` does not live long enough
   --> opentracing-util/src/runtime.rs:155:9
    |
155 |         context.baggage_items();
    |         ^^^^^^^ borrowed value does not live long enough
156 |     }
    |     - `context` dropped here while still borrowed
    |
    = note: values in a scope are dropped in the opposite order they are created

error: aborting due to previous error

[stefano-pogliani]

@daschl I have spend almost three days trying to figure out what I did wrong in the use of references and lifetimes.

I have re-worked things to that the baggage items are access through a Iterator<Item = (String, String)> instead of Iterator<Item = (&String, &String)>. This allowed me to drop all the lifetime complications and reach a working setup.

What do you think?

[daschl]

@stefano-pogliani hmm.. but this means that you need to clone on every iteration (basically hardcoded in the trait itself) which seems a bit wasteful?

I'll take a look at the original approach, I hope we can find a way where we don't force clones

[stefano-pogliani]

I know @daschl, I don't like it either but could not find a better way to fix it.

Thanks for looking, it would be great if you could help me figure out a fix to the version with references.

[daschl]

@stefano-pogliani actually looking at it further, we may be over thinking this. Maybe we don't need a boxed version but rather return a Box<SpanContext<'a>> from the span? Boxing the initialized struct context should be possible.

Also, I think it would make sense to split this PR up in two. One which covers the -api trait and then another one for the util one.. so we can merge in the -api one and then keep iterating on util. Again. maybe we don't even need a special boxed version if the span trait returns a Box<SpanContext<'a>> ?

One more thought: we could also go down the route of trying to not have a associated type but rather an actual BaggageIterator struct where the trait impls need to create one. This is the recommended approach until impl trait is stable I think and it might help making the trait easier to work with? (similar to https://rust-lang-nursery.github.io/api-guidelines/future-proofing.html#newtypes-encapsulate-implementation-details-c-newtype-hide)

[stefano-pogliani]

Hi @daschl, Thanks for looking over my suggestions.

Good news: there is a much better way to deal with this with enums instead of Boxes. It gets rid of all the complex nesting of generics and boxes and lifetimes. I'll update this PR removing the pointless commits.

First though:

• The idea of returning Box<SpanContext<'a>> from Span came to me but the problem is that the Iter type needs to be specified. This means that not all span contexts can be boxed equally. This just gets equally messy down the line I think.

• The BaggageIterator idea is interesting. How do you envision its use? I looked at the docs and I'm not sure what would we gain as opposed to associated items?

As an aside, below is an example approach to runtime tracer choice with enums. Would you mind giving me an opinion on it?

use tracer1;
use tracer2;

enum Iterators<'a> {
    Tracer1(tracer1::SpanContext<'a>::Iter),
    Tracer2(tracer2::SpanContext<'a>::Iter),
}
impl<'a> Iterator for Iterators<'a> {
    ...
}

enum Contexs<'a> {
    Tracer1(tracer1::SpanContext<'a>),
    Tracer2(tracer2::SpanContext<'a>),
}
impl<'a> SpanContext<'a> for Contexts<'a> {
   ...
}

enum Spans<'a> {
    Tracer1(tracer1::Span<'a>),
    Tracer2(tracer2::Span<'a>),
}
impl<'a> Span<'a> for Spans<'a> {
   ...
}

enum Tracers<'a> {
    Tracer1(tracer1::Tracer<'a>),
    Tracer2(tracer2::Tracer<'a>),
}
impl<'a> Tracer<'a> for Tracers<'a> {
   ...
}

fn main() {
    let tracer = Tracers::Tracer1(make_some_tracer());
    // ... snip ...
}

[daschl]

Awesome, I think the enum idea is great. Once we have the full API fleshed out we can just include an example that shows how to use it. I think in general if one really goes down that route of supporting more than one at runtime (its easier at compile time), then it also makes sense to have some abstractions over the span to send and then there is one "receiver" or so in the application that funnels it into the used tracer. So you can keep the code clean and you have only one spot where you need to do the matching/dispatch!