Skip to content

Commit 67c35ae

Browse files
NachoEchevarriaNachoEchevarria
authored
Fix benchmark flakiness (#7790)
## Summary of changes This PR addresses [benchmark flakiness detected](DataDog/relenv-benchmarking-service#91) by eliminating static constructors and moving all initialization to `[GlobalSetup]` methods, which BenchmarkDotNet explicitly excludes from timing measurements. Benchmarks were experiencing flakiness due to: 1. **Static constructor timing issues**: Static constructors run at unpredictable times and their overhead is included in early benchmark measurements 2. **Incorrect static field usage**: Using `static` fields with `[GlobalSetup]` creates confusion since GlobalSetup runs once per benchmark method, not once per class 3. **Native code warmup**: Some benchmarks (especially WAF-related) need some warmup to stabilize native code paths ### Why Use Instance Fields for Runtime State? `[GlobalSetup]` runs **once per benchmark method**, not once per class. Using instance fields for runtime state (clients, services, etc.): 1. **Ensures proper isolation** - Each benchmark gets a fresh class instance 2. **Prevents state leakage** - No shared state between different benchmark methods 3. **Matches BenchmarkDotNet semantics** - Clear intent that each benchmark is isolated ### Why Use `static readonly` for Test Data? For pre-computed immutable test data (lambdas, test objects, constant values): 1. **Better compiler optimization** - `static readonly` fields can be inlined and optimized better than instance fields 2. **Single allocation** - Test data allocated once, shared across all benchmark runs 3. **Clear intent** - Signals that the data never changes and is shared safely ## Reason for change Benchmark variance analysis revealed high Coefficients of Variation (CV) ranging from 5.8% to 151%, indicating unstable and unreliable performance measurements. ## Implementation details ## Test coverage ## Other details <!-- Fixes #{issue} --> <!-- ⚠️ Note: Where possible, please obtain 2 approvals prior to merging. Unless CODEOWNERS specifies otherwise, for external teams it is typically best to have one review from a team member, and one review from apm-dotnet. Trivial changes do not require 2 reviews. MergeQueue is NOT enabled in this repository. If you have write access to the repo, the PR has 1-2 approvals (see above), and all of the required checks have passed, you can use the Squash and Merge button to merge the PR. If you don't have write access, or you need help, reach out in the #apm-dotnet channel in Slack. -->
1 parent b6e31df commit 67c35ae

22 files changed

+555
-196
lines changed

‎tracer/test/benchmarks/Benchmarks.Trace/ActivityBenchmark.cs‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
#nullable enable
1+
#nullable enable
22
using System;
33
using System.Collections;
44
using System.Collections.Generic;
@@ -25,13 +25,14 @@ namespace Benchmarks.Trace;
2525
public class ActivityBenchmark
2626
{
2727
private const string SourceName = "BenchmarkSource";
28-
private static readonly Datadog.Trace.Activity.DuckTypes.ActivitySource _duckSource;
2928
private static readonly DateTime _startTime = DateTimeOffset.FromUnixTimeSeconds(0).UtcDateTime;
3029
private static readonly DateTime _endTime = DateTimeOffset.FromUnixTimeSeconds(5).UtcDateTime;
3130

32-
private static readonly ActivitySource _source;
31+
private Datadog.Trace.Activity.DuckTypes.ActivitySource _duckSource;
32+
private ActivitySource _source;
3333

34-
static ActivityBenchmark()
34+
[GlobalSetup]
35+
public void GlobalSetup()
3536
{
3637
_source = new ActivitySource(SourceName);
3738

@@ -59,7 +60,7 @@ public void StartStopWithChild()
5960
handler.ActivityStopped(SourceName, parentMock);
6061
}
6162

62-
private static Activity CreateActivity(Activity? parent = null)
63+
private Activity CreateActivity(Activity? parent = null)
6364
{
6465
var activity = parent is null
6566
? _source.CreateActivity("parent", System.Diagnostics.ActivityKind.Internal)

‎tracer/test/benchmarks/Benchmarks.Trace/AgentWriterBenchmark.cs‎

Lines changed: 24 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -15,27 +15,18 @@ namespace Benchmarks.Trace
1515
[MemoryDiagnoser]
1616
[BenchmarkAgent1]
1717
[BenchmarkCategory(Constants.TracerCategory)]
18-
1918
public class AgentWriterBenchmark
2019
{
2120
private const int SpanCount = 1000;
2221

23-
private static readonly IAgentWriter AgentWriter;
24-
private static readonly ArraySegment<Span> EnrichedSpans;
25-
static AgentWriterBenchmark()
26-
{
27-
var overrides = new NameValueConfigurationSource(new()
28-
{
29-
{ ConfigurationKeys.StartupDiagnosticLogEnabled, false.ToString() },
30-
{ ConfigurationKeys.TraceEnabled, false.ToString() },
31-
});
32-
var sources = new CompositeConfigurationSource(new[] { overrides, GlobalConfigurationSource.Instance });
33-
var settings = new TracerSettings(sources);
34-
35-
var api = new Api(new FakeApiRequestFactory(settings.Exporter.AgentUri), statsd: null, updateSampleRates: null, partialFlushEnabled: false);
36-
37-
AgentWriter = new AgentWriter(api, statsAggregator: null, statsd: null, automaticFlush: false);
22+
private IAgentWriter _agentWriter;
23+
private ArraySegment<Span> _enrichedSpans;
3824

25+
[GlobalSetup]
26+
public void GlobalSetup()
27+
{
28+
// Create spans in GlobalSetup, not static constructor
29+
// This ensures BenchmarkDotNet excludes allocation overhead from measurements
3930
var enrichedSpans = new Span[SpanCount];
4031
var now = DateTimeOffset.UtcNow;
4132

@@ -46,10 +37,22 @@ static AgentWriterBenchmark()
4637
enrichedSpans[i].SetMetric(Metrics.SamplingRuleDecision, 1.0);
4738
}
4839

49-
EnrichedSpans = new ArraySegment<Span>(enrichedSpans);
40+
_enrichedSpans = new ArraySegment<Span>(enrichedSpans);
41+
42+
var overrides = new NameValueConfigurationSource(new()
43+
{
44+
{ ConfigurationKeys.StartupDiagnosticLogEnabled, false.ToString() },
45+
{ ConfigurationKeys.TraceEnabled, false.ToString() },
46+
});
47+
var sources = new CompositeConfigurationSource(new[] { overrides, GlobalConfigurationSource.Instance });
48+
var settings = new TracerSettings(sources);
49+
50+
var api = new Api(new FakeApiRequestFactory(settings.Exporter.AgentUri), statsd: null, updateSampleRates: null, partialFlushEnabled: false);
51+
52+
_agentWriter = new AgentWriter(api, statsAggregator: null, statsd: null, automaticFlush: false);
5053

51-
// Run benchmarks once to reduce noise
52-
new AgentWriterBenchmark().WriteAndFlushEnrichedTraces().GetAwaiter().GetResult();
54+
// Warmup to reduce noise
55+
WriteAndFlushEnrichedTraces().GetAwaiter().GetResult();
5356
}
5457

5558
/// <summary>
@@ -58,8 +61,8 @@ static AgentWriterBenchmark()
5861
[Benchmark]
5962
public Task WriteAndFlushEnrichedTraces()
6063
{
61-
AgentWriter.WriteTrace(EnrichedSpans);
62-
return AgentWriter.FlushTracesAsync();
64+
_agentWriter.WriteTrace(_enrichedSpans);
65+
return _agentWriter.FlushTracesAsync();
6366
}
6467

6568
/// <summary>

‎tracer/test/benchmarks/Benchmarks.Trace/Asm/AppSecBodyBenchmark.cs‎

Lines changed: 29 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -29,35 +29,23 @@ namespace Benchmarks.Trace.Asm
2929
[IgnoreProfile]
3030
public class AppSecBodyBenchmark
3131
{
32-
private static readonly Security _security;
33-
private readonly ComplexModel _complexModel = new()
34-
{
35-
Age = 12,
36-
Gender = "Female",
37-
Name = "Tata",
38-
LastName = "Toto",
39-
Address = new Address { Number = 12, City = new City { Name = "Paris", Country = new Country { Name = "France", Continent = new Continent { Name = "Europe", Planet = new Planet { Name = "Earth" } } } }, IsHouse = false, NameStreet = "lorem ipsum dolor sit amet" },
40-
Address2 = new Address { Number = 15, City = new City { Name = "Madrid", Country = new Country { Name = "Spain", Continent = new Continent { Name = "Europe", Planet = new Planet { Name = "Earth" } } } }, IsHouse = true, NameStreet = "lorem ipsum dolor sit amet" },
41-
Dogs = new List<Dog> { new Dog { Name = "toto", Dogs = new List<Dog> { new Dog { Name = "titi" }, new Dog { Name = "titi" } } }, new Dog { Name = "toto", Dogs = new List<Dog> { new Dog { Name = "tata" }, new Dog { Name = "tata" } } }, new Dog { Name = "tata", Dogs = new List<Dog> { new Dog { Name = "titi" }, new Dog { Name = "titi" }, new Dog { Name = "tutu" } } } }
42-
};
43-
44-
private readonly Props10String _props10 = ConstructionUtils.ConstructProps10String();
45-
private readonly Props100String _props100 = ConstructionUtils.ConstructProps100String();
46-
private readonly Props1000String _props1000 = ConstructionUtils.ConstructProps1000String();
47-
48-
private readonly Props10Rec _props10x3 = ConstructionUtils.ConstructProps10Rec(3);
49-
private readonly Props10Rec _props10x6 = ConstructionUtils.ConstructProps10Rec(6);
50-
51-
52-
53-
private static HttpContext _httpContext;
54-
55-
static AppSecBodyBenchmark()
32+
private Security _security;
33+
private ComplexModel _complexModel;
34+
private Props10String _props10;
35+
private Props100String _props100;
36+
private Props1000String _props1000;
37+
private Props10Rec _props10x3;
38+
private Props10Rec _props10x6;
39+
private HttpContext _httpContext;
40+
41+
[GlobalSetup]
42+
public void GlobalSetup()
5643
{
5744
AppSecBenchmarkUtils.SetupDummyAgent();
5845
var dir = Directory.GetCurrentDirectory();
5946
Environment.SetEnvironmentVariable("DD_APPSEC_ENABLED", "true");
6047
_security = Security.Instance;
48+
6149
#if NETFRAMEWORK
6250
var ms = new MemoryStream();
6351
using var sw = new StreamWriter(ms);
@@ -66,6 +54,23 @@ static AppSecBodyBenchmark()
6654
#else
6755
_httpContext = new DefaultHttpContext();
6856
#endif
57+
58+
_complexModel = new()
59+
{
60+
Age = 12,
61+
Gender = "Female",
62+
Name = "Tata",
63+
LastName = "Toto",
64+
Address = new Address { Number = 12, City = new City { Name = "Paris", Country = new Country { Name = "France", Continent = new Continent { Name = "Europe", Planet = new Planet { Name = "Earth" } } } }, IsHouse = false, NameStreet = "lorem ipsum dolor sit amet" },
65+
Address2 = new Address { Number = 15, City = new City { Name = "Madrid", Country = new Country { Name = "Spain", Continent = new Continent { Name = "Europe", Planet = new Planet { Name = "Earth" } } } }, IsHouse = true, NameStreet = "lorem ipsum dolor sit amet" },
66+
Dogs = new List<Dog> { new Dog { Name = "toto", Dogs = new List<Dog> { new Dog { Name = "titi" }, new Dog { Name = "titi" } } }, new Dog { Name = "toto", Dogs = new List<Dog> { new Dog { Name = "tata" }, new Dog { Name = "tata" } } }, new Dog { Name = "tata", Dogs = new List<Dog> { new Dog { Name = "titi" }, new Dog { Name = "titi" }, new Dog { Name = "tutu" } } } }
67+
};
68+
69+
_props10 = ConstructionUtils.ConstructProps10String();
70+
_props100 = ConstructionUtils.ConstructProps100String();
71+
_props1000 = ConstructionUtils.ConstructProps1000String();
72+
_props10x3 = ConstructionUtils.ConstructProps10Rec(3);
73+
_props10x6 = ConstructionUtils.ConstructProps10Rec(6);
6974
}
7075

7176
[Benchmark]

‎tracer/test/benchmarks/Benchmarks.Trace/Asm/AppSecEncodeBenchmark.cs‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -21,16 +21,18 @@ namespace Benchmarks.Trace.Asm;
2121
[BenchmarkCategory(Constants.AppSecCategory)]
2222
public class AppSecEncoderBenchmark
2323
{
24-
private static readonly Encoder _encoder;
25-
private static readonly EncoderLegacy _encoderLegacy;
26-
private static readonly NestedMap _args;
24+
private Encoder _encoder;
25+
private EncoderLegacy _encoderLegacy;
26+
private NestedMap _args;
2727

28-
static AppSecEncoderBenchmark()
28+
[GlobalSetup]
29+
public void GlobalSetup()
2930
{
3031
AppSecBenchmarkUtils.SetupDummyAgent();
3132
_encoder = new Encoder();
3233
var wafLibraryInvoker = AppSecBenchmarkUtils.CreateWafLibraryInvoker();
3334
_encoderLegacy = new EncoderLegacy(wafLibraryInvoker);
35+
3436
_args = MakeNestedMap(20);
3537
}
3638

‎tracer/test/benchmarks/Benchmarks.Trace/Asm/AppSecWafBenchmark.cs‎

Lines changed: 30 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -27,14 +27,14 @@ public class AppSecWafBenchmark
2727
{
2828
private const int TimeoutMicroSeconds = 1_000_000;
2929

30-
private static readonly Waf Waf;
31-
32-
private static readonly Dictionary<string, object> stage1 = MakeRealisticNestedMapStage1(false);
33-
private static readonly Dictionary<string, object> stage1Attack = MakeRealisticNestedMapStage1(true);
34-
private static readonly Dictionary<string, object> stage2 = MakeRealisticNestedMapStage2();
35-
private static readonly Dictionary<string, object> stage3 = MakeRealisticNestedMapStage3();
36-
37-
static AppSecWafBenchmark()
30+
private Waf _waf;
31+
private Dictionary<string, object> _stage1;
32+
private Dictionary<string, object> _stage1Attack;
33+
private Dictionary<string, object> _stage2;
34+
private Dictionary<string, object> _stage3;
35+
36+
[GlobalSetup]
37+
public void GlobalSetup()
3838
{
3939
AppSecBenchmarkUtils.SetupDummyAgent();
4040
var wafLibraryInvoker = AppSecBenchmarkUtils.CreateWafLibraryInvoker();
@@ -52,7 +52,22 @@ static AppSecWafBenchmark()
5252
{
5353
throw new ArgumentException($"Waf could not initialize, error message is: {initResult.ErrorMessage}");
5454
}
55-
Waf = initResult.Waf;
55+
_waf = initResult.Waf;
56+
57+
_stage1 = MakeRealisticNestedMapStage1(false);
58+
_stage1Attack = MakeRealisticNestedMapStage1(true);
59+
_stage2 = MakeRealisticNestedMapStage2();
60+
_stage3 = MakeRealisticNestedMapStage3();
61+
}
62+
63+
[IterationSetup]
64+
public void IterationSetup()
65+
{
66+
// Force GC to reduce variance from native memory interactions
67+
// WAF library uses unmanaged memory with allocation patterns outside .NET GC control
68+
GC.Collect();
69+
GC.WaitForPendingFinalizers();
70+
GC.Collect();
5671
}
5772

5873
private static Dictionary<string, object> MakeRealisticNestedMapStage1(bool withAttack)
@@ -132,18 +147,18 @@ private static Dictionary<string, object> MakeRealisticNestedMapStage3()
132147
[Benchmark]
133148
public void RunWafRealisticBenchmark()
134149
{
135-
var context = Waf.CreateContext();
136-
context!.Run(stage1, TimeoutMicroSeconds);
137-
context!.Run(stage2, TimeoutMicroSeconds);
138-
context!.Run(stage3, TimeoutMicroSeconds);
150+
var context = _waf.CreateContext();
151+
context!.Run(_stage1, TimeoutMicroSeconds);
152+
context!.Run(_stage2, TimeoutMicroSeconds);
153+
context!.Run(_stage3, TimeoutMicroSeconds);
139154
context.Dispose();
140155
}
141156

142157
[Benchmark]
143158
public void RunWafRealisticBenchmarkWithAttack()
144159
{
145-
var context = Waf.CreateContext();
146-
context!.Run(stage1Attack, TimeoutMicroSeconds);
160+
var context = _waf.CreateContext();
161+
context!.Run(_stage1Attack, TimeoutMicroSeconds);
147162
context.Dispose();
148163
}
149164
}

‎tracer/test/benchmarks/Benchmarks.Trace/AspNetCoreBenchmark.cs‎

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -21,9 +21,10 @@ namespace Benchmarks.Trace
2121
[BenchmarkCategory(Constants.TracerCategory)]
2222
public class AspNetCoreBenchmark
2323
{
24-
private static readonly HttpClient Client;
24+
private HttpClient _client;
2525

26-
static AspNetCoreBenchmark()
26+
[GlobalSetup]
27+
public void GlobalSetup()
2728
{
2829
var settings = TracerSettings.Create(new() { { ConfigurationKeys.StartupDiagnosticLogEnabled, false } });
2930

@@ -33,18 +34,18 @@ static AspNetCoreBenchmark()
3334
.UseStartup<Startup>();
3435

3536
var testServer = new TestServer(builder);
36-
Client = testServer.CreateClient();
37+
_client = testServer.CreateClient();
3738

3839
Datadog.Trace.ClrProfiler.Instrumentation.Initialize();
3940

40-
var bench = new AspNetCoreBenchmark();
41-
bench.SendRequest();
41+
// Warmup to initialize middleware pipeline
42+
SendRequest();
4243
}
4344

4445
[Benchmark]
4546
public string SendRequest()
4647
{
47-
return Client.GetStringAsync("/Home").GetAwaiter().GetResult();
48+
return _client.GetStringAsync("/Home").GetAwaiter().GetResult();
4849
}
4950

5051
private class Startup
@@ -96,6 +97,11 @@ namespace Benchmarks.Trace
9697
[MemoryDiagnoser]
9798
public class AspNetCoreBenchmark
9899
{
100+
[GlobalSetup]
101+
public void GlobalSetup()
102+
{
103+
}
104+
99105
[Benchmark]
100106
public string SendRequest()
101107
{

‎tracer/test/benchmarks/Benchmarks.Trace/CIVisibilityProtocolWriterBenchmark.cs‎

Lines changed: 21 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -17,21 +17,14 @@ public class CIVisibilityProtocolWriterBenchmark
1717
{
1818
private const int SpanCount = 1000;
1919

20-
private static readonly IEventWriter EventWriter;
21-
private static readonly ArraySegment<Span> EnrichedSpans;
20+
private IEventWriter _eventWriter;
21+
private ArraySegment<Span> _enrichedSpans;
2222

23-
static CIVisibilityProtocolWriterBenchmark()
23+
[GlobalSetup]
24+
public void GlobalSetup()
2425
{
25-
var overrides = new NameValueConfigurationSource(new()
26-
{
27-
{ ConfigurationKeys.StartupDiagnosticLogEnabled, false.ToString() },
28-
{ ConfigurationKeys.TraceEnabled, false.ToString() },
29-
});
30-
var sources = new CompositeConfigurationSource(new[] { overrides, GlobalConfigurationSource.Instance });
31-
var settings = new TestOptimizationSettings(sources, NullConfigurationTelemetry.Instance);
32-
33-
EventWriter = new CIVisibilityProtocolWriter(settings, new FakeCIVisibilityProtocolWriter());
34-
26+
// Create spans in GlobalSetup, not static constructor
27+
// This ensures BenchmarkDotNet excludes allocation overhead from measurements
3528
var enrichedSpans = new Span[SpanCount];
3629
var now = DateTimeOffset.UtcNow;
3730
for (var i = 0; i < SpanCount; i++)
@@ -41,10 +34,20 @@ static CIVisibilityProtocolWriterBenchmark()
4134
enrichedSpans[i].SetMetric(Metrics.SamplingRuleDecision, 1.0);
4235
}
4336

44-
EnrichedSpans = new ArraySegment<Span>(enrichedSpans);
37+
_enrichedSpans = new ArraySegment<Span>(enrichedSpans);
38+
39+
var overrides = new NameValueConfigurationSource(new()
40+
{
41+
{ ConfigurationKeys.StartupDiagnosticLogEnabled, false.ToString() },
42+
{ ConfigurationKeys.TraceEnabled, false.ToString() },
43+
});
44+
var sources = new CompositeConfigurationSource(new[] { overrides, GlobalConfigurationSource.Instance });
45+
var settings = new TestOptimizationSettings(sources, NullConfigurationTelemetry.Instance);
46+
47+
_eventWriter = new CIVisibilityProtocolWriter(settings, new FakeCIVisibilityProtocolWriter());
4548

46-
// Run benchmarks once to reduce noise
47-
new CIVisibilityProtocolWriterBenchmark().WriteAndFlushEnrichedTraces().GetAwaiter().GetResult();
49+
// Warmup to reduce noise
50+
WriteAndFlushEnrichedTraces().GetAwaiter().GetResult();
4851
}
4952

5053
/// <summary>
@@ -53,8 +56,8 @@ static CIVisibilityProtocolWriterBenchmark()
5356
[Benchmark]
5457
public Task WriteAndFlushEnrichedTraces()
5558
{
56-
EventWriter.WriteTrace(EnrichedSpans);
57-
return EventWriter.FlushTracesAsync();
59+
_eventWriter.WriteTrace(_enrichedSpans);
60+
return _eventWriter.FlushTracesAsync();
5861
}
5962

6063
private class FakeCIVisibilityProtocolWriter : ICIVisibilityProtocolWriterSender

‎tracer/test/benchmarks/Benchmarks.Trace/CharSliceBenchmark.cs‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,15 @@ namespace Benchmarks.Trace;
1515
[BenchmarkCategory(Constants.TracerCategory)]
1616
public class CharSliceBenchmark
1717
{
18+
[IterationSetup]
19+
public void Setup()
20+
{
21+
// Force GC to ensure clean state and reduce variance from GC timing
22+
GC.Collect();
23+
GC.WaitForPendingFinalizers();
24+
GC.Collect();
25+
}
26+
1827
[Benchmark]
1928
public void OriginalCharSlice()
2029
{

0 commit comments

Comments
 (0)