Compile Regex in `CustomSamplingRule` by bouwkast · Pull Request #3132 · DataDog/dd-trace-dotnet

bouwkast · 2022-08-26T15:05:58Z

Summary of changes

Compiles Regex used in CustomSamplingRule as it wasn't.

Reason for change

Regex wasn't being compiled.

Implementation details

Swapped out the string patterns for Regex instances that are compiled.

Test coverage

Existing tests appear to cover everything here.

Other details

Somewhat apart of the Single Span Ingestion feature. Also planning on swapping this out with the vendored IndieRegex to get some of those performance improvements here.

(I didn't do any benchmarking to see the actual impact of this)

lucaspimentel

LGTM. Left a few optional comments on ways I think we can simplify this code.

zacharycmontoya

LGTM

pierotibou

Just one question

andrewlock · 2022-09-01T20:21:00Z

Benchmarks Report 🐌

Benchmarks for #3132 compared to master:

1 benchmarks are slower, with geometric mean 1.277
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	713μs	576ns	2.23μs	0.357	3.18 KB
master	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	471μs	226ns	877ns	0	2.58 KB
#3132	`WriteAndFlushEnrichedTraces`	net472	713μs	459ns	1.78μs	0.357	3.18 KB
#3132	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	460μs	66.5ns	249ns	0	2.58 KB

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

Slower ⚠️ in #3132

Benchmark	diff/base	Base Median (ns)	Diff Median (ns)	Modality
Benchmarks.Trace.AppSecBodyBenchmark.AllCycleSimpleBody‑net472	1.277	182.20	232.62

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`AllCycleSimpleBody`	net472	183ns	0.256ns	0.993ns	0.0676	0	425 B
master	`AllCycleSimpleBody`	netcoreapp3.1	240ns	0.387ns	1.5ns	0.00584	0	424 B
master	`AllCycleMoreComplexBody`	net472	206ns	0.154ns	0.555ns	0.0637	0	401 B
master	`AllCycleMoreComplexBody`	netcoreapp3.1	236ns	0.307ns	1.19ns	0.00552	0	400 B
master	`BodyExtractorSimpleBody`	net472	271ns	0.434ns	1.62ns	0.0573	0	361 B
master	`BodyExtractorSimpleBody`	netcoreapp3.1	224ns	0.263ns	1.02ns	0.00363	0	272 B
master	`BodyExtractorMoreComplexBody`	net472	14.3μs	12.9ns	49.9ns	1.21	0.0215	7.62 KB
master	`BodyExtractorMoreComplexBody`	netcoreapp3.1	12.1μs	16.5ns	61.6ns	0.0909	0	6.75 KB
#3132	`AllCycleSimpleBody`	net472	233ns	0.189ns	0.708ns	0.0675	0	425 B
#3132	`AllCycleSimpleBody`	netcoreapp3.1	237ns	0.199ns	0.769ns	0.00585	0	424 B
#3132	`AllCycleMoreComplexBody`	net472	186ns	0.551ns	2.13ns	0.0638	0	401 B
#3132	`AllCycleMoreComplexBody`	netcoreapp3.1	233ns	0.338ns	1.31ns	0.00558	0	400 B
#3132	`BodyExtractorSimpleBody`	net472	248ns	0.515ns	2ns	0.0574	0	361 B
#3132	`BodyExtractorSimpleBody`	netcoreapp3.1	223ns	0.307ns	1.19ns	0.0037	0	272 B
#3132	`BodyExtractorMoreComplexBody`	net472	14.2μs	12.1ns	45.1ns	1.21	0.0213	7.62 KB
#3132	`BodyExtractorMoreComplexBody`	netcoreapp3.1	12.2μs	11.6ns	43.5ns	0.091	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	186μs	297ns	1.15μs	0.187	20.57 KB
#3132	`SendRequest`	net472	0ns	0ns	0ns	0	0 b
#3132	`SendRequest`	netcoreapp3.1	184μs	265ns	1.02μs	0.183	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.86μs	0.819ns	3.17ns	0.15	0.000939	947 B
master	`ExecuteNonQuery`	netcoreapp3.1	1.47μs	1.35ns	5.24ns	0.0126	0	936 B
#3132	`ExecuteNonQuery`	net472	1.84μs	0.551ns	2.06ns	0.15	0.000917	947 B
#3132	`ExecuteNonQuery`	netcoreapp3.1	1.48μs	0.501ns	1.88ns	0.0125	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.47μs	0.668ns	2.5ns	0.183	1.16 KB
master	`CallElasticsearch`	netcoreapp3.1	1.58μs	0.783ns	3.03ns	0.015	1.1 KB
master	`CallElasticsearchAsync`	net472	2.6μs	1.08ns	4.06ns	0.205	1.29 KB
master	`CallElasticsearchAsync`	netcoreapp3.1	1.63μs	1.23ns	4.59ns	0.0165	1.22 KB
#3132	`CallElasticsearch`	net472	2.59μs	1.13ns	4.23ns	0.183	1.16 KB
#3132	`CallElasticsearch`	netcoreapp3.1	1.53μs	0.431ns	1.55ns	0.0146	1.1 KB
#3132	`CallElasticsearchAsync`	net472	2.58μs	0.939ns	3.64ns	0.204	1.29 KB
#3132	`CallElasticsearchAsync`	netcoreapp3.1	1.65μs	0.461ns	1.66ns	0.0166	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.69μs	5.35ns	19.3ns	0.224	1.41 KB
master	`ExecuteAsync`	netcoreapp3.1	1.73μs	4.23ns	16.4ns	0.0182	1.34 KB
#3132	`ExecuteAsync`	net472	2.65μs	6.5ns	25.2ns	0.223	1.41 KB
#3132	`ExecuteAsync`	netcoreapp3.1	1.74μs	4.1ns	15.9ns	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.74μs	18.2ns	70.6ns	0.439	2.77 KB
master	`SendAsync`	netcoreapp3.1	3.61μs	5.89ns	22.8ns	0.0345	2.6 KB
#3132	`SendAsync`	net472	5.73μs	15.1ns	58.4ns	0.437	2.77 KB
#3132	`SendAsync`	netcoreapp3.1	3.68μs	5.75ns	22.3ns	0.0344	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.16μs	2.22ns	8.58ns	0.287	1.81 KB
master	`EnrichedLog`	netcoreapp3.1	2.5μs	1.1ns	4.12ns	0.0248	1.85 KB
#3132	`EnrichedLog`	net472	3.3μs	2.28ns	8.84ns	0.287	1.81 KB
#3132	`EnrichedLog`	netcoreapp3.1	2.63μs	1.03ns	3.98ns	0.0247	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	150μs	157ns	606ns	0.673	0.224	4.65 KB
master	`EnrichedLog`	netcoreapp3.1	116μs	141ns	510ns	0.058	0	4.49 KB
#3132	`EnrichedLog`	net472	154μs	233ns	901ns	0.691	0.23	4.65 KB
#3132	`EnrichedLog`	netcoreapp3.1	117μs	173ns	649ns	0.0589	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.74μs	9.19ns	35.6ns	0.57	0.00284	3.59 KB
master	`EnrichedLog`	netcoreapp3.1	4.64μs	11.2ns	41.7ns	0.0531	0	3.91 KB
#3132	`EnrichedLog`	net472	5.7μs	12.6ns	47.1ns	0.567	0.00285	3.59 KB
#3132	`EnrichedLog`	netcoreapp3.1	4.52μs	12.2ns	47.4ns	0.0537	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.38μs	1.74ns	6.49ns	0.218	1.37 KB
master	`SendReceive`	netcoreapp3.1	1.86μs	0.851ns	3.18ns	0.0177	1.32 KB
#3132	`SendReceive`	net472	2.44μs	2.22ns	8.59ns	0.217	1.37 KB
#3132	`SendReceive`	netcoreapp3.1	1.86μs	0.677ns	2.44ns	0.0176	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.94μs	1.42ns	5.32ns	0.353	2.23 KB
master	`EnrichedLog`	netcoreapp3.1	4.5μs	2.39ns	9.26ns	0.0225	1.8 KB
#3132	`EnrichedLog`	net472	5.16μs	1.99ns	7.17ns	0.354	2.23 KB
#3132	`EnrichedLog`	netcoreapp3.1	4.23μs	1.35ns	5.06ns	0.0232	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.11μs	1.19ns	4.44ns	0.129	810 B
master	`StartFinishSpan`	netcoreapp3.1	924ns	0.427ns	1.6ns	0.0102	760 B
master	`StartFinishScope`	net472	1.37μs	1.51ns	5.67ns	0.141	891 B
master	`StartFinishScope`	netcoreapp3.1	1.13μs	0.267ns	0.961ns	0.0118	880 B
#3132	`StartFinishSpan`	net472	1.15μs	1.09ns	4.09ns	0.129	810 B
#3132	`StartFinishSpan`	netcoreapp3.1	978ns	0.271ns	1.05ns	0.0102	760 B
#3132	`StartFinishScope`	net472	1.39μs	1.03ns	3.72ns	0.141	891 B
#3132	`StartFinishScope`	netcoreapp3.1	1.06μs	0.29ns	1.09ns	0.0121	880 B

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`RunOnMethodBegin`	net472	1.58μs	0.961ns	3.46ns	0.141	891 B
master	`RunOnMethodBegin`	netcoreapp3.1	1.21μs	0.424ns	1.64ns	0.0121	880 B
#3132	`RunOnMethodBegin`	net472	1.55μs	1.15ns	4.14ns	0.141	891 B
#3132	`RunOnMethodBegin`	netcoreapp3.1	1.18μs	0.391ns	1.46ns	0.012	880 B

andrewlock · 2022-09-01T20:49:21Z

Code Coverage Report 📊

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

	master	#3132	Change
Lines	`17482` / `23788`	`17428` / `23843`
Lines %	`73%`	`73%`	`0%` ✔️
Branches	`10347` / `14633`	`10326` / `14693`
Branches %	`71%`	`70%`	`0%` ✔️
Complexity	`15796`	`15857`	`61` ⛔

View the full report for further details:

Datadog.Trace Breakdown ✔️

	master	#3132	Change
Lines %	`73%`	`73%`	`0%` ✔️
Branches %	`71%`	`70%`	`0%` ✔️
Complexity	`15796`	`15857`	`61` ⛔

The following classes have significant coverage changes.

File	Line coverage change	Branch coverage change	Complexity change
Datadog.Trace.Ci.GitInfo	`-19%` ⛔	`-12%` ⛔	`0` ✔️
Datadog.Trace.Agent.Transports.SocketHandlerRequestFactory	`-7%` ⛔	`0%` ✔️	`0` ✔️
Datadog.Trace.Ci.Coverage.DefaultCoverageEventHandler	`0%` ✔️	`-30%` ⛔	`33` ⛔
Datadog.Trace.Ci.CIVisibility	`5%` ✔️	`6%` ✔️	`0` ✔️
Datadog.Trace.Ci.Coverage.CoverageContextContainer	`17%` ✔️	`-75%` ⛔	`3` ⛔

The following classes were added in #3132:

File	Line coverage	Branch coverage	Complexity
Datadog.Trace.Ci.Coverage.CoverageReporter`1	`0%`	`0%`	`13`
Datadog.Trace.Ci.Coverage.Metadata.ModuleCoverageMetadata	`0%`	`100%`	`4`
Datadog.Trace.Ci.Coverage.MethodValues	`0%`	`0%`	`2`
Datadog.Trace.Ci.Coverage.ModuleValue	`0%`	`0%`	`2`
Datadog.Trace.Ci.Coverage.TypeValues	`0%`	`0%`	`2`

2 classes were removed from Datadog.Trace in #3132

View the full reports for further details:

lucaspimentel · 2022-10-04T12:23:04Z

+                var serviceMatch = _serviceNameRegex?.Match(span.ServiceName).Success ?? true;
+                var operationMatch = _operationNameRegex?.Match(span.OperationName).Success ?? true;
+                return serviceMatch && operationMatch;


This comment is super late, but if we don't match on service name then we can short-circuit and skip the operation name regex.

return (_serviceNameRegex?.Match(span.ServiceName).Success ?? true) && (_operationNameRegex?.Match(span.OperationName).Success ?? true);

bouwkast self-assigned this Aug 26, 2022

bouwkast requested a review from a team as a code owner August 26, 2022 15:05

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

This comment has been minimized.

Sign in to view

lucaspimentel approved these changes Aug 29, 2022

View reviewed changes

Comment thread tracer/src/Datadog.Trace/Sampling/CustomSamplingRule.cs Outdated

zacharycmontoya approved these changes Aug 29, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

bouwkast added 2 commits August 31, 2022 11:33

Compile Regex in CustomSamplingRule

ce03498

Simplify CustomSamplingRule.IsMatch

a2fe556

bouwkast force-pushed the steven/custom-sampling-rule-regex branch from 2828a06 to a2fe556 Compare August 31, 2022 15:33

This comment has been minimized.

Sign in to view

pierotibou reviewed Sep 1, 2022

View reviewed changes

Comment thread tracer/src/Datadog.Trace/Sampling/CustomSamplingRule.cs

Throw for invalid regex in CustomSamplingRule

77a3929

pierotibou approved these changes Sep 2, 2022

View reviewed changes

Comment thread tracer/src/Datadog.Trace/Sampling/CustomSamplingRule.cs

bouwkast merged commit e133169 into master Sep 2, 2022

bouwkast deleted the steven/custom-sampling-rule-regex branch September 2, 2022 13:21

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

lucaspimentel reviewed Oct 4, 2022

View reviewed changes

Uh oh!

Conversation

bouwkast commented Aug 26, 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.

lucaspimentel left a comment

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.

This comment has been minimized.

This comment has been minimized.

pierotibou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

andrewlock commented Sep 1, 2022

Benchmarks Report 🐌

Benchmark details

Raw results

Slower ⚠️ in #3132

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 1, 2022

Code Coverage Report 📊

Datadog.Trace Breakdown ✔️

Uh oh!

Uh oh!

lucaspimentel Oct 4, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants