Add a simple pooling mechanism for DDSketches by andrewlock · Pull Request #3177 · DataDog/dd-trace-dotnet

andrewlock · 2022-09-07T06:34:16Z

Summary of changes

Adds a simple pooling mechanism for DDSketch instances for DSM

Reason for change

We could end up creating a lot of them in data streams monitoring, so keep a (bounded) pool

Implementation details

Just a bounded concurrent queue. Bounded so that we don't create huge numbers, but plucking a number out of the air here. Considered basing it on ObjectPool like this or this, but didn't seem worth the extra complexity, especially as the expected access patterns aren't as well defined as for those cases.

Test coverage

Simple unit tests

Other details

Prerequisite for Data Streams Monitoring work. The sketch parameters are based on what we need for DSM but, dd-go and dd-java use different sketch parameters here. Need to verify that the values here are ok

zacharycmontoya · 2022-09-07T23:28:36Z

+    }
+
+    [Fact]
+    public void WhenPoolFull_GetsNewInstance()


I'm not sure the test is fully testing what it declares. I think to fully test it, you'd need to do something like the following:

for 0..maxPoolSize + 1, create a new DDSketch object, save the object reference into your pools list, and call pool.Release(sketch). As a result, the DDSketchPool will have a pool of all but the maxPoolSize + 1 sketch

for 0..maxPoolSize + 1, call pool.Get()

Assert that the first maxPoolSize values are the same and the last sketch values are not

I don't want to add clutter, so I'll clarify here: I think when you create a new DDSketch object, you could continue to use pool.Get() with the later assertion of pools.Should().OnlyHaveUniqueItems(); to demonstrate the empty scenario, while releasing everything to the pool will demonstrate the full scenario

