Remove Span.get_trace_attribute

[michaelsembwever]

Remove Span.get_trace_attribute

• simply initial api (just one way of doing things)
• avoid Heisenberg tracing

References:

• In discrete steps, Simplify the API opentracing-java#7 (comment)
• Remove Span.get_trace_attribute #47

[yurishkuro]

this does not remove Heisenberg effect, since the end app still can get the attribute, it just makes the API harder to use.

[yurishkuro]

Also, this won’t work given current semantics, because the OT tracer has no obligation to return the key/value map with any specific keys. For example, it can merge all trace attributes into a single base64 string and return a single key in the map. Or, more likely, it can prepend a certain prefix to all attribute keys so that the map keys are not mixed up with any other random HTTP headers, as well as to allow the tracer on the receiving side to read only the headers that represent trace attributes.

[michaelsembwever]

For reference sake Heisenberg Tracing refers to:

When tracing goes from only instrumentation to having additional features (that the application becomes dependant on) then;

The act of observation/instrumentation itself changes what is observed.

Now there's no such thing as pure observation-only code. Even logging interferes (synchronous logging with disk IO and latency, asynchronous with memory). And there's an argument that because the DCP mechanism is happening behind the scenes that we're already on that slippery slope and might as well slide down it.

By removing span.get_trace_attribute we're letting Span remain more of a clean instrumentational API. The functionality is still there but moved into a more flexible, and more DCP, part of the API.

Against the current SpanPropagator API the functionality might get lost, but will definitely remain with #45 in place.

[yurishkuro]

@michaelsembwever please see my second comment above, #47 (comment). We cannot rely on the name in the kv-map encoding to retrieve the attribute by the name end-user knows.

[michaelsembwever]

@yurishkuro let's continue this once #45 is sorted. I believe there is ample ways of doing it, but it'll be more clear afterwards.

A quick example is one can use the vanilla map extractor

tracer.getSpanInjector(Map.class).inject(span, map)
map.get("auth-token")

But more to he point, I think we'll see clearer ways of doing even that once the Injector/Extractor discussion evolves.

[yurishkuro]

agree, let's wait, but fwiw, with our tracer implementing OT the only way it would work is

tracer.getSpanInjector(Map.class).inject(span, map)
map.get("uberctx-auth-token")

because the tracer prepends uberctx- to all attribute names when exposing as a map, and the end user cannot rely on that.

Also, as a side note, the distributed context propagation was widely viewed as a positive feature by people at Zipkin workshop, I don't recall anyone objecting to it. In implementations like grpc, the context allows user code to read back the attributes. So why, in OpenTracing, should we try so hard to hide it?

gRPC examples:

• passing metadata: http://www.hward.com/golang-grpc-context-client-server/
• passing trace ID as metadata: harlow/go-micro-services@221a67b

[codefromthecrypt]

I think span is the wrong place to add general purpose propagated attributes. I'll repeat a comment I've made many times, especially as DCP came up for the third time at our third workshop.

The workshop discussed a decoupled context (tag set), baggage, an independent api. It does not require routing through a tracer api to get to baggage. Ex. my scheduler, or metrics api does not need to hold a reference to the tracer api to get the same context. This is what is generally sought out (and in fact implemented in tons of frameworks!)

It is very important to understand a vote for distributed context propagation is not necessarily a vote for Span/Tracer.setAttribute(arbitrary baggage). The fact that DCP is stuffed inside the Tracer api is the polarizing topic, not the act of propagating tags. The day we finally decouple this, is the day we can stop losing time in cyclic discussion.

[dkuebric]

@adriancole I agree, I think it's conceptually much better -- however, in a practical sense I'm not sure exactly how to place DCP in context with OT interface. I thought that perhaps OT could extend a DCP API with a TraceContext object, so I prototyped it over the weekend.

Unfortunately, my results ended up being less satisfactory than I hoped. I discovered this in the process of converting all the use-cases.md examples (and also thinking about what examples are not covered there).

In the spirit of showing reasoning behind things as you mentioned on Monday--and also hoping for a better solution from someone else--here's the PR in case anyone is interested: dkuebric#1

