Make `DirectLogSubmissionSettings` properly immutable and remove `ImmutableDirectLogSubmissionSettings` #6400

andrewlock · 2024-12-05T10:08:16Z

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

datadog-ddstaging · 2024-12-05T10:44:41Z

Datadog Report

Branch report: andrew/remove-loginjection-mutable-settings
Commit report: 9aaaa68
Test service: dd-trace-dotnet

✅ 0 Failed, 464099 Passed, 2846 Skipped, 37h 1m 13.3s Total Time

andrewlock · 2024-12-05T11:07:32Z

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

    section CallTarget+Inlining+NGEN
    This PR (6400) - mean (983ms)  : 957, 1009
     .   : milestone, 983,
    master - mean (988ms)  : 963, 1014
     .   : milestone, 988,

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

    section CallTarget+Inlining+NGEN
    This PR (6400) - mean (685ms)  : 669, 702
     .   : milestone, 685,
    master - mean (687ms)  : 669, 704
     .   : milestone, 687,

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

    section CallTarget+Inlining+NGEN
    This PR (6400) - mean (634ms)  : 619, 649
     .   : milestone, 634,
    master - mean (643ms)  : 626, 660
     .   : milestone, 643,

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

    section CallTarget+Inlining+NGEN
    This PR (6400) - mean (1,095ms)  : 1064, 1126
     .   : milestone, 1095,
    master - mean (1,091ms)  : 1063, 1119
     .   : milestone, 1091,

gantt
    title Execution time (ms) HttpMessageHandler (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6400) - mean (277ms)  : 271, 282
     .   : milestone, 277,
    master - mean (276ms)  : 272, 280
     .   : milestone, 276,

    section CallTarget+Inlining+NGEN
    This PR (6400) - mean (871ms)  : 841, 901
     .   : milestone, 871,
    master - mean (869ms)  : 839, 900
     .   : milestone, 869,

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

    section CallTarget+Inlining+NGEN
    This PR (6400) - mean (852ms)  : 824, 880
     .   : milestone, 852,
    master - mean (850ms)  : 818, 882
     .   : milestone, 850,

andrewlock · 2024-12-05T20:07:32Z

Benchmarks Report for appsec 🐌

Benchmarks for #6400 compared to master:

All benchmarks have the same speed
1 benchmarks have fewer 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 ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`AllCycleSimpleBody`	net6.0	197μs	117ns	436ns	2.85	0.0984	205.62 KB
master	`AllCycleSimpleBody`	netcoreapp3.1	302μs	111ns	400ns	2.88	0	213.06 KB
master	`AllCycleSimpleBody`	net472	266μs	276ns	1.03μs	38.1	2.83	240.26 KB
master	`AllCycleMoreComplexBody`	net6.0	226μs	98.9ns	370ns	2.93	0.113	209.13 KB
master	`AllCycleMoreComplexBody`	netcoreapp3.1	303μs	107ns	387ns	2.9	0	216.48 KB
master	`AllCycleMoreComplexBody`	net472	265μs	169ns	655ns	38.6	2.93	243.77 KB
master	`ObjectExtractorSimpleBody`	net6.0	142ns	0.143ns	0.554ns	0.00397	0	280 B
master	`ObjectExtractorSimpleBody`	netcoreapp3.1	195ns	0.181ns	0.676ns	0.00373	0	272 B
master	`ObjectExtractorSimpleBody`	net472	216ns	0.181ns	0.676ns	0.0446	0	281 B
master	`ObjectExtractorMoreComplexBody`	net6.0	2.9μs	1.08ns	4.18ns	0.0536	0	3.78 KB
master	`ObjectExtractorMoreComplexBody`	netcoreapp3.1	3.76μs	1.82ns	6.82ns	0.0508	0	3.69 KB
master	`ObjectExtractorMoreComplexBody`	net472	4.22μs	7.46ns	28.9ns	0.603	0.00639	3.8 KB
#6400	`AllCycleSimpleBody`	net6.0	213μs	80ns	299ns	2.89	0	204.98 KB
#6400	`AllCycleSimpleBody`	netcoreapp3.1	323μs	101ns	364ns	2.91	0	212.23 KB
#6400	`AllCycleSimpleBody`	net472	279μs	102ns	396ns	37.9	2.78	239.39 KB
#6400	`AllCycleMoreComplexBody`	net6.0	218μs	64.3ns	249ns	2.95	0	208.49 KB
#6400	`AllCycleMoreComplexBody`	netcoreapp3.1	321μs	116ns	435ns	2.88	0	215.65 KB
#6400	`AllCycleMoreComplexBody`	net472	284μs	149ns	575ns	38.6	2.88	242.91 KB
#6400	`ObjectExtractorSimpleBody`	net6.0	143ns	0.109ns	0.407ns	0.00397	0	280 B
#6400	`ObjectExtractorSimpleBody`	netcoreapp3.1	203ns	0.105ns	0.408ns	0.00365	0	272 B
#6400	`ObjectExtractorSimpleBody`	net472	210ns	0.0768ns	0.277ns	0.0446	0	281 B
#6400	`ObjectExtractorMoreComplexBody`	net6.0	2.85μs	1.07ns	3.86ns	0.0528	0	3.78 KB
#6400	`ObjectExtractorMoreComplexBody`	netcoreapp3.1	3.69μs	7.26ns	27.2ns	0.0502	0	3.69 KB
#6400	`ObjectExtractorMoreComplexBody`	net472	4.34μs	4.4ns	16.5ns	0.602	0.00649	3.8 KB

Benchmarks.Trace.Asm.AppSecEncoderBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`EncodeArgs`	net6.0	37.5μs	26.1ns	101ns	0.451	0	32.4 KB
master	`EncodeArgs`	netcoreapp3.1	54.7μs	14.7ns	54.9ns	0.435	0	32.4 KB
master	`EncodeArgs`	net472	67.8μs	43.9ns	164ns	5.16	0.0679	32.5 KB
master	`EncodeLegacyArgs`	net6.0	75.8μs	385ns	1.72μs	0	0	2.14 KB
master	`EncodeLegacyArgs`	netcoreapp3.1	105μs	425ns	1.53μs	0	0	2.14 KB
master	`EncodeLegacyArgs`	net472	156μs	102ns	396ns	0.317	0	2.15 KB
#6400	`EncodeArgs`	net6.0	38.8μs	14.5ns	54.4ns	0.449	0	32.4 KB
#6400	`EncodeArgs`	netcoreapp3.1	54.1μs	28.6ns	111ns	0.431	0	32.4 KB
#6400	`EncodeArgs`	net472	67.8μs	63.9ns	248ns	5.14	0.0676	32.5 KB
#6400	`EncodeLegacyArgs`	net6.0	76μs	35.3ns	137ns	0.0374	0	2.14 KB
#6400	`EncodeLegacyArgs`	netcoreapp3.1	106μs	75.2ns	281ns	0	0	2.14 KB
#6400	`EncodeLegacyArgs`	net472	157μs	100ns	389ns	0.313	0	2.15 KB

Benchmarks.Trace.Asm.AppSecWafBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`RunWafRealisticBenchmark`	net6.0	182μs	159ns	616ns	0	2.44 KB
master	`RunWafRealisticBenchmark`	netcoreapp3.1	194μs	128ns	461ns	0	2.39 KB
master	`RunWafRealisticBenchmark`	net472	207μs	50.4ns	189ns	0.309	2.46 KB
master	`RunWafRealisticBenchmarkWithAttack`	net6.0	122μs	60.4ns	234ns	0	1.47 KB
master	`RunWafRealisticBenchmarkWithAttack`	netcoreapp3.1	130μs	182ns	703ns	0	1.46 KB
master	`RunWafRealisticBenchmarkWithAttack`	net472	140μs	58.1ns	217ns	0.21	1.49 KB
#6400	`RunWafRealisticBenchmark`	net6.0	182μs	69.2ns	259ns	0	2.44 KB
#6400	`RunWafRealisticBenchmark`	netcoreapp3.1	195μs	114ns	427ns	0	2.39 KB
#6400	`RunWafRealisticBenchmark`	net472	208μs	59.4ns	214ns	0.313	2.46 KB
#6400	`RunWafRealisticBenchmarkWithAttack`	net6.0	122μs	43.2ns	167ns	0	1.47 KB
#6400	`RunWafRealisticBenchmarkWithAttack`	netcoreapp3.1	129μs	85.9ns	333ns	0	1.46 KB
#6400	`RunWafRealisticBenchmarkWithAttack`	net472	140μs	45.5ns	170ns	0.21	1.49 KB

Benchmarks.Trace.Iast.StringAspectsBenchmark - Same speed ✔️ Fewer allocations 🎉

Fewer allocations 🎉 in #6400

Benchmark	Base Allocated	Diff Allocated	Change	Change %
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatBenchmark‑net472	59.6 KB	58.57 KB	-1.03 KB	-1.73%

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Allocated
master	`StringConcatBenchmark`	net6.0	62.5μs	729ns	7.25μs	43.44 KB
master	`StringConcatBenchmark`	netcoreapp3.1	54.4μs	187ns	673ns	42.64 KB
master	`StringConcatBenchmark`	net472	38μs	67.7ns	253ns	59.6 KB
master	`StringConcatAspectBenchmark`	net6.0	307μs	1.38μs	6.62μs	253.6 KB
master	`StringConcatAspectBenchmark`	netcoreapp3.1	347μs	1.71μs	11.2μs	253.34 KB
master	`StringConcatAspectBenchmark`	net472	290μs	5.46μs	53.2μs	278.53 KB
#6400	`StringConcatBenchmark`	net6.0	60.1μs	713ns	7.13μs	43.44 KB
#6400	`StringConcatBenchmark`	netcoreapp3.1	63μs	854ns	8.36μs	42.64 KB
#6400	`StringConcatBenchmark`	net472	38.4μs	64.1ns	222ns	58.57 KB
#6400	`StringConcatAspectBenchmark`	net6.0	317μs	1.72μs	10.6μs	254.03 KB
#6400	`StringConcatAspectBenchmark`	netcoreapp3.1	351μs	1.99μs	14.9μs	254.47 KB
#6400	`StringConcatAspectBenchmark`	net472	304μs	7.23μs	70.9μs	278.53 KB

andrewlock · 2024-12-05T20:59:26Z

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 (6400) (11.267M)   : 0, 11266725
    master (11.285M)   : 0, 11285052
    benchmarks/2.9.0 (11.033M)   : 0, 11032866

    section Automatic
    This PR (6400) (7.225M)   : 0, 7225438
    master (7.360M)   : 0, 7360083
    benchmarks/2.9.0 (7.786M)   : 0, 7785853

    section Trace stats
    master (7.611M)   : 0, 7610747

    section Manual
    master (10.966M)   : 0, 10965897

    section Manual + Automatic
    This PR (6400) (6.825M)   : 0, 6825096
    master (6.642M)   : 0, 6642442

    section DD_TRACE_ENABLED=0
    master (10.269M)   : 0, 10269412

gantt
    title Throughput Linux arm64 (Total requests) 
    dateFormat  X
    axisFormat %s
    section Baseline
    This PR (6400) (9.462M)   : 0, 9461712
    master (9.480M)   : 0, 9479670
    benchmarks/2.9.0 (9.495M)   : 0, 9494821

    section Automatic
    This PR (6400) (6.409M)   : 0, 6409397
    master (6.408M)   : 0, 6407802

    section Trace stats
    master (6.473M)   : 0, 6472722

    section Manual
    master (9.550M)   : 0, 9550264

    section Manual + Automatic
    This PR (6400) (6.062M)   : 0, 6061744
    master (5.869M)   : 0, 5868503

    section DD_TRACE_ENABLED=0
    master (8.866M)   : 0, 8866340

gantt
    title Throughput Windows x64 (Total requests) 
    dateFormat  X
    axisFormat %s
    section Baseline
    This PR (6400) (9.380M)   : 0, 9380199
    master (9.668M)   : 0, 9668038
    benchmarks/2.9.0 (10.020M)   : 0, 10019592

    section Automatic
    This PR (6400) (6.207M)   : 0, 6206557
    master (6.298M)   : 0, 6298364
    benchmarks/2.9.0 (7.255M)   : 0, 7255257

    section Trace stats
    master (6.837M)   : 0, 6837280

    section Manual
    master (9.553M)   : 0, 9553067

    section Manual + Automatic
    This PR (6400) (5.940M)   : 0, 5940425
    master (6.020M)   : 0, 6019675

    section DD_TRACE_ENABLED=0
    master (9.077M)   : 0, 9076937

