Skip to content

Make IntegrationSettings properly immutable and remove ImmutableIntegrationSettings#6405

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

Make IntegrationSettings properly immutable and remove ImmutableIntegrationSettings#6405
andrewlock merged 4 commits into
masterfrom
andrew/remove-immutable-integration-settings

Conversation

@andrewlock

@andrewlock andrewlock commented Dec 5, 2024

Copy link
Copy Markdown
Member

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

@datadog-ddstaging

datadog-ddstaging Bot commented Dec 5, 2024

Copy link
Copy Markdown

Datadog Report

Branch report: andrew/remove-immutable-integration-settings
Commit report: 9a096c9
Test service: dd-trace-dotnet

❌ 9 Failed (0 Known Flaky), 462347 Passed, 2863 Skipped, 19h 28m 39.06s Total Time
❄️ 1 New Flaky

❌ Failed Tests (9)

This report shows up to 5 failed tests.

  • PopulatesFromBuilderCorrectly - Datadog.Trace.Tests.Configuration.IntegrationSettingsCollectionTests - Details

    Expand for error
     Method not found: 'Void Datadog.Trace.Configuration.IntegrationSettingsCollection..ctor(Datadog.Trace.Configuration.IConfigurationSource, System.Collections.Generic.HashSet\`1<System.String>)'.
    
  • ReturnsDefaultSettingsForUnknownIntegration - Datadog.Trace.Tests.Configuration.IntegrationSettingsCollectionTests - Details

    Expand for error
     Method not found: 'Void Datadog.Trace.Configuration.IntegrationSettingsCollection..ctor(Datadog.Trace.Configuration.IConfigurationSource, System.Collections.Generic.HashSet\`1<System.String>)'.
    
  • ReturnsIntegrationWhenUsingIncorrectCasing - Datadog.Trace.Tests.Configuration.IntegrationSettingsCollectionTests - Details

    Expand for error
     Method not found: 'Void Datadog.Trace.Configuration.IntegrationSettingsCollection..ctor(Datadog.Trace.Configuration.IConfigurationSource, System.Collections.Generic.HashSet\`1<System.String>)'.
    
  • SettingsRespectsOverride - Datadog.Trace.Tests.Configuration.IntegrationSettingsTests - Details

    Expand for error
     Expected boolean to be false, but found True.
    
  • AutomaticToManual_AllDefaultSettingsAreTransferredCorrectly - Datadog.Trace.Tests.ManualInstrumentation.SettingsInstrumentationTests - Details

    Expand for error
     Method not found: 'Datadog.Trace.Configuration.ExporterSettings Datadog.Trace.Configuration.ExporterSettings.Create(System.Collections.Generic.Dictionary\`2<System.String,System.Object>)'.
    

New Flaky Tests (1)

  • SubmitsTraces - Datadog.Trace.ClrProfiler.IntegrationTests.StackExchangeRedisTests - Last Failure

    Expand for error
     Results do not match.
     Differences:
     Received: StackExchangeRedisTests.Latest.SchemaV0.received.txt
     Verified: StackExchangeRedisTests.Latest.SchemaV0.verified.txt
     Received Content:
     [
       {
         TraceId: Id_1,
         SpanId: Id_2,
         Name: redis.command,
     ...
    

@andrewlock

andrewlock commented Dec 5, 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 (6405) - mean (69ms)  : 66, 72
     .   : milestone, 69,
    master - mean (69ms)  : 66, 73
     .   : milestone, 69,

    section CallTarget+Inlining+NGEN
    This PR (6405) - mean (980ms)  : 952, 1009
     .   : milestone, 980,
    master - mean (978ms)  : 956, 1000
     .   : milestone, 978,

Loading
gantt
    title Execution time (ms) FakeDbCommand (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6405) - mean (108ms)  : 106, 111
     .   : milestone, 108,
    master - mean (108ms)  : 106, 110
     .   : milestone, 108,

    section CallTarget+Inlining+NGEN
    This PR (6405) - mean (680ms)  : 666, 694
     .   : milestone, 680,
    master - mean (681ms)  : 666, 696
     .   : milestone, 681,

Loading
gantt
    title Execution time (ms) FakeDbCommand (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6405) - mean (92ms)  : 90, 94
     .   : milestone, 92,
    master - mean (92ms)  : 90, 94
     .   : milestone, 92,

    section CallTarget+Inlining+NGEN
    This PR (6405) - mean (632ms)  : 613, 651
     .   : milestone, 632,
    master - mean (635ms)  : 619, 651
     .   : milestone, 635,

Loading
gantt
    title Execution time (ms) HttpMessageHandler (.NET Framework 4.6.2) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6405) - mean (192ms)  : 186, 198
     .   : milestone, 192,
    master - mean (192ms)  : 187, 197
     .   : milestone, 192,

    section CallTarget+Inlining+NGEN
    This PR (6405) - mean (1,100ms)  : 1069, 1130
     .   : milestone, 1100,
    master - mean (1,100ms)  : 1070, 1131
     .   : milestone, 1100,

Loading
gantt
    title Execution time (ms) HttpMessageHandler (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6405) - mean (279ms)  : 275, 283
     .   : milestone, 279,
    master - mean (279ms)  : 275, 284
     .   : milestone, 279,

    section CallTarget+Inlining+NGEN
    This PR (6405) - mean (876ms)  : 844, 907
     .   : milestone, 876,
    master - mean (879ms)  : 857, 901
     .   : milestone, 879,

Loading
gantt
    title Execution time (ms) HttpMessageHandler (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6405) - mean (268ms)  : 264, 272
     .   : milestone, 268,
    master - mean (269ms)  : 264, 274
     .   : milestone, 269,

    section CallTarget+Inlining+NGEN
    This PR (6405) - mean (852ms)  : 818, 887
     .   : milestone, 852,
    master - mean (858ms)  : 828, 889
     .   : milestone, 858,

Loading

@andrewlock

andrewlock commented Dec 5, 2024

Copy link
Copy Markdown
Member Author

Benchmarks Report for appsec 🐌

Benchmarks for #6405 compared to master:

  • All benchmarks have the same speed
  • 6 benchmarks have fewer allocations
  • 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.Asm.AppSecBodyBenchmark - Same speed ✔️ Fewer allocations 🎉

Fewer allocations 🎉 in #6405

Benchmark Base Allocated Diff Allocated Change Change %
Benchmarks.Trace.Asm.AppSecBodyBenchmark.AllCycleMoreComplexBody‑net472 243.62 KB 234.39 KB -9.23 KB -3.79%
Benchmarks.Trace.Asm.AppSecBodyBenchmark.AllCycleSimpleBody‑net472 240.1 KB 230.87 KB -9.23 KB -3.84%
Benchmarks.Trace.Asm.AppSecBodyBenchmark.AllCycleMoreComplexBody‑netcoreapp3.1 216.34 KB 207.17 KB -9.17 KB -4.24%
Benchmarks.Trace.Asm.AppSecBodyBenchmark.AllCycleMoreComplexBody‑net6.0 208.98 KB 200.01 KB -8.98 KB -4.30%
Benchmarks.Trace.Asm.AppSecBodyBenchmark.AllCycleSimpleBody‑netcoreapp3.1 212.92 KB 203.75 KB -9.17 KB -4.31%
Benchmarks.Trace.Asm.AppSecBodyBenchmark.AllCycleSimpleBody‑net6.0 205.48 KB 196.5 KB -8.98 KB -4.37%

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master AllCycleSimpleBody net6.0 218μs 97.8ns 353ns 2.83 0 0 205.48 KB
master AllCycleSimpleBody netcoreapp3.1 319μs 112ns 421ns 2.88 0 0 212.92 KB
master AllCycleSimpleBody net472 281μs 207ns 800ns 38.1 2.93 0 240.1 KB
master AllCycleMoreComplexBody net6.0 220μs 93.8ns 351ns 2.98 0 0 208.98 KB
master AllCycleMoreComplexBody netcoreapp3.1 320μs 146ns 507ns 2.91 0 0 216.34 KB
master AllCycleMoreComplexBody net472 291μs 442ns 1.65μs 38.7 2.89 0 243.62 KB
master ObjectExtractorSimpleBody net6.0 139ns 0.11ns 0.412ns 0.00396 0 0 280 B
master ObjectExtractorSimpleBody netcoreapp3.1 199ns 0.156ns 0.604ns 0.00369 0 0 272 B
master ObjectExtractorSimpleBody net472 214ns 0.2ns 0.775ns 0.0446 0 0 281 B
master ObjectExtractorMoreComplexBody net6.0 2.93μs 0.973ns 3.51ns 0.0528 0 0 3.78 KB
master ObjectExtractorMoreComplexBody netcoreapp3.1 3.75μs 5.03ns 19.5ns 0.0503 0 0 3.69 KB
master ObjectExtractorMoreComplexBody net472 4.51μs 22.4ns 100ns 0.602 0.00703 0 3.8 KB
#6405 AllCycleSimpleBody net6.0 213μs 182ns 679ns 2.73 0 0 196.5 KB
#6405 AllCycleSimpleBody netcoreapp3.1 327μs 129ns 498ns 2.77 0 0 203.75 KB
#6405 AllCycleSimpleBody net472 281μs 212ns 821ns 36.7 2.4 0 230.87 KB
#6405 AllCycleMoreComplexBody net6.0 217μs 171ns 640ns 2.82 0 0 200.01 KB
#6405 AllCycleMoreComplexBody netcoreapp3.1 319μs 117ns 436ns 2.87 0 0 207.17 KB
#6405 AllCycleMoreComplexBody net472 287μs 457ns 1.77μs 37.2 2.42 0 234.39 KB
#6405 ObjectExtractorSimpleBody net6.0 146ns 0.139ns 0.538ns 0.00398 0 0 280 B
#6405 ObjectExtractorSimpleBody netcoreapp3.1 203ns 0.272ns 1.05ns 0.00375 0 0 272 B
#6405 ObjectExtractorSimpleBody net472 208ns 0.258ns 1ns 0.0446 0 0 281 B
#6405 ObjectExtractorMoreComplexBody net6.0 2.99μs 1.64ns 6.13ns 0.0525 0 0 3.78 KB
#6405 ObjectExtractorMoreComplexBody netcoreapp3.1 3.72μs 2.23ns 8.35ns 0.0503 0 0 3.69 KB
#6405 ObjectExtractorMoreComplexBody net472 4.28μs 2.51ns 9.05ns 0.602 0.00643 0 3.8 KB
Benchmarks.Trace.Asm.AppSecEncoderBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master EncodeArgs net6.0 37.4μs 18.2ns 68.2ns 0.447 0 0 32.4 KB
master EncodeArgs netcoreapp3.1 55μs 113ns 425ns 0.435 0 0 32.4 KB
master EncodeArgs net472 67.2μs 42.1ns 158ns 5.15 0.0673 0 32.5 KB
master EncodeLegacyArgs net6.0 78.9μs 35.7ns 124ns 0.0372 0 0 2.14 KB
master EncodeLegacyArgs netcoreapp3.1 104μs 346ns 1.34μs 0 0 0 2.14 KB
master EncodeLegacyArgs net472 156μs 214ns 828ns 0.312 0 0 2.15 KB
#6405 EncodeArgs net6.0 38μs 16.4ns 63.6ns 0.452 0 0 32.4 KB
#6405 EncodeArgs netcoreapp3.1 53.8μs 17.4ns 67.4ns 0.43 0 0 32.4 KB
#6405 EncodeArgs net472 66.7μs 42.1ns 157ns 5.14 0.0667 0 32.5 KB
#6405 EncodeLegacyArgs net6.0 75.4μs 112ns 435ns 0 0 0 2.14 KB
#6405 EncodeLegacyArgs netcoreapp3.1 108μs 320ns 1.24μs 0 0 0 2.14 KB
#6405 EncodeLegacyArgs net472 160μs 85.7ns 332ns 0.316 0 0 2.15 KB
Benchmarks.Trace.Asm.AppSecWafBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master RunWafRealisticBenchmark net6.0 181μs 77.5ns 290ns 0 0 0 2.44 KB
master RunWafRealisticBenchmark netcoreapp3.1 194μs 357ns 1.38μs 0 0 0 2.39 KB
master RunWafRealisticBenchmark net472 207μs 97.4ns 364ns 0.308 0 0 2.46 KB
master RunWafRealisticBenchmarkWithAttack net6.0 122μs 63.2ns 245ns 0 0 0 1.47 KB
master RunWafRealisticBenchmarkWithAttack netcoreapp3.1 129μs 104ns 390ns 0 0 0 1.46 KB
master RunWafRealisticBenchmarkWithAttack net472 140μs 68.7ns 257ns 0.21 0 0 1.49 KB
#6405 RunWafRealisticBenchmark net6.0 183μs 86.3ns 311ns 0 0 0 2.44 KB
#6405 RunWafRealisticBenchmark netcoreapp3.1 197μs 223ns 833ns 0 0 0 2.39 KB
#6405 RunWafRealisticBenchmark net472 208μs 118ns 457ns 0.31 0 0 2.46 KB
#6405 RunWafRealisticBenchmarkWithAttack net6.0 122μs 96.6ns 374ns 0 0 0 1.47 KB
#6405 RunWafRealisticBenchmarkWithAttack netcoreapp3.1 130μs 89.2ns 321ns 0 0 0 1.46 KB
#6405 RunWafRealisticBenchmarkWithAttack net472 141μs 123ns 475ns 0.209 0 0 1.49 KB
Benchmarks.Trace.Iast.StringAspectsBenchmark - Same speed ✔️ More allocations ⚠️

More allocations ⚠️ in #6405

Benchmark Base Allocated Diff Allocated Change Change %
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatBenchmark‑net472 57.34 KB 59.04 KB 1.7 KB 2.96%

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master StringConcatBenchmark net6.0 60.4μs 969ns 9.69μs 0 0 0 43.44 KB
master StringConcatBenchmark netcoreapp3.1 55.8μs 256ns 1.45μs 0 0 0 42.64 KB
master StringConcatBenchmark net472 37.7μs 105ns 379ns 0 0 0 57.34 KB
master StringConcatAspectBenchmark net6.0 308μs 1.32μs 5.3μs 0 0 0 253.63 KB
master StringConcatAspectBenchmark netcoreapp3.1 354μs 1.93μs 11.8μs 0 0 0 253.63 KB
master StringConcatAspectBenchmark net472 295μs 6.48μs 62.5μs 0 0 0 278.53 KB
#6405 StringConcatBenchmark net6.0 62.6μs 909ns 9μs 0 0 0 43.44 KB
#6405 StringConcatBenchmark netcoreapp3.1 56.1μs 386ns 3.64μs 0 0 0 42.64 KB
#6405 StringConcatBenchmark net472 38.4μs 127ns 457ns 0 0 0 59.04 KB
#6405 StringConcatAspectBenchmark net6.0 308μs 5.2μs 50.9μs 0 0 0 254.1 KB
#6405 StringConcatAspectBenchmark netcoreapp3.1 358μs 2.06μs 17μs 0 0 0 252.65 KB
#6405 StringConcatAspectBenchmark net472 291μs 4.7μs 45.1μs 0 0 0 278.53 KB

@andrewlock
andrewlock force-pushed the andrew/remove-immutable-integration-settings branch from d5d2e70 to e628e25 Compare December 5, 2024 17:24
@andrewlock

andrewlock commented Dec 5, 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 (6405) (11.006M)   : 0, 11006172
    master (11.088M)   : 0, 11088184
    benchmarks/2.9.0 (11.033M)   : 0, 11032866

    section Automatic
    This PR (6405) (7.192M)   : 0, 7191516
    master (7.254M)   : 0, 7254128
    benchmarks/2.9.0 (7.786M)   : 0, 7785853

    section Trace stats
    master (7.516M)   : 0, 7515745

    section Manual
    master (11.014M)   : 0, 11013989

    section Manual + Automatic
    This PR (6405) (6.673M)   : 0, 6672632
    master (6.700M)   : 0, 6700166

    section DD_TRACE_ENABLED=0
    master (10.228M)   : 0, 10228208

Loading
gantt
    title Throughput Linux arm64 (Total requests) 
    dateFormat  X
    axisFormat %s
    section Baseline
    This PR (6405) (9.481M)   : 0, 9480973
    master (9.438M)   : 0, 9438272
    benchmarks/2.9.0 (9.495M)   : 0, 9494821

    section Automatic
    This PR (6405) (6.443M)   : 0, 6443074
    master (6.384M)   : 0, 6384110

    section Trace stats
    master (6.758M)   : 0, 6758045

    section Manual
    master (9.394M)   : 0, 9393606

    section Manual + Automatic
    This PR (6405) (6.058M)   : 0, 6058469
    master (6.041M)   : 0, 6040522

    section DD_TRACE_ENABLED=0
    master (8.691M)   : 0, 8690632

Loading
gantt
    title Throughput Windows x64 (Total requests) 
    dateFormat  X
    axisFormat %s
    section Baseline
    This PR (6405) (9.062M)   : 0, 9061725
    master (9.438M)   : 0, 9438032
    benchmarks/2.9.0 (10.020M)   : 0, 10019592

    section Automatic
    This PR (6405) (5.988M)   : 0, 5987546
    master (6.073M)   : 0, 6073208
    benchmarks/2.9.0 (7.255M)   : 0, 7255257

    section Trace stats
    master (6.747M)   : 0, 6747095

    section Manual
    master (9.357M)   : 0, 9356938

    section Manual + Automatic
    This PR (6405) (5.634M)   : 0, 5634404
    master (5.605M)   : 0, 5604930

    section DD_TRACE_ENABLED=0
    master (8.787M)   : 0, 8787187

Loading

@andrewlock

andrewlock commented Dec 6, 2024

Copy link
Copy Markdown
Member Author

Benchmarks Report for tracer 🐌

Benchmarks for #6405 compared to master:

  • 3 benchmarks are faster, with geometric mean 1.189
  • All benchmarks have the same allocations

The following thresholds were used for comparing the benchmark speeds:

  • Mann–Whitney U test with statistical test for significance of 5%
  • Only results indicating a difference greater than 10% and 0.3 ns are considered.

Allocation changes below 0.5% are ignored.

Benchmark details

Benchmarks.Trace.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.06μs 43.3ns 241ns 0.0163 0.00817 0 5.61 KB
master StartStopWithChild netcoreapp3.1 9.98μs 54.9ns 320ns 0.0243 0.00972 0 5.8 KB
master StartStopWithChild net472 16.3μs 42ns 157ns 1.03 0.286 0.09 6.22 KB
#6405 StartStopWithChild net6.0 8.04μs 43.5ns 288ns 0.0193 0.00771 0 5.61 KB
#6405 StartStopWithChild netcoreapp3.1 9.84μs 50.5ns 247ns 0.0149 0.00498 0 5.8 KB
#6405 StartStopWithChild net472 16.4μs 63.3ns 245ns 1.04 0.315 0.0911 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 506μs 421ns 1.63μs 0 0 0 2.7 KB
master WriteAndFlushEnrichedTraces netcoreapp3.1 654μs 199ns 745ns 0 0 0 2.7 KB
master WriteAndFlushEnrichedTraces net472 851μs 563ns 2.11μs 0.425 0 0 3.3 KB
#6405 WriteAndFlushEnrichedTraces net6.0 514μs 324ns 1.21μs 0 0 0 2.7 KB
#6405 WriteAndFlushEnrichedTraces netcoreapp3.1 641μs 464ns 1.74μs 0 0 0 2.7 KB
#6405 WriteAndFlushEnrichedTraces net472 838μs 642ns 2.48μs 0.417 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 143μs 807ns 5.93μs 0.145 0 0 14.47 KB
master SendRequest netcoreapp3.1 179μs 1.6μs 15.9μs 0.168 0 0 17.27 KB
master SendRequest net472 0.00208ns 0.000508ns 0.0019ns 0 0 0 0 b
#6405 SendRequest net6.0 144μs 809ns 5.49μs 0.143 0 0 14.47 KB
#6405 SendRequest netcoreapp3.1 162μs 909ns 6.36μs 0.16 0 0 17.27 KB
#6405 SendRequest net472 0ns 0ns 0ns 0 0 0 0 b
Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master WriteAndFlushEnrichedTraces net6.0 553μs 1.8μs 6.75μs 0.563 0 0 41.59 KB
master WriteAndFlushEnrichedTraces netcoreapp3.1 654μs 2.32μs 8.68μs 0.317 0 0 41.7 KB
master WriteAndFlushEnrichedTraces net472 805μs 2.55μs 9.55μs 8.12 2.44 0.406 53.24 KB
#6405 WriteAndFlushEnrichedTraces net6.0 559μs 2.81μs 12.6μs 0.556 0 0 41.55 KB
#6405 WriteAndFlushEnrichedTraces netcoreapp3.1 657μs 3.33μs 16.3μs 0.322 0 0 41.78 KB
#6405 WriteAndFlushEnrichedTraces net472 850μs 3.05μs 11.8μs 8.19 2.59 0.431 53.28 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.28μs 1.36ns 5.28ns 0.0141 0 0 1.02 KB
master ExecuteNonQuery netcoreapp3.1 1.71μs 1.04ns 3.76ns 0.0136 0 0 1.02 KB
master ExecuteNonQuery net472 2.07μs 1.61ns 6.24ns 0.156 0.00104 0 987 B
#6405 ExecuteNonQuery net6.0 1.26μs 1.39ns 5.38ns 0.0145 0 0 1.02 KB
#6405 ExecuteNonQuery netcoreapp3.1 1.8μs 1.48ns 5.73ns 0.0134 0 0 1.02 KB
#6405 ExecuteNonQuery net472 2.11μs 1.57ns 6.08ns 0.157 0.00106 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.33μs 1.07ns 4.14ns 0.0139 0 0 976 B
master CallElasticsearch netcoreapp3.1 1.47μs 0.577ns 2.16ns 0.0133 0 0 976 B
master CallElasticsearch net472 2.54μs 0.915ns 3.3ns 0.158 0 0 995 B
master CallElasticsearchAsync net6.0 1.37μs 0.854ns 3.19ns 0.0131 0 0 952 B
master CallElasticsearchAsync netcoreapp3.1 1.7μs 4.9ns 19ns 0.0137 0 0 1.02 KB
master CallElasticsearchAsync net472 2.65μs 0.862ns 3.34ns 0.167 0 0 1.05 KB
#6405 CallElasticsearch net6.0 1.26μs 0.465ns 1.74ns 0.0134 0 0 976 B
#6405 CallElasticsearch netcoreapp3.1 1.54μs 1.75ns 6.54ns 0.0129 0 0 976 B
#6405 CallElasticsearch net472 2.46μs 2.03ns 7.58ns 0.158 0 0 995 B
#6405 CallElasticsearchAsync net6.0 1.31μs 1.16ns 4.33ns 0.0132 0 0 952 B
#6405 CallElasticsearchAsync netcoreapp3.1 1.68μs 1.45ns 5.43ns 0.0133 0 0 1.02 KB
#6405 CallElasticsearchAsync net472 2.66μs 1.46ns 5.27ns 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.36μs 0.81ns 3.03ns 0.0129 0 0 952 B
master ExecuteAsync netcoreapp3.1 1.61μs 1.15ns 4.31ns 0.013 0 0 952 B
master ExecuteAsync net472 1.9μs 1ns 3.75ns 0.145 0 0 915 B
#6405 ExecuteAsync net6.0 1.28μs 0.769ns 2.88ns 0.0135 0 0 952 B
#6405 ExecuteAsync netcoreapp3.1 1.6μs 2.01ns 7.23ns 0.0128 0 0 952 B
#6405 ExecuteAsync net472 1.82μs 1.54ns 5.76ns 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.42μs 4.18ns 15.6ns 0.0309 0 0 2.31 KB
master SendAsync netcoreapp3.1 5.29μs 3.22ns 12.5ns 0.0371 0 0 2.85 KB
master SendAsync net472 7.48μs 6.25ns 24.2ns 0.493 0 0 3.12 KB
#6405 SendAsync net6.0 4.38μs 15.6ns 60.5ns 0.032 0 0 2.31 KB
#6405 SendAsync netcoreapp3.1 5.47μs 3.86ns 15ns 0.0381 0 0 2.85 KB
#6405 SendAsync net472 7.46μs 2.71ns 10.5ns 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.45μs 0.698ns 2.52ns 0.0232 0 0 1.64 KB
master EnrichedLog netcoreapp3.1 2.34μs 1.6ns 5.99ns 0.0221 0 0 1.64 KB
master EnrichedLog net472 2.66μs 1.32ns 4.96ns 0.249 0 0 1.57 KB
#6405 EnrichedLog net6.0 1.47μs 0.604ns 2.26ns 0.023 0 0 1.64 KB
#6405 EnrichedLog netcoreapp3.1 2.26μs 2.58ns 9.65ns 0.0216 0 0 1.64 KB
#6405 EnrichedLog net472 2.68μs 5.03ns 18.8ns 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 118μs 175ns 678ns 0.06 0 0 4.28 KB
master EnrichedLog netcoreapp3.1 121μs 182ns 704ns 0.0613 0 0 4.28 KB
master EnrichedLog net472 152μs 137ns 531ns 0.678 0.226 0 4.46 KB
#6405 EnrichedLog net6.0 119μs 174ns 673ns 0 0 0 4.28 KB
#6405 EnrichedLog netcoreapp3.1 124μs 142ns 530ns 0.0623 0 0 4.28 KB
#6405 EnrichedLog net472 153μs 218ns 845ns 0.688 0.229 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.99μs 1.55ns 5.99ns 0.0299 0 0 2.2 KB
master EnrichedLog netcoreapp3.1 4.06μs 1.49ns 5.78ns 0.0283 0 0 2.2 KB
master EnrichedLog net472 4.94μs 4.94ns 19.1ns 0.319 0 0 2.02 KB
#6405 EnrichedLog net6.0 3.26μs 1.05ns 4.07ns 0.0304 0 0 2.2 KB
#6405 EnrichedLog netcoreapp3.1 4.16μs 2.02ns 7.55ns 0.029 0 0 2.2 KB
#6405 EnrichedLog net472 4.89μs 1.63ns 6.08ns 0.319 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.38μs 0.339ns 1.31ns 0.0159 0 0 1.14 KB
master SendReceive netcoreapp3.1 1.78μs 1.1ns 4.26ns 0.0152 0 0 1.14 KB
master SendReceive net472 2.18μs 1.21ns 4.51ns 0.183 0 0 1.16 KB
#6405 SendReceive net6.0 1.42μs 0.483ns 1.87ns 0.0165 0 0 1.14 KB
#6405 SendReceive netcoreapp3.1 1.79μs 1.16ns 4.33ns 0.0151 0 0 1.14 KB
#6405 SendReceive net472 2.14μs 1.97ns 7.63ns 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.73μs 1.34ns 4.83ns 0.022 0 0 1.6 KB
master EnrichedLog netcoreapp3.1 3.91μs 2.31ns 8.93ns 0.0216 0 0 1.65 KB
master EnrichedLog net472 4.29μs 1.88ns 7.29ns 0.323 0 0 2.04 KB
#6405 EnrichedLog net6.0 2.75μs 2.15ns 8.32ns 0.0223 0 0 1.6 KB
#6405 EnrichedLog netcoreapp3.1 3.85μs 1.87ns 7.25ns 0.0231 0 0 1.65 KB
#6405 EnrichedLog net472 4.27μs 2.58ns 9.99ns 0.323 0 0 2.04 KB
Benchmarks.Trace.SpanBenchmark - Faster 🎉 Same allocations ✔️

Faster 🎉 in #6405

Benchmark base/diff Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.SpanBenchmark.StartFinishScope‑net6.0 1.176 566.78 481.94
Benchmarks.Trace.SpanBenchmark.StartFinishSpan‑net472 1.138 714.14 627.31

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master StartFinishSpan net6.0 407ns 0.183ns 0.709ns 0.00814 0 0 576 B
master StartFinishSpan netcoreapp3.1 610ns 0.688ns 2.66ns 0.00799 0 0 576 B
master StartFinishSpan net472 713ns 0.651ns 2.52ns 0.0918 0 0 578 B
master StartFinishScope net6.0 567ns 0.366ns 1.42ns 0.00958 0 0 696 B
master StartFinishScope netcoreapp3.1 746ns 0.448ns 1.73ns 0.00938 0 0 696 B
master StartFinishScope net472 897ns 1.13ns 4.39ns 0.104 0 0 658 B
#6405 StartFinishSpan net6.0 450ns 0.201ns 0.778ns 0.00809 0 0 576 B
#6405 StartFinishSpan netcoreapp3.1 638ns 0.211ns 0.818ns 0.00772 0 0 576 B
#6405 StartFinishSpan net472 628ns 0.331ns 1.28ns 0.0917 0 0 578 B
#6405 StartFinishScope net6.0 482ns 0.253ns 0.981ns 0.00966 0 0 696 B
#6405 StartFinishScope netcoreapp3.1 702ns 0.567ns 2.04ns 0.0096 0 0 696 B
#6405 StartFinishScope net472 884ns 0.574ns 2.22ns 0.104 0 0 658 B
Benchmarks.Trace.TraceAnnotationsBenchmark - Faster 🎉 Same allocations ✔️

Faster 🎉 in #6405

Benchmark base/diff Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.TraceAnnotationsBenchmark.RunOnMethodBegin‑net6.0 1.255 809.88 645.24

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master RunOnMethodBegin net6.0 809ns 0.762ns 2.95ns 0.00971 0 0 696 B
master RunOnMethodBegin netcoreapp3.1 896ns 0.537ns 2.08ns 0.00939 0 0 696 B
master RunOnMethodBegin net472 1.12μs 1.13ns 4.37ns 0.104 0 0 658 B
#6405 RunOnMethodBegin net6.0 645ns 0.393ns 1.52ns 0.00974 0 0 696 B
#6405 RunOnMethodBegin netcoreapp3.1 847ns 0.796ns 3.08ns 0.00941 0 0 696 B
#6405 RunOnMethodBegin net472 1.1μs 0.696ns 2.6ns 0.104 0 0 658 B

andrewlock added a commit that referenced this pull request Dec 6, 2024
## Summary of changes

Delete the Public API Generator

## Reason for change

We added this generator to reduce the amount of boilerplate you need to
write to correctly record telemetry for public APIs. However, with the
move to Datadog.Trace.Manual, these APIs are completely unused, so we
may as well remove the complexity and duplication.

## Implementation details

- Delete the `[GeneratePublicApi]` generator
- Keeping the `[PublicApi]` attribute for now - that will be cleared up
in a separate PR
- Rename the previously-decorated properties that _were_ called
`SomePropInternal` to `SomeProp`
- We still have some "leftover" `Internal` suffixes, will tidy those up
separately
- Main changes are in `TracerSettings`, `ExporterSettings`,
`IntegrationSettings` + their immutable counterparts

## Test coverage

Covered by existing tests

## Other details

I noticed that there were some properties that we were getting public
API telemetry for which we kind of lost in the move to
Datadog.Trace.Manual: `SpanContext.Parent`, `SpanContext.ParentId`,
`SpanContext.ServiceName`. I don't _think_ that actually matters, as
these can't actually be accessed any more for technical reasons...

Part of stack 
- #6370
- #6376 👈 This PR
- #6385
- #6386
- #6397
- #6399
- #6405
@andrewlock
andrewlock force-pushed the andrew/remove-loginjection-mutable-settings branch from 969d02e to 6cf9548 Compare December 6, 2024 11:49
@andrewlock
andrewlock requested review from a team as code owners December 6, 2024 11:49
@andrewlock
andrewlock force-pushed the andrew/remove-immutable-integration-settings branch from e628e25 to 6712975 Compare December 6, 2024 11:49
andrewlock added a commit that referenced this pull request Dec 6, 2024
…6385)

