Test `RemoteConfigurationApi` instead of using file provider by andrewlock · Pull Request #3182 · DataDog/dd-trace-dotnet

andrewlock · 2022-09-07T12:37:23Z

Summary of changes

Removes the RemoteConfigurationFileApi in favour of using the actual implementation

Reason for change

Currently we're not testing the IRemoteConfigurationApi that we're using in production, as we're using a completely separate implementation.

This File API was introduced to make debugging easier without an agent, but IMO this is the wrong approach, as it ultimately requires we have completely separate implementations. Given we now require the agent be present before RCM even runs (due to the /info dependency), I don't think this approach is worth pursuing further.

An (IMO) better approach is to make it easy to run a MockTracerAgent instance and allow configuring (this) by file for better debugging. This could then support/test multiple products, instead of the single-use approach we have currently.

Implementation details

Removed the RemoteConfigurationFileApi and associated paraphernalia. Added ability to set the RCM response to use on the MockAgent

Test coverage

Should have more coverage as now we're actually testing the implementation

Other details

Feel free to tell me you hate it @OmerRaviv @shurivich 😄

shurivich · 2022-09-07T17:02:57Z

-                string.IsNullOrWhiteSpace(remoteConfigurationSettings.FilePath)
-                    ? RemoteConfigurationApi.Create(apiRequestFactory, discoveryService)
-                    : RemoteConfigurationFileApi.Create(remoteConfigurationSettings);
+            return RemoteConfigurationApi.Create(apiRequestFactory, discoveryService);


I think you can remove entire class, and use direct call RemoteConfigurationApi.Create(apiRequestFactory, discoveryService), instead?

Could do yeah. I kind of like that it incapsulates this for now, as otherwise it's just more lines in the (already large) Instrumentation class. It could well make sense to remove it when you refactor the RCM creation into TracerManager though 🙂

shurivich · 2022-09-07T17:04:18Z

