Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial Project Layout #1

Closed
daschl opened this issue Feb 15, 2018 · 9 comments
Closed

Initial Project Layout #1

daschl opened this issue Feb 15, 2018 · 9 comments
Labels
question Further information is requested
Milestone

Comments

@daschl
Copy link
Contributor

daschl commented Feb 15, 2018

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:

  • opentracing-rust is a cargo workspace for the following subprojects.
  • opentracing-api provides only the traits which are required to implement a tracer but doesn't have any implementation by itself. It should be very lightweight and not make any assumptions on how the actual tracer implementation is done.
  • opentracing-noop a "noop" implementation of the API, can be used if no tracer needs to be used at runtime and also serves as a sanity check durning compilation.
  • opentracing-mock a simple mock implementation of the API, useful for testing and also showcases how to implement a simple tracer.

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

@daschl daschl added the question Further information is requested label Feb 15, 2018
@daschl daschl added this to the 0.1 milestone Feb 15, 2018
@stefano-pogliani
Copy link
Contributor

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

  • While the GlobalTracer pattern may not be right approach in rust I think we need to provide libraries a way to access the tracer in the -api crate.
    For example: if Iron wanted to add tracing support, how would it access the tracer?
  • For the tracer inject/extract: I found the use of a format enum with variants that contain an appropriate carrier to be a nice interface.
    Is that just me or does it sound like a good approach?
  • If I understood the interaction between traits and generics in rust we can't really use generics in the -api traits.
    Am I correct?

@daschl
Copy link
Contributor Author

daschl commented Feb 15, 2018

  • I think the GlobalTracer can still be available but maybe in the util crate and not directly in API (java does the same thing). I don't know much about iron, but for example in rocket the tracer could also be passed around like this: https://rocket.rs/guide/state/#databases

    If there is a justifiable need to have it in the api crate I'm good with it too I think, but since rust is so nice with pulling in dependencies there might be no harm in just providing a couple different ones for different tasks.

  • On inject/extract: the big diffreence between enum and a trait is that the enum by definition is "closed" in that if we upfront know all the different variants that can be supported its the way to go. The spec defines three formats (https://github.com/opentracing/specification/blob/master/specification.md#note-required-formats-for-injection-and-extraction) but its not 100% clear if there are more allowed or not.

    @tedsuo do you know if the tracer APIs are expected to support more than the three formats defined or is it "closed" down to those three?

  • on traits/generics: I'm not sure I follow. In rust we can use generics in traits (and will have to for the API I think) - can you provide more info what you mean by that?

@stefano-pogliani
Copy link
Contributor

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
Copy link
Contributor Author

daschl commented Feb 15, 2018

@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
Copy link
Contributor Author

daschl commented Feb 15, 2018

@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
Copy link
Contributor

@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
Copy link
Contributor Author

daschl commented Feb 15, 2018

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

@daschl
Copy link
Contributor Author

daschl commented Feb 15, 2018

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.

daschl added a commit that referenced this issue Feb 15, 2018
This change applies the basic repository layout discussed in

Fixes #1
@daschl daschl closed this as completed in #3 Feb 19, 2018
daschl added a commit that referenced this issue Feb 19, 2018
* Add initial cargo crate layout.

This change applies the basic repository layout discussed in

Fixes #1

* Extend Cargo.toml with more info
@stefano-pogliani
Copy link
Contributor

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants