Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove settings snapshot generator #6370

Merged
merged 4 commits into from
Dec 5, 2024

Conversation

andrewlock
Copy link
Member

@andrewlock andrewlock commented Nov 28, 2024

Summary of changes

  • Delete the tracer settings snapshot source generator
  • Delete the snapshot objects

Reason for change

We used these for tracking when customers use config in code so we know which properties they're changing. However, in v3 we explicitly set the values ourselves in the ConfigureIntegration so we may as well do all the work there

Implementation details

  • Delete the source generator
  • Delete usages of [GenerateSnapshot], [IgnoreForSnapshot], and [ConfigKey]
  • Delete TracerSettingsSnapshot and associated files and usages
  • Record config changes in ConfigureIntegration

Test coverage

Added a simple basic assertion to the manual instrumentation tests to assert that we're still recording the code origin

Other details

This is step 1 of a whole load of post-v3 code cleanup 😄

@andrewlock andrewlock added area:tracer The core tracer library (Datadog.Trace, does not include OpenTracing, native code, or integrations) type:cleanup Minor code clean up area:telemetry labels Nov 28, 2024
@andrewlock andrewlock requested review from a team as code owners November 28, 2024 16:01
@datadog-ddstaging
Copy link

datadog-ddstaging bot commented Nov 28, 2024

Datadog Report

Branch report: andrew/remove-snapshot-generation
Commit report: cee1ebb
Test service: dd-trace-dotnet

✅ 0 Failed, 453943 Passed, 2778 Skipped, 19h 23m 51.23s Total Time
❄️ 1 New Flaky

New Flaky Tests (1)

  • HttpClient_SubmitsTraces - Datadog.Trace.ClrProfiler.IntegrationTests.HttpMessageHandlerTests

@andrewlock
Copy link
Member Author

andrewlock commented Nov 28, 2024

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 (6370) - mean (69ms)  : 66, 71
     .   : milestone, 69,
    master - mean (69ms)  : 66, 72
     .   : milestone, 69,

    section CallTarget+Inlining+NGEN
    This PR (6370) - mean (971ms)  : 945, 998
     .   : milestone, 971,
    master - mean (983ms)  : 961, 1006
     .   : milestone, 983,

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

    section CallTarget+Inlining+NGEN
    This PR (6370) - mean (675ms)  : 655, 696
     .   : milestone, 675,
    master - mean (681ms)  : 663, 699
     .   : milestone, 681,

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

    section CallTarget+Inlining+NGEN
    This PR (6370) - mean (623ms)  : 606, 640
     .   : milestone, 623,
    master - mean (632ms)  : 619, 645
     .   : milestone, 632,

Loading
gantt
    title Execution time (ms) HttpMessageHandler (.NET Framework 4.6.2) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6370) - mean (191ms)  : 186, 196
     .   : milestone, 191,
    master - mean (190ms)  : 185, 195
     .   : milestone, 190,

    section CallTarget+Inlining+NGEN
    This PR (6370) - mean (1,085ms)  : 1059, 1111
     .   : milestone, 1085,
    master - mean (1,090ms)  : 1058, 1122
     .   : milestone, 1090,

Loading
gantt
    title Execution time (ms) HttpMessageHandler (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6370) - mean (276ms)  : 271, 282
     .   : milestone, 276,
    master - mean (275ms)  : 270, 281
     .   : milestone, 275,

    section CallTarget+Inlining+NGEN
    This PR (6370) - mean (867ms)  : 829, 904
     .   : milestone, 867,
    master - mean (869ms)  : 844, 894
     .   : milestone, 869,

Loading
gantt
    title Execution time (ms) HttpMessageHandler (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6370) - mean (266ms)  : 261, 270
     .   : milestone, 266,
    master - mean (265ms)  : 261, 269
     .   : milestone, 265,

    section CallTarget+Inlining+NGEN
    This PR (6370) - mean (841ms)  : 805, 876
     .   : milestone, 841,
    master - mean (847ms)  : 817, 878
     .   : milestone, 847,

Loading

@andrewlock andrewlock marked this pull request as draft November 28, 2024 17:12
@andrewlock
Copy link
Member Author

andrewlock commented Nov 28, 2024

Benchmarks Report for tracer 🐌

Benchmarks for #6370 compared to master:

  • 1 benchmarks are faster, with geometric mean 1.194
  • 3 benchmarks are slower, with geometric mean 1.156
  • 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 7.98μs 44.9ns 287ns 0.0159 0.00398 0 5.61 KB
master StartStopWithChild netcoreapp3.1 10.1μs 51.1ns 234ns 0.02 0.00998 0 5.8 KB
master StartStopWithChild net472 16.4μs 45.8ns 177ns 1.08 0.34 0.113 6.21 KB
#6370 StartStopWithChild net6.0 8.11μs 45.9ns 308ns 0.0117 0.00777 0 5.61 KB
#6370 StartStopWithChild netcoreapp3.1 10.4μs 55.2ns 297ns 0.0201 0.01 0 5.8 KB
#6370 StartStopWithChild net472 16.4μs 63.3ns 237ns 1.04 0.296 0.0881 6.21 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 491μs 434ns 1.68μs 0 0 0 2.7 KB
master WriteAndFlushEnrichedTraces netcoreapp3.1 665μs 521ns 2.02μs 0 0 0 2.7 KB
master WriteAndFlushEnrichedTraces net472 848μs 289ns 1.04μs 0.422 0 0 3.3 KB
#6370 WriteAndFlushEnrichedTraces net6.0 482μs 378ns 1.46μs 0 0 0 2.7 KB
#6370 WriteAndFlushEnrichedTraces netcoreapp3.1 658μs 572ns 2.06μs 0 0 0 2.7 KB
#6370 WriteAndFlushEnrichedTraces net472 845μs 457ns 1.71μs 0.419 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 150μs 865ns 7.29μs 0.147 0 0 14.47 KB
master SendRequest netcoreapp3.1 172μs 1.05μs 10.4μs 0.166 0 0 17.27 KB
master SendRequest net472 0.00145ns 0.000771ns 0.00299ns 0 0 0 0 b
#6370 SendRequest net6.0 150μs 809ns 5.66μs 0.141 0 0 14.47 KB
#6370 SendRequest netcoreapp3.1 162μs 931ns 7.33μs 0.159 0 0 17.27 KB
#6370 SendRequest net472 0.00408ns 0.00114ns 0.00442ns 0 0 0 0 b
Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark - Same speed ✔️ More allocations ⚠️

More allocations ⚠️ in #6370

Benchmark Base Allocated Diff Allocated Change Change %
Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark.WriteAndFlushEnrichedTraces‑net6.0 41.5 KB 41.79 KB 290 B 0.70%

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master WriteAndFlushEnrichedTraces net6.0 541μs 2.72μs 12.8μs 0.532 0 0 41.5 KB
master WriteAndFlushEnrichedTraces netcoreapp3.1 759μs 4.26μs 30.7μs 0.379 0 0 41.73 KB
master WriteAndFlushEnrichedTraces net472 864μs 4.24μs 18.5μs 8.36 2.64 0.44 53.28 KB
#6370 WriteAndFlushEnrichedTraces net6.0 573μs 2.62μs 11.7μs 0.548 0 0 41.79 KB
#6370 WriteAndFlushEnrichedTraces netcoreapp3.1 672μs 3.64μs 19.6μs 0.332 0 0 41.83 KB
#6370 WriteAndFlushEnrichedTraces net472 881μs 4.32μs 17.3μs 8.3 2.62 0.437 53.31 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.21μs 1.42ns 5.51ns 0.0145 0 0 1.02 KB
master ExecuteNonQuery netcoreapp3.1 1.79μs 1.2ns 4.63ns 0.0144 0 0 1.02 KB
master ExecuteNonQuery net472 1.95μs 1.61ns 6.01ns 0.157 0.000979 0 987 B
#6370 ExecuteNonQuery net6.0 1.28μs 0.963ns 3.73ns 0.0146 0 0 1.02 KB
#6370 ExecuteNonQuery netcoreapp3.1 1.74μs 3.07ns 11.9ns 0.0133 0 0 1.02 KB
#6370 ExecuteNonQuery net472 2.06μs 1.4ns 5.42ns 0.156 0.00104 0 987 B
Benchmarks.Trace.ElasticsearchBenchmark - Slower ⚠️ Same allocations ✔️

Slower ⚠️ in #6370

Benchmark diff/base Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.ElasticsearchBenchmark.CallElasticsearch‑net6.0 1.162 1,140.80 1,325.50

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master CallElasticsearch net6.0 1.14μs 2.25ns 8.41ns 0.0136 0 0 976 B
master CallElasticsearch netcoreapp3.1 1.54μs 0.751ns 2.91ns 0.0131 0 0 976 B
master CallElasticsearch net472 2.44μs 2.49ns 8.98ns 0.157 0 0 995 B
master CallElasticsearchAsync net6.0 1.22μs 0.398ns 1.49ns 0.0134 0 0 952 B
master CallElasticsearchAsync netcoreapp3.1 1.68μs 0.58ns 2.17ns 0.0135 0 0 1.02 KB
master CallElasticsearchAsync net472 2.54μs 0.491ns 1.84ns 0.167 0 0 1.05 KB
#6370 CallElasticsearch net6.0 1.33μs 0.378ns 1.42ns 0.0135 0 0 976 B
#6370 CallElasticsearch netcoreapp3.1 1.57μs 0.711ns 2.46ns 0.0134 0 0 976 B
#6370 CallElasticsearch net472 2.59μs 1.4ns 5.44ns 0.157 0 0 995 B
#6370 CallElasticsearchAsync net6.0 1.35μs 0.552ns 2.06ns 0.0134 0 0 952 B
#6370 CallElasticsearchAsync netcoreapp3.1 1.7μs 1.27ns 4.75ns 0.0135 0 0 1.02 KB
#6370 CallElasticsearchAsync net472 2.71μs 1.36ns 5.28ns 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.22μs 0.477ns 1.78ns 0.0135 0 0 952 B
master ExecuteAsync netcoreapp3.1 1.72μs 1.48ns 5.33ns 0.013 0 0 952 B
master ExecuteAsync net472 1.8μs 1.32ns 5.09ns 0.145 0 0 915 B
#6370 ExecuteAsync net6.0 1.3μs 1.52ns 5.69ns 0.013 0 0 952 B
#6370 ExecuteAsync netcoreapp3.1 1.71μs 0.931ns 3.48ns 0.0128 0 0 952 B
#6370 ExecuteAsync net472 1.83μs 1.92ns 7.43ns 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.32μs 1.81ns 7.03ns 0.0326 0 0 2.31 KB
master SendAsync netcoreapp3.1 5.26μs 1.85ns 6.93ns 0.0369 0 0 2.85 KB
master SendAsync net472 7.33μs 1.52ns 5.88ns 0.496 0 0 3.12 KB
#6370 SendAsync net6.0 4.38μs 1.42ns 5.49ns 0.033 0 0 2.31 KB
#6370 SendAsync netcoreapp3.1 5.32μs 2.7ns 10.1ns 0.0373 0 0 2.85 KB
#6370 SendAsync net472 7.2μs 1.92ns 7.42ns 0.493 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.775ns 2.79ns 0.0232 0 0 1.64 KB
master EnrichedLog netcoreapp3.1 2.21μs 0.803ns 3.11ns 0.022 0 0 1.64 KB
master EnrichedLog net472 2.51μs 0.932ns 3.61ns 0.249 0 0 1.57 KB
#6370 EnrichedLog net6.0 1.47μs 1.68ns 6.51ns 0.0231 0 0 1.64 KB
#6370 EnrichedLog netcoreapp3.1 2.17μs 0.992ns 3.71ns 0.0217 0 0 1.64 KB
#6370 EnrichedLog net472 2.54μs 0.98ns 3.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 120μs 96.9ns 375ns 0 0 0 4.28 KB
master EnrichedLog netcoreapp3.1 125μs 114ns 441ns 0 0 0 4.28 KB
master EnrichedLog net472 152μs 72.3ns 280ns 0.686 0.229 0 4.46 KB
#6370 EnrichedLog net6.0 118μs 245ns 949ns 0.0602 0 0 4.28 KB
#6370 EnrichedLog netcoreapp3.1 123μs 125ns 486ns 0 0 0 4.28 KB
#6370 EnrichedLog net472 151μs 181ns 653ns 0.678 0.226 0 4.46 KB
Benchmarks.Trace.NLogBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master EnrichedLog net6.0 3.04μs 1.05ns 3.93ns 0.031 0 0 2.2 KB
master EnrichedLog netcoreapp3.1 4.33μs 1.05ns 3.93ns 0.0299 0 0 2.2 KB
master EnrichedLog net472 4.84μs 1.87ns 7.24ns 0.32 0 0 2.02 KB
#6370 EnrichedLog net6.0 3.09μs 0.803ns 3.01ns 0.0309 0 0 2.2 KB
#6370 EnrichedLog netcoreapp3.1 4.22μs 1.98ns 7.67ns 0.0296 0 0 2.2 KB
#6370 EnrichedLog net472 4.79μs 1.27ns 4.93ns 0.319 0 0 2.02 KB
Benchmarks.Trace.RedisBenchmark - Slower ⚠️ Same allocations ✔️

Slower ⚠️ in #6370

Benchmark diff/base Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.RedisBenchmark.SendReceive‑net6.0 1.114 1,360.79 1,515.96

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master SendReceive net6.0 1.36μs 0.443ns 1.66ns 0.0163 0 0 1.14 KB
master SendReceive netcoreapp3.1 1.81μs 0.726ns 2.81ns 0.0156 0 0 1.14 KB
master SendReceive net472 1.99μs 2.37ns 9.19ns 0.183 0 0 1.16 KB
#6370 SendReceive net6.0 1.52μs 0.718ns 2.78ns 0.0158 0 0 1.14 KB
#6370 SendReceive netcoreapp3.1 1.76μs 1.32ns 5.12ns 0.015 0 0 1.14 KB
#6370 SendReceive net472 2.02μs 1.94ns 7.51ns 0.183 0 0 1.16 KB
Benchmarks.Trace.SerilogBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master EnrichedLog net6.0 2.75μs 2.96ns 11.1ns 0.0219 0 0 1.6 KB
master EnrichedLog netcoreapp3.1 3.94μs 1.63ns 6.31ns 0.0218 0 0 1.65 KB
master EnrichedLog net472 4.42μs 1.73ns 6.72ns 0.322 0 0 2.04 KB
#6370 EnrichedLog net6.0 2.74μs 2.64ns 10.2ns 0.0218 0 0 1.6 KB
#6370 EnrichedLog netcoreapp3.1 3.83μs 1.86ns 7.21ns 0.0211 0 0 1.65 KB
#6370 EnrichedLog net472 4.44μs 1.73ns 6.47ns 0.323 0 0 2.04 KB
Benchmarks.Trace.SpanBenchmark - Faster 🎉 Same allocations ✔️

Faster 🎉 in #6370

Benchmark base/diff Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.SpanBenchmark.StartFinishSpan‑net472 1.194 722.03 604.64

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master StartFinishSpan net6.0 403ns 0.472ns 1.83ns 0.00817 0 0 576 B
master StartFinishSpan netcoreapp3.1 585ns 0.612ns 2.29ns 0.00777 0 0 576 B
master StartFinishSpan net472 721ns 1.6ns 6.21ns 0.0915 0 0 578 B
master StartFinishScope net6.0 513ns 0.599ns 2.32ns 0.00977 0 0 696 B
master StartFinishScope netcoreapp3.1 686ns 1.26ns 4.89ns 0.00928 0 0 696 B
master StartFinishScope net472 873ns 1.75ns 6.77ns 0.105 0 0 658 B
#6370 StartFinishSpan net6.0 442ns 0.812ns 3.15ns 0.00799 0 0 576 B
#6370 StartFinishSpan netcoreapp3.1 637ns 3.42ns 18.1ns 0.0078 0 0 576 B
#6370 StartFinishSpan net472 605ns 0.717ns 2.87ns 0.0917 0 0 578 B
#6370 StartFinishScope net6.0 535ns 0.786ns 3.04ns 0.00986 0 0 696 B
#6370 StartFinishScope netcoreapp3.1 672ns 0.881ns 3.41ns 0.00944 0 0 696 B
#6370 StartFinishScope net472 848ns 2.03ns 7.86ns 0.104 0 0 658 B
Benchmarks.Trace.TraceAnnotationsBenchmark - Slower ⚠️ Same allocations ✔️

Slower ⚠️ in #6370

Benchmark diff/base Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.TraceAnnotationsBenchmark.RunOnMethodBegin‑net6.0 1.193 584.36 696.97

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master RunOnMethodBegin net6.0 584ns 0.775ns 3ns 0.00963 0 0 696 B
master RunOnMethodBegin netcoreapp3.1 838ns 1.27ns 4.92ns 0.0093 0 0 696 B
master RunOnMethodBegin net472 1.13μs 2.28ns 8.83ns 0.105 0 0 658 B
#6370 RunOnMethodBegin net6.0 697ns 0.881ns 3.41ns 0.00969 0 0 696 B
#6370 RunOnMethodBegin netcoreapp3.1 908ns 1.67ns 6.46ns 0.00933 0 0 696 B
#6370 RunOnMethodBegin net472 1.17μs 2.38ns 9.21ns 0.104 0 0 658 B

