Skip to content

Rename RuleBasedSampler to TraceSampler#3156

Merged
bouwkast merged 3 commits into
masterfrom
steven/rename-rulebasedsampler
Sep 22, 2022
Merged

Rename RuleBasedSampler to TraceSampler#3156
bouwkast merged 3 commits into
masterfrom
steven/rename-rulebasedsampler

Conversation

@bouwkast

@bouwkast bouwkast commented Sep 2, 2022 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Summary of changes

This renames the following:

  • Within Datadog.Agent.TraceSamplers
    • ITraceSampler-> ITraceChunkSampler (over alternative ITraceAgentSampler)
    • RareSampler, ErrorSampler, etc all stay the same
  • Within Datadog.Trace.Sampling
    • ISampler -> ITraceSampler
    • RuleBasedSampler -> TraceSampler
    • CustomSamplingRule, DefaultSamplingRule, GlobalSamplingRule stay the same

Reason for change

Motivation is that this is not just a rule-based sampler and ultimately just operates on traces.
For single span ingestion we'll have a SpanSampler so this would help clarify the differences between the two.

Test coverage

The now renamed TraceSamplerTests all still pass.

@bouwkast bouwkast self-assigned this Sep 2, 2022
@bouwkast
bouwkast requested a review from a team as a code owner September 2, 2022 13:57
andrewlock
andrewlock reviewed Sep 2, 2022
View reviewed changes

@andrewlock andrewlock left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about this:

  • In Zach's stats PR he introduces ITraceSampler - this could get confusing I think
  • Arguably, this makes sampling decisions for Spans rather than traces?
  • The CiSampler is also a trace sampler

If we're going to rename it, I would probably just call it DefaultSampler?

@pierotibou

pierotibou commented Sep 2, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

he introduces ITraceSampler - this could get confusing I think

True. Though, what we call sampler everywhere is more the ones we have here. I believe we shall rename the ones in Zach's PR actually as most of them just decide to drop/keep based on a previous sampling decision.

Arguably, this makes sampling decisions for Spans rather than traces?

We check spans but the result is to keep the whole trace.

The CiSampler is also a trace sampler

Indeed.

Note for self, I should have discussed this plan in a team meeting to raise remarks/questions early on.

@andrewlock

This comment has been minimized.

Sign in to view
@bouwkast

bouwkast commented Sep 2, 2022

Copy link
Copy Markdown
Collaborator Author

I'm not sure about this:

  • In Zach's stats PR he introduces ITraceSampler - this could get confusing I think
  • Arguably, this makes sampling decisions for Spans rather than traces?
  • The CiSampler is also a trace sampler

If we're going to rename it, I would probably just call it DefaultSampler?

Good catch on that ITraceSampler that was added

I'll leave this here for now, but TraceSampler sounds like maybe not the best name at the moment, so maybe either go with the DefaultSampler or close this and come up with a better name in the future potentially

@andrewlock

This comment has been minimized.

Sign in to view
@bouwkast
bouwkast force-pushed the steven/rename-rulebasedsampler branch from 89b03cf to f27b5cb Compare September 20, 2022 15:26
@bouwkast
bouwkast requested a review from a team as a code owner September 20, 2022 15:26
@andrewlock

This comment has been minimized.

Sign in to view
@bouwkast
bouwkast force-pushed the steven/rename-rulebasedsampler branch from f27b5cb to cd7b0db Compare September 21, 2022 15:15
@andrewlock

andrewlock commented Sep 21, 2022

Copy link
Copy Markdown
Member

Benchmarks Report 🐌

Benchmarks for #3156 compared to master:

  • All benchmarks have the same speed
  • 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 StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master WriteAndFlushEnrichedTraces net472 710μs 506ns 1.96μs 0.357 0 0 3.18 KB
