Opentracing Support #263
Opentracing Support #263
Conversation
Enhances the ContextPropagator API by providing more lifecycle methods, and adds an implementation of OpenTracing
|
|
| } | ||
|
|
||
| @Override | ||
| public Map<String, Payload> serializeContext(Object context) { |
There was a problem hiding this comment.
The header is a shared resource. So we cannot assume that all the keys in it are created using this specific propagator. So the solution is to store the whole context in a single header key. We also want this to be compatible with Go SDK. Here is the Go SDK open trace propagator.
There was a problem hiding this comment.
That's a good point - I will take a look at the go propagator and work on ensuring cross-platform.
| @Override | ||
| public void setUp() { | ||
| Tracer openTracingTracer = GlobalTracer.get(); | ||
| Tracer.SpanBuilder builder = openTracingTracer.buildSpan("cadence.workflow"); |
There was a problem hiding this comment.
I don't think it is going to work. The problem is that during replay a new span is going to be created.
Unfortunately, there is no way to create a span passing deterministically generated ID to it.
Another option would be using SideEffect to store the span, but that implies additional MarkerEvent for every workflow.
There was a problem hiding this comment.
Does it really matter if we start multiple spans if we only finish one? I don't think spans get reported until you finish() them.
| @@ -126,4 +130,27 @@ | |||
|
|
|||
| /** Sets the current context */ | |||
| void setCurrentContext(Object context); | |||
|
|
|||
There was a problem hiding this comment.
Looking at this and the implementation I'm getting more inclined to just using interceptors for this integration and getting rid of the context propagator interface completely as it has very limited use.
There was a problem hiding this comment.
I’d be happy to look into that - are there any docs or examples of interceptors anywhere? I didn’t see a way of propagating information from worker/client to the server and back using interceptors.
There was a problem hiding this comment.
I think you are right that there is something missing in the interceptors to support this use case. Let me think about this.
|
The newly added WorkerInterceptor should be enough to implement open tracing support. I'm going to close this PR. I would love to review a contribution that relies on the interceptor. |
Addresses #258 - bringing over the ContextPropagator improvements and adding a default OpenTracing implementation of context propagation