WIP: Span #12

daschl · 2018-03-13T13:12:19Z

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

I don't like that we somehow take str, sometimes String.. it feels not polished
is the tag okay with TagValue?
I think with Baggage we can only accept String as value anyways
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?
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)

stefano-pogliani

I left a few comments here and there.

Something that I noticed is missing (both here and in the Java interface) is the span referencing logic.
I am not sure how opentracing-java is meant to do child of/follows from/possible extensions.

My reading of https://github.com/opentracing/specification/blob/master/specification.md#references-between-spans is that Spans should carry these references and it seems to be missing.

stefano-pogliani · 2018-03-13T22:21:33Z

opentracing-api/src/span.rs

+    /// Retrieve the associated `SpanContext`.
+    ///
+    /// This may be called any time, including after `finish`.
+    fn context(&self) -> &'a Self::Context;


I have been debating in my own head if we want this to return an owned SpanContext.
This is because we have not required for SpanContext to be Clone.
The SpanContext should be Clone or returned owned is that it will be used for references (child of/follows from).

How do you think references should look like?

The SpanContext should be Clone or returned owned is that it will be used for references (child of/follows from). -> if thats the case then I think we should add the clone constraint so users can clone if needed? .. let's see how it turns out down the road once we do more impls

I agree it is best if we mark SpanContext: Clone as that gives more flexibility to users:+1:

stefano-pogliani · 2018-03-13T22:27:45Z

opentracing-api/src/span.rs

+    fn set_tag(&mut self, key: &str, value: TagValue);
+
+    /// Record an event at the current walltime timestamp.
+    fn log(&mut self, event: String);


I'm thinking the event type should be some specialised type.
According to the spec this is a timestamped Key/Value map where the map can be anything.

Personally I went for a LogValue enum that mirrors the TagValue and a Log struct that has a timestamp and a String => LogValue map.
Not sure if that is the best approach though.

stefano-pogliani · 2018-03-13T22:29:08Z

opentracing-api/src/span.rs

+    fn context(&self) -> &'a Self::Context;
+
+    /// Sets a key:value tag on the `Span`.
+    fn set_tag(&mut self, key: &str, value: TagValue);


Should the key be a String?
We are likely taking ownership/cloning it to add it to a map of sorts.

stefano-pogliani · 2018-03-13T22:30:03Z

opentracing-api/src/span.rs

+
+    /// the value of the baggage item identified by the given key, or None if no such item
+    /// could be found.
+    fn get_baggage_item(&self, key: &str) -> Option<&String>;


Should the return value be Option<&'a String>?

stefano-pogliani · 2018-03-13T22:33:33Z

opentracing-api/src/span.rs

+    /// call made to the span instance. Future calls to `finish` are defined
+    /// as noops, and future calls to other methods other than `context()`
+    /// lead to undefined behavior.
+    fn finish(&mut self);


I used a finish(self) -> FinishedSpan approach to have the type system enforce the distinction between a finished and unfinished span.
The FinishedSpan takes ownership of the state of the Span but only implements read-only methods to revent editing.

I found it quite a nice approach, what do you think?

I like that!

stefano-pogliani · 2018-03-13T23:23:46Z

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 · 2018-03-14T07:08:11Z

@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 · 2018-03-14T19:21:14Z

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 · 2018-03-14T19:46:30Z

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

daschl · 2018-03-15T08:18:03Z

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 · 2018-03-15T21:46:14Z

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

daschl · 2018-03-16T07:31:22Z

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

still todo: - add LogValue - implement log - implement finish - add docs to everything once done

stefano-pogliani

I'm still unclear how References are used.
Could you show me an example of a child span being created?

stefano-pogliani · 2018-03-25T13:30:47Z

opentracing-api/src/span.rs

+
+    /// Allows to unset a tag based on the given key. Noop if
+    /// it doesn't exist.
+    fn unset_tag<S>(&mut self, key: S)


This is not technically part of the spec and I did not see it in the Java interface.
Should this be provided by implementations rather then the interface?

stefano-pogliani · 2018-03-25T13:31:50Z