-
-namespace Datadog.Trace.RemoteConfigurationManagement.Transport
-{
-    internal class RemoteConfigurationFileApi : IRemoteConfigurationApi


I'm so happy this will be deleted! 😄

shurivich

LGTM
I'm happy with this change

andrewlock · 2022-09-08T09:20:42Z

Benchmarks Report 🐌

Benchmarks for #3182 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	728μs	434ns	1.62μs	0.361	3.18 KB
master	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	468μs	249ns	934ns	0	2.58 KB
#3182	`WriteAndFlushEnrichedTraces`	net472	726μs	495ns	1.92μs	0.361	3.18 KB
#3182	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	468μs	133ns	496ns	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	185ns	0.206ns	0.797ns	0.0675	0	425 B
master	`AllCycleSimpleBody`	netcoreapp3.1	236ns	0.223ns	0.864ns	0.00585	0	424 B
master	`AllCycleMoreComplexBody`	net472	205ns	0.447ns	1.73ns	0.0637	0	401 B
master	`AllCycleMoreComplexBody`	netcoreapp3.1	234ns	0.356ns	1.38ns	0.00542	0	400 B
master	`BodyExtractorSimpleBody`	net472	271ns	0.24ns	0.929ns	0.0573	0	361 B
master	`BodyExtractorSimpleBody`	netcoreapp3.1	230ns	0.264ns	1.02ns	0.00374	0	272 B
master	`BodyExtractorMoreComplexBody`	net472	14.7μs	16.8ns	65.3ns	1.21	0.0147	7.62 KB
master	`BodyExtractorMoreComplexBody`	netcoreapp3.1	11.9μs	10.3ns	37.3ns	0.0901	0	6.75 KB
#3182	`AllCycleSimpleBody`	net472	186ns	0.157ns	0.587ns	0.0675	0	425 B
#3182	`AllCycleSimpleBody`	netcoreapp3.1	236ns	0.31ns	1.2ns	0.0058	0	424 B
#3182	`AllCycleMoreComplexBody`	net472	188ns	0.188ns	0.702ns	0.0637	0	401 B
#3182	`AllCycleMoreComplexBody`	netcoreapp3.1	235ns	0.361ns	1.4ns	0.00551	0	400 B
#3182	`BodyExtractorSimpleBody`	net472	263ns	0.304ns	1.18ns	0.0573	0	361 B
#3182	`BodyExtractorSimpleBody`	netcoreapp3.1	222ns	0.21ns	0.813ns	0.00367	0	272 B
#3182	`BodyExtractorMoreComplexBody`	net472	14.5μs	14.2ns	54.9ns	1.21	0.0216	7.62 KB
#3182	`BodyExtractorMoreComplexBody`	netcoreapp3.1	11.7μs	15.5ns	60.1ns	0.0882	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	182μs	154ns	595ns	0.181	20.57 KB
#3182	`SendRequest`	net472	0ns	0ns	0ns	0	0 b
#3182	`SendRequest`	netcoreapp3.1	183μs	153ns	591ns	0.182	20.57 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.82μs	0.635ns	2.38ns	0.15	0.000911	947 B
master	`ExecuteNonQuery`	netcoreapp3.1	1.42μs	1.36ns	5.27ns	0.0127	0	936 B
#3182	`ExecuteNonQuery`	net472	1.81μs	0.429ns	1.55ns	0.15	0.000913	947 B
#3182	`ExecuteNonQuery`	netcoreapp3.1	1.47μs	0.629ns	2.35ns	0.0124	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.61μs	0.715ns	2.58ns	0.183	1.16 KB
master	`CallElasticsearch`	netcoreapp3.1	1.55μs	0.641ns	2.48ns	0.0148	1.1 KB
master	`CallElasticsearchAsync`	net472	2.71μs	0.888ns	3.32ns	0.204	1.29 KB
master	`CallElasticsearchAsync`	netcoreapp3.1	1.63μs	0.997ns	3.86ns	0.0163	1.22 KB
#3182	`CallElasticsearch`	net472	2.61μs	1.03ns	3.99ns	0.183	1.16 KB
#3182	`CallElasticsearch`	netcoreapp3.1	1.57μs	1.18ns	4.42ns	0.0148	1.1 KB
#3182	`CallElasticsearchAsync`	net472	2.57μs	2.25ns	8.41ns	0.205	1.29 KB
#3182	`CallElasticsearchAsync`	netcoreapp3.1	1.65μs	7.33ns	27.4ns	0.0162	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.61μs	7.8ns	30.2ns	0.226	1.42 KB
master	`ExecuteAsync`	netcoreapp3.1	1.74μs	4.5ns	17.4ns	0.0181	1.34 KB
#3182	`ExecuteAsync`	net472	2.72μs	8.4ns	32.5ns	0.226	1.42 KB
#3182	`ExecuteAsync`	netcoreapp3.1	1.72μs	3.33ns	12.9ns	0.0187	1.34 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendAsync`	net472	5.66μs	8.01ns	28.9ns	0.438	2.77 KB
master	`SendAsync`	netcoreapp3.1	3.6μs	8.33ns	32.3ns	0.0343	2.6 KB
#3182	`SendAsync`	net472	5.68μs	6.99ns	26.1ns	0.437	2.77 KB
#3182	`SendAsync`	netcoreapp3.1	3.51μs	7.47ns	28.9ns	0.0352	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.24μs	1.48ns	5.12ns	0.287	1.81 KB
master	`EnrichedLog`	netcoreapp3.1	2.45μs	0.941ns	3.64ns	0.0246	1.85 KB
#3182	`EnrichedLog`	net472	3.21μs	2.17ns	7.83ns	0.288	1.81 KB
#3182	`EnrichedLog`	netcoreapp3.1	2.52μs	0.628ns	2.35ns	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	96.5ns	374ns	0.689	0.23	4.65 KB
master	`EnrichedLog`	netcoreapp3.1	115μs	142ns	530ns	0	0	4.49 KB
#3182	`EnrichedLog`	net472	149μs	50.7ns	190ns	0.674	0.225	4.65 KB
#3182	`EnrichedLog`	netcoreapp3.1	117μs	152ns	570ns	0.0584	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.51μs	15.2ns	58.9ns	0.569	0.00274	3.59 KB
master	`EnrichedLog`	netcoreapp3.1	4.44μs	8.1ns	30.3ns	0.0538	0	3.91 KB
#3182	`EnrichedLog`	net472	5.57μs	9.55ns	37ns	0.569	0.00279	3.59 KB
#3182	`EnrichedLog`	netcoreapp3.1	4.36μs	10.6ns	41.2ns	0.0529	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.33μs	1.21ns	4.69ns	0.218	1.37 KB
master	`SendReceive`	netcoreapp3.1	1.76μs	0.821ns	2.96ns	0.0184	1.32 KB
#3182	`SendReceive`	net472	2.36μs	5.9ns	22.1ns	0.218	1.37 KB
#3182	`SendReceive`	netcoreapp3.1	1.89μs	0.713ns	2.57ns	0.018	1.32 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net472	4.92μs	1.49ns	5.57ns	0.353	2.23 KB
master	`EnrichedLog`	netcoreapp3.1	4.21μs	1.79ns	6.93ns	0.0251	1.8 KB
#3182	`EnrichedLog`	net472	5μs	2.04ns	7.9ns	0.353	2.23 KB
#3182	`EnrichedLog`	netcoreapp3.1	4.13μs	1.17ns	4.04ns	0.0229	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.381ns	1.48ns	0.129	810 B
master	`StartFinishSpan`	netcoreapp3.1	915ns	0.328ns	1.23ns	0.0102	760 B
master	`StartFinishScope`	net472	1.36μs	0.357ns	1.38ns	0.141	891 B
master	`StartFinishScope`	netcoreapp3.1	1.04μs	0.279ns	1ns	0.012	880 B
#3182	`StartFinishSpan`	net472	1.14μs	0.595ns	2.23ns	0.128	810 B
#3182	`StartFinishSpan`	netcoreapp3.1	922ns	0.478ns	1.85ns	0.0101	760 B
#3182	`StartFinishScope`	net472	1.45μs	0.488ns	1.89ns	0.141	891 B
#3182	`StartFinishScope`	netcoreapp3.1	1.1μs	0.506ns	1.89ns	0.012	880 B

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`RunOnMethodBegin`	net472	1.44μs	0.336ns	1.3ns	0.141	891 B
master	`RunOnMethodBegin`	netcoreapp3.1	1.15μs	0.382ns	1.48ns	0.012	880 B
#3182	`RunOnMethodBegin`	net472	1.51μs	0.753ns	2.92ns	0.141	891 B
#3182	`RunOnMethodBegin`	netcoreapp3.1	1.1μs	0.547ns	2.12ns	0.0116	880 B

andrewlock · 2022-09-08T10:00:32Z

Code Coverage Report 📊

✔️ Merging #3182 into master will not change line coverage
✔️ Merging #3182 into master will not change branch coverage
✔️ Merging #3182 into master will will decrease complexity by 6

	master	#3182	Change
Lines	`17816` / `24332`	`17810` / `24322`
Lines %	`73%`	`73%`	`0%` ✔️
Branches	`10500` / `14962`	`10502` / `14958`
Branches %	`70%`	`70%`	`0%` ✔️
Complexity	`16161`	`16155`	`-6` ✔️

View the full report for further details:

Datadog.Trace Breakdown ✔️

	master	#3182	Change
Lines %	`73%`	`73%`	`0%` ✔️
Branches %	`70%`	`70%`	`0%` ✔️
Complexity	`16161`	`16155`	`-6` ✔️

The following classes have significant coverage changes.

File	Line coverage change	Branch coverage change	Complexity change
Datadog.Trace.Ci.CIVisibility	`5%` ✔️	`6%` ✔️	`0` ✔️

1 classes were removed from Datadog.Trace in #3182

View the full reports for further details:

OmerRaviv

LGTM

andrewlock added the area:tests unit tests, integration tests label Sep 7, 2022

andrewlock requested review from a team as code owners September 7, 2022 12:37

andrewlock force-pushed the andrew/replace-rcm-file-provider branch from 6bd31fd to a189ea6 Compare September 7, 2022 15:40

This comment has been minimized.

Sign in to view

shurivich reviewed Sep 7, 2022

View reviewed changes

shurivich approved these changes Sep 7, 2022

View reviewed changes

robertpi approved these changes Sep 7, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

andrewlock added 3 commits September 8, 2022 09:37

Remove RemoteConfigurationFileApi

b119d0e

Add ability to set RCM response in MockTracerAgent

1847c5f

Update tests to use API-based RCM instead of setting a file

cc10fd1

andrewlock force-pushed the andrew/replace-rcm-file-provider branch from a189ea6 to cc10fd1 Compare September 8, 2022 08:39

anna-git approved these changes Sep 8, 2022

View reviewed changes

OmerRaviv self-requested a review September 8, 2022 11:00

OmerRaviv approved these changes Sep 8, 2022

View reviewed changes

andrewlock merged commit 6c1f858 into master Sep 8, 2022

andrewlock deleted the andrew/replace-rcm-file-provider branch September 8, 2022 12:28

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

Uh oh!

Conversation

andrewlock commented Sep 7, 2022

Summary of changes

Reason for change

Implementation details

Test coverage

Other details

Uh oh!

This comment has been minimized.

shurivich Sep 7, 2022

Choose a reason for hiding this comment

Uh oh!

andrewlock Sep 8, 2022

Choose a reason for hiding this comment

Uh oh!

shurivich Sep 7, 2022

Choose a reason for hiding this comment

Uh oh!

shurivich left a comment

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

andrewlock commented Sep 8, 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 8, 2022

Code Coverage Report 📊

Datadog.Trace Breakdown ✔️

Uh oh!

OmerRaviv left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants