RUMM-2165: Add trace sampling for instrumented network requests #934
Conversation
Codecov Report
@@ Coverage Diff @@
## release/1.13.0 #934 +/- ##
==================================================
- Coverage 83.04% 82.95% -0.09%
==================================================
Files 267 267
Lines 9060 9091 +31
Branches 1457 1467 +10
==================================================
+ Hits 7523 7541 +18
- Misses 1140 1148 +8
- Partials 397 402 +5
|
| ) | ||
| tracedRequestBuilder.addHeader(SAMPLING_PRIORITY_HEADER, DROP_SAMPLING_DECISION) | ||
| } else { | ||
| tracer.inject( |
There was a problem hiding this comment.
shouldn't you add the SAMPLING_PRIORITY_HEADER here with 1 ? Otherwise when you check for it in the TracingInterceptor the way I've seen you are going to apply a sampling there which is not wanted if the request was not sampled out here.
There was a problem hiding this comment.
Good question :) I was thinking about it, but let it as it is now. So normally the expectation is that Injector will add it, because this is what DatadogHttpCodec is doing here
We could check if header is there after the injection and if it is not, we could add it, but the thing is that Request.Builder has no method to check if header is already there, and calling removeHeader + addHeader (because simple addHeader just adds it and not replacing if there is an existing one) can set a header with a value different from injector. But anyway, with DatadogHttpCodec everything should be fine.
Now the question is: what if customer is using non-Datadog tracer (generally speaking it is possible, because we accept high-level io.opentracing.Tracer interface, not necessary Datadog tracer). So it won't be DatadogHttpCodec there and so maybe x-datadog-sampling-priority won't be set in this branch. If it is not set, then simply on the backend side service will make its own decision about the sampling, so not a big deal.
And since we are accepting io.opentracing.Tracer and io.opentracing.Span interface which generally have a way to set priority sampling, but there is no way to read it, we have to rely on the headers with such kind of hacky decision.
We could assume that Datadog tracer is always used (and then use DDSpan, which have an opportunity to read back the sampling priority) and not rely on the headers, but then traceSamplingRate parameter would be useless if customer is using non-Datadog tracer and so they will face the new billing model in a not very nice way.
There was a problem hiding this comment.
This is done automatically by the inject method
| ) | ||
| tracedRequestBuilder.addHeader(SAMPLING_PRIORITY_HEADER, DROP_SAMPLING_DECISION) | ||
| } else { | ||
| tracer.inject( |
There was a problem hiding this comment.
This is done automatically by the inject method
What does this PR do?
This change adds the trace sampling for auto-instrumented network requests (instrumentation via
TracingInterceptor,RumInterceptorandDatadogInterceptor) by addingx-datadog-sampling-priorityheader to the outgoing requests which may have a value of1(keep) or0(drop). This header will later be parsed on the backend side and sampling decision will be respected for the services downstream.Sampler will make a decision if request should be traced (by injecting tracing headers) or not. This will allow customer to control amount of traces created directly (via
TracingInterceptor) or indirectly (via RUM2APM integration in case ofDatadogInterceptororRumInterceptor), which is necessary to align with a new billing model. Default trace sampling is set to 20%.The sampling is applied only to the tracing part of the tracking, meaning that if, for example, trace sampling rate is 75% and 100 requests were made, in case of
DatadogInterceptororRumInterceptorit will be 100 RUM resources created, but only 75 spans roughly.Also this change removes
x-datadog-sampledheader, which is not really used.Generally, we can add the sampling priority information to the span in the following way:
MutableSpan, we can useMutableSpan#setSamplingPrioritySpan#setTagwith valuesTags.MANUAL_KEEP(orMANUAL_DROP)The problem is that in the former case
MutableSpanexists only for spans created by Datadog tracer, but we need also support non-Datadog tracers, so we cannot rely on this interface. In the latter case there is no way to read a tag from the span and it is also not available in theSpanContextinterface.So in this implementation we have to access
x-datadog-sampling-priorityheader directly for reading/writing sampling decision instead of relying on theSpan(and we need to add it explicitly anyway even if non-Datadog tracer is used, because in the end data is sent to the Datadog servers anyway).Also this PR includes commit 2a08eae which is optional and can be discussed - it just makes chained interceptors respect sampling decision for the request which was made in the upstream interceptor.
Review checklist (to be filled by reviewers)