Fix ParentBased sampler for implicit parent spans #1394
Conversation
consider the sample decision of implicit parent spans (when creating a span without explicitly providing a context) instead of forwarding to the delegating sampler.
mariojonke
requested review from
a team,
toumorokoshi and
ocelotl
and removed request for
a team
* samplers did not return the trace_state in the sampling result when a span was dropped. This caused the trace_state extracted from a remote parent span to be erased and further context propagation to break. * fix also TraceIdRatioBased which erased the trace_state independent of the sampling outcome.
| @@ -151,7 +151,7 @@ def should_sample( | |||
| trace_state: "TraceState" = None, | |||
| ) -> "SamplingResult": | |||
| if self._decision is Decision.DROP: | |||
| return SamplingResult(self._decision) | |||
| attributes = None | |||
| return SamplingResult(self._decision, attributes, trace_state) | |||
There was a problem hiding this comment.
Do we need to pass in the trace_state for dropped spans?
There was a problem hiding this comment.
Yes as otherwise the trace breaks when the span is propagated further. E.g. flask instrumentation extracts span a span where the sampling flag in the trace parent is false. The handler in the flask app makes an outgoing HTTP request with the requests library. The requests instrumentation has the flask span as parent so the created span will be dropped. The dropped span however is used for injecting/propagation, so it needs to include the trace_state of the parent.
w3c_tracecontext tests also rely on that the trace_state gets propagated.
|
Besides the The failing BatchSpanProcessor test seems flaky. |
The
ParentBasedsampler currently is only considering parent spans if an explicit parentContextgets passed. Implicit spans in the currentContextare ignored and sampling is forwarded to the delegating sampler.With this PR also parent spans via the implicit/current
Contextare considered.Fixes #1393
Type of change
How Has This Been Tested?
Additional
ParentBasedsampler unit tests considering implicit parent spans.Checklist: