Add `SpanSamplingRule` for Single Span Ingestion by bouwkast · Pull Request #3283 · DataDog/dd-trace-dotnet

bouwkast · 2022-09-30T13:08:01Z

Summary of changes

Adds a SpanSamplingRule that is used for configuring Single Span Ingestion rules.

Reason for change

Single Span Ingestion needs its own set of rules.

Implementation details

From the RFC:

<span sampling rules>  ::=  [<span sampling rule>*]

<span sampling rule>  ::=  {
    "service": <glob pattern>,     [default: "*"]
    "name": <glob pattern>,        [default: "*"]
    "sample_rate": float,          [default: 1.0]
    "max_per_second": float [unlimited if absent]
}

<glob pattern>  ::=  string

For the glob pattern, I opted to just convert it to Regex for the time being.

Test coverage

I've added tests that are very similar to CustomSamplingRuleTests which may point out that there is some refactoring that should be done.
I didn't test the actual performance of the Regex, which I should probably do now that I think about it

Other details

Dependent on Add SpanRateLimiter for Single Span Ingestion #3282

andrewlock

Just minor code style comments/questions, mostly because I don't have all the sampling stuff in my head yet - better for Lucas or Zach to review that side of it I think :D

andrewlock · 2022-09-30T13:15:27Z

+        /// <para><c>0.0</c> is drop all.</para>
+        /// <para><c>1.0</c> is accept all.</para>
+        /// </value>
+        float SamplingRate { get; }


Any reason for float over double?

Good question
I chose a float because the RFC called it a float

andrewlock · 2022-09-30T13:47:08Z

+        {
+            // TODO default glob (maybe null/empty/whitespace) should be *
+            var regexPattern = "^" + Regex.Escape(glob).Replace("\\?", ".").Replace("\\*", ".*") + "$";
+            var regex = new Regex(regexPattern, RegexOptions.Compiled, RegexTimeout);


Maybe worth using the non-backtracking regex if available?

👍

I had to add a #if NETCOREAPP3_1_OR_GREATER to it to get that option 2b186e9
Is there a way to do non-backtracking regex in previous frameworks?

Unfortunately not, but probably still worth it 🙂

zacharycmontoya · 2022-10-04T00:06:58Z

+            // TODO should we log here?
+            if (!IsMatch(span))
+            {
+                return false;


I'm not quite sure how ShouldKeep will be used, but I'm a little concerned that the decision for "didn't match" and "chose not to sample" both result in a false return value. What do you think about making the return type bool?? Or perhaps we require the caller to call IsMatch first so we always know that the rule applies?

I like the idea of breaking it up to try and call the IsMatch first and then go and making the sampling decision with some separate function

I'll try that out and see how that works with the SpanSampler implementation

Split these up in 300ce78
To be a IsMatch that returns a bool for whether the glob patterns match and ShouldSample that returns bool for whether the sampling rate/limit are good

zacharycmontoya

Looks good with some really small feedback 👍🏼

…/dd-trace-dotnet into steven/single-span/limiter

andrewlock · 2022-10-13T16:14:48Z

Benchmarks Report 🐌

Benchmarks for #3283 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	Allocated
master	`WriteAndFlushEnrichedTraces`	net472	782μs	876ns	3.39μs	0.388	3.22 KB
master	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	535μs	197ns	736ns	0	2.63 KB
#3283	`WriteAndFlushEnrichedTraces`	net472	787μs	378ns	1.36μs	0.391	3.22 KB
#3283	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	533μs	360ns	1.4μs	0	2.62 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`AllCycleSimpleBody`	net472	198ns	0.0787ns	0.284ns	0.0726	0	457 B
master	`AllCycleSimpleBody`	netcoreapp3.1	255ns	0.146ns	0.548ns	0.00614	0	456 B
master	`AllCycleMoreComplexBody`	net472	200ns	0.052ns	0.18ns	0.0688	0	433 B
master	`AllCycleMoreComplexBody`	netcoreapp3.1	252ns	0.161ns	0.601ns	0.00595	0	432 B
master	`BodyExtractorSimpleBody`	net472	317ns	0.0885ns	0.319ns	0.0572	0	361 B
master	`BodyExtractorSimpleBody`	netcoreapp3.1	238ns	0.0973ns	0.351ns	0.0037	0	272 B
master	`BodyExtractorMoreComplexBody`	net472	15.8μs	17.1ns	63.8ns	1.21	0.0158	7.62 KB
master	`BodyExtractorMoreComplexBody`	netcoreapp3.1	12.8μs	5.29ns	20.5ns	0.089	0	6.75 KB
#3283	`AllCycleSimpleBody`	net472	200ns	0.18ns	0.697ns	0.0726	0	457 B
#3283	`AllCycleSimpleBody`	netcoreapp3.1	254ns	0.114ns	0.411ns	0.00625	0	456 B
#3283	`AllCycleMoreComplexBody`	net472	194ns	0.0618ns	0.223ns	0.0689	0	433 B
#3283	`AllCycleMoreComplexBody`	netcoreapp3.1	252ns	0.21ns	0.812ns	0.00596	0	432 B
#3283	`BodyExtractorSimpleBody`	net472	285ns	0.61ns	2.28ns	0.0574	0	361 B
#3283	`BodyExtractorSimpleBody`	netcoreapp3.1	233ns	1.16ns	4.91ns	0.00378	0	272 B
#3283	`BodyExtractorMoreComplexBody`	net472	15.8μs	15ns	57.9ns	1.2	0.0157	7.62 KB
#3283	`BodyExtractorMoreComplexBody`	netcoreapp3.1	12.8μs	6.51ns	25.2ns	0.0898	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	181μs	185ns	715ns	0.27	20.68 KB
#3283	`SendRequest`	net472	0ns	0ns	0ns	0	0 b
#3283	`SendRequest`	netcoreapp3.1	183μs	330ns	1.28μs	0.275	20.68 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.79μs	0.719ns	2.69ns	0.155	0.000901	979 B
master	`ExecuteNonQuery`	netcoreapp3.1	1.46μs	0.646ns	2.33ns	0.0131	0	968 B
#3283	`ExecuteNonQuery`	net472	1.88μs	1.52ns	5.88ns	0.155	0.000938	979 B
#3283	`ExecuteNonQuery`	netcoreapp3.1	1.45μs	0.293ns	1.13ns	0.0131	0	968 B

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`CallElasticsearch`	net472	2.48μs	1.12ns	4.35ns	0.188	1.19 KB
master	`CallElasticsearch`	netcoreapp3.1	1.58μs	0.725ns	2.81ns	0.015	1.13 KB
master	`CallElasticsearchAsync`	net472	2.7μs	1.22ns	4.73ns	0.209	1.32 KB
master	`CallElasticsearchAsync`	netcoreapp3.1	1.63μs	0.616ns	2.31ns	0.0171	1.25 KB
#3283	`CallElasticsearch`	net472	2.49μs	0.642ns	2.49ns	0.188	1.19 KB
#3283	`CallElasticsearch`	netcoreapp3.1	1.58μs	0.772ns	2.78ns	0.015	1.13 KB
#3283	`CallElasticsearchAsync`	net472	2.78μs	1.33ns	5.16ns	0.21	1.32 KB
#3283	`CallElasticsearchAsync`	netcoreapp3.1	1.67μs	0.721ns	2.7ns	0.0167	1.25 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`ExecuteAsync`	net472	2.76μs	1.59ns	6.16ns	0.23	1.45 KB
master	`ExecuteAsync`	netcoreapp3.1	1.73μs	0.672ns	2.42ns	0.0183	1.38 KB
#3283	`ExecuteAsync`	net472	2.84μs	0.565ns	2.04ns	0.23	1.45 KB
#3283	`ExecuteAsync`	netcoreapp3.1	1.83μs	0.341ns	1.18ns	0.0183	1.38 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendAsync`	net472	5.85μs	2.16ns	8.36ns	0.444	2.8 KB
master	`SendAsync`	netcoreapp3.1	3.63μs	1.26ns	4.7ns	0.0344	2.63 KB
#3283	`SendAsync`	net472	5.78μs	1.62ns	6.26ns	0.444	2.8 KB
#3283	`SendAsync`	netcoreapp3.1	3.66μs	1.26ns	4.73ns	0.0349	2.63 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net472	3.13μs	3.5ns	13.1ns	0.297	1.88 KB
master	`EnrichedLog`	netcoreapp3.1	2.44μs	2.13ns	7.96ns	0.0257	1.91 KB
#3283	`EnrichedLog`	net472	3.19μs	2.42ns	9.37ns	0.298	1.88 KB
#3283	`EnrichedLog`	netcoreapp3.1	2.56μs	2.34ns	8.45ns	0.0257	1.91 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`EnrichedLog`	net472	153μs	172ns	642ns	0.688	0.229	4.72 KB
master	`EnrichedLog`	netcoreapp3.1	122μs	229ns	886ns	0.0606	0	4.55 KB
#3283	`EnrichedLog`	net472	151μs	166ns	643ns	0.68	0.227	4.72 KB
#3283	`EnrichedLog`	netcoreapp3.1	120μs	189ns	708ns	0.06	0	4.55 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.66μs	2.45ns	9.18ns	0.578	0.00284	3.65 KB
master	`EnrichedLog`	netcoreapp3.1	4.81μs	1.97ns	7.35ns	0.0531	0	3.98 KB
#3283	`EnrichedLog`	net472	5.88μs	2.15ns	8.05ns	0.58	0.00293	3.65 KB
#3283	`EnrichedLog`	netcoreapp3.1	4.42μs	1.89ns	7.07ns	0.0555	0	3.98 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendReceive`	net472	2.35μs	2.84ns	11ns	0.222	1.4 KB
master	`SendReceive`	netcoreapp3.1	1.87μs	0.794ns	2.97ns	0.0179	1.35 KB
#3283	`SendReceive`	net472	2.27μs	2.2ns	8.53ns	0.223	1.4 KB
#3283	`SendReceive`	netcoreapp3.1	1.73μs	0.553ns	2.07ns	0.0179	1.35 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net472	5.08μs	1.81ns	7ns	0.364	2.3 KB
master	`EnrichedLog`	netcoreapp3.1	4.18μs	1.36ns	5.28ns	0.0251	1.86 KB
#3283	`EnrichedLog`	net472	5.06μs	1.97ns	7.64ns	0.364	2.3 KB
#3283	`EnrichedLog`	netcoreapp3.1	4.14μs	1.37ns	4.95ns	0.0249	1.86 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`StartFinishSpan`	net472	1.21μs	0.369ns	1.33ns	0.134	842 B
master	`StartFinishSpan`	netcoreapp3.1	959ns	0.552ns	2.14ns	0.0111	792 B
master	`StartFinishScope`	net472	1.37μs	0.43ns	1.67ns	0.147	923 B
master	`StartFinishScope`	netcoreapp3.1	1.07μs	0.261ns	0.975ns	0.0123	912 B
#3283	`StartFinishSpan`	net472	1.22μs	0.321ns	1.2ns	0.133	842 B
#3283	`StartFinishSpan`	netcoreapp3.1	918ns	0.244ns	0.945ns	0.0105	792 B
#3283	`StartFinishScope`	net472	1.4μs	1.46ns	5.27ns	0.146	923 B
#3283	`StartFinishScope`	netcoreapp3.1	1.05μs	0.376ns	1.3ns	0.0126	912 B

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`RunOnMethodBegin`	net472	1.54μs	0.34ns	1.27ns	0.147	923 B
master	`RunOnMethodBegin`	netcoreapp3.1	1.18μs	0.32ns	1.24ns	0.0123	912 B
#3283	`RunOnMethodBegin`	net472	1.55μs	0.619ns	2.32ns	0.146	923 B
#3283	`RunOnMethodBegin`	netcoreapp3.1	1.16μs	0.297ns	1.07ns	0.0123	912 B

andrewlock · 2022-10-13T17:53:17Z

Code Coverage Report 📊

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

	master	#3283	Change
Lines	`19534` / `26924`	`19561` / `26971`
Lines %	`73%`	`73%`	`0%` ✔️
Branches	`11607` / `16747`	`11633` / `16770`
Branches %	`69%`	`69%`	`0%` ✔️
Complexity	`18113`	`18142`	`29` ⛔

View the full report for further details:

Datadog.Trace Breakdown ✔️

	master	#3283	Change
Lines %	`73%`	`73%`	`0%` ✔️
Branches %	`69%`	`69%`	`0%` ✔️
Complexity	`18113`	`18142`	`29` ⛔

The following classes have significant coverage changes.

File	Line coverage change	Branch coverage change	Complexity change
Datadog.Trace.Ci.GitInfo	`-17%` ⛔	`-11%` ⛔	`0` ✔️
Datadog.Trace.Propagators.SpanContextPropagator	`8%` ✔️	`6%` ✔️	`0` ✔️

The following classes were added in #3283:

File	Line coverage	Branch coverage	Complexity
Datadog.Trace.Sampling.SpanRateLimiter	`50%`	`100%`	`4`
Datadog.Trace.Sampling.SpanSamplingRule	`100%`	`87%`	`25`

View the full reports for further details:

Add SpanRateLimiter for Single Span Ingestion

99768e3

bouwkast requested a review from a team as a code owner September 30, 2022 13:08

bouwkast self-assigned this Sep 30, 2022

This comment has been minimized.

Sign in to view

andrewlock approved these changes Sep 30, 2022

View reviewed changes

bouwkast mentioned this pull request Sep 30, 2022

Implement the SpanSampler into the Tracer #3286

Merged

2 tasks

Swap log statement from Warning to Debug

7ba0b8e

pierotibou approved these changes Oct 3, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

zacharycmontoya reviewed Oct 3, 2022

View reviewed changes

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

zacharycmontoya reviewed Oct 4, 2022

View reviewed changes

zacharycmontoya approved these changes Oct 4, 2022

View reviewed changes

lucaspimentel changed the title ~~Add SpanSamplingRule for Single Span Ingestion~~ Add SpanSamplingRule for Single Span Ingestion Oct 4, 2022

lucaspimentel added the area:tracer The core tracer library (Datadog.Trace, does not include OpenTracing, native code, or integrations) label Oct 4, 2022

This comment has been minimized.

Sign in to view

bouwkast added 2 commits October 13, 2022 08:40

Add SpanRateLimiter for Single Span Ingestion

a7e6cea

Swap log statement from Warning to Debug

e17b72b

bouwkast force-pushed the steven/single-span/limiter branch from 7ba0b8e to e17b72b Compare October 13, 2022 12:40

bouwkast added 7 commits October 13, 2022 08:41

Merge branch 'steven/single-span/limiter' of ssh://github.com/DataDog…

9f0fecb

…/dd-trace-dotnet into steven/single-span/limiter

Add the ISpanSamplingRule interface

5cbc4ff

Add SpanSamplingRule implementation

46676b1

Remove unused constructor

3e55d92

Use NonBacktracking RegexOption in SpanSamplingRule

87a1ab1

Split ShouldKeep into IsMatch and ShouldSample

65852ec

Remove unnecessary ToList

0a15375

bouwkast added 7 commits October 13, 2022 08:41

Cache tagging strings to reduce allocations

2b3efca

Add cached sampling rule strings to interface

dac2e3e

Add test coverage for span sampling cached strings

3126164

Set MaxPerSecondString correctly

feabcf8

Fix MaxPerSecondString tests

9e64387

Return false in ShouldSample when span is null

6e89ebf

Swap tests to FluentAssertions

fd809c4

bouwkast force-pushed the steven/single-span/rule branch from 21ce657 to fd809c4 Compare October 13, 2022 12:44

This comment has been minimized.

Sign in to view

Base automatically changed from steven/single-span/limiter to master October 13, 2022 14:59

bouwkast merged commit 3053c42 into master Oct 13, 2022

bouwkast deleted the steven/single-span/rule branch October 13, 2022 18:43

github-actions Bot added this to the vNext milestone Oct 13, 2022

Uh oh!

Conversation

bouwkast commented Sep 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of changes

Reason for change

Implementation details

Test coverage

Other details

Uh oh!

This comment has been minimized.

andrewlock left a comment

Choose a reason for hiding this comment

Uh oh!

andrewlock Sep 30, 2022

Choose a reason for hiding this comment

Uh oh!

bouwkast Oct 3, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

andrewlock Sep 30, 2022

Choose a reason for hiding this comment

Uh oh!

bouwkast Oct 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewlock Oct 11, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

Uh oh!

zacharycmontoya Oct 4, 2022

Choose a reason for hiding this comment

Uh oh!

bouwkast Oct 5, 2022

Choose a reason for hiding this comment

Uh oh!

bouwkast Oct 11, 2022

Choose a reason for hiding this comment

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.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

andrewlock commented Oct 13, 2022

Benchmarks Report 🐌

Benchmark details

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

Raw results

Uh oh!

andrewlock commented Oct 13, 2022

Code Coverage Report 📊

Datadog.Trace Breakdown ✔️

Uh oh!

Reviewers

Assignees

Labels

Projects

bouwkast commented Sep 30, 2022 •

edited

Loading

bouwkast Oct 3, 2022 •

edited

Loading