Avoid multiple configuration source enumeration by shurivich · Pull Request #3178 · DataDog/dd-trace-dotnet

shurivich · 2022-09-07T08:11:38Z

Summary of changes

Change GlobalSettings:

Add IConfigurationSource ConfigurationSource static property, and initialize it with CreateDefaultConfigurationSource method.
Remove all usages of CreateDefaultConfigurationSource to avoid multiple enumeration of configuration sources, and use ConfigurationSource property instead.
Rename DebuggerSource Source to Instance to match naming conventions of other single tones.

Reason for change

Avoid multiple enumeration of configuration sources

andrewlock

Looking good, just a couple of comments

I would try to decouple the default config source from GlobalSettings. It doesn't really belong there IMO
Some of GlobalSettings may be obsolete, might be worth looking into

andrewlock · 2022-09-13T09:40:24Z

Benchmarks Report 🐌

Benchmarks for #3178 compared to master:

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`WriteAndFlushEnrichedTraces`	net472	717μs	607ns	2.35μs	0.357	3.18 KB
master	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	471μs	645ns	2.5μs	0	2.59 KB
#3178	`WriteAndFlushEnrichedTraces`	net472	713μs	658ns	2.55μs	0.357	3.18 KB
#3178	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	454μs	410ns	1.59μs	0	2.58 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`AllCycleSimpleBody`	net472	201ns	0.302ns	1.17ns	0.0675	0	425 B
master	`AllCycleSimpleBody`	netcoreapp3.1	236ns	0.297ns	1.15ns	0.0058	0	424 B
master	`AllCycleMoreComplexBody`	net472	190ns	0.237ns	0.885ns	0.0637	0	401 B
master	`AllCycleMoreComplexBody`	netcoreapp3.1	232ns	0.195ns	0.756ns	0.00549	0	400 B
master	`BodyExtractorSimpleBody`	net472	276ns	0.472ns	1.76ns	0.0574	0	361 B
master	`BodyExtractorSimpleBody`	netcoreapp3.1	235ns	0.241ns	0.933ns	0.00367	0	272 B
master	`BodyExtractorMoreComplexBody`	net472	14.7μs	4.28ns	14.8ns	1.21	0.0148	7.62 KB
master	`BodyExtractorMoreComplexBody`	netcoreapp3.1	12μs	12.2ns	47.3ns	0.0899	0	6.75 KB
#3178	`AllCycleSimpleBody`	net472	183ns	0.206ns	0.797ns	0.0675	0	425 B
#3178	`AllCycleSimpleBody`	netcoreapp3.1	240ns	0.242ns	0.937ns	0.00572	0	424 B
#3178	`AllCycleMoreComplexBody`	net472	198ns	0.647ns	2.51ns	0.0637	0	401 B
#3178	`AllCycleMoreComplexBody`	netcoreapp3.1	232ns	0.305ns	1.18ns	0.00537	0	400 B
#3178	`BodyExtractorSimpleBody`	net472	262ns	0.268ns	1.04ns	0.0573	0	361 B
#3178	`BodyExtractorSimpleBody`	netcoreapp3.1	219ns	0.215ns	0.832ns	0.00379	0	272 B
#3178	`BodyExtractorMoreComplexBody`	net472	14.4μs	26.8ns	104ns	1.21	0.0215	7.62 KB
#3178	`BodyExtractorMoreComplexBody`	netcoreapp3.1	12μs	11.3ns	43.6ns	0.0897	0	6.75 KB