andrewlock · 2024-12-06T00:44:50Z

Benchmarks Report for tracer 🐌

Benchmarks for #6400 compared to master:

1 benchmarks are faster, with geometric mean 1.144
1 benchmarks are slower, with geometric mean 1.159
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	7.89μs	44.4ns	301ns	0.0124	0.00415	0	5.61 KB
master	`StartStopWithChild`	netcoreapp3.1	10.2μs	55ns	316ns	0.0146	0.00486	0	5.8 KB
master	`StartStopWithChild`	net472	16.4μs	50.5ns	196ns	1.03	0.307	0.097	6.2 KB
#6400	`StartStopWithChild`	net6.0	8.21μs	45.7ns	300ns	0.012	0.00398	0	5.61 KB
#6400	`StartStopWithChild`	netcoreapp3.1	10μs	54.3ns	302ns	0.0193	0.00967	0	5.8 KB
#6400	`StartStopWithChild`	net472	16.2μs	42.8ns	160ns	1.05	0.315	0.105	6.21 KB

Benchmarks.Trace.AgentWriterBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`WriteAndFlushEnrichedTraces`	net6.0	507μs	240ns	897ns	0	2.7 KB
master	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	662μs	587ns	2.2μs	0	2.7 KB
master	`WriteAndFlushEnrichedTraces`	net472	847μs	603ns	2.34μs	0.425	3.3 KB
#6400	`WriteAndFlushEnrichedTraces`	net6.0	490μs	283ns	1.1μs	0	2.7 KB
#6400	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	665μs	311ns	1.12μs	0	2.7 KB
#6400	`WriteAndFlushEnrichedTraces`	net472	851μs	642ns	2.49μs	0.428	3.3 KB

Benchmarks.Trace.AspNetCoreBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendRequest`	net6.0	147μs	926ns	9.16μs	0.147	14.47 KB
master	`SendRequest`	netcoreapp3.1	165μs	940ns	6.84μs	0.161	17.27 KB
master	`SendRequest`	net472	0.00054ns	0.000248ns	0.000929ns	0	0 b
#6400	`SendRequest`	net6.0	150μs	886ns	8.77μs	0.149	14.47 KB
#6400	`SendRequest`	netcoreapp3.1	165μs	950ns	8.06μs	0.157	17.27 KB
#6400	`SendRequest`	net472	0.00375ns	0.00119ns	0.00446ns	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	572μs	2.85μs	12.1μs	0.573	0	0	41.8 KB
master	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	657μs	2.39μs	8.94μs	0.332	0	0	41.61 KB
master	`WriteAndFlushEnrichedTraces`	net472	837μs	3.61μs	13.5μs	8.39	2.52	0.419	53.28 KB
#6400	`WriteAndFlushEnrichedTraces`	net6.0	546μs	1.73μs	6.48μs	0.536	0	0	41.74 KB
#6400	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	649μs	3.22μs	14μs	0.327	0	0	41.77 KB
#6400	`WriteAndFlushEnrichedTraces`	net472	836μs	4.22μs	20.3μs	8.22	2.47	0.411	53.33 KB

Benchmarks.Trace.DbCommandBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`ExecuteNonQuery`	net6.0	1.26μs	1.49ns	5.77ns	0.0139	0	1.02 KB
master	`ExecuteNonQuery`	netcoreapp3.1	1.76μs	1.22ns	4.58ns	0.0139	0	1.02 KB
master	`ExecuteNonQuery`	net472	2.03μs	1.08ns	4.2ns	0.157	0.00102	987 B
#6400	`ExecuteNonQuery`	net6.0	1.33μs	0.973ns	3.77ns	0.0139	0	1.02 KB
#6400	`ExecuteNonQuery`	netcoreapp3.1	1.77μs	1.92ns	7.45ns	0.0134	0	1.02 KB
#6400	`ExecuteNonQuery`	net472	2.06μs	1.12ns	4.32ns	0.156	0.00102	987 B

Benchmarks.Trace.ElasticsearchBenchmark - Slower ⚠️ Same allocations ✔️

Slower ⚠️ in #6400

Benchmark	diff/base	Base Median (ns)	Diff Median (ns)	Modality
Benchmarks.Trace.ElasticsearchBenchmark.CallElasticsearch‑net6.0	1.159	1,202.59	1,394.24

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`CallElasticsearch`	net6.0	1.2μs	0.435ns	1.69ns	0.0139	976 B
master	`CallElasticsearch`	netcoreapp3.1	1.58μs	0.79ns	3.06ns	0.0134	976 B
master	`CallElasticsearch`	net472	2.57μs	2.05ns	7.66ns	0.158	995 B
master	`CallElasticsearchAsync`	net6.0	1.3μs	0.556ns	2.15ns	0.0131	952 B
master	`CallElasticsearchAsync`	netcoreapp3.1	1.69μs	1.19ns	4.63ns	0.0135	1.02 KB
master	`CallElasticsearchAsync`	net472	2.64μs	0.971ns	3.63ns	0.167	1.05 KB
#6400	`CallElasticsearch`	net6.0	1.39μs	0.804ns	3.01ns	0.0136	976 B
#6400	`CallElasticsearch`	netcoreapp3.1	1.58μs	0.891ns	3.45ns	0.0132	976 B
#6400	`CallElasticsearch`	net472	2.45μs	1.42ns	5.3ns	0.158	995 B
#6400	`CallElasticsearchAsync`	net6.0	1.45μs	0.747ns	2.89ns	0.013	952 B
#6400	`CallElasticsearchAsync`	netcoreapp3.1	1.64μs	0.973ns	3.64ns	0.0133	1.02 KB
#6400	`CallElasticsearchAsync`	net472	2.52μs	1.51ns	5.45ns	0.167	1.05 KB

Benchmarks.Trace.GraphQLBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`ExecuteAsync`	net6.0	1.27μs	0.761ns	2.95ns	0.0134	952 B
master	`ExecuteAsync`	netcoreapp3.1	1.67μs	0.936ns	3.62ns	0.0123	952 B
master	`ExecuteAsync`	net472	1.83μs	0.553ns	2.14ns	0.145	915 B
#6400	`ExecuteAsync`	net6.0	1.3μs	1.94ns	7.24ns	0.013	952 B
#6400	`ExecuteAsync`	netcoreapp3.1	1.67μs	1.18ns	4.58ns	0.0125	952 B
#6400	`ExecuteAsync`	net472	1.93μs	0.74ns	2.86ns	0.145	915 B

Benchmarks.Trace.HttpClientBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendAsync`	net6.0	4.45μs	2.05ns	7.92ns	0.0318	2.31 KB
master	`SendAsync`	netcoreapp3.1	5.25μs	1.28ns	4.8ns	0.0368	2.85 KB
master	`SendAsync`	net472	7.36μs	1.45ns	5.42ns	0.496	3.12 KB
#6400	`SendAsync`	net6.0	4.43μs	3.05ns	11.4ns	0.0312	2.31 KB
#6400	`SendAsync`	netcoreapp3.1	5.23μs	2.72ns	10.6ns	0.0369	2.85 KB
#6400	`SendAsync`	net472	7.42μs	1.93ns	7.47ns	0.493	3.12 KB