master WriteAndFlushEnrichedTraces netcoreapp3.1 459μs 275ns 1.06μs 0 0 0 2.58 KB
#3156 WriteAndFlushEnrichedTraces net472 721μs 567ns 2.19μs 0.359 0 0 3.18 KB
#3156 WriteAndFlushEnrichedTraces netcoreapp3.1 463μs 96.3ns 347ns 0 0 0 2.58 KB
Benchmarks.Trace.AppSecBodyBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master AllCycleSimpleBody net472 182ns 0.155ns 0.599ns 0.0675 9.18E-05 0 425 B
master AllCycleSimpleBody netcoreapp3.1 239ns 0.154ns 0.575ns 0.00577 0 0 424 B
master AllCycleMoreComplexBody net472 182ns 0.179ns 0.67ns 0.0637 0 0 401 B
master AllCycleMoreComplexBody netcoreapp3.1 245ns 0.304ns 1.18ns 0.00557 0 0 400 B
master BodyExtractorSimpleBody net472 255ns 0.209ns 0.783ns 0.0574 0 0 361 B
master BodyExtractorSimpleBody netcoreapp3.1 216ns 0.251ns 0.938ns 0.00367 0 0 272 B
master BodyExtractorMoreComplexBody net472 14.8μs 9.99ns 36ns 1.21 0.0148 0 7.62 KB
master BodyExtractorMoreComplexBody netcoreapp3.1 12μs 18.8ns 72.7ns 0.09 0 0 6.75 KB
#3156 AllCycleSimpleBody net472 200ns 0.218ns 0.843ns 0.0675 0 0 425 B
#3156 AllCycleSimpleBody netcoreapp3.1 250ns 0.226ns 0.875ns 0.00582 0 0 424 B
#3156 AllCycleMoreComplexBody net472 195ns 0.0891ns 0.334ns 0.0637 0 0 401 B
#3156 AllCycleMoreComplexBody netcoreapp3.1 258ns 0.194ns 0.75ns 0.00542 0 0 400 B
#3156 BodyExtractorSimpleBody net472 278ns 0.0855ns 0.331ns 0.0573 0 0 361 B
#3156 BodyExtractorSimpleBody netcoreapp3.1 232ns 0.141ns 0.509ns 0.00368 0 0 272 B
#3156 BodyExtractorMoreComplexBody net472 16μs 12.8ns 49.6ns 1.21 0.016 0 7.62 KB
#3156 BodyExtractorMoreComplexBody netcoreapp3.1 12.4μs 19.9ns 74.6ns 0.0869 0 0 6.75 KB
Benchmarks.Trace.AspNetCoreBenchmark - Unknown 🤷 Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master SendRequest net472 0ns 0ns 0ns 0 0 0 0 b
master SendRequest netcoreapp3.1 178μs 163ns 588ns 0.268 0 0 20.58 KB
#3156 SendRequest net472 0ns 0ns 0ns 0 0 0 0 b
#3156 SendRequest netcoreapp3.1 182μs 239ns 925ns 0.182 0 0 20.58 KB
Benchmarks.Trace.DbCommandBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master ExecuteNonQuery net472 1.84μs 1.08ns 4.17ns 0.15 0 0 947 B
master ExecuteNonQuery netcoreapp3.1 1.55μs 1.26ns 4.87ns 0.0129 0 0 936 B
#3156 ExecuteNonQuery net472 1.87μs 0.729ns 2.73ns 0.15 0.000937 0 947 B
#3156 ExecuteNonQuery netcoreapp3.1 1.47μs 0.516ns 1.86ns 0.0122 0 0 936 B
Benchmarks.Trace.ElasticsearchBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master CallElasticsearch net472 2.5μs 1.45ns 5.43ns 0.183 0 0 1.16 KB
master CallElasticsearch netcoreapp3.1 1.56μs 0.979ns 3.66ns 0.0148 0 0 1.1 KB
master CallElasticsearchAsync net472 2.59μs 1.71ns 6.61ns 0.205 0 0 1.29 KB
master CallElasticsearchAsync netcoreapp3.1 1.69μs 0.671ns 2.42ns 0.0162 0 0 1.22 KB
#3156 CallElasticsearch net472 2.59μs 1.26ns 4.7ns 0.182 0 0 1.16 KB
#3156 CallElasticsearch netcoreapp3.1 1.58μs 0.926ns 3.59ns 0.0143 0 0 1.1 KB
#3156 CallElasticsearchAsync net472 2.8μs 2.28ns 8.84ns 0.204 0 0 1.29 KB
#3156 CallElasticsearchAsync netcoreapp3.1 1.61μs 0.738ns 2.86ns 0.0168 0 0 1.22 KB
Benchmarks.Trace.GraphQLBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master ExecuteAsync net472 2.82μs 11ns 42.7ns 0.225 0 0 1.42 KB
master ExecuteAsync netcoreapp3.1 1.71μs 1.27ns 4.91ns 0.0179 0 0 1.34 KB
#3156 ExecuteAsync net472 2.93μs 1.73ns 6.49ns 0.226 0 0 1.42 KB
#3156 ExecuteAsync netcoreapp3.1 1.75μs 0.692ns 2.59ns 0.0183 0 0 1.34 KB
Benchmarks.Trace.HttpClientBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master SendAsync net472 5.85μs 3.31ns 12.8ns 0.438 0 0 2.77 KB
master SendAsync netcoreapp3.1 3.66μs 3.8ns 14.2ns 0.0349 0 0 2.6 KB
#3156 SendAsync net472 5.84μs 5.86ns 22.7ns 0.44 0 0 2.77 KB
#3156 SendAsync netcoreapp3.1 3.64μs 2.5ns 9.69ns 0.0346 0 0 2.6 KB
Benchmarks.Trace.ILoggerBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master EnrichedLog net472 3.24μs 3.36ns 13ns 0.288 0 0 1.81 KB
master EnrichedLog netcoreapp3.1 2.52μs 1.48ns 5.74ns 0.0252 0 0 1.85 KB
#3156 EnrichedLog net472 3.31μs 1.95ns 7.56ns 0.287 0 0 1.81 KB
#3156 EnrichedLog netcoreapp3.1 2.52μs 2.1ns 7.87ns 0.0251 0 0 1.85 KB
Benchmarks.Trace.Log4netBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master EnrichedLog net472 152μs 155ns 600ns 0.685 0.228 0 4.65 KB
master EnrichedLog netcoreapp3.1 118μs 213ns 823ns 0.0579 0 0 4.49 KB
#3156 EnrichedLog net472 155μs 126ns 488ns 0.694 0.231 0 4.66 KB
#3156 EnrichedLog netcoreapp3.1 121μs 275ns 1.07μs 0 0 0 4.5 KB
Benchmarks.Trace.NLogBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master EnrichedLog net472 5.72μs 14ns 54.3ns 0.57 0.00285 0 3.59 KB
master EnrichedLog netcoreapp3.1 4.39μs 3.4ns 12.3ns 0.0532 0.00213 0 3.91 KB
#3156 EnrichedLog net472 5.97μs 5.8ns 22.5ns 0.568 0.00298 0 3.59 KB
#3156 EnrichedLog netcoreapp3.1 4.48μs 1.98ns 7.68ns 0.0537 0 0 3.91 KB
Benchmarks.Trace.RedisBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master SendReceive net472 2.38μs 3.65ns 14.1ns 0.218 0 0 1.37 KB
master SendReceive netcoreapp3.1 1.76μs 0.62ns 2.4ns 0.0185 0 0 1.32 KB
#3156 SendReceive net472 2.28μs 3.32ns 12.8ns 0.218 0 0 1.37 KB
#3156 SendReceive netcoreapp3.1 1.78μs 1.6ns 6ns 0.0178 0 0 1.32 KB
Benchmarks.Trace.SerilogBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master EnrichedLog net472 4.95μs 1.44ns 4.97ns 0.353 0 0 2.23 KB
master EnrichedLog netcoreapp3.1 4.3μs 3.04ns 11.4ns 0.0236 0 0 1.8 KB
#3156 EnrichedLog net472 5.11μs 3.22ns 12.5ns 0.353 0 0 2.23 KB
#3156 EnrichedLog netcoreapp3.1 4.23μs 4.83ns 18.7ns 0.0231 0 0 1.8 KB
Benchmarks.Trace.SpanBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master StartFinishSpan net472 1.15μs 0.711ns 2.75ns 0.129 0 0 810 B
master StartFinishSpan netcoreapp3.1 985ns 0.614ns 2.38ns 0.0103 0 0 760 B
master StartFinishScope net472 1.45μs 0.422ns 1.52ns 0.141 0 0 891 B
master StartFinishScope netcoreapp3.1 1.05μs 0.371ns 1.44ns 0.0116 0 0 880 B
#3156 StartFinishSpan net472 1.18μs 0.429ns 1.66ns 0.128 0 0 810 B
#3156 StartFinishSpan netcoreapp3.1 924ns 0.287ns 1.07ns 0.0102 0 0 760 B
#3156 StartFinishScope net472 1.43μs 0.73ns 2.83ns 0.141 0 0 891 B
#3156 StartFinishScope netcoreapp3.1 1.11μs 0.358ns 1.34ns 0.0117 0 0 880 B
Benchmarks.Trace.TraceAnnotationsBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master RunOnMethodBegin net472 1.58μs 1.19ns 4.62ns 0.142 0 0 891 B
master RunOnMethodBegin netcoreapp3.1 1.18μs 0.571ns 2.21ns 0.0118 0 0 880 B
#3156 RunOnMethodBegin net472 1.47μs 0.717ns 2.78ns 0.141 0 0 891 B
#3156 RunOnMethodBegin netcoreapp3.1 1.16μs 0.213ns 0.737ns 0.0117 0 0 880 B

