-
Notifications
You must be signed in to change notification settings - Fork 145
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make more integration settings case insensitive #6393
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Datadog ReportBranch report: ✅ 0 Failed, 450133 Passed, 2763 Skipped, 20h 17m 23.8s Total Time New Flaky Tests (2)
|
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 (6393) - mean (69ms) : 66, 72
. : milestone, 69,
master - mean (69ms) : 66, 72
. : milestone, 69,
section CallTarget+Inlining+NGEN
This PR (6393) - mean (981ms) : 954, 1008
. : milestone, 981,
master - mean (977ms) : 957, 996
. : milestone, 977,
gantt
title Execution time (ms) FakeDbCommand (.NET Core 3.1)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6393) - mean (108ms) : 105, 111
. : milestone, 108,
master - mean (108ms) : 106, 110
. : milestone, 108,
section CallTarget+Inlining+NGEN
This PR (6393) - mean (680ms) : 664, 696
. : milestone, 680,
master - mean (677ms) : 661, 692
. : milestone, 677,
gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6393) - mean (92ms) : 89, 94
. : milestone, 92,
master - mean (91ms) : 89, 94
. : milestone, 91,
section CallTarget+Inlining+NGEN
This PR (6393) - mean (634ms) : 618, 650
. : milestone, 634,
master - mean (634ms) : 616, 652
. : milestone, 634,
gantt
title Execution time (ms) HttpMessageHandler (.NET Framework 4.6.2)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6393) - mean (190ms) : 187, 194
. : milestone, 190,
master - mean (190ms) : 186, 195
. : milestone, 190,
section CallTarget+Inlining+NGEN
This PR (6393) - mean (1,094ms) : 1057, 1131
. : milestone, 1094,
master - mean (1,090ms) : 1059, 1121
. : milestone, 1090,
gantt
title Execution time (ms) HttpMessageHandler (.NET Core 3.1)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6393) - mean (277ms) : 273, 281
. : milestone, 277,
master - mean (277ms) : 273, 281
. : milestone, 277,
section CallTarget+Inlining+NGEN
This PR (6393) - mean (874ms) : 838, 909
. : milestone, 874,
master - mean (870ms) : 839, 901
. : milestone, 870,
gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6393) - mean (266ms) : 261, 271
. : milestone, 266,
master - mean (266ms) : 260, 271
. : milestone, 266,
section CallTarget+Inlining+NGEN
This PR (6393) - mean (850ms) : 813, 887
. : milestone, 850,
master - mean (849ms) : 812, 887
. : milestone, 849,
|
Benchmarks Report for tracer 🐌Benchmarks for #6393 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 ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.DbCommandBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.ElasticsearchBenchmark - Slower
|
Benchmark | diff/base | Base Median (ns) | Diff Median (ns) | Modality |
---|---|---|---|---|
Benchmarks.Trace.ElasticsearchBenchmark.CallElasticsearch‑net6.0 | 1.186 | 1,096.31 | 1,299.70 |
Raw results
Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
---|---|---|---|---|---|---|---|---|---|
master | CallElasticsearch |
net6.0 | 1.1μs | 0.635ns | 2.38ns | 0.0137 | 0 | 0 | 976 B |
master | CallElasticsearch |
netcoreapp3.1 | 1.62μs | 0.584ns | 2.02ns | 0.013 | 0 | 0 | 976 B |
master | CallElasticsearch |
net472 | 2.62μs | 1.98ns | 7.66ns | 0.158 | 0 | 0 | 995 B |
master | CallElasticsearchAsync |
net6.0 | 1.21μs | 0.559ns | 2.16ns | 0.0133 | 0 | 0 | 952 B |
master | CallElasticsearchAsync |
netcoreapp3.1 | 1.64μs | 1.27ns | 4.56ns | 0.0139 | 0 | 0 | 1.02 KB |
master | CallElasticsearchAsync |
net472 | 2.59μs | 0.914ns | 3.3ns | 0.167 | 0 | 0 | 1.05 KB |
#6393 | CallElasticsearch |
net6.0 | 1.3μs | 0.615ns | 2.3ns | 0.0136 | 0 | 0 | 976 B |
#6393 | CallElasticsearch |
netcoreapp3.1 | 1.62μs | 7.43ns | 28.8ns | 0.0132 | 0 | 0 | 976 B |
#6393 | CallElasticsearch |
net472 | 2.5μs | 1.13ns | 4.24ns | 0.157 | 0 | 0 | 995 B |
#6393 | CallElasticsearchAsync |
net6.0 | 1.34μs | 1.68ns | 6.28ns | 0.013 | 0 | 0 | 952 B |
#6393 | CallElasticsearchAsync |
netcoreapp3.1 | 1.64μs | 0.96ns | 3.59ns | 0.0139 | 0 | 0 | 1.02 KB |
#6393 | CallElasticsearchAsync |
net472 | 2.62μs | 0.715ns | 2.68ns | 0.167 | 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 | 1.13ns | 4.36ns | 0.0137 | 0 | 0 | 952 B |
master | ExecuteAsync |
netcoreapp3.1 | 1.65μs | 0.829ns | 2.99ns | 0.0123 | 0 | 0 | 952 B |
master | ExecuteAsync |
net472 | 1.8μs | 0.627ns | 2.43ns | 0.145 | 0 | 0 | 915 B |
#6393 | ExecuteAsync |
net6.0 | 1.21μs | 1.12ns | 4.19ns | 0.0133 | 0 | 0 | 952 B |
#6393 | ExecuteAsync |
netcoreapp3.1 | 1.68μs | 0.622ns | 2.33ns | 0.0127 | 0 | 0 | 952 B |
#6393 | ExecuteAsync |
net472 | 1.79μs | 0.697ns | 2.61ns | 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.52μs | 10.4ns | 40.1ns | 0.0314 | 0 | 0 | 2.31 KB |
master | SendAsync |
netcoreapp3.1 | 5.34μs | 1.79ns | 6.46ns | 0.0373 | 0 | 0 | 2.85 KB |
master | SendAsync |
net472 | 7.35μs | 2.71ns | 10.5ns | 0.494 | 0 | 0 | 3.12 KB |
#6393 | SendAsync |
net6.0 | 4.39μs | 1.33ns | 4.97ns | 0.0305 | 0 | 0 | 2.31 KB |
#6393 | SendAsync |
netcoreapp3.1 | 5.36μs | 1.65ns | 6.4ns | 0.0378 | 0 | 0 | 2.85 KB |
#6393 | SendAsync |
net472 | 7.39μs | 1.98ns | 7.43ns | 0.495 | 0 | 0 | 3.12 KB |
Benchmarks.Trace.ILoggerBenchmark - Same speed ✔️ Same allocations ✔️
Raw results
Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
---|---|---|---|---|---|---|---|---|---|
master | EnrichedLog |
net6.0 | 1.51μs | 3.83ns | 14.8ns | 0.0227 | 0 | 0 | 1.64 KB |
master | EnrichedLog |
netcoreapp3.1 | 2.25μs | 0.994ns | 3.72ns | 0.0226 | 0 | 0 | 1.64 KB |
master | EnrichedLog |
net472 | 2.61μs | 0.894ns | 3.35ns | 0.249 | 0 | 0 | 1.57 KB |
#6393 | EnrichedLog |
net6.0 | 1.61μs | 1.14ns | 4.41ns | 0.0227 | 0 | 0 | 1.64 KB |
#6393 | EnrichedLog |
netcoreapp3.1 | 2.15μs | 1.36ns | 4.92ns | 0.0226 | 0 | 0 | 1.64 KB |
#6393 | EnrichedLog |
net472 | 2.49μs | 1.8ns | 6.74ns | 0.25 | 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 | 118μs | 209ns | 811ns | 0.0595 | 0 | 0 | 4.28 KB |
master | EnrichedLog |
netcoreapp3.1 | 125μs | 154ns | 598ns | 0 | 0 | 0 | 4.28 KB |
master | EnrichedLog |
net472 | 153μs | 164ns | 637ns | 0.689 | 0.23 | 0 | 4.46 KB |
#6393 | EnrichedLog |
net6.0 | 120μs | 194ns | 751ns | 0.0608 | 0 | 0 | 4.28 KB |
#6393 | EnrichedLog |
netcoreapp3.1 | 125μs | 181ns | 700ns | 0 | 0 | 0 | 4.28 KB |
#6393 | EnrichedLog |
net472 | 151μs | 282ns | 1.09μs | 0.678 | 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 | 3.14μs | 1.54ns | 5.98ns | 0.0309 | 0 | 0 | 2.2 KB |
master | EnrichedLog |
netcoreapp3.1 | 4.26μs | 1.48ns | 5.75ns | 0.0296 | 0 | 0 | 2.2 KB |
master | EnrichedLog |
net472 | 4.78μs | 2.13ns | 8.23ns | 0.319 | 0 | 0 | 2.02 KB |
#6393 | EnrichedLog |
net6.0 | 3.04μs | 0.91ns | 3.52ns | 0.0301 | 0 | 0 | 2.2 KB |
#6393 | EnrichedLog |
netcoreapp3.1 | 4.19μs | 1.55ns | 5.58ns | 0.0277 | 0 | 0 | 2.2 KB |
#6393 | EnrichedLog |
net472 | 5.07μs | 1.15ns | 4.45ns | 0.318 | 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.32μs | 1.64ns | 6.12ns | 0.0162 | 0 | 0 | 1.14 KB |
master | SendReceive |
netcoreapp3.1 | 1.81μs | 0.814ns | 3.15ns | 0.0154 | 0 | 0 | 1.14 KB |
master | SendReceive |
net472 | 2.06μs | 1.1ns | 3.96ns | 0.183 | 0 | 0 | 1.16 KB |
#6393 | SendReceive |
net6.0 | 1.4μs | 0.738ns | 2.76ns | 0.016 | 0 | 0 | 1.14 KB |
#6393 | SendReceive |
netcoreapp3.1 | 1.76μs | 1.23ns | 4.76ns | 0.0151 | 0 | 0 | 1.14 KB |
#6393 | SendReceive |
net472 | 2.06μs | 1.97ns | 7.37ns | 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.75μs | 1.63ns | 6.1ns | 0.0221 | 0 | 0 | 1.6 KB |
master | EnrichedLog |
netcoreapp3.1 | 3.98μs | 1.86ns | 7.22ns | 0.0217 | 0 | 0 | 1.65 KB |
master | EnrichedLog |
net472 | 4.45μs | 2.51ns | 9.74ns | 0.323 | 0 | 0 | 2.04 KB |
#6393 | EnrichedLog |
net6.0 | 2.84μs | 0.884ns | 3.42ns | 0.0228 | 0 | 0 | 1.6 KB |
#6393 | EnrichedLog |
netcoreapp3.1 | 3.85μs | 3.9ns | 15.1ns | 0.021 | 0 | 0 | 1.65 KB |
#6393 | EnrichedLog |
net472 | 4.49μs | 2.22ns | 8.3ns | 0.322 | 0 | 0 | 2.04 KB |
Benchmarks.Trace.SpanBenchmark - Slower ⚠️ Same allocations ✔️
Slower ⚠️ in #6393
Benchmark
diff/base
Base Median (ns)
Diff Median (ns)
Modality
Benchmarks.Trace.SpanBenchmark.StartFinishScope‑net472
1.154
819.52
945.35
Benchmark | diff/base | Base Median (ns) | Diff Median (ns) | Modality |
---|---|---|---|---|
Benchmarks.Trace.SpanBenchmark.StartFinishScope‑net472 | 1.154 | 819.52 | 945.35 |
Raw results
Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
---|---|---|---|---|---|---|---|---|---|
master | StartFinishSpan |
net6.0 | 421ns | 0.693ns | 2.68ns | 0.00816 | 0 | 0 | 576 B |
master | StartFinishSpan |
netcoreapp3.1 | 643ns | 1.54ns | 5.95ns | 0.00777 | 0 | 0 | 576 B |
master | StartFinishSpan |
net472 | 669ns | 1.34ns | 5.21ns | 0.0915 | 0 | 0 | 578 B |
master | StartFinishScope |
net6.0 | 488ns | 0.756ns | 2.93ns | 0.00976 | 0 | 0 | 696 B |
master | StartFinishScope |
netcoreapp3.1 | 737ns | 0.902ns | 3.37ns | 0.00955 | 0 | 0 | 696 B |
master | StartFinishScope |
net472 | 819ns | 1.43ns | 5.55ns | 0.105 | 0 | 0 | 658 B |
#6393 | StartFinishSpan |
net6.0 | 395ns | 0.613ns | 2.37ns | 0.00806 | 0 | 0 | 576 B |
#6393 | StartFinishSpan |
netcoreapp3.1 | 631ns | 0.784ns | 3.04ns | 0.00788 | 0 | 0 | 576 B |
#6393 | StartFinishSpan |
net472 | 634ns | 1.46ns | 5.67ns | 0.0916 | 0 | 0 | 578 B |
#6393 | StartFinishScope |
net6.0 | 534ns | 0.876ns | 3.39ns | 0.00971 | 0 | 0 | 696 B |
#6393 | StartFinishScope |
netcoreapp3.1 | 695ns | 1.37ns | 5.29ns | 0.00946 | 0 | 0 | 696 B |
#6393 | StartFinishScope |
net472 | 942ns | 2.85ns | 11.1ns | 0.104 | 0 | 0 | 658 B |
Benchmarks.Trace.TraceAnnotationsBenchmark - Same speed ✔️ Same allocations ✔️
Raw results
Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
---|---|---|---|---|---|---|---|---|---|
master | RunOnMethodBegin |
net6.0 | 661ns | 0.835ns | 3.24ns | 0.00965 | 0 | 0 | 696 B |
master | RunOnMethodBegin |
netcoreapp3.1 | 897ns | 1.03ns | 3.86ns | 0.00938 | 0 | 0 | 696 B |
master | RunOnMethodBegin |
net472 | 1.2μs | 2.45ns | 9.5ns | 0.104 | 0 | 0 | 658 B |
#6393 | RunOnMethodBegin |
net6.0 | 658ns | 1.11ns | 4.31ns | 0.00989 | 0 | 0 | 696 B |
#6393 | RunOnMethodBegin |
netcoreapp3.1 | 971ns | 1.84ns | 7.13ns | 0.00928 | 0 | 0 | 696 B |
#6393 | RunOnMethodBegin |
net472 | 1.16μs | 1.91ns | 7.41ns | 0.104 | 0 | 0 | 658 B |
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 (6393) (11.214M) : 0, 11214213
master (11.163M) : 0, 11163279
benchmarks/2.9.0 (11.033M) : 0, 11032866
section Automatic
This PR (6393) (7.263M) : 0, 7263142
master (7.379M) : 0, 7378518
benchmarks/2.9.0 (7.786M) : 0, 7785853
section Trace stats
master (7.698M) : 0, 7697685
section Manual
master (11.208M) : 0, 11207970
section Manual + Automatic
This PR (6393) (6.849M) : 0, 6848954
master (6.848M) : 0, 6847878
section DD_TRACE_ENABLED=0
master (10.300M) : 0, 10300258
gantt
title Throughput Linux arm64 (Total requests)
dateFormat X
axisFormat %s
section Baseline
This PR (6393) (9.680M) : 0, 9679570
master (9.614M) : 0, 9613662
benchmarks/2.9.0 (9.495M) : 0, 9494821
section Automatic
This PR (6393) (6.390M) : 0, 6389886
master (6.420M) : 0, 6420484
section Trace stats
master (6.620M) : 0, 6620303
section Manual
master (9.598M) : 0, 9598372
section Manual + Automatic
This PR (6393) (5.938M) : 0, 5938135
master (5.968M) : 0, 5968390
section DD_TRACE_ENABLED=0
master (8.911M) : 0, 8911128
gantt
title Throughput Windows x64 (Total requests)
dateFormat X
axisFormat %s
section Baseline
This PR (6393) (10.519M) : 0, 10518733
master (9.949M) : 0, 9949075
benchmarks/2.9.0 (10.020M) : 0, 10019592
section Automatic
This PR (6393) (6.617M) : 0, 6617289
master (6.538M) : 0, 6537814
benchmarks/2.9.0 (7.255M) : 0, 7255257
section Trace stats
master (7.075M) : 0, 7075474
section Manual
master (9.758M) : 0, 9757921
section Manual + Automatic
This PR (6393) (6.274M) : 0, 6273953
master (6.151M) : 0, 6151047
section DD_TRACE_ENABLED=0
master (9.404M) : 0, 9403695
|
…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
## Summary of changes Make the other settings in `IntegrationSettings` case-insensitive for the `IntegrationId` ## Reason for change In #6175 we made the `DD_TRACE_{0}_ENABLED` configurations case-insensitive for the `IntegrationId`. This PR it makes the other related settings behave similarly ## Implementation details Add additional fallbacks, the same as for the Enabled case ## Test coverage Added unit test for it ## Other details Switched the string.Format to interpolated because, you know, it's 2024
…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
Summary of changes
Make the other settings in
IntegrationSettings
case-insensitive for theIntegrationId
Reason for change
In #6175 we made the
DD_TRACE_{0}_ENABLED
configurations case-insensitive for theIntegrationId
. This PR it makes the other related settings behave similarlyImplementation details
Add additional fallbacks, the same as for the Enabled case
Test coverage
Added unit test for it
Other details
Switched the string.Format to interpolated because, you know, it's 2024