Benchmarks.Trace.ILoggerBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net6.0	1.48μs	0.734ns	2.75ns	0.0231	1.64 KB
master	`EnrichedLog`	netcoreapp3.1	2.21μs	0.725ns	2.71ns	0.022	1.64 KB
master	`EnrichedLog`	net472	2.66μs	1.17ns	4.39ns	0.249	1.57 KB
#6400	`EnrichedLog`	net6.0	1.45μs	0.99ns	3.7ns	0.0233	1.64 KB
#6400	`EnrichedLog`	netcoreapp3.1	2.26μs	1.58ns	6.13ns	0.0225	1.64 KB
#6400	`EnrichedLog`	net472	2.7μs	1.53ns	5.73ns	0.249	1.57 KB

Benchmarks.Trace.Log4netBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`EnrichedLog`	net6.0	120μs	183ns	709ns	0.0608	0	4.28 KB
master	`EnrichedLog`	netcoreapp3.1	125μs	75.6ns	283ns	0	0	4.28 KB
master	`EnrichedLog`	net472	152μs	97.3ns	364ns	0.684	0.228	4.46 KB
#6400	`EnrichedLog`	net6.0	119μs	128ns	495ns	0.0597	0	4.28 KB
#6400	`EnrichedLog`	netcoreapp3.1	124μs	113ns	438ns	0.063	0	4.28 KB
#6400	`EnrichedLog`	net472	151μs	119ns	461ns	0.681	0.227	4.46 KB

Benchmarks.Trace.NLogBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net6.0	3.08μs	1.16ns	4.5ns	0.0308	2.2 KB
master	`EnrichedLog`	netcoreapp3.1	4.21μs	1.21ns	4.36ns	0.0296	2.2 KB
master	`EnrichedLog`	net472	4.94μs	2.46ns	9.53ns	0.319	2.02 KB
#6400	`EnrichedLog`	net6.0	2.96μs	1.3ns	4.85ns	0.0298	2.2 KB
#6400	`EnrichedLog`	netcoreapp3.1	4.33μs	1.64ns	6.34ns	0.0281	2.2 KB
#6400	`EnrichedLog`	net472	4.8μs	1.41ns	5.47ns	0.321	2.02 KB

