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.

Sign up for GitHub

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

Jump to bottom

Use UserKeep(2)/UserReject(-1) in the rules-based sampler #1937

Merged
merged 12 commits into from
Nov 4, 2021
Merged

Use UserKeep(2)/UserReject(-1) in the rules-based sampler #1937

merged 12 commits into from
Nov 4, 2021

Conversation

lucaspimentel
Copy link
Member

see also

To quote @dgoffredo from the DataDog/dd-opentracing-cpp PR above:

The immediate goal is to prevent the agent from overriding sampling decisions made based on sampling rules. Eventually there will be a new data structure associated with each trace at the root, and information like "who made the sampling decision" will go there. Until that work is done, though, as an interim measure we'll change the sampling priority of sampling rule based decisions. Currently they are "sampler" type, i.e. priority {0, 1}. The agent can override these. Instead, we change them to "user" type, i.e. priority {2, -1}. The agent will not override these.

@lucaspimentel lucaspimentel added type:enhancement Improvement to an existing feature area:tracer The core tracer library (Datadog.Trace, does not include OpenTracing, native code, or integrations) labels Oct 25, 2021
@andrewlock

This comment has been minimized.

Sign in to view
@andrewlock

This comment has been minimized.

Sign in to view
@lucaspimentel lucaspimentel force-pushed the lpimentel/update-rules-based-sampling-priorities branch 2 times, most recently from 57edb48 to d6c64cf Compare October 28, 2021 20:44
@andrewlock

This comment has been minimized.

Sign in to view
@andrewlock

This comment has been minimized.

Sign in to view
@lucaspimentel lucaspimentel force-pushed the lpimentel/update-rules-based-sampling-priorities branch from efc1f98 to ffa083e Compare November 1, 2021 18:49
@lucaspimentel lucaspimentel force-pushed the lpimentel/update-rules-based-sampling-priorities branch from 30500fa to c04569c Compare November 1, 2021 18:54
@lucaspimentel lucaspimentel marked this pull request as ready for review November 1, 2021 18:55
@lucaspimentel lucaspimentel requested a review from a team as a code owner November 1, 2021 18:55
@andrewlock

This comment has been minimized.

Sign in to view
@andrewlock

This comment has been minimized.

Sign in to view
andrewlock
andrewlock approved these changes Nov 3, 2021
View reviewed changes
tracer/src/Datadog.Trace/SamplingPriority.cs Outdated Show resolved Hide resolved
tracer/src/Datadog.Trace/SamplingPriority.cs Outdated Show resolved Hide resolved
lucaspimentel and others added 2 commits November 3, 2021 12:27
@lucaspimentel
update comment
7720796
@lucaspimentel @andrewlock @zacharycmontoya
Apply suggestions from code review
253f45a 
Co-authored-by: Andrew Lock <[email protected]>
Co-authored-by: Zach Montoya <[email protected]>
@andrewlock
Copy link
Member

Benchmarks Report 🐌

Benchmarks for #1937 compared to master:

  • 4 benchmarks are slower, with geometric mean 1.246
  • All benchmarks have the same allocations

The following thresholds were used for comparing the benchmark speeds:

  • Mann–Whitney U test with statistical test for significance of 5%
  • Only results indicating a difference greater than 10% and 0.3 ns are considered.

Allocation changes below 0.5% are ignored.

Benchmark details

