Brave4 instrument 166

[kmaci3k]

An attempt to implement requirements mentioned in #166.

Commits are mainly related to attaching context (i.e. for MDC) when some portion of code in done in context of a Span.

[devinsba]

My personal opinion is that we should avoid a class like this that uses statics to keep state. If there was a Factory in between the implementation and the users that would be more desirable to me

[kmaci3k]

I did it in this way because I needed a single object which would keep state for each thread.

I'm wondering how I can achieve similar result using Factory. @devinsba, maybe you have something in mind ?

[codefromthecrypt]

hehe

[codefromthecrypt]

copying in @reta as he's had similar interest

[codefromthecrypt]

Here are some notes from https://gitter.im/openzipkin/zipkin

General idea

There are two lifetimes: Lifetime of the span and Lifetime of the context

• The span’s lifetime is start to finish
• The span has a lifetime within a context (attach/detach from a thread)

https://docs.google.com/document/d/16mKbo6pWKHq30KvYYVSqo6c8uTOTkNLDSgjKwUs1PGs/edit#

Implementation idea

try (Thing attachment = tracer.withCurrentSpan(span)) {
   // now span is in a thread local, MDC whatever
}

This PR is codebase for implementing the attaching part. One thing unlikely is that this would be a hook in the Span type. I think better to be in the Tracer itself, or most separate would be an object that only deals with in-memory context propagation.

The try/finally in this case is a good one for many reasons including that the implicit close is safe (no chance of accidentally bubbling exceptions when reporting etc). Thing above is most likely a sub-type of Closeable which doesn't throw.

Takeaway

from @kmaci3k

if I understand correctly I should do following changes:
Attachment of Span to ThreadLocal should be moved from Span to Tracer (or completely separate object). TraceContext should not have anything to do with attaching to/detaching from ThreadLocal.
tracer.withCurrentSpan(span) should not return Span but an object which just closes current Span, nothing more.

[codefromthecrypt]

ps while there's no goal to exactly match, and we do want MDC plugin for sure, here's another project that has a somewhat similar api hook. you can play with it to see how it works (Even if there's only a no-op impl) https://github.com/google/instrumentation-java/blob/master/core/src/main/java/com/google/instrumentation/trace/ContextSpanHandler.java

[reta]

Could we simplify tracer API to support just something like that Span span = tracer.newTrace(parent); so the tracer will make a decision about newTrace / newChild flow?

[codefromthecrypt]

newTrace would be a bad name, right? because it is not a new trace if there's an existing parent. maybe nextSpan?

[reta]

Agree, nextSpan or startSpan, would be better ones

[reta]

Right, a bit relayed to related to previous comment, tracer.newTrace() could make a decision itself based on current span if it has to start a newTrace or newChild ... (TODO item)

[codefromthecrypt]

here's a first draft of the way to plugin custom thread state handlers #369