Skip to content

Add a thread-safe Random implementation (for Data streams monitoring)#3130

Merged
andrewlock merged 6 commits into
masterfrom
andrew/thread-safe-random
Aug 31, 2022
Merged

Add a thread-safe Random implementation (for Data streams monitoring)#3130
andrewlock merged 6 commits into
masterfrom
andrew/thread-safe-random

Conversation

@andrewlock

@andrewlock andrewlock commented Aug 26, 2022

Copy link
Copy Markdown
Member

Summary of changes

Adds a thread-safe random implementation.

Reason for change

Required by data streams monitoring. The Debugger already had its own implementation, so centralised, hardened, and defaulted to the built-in version in .NET 6.

Implementation details

ThreadLocal Random, and a "global" random for seeding the thread-local ones.

Test coverage

Yeah... should probably be some...

Other details

Required for data streams monitoring

@andrewlock
andrewlock requested review from a team as code owners August 26, 2022 12:31
@github-actions github-actions Bot added the area:tracer The core tracer library (Datadog.Trace, does not include OpenTracing, native code, or integrations) label Aug 26, 2022
@andrewlock
andrewlock force-pushed the andrew/thread-safe-random branch from d3847e2 to 2a31940 Compare August 26, 2022 13:05
@andrewlock

This comment has been minimized.

Sign in to view
@andrewlock

This comment has been minimized.

Sign in to view
lucaspimentel
lucaspimentel requested changes Aug 26, 2022
View reviewed changes

@lucaspimentel lucaspimentel left a comment •
edited
Loading

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.

We have another implementation in SpanIdGenerator, and it's different than this one:

  • [ThreadStatic] vs ThreadLocal<T> (which one should we use?)
  • on .NET Framework, we use one static Random to seed each new Random instance to work around an issue with low clock resolution

We should probably consolidate the code with that one as well.

EDIT: never mind, I missed the global random used for seeds in the new implementation 🤦🏽‍♂️

kevingosse
kevingosse approved these changes Aug 26, 2022
View reviewed changes

@kevingosse kevingosse left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM but you should use it in the SpanIdGenerator as well (it's pretty much the same algorithm as far as I can tell)

lucaspimentel
lucaspimentel approved these changes Aug 26, 2022
View reviewed changes

@lucaspimentel lucaspimentel left a comment •
edited
Loading

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.

What Kevin said. And I think we have tests for SpanIdGenerator, so if you use the new one there maybe that's enough tests.

@kevingosse

kevingosse commented Aug 26, 2022

Copy link
Copy Markdown
Contributor

[ThreadStatic] vs ThreadLocal (which one should we use?)

As much as possible I'd recommend using [ThreadStatic] instead of ThreadLocal<T>. ThreadLocal has a finalizer that puts additional pressure on the GC.

OmerRaviv
OmerRaviv approved these changes Aug 26, 2022
View reviewed changes

@OmerRaviv OmerRaviv left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM. Thanks!

@andrewlock
andrewlock force-pushed the andrew/thread-safe-random branch from 2a31940 to 6ccc6c3 Compare August 30, 2022 08:39
@andrewlock

This comment has been minimized.

Sign in to view
@andrewlock
andrewlock force-pushed the andrew/thread-safe-random branch from 6ccc6c3 to e289786 Compare August 30, 2022 09:14
@andrewlock

This comment has been minimized.

Sign in to view
@andrewlock
Add a ThreadSafe random implementation
ad30c7f
@andrewlock
Add NextDouble() to ThreadSafeRandom
00b1f9f
@andrewlock
Update AdaptiveSampler to use ThreadSafeRandom
fb043b0
@andrewlock
Add Next(int minValue, int maxValue) to ThreadSafeRandom
9218fe7
@andrewlock
Convert ThreadLocal<T> -> [ThreadStatic]
e5d1269
@andrewlock
Replace the SpanIdGenerator with ThreadSafeRandom
8a14996 
SpanIdGenerator was a thin wrapper around essentially the same implementation, so converted it to a simple static method that uses ThreadSafeRandom implicitly

The downside to this is we can't create a `SpanIdGenerator` with a specific seed any more. But as far as I can tell we were never doing that anyway, even in tests, so I think this is simpler
@andrewlock
andrewlock force-pushed the andrew/thread-safe-random branch from e289786 to 8a14996 Compare August 30, 2022 18:43
@andrewlock

andrewlock commented Aug 30, 2022

Copy link
Copy Markdown
Member Author

Benchmarks Report 🐌

Benchmarks for #3130 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 716μs 388ns 1.45μs 0.355 0 0 3.18 KB
master WriteAndFlushEnrichedTraces netcoreapp3.1 454μs 272ns 1.02μs 0 0 0 2.58 KB
#3130 WriteAndFlushEnrichedTraces net472 720μs 386ns 1.5μs 0.361 0 0 3.18 KB
#3130 WriteAndFlushEnrichedTraces netcoreapp3.1 461μs 321ns 1.24μs 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 188ns 0.508ns 1.97ns 0.0675 0 0 425 B
master AllCycleSimpleBody netcoreapp3.1 241ns 0.664ns 2.39ns 0.00571 0 0 424 B
master AllCycleMoreComplexBody net472 199ns 0.772ns 2.99ns 0.0637 0 0 401 B
master AllCycleMoreComplexBody netcoreapp3.1 244ns 0.552ns 2.14ns 0.0055 0 0 400 B
master BodyExtractorSimpleBody net472 264ns 0.885ns 3.43ns 0.0573 0 0 361 B
master BodyExtractorSimpleBody netcoreapp3.1 238ns 0.654ns 2.53ns 0.00362 0 0 272 B
master BodyExtractorMoreComplexBody net472 15μs 53ns 205ns 1.21 0.0219 0 7.62 KB
master BodyExtractorMoreComplexBody netcoreapp3.1 13μs 40.7ns 157ns 0.0903 0 0 6.75 KB
#3130 AllCycleSimpleBody net472 188ns 0.702ns 2.72ns 0.0675 0 0 425 B
#3130 AllCycleSimpleBody netcoreapp3.1 240ns 0.691ns 2.68ns 0.00579 0 0 424 B
#3130 AllCycleMoreComplexBody net472 189ns 0.729ns 2.82ns 0.0637 0 0 401 B
#3130 AllCycleMoreComplexBody netcoreapp3.1 244ns 0.625ns 2.42ns 0.00553 0 0 400 B
#3130 BodyExtractorSimpleBody net472 266ns 1.19ns 4.62ns 0.0573 0 0 361 B
#3130 BodyExtractorSimpleBody netcoreapp3.1 249ns 0.842ns 3.26ns 0.00379 0 0 272 B
#3130 BodyExtractorMoreComplexBody net472 15.3μs 51.2ns 198ns 1.21 0.0151 0 7.62 KB
#3130 BodyExtractorMoreComplexBody netcoreapp3.1 12.8μs 35.9ns 139ns 0.0906 0 0 6.75 KB
Benchmarks.Trace.AspNetCoreBenchmark - Same speed ✔️ 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 180μs 96.9ns 375ns 0.179 0 0 20.57 KB
#3130 SendRequest net472 0ns 0ns 0ns 0 0 0 0 b
#3130 SendRequest netcoreapp3.1 180μs 167ns 646ns 0.269 0 0 20.57 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.75μs 0.738ns 2.86ns 0.15 0.000872 0 947 B
master ExecuteNonQuery netcoreapp3.1 1.41μs 0.459ns 1.72ns 0.0128 0 0 936 B
#3130 ExecuteNonQuery net472 1.81μs 0.507ns 1.9ns 0.15 0.00091 0 947 B
#3130 ExecuteNonQuery netcoreapp3.1 1.45μs 1.77ns 6.63ns 0.0129 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.61μs 1.17ns 4.54ns 0.184 0 0 1.16 KB
master CallElasticsearch netcoreapp3.1 1.52μs 0.645ns 2.33ns 0.0144 0 0 1.1 KB
master CallElasticsearchAsync net472 2.58μs 0.52ns 2.02ns 0.204 0 0 1.29 KB
master CallElasticsearchAsync netcoreapp3.1 1.55μs 0.613ns 2.37ns 0.0163 0 0 1.22 KB
#3130 CallElasticsearch net472 2.5μs 1.27ns 4.94ns 0.183 0 0 1.16 KB
#3130 CallElasticsearch netcoreapp3.1 1.6μs 1.07ns 3.85ns 0.0144 0 0 1.1 KB
#3130 CallElasticsearchAsync net472 2.66μs 0.973ns 3.77ns 0.204 0 0 1.29 KB
#3130 CallElasticsearchAsync netcoreapp3.1 1.66μs 0.707ns 2.65ns 0.0159 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.66μs 6.32ns 24.5ns 0.224 0 0 1.41 KB
master ExecuteAsync netcoreapp3.1 1.72μs 3.26ns 12.2ns 0.0181 0 0 1.34 KB
#3130 ExecuteAsync net472 2.6μs 4.72ns 18.3ns 0.224 0 0 1.41 KB
#3130 ExecuteAsync netcoreapp3.1 1.7μs 4.2ns 15.7ns 0.0177 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.89μs 7.88ns 30.5ns 0.438 0 0 2.77 KB
master SendAsync netcoreapp3.1 3.56μs 7.87ns 30.5ns 0.0342 0 0 2.6 KB
#3130 SendAsync net472 5.67μs 8.87ns 34.4ns 0.438 0 0 2.77 KB
#3130 SendAsync netcoreapp3.1 3.6μs 6.42ns 24.8ns 0.0343 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.11μs 2.44ns 9.46ns 0.288 0 0 1.81 KB
master EnrichedLog netcoreapp3.1 2.41μs 1.21ns 4.53ns 0.0255 0 0 1.85 KB
#3130 EnrichedLog net472 3.19μs 2.26ns 8.76ns 0.288 0 0 1.81 KB
#3130 EnrichedLog netcoreapp3.1 2.66μs 1.02ns 3.81ns 0.0249 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 228ns 885ns 0.683 0.228 0 4.65 KB
master EnrichedLog netcoreapp3.1 118μs 174ns 627ns 0.058 0 0 4.49 KB
#3130 EnrichedLog net472 151μs 171ns 638ns 0.687 0.229 0 4.65 KB
#3130 EnrichedLog netcoreapp3.1 115μs 138ns 496ns 0.0574 0 0 4.49 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.64μs 6.11ns 22ns 0.569 0.00283 0 3.59 KB
master EnrichedLog netcoreapp3.1 4.32μs 9.36ns 35ns 0.0521 0 0 3.91 KB
#3130 EnrichedLog net472 5.55μs 6.33ns 24.5ns 0.569 0.00279 0 3.59 KB
#3130 EnrichedLog netcoreapp3.1 4.27μs 8.58ns 33.2ns 0.0528 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.31μs 1.49ns 5.76ns 0.218 0 0 1.37 KB
master SendReceive netcoreapp3.1 1.89μs 0.897ns 3.47ns 0.0181 0 0 1.32 KB
#3130 SendReceive net472 2.32μs 2.37ns 9.17ns 0.218 0 0 1.37 KB
#3130 SendReceive netcoreapp3.1 1.83μs 0.93ns 3.48ns 0.0173 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 5.07μs 1.89ns 7.31ns 0.353 0 0 2.23 KB
master EnrichedLog netcoreapp3.1 4.21μs 1.72ns 6.45ns 0.0231 0 0 1.8 KB
#3130 EnrichedLog net472 5.07μs 1.21ns 4.68ns 0.354 0 0 2.23 KB
#3130 EnrichedLog netcoreapp3.1 4.37μs 5.98ns 23.2ns 0.0238 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.1μs 0.462ns 1.79ns 0.128 0 0 810 B
master StartFinishSpan netcoreapp3.1 1.03μs 0.339ns 1.27ns 0.0102 0 0 760 B
master StartFinishScope net472 1.35μs 0.495ns 1.92ns 0.141 0 0 891 B
master StartFinishScope netcoreapp3.1 1.09μs 0.382ns 1.43ns 0.0119 0 0 880 B
#3130 StartFinishSpan net472 1.12μs 0.454ns 1.76ns 0.128 0 0 810 B
#3130 StartFinishSpan netcoreapp3.1 933ns 0.419ns 1.57ns 0.0103 0 0 760 B
#3130 StartFinishScope net472 1.44μs 0.28ns 1.05ns 0.141 0 0 891 B
#3130 StartFinishScope netcoreapp3.1 1.12μs 0.322ns 1.25ns 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.5μs 0.599ns 2.32ns 0.141 0 0 891 B
master RunOnMethodBegin netcoreapp3.1 1.23μs 0.55ns 1.98ns 0.0119 0 0 880 B
#3130 RunOnMethodBegin net472 1.49μs 0.354ns 1.37ns 0.141 0 0 891 B
#3130 RunOnMethodBegin netcoreapp3.1 1.17μs 0.393ns 1.42ns 0.0118 0 0 880 B

@andrewlock

andrewlock commented Aug 31, 2022

Copy link
Copy Markdown
Member Author

Code Coverage Report 📊

✔️ Merging #3130 into master will not change line coverage
✔️ Merging #3130 into master will not change branch coverage
✔️ Merging #3130 into master will will decrease complexity by 5

master #3130 Change
Lines 17297 / 23587 17237 / 23570
Lines % 73% 73% 0% ✔️
Branches 10265 / 14548 10233 / 14540
Branches % 71% 70% 0% ✔️
Complexity 15666 15661 -5 ✔️

View the full report for further details:

Datadog.Trace Breakdown ✔️

master #3130 Change
Lines % 73% 73% 0% ✔️
Branches % 71% 70% 0% ✔️
Complexity 15666 15661 -5 ✔️

The following classes have significant coverage changes.

File Line coverage change Branch coverage change Complexity change
Datadog.Trace.Processors.ObfuscatorTagsProcessor -25% ⛔ 0% ✔️ 0 ✔️
Datadog.Trace.Processors.NormalizerTraceProcessor -22% ⛔ -19% ⛔ 0 ✔️
Datadog.Trace.Ci.CIVisibility 5% ✔️ 6% ✔️ 0 ✔️

The following classes were added in #3130:

File Line coverage Branch coverage Complexity
Datadog.Trace.TraceProcessors.ObfuscatorTraceProcessor 76% 78% 63
Datadog.Trace.Util.ThreadSafeRandom 85% 100% 9

1 classes were removed from Datadog.Trace in #3130

View the full reports for further details:

@andrewlock
andrewlock merged commit bae586c into master Aug 31, 2022
@andrewlock
andrewlock deleted the andrew/thread-safe-random branch August 31, 2022 08:47
@github-actions github-actions Bot added this to the vNext milestone Aug 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@OmerRaviv OmerRaviv OmerRaviv approved these changes
@lucaspimentel lucaspimentel lucaspimentel approved these changes
+1 more reviewer
@kevingosse kevingosse kevingosse approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area:data-streams-monitoring 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.

4 participants

@andrewlock @kevingosse @OmerRaviv @lucaspimentel