Benchmarks.Trace.AgentWriterBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
master WriteAndFlushEnrichedTraces net472 1,000.1 μs 19.68 μs 45.99 μs 0.0000 0.0000 0.0000 3.1 KB
master WriteAndFlushEnrichedTraces netcoreapp3.1 738.8 μs 9.31 μs 8.26 μs 0.0000 0.0000 0.0000 2.51 KB
#1937 WriteAndFlushEnrichedTraces net472 1,088.6 μs 20.52 μs 17.14 μs 0.0000 0.0000 0.0000 3.09 KB
#1937 WriteAndFlushEnrichedTraces netcoreapp3.1 807.2 μs 8.87 μs 7.86 μs 0.0000 0.0000 0.0000 2.51 KB
Benchmarks.Trace.AspNetCoreBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
master SendRequest net472 0.0000 ns 0.0000 ns 0.0000 ns 0.0000 0.0000 0.0000 0 B
master SendRequest netcoreapp3.1 309,199.7099 ns 4,274.8656 ns 3,998.7119 ns 0.1563 0.0000 0.0000 19777 B
#1937 SendRequest net472 0.0000 ns 0.0000 ns 0.0000 ns 0.0000 0.0000 0.0000 0 B
#1937 SendRequest netcoreapp3.1 325,663.4869 ns 4,604.3034 ns 4,306.8682 ns 0.1611 0.0000 0.0000 19778 B
Benchmarks.Trace.DbCommandBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
master ExecuteNonQuery net472 2.065 μs 0.0390 μs 0.0434 μs 0.1081 0.0010 0.0000 682 B
master ExecuteNonQuery netcoreapp3.1 1.981 μs 0.0396 μs 0.0817 μs 0.0098 0.0000 0.0000 712 B
#1937 ExecuteNonQuery net472 2.253 μs 0.0404 μs 0.0338 μs 0.1081 0.0011 0.0000 682 B
#1937 ExecuteNonQuery netcoreapp3.1 2.048 μs 0.0190 μs 0.0177 μs 0.0103 0.0000 0.0000 712 B
Benchmarks.Trace.ElasticsearchBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
master CallElasticsearch net472 3.203 μs 0.0624 μs 0.0613 μs 0.1279 0.0000 0.0000 827 B
master CallElasticsearch netcoreapp3.1 1.661 μs 0.0270 μs 0.0239 μs 0.0108 0.0000 0.0000 768 B
master CallElasticsearchAsync net472 3.352 μs 0.0664 μs 0.0588 μs 0.1498 0.0000 0.0000 963 B
master CallElasticsearchAsync netcoreapp3.1 1.898 μs 0.0166 μs 0.0147 μs 0.0125 0.0000 0.0000 888 B
#1937 CallElasticsearch net472 3.497 μs 0.0682 μs 0.0700 μs 0.1271 0.0000 0.0000 827 B
#1937 CallElasticsearch netcoreapp3.1 1.730 μs 0.0264 μs 0.0247 μs 0.0103 0.0000 0.0000 768 B
#1937 CallElasticsearchAsync net472 3.641 μs 0.0331 μs 0.0277 μs 0.1500 0.0000 0.0000 963 B
#1937 CallElasticsearchAsync netcoreapp3.1 1.977 μs 0.0175 μs 0.0146 μs 0.0127 0.0000 0.0000 888 B
Benchmarks.Trace.GraphQLBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
master ExecuteAsync net472 4.024 μs 0.0789 μs 0.1229 μs 0.1677 0.0000 0.0000 1083 B
master ExecuteAsync netcoreapp3.1 2.017 μs 0.0340 μs 0.0302 μs 0.0143 0.0000 0.0000 1008 B
#1937 ExecuteAsync net472 3.925 μs 0.0553 μs 0.0518 μs 0.1674 0.0000 0.0000 1083 B
#1937 ExecuteAsync netcoreapp3.1 2.013 μs 0.0247 μs 0.0219 μs 0.0140 0.0000 0.0000 1008 B
Benchmarks.Trace.HttpClientBenchmark - Slower ⚠️ Same allocations ✔️

Slower ⚠️ in #1937

Benchmark diff/base Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.HttpClientBenchmark.SendAsync‑net472 1.154 6,224.26 7,182.05 several?

Raw results

