openzipkin-contrib / brave-opentracing

Bridge between OpenTracing and Brave
http://opentracing.io
Apache License 2.0
64 stars 39 forks source link

BraveTracer#extract doesn't handle all 3 types of TraceContextOrSamplingFlags #67

Closed codefromthecrypt closed 6 years ago

codefromthecrypt commented 6 years ago

From @tabdulradi on January 23, 2018 22:13

brave.opentracing.BraveTracer#extract doesn't handle all 3 types of TraceContextOrSamplingFlags. In line 184, only result.context() is called (which returns a value only if it's type == 1), ignoring the other 2 cases (result.traceIdContext() and result.samplingFlags()).

I am using BraveTracing with AWSPropagation, whose Extractor does return a type 2 TraceContextOrSamplingFlags if the header doesn't have a Parent information.

Copied from original issue: openzipkin/brave#585

codefromthecrypt commented 6 years ago

I actually encountered this, but didn't raise an issue as the way that people handle situations like this is under assumptions that a SpanContext includes a span :) For example, there are some instrumentation that do things like check null and use that to decide if something is a parent or not https://github.com/openzipkin-contrib/brave-opentracing/issues/49

That said, I think following the null == no incoming context is worse than doing the right thing. Will change the code (cc @jcchavezs @basvanbeek)

codefromthecrypt commented 6 years ago

the general approach here is to take the entire extraction data and treat it as a SpanContext. Then unwrap it and let the tracer decide what to do with it.

codefromthecrypt commented 6 years ago

notably, this will also prevent us from dropping baggage when there is no trace ID

codefromthecrypt commented 6 years ago

looks like jaeger have addressed this in a tracer-specific way a long time ago https://github.com/jaegertracing/jaeger-client-java/blob/7f01beeaa86c853c9fe796bf9f6fb30bbaf3131d/jaeger-core/src/main/java/com/uber/jaeger/Tracer.java#L380

@felixbarny @wu-sheng @tylerbenson are you also addressing this issue of partial extraction result (just sampled, baggage, or trace ID)? if so, how. Seems something the api could take care of. Right now, the only options are SpanContext or null, and seems unexpected that we'd need to hide partial state. Would be better with a separate result object for extract, as that's where the problem is.

codefromthecrypt commented 6 years ago

work in progress here https://github.com/openzipkin-contrib/brave-opentracing/pull/68

tylerbenson commented 6 years ago

I’m sure there are lots of edge cases like this we don’t really cover yet.

wu-sheng commented 6 years ago

First of all, SkyWalking treat sampled and other context separately.

Sampling is not propagate, because we are auto-instrument based. If exist the context in headers, then sample the trace, otherwise, start the sampling mechanism. You can say, sample mechanism only works for the entry point.

Partial context is not allowed in SkyWalking context so far. Extract like this: https://github.com/apache/incubator-skywalking/blob/master/apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/ContextCarrier.java#L114

And valid the context like this: https://github.com/apache/incubator-skywalking/blob/master/apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/ContextCarrier.java#L140

codefromthecrypt commented 6 years ago

Ok so sounds like only B3 and jaeger tracers have this concern so far (amazon is indirect because using brave you can use amazon)

Whenever someone propagates correlation-context or anything else tracer is responsible to handle, will pop up again.

For now will just handle locally and glad you are aware of it. Surprised this wasnt discussed earlier but better late than never

On 25 Jan 2018 2:25 pm, "吴晟 Wu Sheng" notifications@github.com wrote:

First of all, SkyWalking treat sampled and other context separately.

  • Sampling is for TracingContext

Sampling is not propagate, because we are auto-instrument based. If exist the context in headers, then sample the trace, otherwise, start the sampling mechanism. You can say, sample mechanism only works for the entry point.

  • Issue of partial extraction result

Partial context is not allowed in SkyWalking context so far. Extract like this: https://github.com/apache/incubator-skywalking/blob/ master/apm-sniffer/apm-agent-core/src/main/java/org/apache/ skywalking/apm/agent/core/context/ContextCarrier.java#L114

And valid the context like this: https://github.com/apache/incubator-skywalking/blob/ master/apm-sniffer/apm-agent-core/src/main/java/org/apache/ skywalking/apm/agent/core/context/ContextCarrier.java#L140

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/openzipkin-contrib/brave-opentracing/issues/67#issuecomment-360373389, or mute the thread https://github.com/notifications/unsubscribe-auth/AAD614QdKnJPmZydbrG27onZrA-6BE9-ks5tOB5jgaJpZM4RqhBy .

codefromthecrypt commented 6 years ago

going out as 0.26.0 (cc @mpetazzoni this release also fixes the NPE issue)