## Summary of changes

Remove `PublicApiTests.PublicApiHasNotChanged` for Datadog.Trace.dll

## Reason for change

The public API surface of Datadog.Trace is not _really_ public any more,
as it's not directly referenced, so these tests are largely
unneccessary.

As of right now, `public` vs `internal` is essentially irrelevant in
this project.

## Implementation details

Remove the `PublicApiHasNotChanged` test. The other public API tests
e.g. `AssemblyReferencesHaveNotChanged` _are_ still relevant, so jumped
through some hoops to keep them

## Test coverage

Slightly less now.

## Other details

Part of stack 

- #6370
- #6376
- #6385 👈 This PR
- #6386
- #6397
- #6399
- #6400
- #6405
@andrewlock
andrewlock force-pushed the andrew/remove-loginjection-mutable-settings branch from 6cf9548 to 0733b82 Compare December 6, 2024 14:10
@andrewlock
andrewlock force-pushed the andrew/remove-immutable-integration-settings branch from 6712975 to fce01d7 Compare December 6, 2024 14:10
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-loginjection-mutable-settings branch from 0733b82 to db4f2d0 Compare December 6, 2024 16:32
@andrewlock
andrewlock force-pushed the andrew/remove-immutable-integration-settings branch from fce01d7 to 66412f3 Compare December 6, 2024 16:32
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-loginjection-mutable-settings branch from db4f2d0 to 9aaaa68 Compare December 9, 2024 09:08
@andrewlock
andrewlock force-pushed the andrew/remove-immutable-integration-settings branch from 66412f3 to 5002f8b 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
Base automatically changed from andrew/remove-loginjection-mutable-settings to master December 10, 2024 14:04
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
…mutate DisabledIntegrationNames, and so that we use the final values in the integration settings collection
…of mutating where it touches IntegrationSettingsCollection
@andrewlock
andrewlock force-pushed the andrew/remove-immutable-integration-settings branch from 5002f8b to 92bba79 Compare December 10, 2024 14:20
@andrewlock
andrewlock merged commit 8319e88 into master Dec 11, 2024
@andrewlock
andrewlock deleted the andrew/remove-immutable-integration-settings branch December 11, 2024 12:42
@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 11, 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
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 the Public API Generator