Branch Method Toolchain Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
master SendAsync net472 6.266 μs 0.1127 μs 0.1425 μs 0.3496 0.0000 0.0000 2.19 KB
master SendAsync netcoreapp3.1 4.336 μs 0.0573 μs 0.0508 μs 0.0306 0.0000 0.0000 2.09 KB
#1937 SendAsync net472 7.165 μs 0.1421 μs 0.2965 μs 0.3502 0.0000 0.0000 2.19 KB
#1937 SendAsync netcoreapp3.1 4.784 μs 0.0929 μs 0.1447 μs 0.0306 0.0000 0.0000 2.09 KB
Benchmarks.Trace.ILoggerBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
master EnrichedLog net472 4.066 μs 0.0529 μs 0.0469 μs 0.2857 0.0000 0.0000 1.79 KB
master EnrichedLog netcoreapp3.1 3.807 μs 0.0412 μs 0.0344 μs 0.0285 0.0000 0.0000 1.94 KB
#1937 EnrichedLog net472 4.334 μs 0.0422 μs 0.0353 μs 0.2862 0.0000 0.0000 1.79 KB
#1937 EnrichedLog netcoreapp3.1 3.966 μs 0.0120 μs 0.0093 μs 0.0278 0.0000 0.0000 1.94 KB
Benchmarks.Trace.Log4netBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
master EnrichedLog net472 181.8 μs 2.75 μs 2.44 μs 0.6434 0.1838 0.0000 5.23 KB
master EnrichedLog netcoreapp3.1 153.4 μs 2.23 μs 1.98 μs 0.0000 0.0000 0.0000 5.05 KB
#1937 EnrichedLog net472 188.1 μs 2.12 μs 1.88 μs 0.6540 0.1868 0.0000 5.23 KB
#1937 EnrichedLog netcoreapp3.1 159.3 μs 2.27 μs 2.13 μs 0.0781 0.0000 0.0000 5.05 KB
Benchmarks.Trace.NLogBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
master EnrichedLog net472 12.46 μs 0.244 μs 0.216 μs 0.8539 0.0000 0.0000 5.31 KB
master EnrichedLog netcoreapp3.1 10.54 μs 0.179 μs 0.158 μs 0.0898 0.0000 0.0000 6.28 KB
#1937 EnrichedLog net472 13.03 μs 0.240 μs 0.200 μs 0.8514 0.0000 0.0000 5.31 KB
#1937 EnrichedLog netcoreapp3.1 11.09 μs 0.095 μs 0.074 μs 0.0897 0.0000 0.0000 6.28 KB
Benchmarks.Trace.RedisBenchmark - Slower ⚠️ Same allocations ✔️

Slower ⚠️ in #1937

Benchmark diff/base Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.RedisBenchmark.SendReceive‑netcoreapp3.1 1.149 1,988.83 2,285.45

Raw results

Branch Method Toolchain Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
master SendReceive net472 2.231 μs 0.0344 μs 0.0305 μs 0.1549 0.0000 0.0000 987 B
master SendReceive netcoreapp3.1 1.995 μs 0.0305 μs 0.0285 μs 0.0137 0.0000 0.0000 984 B
#1937 SendReceive net472 2.447 μs 0.0355 μs 0.0315 μs 0.1540 0.0000 0.0000 987 B
#1937 SendReceive netcoreapp3.1 2.293 μs 0.0238 μs 0.0223 μs 0.0132 0.0000 0.0000 984 B
Benchmarks.Trace.SerilogBenchmark - Slower ⚠️ Same allocations ✔️

Slower ⚠️ in #1937

Benchmark diff/base Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.SerilogBenchmark.EnrichedLog‑netcoreapp3.1 1.131 7,143.16 8,081.87

Raw results