Benchmarks.Trace.AspNetCoreBenchmark - Unknown 🤷 Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendRequest`	net472	0ns	0ns	0ns	0	0 b
master	`SendRequest`	netcoreapp3.1	181μs	259ns	1μs	0.181	20.57 KB
#3178	`SendRequest`	net472	0ns	0ns	0ns	0	0 b
#3178	`SendRequest`	netcoreapp3.1	181μs	304ns	1.18μs	0.27	20.58 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`ExecuteNonQuery`	net472	1.89μs	2.64ns	9.87ns	0.151	0	947 B
master	`ExecuteNonQuery`	netcoreapp3.1	1.4μs	0.769ns	2.88ns	0.0121	0	936 B
#3178	`ExecuteNonQuery`	net472	1.89μs	2.74ns	10.3ns	0.15	0.000948	947 B
#3178	`ExecuteNonQuery`	netcoreapp3.1	1.5μs	5.62ns	21.8ns	0.0125	0	936 B

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`CallElasticsearch`	net472	2.54μs	1.36ns	5.28ns	0.183	1.16 KB
master	`CallElasticsearch`	netcoreapp3.1	1.5μs	0.587ns	2.12ns	0.015	1.1 KB
master	`CallElasticsearchAsync`	net472	2.77μs	2.51ns	9.71ns	0.205	1.29 KB
master	`CallElasticsearchAsync`	netcoreapp3.1	1.62μs	0.666ns	2.49ns	0.0162	1.22 KB
#3178	`CallElasticsearch`	net472	2.57μs	1.36ns	5.26ns	0.183	1.16 KB
#3178	`CallElasticsearch`	netcoreapp3.1	1.53μs	0.464ns	1.67ns	0.0146	1.1 KB
#3178	`CallElasticsearchAsync`	net472	2.75μs	1.76ns	6.83ns	0.205	1.29 KB
#3178	`CallElasticsearchAsync`	netcoreapp3.1	1.62μs	0.858ns	3.32ns	0.0161	1.22 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`ExecuteAsync`	net472	2.71μs	5.27ns	20.4ns	0.225	1.42 KB
master	`ExecuteAsync`	netcoreapp3.1	1.67μs	3.89ns	15.1ns	0.0183	1.34 KB
#3178	`ExecuteAsync`	net472	2.77μs	4.5ns	17.4ns	0.225	1.42 KB
#3178	`ExecuteAsync`	netcoreapp3.1	1.74μs	1.29ns	4.65ns	0.0183	1.34 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`SendAsync`	net472	5.68μs	13.4ns	52ns	0.438	0	2.77 KB
master	`SendAsync`	netcoreapp3.1	3.55μs	8.48ns	32.9ns	0.0359	0	2.6 KB
#3178	`SendAsync`	net472	5.7μs	11.9ns	46ns	0.44	0.00284	2.77 KB
#3178	`SendAsync`	netcoreapp3.1	3.49μs	7.65ns	29.6ns	0.036	0	2.6 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net472	3.07μs	1.55ns	6ns	0.287	1.81 KB
master	`EnrichedLog`	netcoreapp3.1	2.56μs	1.37ns	5.32ns	0.0248	1.85 KB
#3178	`EnrichedLog`	net472	3.19μs	3.85ns	14.9ns	0.287	1.81 KB
#3178	`EnrichedLog`	netcoreapp3.1	2.52μs	1.06ns	3.68ns	0.0251	1.85 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`EnrichedLog`	net472	152μs	180ns	650ns	0.687	0.229	4.65 KB
master	`EnrichedLog`	netcoreapp3.1	118μs	223ns	864ns	0	0	4.49 KB
#3178	`EnrichedLog`	net472	150μs	57ns	205ns	0.67	0.0745	4.66 KB
#3178	`EnrichedLog`	netcoreapp3.1	115μs	127ns	494ns	0.0579	0	4.49 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`EnrichedLog`	net472	5.57μs	9.78ns	37.9ns	0.57	0.00275	3.59 KB
master	`EnrichedLog`	netcoreapp3.1	4.33μs	5.75ns	21.5ns	0.052	0	3.91 KB
#3178	`EnrichedLog`	net472	5.57μs	10.2ns	36.8ns	0.568	0.00277	3.59 KB
#3178	`EnrichedLog`	netcoreapp3.1	4.26μs	6.63ns	24.8ns	0.0531	0	3.91 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendReceive`	net472	2.24μs	1.38ns	5.36ns	0.218	1.37 KB
master	`SendReceive`	netcoreapp3.1	1.76μs	1.03ns	3.7ns	0.0176	1.32 KB
#3178	`SendReceive`	net472	2.29μs	1.59ns	5.94ns	0.218	1.37 KB
#3178	`SendReceive`	netcoreapp3.1	1.86μs	1.33ns	4.96ns	0.0176	1.32 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`EnrichedLog`	net472	5.16μs	3.56ns	13.8ns	0.353	0	2.23 KB
master	`EnrichedLog`	netcoreapp3.1	4.35μs	2.09ns	8.11ns	0.024	0	1.8 KB
#3178	`EnrichedLog`	net472	5.06μs	2.63ns	10.2ns	0.353	0.00252	2.23 KB
#3178	`EnrichedLog`	netcoreapp3.1	4.29μs	4.56ns	17.1ns	0.0234	0	1.8 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`StartFinishSpan`	net472	1.19μs	0.775ns	3ns	0.128	810 B
master	`StartFinishSpan`	netcoreapp3.1	913ns	0.503ns	1.81ns	0.0099	760 B
master	`StartFinishScope`	net472	1.42μs	0.648ns	2.51ns	0.141	891 B
master	`StartFinishScope`	netcoreapp3.1	1.11μs	0.543ns	2.1ns	0.0116	880 B
#3178	`StartFinishSpan`	net472	1.16μs	0.275ns	0.991ns	0.128	810 B
#3178	`StartFinishSpan`	netcoreapp3.1	900ns	0.353ns	1.37ns	0.0105	760 B
#3178	`StartFinishScope`	net472	1.38μs	0.617ns	2.39ns	0.141	891 B
#3178	`StartFinishScope`	netcoreapp3.1	1.13μs	10.4ns	97.7ns	0.0118	880 B

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`RunOnMethodBegin`	net472	1.53μs	1.04ns	4.03ns	0.141	891 B
master	`RunOnMethodBegin`	netcoreapp3.1	1.21μs	0.373ns	1.44ns	0.012	880 B
#3178	`RunOnMethodBegin`	net472	1.51μs	0.847ns	3.28ns	0.141	891 B
#3178	`RunOnMethodBegin`	netcoreapp3.1	1.14μs	0.388ns	1.45ns	0.012	880 B