Benchmarks.Trace.RedisBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendReceive`	net6.0	1.3μs	0.649ns	2.51ns	0.0161	1.14 KB
master	`SendReceive`	netcoreapp3.1	1.79μs	1.09ns	4.07ns	0.0153	1.14 KB
master	`SendReceive`	net472	2.11μs	0.805ns	2.79ns	0.183	1.16 KB
#6400	`SendReceive`	net6.0	1.44μs	0.821ns	3.07ns	0.0159	1.14 KB
#6400	`SendReceive`	netcoreapp3.1	1.72μs	1.23ns	4.45ns	0.0155	1.14 KB
#6400	`SendReceive`	net472	2.12μs	1.34ns	5.02ns	0.183	1.16 KB

Benchmarks.Trace.SerilogBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net6.0	2.8μs	0.721ns	2.7ns	0.0224	1.6 KB
master	`EnrichedLog`	netcoreapp3.1	3.88μs	2.46ns	9.51ns	0.0214	1.65 KB
master	`EnrichedLog`	net472	4.24μs	1.71ns	6.4ns	0.323	2.04 KB
#6400	`EnrichedLog`	net6.0	2.74μs	0.751ns	2.81ns	0.0219	1.6 KB
#6400	`EnrichedLog`	netcoreapp3.1	3.91μs	0.577ns	2ns	0.0215	1.65 KB
#6400	`EnrichedLog`	net472	4.33μs	1.96ns	7.34ns	0.324	2.04 KB

Benchmarks.Trace.SpanBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`StartFinishSpan`	net6.0	401ns	0.364ns	1.41ns	0.00807	576 B
master	`StartFinishSpan`	netcoreapp3.1	565ns	0.252ns	0.942ns	0.00766	576 B
master	`StartFinishSpan`	net472	651ns	0.474ns	1.83ns	0.0917	578 B
master	`StartFinishScope`	net6.0	502ns	0.321ns	1.24ns	0.00975	696 B
master	`StartFinishScope`	netcoreapp3.1	741ns	0.466ns	1.81ns	0.00927	696 B
master	`StartFinishScope`	net472	885ns	0.453ns	1.76ns	0.104	658 B
#6400	`StartFinishSpan`	net6.0	403ns	0.207ns	0.776ns	0.00805	576 B
#6400	`StartFinishSpan`	netcoreapp3.1	546ns	0.378ns	1.47ns	0.00792	576 B
#6400	`StartFinishSpan`	net472	711ns	0.334ns	1.25ns	0.0916	578 B
#6400	`StartFinishScope`	net6.0	509ns	0.369ns	1.43ns	0.00968	696 B
#6400	`StartFinishScope`	netcoreapp3.1	700ns	0.426ns	1.6ns	0.00947	696 B
#6400	`StartFinishScope`	net472	838ns	0.509ns	1.97ns	0.104	658 B

Benchmarks.Trace.TraceAnnotationsBenchmark - Faster 🎉 Same allocations ✔️

Faster 🎉 in #6400

Benchmark	base/diff	Base Median (ns)	Diff Median (ns)	Modality
Benchmarks.Trace.TraceAnnotationsBenchmark.RunOnMethodBegin‑net6.0	1.144	667.76	583.64

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`RunOnMethodBegin`	net6.0	668ns	0.918ns	3.56ns	0.00967	696 B
master	`RunOnMethodBegin`	netcoreapp3.1	934ns	1.5ns	5.59ns	0.00928	696 B
master	`RunOnMethodBegin`	net472	1.14μs	0.959ns	3.71ns	0.105	658 B
#6400	`RunOnMethodBegin`	net6.0	584ns	0.424ns	1.64ns	0.00968	696 B
#6400	`RunOnMethodBegin`	netcoreapp3.1	916ns	0.822ns	3.19ns	0.00933	696 B
#6400	`RunOnMethodBegin`	net472	1.14μs	0.671ns	2.6ns	0.104	658 B

…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

## Summary of changes - Delete legacy `IConfigurationSource` - Rename `ITelmeteredConfigurationSource` => `IConfigurationSource` - Remove some "public" API workarounds - Make some APIs `public` instead of `internal` (just to satisfy compiler) - Fix tests ## Reason for change `ITelmeteredConfigurationSource` was introduced when `IConfigurationSource` was public, and so could not be changed. We have since removed that interface from the public API (it's not in `Datadog.trace.Manual`) so now it simply adds confusion and complexity (as it should _not_ be used internally in Datadog.Trace). ## Implementation details This PR looks more complex than it is, because in order to replace all the usages of the old `IConfigurationSource` with the new one, I had to mark some other APIs public (and document them to satisfy the analyzers). It is mostly - Delete legacy `IConfigurationSource` - Rename `ITelmeteredConfigurationSource` => `IConfigurationSource` - Fix any errors I took the opportunity to also remove some of the `*Internal` methods which were used to avoid calling the "public" APIs; seeing as these aren't _actually_ public any more, that's just unnecessary duplication. ## Test coverage The testing was a pain. The `ConfigurationBuilder` tests were all designed to check that `ITelmeteredConfigurationSource` gave the same results as `IConfigurationSource` (while never _explicitly_ specifying what the "expected" behaviour was. I took some time to enumerate the _actual_ expected values for the `NameValueCollection` source, but the `Json` source has very different behaviour that is more of a pain to test, so I chose to simplify a lot there. We could do a _lot_ to clean up those tests, but I didn't want to add even more complexity in this PR. ## Other details To help "fix" the ASM tests I introduced a `DictionaryObjectConfigurationSource`, in which you pass in a `Dictionary<string, object>`. This is useful for testing (as it was previously used) but is actually going to be useful for a future clean-up PR too, so I kept it in Datadog.Trace instead of in the test project. Part of stack - #6370 - #6376 - #6385 - #6386 👈 This PR - #6397 - #6399 - #6400 - #6405 - #6408

…nSource` (#6397) Change how we "apply" settings from manual configuration to the automatic side to use `IConfigurationSource` ## Reason for change The previous design of how we "apply" settings made in code using manual instrumentation required mutating a `TracerSettings` object after it was already built. In fact, this is pretty much the _only_ place that we mutate the settings. By switching to using a "configuration source" approach, so that the settings are built _once_ with the correct values opens up the option to make these immutable (and therefore delete all of the `Immutable*` settings we currently have. This reduces both code duplication and work we do on startup, and opens the path to further refactoring improvements. Note that the public API does not change, so consumers of Datadog.Trace.Manual are still working with a mutable object initially. ## Implementation details Currently we pass a `Dictionary<string, object>` between the manual and automatic side. Previously, we then iterated the keys, compared against known values, and modified `TracerSettings` as required. With the changes, we have a `ManualInstrumentationConfigurationSource` which just "wraps" the `Dictionary<>`, and returns the correct value as required for a given key. Overall, I think this is much cleaner. Where things get messy is how we handle disabling specific integrations. The existing dictionary is optimised for looping through the provided values, fetching the setting that needs to be modified, and changing all the required properties. Unfortunately, the `IConfigurationSource` approach where we're looking up by a key like `DD_TRACE_NPGSQL_ENABLED` works _horribly_ for this pattern 🙁. So I introduced an additional approach which explicitly _additionally_ transfers the settings using these values, making them just "standard" lookups. > Note that due to backwards + forwards compatibility requirements > - We _still_ need to send the "old" version of integration settings from the manual side, in case it's used with an old version of the auto instrumentation > - We _still_ need to handle the "old" version of integration settings in the auto side, in case it's used with an old version of the manual instrumentation. > - At least in this case we can use the more efficient `IConfigurationSource` reader, so we don't pay the expense of retrieving the settings. The only downside is a couple of extra allocations when they _do_ disable integrations in code. Minor other changes: - Add helper ctor to `CompositeConfigurationSource` for creating the internal list from a collection of `IConfigurationSource` - Tweak `DictionaryObjectConfigurationSource` so we can derive from it - Create a separate integration for <3.7.0 manual instrumentation that uses the legacy settings, otherwise use the new settings objects ## Test coverage Mostly covered by existing unit tests (and indirectly by integration tests). Tweaked the test to test both the new and legacy configuration source. ## Other details Requires #6393 to fix how we read integration settings Part of stack - #6370 - #6376 - #6385 - #6386 - #6397 👈 This PR - #6399 - #6400 - #6405 - #6408

Use configuration source instead

…bleDirectLogSubmissionSettings

@tonyredondo

…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

kevingosse

ngl, I'm not super confident about this

tracer/src/Datadog.Trace/Ci/CIVisibility.cs

kevingosse · 2024-12-10T10:47:06Z

tracer/src/Datadog.Trace/Configuration/TracerSettings.cs

+                }
+
+                // Normalize the service name
+                ciVisServiceName = NormalizerTraceProcessor.NormalizeService(ciVisServiceName);


Maybe we should move the debug log somewhere around there

IMO not required. It gets logged by the tracer manager and in telemetry anyway 🤷‍♂️

tracer/src/Datadog.Trace/Logging/DirectSubmission/DirectLogSubmissionSettings.cs

tracer/src/Datadog.Trace/Ci/CIVisibility.cs