Branch Method Toolchain Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
master EnrichedLog net472 7.947 μs 0.1451 μs 0.1286 μs 0.4472 0.0000 0.0000 2.8 KB
master EnrichedLog netcoreapp3.1 7.119 μs 0.1328 μs 0.1177 μs 0.0371 0.0000 0.0000 2.61 KB
#1937 EnrichedLog net472 8.906 μs 0.0875 μs 0.0731 μs 0.4455 0.0000 0.0000 2.8 KB
#1937 EnrichedLog netcoreapp3.1 8.116 μs 0.1552 μs 0.1376 μs 0.0360 0.0000 0.0000 2.61 KB
Benchmarks.Trace.SpanBenchmark - Slower ⚠️ Same allocations ✔️

Slower ⚠️ in #1937

Benchmark diff/base Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.SpanBenchmark.StartFinishScope‑netcoreapp3.1 1.607 1,124.61 1,807.44

Raw results

Branch Method Toolchain Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
master StartFinishSpan net472 972.8 ns 19.21 ns 38.36 ns 0.0681 0.0000 0.0000 433 B
master StartFinishSpan netcoreapp3.1 877.9 ns 7.32 ns 6.85 ns 0.0062 0.0000 0.0000 432 B
master StartFinishScope net472 1,103.0 ns 12.53 ns 16.72 ns 0.0802 0.0000 0.0000 514 B
master StartFinishScope netcoreapp3.1 1,125.1 ns 16.59 ns 13.85 ns 0.0079 0.0000 0.0000 552 B
#1937 StartFinishSpan net472 967.0 ns 10.29 ns 9.63 ns 0.0679 0.0000 0.0000 433 B
#1937 StartFinishSpan netcoreapp3.1 940.3 ns 18.52 ns 31.94 ns 0.0058 0.0000 0.0000 432 B
#1937 StartFinishScope net472 1,221.8 ns 22.74 ns 44.88 ns 0.0804 0.0000 0.0000 514 B
#1937 StartFinishScope netcoreapp3.1 1,690.4 ns 112.45 ns 331.57 ns 0.0075 0.0000 0.0000 552 B

@andrewlock
Copy link
Member

Code Coverage Report 📊

✔️ Merging #1937 into master will not change line coverage
✔️ Merging #1937 into master will not change branch coverage
✔️ Merging #1937 into master will will decrease complexity by 6

master #1937 Change
Lines 10413 / 15729 10406 / 15722
Lines % 66% 66% 0% ✔️
Branches 4541 / 6870 4532 / 6864
Branches % 66% 66% 0% ✔️
Complexity 8381 8375 -6 ✔️

View the full report for further details:

Datadog.Trace Breakdown ✔️

master #1937 Change
Lines % 66% 66% 0% ✔️
Branches % 66% 66% 0% ✔️
Complexity 8381 8375 -6 ✔️

The following classes have significant coverage changes.

File Line coverage change Branch coverage change Complexity change
Datadog.Trace.ClrProfiler.AutoInstrumentation.Testing.NUnit.NUnitCompositeWorkItemSkipChildrenIntegration -8% ⛔ -8% ⛔ 0 ✔️
Datadog.Trace.ClrProfiler.Integrations.Testing.NUnitIntegration -2% ⚠️ -14% ⛔ 0 ✔️
Datadog.Trace.ClrProfiler.Helpers.AsyncHelper 2% ✔️ -6% ⛔ 0 ✔️

View the full reports for further details:

@lucaspimentel lucaspimentel merged commit 86c0bc5 into master Nov 4, 2021
@lucaspimentel lucaspimentel deleted the lpimentel/update-rules-based-sampling-priorities branch November 4, 2021 18:04
@github-actions github-actions bot added this to the vNext milestone Nov 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zacharycmontoya zacharycmontoya zacharycmontoya approved these changes

@andrewlock andrewlock andrewlock approved these changes

Assignees
No one assigned
Labels
area:tracer The core tracer library (Datadog.Trace, does not include OpenTracing, native code, or integrations) type:enhancement Improvement to an existing feature
Projects
None yet
Milestone
1.30.0
Development

Successfully merging this pull request may close these issues.
3 participants
@lucaspimentel @andrewlock @zacharycmontoya