@andrewlock

andrewlock commented Sep 21, 2022

Copy link
Copy Markdown
Member

Code Coverage Report 📊

⛔ Merging #3156 into master will will decrease line coverage by 5%
⚠️ Merging #3156 into master will will decrease branch coverage by 3%
⛔ Merging #3156 into master will will increase complexity by 20

master #3156 Change
Lines 17963 / 24889 16760 / 24930
Lines % 72% 67% -5% ⛔
Branches 10472 / 15254 9999 / 15274
Branches % 69% 65% -3% ⚠️
Complexity 16477 16497 20 ⛔

View the full report for further details:

Datadog.Trace Breakdown ⛔

master #3156 Change
Lines % 72% 67% -5% ⛔
Branches % 69% 65% -3% ⚠️
Complexity 16477 16497 20 ⛔

The following classes have significant coverage changes.

File Line coverage change Branch coverage change Complexity change
Datadog.Trace.Debugger.PInvoke.NativeMethodProbeDefinition -100% ⛔ -100% ⛔ 0 ✔️
Datadog.Trace.Debugger.Sink.SnapshotSink -100% ⛔ -100% ⛔ 0 ✔️
Datadog.Trace.Debugger.Snapshots.LazySnapshotSerializerFieldsAndPropsSelector -100% ⛔ -100% ⛔ 0 ✔️
Datadog.Trace.Debugger.Snapshots.DebuggerSnapshotCreator -100% ⛔ -68% ⛔ 1 ⛔
BoundLineProbeLocation -100% ⛔ 0% ✔️ 0 ✔️
Datadog.Trace.Debugger.CapturedLines -100% ⛔ 0% ✔️ 0 ✔️
Datadog.Trace.Debugger.Instrumentation.DebuggerReturn -100% ⛔ 0% ✔️ 0 ✔️
Datadog.Trace.Debugger.Instrumentation.MethodMetadataInfo -100% ⛔ 0% ✔️ 0 ✔️
Datadog.Trace.Debugger.Models.LineProbeResolveResult -100% ⛔ 0% ✔️ 0 ✔️
Datadog.Trace.Debugger.PInvoke.NativeLineProbeDefinition -100% ⛔ 0% ✔️ 0 ✔️
...And 50 more

The following classes were added in #3156:

File Line coverage Branch coverage Complexity
Datadog.Trace.Sampling.TraceSampler 95% 80% 22

1 classes were removed from Datadog.Trace in #3156

View the full reports for further details:

@bouwkast
bouwkast merged commit df2e4b5 into master Sep 22, 2022
@bouwkast
bouwkast deleted the steven/rename-rulebasedsampler branch September 22, 2022 12:33
@github-actions github-actions Bot added this to the vNext milestone Sep 22, 2022
@lucaspimentel lucaspimentel changed the title Rename RuleBasedSampler to TraceSampler Rename RuleBasedSampler to TraceSampler Oct 4, 2022
@lucaspimentel lucaspimentel added the area:tracer The core tracer library (Datadog.Trace, does not include OpenTracing, native code, or integrations) label Oct 4, 2022
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
@pierotibou pierotibou pierotibou approved these changes

Assignees

@bouwkast bouwkast

Labels

area:tracer The core tracer library (Datadog.Trace, does not include OpenTracing, native code, or integrations)

Projects

None yet

Milestone

2.15.0

Development

Successfully merging this pull request may close these issues.

5 participants

@bouwkast @pierotibou @andrewlock @zacharycmontoya @lucaspimentel