You're right, my original test wasn't right at all. I have rewritten to use your suggestion (though I didn't put any assertions around the order of the items from the pool, so we can switch to a stack/array etc if needs be without breaking these tests) in bf7a469

zacharycmontoya

Besides the open question of sketch parameters, LGTM. I also offered some minor test suggestions I hope you consider

We could end up creating a _lot_ of them, so keep a (bounded) pool

…PoolTests.cs Co-authored-by: Zach Montoya <[email protected]>

…PoolTests.cs Co-authored-by: Pierre Bonet <[email protected]>

andrewlock · 2022-09-08T09:46:23Z

Benchmarks Report 🐌

Benchmarks for #3177 compared to master:

1 benchmarks are slower, with geometric mean 1.147
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	Allocated
master	`WriteAndFlushEnrichedTraces`	net472	728μs	434ns	1.62μs	0.361	3.18 KB
master	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	468μs	249ns	934ns	0	2.58 KB
#3177	`WriteAndFlushEnrichedTraces`	net472	722μs	297ns	1.11μs	0.361	3.18 KB
#3177	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	459μs	197ns	737ns	0	2.58 KB

Benchmarks.Trace.AppSecBodyBenchmark - Slower ⚠️ Same allocations ✔️

Slower ⚠️ in #3177

Benchmark	diff/base	Base Median (ns)	Diff Median (ns)	Modality
Benchmarks.Trace.AppSecBodyBenchmark.AllCycleSimpleBody‑net472	1.147	185.19	212.39

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`AllCycleSimpleBody`	net472	185ns	0.206ns	0.797ns	0.0675	0	425 B
master	`AllCycleSimpleBody`	netcoreapp3.1	236ns	0.223ns	0.864ns	0.00585	0	424 B
master	`AllCycleMoreComplexBody`	net472	205ns	0.447ns	1.73ns	0.0637	0	401 B
master	`AllCycleMoreComplexBody`	netcoreapp3.1	234ns	0.356ns	1.38ns	0.00542	0	400 B
master	`BodyExtractorSimpleBody`	net472	271ns	0.24ns	0.929ns	0.0573	0	361 B
master	`BodyExtractorSimpleBody`	netcoreapp3.1	230ns	0.264ns	1.02ns	0.00374	0	272 B
master	`BodyExtractorMoreComplexBody`	net472	14.7μs	16.8ns	65.3ns	1.21	0.0147	7.62 KB
master	`BodyExtractorMoreComplexBody`	netcoreapp3.1	11.9μs	10.3ns	37.3ns	0.0901	0	6.75 KB
#3177	`AllCycleSimpleBody`	net472	212ns	0.205ns	0.794ns	0.0675	0	425 B
#3177	`AllCycleSimpleBody`	netcoreapp3.1	235ns	0.26ns	0.972ns	0.00576	0	424 B
#3177	`AllCycleMoreComplexBody`	net472	186ns	0.103ns	0.37ns	0.0638	0	401 B
#3177	`AllCycleMoreComplexBody`	netcoreapp3.1	235ns	0.289ns	1.12ns	0.00543	0	400 B
#3177	`BodyExtractorSimpleBody`	net472	269ns	0.325ns	1.26ns	0.0573	0	361 B
#3177	`BodyExtractorSimpleBody`	netcoreapp3.1	229ns	0.18ns	0.651ns	0.00369	0	272 B
#3177	`BodyExtractorMoreComplexBody`	net472	15.5μs	7.12ns	26.6ns	1.2	0.0154	7.62 KB
#3177	`BodyExtractorMoreComplexBody`	netcoreapp3.1	12.2μs	14.8ns	57.5ns	0.0912	0	6.75 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendRequest`	net472	0ns	0ns	0ns	0	0 b
master	`SendRequest`	netcoreapp3.1	182μs	154ns	595ns	0.181	20.57 KB
#3177	`SendRequest`	net472	0ns	0ns	0ns	0	0 b
#3177	`SendRequest`	netcoreapp3.1	182μs	231ns	896ns	0.182	20.57 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`ExecuteNonQuery`	net472	1.82μs	0.635ns	2.38ns	0.15	0.000911	947 B
master	`ExecuteNonQuery`	netcoreapp3.1	1.42μs	1.36ns	5.27ns	0.0127	0	936 B
#3177	`ExecuteNonQuery`	net472	1.82μs	0.817ns	3.06ns	0.15	0.00091	947 B
#3177	`ExecuteNonQuery`	netcoreapp3.1	1.44μs	0.339ns	1.27ns	0.0122	0	936 B

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`CallElasticsearch`	net472	2.61μs	0.715ns	2.58ns	0.183	1.16 KB
master	`CallElasticsearch`	netcoreapp3.1	1.55μs	0.641ns	2.48ns	0.0148	1.1 KB
master	`CallElasticsearchAsync`	net472	2.71μs	0.888ns	3.32ns	0.204	1.29 KB
master	`CallElasticsearchAsync`	netcoreapp3.1	1.63μs	0.997ns	3.86ns	0.0163	1.22 KB
#3177	`CallElasticsearch`	net472	2.62μs	0.425ns	1.64ns	0.183	1.16 KB
#3177	`CallElasticsearch`	netcoreapp3.1	1.59μs	0.51ns	1.98ns	0.0151	1.1 KB
#3177	`CallElasticsearchAsync`	net472	2.63μs	1.46ns	5.66ns	0.204	1.29 KB
#3177	`CallElasticsearchAsync`	netcoreapp3.1	1.62μs	1.18ns	4.26ns	0.0161	1.22 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`ExecuteAsync`	net472	2.61μs	7.8ns	30.2ns	0.226	1.42 KB
master	`ExecuteAsync`	netcoreapp3.1	1.74μs	4.5ns	17.4ns	0.0181	1.34 KB
#3177	`ExecuteAsync`	net472	2.72μs	10.4ns	40.3ns	0.225	1.42 KB
#3177	`ExecuteAsync`	netcoreapp3.1	1.77μs	4.56ns	17.1ns	0.0179	1.34 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendAsync`	net472	5.66μs	8.01ns	28.9ns	0.438	2.77 KB
master	`SendAsync`	netcoreapp3.1	3.6μs	8.33ns	32.3ns	0.0343	2.6 KB
#3177	`SendAsync`	net472	5.75μs	12.5ns	48.6ns	0.437	2.77 KB
#3177	`SendAsync`	netcoreapp3.1	3.67μs	7.34ns	28.4ns	0.0349	2.6 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net472	3.24μs	1.48ns	5.12ns	0.287	1.81 KB
master	`EnrichedLog`	netcoreapp3.1	2.45μs	0.941ns	3.64ns	0.0246	1.85 KB
#3177	`EnrichedLog`	net472	3.2μs	1.15ns	4.3ns	0.287	1.81 KB
#3177	`EnrichedLog`	netcoreapp3.1	2.53μs	0.837ns	3.13ns	0.0251	1.85 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`EnrichedLog`	net472	152μs	96.5ns	374ns	0.689	0.23	4.65 KB
master	`EnrichedLog`	netcoreapp3.1	115μs	142ns	530ns	0	0	4.49 KB
#3177	`EnrichedLog`	net472	152μs	98.4ns	381ns	0.688	0.229	4.65 KB
#3177	`EnrichedLog`	netcoreapp3.1	116μs	267ns	1.03μs	0	0	4.49 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`EnrichedLog`	net472	5.51μs	15.2ns	58.9ns	0.569	0.00274	3.59 KB
master	`EnrichedLog`	netcoreapp3.1	4.44μs	8.1ns	30.3ns	0.0538	0	3.91 KB
#3177	`EnrichedLog`	net472	5.61μs	21.2ns	82ns	0.568	0.00278	3.59 KB
#3177	`EnrichedLog`	netcoreapp3.1	4.36μs	11.5ns	44.5ns	0.0522	0	3.91 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendReceive`	net472	2.33μs	1.21ns	4.69ns	0.218	1.37 KB
master	`SendReceive`	netcoreapp3.1	1.76μs	0.821ns	2.96ns	0.0184	1.32 KB
#3177	`SendReceive`	net472	2.32μs	3.26ns	12.6ns	0.218	1.37 KB
#3177	`SendReceive`	netcoreapp3.1	1.81μs	0.749ns	2.9ns	0.0181	1.32 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net472	4.92μs	1.49ns	5.57ns	0.353	2.23 KB
master	`EnrichedLog`	netcoreapp3.1	4.21μs	1.79ns	6.93ns	0.0251	1.8 KB
#3177	`EnrichedLog`	net472	5.09μs	2.86ns	10.7ns	0.354	2.23 KB
#3177	`EnrichedLog`	netcoreapp3.1	4.3μs	3.78ns	14.6ns	0.0235	1.8 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`StartFinishSpan`	net472	1.19μs	0.381ns	1.48ns	0.129	810 B
master	`StartFinishSpan`	netcoreapp3.1	915ns	0.328ns	1.23ns	0.0102	760 B
master	`StartFinishScope`	net472	1.36μs	0.357ns	1.38ns	0.141	891 B
master	`StartFinishScope`	netcoreapp3.1	1.04μs	0.279ns	1ns	0.012	880 B
#3177	`StartFinishSpan`	net472	1.13μs	0.59ns	2.21ns	0.128	810 B
#3177	`StartFinishSpan`	netcoreapp3.1	973ns	3.17ns	11.4ns	0.0104	760 B
#3177	`StartFinishScope`	net472	1.33μs	0.443ns	1.66ns	0.141	891 B
#3177	`StartFinishScope`	netcoreapp3.1	1.1μs	0.43ns	1.67ns	0.0116	880 B

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`RunOnMethodBegin`	net472	1.44μs	0.336ns	1.3ns	0.141	891 B
master	`RunOnMethodBegin`	netcoreapp3.1	1.15μs	0.382ns	1.48ns	0.012	880 B
#3177	`RunOnMethodBegin`	net472	1.49μs	0.278ns	1ns	0.141	891 B
#3177	`RunOnMethodBegin`	netcoreapp3.1	1.23μs	0.33ns	1.28ns	0.0117	880 B

andrewlock · 2022-09-08T10:17:54Z

Code Coverage Report 📊

✔️ Merging #3177 into master will not change line coverage
✔️ Merging #3177 into master will not change branch coverage
⛔ Merging #3177 into master will will increase complexity by 5

	master	#3177	Change
Lines	`17816` / `24332`	`17885` / `24344`
Lines %	`73%`	`73%`	`0%` ✔️
Branches	`10500` / `14962`	`10532` / `14964`
Branches %	`70%`	`70%`	`0%` ✔️
Complexity	`16161`	`16166`	`5` ⛔

View the full report for further details:

Datadog.Trace Breakdown ✔️

	master	#3177	Change
Lines %	`73%`	`73%`	`0%` ✔️
Branches %	`70%`	`70%`	`0%` ✔️
Complexity	`16161`	`16166`	`5` ⛔

The following classes have significant coverage changes.

File	Line coverage change	Branch coverage change	Complexity change
Datadog.Trace.Debugger.ProbeStatuses.ProbeStatusPoller	`0%` ✔️	`-6%` ⛔	`0` ✔️
Datadog.Trace.Ci.CIVisibility	`5%` ✔️	`6%` ✔️	`0` ✔️
Datadog.Trace.Ci.GitInfo	`5%` ✔️	`8%` ✔️	`0` ✔️
Datadog.Trace.Telemetry.Transports.JsonTelemetryTransport	`9%` ✔️	`17%` ✔️	`0` ✔️
Datadog.Trace.Agent.Transports.HttpStreamRequestFactory	`12%` ✔️	`0%` ✔️	`0` ✔️

The following classes were added in #3177:

File	Line coverage	Branch coverage	Complexity
Datadog.Trace.DataStreamsMonitoring.Utils.DDSketchPool	`100%`	`100%`	`5`

View the full reports for further details:

andrewlock added the area:data-streams-monitoring label Sep 7, 2022

andrewlock requested a review from a team as a code owner September 7, 2022 06:34

This comment has been minimized.

Sign in to view

zacharycmontoya reviewed Sep 7, 2022

View reviewed changes

Comment thread tracer/test/Datadog.Trace.Tests/DataStreamsMonitoring/DDSketchPoolTests.cs Outdated

zacharycmontoya approved these changes Sep 7, 2022

View reviewed changes

andrewlock and others added 3 commits September 8, 2022 09:35

Add a simple pooling mechanism for DDSketches

4269398

We could end up creating a _lot_ of them, so keep a (bounded) pool

Update tracer/test/Datadog.Trace.Tests/DataStreamsMonitoring/DDSketch…

7a7eee1

…PoolTests.cs Co-authored-by: Zach Montoya <[email protected]>

Fix tests

bf7a469

andrewlock force-pushed the andrew/ddsketch-pooling branch from de4e9e8 to bf7a469 Compare September 8, 2022 08:35

This comment has been minimized.

Sign in to view

pierotibou approved these changes Sep 8, 2022

View reviewed changes

Comment thread tracer/test/Datadog.Trace.Tests/DataStreamsMonitoring/DDSketchPoolTests.cs Outdated

Update tracer/test/Datadog.Trace.Tests/DataStreamsMonitoring/DDSketch…

a616c67

…PoolTests.cs Co-authored-by: Pierre Bonet <[email protected]>

andrewlock merged commit 43d3a24 into master Sep 8, 2022

andrewlock deleted the andrew/ddsketch-pooling branch September 8, 2022 10:42

github-actions Bot added this to the vNext milestone Sep 8, 2022

Uh oh!

Conversation

andrewlock commented Sep 7, 2022

Summary of changes

Reason for change

Implementation details

Test coverage

Other details

Uh oh!

This comment has been minimized.

This comment has been minimized.

zacharycmontoya Sep 7, 2022

Choose a reason for hiding this comment

Uh oh!

zacharycmontoya Sep 7, 2022

Choose a reason for hiding this comment

Uh oh!

andrewlock Sep 8, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zacharycmontoya left a comment

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

Uh oh!

andrewlock commented Sep 8, 2022

Benchmarks Report 🐌

Benchmark details

Raw results

Slower ⚠️ in #3177

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Uh oh!

andrewlock commented Sep 8, 2022

Code Coverage Report 📊

Datadog.Trace Breakdown ✔️

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants