Initial implementation of ContinuationLocal

[eirslett]

Initial implementation of ContinuationLocal
The implementation is a direct port of com.twitter.util.Local:

https://github.com/twitter/util/blob/fe29be6ce62d27611ddfe94fdc9e9ec294375330/util-core/src/main/scala/com/twitter/util/Local.scala
https://github.com/twitter/util/blob/fe29be6ce62d27611ddfe94fdc9e9ec294375330/util-core/src/test/scala/com/twitter/util/LocalTest.scala

An ExecutorService wrapper is also provided, meant to wrap thread
pools so that the ContinuationLocal state is passed on. This
implementation is inspired by BraveExecutorService and FuturePool:

https://github.com/kristofa/brave/blob/df55efd5653bccdb4736fd63413f10772a5bb5e2/brave-core/src/main/java/com/github/kristofa/brave/BraveExecutorService.java
https://github.com/twitter/util/blob/fe29be6ce62d27611ddfe94fdc9e9ec294375330/util-core/src/main/scala/com/twitter/util/FuturePool.scala

Here's a (more or less) direct port of com.twitter.util.Local to Java, with only a few differences.
This ContinuationLocal needs mainstream adoption in order to work (every library that's dealing with threads and/or events needs to support it), so we have to get it right.

Any comments from the Twitter guys who wrote this (in Scala) in the first place?
@adriancole @mosesn @mariusae @evnm ?

[codefromthecrypt]

@daniel-bryant-uk looking at your not so recent blog, you might be interested in this.

[codefromthecrypt]

typo

[eirslett]

Oops. Referring to the missing semicolon?

[codefromthecrypt]

Distribu'v'e A couple places in the file.

[eirslett]

ok, fixed!

[codefromthecrypt]

TL;DR;
I think a port of com.twitter.util.Local might be better in the openzipkin org, as that zipkin is the only JVM implementation that would be biased towards twitter commons idioms.

---

Disclaimer: this is just an opinion of a passer-by.. don't anchor too much on it. I'm just voicing my 2p.

---

I'm glad to see a proposal to a part of the Shared JVM Implementation problem. My opinion, but I'd define success in this org as folks actually sharing a context vs being like com.twitter.util.Local, which only zipkin is interested in. Don't get me wrong.. I do think this is valuable, just guessing it is more valuable in the openzipkin org, as zipkin is the only impl we can assume is familiar with com.twitter.util.Local.

Those who might use a java context propagation api, in terms of dapper, include HTrace (@saintstack), Brave (@kristofa), Pinpoint (@Xylus), Lyft (@jpinner), Spring Cloud (@dsyer @spencergibb) and Netflix (@NiteshKant). There's also work brewing in GRPC which will eventually support dapper itself.

[eirslett]

Yes, it's a port of com.twitter.util.Local, but that's just to have a starting point that is proven to work, not to be biased towards twitter commons in the long term (or even in the short term).
The reason why I think it belongs to distributedtracing in general, rather than openzipkin specifically, is that I think it will gain wider adoption from library authors (netflix oss, rxjava, finagle, akka...) if we keep the construct as agnostic as possible.
For example reactive streams, which is a very generic concept, would benefit from supporting continuation-local storage, but probably wouldn't support a Zipkin-specific construct because that's too implementation-specific.
I suppose Pinpoint could benefit from using this construct as well?

[eirslett]

as zipkin is the only impl we can assume is familiar with com.twitter.util.Local.

It looks like most other implementations are familiar with ThreadLocal from Java, it's just that we have to sell ContinuationLocal as simply an extension to ThreadLocal that also supports thread pools and event loops?

[codefromthecrypt]

It looks like most other implementations are familiar with ThreadLocal
from Java, it's just that we have to sell ContinuationLocal as simply an
extension to ThreadLocal that also supports thread pools and event loops

Pretty much.

I think that the 'inspired by' story is more a sideline than a headline.
Start with what this code buys you and how to use it (ex htrace homepage
does this well).

Credit inspiration, but at the same time I suspect this will deviate and
that's OK (whereas deviation in clones is usually implicitly not OK).

These suggestions follow normal disclaimer of 'toss if unneeded' ;)

[louiscryan]

Is this ultimately what we want? Might as well state your intent and go for

org.continuation

from the start

[b]

+1 to not having it in the com.github namespace.

[eirslett]

I do +1 it myself.
org.continuation is a good namespace, since it's not directly tied to distributed tracing. One could use a ContinuationLocal for other things as well, like passing around business logic state, even though I would certainly not recommend that.

[schlosna]

I'm curious what the next steps are here to get this merging, or if this is effectively at dead PR. I'm mostly interested in this in the context of openzipkin/brave#166 so if there is anything I can do to help push this, please let me know. Thanks!

[codefromthecrypt]

Jakub was looking at this, but offline. If you are in a position to tackle
this in Brave, I'd go for it. Many people would like this solved including
me.

On 10 Aug 2016 20:03, "David Schlosnagle" [email protected] wrote:

I'm curious what the next steps are here to get this merging, or if this
is effectively at dead PR. I'm mostly interested in this in the context of
openzipkin/brave#166 openzipkin/brave#166 so
if there is anything I can do to help push this, please let me know. Thanks!

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAD61-pfQBmR5Ef9HTxv_o4SUIIIXzMuks5qeb4QgaJpZM4FWrbX
.

[schlosna]

Thanks Adrian. I actually work with Jakub, and have some time this week as part of a hack week project instrumenting a variety of systems with Zipkin & Brave, so I'll create a Brave PR to iterate.

[eirslett]

You're free to pick it up from here!

[codefromthecrypt]

Keep us posted on gitter! Ps once things are sorted, local to brave, we can
also revisit this PR with experiences from it.

On 10 Aug 2016 9:13 pm, "David Schlosnagle" [email protected]
wrote:

Thanks Adrian. I actually work with Jakub, and have some time this week as
part of a hack week project instrumenting a variety of systems with Zipkin
& Brave, so I'll create a Brave PR to iterate.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAD61zbM8s2ziZFTXIIOBPAjENozOybBks5qec5ZgaJpZM4FWrbX
.

[codefromthecrypt]

some interesting development out of this is a recent addition to log4j 2.7 which incidentally came out of a feature request to pull in state from finagle locals. https://issues.apache.org/jira/browse/LOG4J2-1010

I'd say ContextDataInjector is an example of something we could attempt to implement in a usage test case, to see if we can share a context across diversely written apps.

[codefromthecrypt]

ps thanks for this.. I've grabbed the parts needed for brave. we can revisit this in the new design as context is pluggable now

openzipkin/brave#369