opentracing-api/src/span.rs

+
+    /// Allows to unset a baggage item based on the given key. Noop if
+    /// it doesn't exist.
+    fn unset_baggage_item<S>(&mut self, key: S)


Same as above.
I'm not sure we should add non-standard features to the core interface.

stefano-pogliani · 2018-03-25T13:44:11Z

opentracing-mock/src/lib.rs


 pub struct MockSpanContext {
    baggage: HashMap<String, String>,
 }

 impl MockSpanContext {
    /// Create a new `MockSpanContext` with the given baggage.
-    pub fn new(baggage: HashMap<String, String>) -> Self {
+    fn new(baggage: HashMap<String, String>) -> Self {


I think because this is a mock library we probably do want this to be public.

stefano-pogliani · 2018-03-25T14:24:02Z

opentracing-api/src/span.rs

+        S: Into<String>;
+
+    /// Record an event at the current walltime timestamp.
+    fn log(&mut self, event: String);


With regards to the event type: what if it was any serde::Serialize type?

fn log<E>(&mut self, event: E) where E: serde::Serialize;

Alternatively we can do what slog does and define an Event trait with an interface tailored to use by tracers.
Common types in the standard library would get an implementation of Event and users can implement it for any other type then need to log.

mohabusama · 2019-04-09T21:53:44Z

opentracing-api/src/span.rs

+        S: Into<String>;
+
+    /// Returns the operation name if set, None otherwise.
+    fn operation_name(&self) -> &String;


Should it be Option<&String>?

daschl mentioned this pull request Mar 13, 2018

Span API Discussion #13

Open

stefano-pogliani reviewed Mar 13, 2018

View reviewed changes

daschl force-pushed the span branch from 236eb02 to bf4359a Compare March 15, 2018 08:17

daschl force-pushed the span branch from bf4359a to fecbe70 Compare March 15, 2018 08:34

daschl force-pushed the span branch from fecbe70 to 77f7cc2 Compare March 20, 2018 17:08

WIP: Span

2715bcc

still todo: - add LogValue - implement log - implement finish - add docs to everything once done

daschl force-pushed the span branch from 77f7cc2 to 2715bcc Compare March 21, 2018 08:34

stefano-pogliani reviewed Mar 25, 2018

View reviewed changes

james-callahan mentioned this pull request Aug 9, 2018

Misc changes opentracing/opentracing-lua#5

Merged

mohabusama reviewed Apr 9, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Span #12

WIP: Span #12

daschl commented Mar 13, 2018 •

edited

Loading

stefano-pogliani left a comment

stefano-pogliani Mar 13, 2018

daschl Mar 14, 2018

stefano-pogliani Mar 14, 2018

stefano-pogliani Mar 13, 2018

stefano-pogliani Mar 13, 2018

stefano-pogliani Mar 13, 2018

stefano-pogliani Mar 13, 2018

daschl Mar 14, 2018

stefano-pogliani commented Mar 13, 2018

daschl commented Mar 14, 2018

stefano-pogliani commented Mar 14, 2018

daschl commented Mar 14, 2018

daschl commented Mar 15, 2018

stefano-pogliani commented Mar 15, 2018

daschl commented Mar 16, 2018

stefano-pogliani left a comment •

edited

Loading

stefano-pogliani Mar 25, 2018

stefano-pogliani Mar 25, 2018

stefano-pogliani Mar 25, 2018

stefano-pogliani Mar 25, 2018

mohabusama Apr 9, 2019

WIP: Span #12

Are you sure you want to change the base?

WIP: Span #12

Conversation

daschl commented Mar 13, 2018 • edited Loading

stefano-pogliani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefano-pogliani commented Mar 13, 2018

daschl commented Mar 14, 2018

stefano-pogliani commented Mar 14, 2018

daschl commented Mar 14, 2018

daschl commented Mar 15, 2018

stefano-pogliani commented Mar 15, 2018

daschl commented Mar 16, 2018

stefano-pogliani left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschl commented Mar 13, 2018 •

edited

Loading

stefano-pogliani left a comment •

edited

Loading