[bhs]

@adriancole I hear you (in fact, I will point out that my initial forays here separated the DCP stuff as part of TraceContext)... the question in my mind is how to "make the simple things simple" while still supporting the cleanest DCP-friendly API we can muster. I guess I would encourage anyone and everyone to try and solve this via a diff to the core use-cases (like @dkuebric did). In my experience it was (too) challenging to both cleanly separate DCP and keep vanilla span creation and propagation "concept-lean".

[bhs]

PS: this is an excellent thing to discuss at our inaugural hangout in a few hours!

[codefromthecrypt]

Would love to join, but my flight plans seem to be clash quite consistency with these catchups :( just wanted to send some thoughts before I became AWOL in transit.. And on that note boarding

[dkuebric]

@adriancole safe flight! Here's the notes from @bensigelman on the hangout: #44 (comment)

For the relevant part, discussion on Attributes: they are important enough that OT wants them, but context isolation not important enough for OT to make extra work for instrumentation that would be required by a clean separation of context and span.

(I would have been happy for a clean API that both separates the two and also does not require extra work in propagating multiple objects. Unfortunately I was not able to produce one and in the process of trying to do so convinced myself it is quite a challenge.)

It will still be quite easy to implement a context-only tracer, which uses the same instrumentation (for context propagation purposes only) and no-ops Spans. The semantic issue faced is that Span will be the interface to the Attribute data, which isn't a very obvious API as you point out above.

[yurishkuro]

I think we have reached the agreement on the video call to keep read/write methods for trace attributes.

Suggest to close this in 48hrs.

[michaelsembwever]

Sorry to bring this up late but…

Why not have just the get_tag method instead of the get_trace_attribute method?

Since the get_tag is going to return trace attributes (if trace attributes are implemented).

[yurishkuro]

I am not sure what you mean, tags and attributes are different sets of data. We do not expose read function for tags in the API, they are write-only, reported to storage out of band, and tracers are not required to guarantee that all tags will even be preserved.

[michaelsembwever]

I am not sure what you mean, tags and attributes are different sets of data. We do not expose read function for tags in the API, they are write-only, reported to storage out of band, and tracers are not required to guarantee that all tags will even be preserved.

That is, has the decision been made that the two sets are kept separate.
Or… could a get_tag call return a trace attribute?

That is can trace attributes be exposed via tags. This would make sense because these are the things are reported to storage. But it wouldn't make sense because of the different value types permitted.

My questioning was as much to ask for the reference to the discussion/rationale to any decision made on this front. " We do not …" doesn't cut it.

[yurishkuro]

I don't have specific pointers to discussions, as this topic was touched upon in various other related discussions. I can try to summarize:

• Currently, a Span has setTag and setTraceAttribute methods. They are separate because the semantics of tags an attributes are very different, as documented in the docs site.
• So far nobody has requested read access to the tags. In fact, no other piece of data on the Span has read API aside from trace attributes. I've seen implementations of several different tracers, none of them expose span data back to the application.
• Trace attributes have read/write API by design, because that is their raison d'être, to allow services to propagate metadata to downstream calls.

[michaelsembwever]

I can try to summarize…

thanks.

[bhs]

@yurishkuro @michaelsembwever

+1 on Yuri's summary. I will concede that "trace attributes" never really rolled of the tongue (and still doesn't). I had a nice VC with Rodrigo Fonseca and Jonathan Mace (author of the excellent PivotTracing paper among other things) and I started to like the term "baggage" since real-life baggage is something you "carry with you", much as these key:value pairs are carried with the trace. So I am open to something like SetBaggage/GetBaggage; it's a non-standard name, but it's also a non-standard idea and would maybe help to get users to actual RTFComments (a particularly good thing in this case).

I would like to keep them totally disjoint from "span tags", though, especially since they are "beta" compared with the rest of the OT API.

[yurishkuro]

I am not opposed to "baggage".

[dkuebric]

I like "baggage" as well. "Trace attribute" is probably one of the more confusing terminology issues right now. This would make it better differentiated from tags.