Fix B3 sampling API to support missing values #1640
Conversation
tkountis
added
the
breaking-change
tkountis
requested review from
Scottmitch and
idelpivnitskiy
and removed request for
Scottmitch
|
@eddie4941 - FYI. PTAL. |
| * | ||
| * @return low 64 bits of the trace ID | ||
| */ | ||
| default long traceId() { |
There was a problem hiding this comment.
Are these used in the wild, should I offer an alternative accessor through context
There was a problem hiding this comment.
lets stick with single access point on the API for now, and we can add utility methods if necessary.
| if (parentSpanIdHex != null) { | ||
| carrier.set(PARENT_SPAN_ID, parentSpanIdHex); | ||
| } | ||
| carrier.set(SAMPLED, state.isSampled() ? "1" : "0"); | ||
| carrier.set(SAMPLED, context.isSampled() ? "1" : "0"); |
There was a problem hiding this comment.
context.isSampled() may be null, add null protection?
| public void inject(InMemoryTraceState state, SingleLineValue carrier) { | ||
| carrier.set(format(state.traceIdHex(), state.spanIdHex(), state.parentSpanIdHex(), state.isSampled())); | ||
| public void inject(final InMemorySpanContext context, final SingleLineValue carrier) { | ||
| carrier.set(format(context.toTraceId(), context.toSpanId(), context.parentSpanId(), context.isSampled())); |
There was a problem hiding this comment.
context.isSampled() maybe null. check other usages of isSampled() to avoid NPE.
| * | ||
| * @return low 64 bits of the trace ID | ||
| */ | ||
| default long traceId() { |
There was a problem hiding this comment.
lets stick with single access point on the API for now, and we can add utility methods if necessary.
| * | ||
| * @return parent span ID in hex | ||
| */ | ||
| default String nonnullParentSpanIdHex() { |
There was a problem hiding this comment.
since the "hex" variation of parentSpanIdHex longer exist and this method name doesn't really make sense any more. Also the impl uses some internal types (NO_PARENT_ID) ... lets move this method to an internal utility class for now.
| */ | ||
| protected abstract InMemorySpanContext newSpanContext(InMemoryTraceState state); | ||
| protected abstract InMemorySpanContext newSpanContext(String traceId, String spanId, String parentSpanId, |
There was a problem hiding this comment.
this method is no longer called in this class, make it private to DefaultInMemoryTracer?
| */ | ||
| class DefaultInMemorySpanContext implements InMemorySpanContext { | ||
| public class DefaultInMemorySpanContext implements InMemorySpanContext { |
There was a problem hiding this comment.
can we make this class final for now (more restrictive access pattern to minimize our API surface)?
tkountis
force-pushed
the
api/opentracing
branch
from
ad5d2bc to
0ed8cd6
Compare
Motivation: B3 sampling handling was supposed to support both present and not values. However, in practise it only supported present values, and treated non existing as sampling=off. Modifications: Support Boolean values for handling sampling which now allows for not present values in the carrier, and the local sampler can make the final decision on whether to enable sampling for the local span or not. The change unfortunately broke API because of the type change, thus we took advantage of that and cleaned up some problematic relationships between classes which results in a cleaner more consistent API, which also follows closer the spec. Result: Correct sampling & cleaner API
tkountis
force-pushed
the
api/opentracing
branch
from
0ed8cd6 to
4ed2877
Compare
Motivation:
B3 sampling handling was supposed to support both present and not values.
However, in practise it only supported present values, and treated non existing as sampling=off.
Modifications:
Support Boolean values for handling sampling which now allows for not present values in the carrier,
and the local sampler can make the final decision on whether to enable sampling for the local span or not.
The change unfortunately broke API because of the type change, thus we took advantage of that and cleaned up
some problematic relationships between classes which results in a cleaner more consistent API, which also
follows closer the spec.
InMemoryTraceStatesince it was mainly duplication ofSpanContextand the spec suggests that all these variables can/should be part the context.context. see. https://github.com/opentracing/specification/blob/master/rfc/trace_identifiers.mdResult:
Correct sampling & cleaner API
Link: #1512