…tegrationSettings` (#6405) ## Summary of changes Merge `IntegrationSettings` with `ImmutableIntegrationSettings` ## Reason for change This stack of PRs is about doing one-shot configuration instead of mutation. We never mutate these in the tracer after creation, so there's no need for the separate types. ## Implementation details - Moved additional logic (handling `DisabledNames` that was previously inside `ImmutableIntegrationSettings` into `IntegrationSettings` - Replace all usages of `Immutable*` with `IntegrationSettings` - Move `Immutable*Tests` into appropriate file and tweak - Replace mutations with initialization - Reorder initialization of `DisabledIntegrationNames` in `TracerSettings` so that it can be used in the `IntegrationSettings` constructor ## Test coverage All covered by existing details ## Other details Part of Stack - #6370 - #6376 - #6385 - #6386 - #6397 - #6399 - #6400 - #6405 👈 This PR - #6408 - #6415

…terSettings` (#6408) ## Summary of changes Merge `ExporterSettings` with `ImmutableExporterSettings` ## Reason for change This stack of PRs is about doing one-shot configuration instead of mutation. We never mutate these in the tracer after creation, so there's no need for the separate types. ## Implementation details - Made the properties in `ExporterSettings` get-only. This required quite a lot of work because we were doing a lot of mutating of the settings in the "helper" functions. - I only _lightly_ refactored those methods (as much as possible) to avoid setting the properties in the functions and instead returning the details to set later. - These are prime candidates for some _much_ heavier refactoring later, but I didn't want to get bogged down with that in this PR - Replace all usages of `Immutable*` with `ExporterSettings` - Replace usages of `AgentUriInternal` with `AgentUri` - Move `Immutable*Tests` into appropriate file and tweak - Replace mutations with initialization ## Test coverage All covered by existing details ## Other details Part of Stack - #6370 - #6376 - #6385 - #6386 - #6397 - #6399 - #6400 - #6405 - #6408 👈 This PR - #6415

…ettings` (#6415) ## 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

…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

## Summary of changes - Delete legacy `IConfigurationSource` - Rename `ITelmeteredConfigurationSource` => `IConfigurationSource` - Remove some "public" API workarounds - Make some APIs `public` instead of `internal` (just to satisfy compiler) - Fix tests ## Reason for change `ITelmeteredConfigurationSource` was introduced when `IConfigurationSource` was public, and so could not be changed. We have since removed that interface from the public API (it's not in `Datadog.trace.Manual`) so now it simply adds confusion and complexity (as it should _not_ be used internally in Datadog.Trace). ## Implementation details This PR looks more complex than it is, because in order to replace all the usages of the old `IConfigurationSource` with the new one, I had to mark some other APIs public (and document them to satisfy the analyzers). It is mostly - Delete legacy `IConfigurationSource` - Rename `ITelmeteredConfigurationSource` => `IConfigurationSource` - Fix any errors I took the opportunity to also remove some of the `*Internal` methods which were used to avoid calling the "public" APIs; seeing as these aren't _actually_ public any more, that's just unnecessary duplication. ## Test coverage The testing was a pain. The `ConfigurationBuilder` tests were all designed to check that `ITelmeteredConfigurationSource` gave the same results as `IConfigurationSource` (while never _explicitly_ specifying what the "expected" behaviour was. I took some time to enumerate the _actual_ expected values for the `NameValueCollection` source, but the `Json` source has very different behaviour that is more of a pain to test, so I chose to simplify a lot there. We could do a _lot_ to clean up those tests, but I didn't want to add even more complexity in this PR. ## Other details To help "fix" the ASM tests I introduced a `DictionaryObjectConfigurationSource`, in which you pass in a `Dictionary<string, object>`. This is useful for testing (as it was previously used) but is actually going to be useful for a future clean-up PR too, so I kept it in Datadog.Trace instead of in the test project. Part of stack - #6370 - #6376 - #6385 - #6386 👈 This PR - #6397 - #6399 - #6400 - #6405 - #6408

…nSource` (#6397) Change how we "apply" settings from manual configuration to the automatic side to use `IConfigurationSource` ## Reason for change The previous design of how we "apply" settings made in code using manual instrumentation required mutating a `TracerSettings` object after it was already built. In fact, this is pretty much the _only_ place that we mutate the settings. By switching to using a "configuration source" approach, so that the settings are built _once_ with the correct values opens up the option to make these immutable (and therefore delete all of the `Immutable*` settings we currently have. This reduces both code duplication and work we do on startup, and opens the path to further refactoring improvements. Note that the public API does not change, so consumers of Datadog.Trace.Manual are still working with a mutable object initially. ## Implementation details Currently we pass a `Dictionary<string, object>` between the manual and automatic side. Previously, we then iterated the keys, compared against known values, and modified `TracerSettings` as required. With the changes, we have a `ManualInstrumentationConfigurationSource` which just "wraps" the `Dictionary<>`, and returns the correct value as required for a given key. Overall, I think this is much cleaner. Where things get messy is how we handle disabling specific integrations. The existing dictionary is optimised for looping through the provided values, fetching the setting that needs to be modified, and changing all the required properties. Unfortunately, the `IConfigurationSource` approach where we're looking up by a key like `DD_TRACE_NPGSQL_ENABLED` works _horribly_ for this pattern 🙁. So I introduced an additional approach which explicitly _additionally_ transfers the settings using these values, making them just "standard" lookups. > Note that due to backwards + forwards compatibility requirements > - We _still_ need to send the "old" version of integration settings from the manual side, in case it's used with an old version of the auto instrumentation > - We _still_ need to handle the "old" version of integration settings in the auto side, in case it's used with an old version of the manual instrumentation. > - At least in this case we can use the more efficient `IConfigurationSource` reader, so we don't pay the expense of retrieving the settings. The only downside is a couple of extra allocations when they _do_ disable integrations in code. Minor other changes: - Add helper ctor to `CompositeConfigurationSource` for creating the internal list from a collection of `IConfigurationSource` - Tweak `DictionaryObjectConfigurationSource` so we can derive from it - Create a separate integration for <3.7.0 manual instrumentation that uses the legacy settings, otherwise use the new settings objects ## Test coverage Mostly covered by existing unit tests (and indirectly by integration tests). Tweaked the test to test both the new and legacy configuration source. ## Other details Requires #6393 to fix how we read integration settings Part of stack - #6370 - #6376 - #6385 - #6386 - #6397 👈 This PR - #6399 - #6400 - #6405 - #6408

@tonyredondo

…tead of mutating (#6399) ## Summary of changes Change how CI Visibility creates its `TracerSettings` object to avoid mutating settings afterwards ## Reason for change This stack of changes is about removing duplication (among other things) in the `TracerSettings` etc related objects. This is _partially_ required in order to remove the "snapshot generator" complexity that was removed in #6370. Given `TracerSettings` are not exposed publicly in Datadog.Trace, we want to move to doing a "one shot" configuring of them, ultimately so that we can make the object immutable (and remove `ImmutableTracerSettings` and friends). ## Implementation details CI Visibility is one of the few places where we mutate settings _after_ creating the `TracerSettings`. This is mostly because there's additional logic that CI Visibility wants to perform. Ultimately, the "solution" to that issue in this PR is to move that logic into the `TracerSettings` constructor. I'm not entirely happy about it, but it's the only approach I could find that works. - Add a "dummy" configuration value to a configuration source when creating the `TracerSettings`. This is used purely as a "switch" to say "we're in CI Visibility mode". - We absolutely could just pass this in as a constructor parameter. I went that route first and then backed away, but can totally be swayed. - Add an additional configuration source to update settings that we want to change in CI Vis (e.g. logs injection). - In the `TracerSettings` ctor, add an additional ignored URL when in CI Vis mode, modify the default service name (if required) and add an additional GlobalTag. ## Test coverage I'd love to have some, but the CI Visibility configuration is kinda all over the place, so if you have any pointers @tonyredondo I'm all ears... ## Other details Part of stack - #6370 - #6376 - #6385 - #6386 - #6397 - #6399 👈 This PR - #6400 - #6405 - #6408

…utableDirectLogSubmissionSettings` (#6400) ## Summary of changes Merge `DirectLogSubmissionSettings` with `ImmutableDirectLogSubmissionSettings` ## Reason for change There was never really a good reason for having these as separate types. It was primarily to make testing a little easier and to mirror the `TracerSettings`/`ImmutableTracerSettings` dichotomy, but as that's going away, this is just unnecessary complexity ## Implementation details - Moved the additional logic that was previously inside `ImmutableDirectLogSubmissionSettings` into `DirectLogSubmissionSettings` - Renamed the `DirectLogSubmissionSettings` properties to match the "Immutable" version (and remove the unnecessary prefix) - Replace all usages of `Immutable*` with `DirectLogSubmissionSettings` - Move `Immutable*Tests` into appropriate file and tweak - Replace mutations with initialization (using `IConfigurationSource`) ## Test coverage Essentially the same. I removed/tweaked some tests that are no longer relevant ## Other details Part of stack - #6370 - #6376 - #6385 - #6386 - #6397 - #6399 - #6400 👈 This PR - #6405 - #6408 - #6415

…tegrationSettings` (#6405) ## Summary of changes Merge `IntegrationSettings` with `ImmutableIntegrationSettings` ## Reason for change This stack of PRs is about doing one-shot configuration instead of mutation. We never mutate these in the tracer after creation, so there's no need for the separate types. ## Implementation details - Moved additional logic (handling `DisabledNames` that was previously inside `ImmutableIntegrationSettings` into `IntegrationSettings` - Replace all usages of `Immutable*` with `IntegrationSettings` - Move `Immutable*Tests` into appropriate file and tweak - Replace mutations with initialization - Reorder initialization of `DisabledIntegrationNames` in `TracerSettings` so that it can be used in the `IntegrationSettings` constructor ## Test coverage All covered by existing details ## Other details Part of Stack - #6370 - #6376 - #6385 - #6386 - #6397 - #6399 - #6400 - #6405 👈 This PR - #6408 - #6415

…terSettings` (#6408) ## Summary of changes Merge `ExporterSettings` with `ImmutableExporterSettings` ## Reason for change This stack of PRs is about doing one-shot configuration instead of mutation. We never mutate these in the tracer after creation, so there's no need for the separate types. ## Implementation details - Made the properties in `ExporterSettings` get-only. This required quite a lot of work because we were doing a lot of mutating of the settings in the "helper" functions. - I only _lightly_ refactored those methods (as much as possible) to avoid setting the properties in the functions and instead returning the details to set later. - These are prime candidates for some _much_ heavier refactoring later, but I didn't want to get bogged down with that in this PR - Replace all usages of `Immutable*` with `ExporterSettings` - Replace usages of `AgentUriInternal` with `AgentUri` - Move `Immutable*Tests` into appropriate file and tweak - Replace mutations with initialization ## Test coverage All covered by existing details ## Other details Part of Stack - #6370 - #6376 - #6385 - #6386 - #6397 - #6399 - #6400 - #6405 - #6408 👈 This PR - #6415

…ettings` (#6415) Merge `TracerSettings` with `ImmutableTracerSettings` This stack of PRs is about doing one-shot configuration instead of mutation. We never mutate these in the tracer after creation, so there's no need for the separate types. - Make the properties in `TracerSettings` get-only. - Make the collections in `TracerSettings` readonly. - Move logic that used to be in the constructor of `ImmutableTracerSettings` into `TracerSettings` - e.g. Service/Version/Env were being changed based on DD_TAGS values. Moved that to TracerSettings and (importantly) added missing telemetry recording of these values. - Added missing recording of _effective_ `DisabledInstegrations` - Moving this logic caused some _tests_ to be broken (checking default values). Updated the expected values of those tests in a single - Replace all usages of `ImmutableTracerSettings` with `TracerSettings` - Move `ITracer` to Datadog.Trace.Manual - It's only used there, and references the manual-version of `ImmutableTracerSettings` which we _want_ to keep. - Move `Immutable*Tests` into appropriate file and tweak - Replace mutations with initialization in tests All covered by existing tests (I hope) 🤞 There's still a _lot_ of scope to improve this Part of Stack - #6370 - #6376 - #6385 - #6386 - #6397 - #6399 - #6400 - #6405 - #6408 - #6415 👈 This PR

andrewlock added area:tracer The core tracer library (Datadog.Trace, does not include OpenTracing, native code, or integrations) type:refactor area:direct-log-submission labels Dec 5, 2024

andrewlock requested a review from a team as a code owner December 5, 2024 10:08

andrewlock force-pushed the andrew/ci-vis-use-configuration-source branch from bcae049 to 8c4bbd5 Compare December 5, 2024 12:52

andrewlock requested review from a team as code owners December 5, 2024 12:52

andrewlock force-pushed the andrew/remove-loginjection-mutable-settings branch from 5512fc4 to 276080c Compare December 5, 2024 12:52

andrewlock force-pushed the andrew/ci-vis-use-configuration-source branch from 8c4bbd5 to 32cf526 Compare December 5, 2024 15:07

andrewlock force-pushed the andrew/remove-loginjection-mutable-settings branch from 276080c to 969d02e Compare December 5, 2024 15:07

andrewlock mentioned this pull request Dec 5, 2024

Make IntegrationSettings properly immutable and remove ImmutableIntegrationSettings #6405

Merged

andrewlock force-pushed the andrew/ci-vis-use-configuration-source branch from 32cf526 to bf1018d Compare December 6, 2024 11:49

andrewlock force-pushed the andrew/remove-loginjection-mutable-settings branch from 969d02e to 6cf9548 Compare December 6, 2024 11:49

andrewlock force-pushed the andrew/ci-vis-use-configuration-source branch from bf1018d to 455d139 Compare December 6, 2024 14:10

andrewlock force-pushed the andrew/remove-loginjection-mutable-settings branch from 6cf9548 to 0733b82 Compare December 6, 2024 14:10

andrewlock mentioned this pull request Dec 6, 2024

Make ExporterSettings properly immutable and remove ImmutableExporterSettings #6408

Merged

andrewlock force-pushed the andrew/ci-vis-use-configuration-source branch from 455d139 to c6a2380 Compare December 6, 2024 16:32

andrewlock force-pushed the andrew/remove-loginjection-mutable-settings branch from 0733b82 to db4f2d0 Compare December 6, 2024 16:32

andrewlock added 2 commits December 9, 2024 09:07

Remove modification of TracerSettings by CIVisibility

244bf25

Use configuration source instead

Make DirectLogSubmissionSettings properly immutable and remove Immuta…

9aaaa68

…bleDirectLogSubmissionSettings

andrewlock force-pushed the andrew/ci-vis-use-configuration-source branch from c6a2380 to 244bf25 Compare December 9, 2024 09:08

andrewlock force-pushed the andrew/remove-loginjection-mutable-settings branch from db4f2d0 to 9aaaa68 Compare December 9, 2024 09:08

Base automatically changed from andrew/ci-vis-use-configuration-source to master December 9, 2024 17:52

kevingosse approved these changes Dec 10, 2024

View reviewed changes

andrewlock mentioned this pull request Dec 10, 2024

Make TracerSettings properly immutable and remove ImmutableTracerSettings #6415

Merged

tonyredondo approved these changes Dec 10, 2024

View reviewed changes

tracer/src/Datadog.Trace/Ci/CIVisibility.cs Show resolved Hide resolved

tracer/src/Datadog.Trace/Ci/CIVisibility.cs Show resolved Hide resolved

andrewlock merged commit 21f2245 into master Dec 10, 2024
58 of 60 checks passed

andrewlock deleted the andrew/remove-loginjection-mutable-settings branch December 10, 2024 14:04

github-actions bot added this to the vNext-v3 milestone Dec 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `DirectLogSubmissionSettings` properly immutable and remove `ImmutableDirectLogSubmissionSettings` #6400

Make `DirectLogSubmissionSettings` properly immutable and remove `ImmutableDirectLogSubmissionSettings` #6400

andrewlock commented Dec 5, 2024 •

edited

Loading

datadog-ddstaging bot commented Dec 5, 2024 •

edited

Loading

andrewlock commented Dec 5, 2024 •

edited

Loading

andrewlock commented Dec 5, 2024 •

edited

Loading

Raw results

Raw results

Raw results

Fewer allocations 🎉 in #6400

Raw results

andrewlock commented Dec 5, 2024 •

edited

Loading

andrewlock commented Dec 6, 2024 •

edited

Loading

Raw results

Raw results

Raw results

Raw results

Raw results

Slower ⚠️ in #6400

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Faster 🎉 in #6400

Raw results

kevingosse left a comment

kevingosse Dec 10, 2024

andrewlock Dec 10, 2024

Make DirectLogSubmissionSettings properly immutable and remove ImmutableDirectLogSubmissionSettings #6400

Make DirectLogSubmissionSettings properly immutable and remove ImmutableDirectLogSubmissionSettings #6400

Conversation

andrewlock commented Dec 5, 2024 • edited Loading

Summary of changes

Reason for change

Implementation details

Test coverage

Other details

datadog-ddstaging bot commented Dec 5, 2024 • edited Loading

Datadog Report

andrewlock commented Dec 5, 2024 • edited Loading

Execution-Time Benchmarks Report ⏱️

andrewlock commented Dec 5, 2024 • edited Loading

Benchmarks Report for appsec 🐌

Benchmark details

Raw results

Raw results

Raw results

Fewer allocations 🎉 in #6400

Raw results

andrewlock commented Dec 5, 2024 • edited Loading

Throughput/Crank Report ⚡

andrewlock commented Dec 6, 2024 • edited Loading

Benchmarks Report for tracer 🐌

Benchmark details

Raw results

Raw results

Raw results

Raw results

Raw results

Slower ⚠️ in #6400

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Faster 🎉 in #6400

Raw results

kevingosse left a comment

Choose a reason for hiding this comment

kevingosse Dec 10, 2024

Choose a reason for hiding this comment

andrewlock Dec 10, 2024

Choose a reason for hiding this comment

Make `DirectLogSubmissionSettings` properly immutable and remove `ImmutableDirectLogSubmissionSettings` #6400

Make `DirectLogSubmissionSettings` properly immutable and remove `ImmutableDirectLogSubmissionSettings` #6400

andrewlock commented Dec 5, 2024 •

edited

Loading

datadog-ddstaging bot commented Dec 5, 2024 •

edited

Loading

andrewlock commented Dec 5, 2024 •

edited

Loading

andrewlock commented Dec 5, 2024 •

edited

Loading

andrewlock commented Dec 5, 2024 •

edited

Loading

andrewlock commented Dec 6, 2024 •

edited

Loading