## Reason for change

We added this generator to reduce the amount of boilerplate you need to
write to correctly record telemetry for public APIs. However, with the
move to Datadog.Trace.Manual, these APIs are completely unused, so we
may as well remove the complexity and duplication.

## Implementation details

- Delete the `[GeneratePublicApi]` generator
- Keeping the `[PublicApi]` attribute for now - that will be cleared up
in a separate PR
- Rename the previously-decorated properties that _were_ called
`SomePropInternal` to `SomeProp`
- We still have some "leftover" `Internal` suffixes, will tidy those up
separately
- Main changes are in `TracerSettings`, `ExporterSettings`,
`IntegrationSettings` + their immutable counterparts

## Test coverage

Covered by existing tests

## Other details

I noticed that there were some properties that we were getting public
API telemetry for which we kind of lost in the move to
Datadog.Trace.Manual: `SpanContext.Parent`, `SpanContext.ParentId`,
`SpanContext.ServiceName`. I don't _think_ that actually matters, as
these can't actually be accessed any more for technical reasons...

Part of stack 
- #6370
- #6376 👈 This PR
- #6385
- #6386
- #6397
- #6399
- #6405
veerbia pushed a commit that referenced this pull request Dec 16, 2024
…6385)

## Summary of changes

Remove `PublicApiTests.PublicApiHasNotChanged` for Datadog.Trace.dll

## Reason for change

The public API surface of Datadog.Trace is not _really_ public any more,
as it's not directly referenced, so these tests are largely
unneccessary.

As of right now, `public` vs `internal` is essentially irrelevant in
this project.

## Implementation details

Remove the `PublicApiHasNotChanged` test. The other public API tests
e.g. `AssemblyReferencesHaveNotChanged` _are_ still relevant, so jumped
through some hoops to keep them

## Test coverage

Slightly less now.

## Other details

Part of stack 

- #6370
- #6376
- #6385 👈 This PR
- #6386
- #6397
- #6399
- #6400
- #6405
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.

2 participants