opentracing / opentracing-rust

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

Initial Project Layout #1

Closed daschl closed 6 years ago

daschl commented 6 years ago

Hi,

as a first step, I think we need to figure out a rough project structure. Not that we can't change it later, but we need to come up with something.

Since I think OT Java is currently the most active developed/used (correct me if I'm wrong) it makes sense to take this as a starting point.

With that in mind, I propose the following:

We might also need utils (which would hold the global tracer) and of course examples, but we can add those as soon as we need them.

ping @hanscj1 @stefano-pogliani @sile

stefano-pogliani commented 6 years ago

Looks good to me :+1: Some notes (probably more related to the API design):

daschl commented 6 years ago
stefano-pogliani commented 6 years ago

On traits/generics: say we have a trait like this one:

pub trait Span<Context> {
    fn span_context() -> Context;
    // ...
}

That means that Span<ZipkinContext> and Span<Jaeger Context> are not the same trait.

If the tracer had an API looking like the below it is not possible to have runtime configuration of the tracer.

pub trait Tracer<Context> {
    pub span(...) -> Span<Context>;
    // ...
}

Does this make sense?

daschl commented 6 years ago

@stefano-pogliani the way I understood it is that at runtime they are not the same trait implementation, so if you want to switch at runtime your API needs to take a Box<Tracer> if that makes sense. So if you want to switch at runtime you'd initialise either a JaegerTracer or a ZipkinTracer and then your downstream API would consume a Box<Tracer>.

daschl commented 6 years ago

@stefano-pogliani here is a simple version with generics: https://play.rust-lang.org/?gist=478ac54c30e3f9535eb5c988d740e47d&version=stable but honestly I think it works much better with associated types: https://play.rust-lang.org/?gist=7287f8e211c291087f7ddb722613b7f9&version=stable

stefano-pogliani commented 6 years ago

@daschl I like the associated types much better too but I still think they would cause issues with applications that want to configure the tracers in fn main().

I have added to your examples to show you what I mean:

I think it stands from the fact that in both cases Box<Tracer> is an incomplete type. I also think we need to allow for this use case to be supported.

To support this in my crate I used the Any trait and the Any::downcast_ref method. It is not nice though ...

daschl commented 6 years ago

@stefano-pogliani when you update the gist you need to generate new links, I don't see your changes on those unfortunately.

daschl commented 6 years ago

Also, lets discuss this further in #2 since API is different from project layout.

Relating to this, I'll come up with an initial PR for the project structure and we can take it from there on the PR.

stefano-pogliani commented 6 years ago

Just realised: @daschl do we want a changelog somewhere in the repo? I know it is painful to keep them up-to-date and correct but they are very useful (at least to me when using other libraries).

I don't think we need anything too advanced (version, maybe date, list of changes, highlight breaking changes is what I tend to use them for).