opencensus-go icon indicating copy to clipboard operation
opencensus-go copied to clipboard

ProbabilitySampler always sample when incoming trace-id is 64-bit (B3/Jaeger)

Open shanipribadi opened this issue 2 years ago • 0 comments

Probability Sampler right now takes the 8 Most Significant Bytes (leftmost / upper), parse it as a BigEndian uint64, divides by 2, and then compares it against an upper bound value based on fraction * 2^63. https://github.com/census-instrumentation/opencensus-go/blob/052120675fac2ace91dc2c01e5f63c3e6ec62f04/trace/sampling.go#L50-L56

But if we are using B3 (Zipkin) or Jaeger propagation format for incoming trace-id and the incoming trace-id is 64-bit, the 8 MSBytes taken by the sampler will always be 0 valued, and is always evaluated to be lower than the trace upper bound. This makes all requests coming from systems that are pushing 64-bit trace-id to always be sampled.

References: b3 propagation and jaeger propagation are stored in the rightmost/lower bytes (LSB) in case it's 64-bit or less. https://github.com/census-instrumentation/opencensus-go/blob/052120675fac2ace91dc2c01e5f63c3e6ec62f04/plugin/ochttp/propagation/b3/b3.go#L75-L78 https://github.com/census-ecosystem/opencensus-go-exporter-jaeger/blob/30c8b0fe8ad9d0eac5785893f3941b2e72c5aaaa/propagation/http.go#L60

The same behaviour also appears in opencensus-java https://github.com/census-instrumentation/opencensus-java/blob/a6ec0416fd0c33a5cb04083b58bacab20b2dea98/api/src/main/java/io/opencensus/trace/TraceId.java#L226 (getLowerLong returns the hi bytes rather than the lower bytes) as well as in opentelemetry-go https://github.com/open-telemetry/opentelemetry-go/blob/c05c3e237d94cc57e5a87c8fa4b39aaa8b862f65/sdk/trace/sampling.go#L84 (I assume since it copied the same logic from opencensus)

Given that the Sampler interface is available, people can just create their own fixed ProbabilitySampler implementation, But having this behaviour in ProbabilitySampler is surprising, and potentially can be costly when it happens on production traffic without it being documented.

shanipribadi avatar Apr 24 '22 10:04 shanipribadi