Implement the SpanSampler into the Tracer by bouwkast · Pull Request #3286 · DataDog/dd-trace-dotnet

bouwkast · 2022-09-30T15:13:27Z

Summary of changes

This pull request adds the SpanSampler into the Tracer to support the Single Span Ingestion feature.

Reason for change

To allow for individual Spans that were going to be dropped to be sampled independently of their host Trace.

Implementation details

This adds the ISpanSampler interface and SpanSampler implementation that will sample a Span within TraceContext.CloseSpan when its containing trace is being dropped due to having a priority <= 0.

Test coverage

Added unit tests for the SpanSampler -> SpanSamplerTests
Added some integration tests for testing the TraceContext.CloseSpan in a new class SpanTagTests to check whether the spans get tagged correctly

Other details

Previous PRs:

pierotibou

Nice work!
With a few comments, mainly on tests.

andrewlock

LGTM (though sampling's not really my thing!) Just minor comments

zacharycmontoya

LGTM

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

kevingosse · 2022-10-18T12:51:28Z

I tried running the benchmarks on master and they're the same as your PR, so I guess the difference comes from a commit you haven't merged into your branch.

The tags wouldn't get set throgh Span.SetTag as the Span would be marked as finished before it was actually closed. When it gets closed is when the span sampler runs.

andrewlock · 2022-10-18T15:24:47Z

Benchmarks Report 🐌

Benchmarks for #3286 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	808μs	590ns	2.21μs	0.403	3.22 KB
master	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	556μs	272ns	1.02μs	0	2.62 KB
#3286	`WriteAndFlushEnrichedTraces`	net472	795μs	797ns	2.98μs	0.393	3.22 KB
#3286	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	562μs	120ns	449ns	0	2.63 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`AllCycleSimpleBody`	net472	197ns	0.0869ns	0.313ns	0.0701	9.85E-05	441 B
master	`AllCycleSimpleBody`	netcoreapp3.1	257ns	0.24ns	0.928ns	0.00605	0	440 B
master	`AllCycleMoreComplexBody`	net472	196ns	0.0453ns	0.17ns	0.0663	0	417 B
master	`AllCycleMoreComplexBody`	netcoreapp3.1	252ns	0.244ns	0.945ns	0.00567	0	416 B
master	`BodyExtractorSimpleBody`	net472	270ns	0.365ns	1.37ns	0.0573	0	361 B
master	`BodyExtractorSimpleBody`	netcoreapp3.1	244ns	0.123ns	0.461ns	0.00368	0	272 B
master	`BodyExtractorMoreComplexBody`	net472	16.3μs	17.7ns	68.4ns	1.2	0.0163	7.62 KB
master	`BodyExtractorMoreComplexBody`	netcoreapp3.1	12.9μs	6.84ns	26.5ns	0.0895	0	6.75 KB
#3286	`AllCycleSimpleBody`	net472	198ns	0.142ns	0.532ns	0.0701	0	441 B
#3286	`AllCycleSimpleBody`	netcoreapp3.1	252ns	0.315ns	1.22ns	0.00607	0	440 B
#3286	`AllCycleMoreComplexBody`	net472	198ns	0.0701ns	0.253ns	0.0663	0	417 B
#3286	`AllCycleMoreComplexBody`	netcoreapp3.1	252ns	0.197ns	0.762ns	0.00578	0	416 B
#3286	`BodyExtractorSimpleBody`	net472	268ns	0.178ns	0.668ns	0.0573	0	361 B
#3286	`BodyExtractorSimpleBody`	netcoreapp3.1	240ns	0.162ns	0.607ns	0.00373	0	272 B
#3286	`BodyExtractorMoreComplexBody`	net472	15.7μs	14.9ns	57.6ns	1.21	0.0157	7.62 KB
#3286	`BodyExtractorMoreComplexBody`	netcoreapp3.1	12.9μs	3.81ns	14.2ns	0.0901	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	184μs	186ns	720ns	0.274	20.66 KB
#3286	`SendRequest`	net472	0ns	0ns	0ns	0	0 b
#3286	`SendRequest`	netcoreapp3.1	182μs	234ns	905ns	0.181	20.66 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.81μs	0.509ns	1.91ns	0.158	0.000903	995 B
master	`ExecuteNonQuery`	netcoreapp3.1	1.4μs	0.516ns	1.86ns	0.0133	0	984 B
#3286	`ExecuteNonQuery`	net472	1.83μs	0.671ns	2.6ns	0.158	0.000908	995 B
#3286	`ExecuteNonQuery`	netcoreapp3.1	1.46μs	0.428ns	1.6ns	0.0131	0	984 B

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`CallElasticsearch`	net472	2.5μs	0.855ns	3.08ns	0.191	1.2 KB
master	`CallElasticsearch`	netcoreapp3.1	1.51μs	0.938ns	3.51ns	0.0158	1.14 KB
master	`CallElasticsearchAsync`	net472	2.63μs	0.441ns	1.59ns	0.212	1.34 KB
master	`CallElasticsearchAsync`	netcoreapp3.1	1.58μs	0.483ns	1.74ns	0.0173	1.26 KB
#3286	`CallElasticsearch`	net472	2.57μs	0.609ns	2.36ns	0.191	1.2 KB
#3286	`CallElasticsearch`	netcoreapp3.1	1.56μs	0.844ns	3.27ns	0.0156	1.14 KB
#3286	`CallElasticsearchAsync`	net472	2.67μs	0.866ns	3.35ns	0.213	1.34 KB
#3286	`CallElasticsearchAsync`	netcoreapp3.1	1.65μs	0.645ns	2.5ns	0.0165	1.26 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`ExecuteAsync`	net472	2.77μs	3.81ns	14.2ns	0.233	1.47 KB
master	`ExecuteAsync`	netcoreapp3.1	1.74μs	1.45ns	5.41ns	0.0184	1.39 KB
#3286	`ExecuteAsync`	net472	2.74μs	1.16ns	4.48ns	0.233	1.47 KB
#3286	`ExecuteAsync`	netcoreapp3.1	1.78μs	0.526ns	1.97ns	0.0187	1.39 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendAsync`	net472	5.96μs	1.27ns	4.77ns	0.446	2.82 KB
master	`SendAsync`	netcoreapp3.1	3.69μs	1.98ns	7.68ns	0.035	2.65 KB
#3286	`SendAsync`	net472	5.76μs	2.45ns	9.18ns	0.445	2.82 KB
#3286	`SendAsync`	netcoreapp3.1	3.77μs	1.09ns	4.09ns	0.0362	2.65 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net472	3.23μs	4.46ns	16.1ns	0.297	1.88 KB
master	`EnrichedLog`	netcoreapp3.1	2.51μs	1.84ns	6.9ns	0.0264	1.91 KB
#3286	`EnrichedLog`	net472	3.19μs	1.47ns	5.51ns	0.298	1.88 KB
#3286	`EnrichedLog`	netcoreapp3.1	2.44μs	1.36ns	5.27ns	0.0255	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	151μs	170ns	657ns	0.679	0.226	4.72 KB
master	`EnrichedLog`	netcoreapp3.1	121μs	136ns	510ns	0.0597	0	4.55 KB
#3286	`EnrichedLog`	net472	152μs	232ns	899ns	0.685	0.228	4.72 KB
#3286	`EnrichedLog`	netcoreapp3.1	121μs	206ns	771ns	0	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	1.64ns	6.35ns	0.579	0.00284	3.65 KB
master	`EnrichedLog`	netcoreapp3.1	4.42μs	1.63ns	6.09ns	0.053	0	3.98 KB
#3286	`EnrichedLog`	net472	5.93μs	1.67ns	6.01ns	0.58	0.00296	3.65 KB
#3286	`EnrichedLog`	netcoreapp3.1	4.33μs	2.55ns	9.55ns	0.0541	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.24μs	4.02ns	15.6ns	0.226	1.42 KB
master	`SendReceive`	netcoreapp3.1	1.84μs	0.668ns	2.59ns	0.0184	1.37 KB
#3286	`SendReceive`	net472	2.4μs	2.24ns	8.67ns	0.226	1.42 KB
#3286	`SendReceive`	netcoreapp3.1	1.85μs	0.973ns	3.77ns	0.0185	1.37 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	3.09ns	12ns	0.365	2.3 KB
master	`EnrichedLog`	netcoreapp3.1	4.34μs	2.07ns	7.45ns	0.0242	1.86 KB
#3286	`EnrichedLog`	net472	4.99μs	2ns	7.48ns	0.364	2.3 KB
#3286	`EnrichedLog`	netcoreapp3.1	4.25μs	2.55ns	9.53ns	0.0253	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.18μs	0.48ns	1.8ns	0.136	859 B
master	`StartFinishSpan`	netcoreapp3.1	936ns	0.362ns	1.31ns	0.011	808 B
master	`StartFinishScope`	net472	1.42μs	0.462ns	1.73ns	0.149	939 B
master	`StartFinishScope`	netcoreapp3.1	1.04μs	0.754ns	2.92ns	0.0125	928 B
#3286	`StartFinishSpan`	net472	1.24μs	0.21ns	0.812ns	0.136	859 B
#3286	`StartFinishSpan`	netcoreapp3.1	908ns	0.299ns	1.12ns	0.0109	808 B
#3286	`StartFinishScope`	net472	1.4μs	0.513ns	1.78ns	0.149	939 B
#3286	`StartFinishScope`	netcoreapp3.1	1.12μs	0.391ns	1.51ns	0.0122	928 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.546ns	2.12ns	0.149	939 B
master	`RunOnMethodBegin`	netcoreapp3.1	1.17μs	0.438ns	1.7ns	0.0122	928 B
#3286	`RunOnMethodBegin`	net472	1.54μs	0.768ns	2.97ns	0.149	939 B
#3286	`RunOnMethodBegin`	netcoreapp3.1	1.13μs	0.463ns	1.67ns	0.0125	928 B

andrewlock · 2022-10-18T16:34:38Z

Code Coverage Report 📊

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

	master	#3286	Change
Lines	`19602` / `27011`	`19681` / `27042`
Lines %	`73%`	`73%`	`0%` ✔️
Branches	`11633` / `16784`	`11675` / `16806`
Branches %	`69%`	`69%`	`0%` ✔️
Complexity	`18160`	`18184`	`24` ⛔

View the full report for further details:

Datadog.Trace Breakdown ✔️

	master	#3286	Change
Lines %	`73%`	`73%`	`0%` ✔️
Branches %	`69%`	`69%`	`0%` ✔️
Complexity	`18160`	`18184`	`24` ⛔

The following classes have significant coverage changes.

File	Line coverage change	Branch coverage change	Complexity change
Datadog.Trace.Iast.StackWalker	`0%` ✔️	`6%` ✔️	`0` ✔️
Datadog.Trace.Debugger.PInvoke.DebuggerNativeMethods	`6%` ✔️	`0%` ✔️	`0` ✔️
Datadog.Trace.DataStreamsMonitoring.Transport.DataStreamsApi	`17%` ✔️	`0%` ✔️	`0` ✔️
Datadog.Trace.ClrProfiler.AutoInstrumentation.Http.HttpClient.WinHttpHandler.WinHttpHandlerIntegration	`100%` ✔️	`0%` ✔️	`0` ✔️

The following classes were added in #3286:

File	Line coverage	Branch coverage	Complexity
Datadog.Trace.Sampling.SpanSampler	`100%`	`100%`	`13`

View the full reports for further details:

bouwkast added 2 commits September 30, 2022 08:44

Add SpanRateLimiter for Single Span Ingestion

99768e3

Swap log statement from Warning to Debug

7ba0b8e

lucaspimentel reviewed Oct 4, 2022

View reviewed changes

Comment thread tracer/src/Datadog.Trace/IDatadogTracer.cs

lucaspimentel reviewed Oct 4, 2022

View reviewed changes

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

lucaspimentel reviewed Oct 4, 2022

View reviewed changes

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

lucaspimentel reviewed Oct 4, 2022

View reviewed changes

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

lucaspimentel reviewed Oct 4, 2022

View reviewed changes

Comment thread tracer/src/Datadog.Trace/TraceContext.cs Outdated

lucaspimentel reviewed Oct 4, 2022

View reviewed changes

Comment thread tracer/src/Datadog.Trace/Tracer.cs Outdated

bouwkast force-pushed the steven/single-span/sampler branch from 0283733 to 14ce1ec Compare October 7, 2022 15:30

pierotibou reviewed Oct 10, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

bouwkast marked this pull request as ready for review October 11, 2022 18:40

bouwkast requested review from a team as code owners October 11, 2022 18:40

This comment has been minimized.

Sign in to view

pierotibou approved these changes Oct 12, 2022

View reviewed changes

andrewlock approved these changes Oct 12, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

zacharycmontoya reviewed Oct 12, 2022

View reviewed changes

Comment thread tracer/test/Datadog.Trace.Tests/Sampling/SpanSamplerTests.cs

zacharycmontoya approved these changes Oct 12, 2022

View reviewed changes

bouwkast force-pushed the steven/single-span/sampler branch from 83daacc to 2bcd43e Compare October 13, 2022 12:34

bouwkast added 7 commits October 13, 2022 08:40

Add SpanRateLimiter for Single Span Ingestion

a7e6cea

Swap log statement from Warning to Debug

e17b72b

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

bouwkast added 4 commits October 13, 2022 08:53

Add additional glob pattern tests to SpanSamplerTests

7159d10

Remove TODO on testing rate limiter

55f05d5

Create a SpanSampler without any rules instead of null

5630bb8

Add examples to SpanSamplingRules in ConfigurationKeys

13a2096

This comment has been minimized.

Sign in to view

bouwkast force-pushed the steven/single-span/sampler branch from 2bcd43e to 13a2096 Compare October 13, 2022 17:24

This comment has been minimized.

Sign in to view

Base automatically changed from steven/single-span/rule to master October 13, 2022 18:43

bouwkast added 2 commits October 13, 2022 15:30

Fix logic to prioritize first matched rule

2ac5825

Merge branch 'master' into steven/single-span/sampler

c093525

This comment has been minimized.

Sign in to view

Re-add fake AgentWriter to SpanTagTests

3099dd9

This comment has been minimized.

Sign in to view

Commit initializing the mock api

6615296

This comment has been minimized.

Sign in to view

Merge branch 'master' into steven/single-span/sampler

a3042e1

This comment has been minimized.

Sign in to view

Merge branch 'master' into steven/single-span/sampler

871f320

This comment has been minimized.

Sign in to view

Set tags through Span.Tags.SetTag

3f8bda3

The tags wouldn't get set throgh Span.SetTag as the Span would be marked as finished before it was actually closed. When it gets closed is when the span sampler runs.

bouwkast merged commit e10b088 into master Oct 18, 2022

bouwkast deleted the steven/single-span/sampler branch October 18, 2022 17:42

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

anna-git added the area:tracer The core tracer library (Datadog.Trace, does not include OpenTracing, native code, or integrations) label Oct 25, 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andrewlock left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

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.

This comment has been minimized.

This comment has been minimized.

kevingosse commented Oct 18, 2022

Uh oh!

This comment has been minimized.

andrewlock commented Oct 18, 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

bouwkast commented Sep 30, 2022 •

edited

Loading