@andrewlock
Copy link
Member Author

andrewlock commented Nov 28, 2024

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 (6370) (11.236M)   : 0, 11236317
    master (10.950M)   : 0, 10950268
    benchmarks/2.9.0 (11.033M)   : 0, 11032866

    section Automatic
    This PR (6370) (7.340M)   : 0, 7340058
    master (7.144M)   : 0, 7144062
    benchmarks/2.9.0 (7.786M)   : 0, 7785853

    section Trace stats
    master (7.592M)   : 0, 7592116

    section Manual
    master (11.151M)   : 0, 11151368

    section Manual + Automatic
    This PR (6370) (6.742M)   : 0, 6741997
    master (6.721M)   : 0, 6721227

    section DD_TRACE_ENABLED=0
    master (10.133M)   : 0, 10132646

Loading
gantt
    title Throughput Linux arm64 (Total requests) 
    dateFormat  X
    axisFormat %s
    section Baseline
    This PR (6370) (9.540M)   : 0, 9539831
    master (9.581M)   : 0, 9581060
    benchmarks/2.9.0 (9.495M)   : 0, 9494821

    section Automatic
    This PR (6370) (6.377M)   : 0, 6376931
    master (6.411M)   : 0, 6411077

    section Trace stats
    master (6.557M)   : 0, 6557433

    section Manual
    master (9.539M)   : 0, 9539314

    section Manual + Automatic
    This PR (6370) (5.770M)   : 0, 5770420
    master (5.854M)   : 0, 5854294

    section DD_TRACE_ENABLED=0
    master (8.891M)   : 0, 8890821

Loading
gantt
    title Throughput Windows x64 (Total requests) 
    dateFormat  X
    axisFormat %s
    section Baseline
    This PR (6370) (9.874M)   : 0, 9874127
    master (10.194M)   : 0, 10193580
    benchmarks/2.9.0 (10.020M)   : 0, 10019592

    section Automatic
    This PR (6370) (6.266M)   : crit ,0, 6266172
    master (6.769M)   : 0, 6768819
    benchmarks/2.9.0 (7.255M)   : 0, 7255257

    section Trace stats
    master (7.357M)   : 0, 7356753

    section Manual
    master (10.447M)   : 0, 10447376

    section Manual + Automatic
    This PR (6370) (5.758M)   : 0, 5758009
    master (5.985M)   : 0, 5985300

    section DD_TRACE_ENABLED=0
    master (9.475M)   : 0, 9475300

Loading

@andrewlock andrewlock force-pushed the andrew/remove-snapshot-generation branch from 8a618f7 to b2e414e Compare November 29, 2024 10:17
@andrewlock andrewlock marked this pull request as ready for review November 29, 2024 10:18
@andrewlock andrewlock requested a review from a team as a code owner November 29, 2024 10:18
Copy link
Collaborator

@kevingosse kevingosse left a comment

Choose a reason for hiding this comment

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

I like when code is deleted

@andrewlock andrewlock force-pushed the andrew/remove-snapshot-generation branch from b2e414e to a9bea3f Compare November 29, 2024 13:40
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 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 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 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 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
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 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
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 tracer settings snapshot source generator
- Delete the snapshot objects

## Reason for change

We used these for tracking when customers use config in code so we know
which properties they're changing. However, in v3 we explicitly set the
values ourselves in the `ConfigureIntegration` so we may as well do all
the work there

## Implementation details

- Delete the source generator
- Delete usages of `[GenerateSnapshot]`, `[IgnoreForSnapshot]`, and
`[ConfigKey]`
- Delete `TracerSettingsSnapshot` and associated files and usages
- Record config changes in `ConfigureIntegration`

## Test coverage

Added a simple basic assertion to the manual instrumentation tests to
assert that we're still recording the `code` origin

## Other details

This is step 1 of a whole load of post-v3 code cleanup 😄 

- #6370 👈 This PR
- #6376
- #6385
- #6386
- #6397
- #6399
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:telemetry area:tracer The core tracer library (Datadog.Trace, does not include OpenTracing, native code, or integrations) type:cleanup Minor code clean up
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants