feat(otel): otel sampling support in distributed tracing - #4177
Conversation
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f7329a3d7e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } else if (decision == DDTRACE_OTEL_SAMPLING_DECISION_PROBABILITY && | ||
| !fields.random_value && !fields.threshold && sample_rate > 0) { | ||
| ddtrace_otel_generate_fields(&fields, generated_random_value, | ||
| generated_threshold, trace_id, | ||
| sampling_priority, sample_rate); |
There was a problem hiding this comment.
ot fields after a local probability decision
When an inbound trace was dropped and a local target_span:any sampling rule makes a new probability decision, any valid inbound rv or th prevents regeneration here. The outbound traceparent can therefore say sampled while the preserved random value remains below the preserved threshold, or a partial member can omit the local threshold entirely, causing downstream OpenTelemetry services to calculate the wrong effective sampling probability. Preserve these fields only for INHERITED; a local probability decision needs fields reconciled with its own rate and final priority.
Useful? React with 👍 / 👎.
| if (members.otel_member) { | ||
| ddtrace_otel_append_member(&limited, members.otel_member, | ||
| members.otel_member_len, &member_count); |
There was a problem hiding this comment.
With a large dd member—possible because propagated Datadog tags default to a 512-byte allowance—the first append can consume nearly the entire tracestate limit and the ot append then returns false. Its failure is ignored, and the later loop deliberately skips that ot member, so the emitted header silently loses the new sampling threshold exactly on traces carrying substantial propagated tags. The limiting pass should reserve capacity for both owned members or deliberately trim/evict lower-priority data.
Useful? React with 👍 / 👎.
| // W3C Trace Context permits at most 32 list-members. Owned members | ||
| // are removed or normalized after this pass, then protected when | ||
| // the outbound header is rebuilt. | ||
| if (last_comma && ++commas == 32) { |
There was a problem hiding this comment.
Allowing all 32 extracted members is safe only on the outbound-header path, which runs the limiter again after rebuilding the dd member. DDTrace\SpanLink::fromHeaders() and the restart behavior instead pass this result to ddtrace_build_span_link_from_result(), where ddtrace_format_tracestate() can prepend a dd member for an extracted Datadog origin or tags without another limiting pass. An otherwise valid 32-member or 512-byte tracecontext input can therefore produce an over-limit traceState on the span link; retain the previous reservation or normalize the rebuilt link tracestate too.
Useful? React with 👍 / 👎.
| span->otel_sampling_rate = sample_rate; | ||
| span->otel_sampling_decision = mechanism == DD_MECHANISM_MANUAL || mechanism == DD_MECHANISM_ASM || limited || sample_rate <= 0 | ||
| ? DDTRACE_OTEL_SAMPLING_DECISION_NON_PROBABILITY | ||
| : DDTRACE_OTEL_SAMPLING_DECISION_PROBABILITY; |
There was a problem hiding this comment.
When DD_TRACE_SAMPLE_RATE enables the trace rate limiter and the limiter approves the current candidate, limited is false, so this records a pure probability decision and emits a threshold based only on sample_rate. At throughput above DD_TRACE_RATE_LIMIT, however, admission also depends on the token-bucket limiter: for example, approved traces at sample rate 1 emit th:0 even though many equivalent traces are rejected by the limiter. Downstream OpenTelemetry metrics then treat the surviving spans as unsampled and undercount traffic; any decision for which the active limiter was consulted must omit the probability threshold (or encode the combined effective probability), not only decisions the limiter rejected.
Useful? React with 👍 / 👎.
1 participant
Description
Add support for OTel sampling in distributed tracing, by reading and emitting ot.th and ot.rv tags. These tags will be forwarded to services using OTel SDKs, that should forward them to the backend.
Distributed tracing will work between DD-instrumented services and OTel-instrumented services, downstream OTel services will be able to forward the th field it to the collector and backend, which will calculate metrics correctly
Locally passes DataDog/system-tests#7518
Reviewer checklist