andrewlock · 2022-09-14T08:08:56Z

Code Coverage Report 📊

✔️ Merging #3178 into master will not change line coverage
✔️ Merging #3178 into master will not change branch coverage
✔️ Merging #3178 into master will will decrease complexity by 2

	master	#3178	Change
Lines	`17971` / `24517`	`17987` / `24512`
Lines %	`73%`	`73%`	`0%` ✔️
Branches	`10585` / `15050`	`10597` / `15046`
Branches %	`70%`	`70%`	`0%` ✔️
Complexity	`16261`	`16259`	`-2` ✔️

View the full report for further details:

Datadog.Trace Breakdown ✔️

	master	#3178	Change
Lines %	`73%`	`73%`	`0%` ✔️
Branches %	`70%`	`70%`	`0%` ✔️
Complexity	`16261`	`16259`	`-2` ✔️

The following classes have significant coverage changes.

File	Line coverage change	Branch coverage change	Complexity change
Datadog.Trace.RemoteConfigurationManagement.RemoteConfigurationSettings	`-2%` ⚠️	`12%` ✔️	`-4` ✔️
Datadog.Trace.Ci.CIVisibility	`5%` ✔️	`6%` ✔️	`0` ✔️

The following classes were added in #3178:

File	Line coverage	Branch coverage	Complexity
Datadog.Trace.Configuration.GlobalConfigurationSource	`90%`	`83%`	`15`

View the full reports for further details:

OmerRaviv · 2022-09-14T09:13:21Z

                    exception = ex;

-                    if (_globalSettings.DebugEnabled)
+                    if (_isDebugEnabled)


Nit: Do we really need this field anymore? Can't we just do

Suggested change

if (_isDebugEnabled)

if (GlobalSettings.Instance.DebugEnabled)

Total nit though, take it or leave it :)

OmerRaviv

LGTM, sweet improvement. Thanks!

shurivich requested review from a team as code owners September 7, 2022 08:11

shurivich self-assigned this Sep 7, 2022

andrewlock reviewed Sep 7, 2022

View reviewed changes

Comment thread tracer/src/Datadog.Trace/Configuration/GlobalSettings.cs Outdated

Comment thread tracer/src/Datadog.Trace/Configuration/GlobalSettings.cs Outdated

Comment thread tracer/src/Datadog.Trace/Configuration/GlobalSettings.cs

Comment thread tracer/src/Datadog.Trace.Tools.Runner/Utils.cs

This comment has been minimized.

Sign in to view

shurivich force-pushed the shurivich/avoid_multiple_configuration_source_creation branch from 45afa07 to 1b12a26 Compare September 8, 2022 12:58

shurivich requested a review from a team as a code owner September 8, 2022 13:40

This comment has been minimized.

Sign in to view

shurivich force-pushed the shurivich/avoid_multiple_configuration_source_creation branch from 3139862 to 1b12a26 Compare September 8, 2022 14:42

This comment has been minimized.

Sign in to view

shurivich added 2 commits September 13, 2022 12:02

avoid multiple configuration source creation

f3f822c

comment resolve

2da49e3

shurivich force-pushed the shurivich/avoid_multiple_configuration_source_creation branch from 1b12a26 to 2da49e3 Compare September 13, 2022 09:02

andrewlock approved these changes Sep 14, 2022

View reviewed changes

OmerRaviv reviewed Sep 14, 2022

View reviewed changes

OmerRaviv approved these changes Sep 14, 2022

View reviewed changes

shurivich merged commit e33a307 into master Sep 15, 2022

shurivich deleted the shurivich/avoid_multiple_configuration_source_creation branch September 15, 2022 14:20

github-actions Bot added this to the vNext milestone Sep 15, 2022

	if (_isDebugEnabled)
	if (GlobalSettings.Instance.DebugEnabled)

Uh oh!

Conversation

shurivich commented Sep 7, 2022

Summary of changes

Reason for change

Uh oh!

andrewlock left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

andrewlock commented Sep 13, 2022

Benchmarks Report 🐌

Benchmark details

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Uh oh!

andrewlock commented Sep 14, 2022

Code Coverage Report 📊

Datadog.Trace Breakdown ✔️

Uh oh!

OmerRaviv Sep 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OmerRaviv Sep 14, 2022

Choose a reason for hiding this comment

Uh oh!

OmerRaviv left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

OmerRaviv Sep 14, 2022 •

edited

Loading