Make ExporterSettings properly immutable and remove ImmutableExporterSettings#6408
Conversation
Datadog ReportBranch report: ✅ 0 Failed, 458069 Passed, 2823 Skipped, 19h 11m 9.25s Total Time |
## Summary of changes - Delete legacy `IConfigurationSource` - Rename `ITelmeteredConfigurationSource` => `IConfigurationSource` - Remove some "public" API workarounds - Make some APIs `public` instead of `internal` (just to satisfy compiler) - Fix tests ## Reason for change `ITelmeteredConfigurationSource` was introduced when `IConfigurationSource` was public, and so could not be changed. We have since removed that interface from the public API (it's not in `Datadog.trace.Manual`) so now it simply adds confusion and complexity (as it should _not_ be used internally in Datadog.Trace). ## Implementation details This PR looks more complex than it is, because in order to replace all the usages of the old `IConfigurationSource` with the new one, I had to mark some other APIs public (and document them to satisfy the analyzers). It is mostly - Delete legacy `IConfigurationSource` - Rename `ITelmeteredConfigurationSource` => `IConfigurationSource` - Fix any errors I took the opportunity to also remove some of the `*Internal` methods which were used to avoid calling the "public" APIs; seeing as these aren't _actually_ public any more, that's just unnecessary duplication. ## Test coverage The testing was a pain. The `ConfigurationBuilder` tests were all designed to check that `ITelmeteredConfigurationSource` gave the same results as `IConfigurationSource` (while never _explicitly_ specifying what the "expected" behaviour was. I took some time to enumerate the _actual_ expected values for the `NameValueCollection` source, but the `Json` source has very different behaviour that is more of a pain to test, so I chose to simplify a lot there. We could do a _lot_ to clean up those tests, but I didn't want to add even more complexity in this PR. ## Other details To help "fix" the ASM tests I introduced a `DictionaryObjectConfigurationSource`, in which you pass in a `Dictionary<string, object>`. This is useful for testing (as it was previously used) but is actually going to be useful for a future clean-up PR too, so I kept it in Datadog.Trace instead of in the test project. Part of stack - #6370 - #6376 - #6385 - #6386 👈 This PR - #6397 - #6399 - #6400 - #6405 - #6408
fce01d7 to
66412f3
Compare
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing the following branches/commits: Execution-time benchmarks measure the whole time it takes to execute a program. And are intended to measure the one-off costs. Cases where the execution time results for the PR are worse than latest master results are shown in red. The following thresholds were used for comparing the execution times:
Note that these results are based on a single point-in-time result for each branch. For full results, see the dashboard. Graphs show the p99 interval based on the mean and StdDev of the test run, as well as the mean value of the run (shown as a diamond below the graph). gantt
title Execution time (ms) FakeDbCommand (.NET Framework 4.6.2)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6408) - mean (69ms) : 65, 72
. : milestone, 69,
master - mean (68ms) : 66, 71
. : milestone, 68,
section CallTarget+Inlining+NGEN
This PR (6408) - mean (976ms) : 960, 992
. : milestone, 976,
master - mean (983ms) : 953, 1013
. : milestone, 983,
gantt
title Execution time (ms) FakeDbCommand (.NET Core 3.1)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6408) - mean (108ms) : 105, 110
. : milestone, 108,
master - mean (108ms) : 106, 110
. : milestone, 108,
section CallTarget+Inlining+NGEN
This PR (6408) - mean (679ms) : 662, 697
. : milestone, 679,
master - mean (682ms) : 665, 699
. : milestone, 682,
gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6408) - mean (91ms) : 89, 93
. : milestone, 91,
master - mean (91ms) : 89, 94
. : milestone, 91,
section CallTarget+Inlining+NGEN
This PR (6408) - mean (635ms) : 617, 652
. : milestone, 635,
master - mean (639ms) : 625, 654
. : milestone, 639,
gantt
title Execution time (ms) HttpMessageHandler (.NET Framework 4.6.2)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6408) - mean (194ms) : 189, 199
. : milestone, 194,
master - mean (193ms) : 189, 197
. : milestone, 193,
section CallTarget+Inlining+NGEN
This PR (6408) - mean (1,104ms) : 1070, 1138
. : milestone, 1104,
master - mean (1,097ms) : 1071, 1124
. : milestone, 1097,
gantt
title Execution time (ms) HttpMessageHandler (.NET Core 3.1)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6408) - mean (277ms) : 272, 283
. : milestone, 277,
master - mean (278ms) : 273, 283
. : milestone, 278,
section CallTarget+Inlining+NGEN
This PR (6408) - mean (875ms) : 838, 912
. : milestone, 875,
master - mean (871ms) : 839, 903
. : milestone, 871,
gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6408) - mean (267ms) : 263, 271
. : milestone, 267,
master - mean (266ms) : 262, 270
. : milestone, 266,
section CallTarget+Inlining+NGEN
This PR (6408) - mean (856ms) : 814, 898
. : milestone, 856,
master - mean (854ms) : 824, 885
. : milestone, 854,
|
9627c71 to
91678de
Compare
Throughput/Crank Report ⚡Throughput results for AspNetCoreSimpleController comparing the following branches/commits: Cases where throughput results for the PR are worse than latest master (5% drop or greater), results are shown in red. Note that these results are based on a single point-in-time result for each branch. For full results, see one of the many, many dashboards! gantt
title Throughput Linux x64 (Total requests)
dateFormat X
axisFormat %s
section Baseline
This PR (6408) (11.083M) : 0, 11082835
master (11.199M) : 0, 11199251
benchmarks/2.9.0 (11.033M) : 0, 11032866
section Automatic
This PR (6408) (7.268M) : 0, 7267707
master (7.234M) : 0, 7234132
benchmarks/2.9.0 (7.786M) : 0, 7785853
section Trace stats
master (7.559M) : 0, 7558761
section Manual
master (11.028M) : 0, 11028498
section Manual + Automatic
This PR (6408) (6.731M) : 0, 6731365
master (6.711M) : 0, 6710751
section DD_TRACE_ENABLED=0
master (10.230M) : 0, 10230285
gantt
title Throughput Linux arm64 (Total requests)
dateFormat X
axisFormat %s
section Baseline
This PR (6408) (9.565M) : 0, 9565139
master (9.417M) : 0, 9416704
benchmarks/2.9.0 (9.495M) : 0, 9494821
section Automatic
This PR (6408) (6.419M) : 0, 6419317
master (6.437M) : 0, 6437027
section Trace stats
master (6.619M) : 0, 6618620
section Manual
master (9.205M) : 0, 9205053
section Manual + Automatic
This PR (6408) (5.947M) : 0, 5947495
master (5.852M) : 0, 5852490
section DD_TRACE_ENABLED=0
master (8.872M) : 0, 8871934
gantt
title Throughput Windows x64 (Total requests)
dateFormat X
axisFormat %s
section Baseline
This PR (6408) (10.019M) : 0, 10018864
master (9.905M) : 0, 9904680
benchmarks/2.9.0 (10.020M) : 0, 10019592
section Automatic
This PR (6408) (6.429M) : 0, 6429431
master (6.337M) : 0, 6337163
benchmarks/2.9.0 (7.255M) : 0, 7255257
section Trace stats
master (6.938M) : 0, 6938182
section Manual
master (9.708M) : 0, 9708036
section Manual + Automatic
This PR (6408) (6.135M) : 0, 6134919
master (5.665M) : 0, 5664554
section DD_TRACE_ENABLED=0
master (9.111M) : 0, 9111450
|
Benchmarks Report for tracer 🐌Benchmarks for #6408 compared to master:
The following thresholds were used for comparing the benchmark speeds:
Allocation changes below 0.5% are ignored. Benchmark detailsBenchmarks.Trace.ActivityBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.AgentWriterBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.AspNetCoreBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark - Same speed ✔️ More allocations
|
| Benchmark | Base Allocated | Diff Allocated | Change | Change % |
|---|---|---|---|---|
| Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark.WriteAndFlushEnrichedTraces‑netcoreapp3.1 | 41.69 KB | 41.98 KB | 296 B | 0.71% |
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | WriteAndFlushEnrichedTraces |
net6.0 | 565μs | 2.92μs | 13.4μs | 0.566 | 0 | 0 | 41.64 KB |
| master | WriteAndFlushEnrichedTraces |
netcoreapp3.1 | 646μs | 2.46μs | 9.2μs | 0.322 | 0 | 0 | 41.69 KB |
| master | WriteAndFlushEnrichedTraces |
net472 | 850μs | 4.06μs | 16.7μs | 8.45 | 2.53 | 0.422 | 53.28 KB |
| #6408 | WriteAndFlushEnrichedTraces |
net6.0 | 543μs | 2.67μs | 15.8μs | 0.548 | 0 | 0 | 41.56 KB |
| #6408 | WriteAndFlushEnrichedTraces |
netcoreapp3.1 | 693μs | 3.99μs | 32.7μs | 0.331 | 0 | 0 | 41.98 KB |
| #6408 | WriteAndFlushEnrichedTraces |
net472 | 842μs | 3.37μs | 13μs | 8.63 | 2.47 | 0.411 | 53.29 KB |
Benchmarks.Trace.DbCommandBenchmark - Same speed ✔️ Same allocations ✔️
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | ExecuteNonQuery |
net6.0 | 1.3μs | 1.46ns | 5.65ns | 0.0144 | 0 | 0 | 1.02 KB |
| master | ExecuteNonQuery |
netcoreapp3.1 | 1.78μs | 1.32ns | 4.94ns | 0.0132 | 0 | 0 | 1.02 KB |
| master | ExecuteNonQuery |
net472 | 2.04μs | 1.79ns | 6.93ns | 0.156 | 0.00102 | 0 | 987 B |
| #6408 | ExecuteNonQuery |
net6.0 | 1.39μs | 0.795ns | 3.08ns | 0.0139 | 0 | 0 | 1.02 KB |
| #6408 | ExecuteNonQuery |
netcoreapp3.1 | 1.82μs | 2.07ns | 8.02ns | 0.013 | 0 | 0 | 1.02 KB |
| #6408 | ExecuteNonQuery |
net472 | 2.1μs | 2.64ns | 9.87ns | 0.156 | 0.00105 | 0 | 987 B |
Benchmarks.Trace.ElasticsearchBenchmark - Same speed ✔️ Same allocations ✔️
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | CallElasticsearch |
net6.0 | 1.26μs | 0.99ns | 3.7ns | 0.0133 | 0 | 0 | 976 B |
| master | CallElasticsearch |
netcoreapp3.1 | 1.63μs | 1.56ns | 6.03ns | 0.0135 | 0 | 0 | 976 B |
| master | CallElasticsearch |
net472 | 2.6μs | 2.78ns | 10.8ns | 0.157 | 0 | 0 | 995 B |
| master | CallElasticsearchAsync |
net6.0 | 1.28μs | 0.749ns | 2.8ns | 0.0129 | 0 | 0 | 952 B |
| master | CallElasticsearchAsync |
netcoreapp3.1 | 1.72μs | 0.652ns | 2.44ns | 0.0138 | 0 | 0 | 1.02 KB |
| master | CallElasticsearchAsync |
net472 | 2.63μs | 0.87ns | 3.37ns | 0.166 | 0 | 0 | 1.05 KB |
| #6408 | CallElasticsearch |
net6.0 | 1.21μs | 2.45ns | 9.48ns | 0.0137 | 0 | 0 | 976 B |
| #6408 | CallElasticsearch |
netcoreapp3.1 | 1.61μs | 0.577ns | 2.16ns | 0.0128 | 0 | 0 | 976 B |
| #6408 | CallElasticsearch |
net472 | 2.48μs | 2.75ns | 10.6ns | 0.158 | 0 | 0 | 995 B |
| #6408 | CallElasticsearchAsync |
net6.0 | 1.38μs | 0.896ns | 3.47ns | 0.0131 | 0 | 0 | 952 B |
| #6408 | CallElasticsearchAsync |
netcoreapp3.1 | 1.6μs | 2.01ns | 7.77ns | 0.0136 | 0 | 0 | 1.02 KB |
| #6408 | CallElasticsearchAsync |
net472 | 2.6μs | 1.47ns | 5.69ns | 0.166 | 0 | 0 | 1.05 KB |
Benchmarks.Trace.GraphQLBenchmark - Same speed ✔️ Same allocations ✔️
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | ExecuteAsync |
net6.0 | 1.31μs | 0.75ns | 2.81ns | 0.0133 | 0 | 0 | 952 B |
| master | ExecuteAsync |
netcoreapp3.1 | 1.64μs | 0.677ns | 2.53ns | 0.0123 | 0 | 0 | 952 B |
| master | ExecuteAsync |
net472 | 1.81μs | 0.739ns | 2.86ns | 0.145 | 0 | 0 | 915 B |
| #6408 | ExecuteAsync |
net6.0 | 1.19μs | 0.461ns | 1.72ns | 0.0131 | 0 | 0 | 952 B |
| #6408 | ExecuteAsync |
netcoreapp3.1 | 1.69μs | 1.18ns | 4.43ns | 0.0128 | 0 | 0 | 952 B |
| #6408 | ExecuteAsync |
net472 | 1.85μs | 0.463ns | 1.79ns | 0.145 | 0 | 0 | 915 B |
Benchmarks.Trace.HttpClientBenchmark - Same speed ✔️ Same allocations ✔️
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | SendAsync |
net6.0 | 4.35μs | 1.86ns | 7.22ns | 0.0325 | 0 | 0 | 2.31 KB |
| master | SendAsync |
netcoreapp3.1 | 5.38μs | 1.99ns | 7.46ns | 0.0376 | 0 | 0 | 2.85 KB |
| master | SendAsync |
net472 | 7.28μs | 2.43ns | 9.41ns | 0.495 | 0 | 0 | 3.12 KB |
| #6408 | SendAsync |
net6.0 | 4.55μs | 10.6ns | 36.8ns | 0.0336 | 0 | 0 | 2.31 KB |
| #6408 | SendAsync |
netcoreapp3.1 | 5.33μs | 1.9ns | 7.38ns | 0.0373 | 0 | 0 | 2.85 KB |
| #6408 | SendAsync |
net472 | 7.44μs | 2.31ns | 8.94ns | 0.494 | 0 | 0 | 3.12 KB |
Benchmarks.Trace.ILoggerBenchmark - Faster 🎉 Same allocations ✔️
Faster 🎉 in #6408
Benchmark
base/diff
Base Median (ns)
Diff Median (ns)
Modality
Benchmarks.Trace.ILoggerBenchmark.EnrichedLog‑netcoreapp3.1
1.119
2,371.88
2,119.29
| Benchmark | base/diff | Base Median (ns) | Diff Median (ns) | Modality |
|---|---|---|---|---|
| Benchmarks.Trace.ILoggerBenchmark.EnrichedLog‑netcoreapp3.1 | 1.119 | 2,371.88 | 2,119.29 |
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | EnrichedLog |
net6.0 | 1.55μs | 0.629ns | 2.35ns | 0.0233 | 0 | 0 | 1.64 KB |
| master | EnrichedLog |
netcoreapp3.1 | 2.37μs | 0.939ns | 3.51ns | 0.0224 | 0 | 0 | 1.64 KB |
| master | EnrichedLog |
net472 | 2.55μs | 0.688ns | 2.57ns | 0.249 | 0 | 0 | 1.57 KB |
| #6408 | EnrichedLog |
net6.0 | 1.47μs | 0.633ns | 2.37ns | 0.0229 | 0 | 0 | 1.64 KB |
| #6408 | EnrichedLog |
netcoreapp3.1 | 2.12μs | 1.18ns | 4.25ns | 0.0222 | 0 | 0 | 1.64 KB |
| #6408 | EnrichedLog |
net472 | 2.48μs | 1.27ns | 4.76ns | 0.249 | 0 | 0 | 1.57 KB |
Benchmarks.Trace.Log4netBenchmark - Same speed ✔️ Same allocations ✔️
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | EnrichedLog |
net6.0 | 120μs | 139ns | 539ns | 0 | 0 | 0 | 4.28 KB |
| master | EnrichedLog |
netcoreapp3.1 | 124μs | 116ns | 450ns | 0.0624 | 0 | 0 | 4.28 KB |
| master | EnrichedLog |
net472 | 150μs | 136ns | 510ns | 0.676 | 0.225 | 0 | 4.46 KB |
| #6408 | EnrichedLog |
net6.0 | 118μs | 224ns | 836ns | 0 | 0 | 0 | 4.28 KB |
| #6408 | EnrichedLog |
netcoreapp3.1 | 121μs | 129ns | 483ns | 0 | 0 | 0 | 4.28 KB |
| #6408 | EnrichedLog |
net472 | 150μs | 153ns | 594ns | 0.679 | 0.226 | 0 | 4.46 KB |
Benchmarks.Trace.NLogBenchmark - Same speed ✔️ Same allocations ✔️
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | EnrichedLog |
net6.0 | 2.87μs | 0.999ns | 3.74ns | 0.0304 | 0 | 0 | 2.2 KB |
| master | EnrichedLog |
netcoreapp3.1 | 4.13μs | 2.33ns | 8.71ns | 0.0291 | 0 | 0 | 2.2 KB |
| master | EnrichedLog |
net472 | 4.85μs | 1.59ns | 6.18ns | 0.32 | 0 | 0 | 2.02 KB |
| #6408 | EnrichedLog |
net6.0 | 3.16μs | 0.898ns | 3.48ns | 0.0298 | 0 | 0 | 2.2 KB |
| #6408 | EnrichedLog |
netcoreapp3.1 | 4.13μs | 2.23ns | 8.36ns | 0.0289 | 0 | 0 | 2.2 KB |
| #6408 | EnrichedLog |
net472 | 4.87μs | 2.17ns | 8.39ns | 0.32 | 0 | 0 | 2.02 KB |
Benchmarks.Trace.RedisBenchmark - Same speed ✔️ Same allocations ✔️
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | SendReceive |
net6.0 | 1.39μs | 0.568ns | 2.12ns | 0.016 | 0 | 0 | 1.14 KB |
| master | SendReceive |
netcoreapp3.1 | 1.77μs | 0.736ns | 2.75ns | 0.0149 | 0 | 0 | 1.14 KB |
| master | SendReceive |
net472 | 2.17μs | 1.27ns | 4.93ns | 0.183 | 0 | 0 | 1.16 KB |
| #6408 | SendReceive |
net6.0 | 1.4μs | 0.784ns | 3.04ns | 0.0162 | 0 | 0 | 1.14 KB |
| #6408 | SendReceive |
netcoreapp3.1 | 1.73μs | 1.28ns | 4.96ns | 0.0156 | 0 | 0 | 1.14 KB |
| #6408 | SendReceive |
net472 | 2.08μs | 1.67ns | 6.48ns | 0.183 | 0 | 0 | 1.16 KB |
Benchmarks.Trace.SerilogBenchmark - Same speed ✔️ Same allocations ✔️
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | EnrichedLog |
net6.0 | 2.8μs | 1.98ns | 7.66ns | 0.0223 | 0 | 0 | 1.6 KB |
| master | EnrichedLog |
netcoreapp3.1 | 3.82μs | 1.33ns | 4.98ns | 0.0211 | 0 | 0 | 1.65 KB |
| master | EnrichedLog |
net472 | 4.35μs | 3.12ns | 11.7ns | 0.323 | 0 | 0 | 2.04 KB |
| #6408 | EnrichedLog |
net6.0 | 2.93μs | 1.13ns | 4.38ns | 0.022 | 0 | 0 | 1.6 KB |
| #6408 | EnrichedLog |
netcoreapp3.1 | 3.98μs | 2.42ns | 9.05ns | 0.0217 | 0 | 0 | 1.65 KB |
| #6408 | EnrichedLog |
net472 | 4.39μs | 2.41ns | 9.33ns | 0.323 | 0 | 0 | 2.04 KB |
Benchmarks.Trace.SpanBenchmark - Slower ⚠️ Same allocations ✔️
Slower ⚠️ in #6408
Benchmark
diff/base
Base Median (ns)
Diff Median (ns)
Modality
Benchmarks.Trace.SpanBenchmark.StartFinishSpan‑net472
1.240
580.84
720.23
| Benchmark | diff/base | Base Median (ns) | Diff Median (ns) | Modality |
|---|---|---|---|---|
| Benchmarks.Trace.SpanBenchmark.StartFinishSpan‑net472 | 1.240 | 580.84 | 720.23 |
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | StartFinishSpan |
net6.0 | 399ns | 0.275ns | 1.06ns | 0.00803 | 0 | 0 | 576 B |
| master | StartFinishSpan |
netcoreapp3.1 | 627ns | 0.378ns | 1.46ns | 0.00777 | 0 | 0 | 576 B |
| master | StartFinishSpan |
net472 | 580ns | 0.32ns | 1.24ns | 0.0915 | 0 | 0 | 578 B |
| master | StartFinishScope |
net6.0 | 493ns | 0.228ns | 0.854ns | 0.00967 | 0 | 0 | 696 B |
| master | StartFinishScope |
netcoreapp3.1 | 759ns | 0.389ns | 1.46ns | 0.00938 | 0 | 0 | 696 B |
| master | StartFinishScope |
net472 | 962ns | 1.17ns | 4.54ns | 0.104 | 0 | 0 | 658 B |
| #6408 | StartFinishSpan |
net6.0 | 402ns | 0.22ns | 0.824ns | 0.008 | 0 | 0 | 576 B |
| #6408 | StartFinishSpan |
netcoreapp3.1 | 595ns | 0.429ns | 1.66ns | 0.00751 | 0 | 0 | 576 B |
| #6408 | StartFinishSpan |
net472 | 720ns | 0.377ns | 1.46ns | 0.0916 | 0 | 0 | 578 B |
| #6408 | StartFinishScope |
net6.0 | 479ns | 0.286ns | 1.11ns | 0.00979 | 0 | 0 | 696 B |
| #6408 | StartFinishScope |
netcoreapp3.1 | 705ns | 0.713ns | 2.76ns | 0.00951 | 0 | 0 | 696 B |
| #6408 | StartFinishScope |
net472 | 898ns | 1.04ns | 4.03ns | 0.104 | 0 | 0 | 658 B |
Benchmarks.Trace.TraceAnnotationsBenchmark - Faster 🎉 Same allocations ✔️
Faster 🎉 in #6408
Benchmark
base/diff
Base Median (ns)
Diff Median (ns)
Modality
Benchmarks.Trace.TraceAnnotationsBenchmark.RunOnMethodBegin‑net6.0
1.129
672.21
595.58
| Benchmark | base/diff | Base Median (ns) | Diff Median (ns) | Modality |
|---|---|---|---|---|
| Benchmarks.Trace.TraceAnnotationsBenchmark.RunOnMethodBegin‑net6.0 | 1.129 | 672.21 | 595.58 |
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | RunOnMethodBegin |
net6.0 | 672ns | 0.399ns | 1.55ns | 0.00972 | 0 | 0 | 696 B |
| master | RunOnMethodBegin |
netcoreapp3.1 | 950ns | 0.649ns | 2.43ns | 0.00899 | 0 | 0 | 696 B |
| master | RunOnMethodBegin |
net472 | 1.16μs | 0.575ns | 2.23ns | 0.104 | 0 | 0 | 658 B |
| #6408 | RunOnMethodBegin |
net6.0 | 596ns | 0.401ns | 1.55ns | 0.00959 | 0 | 0 | 696 B |
| #6408 | RunOnMethodBegin |
netcoreapp3.1 | 928ns | 0.413ns | 1.6ns | 0.00928 | 0 | 0 | 696 B |
| #6408 | RunOnMethodBegin |
net472 | 1.09μs | 0.529ns | 2.05ns | 0.104 | 0 | 0 | 658 B |
…nSource` (#6397) Change how we "apply" settings from manual configuration to the automatic side to use `IConfigurationSource` ## Reason for change The previous design of how we "apply" settings made in code using manual instrumentation required mutating a `TracerSettings` object after it was already built. In fact, this is pretty much the _only_ place that we mutate the settings. By switching to using a "configuration source" approach, so that the settings are built _once_ with the correct values opens up the option to make these immutable (and therefore delete all of the `Immutable*` settings we currently have. This reduces both code duplication and work we do on startup, and opens the path to further refactoring improvements. Note that the public API does not change, so consumers of Datadog.Trace.Manual are still working with a mutable object initially. ## Implementation details Currently we pass a `Dictionary<string, object>` between the manual and automatic side. Previously, we then iterated the keys, compared against known values, and modified `TracerSettings` as required. With the changes, we have a `ManualInstrumentationConfigurationSource` which just "wraps" the `Dictionary<>`, and returns the correct value as required for a given key. Overall, I think this is much cleaner. Where things get messy is how we handle disabling specific integrations. The existing dictionary is optimised for looping through the provided values, fetching the setting that needs to be modified, and changing all the required properties. Unfortunately, the `IConfigurationSource` approach where we're looking up by a key like `DD_TRACE_NPGSQL_ENABLED` works _horribly_ for this pattern 🙁. So I introduced an additional approach which explicitly _additionally_ transfers the settings using these values, making them just "standard" lookups. > Note that due to backwards + forwards compatibility requirements > - We _still_ need to send the "old" version of integration settings from the manual side, in case it's used with an old version of the auto instrumentation > - We _still_ need to handle the "old" version of integration settings in the auto side, in case it's used with an old version of the manual instrumentation. > - At least in this case we can use the more efficient `IConfigurationSource` reader, so we don't pay the expense of retrieving the settings. The only downside is a couple of extra allocations when they _do_ disable integrations in code. Minor other changes: - Add helper ctor to `CompositeConfigurationSource` for creating the internal list from a collection of `IConfigurationSource` - Tweak `DictionaryObjectConfigurationSource` so we can derive from it - Create a separate integration for <3.7.0 manual instrumentation that uses the legacy settings, otherwise use the new settings objects ## Test coverage Mostly covered by existing unit tests (and indirectly by integration tests). Tweaked the test to test both the new and legacy configuration source. ## Other details Requires #6393 to fix how we read integration settings Part of stack - #6370 - #6376 - #6385 - #6386 - #6397 👈 This PR - #6399 - #6400 - #6405 - #6408
66412f3 to
5002f8b
Compare
91678de to
c4cd7f8
Compare
…tead of mutating (#6399) ## Summary of changes Change how CI Visibility creates its `TracerSettings` object to avoid mutating settings afterwards ## Reason for change This stack of changes is about removing duplication (among other things) in the `TracerSettings` etc related objects. This is _partially_ required in order to remove the "snapshot generator" complexity that was removed in #6370. Given `TracerSettings` are not exposed publicly in Datadog.Trace, we want to move to doing a "one shot" configuring of them, ultimately so that we can make the object immutable (and remove `ImmutableTracerSettings` and friends). ## Implementation details CI Visibility is one of the few places where we mutate settings _after_ creating the `TracerSettings`. This is mostly because there's additional logic that CI Visibility wants to perform. Ultimately, the "solution" to that issue in this PR is to move that logic into the `TracerSettings` constructor. I'm not entirely happy about it, but it's the only approach I could find that works. - Add a "dummy" configuration value to a configuration source when creating the `TracerSettings`. This is used purely as a "switch" to say "we're in CI Visibility mode". - We absolutely could just pass this in as a constructor parameter. I went that route first and then backed away, but can totally be swayed. - Add an additional configuration source to update settings that we want to change in CI Vis (e.g. logs injection). - In the `TracerSettings` ctor, add an additional ignored URL when in CI Vis mode, modify the default service name (if required) and add an additional GlobalTag. ## Test coverage I'd love to have some, but the CI Visibility configuration is kinda all over the place, so if you have any pointers @tonyredondo I'm all ears... ## Other details Part of stack - #6370 - #6376 - #6385 - #6386 - #6397 - #6399 👈 This PR - #6400 - #6405 - #6408
|
|
||
| // internal for testing | ||
| internal static ExporterSettings Create(Dictionary<string, object?> settings) | ||
| => new(new DictionaryConfigurationSource(settings.ToDictionary(x => x.Key, x => x.Value?.ToString()!)), new ConfigurationTelemetry()); |
There was a problem hiding this comment.
x.Value?.ToString()!
:stare:
There was a problem hiding this comment.
Yeah, the trouble is the DictionaryConfigurationSource needs a <string, string> sooooo 😅 We should merge DictionaryConfigurationSource and DictionaryObjectConfigurationSource tbh, but separate PR...
Given this is only for testing anyway, meh 😉
…utableDirectLogSubmissionSettings` (#6400) ## Summary of changes Merge `DirectLogSubmissionSettings` with `ImmutableDirectLogSubmissionSettings` ## Reason for change There was never really a good reason for having these as separate types. It was primarily to make testing a little easier and to mirror the `TracerSettings`/`ImmutableTracerSettings` dichotomy, but as that's going away, this is just unnecessary complexity ## Implementation details - Moved the additional logic that was previously inside `ImmutableDirectLogSubmissionSettings` into `DirectLogSubmissionSettings` - Renamed the `DirectLogSubmissionSettings` properties to match the "Immutable" version (and remove the unnecessary prefix) - Replace all usages of `Immutable*` with `DirectLogSubmissionSettings` - Move `Immutable*Tests` into appropriate file and tweak - Replace mutations with initialization (using `IConfigurationSource`) ## Test coverage Essentially the same. I removed/tweaked some tests that are no longer relevant ## Other details Part of stack - #6370 - #6376 - #6385 - #6386 - #6397 - #6399 - #6400 👈 This PR - #6405 - #6408 - #6415
5002f8b to
92bba79
Compare
c039b45 to
5b80296
Compare
…tegrationSettings` (#6405) ## Summary of changes Merge `IntegrationSettings` with `ImmutableIntegrationSettings` ## Reason for change This stack of PRs is about doing one-shot configuration instead of mutation. We never mutate these in the tracer after creation, so there's no need for the separate types. ## Implementation details - Moved additional logic (handling `DisabledNames` that was previously inside `ImmutableIntegrationSettings` into `IntegrationSettings` - Replace all usages of `Immutable*` with `IntegrationSettings` - Move `Immutable*Tests` into appropriate file and tweak - Replace mutations with initialization - Reorder initialization of `DisabledIntegrationNames` in `TracerSettings` so that it can be used in the `IntegrationSettings` constructor ## Test coverage All covered by existing details ## Other details Part of Stack - #6370 - #6376 - #6385 - #6386 - #6397 - #6399 - #6400 - #6405 👈 This PR - #6408 - #6415
Minimal refactoring, mostly just making properties readonly and making functions "more pure".
5b80296 to
ced5042
Compare
…ettings` (#6415) ## Summary of changes Merge `TracerSettings` with `ImmutableTracerSettings` ## Reason for change This stack of PRs is about doing one-shot configuration instead of mutation. We never mutate these in the tracer after creation, so there's no need for the separate types. ## Implementation details - Make the properties in `TracerSettings` get-only. - Make the collections in `TracerSettings` readonly. - Move logic that used to be in the constructor of `ImmutableTracerSettings` into `TracerSettings` - e.g. Service/Version/Env were being changed based on DD_TAGS values. Moved that to TracerSettings and (importantly) added missing telemetry recording of these values. - Added missing recording of _effective_ `DisabledInstegrations` - Moving this logic caused some _tests_ to be broken (checking default values). Updated the expected values of those tests in a single - Replace all usages of `ImmutableTracerSettings` with `TracerSettings` - Move `ITracer` to Datadog.Trace.Manual - It's only used there, and references the manual-version of `ImmutableTracerSettings` which we _want_ to keep. - Move `Immutable*Tests` into appropriate file and tweak - Replace mutations with initialization in tests ## Test coverage All covered by existing tests (I hope) 🤞 ## Other details There's still a _lot_ of scope to improve this Part of Stack - #6370 - #6376 - #6385 - #6386 - #6397 - #6399 - #6400 - #6405 - #6408 - #6415 👈 This PR
## Summary of changes - Delete legacy `IConfigurationSource` - Rename `ITelmeteredConfigurationSource` => `IConfigurationSource` - Remove some "public" API workarounds - Make some APIs `public` instead of `internal` (just to satisfy compiler) - Fix tests ## Reason for change `ITelmeteredConfigurationSource` was introduced when `IConfigurationSource` was public, and so could not be changed. We have since removed that interface from the public API (it's not in `Datadog.trace.Manual`) so now it simply adds confusion and complexity (as it should _not_ be used internally in Datadog.Trace). ## Implementation details This PR looks more complex than it is, because in order to replace all the usages of the old `IConfigurationSource` with the new one, I had to mark some other APIs public (and document them to satisfy the analyzers). It is mostly - Delete legacy `IConfigurationSource` - Rename `ITelmeteredConfigurationSource` => `IConfigurationSource` - Fix any errors I took the opportunity to also remove some of the `*Internal` methods which were used to avoid calling the "public" APIs; seeing as these aren't _actually_ public any more, that's just unnecessary duplication. ## Test coverage The testing was a pain. The `ConfigurationBuilder` tests were all designed to check that `ITelmeteredConfigurationSource` gave the same results as `IConfigurationSource` (while never _explicitly_ specifying what the "expected" behaviour was. I took some time to enumerate the _actual_ expected values for the `NameValueCollection` source, but the `Json` source has very different behaviour that is more of a pain to test, so I chose to simplify a lot there. We could do a _lot_ to clean up those tests, but I didn't want to add even more complexity in this PR. ## Other details To help "fix" the ASM tests I introduced a `DictionaryObjectConfigurationSource`, in which you pass in a `Dictionary<string, object>`. This is useful for testing (as it was previously used) but is actually going to be useful for a future clean-up PR too, so I kept it in Datadog.Trace instead of in the test project. Part of stack - #6370 - #6376 - #6385 - #6386 👈 This PR - #6397 - #6399 - #6400 - #6405 - #6408
…nSource` (#6397) Change how we "apply" settings from manual configuration to the automatic side to use `IConfigurationSource` ## Reason for change The previous design of how we "apply" settings made in code using manual instrumentation required mutating a `TracerSettings` object after it was already built. In fact, this is pretty much the _only_ place that we mutate the settings. By switching to using a "configuration source" approach, so that the settings are built _once_ with the correct values opens up the option to make these immutable (and therefore delete all of the `Immutable*` settings we currently have. This reduces both code duplication and work we do on startup, and opens the path to further refactoring improvements. Note that the public API does not change, so consumers of Datadog.Trace.Manual are still working with a mutable object initially. ## Implementation details Currently we pass a `Dictionary<string, object>` between the manual and automatic side. Previously, we then iterated the keys, compared against known values, and modified `TracerSettings` as required. With the changes, we have a `ManualInstrumentationConfigurationSource` which just "wraps" the `Dictionary<>`, and returns the correct value as required for a given key. Overall, I think this is much cleaner. Where things get messy is how we handle disabling specific integrations. The existing dictionary is optimised for looping through the provided values, fetching the setting that needs to be modified, and changing all the required properties. Unfortunately, the `IConfigurationSource` approach where we're looking up by a key like `DD_TRACE_NPGSQL_ENABLED` works _horribly_ for this pattern 🙁. So I introduced an additional approach which explicitly _additionally_ transfers the settings using these values, making them just "standard" lookups. > Note that due to backwards + forwards compatibility requirements > - We _still_ need to send the "old" version of integration settings from the manual side, in case it's used with an old version of the auto instrumentation > - We _still_ need to handle the "old" version of integration settings in the auto side, in case it's used with an old version of the manual instrumentation. > - At least in this case we can use the more efficient `IConfigurationSource` reader, so we don't pay the expense of retrieving the settings. The only downside is a couple of extra allocations when they _do_ disable integrations in code. Minor other changes: - Add helper ctor to `CompositeConfigurationSource` for creating the internal list from a collection of `IConfigurationSource` - Tweak `DictionaryObjectConfigurationSource` so we can derive from it - Create a separate integration for <3.7.0 manual instrumentation that uses the legacy settings, otherwise use the new settings objects ## Test coverage Mostly covered by existing unit tests (and indirectly by integration tests). Tweaked the test to test both the new and legacy configuration source. ## Other details Requires #6393 to fix how we read integration settings Part of stack - #6370 - #6376 - #6385 - #6386 - #6397 👈 This PR - #6399 - #6400 - #6405 - #6408
…tead of mutating (#6399) ## Summary of changes Change how CI Visibility creates its `TracerSettings` object to avoid mutating settings afterwards ## Reason for change This stack of changes is about removing duplication (among other things) in the `TracerSettings` etc related objects. This is _partially_ required in order to remove the "snapshot generator" complexity that was removed in #6370. Given `TracerSettings` are not exposed publicly in Datadog.Trace, we want to move to doing a "one shot" configuring of them, ultimately so that we can make the object immutable (and remove `ImmutableTracerSettings` and friends). ## Implementation details CI Visibility is one of the few places where we mutate settings _after_ creating the `TracerSettings`. This is mostly because there's additional logic that CI Visibility wants to perform. Ultimately, the "solution" to that issue in this PR is to move that logic into the `TracerSettings` constructor. I'm not entirely happy about it, but it's the only approach I could find that works. - Add a "dummy" configuration value to a configuration source when creating the `TracerSettings`. This is used purely as a "switch" to say "we're in CI Visibility mode". - We absolutely could just pass this in as a constructor parameter. I went that route first and then backed away, but can totally be swayed. - Add an additional configuration source to update settings that we want to change in CI Vis (e.g. logs injection). - In the `TracerSettings` ctor, add an additional ignored URL when in CI Vis mode, modify the default service name (if required) and add an additional GlobalTag. ## Test coverage I'd love to have some, but the CI Visibility configuration is kinda all over the place, so if you have any pointers @tonyredondo I'm all ears... ## Other details Part of stack - #6370 - #6376 - #6385 - #6386 - #6397 - #6399 👈 This PR - #6400 - #6405 - #6408
…utableDirectLogSubmissionSettings` (#6400) ## Summary of changes Merge `DirectLogSubmissionSettings` with `ImmutableDirectLogSubmissionSettings` ## Reason for change There was never really a good reason for having these as separate types. It was primarily to make testing a little easier and to mirror the `TracerSettings`/`ImmutableTracerSettings` dichotomy, but as that's going away, this is just unnecessary complexity ## Implementation details - Moved the additional logic that was previously inside `ImmutableDirectLogSubmissionSettings` into `DirectLogSubmissionSettings` - Renamed the `DirectLogSubmissionSettings` properties to match the "Immutable" version (and remove the unnecessary prefix) - Replace all usages of `Immutable*` with `DirectLogSubmissionSettings` - Move `Immutable*Tests` into appropriate file and tweak - Replace mutations with initialization (using `IConfigurationSource`) ## Test coverage Essentially the same. I removed/tweaked some tests that are no longer relevant ## Other details Part of stack - #6370 - #6376 - #6385 - #6386 - #6397 - #6399 - #6400 👈 This PR - #6405 - #6408 - #6415
…tegrationSettings` (#6405) ## Summary of changes Merge `IntegrationSettings` with `ImmutableIntegrationSettings` ## Reason for change This stack of PRs is about doing one-shot configuration instead of mutation. We never mutate these in the tracer after creation, so there's no need for the separate types. ## Implementation details - Moved additional logic (handling `DisabledNames` that was previously inside `ImmutableIntegrationSettings` into `IntegrationSettings` - Replace all usages of `Immutable*` with `IntegrationSettings` - Move `Immutable*Tests` into appropriate file and tweak - Replace mutations with initialization - Reorder initialization of `DisabledIntegrationNames` in `TracerSettings` so that it can be used in the `IntegrationSettings` constructor ## Test coverage All covered by existing details ## Other details Part of Stack - #6370 - #6376 - #6385 - #6386 - #6397 - #6399 - #6400 - #6405 👈 This PR - #6408 - #6415
…terSettings` (#6408) ## Summary of changes Merge `ExporterSettings` with `ImmutableExporterSettings` ## Reason for change This stack of PRs is about doing one-shot configuration instead of mutation. We never mutate these in the tracer after creation, so there's no need for the separate types. ## Implementation details - Made the properties in `ExporterSettings` get-only. This required quite a lot of work because we were doing a lot of mutating of the settings in the "helper" functions. - I only _lightly_ refactored those methods (as much as possible) to avoid setting the properties in the functions and instead returning the details to set later. - These are prime candidates for some _much_ heavier refactoring later, but I didn't want to get bogged down with that in this PR - Replace all usages of `Immutable*` with `ExporterSettings` - Replace usages of `AgentUriInternal` with `AgentUri` - Move `Immutable*Tests` into appropriate file and tweak - Replace mutations with initialization ## Test coverage All covered by existing details ## Other details Part of Stack - #6370 - #6376 - #6385 - #6386 - #6397 - #6399 - #6400 - #6405 - #6408 👈 This PR - #6415
…ettings` (#6415) Merge `TracerSettings` with `ImmutableTracerSettings` This stack of PRs is about doing one-shot configuration instead of mutation. We never mutate these in the tracer after creation, so there's no need for the separate types. - Make the properties in `TracerSettings` get-only. - Make the collections in `TracerSettings` readonly. - Move logic that used to be in the constructor of `ImmutableTracerSettings` into `TracerSettings` - e.g. Service/Version/Env were being changed based on DD_TAGS values. Moved that to TracerSettings and (importantly) added missing telemetry recording of these values. - Added missing recording of _effective_ `DisabledInstegrations` - Moving this logic caused some _tests_ to be broken (checking default values). Updated the expected values of those tests in a single - Replace all usages of `ImmutableTracerSettings` with `TracerSettings` - Move `ITracer` to Datadog.Trace.Manual - It's only used there, and references the manual-version of `ImmutableTracerSettings` which we _want_ to keep. - Move `Immutable*Tests` into appropriate file and tweak - Replace mutations with initialization in tests All covered by existing tests (I hope) 🤞 There's still a _lot_ of scope to improve this Part of Stack - #6370 - #6376 - #6385 - #6386 - #6397 - #6399 - #6400 - #6405 - #6408 - #6415 👈 This PR
Summary of changes
Merge
ExporterSettingswithImmutableExporterSettingsReason for change
This stack of PRs is about doing one-shot configuration instead of mutation. We never mutate these in the tracer after creation, so there's no need for the separate types.
Implementation details
ExporterSettingsget-only. This required quite a lot of work because we were doing a lot of mutating of the settings in the "helper" functions.Immutable*withExporterSettingsAgentUriInternalwithAgentUriImmutable*Testsinto appropriate file and tweakTest coverage
All covered by existing details
Other details
Part of Stack
PublicApiTests.PublicApiHasNotChangedfor Datadog.Trace.dll #6385IConfigurationSource#6386IConfigurationSource#6397TracerSettingsusing a configuration source instead of mutating #6399DirectLogSubmissionSettingsproperly immutable and removeImmutableDirectLogSubmissionSettings#6400IntegrationSettingsproperly immutable and removeImmutableIntegrationSettings#6405ExporterSettingsproperly immutable and removeImmutableExporterSettings#6408 👈 This PRTracerSettingsproperly immutable and removeImmutableTracerSettings#6415