Skip to content

Make ExporterSettings properly immutable and remove ImmutableExporterSettings#6408

Merged
andrewlock merged 4 commits into
masterfrom
andrew/make-exporter-settings-immutable
Dec 11, 2024
Merged

Make ExporterSettings properly immutable and remove ImmutableExporterSettings#6408
andrewlock merged 4 commits into
masterfrom
andrew/make-exporter-settings-immutable

Conversation

@andrewlock

@andrewlock andrewlock commented Dec 6, 2024

Copy link
Copy Markdown
Member

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

@datadog-ddstaging

datadog-ddstaging Bot commented Dec 6, 2024

Copy link
Copy Markdown

Datadog Report

Branch report: andrew/make-exporter-settings-immutable
Commit report: ced5042
Test service: dd-trace-dotnet

✅ 0 Failed, 458069 Passed, 2823 Skipped, 19h 11m 9.25s Total Time

andrewlock added a commit that referenced this pull request Dec 6, 2024
## 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
@andrewlock
andrewlock force-pushed the andrew/remove-immutable-integration-settings branch from fce01d7 to 66412f3 Compare December 6, 2024 16:32
@andrewlock
andrewlock requested a review from a team as a code owner December 6, 2024 16:32
@andrewlock

andrewlock commented Dec 6, 2024

Copy link
Copy Markdown
Member Author

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:

  • Welch test with statistical test for significance of 5%
  • Only results indicating a difference greater than 5% and 5 ms are considered.

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,

Loading
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,

Loading
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,

Loading
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,

Loading
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,

Loading
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,

Loading

@andrewlock
andrewlock force-pushed the andrew/make-exporter-settings-immutable branch from 9627c71 to 91678de Compare December 6, 2024 16:32
@andrewlock

andrewlock commented Dec 6, 2024

Copy link
Copy Markdown
Member Author

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

Loading
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

Loading
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

Loading

@andrewlock

andrewlock commented Dec 7, 2024

Copy link
Copy Markdown
Member Author

Benchmarks Report for tracer 🐌

Benchmarks for #6408 compared to master:

  • 2 benchmarks are faster, with geometric mean 1.124
  • 1 benchmarks are slower, with geometric mean 1.240
  • 1 benchmarks have more 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.ActivityBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master StartStopWithChild net6.0 8.05μs 42.6ns 208ns 0.0156 0.0078 0 5.62 KB
master StartStopWithChild netcoreapp3.1 10.2μs 50.2ns 219ns 0.015 0.00501 0 5.8 KB
master StartStopWithChild net472 16.4μs 42.7ns 160ns 1.05 0.325 0.1 6.21 KB
#6408 StartStopWithChild net6.0 8.06μs 45.1ns 312ns 0.0195 0.00781 0 5.61 KB
#6408 StartStopWithChild netcoreapp3.1 10.4μs 53.6ns 257ns 0.0209 0.0105 0 5.81 KB
#6408 StartStopWithChild net472 16.3μs 41.6ns 156ns 1.03 0.287 0.0956 6.22 KB
Benchmarks.Trace.AgentWriterBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master WriteAndFlushEnrichedTraces net6.0 508μs 269ns 1.04μs 0 0 0 2.7 KB
master WriteAndFlushEnrichedTraces netcoreapp3.1 689μs 233ns 901ns 0 0 0 2.7 KB
master WriteAndFlushEnrichedTraces net472 853μs 383ns 1.48μs 0.425 0 0 3.3 KB
#6408 WriteAndFlushEnrichedTraces net6.0 484μs 337ns 1.3μs 0 0 0 2.7 KB
#6408 WriteAndFlushEnrichedTraces netcoreapp3.1 660μs 378ns 1.46μs 0 0 0 2.7 KB
#6408 WriteAndFlushEnrichedTraces net472 867μs 417ns 1.56μs 0.428 0 0 3.3 KB
Benchmarks.Trace.AspNetCoreBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master SendRequest net6.0 149μs 843ns 5.72μs 0.142 0 0 14.47 KB
master SendRequest netcoreapp3.1 165μs 930ns 6.03μs 0.17 0 0 17.27 KB
master SendRequest net472 0.000451ns 0.000277ns 0.00107ns 0 0 0 0 b
#6408 SendRequest net6.0 152μs 936ns 9.17μs 0.14 0 0 14.47 KB
#6408 SendRequest netcoreapp3.1 159μs 714ns 3.03μs 0.155 0 0 17.27 KB
#6408 SendRequest net472 0.000162ns 0.000146ns 0.000546ns 0 0 0 0 b
Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark - Same speed ✔️ More allocations ⚠️

More allocations ⚠️ in #6408

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

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

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

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

andrewlock added a commit that referenced this pull request Dec 9, 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
@andrewlock
andrewlock force-pushed the andrew/remove-immutable-integration-settings branch from 66412f3 to 5002f8b Compare December 9, 2024 09:08
@andrewlock
andrewlock force-pushed the andrew/make-exporter-settings-immutable branch from 91678de to c4cd7f8 Compare December 9, 2024 09:08
andrewlock added a commit that referenced this pull request Dec 9, 2024
…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());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

x.Value?.ToString()!
:stare:

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😉

@tonyredondo tonyredondo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI Visibility stuff 👌🏻

andrewlock added a commit that referenced this pull request Dec 10, 2024
…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
@andrewlock
andrewlock force-pushed the andrew/remove-immutable-integration-settings branch from 5002f8b to 92bba79 Compare December 10, 2024 14:20
@andrewlock
andrewlock force-pushed the andrew/make-exporter-settings-immutable branch 3 times, most recently from c039b45 to 5b80296 Compare December 10, 2024 15:55
andrewlock added a commit that referenced this pull request Dec 11, 2024
…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
Base automatically changed from andrew/remove-immutable-integration-settings to master December 11, 2024 12:42
@andrewlock
andrewlock force-pushed the andrew/make-exporter-settings-immutable branch from 5b80296 to ced5042 Compare December 11, 2024 12:43
@andrewlock
andrewlock merged commit d7ce88e into master Dec 11, 2024
@andrewlock
andrewlock deleted the andrew/make-exporter-settings-immutable branch December 11, 2024 17:03
@github-actions github-actions Bot added this to the vNext-v3 milestone Dec 11, 2024
andrewlock added a commit that referenced this pull request Dec 12, 2024
…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
veerbia pushed a commit that referenced this pull request Dec 16, 2024
## 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
veerbia pushed a commit that referenced this pull request Dec 16, 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
veerbia pushed a commit that referenced this pull request Dec 16, 2024
…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
veerbia pushed a commit that referenced this pull request Dec 16, 2024
…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
veerbia pushed a commit that referenced this pull request Dec 16, 2024
…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
veerbia pushed a commit that referenced this pull request Dec 16, 2024
…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
veerbia pushed a commit that referenced this pull request Dec 16, 2024
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:tracer The core tracer library (Datadog.Trace, does not include OpenTracing, native code, or integrations) type:refactor

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants