opentracing / opentracing-rust

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

WIP: Span #12

Open daschl opened 6 years ago

daschl commented 6 years ago

@stefano-pogliani next item, the span!

I think this one is not as complicated either but I still have some questions / stuff we should work through:

stefano-pogliani commented 6 years ago

With regards to the other things in your list @daschl:

I don't like that we somehow take str, sometimes String.. it feels not polished

Consistency would be nice. Hopefully we can get there iterating over things.

Is the tag okay with TagValue?

Yep, looks good. Should we have i64 and unsigned float and int variants too?

I think with Baggage we can only accept String as value anyways

That is what the specs say.

I'm totally unsure about the values of the log. I put in string only for now but java allows all kinds of stuff including maps .. wdyt?

My idea hinted at in the code, I guess we can discuss further in #13

Docs, but only once it is done api wise For consistency, should we add get_tag? (note that I added a getter for the op name as well which java hasn't but if we have setters we might as well have getters too)

Having a get_tag sounds nice, especially for testing. Do we also want a get_log?

By the way: getters name are recommended not to have get_: https://rust-lang-nursery.github.io/api-guidelines/naming.html#getter-names-follow-rust-convention-c-getter

daschl commented 6 years ago

@stefano-pogliani with regards to https://rust-lang-nursery.github.io/api-guidelines/naming.html#getter-names-follow-rust-convention-c-getter : If there are only getters it makes sense to omit, but i'd also find it weird if you have getter and setter to look something like:

fn tag(&self);
fn set_tag(&mut self);

vs.

fn get_tag(&self);
fn set_tag(&mut self);
stefano-pogliani commented 6 years ago

For the getters name: I'm not sure which way it is best but I'm trying to stick to the API guidelines when possible.

I am too used to the get_name and set_name version. It seems to me that rust goes for the name and set_name unlike many other languages. For example std::vec::Vec has the len and set_len methods instead of the get_len and set_len methods.

daschl commented 6 years ago

https://github.com/rust-lang/rfcs/blob/master/text/0344-conventions-galore.md#gettersetter-apis Yep looks like it - will make the modifications

daschl commented 6 years ago

Started to experiment more, will update all the span stuff as described above and also do the mock and noop impls right away so we have something to actually validate against. I think I can finish it this week - lets see :)

stefano-pogliani commented 6 years ago

That sounds great. Let me know if there is anything I can do in the meantime.

daschl commented 6 years ago

@stefano-pogliani thinking of the relation, it reminds me there is another component I totally forgot: the SpanBuilder! In java its defined as part of the tracer, but I think for us it needs to be a standalone trait as well I think (https://github.com/opentracing/opentracing-java/blob/master/opentracing-api/src/main/java/io/opentracing/Tracer.java#L109)