Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RUM-8051: Implement Head-based sampling for network instrumentation #2483

Merged
merged 1 commit into from
Jan 16, 2025

Conversation

0xnm
Copy link
Member

@0xnm 0xnm commented Jan 13, 2025

What does this PR do?

This PR implements Head-based sampling for the network instrumentation to align with default sampling existing in other Datadog tracers.

In short, Head-based sampling (HBS) allows to have a single sampling decision for the whole trace: it is kept or dropped, instead of the individual spans.

This also aligns with the following changes in iOS SDK done in the past:

Now, if there is a parent span (propagated through the headers or via request tag; OpenTracing or OpenTelemetry), then its sampling decision will be taken into account instead of the sampling decision made by the network instrumentation.

If there is no parent tracing context for the incoming request, then sampling decision will be made by interceptor.

The necessary integration tests are added, they uncovered some issues we had with trace propagation.

Also this PR bring the following change: spans with DROP sampling decision are not written, they are rejected by the intake anyway, so it makes no sense to send them.

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • Make sure you discussed the feature or bugfix with the maintaining team in an Issue
  • Make sure each commit and the PR mention the Issue number (cf the CONTRIBUTING doc)

@0xnm 0xnm requested review from a team as code owners January 13, 2025 14:54
@codecov-commenter
Copy link

codecov-commenter commented Jan 13, 2025

Codecov Report

Attention: Patch coverage is 57.53425% with 31 lines in your changes missing coverage. Please review.

Project coverage is 70.01%. Comparing base (eebc972) to head (8852339).
Report is 2 commits behind head on develop.

Files with missing lines Patch % Lines
...datadog/android/okhttp/trace/TracingInterceptor.kt 53.06% 18 Missing and 5 partials ⚠️
...otlin/com/datadog/android/okhttp/otel/OkHttpExt.kt 22.22% 5 Missing and 2 partials ⚠️
...adog/opentracing/propagation/DatadogHttpCodec.java 50.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2483      +/-   ##
===========================================
+ Coverage    69.92%   70.01%   +0.09%     
===========================================
  Files          788      788              
  Lines        29604    29647      +43     
  Branches      4944     4961      +17     
===========================================
+ Hits         20698    20756      +58     
+ Misses        7527     7520       -7     
+ Partials      1379     1371       -8     
Files with missing lines Coverage Δ
...cy/trace/common/sampling/RateByServiceSampler.java 60.87% <100.00%> (ø)
...dog/android/trace/internal/data/OtelTraceWriter.kt 89.66% <100.00%> (+1.19%) ⬆️
...datadog/android/trace/internal/data/TraceWriter.kt 93.75% <100.00%> (+0.65%) ⬆️
...adog/opentracing/propagation/DatadogHttpCodec.java 83.33% <50.00%> (-1.17%) ⬇️
...otlin/com/datadog/android/okhttp/otel/OkHttpExt.kt 30.00% <22.22%> (-45.00%) ⬇️
...datadog/android/okhttp/trace/TracingInterceptor.kt 75.21% <53.06%> (-1.80%) ⬇️

... and 32 files with indirect coverage changes

@0xnm 0xnm force-pushed the nogorodnikov/rum-8051/implement-head-based-sampling branch from ac37341 to 4ee9caa Compare January 13, 2025 15:35
@0xnm 0xnm force-pushed the nogorodnikov/rum-8051/implement-head-based-sampling branch from 4ee9caa to 8852339 Compare January 15, 2025 16:16
@0xnm 0xnm merged commit 58f27a4 into develop Jan 16, 2025
24 checks passed
@0xnm 0xnm deleted the nogorodnikov/rum-8051/implement-head-based-sampling branch January 16, 2025 08:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants