Remote Configuration Tracer Extension 3 - Remote Configuration Client capabilities by robertpi · Pull Request #3186 · DataDog/dd-trace-dotnet

robertpi · 2022-09-07T15:22:34Z

Summary of changes

RCTE3 allows a library to advertise what remote config features (capabilities) that it supports.

Reason for change

It will be helpful for clients to be able to see in the interface if they can make a change to a services via remote config or not.

Implementation details

The implementation is mostly straight forward. The only complicated part is a reference to the RemoteConfigurationManager class is required at startup and the initialization order doesn't seem to be deterministic. To get round this I wrote a small mechanism that checks if RemoteConfigurationManager is initialized and queues the request for a reference if it isn't. We may need a generalization similar to this, for other services, in the future.

Test coverage

Tests piggy back on the "toggle" tests for activating and deactivating ASM via remote config. This allows us to check the capabilities are correctly sent with each request.

robertpi · 2022-09-16T10:10:35Z

+            if (enableSecurity == true)
+            {
+                await logEntryWatcher.WaitForLogEntry("AppSec Disabled");
+                await Task.Delay(1500);


I'm unsure why this is needed, but it seems to make the CI a lot more stable (it always passes locally).

My initial thought that it was dues to the async rejit request seems to be wrong as the rejitted methods are only needed for handling the body.

andrewlock

I haven't reviewed yet, just saw the comment and implementation for GetInitializedInstance, and got a little worried. The whole Initialization of RCM needs to be written to happen inside of TracerManagerFactory instead of as it is currently in Instrumentation. This is ongoing work by the LiveDebugger team I think. The biggest change is we won't have a global singleton RCM instance, instead it will hang off TracerManager.Instance like all the other singleton services we have (which need to respond to tracersettings changes etc). Maybe this is ok for now, but just want to make you aware

robertpi · 2022-09-16T16:49:32Z

I haven't reviewed yet, just saw the comment and implementation for GetInitializedInstance, and got a little worried. The whole Initialization of RCM needs to be written to happen inside of TracerManagerFactory instead of as it is currently in Instrumentation. This is ongoing work by the LiveDebugger team I think. The biggest change is we won't have a global singleton RCM instance, instead it will hang off TracerManager.Instance like all the other singleton services we have (which need to respond to tracersettings changes etc). Maybe this is ok for now, but just want to make you aware

Thanks, I something like GetInitializedInstance will still be needed. Whenever we've tried to access TracerManager.Instance from Security it's always caused issues. So we'll still need a way of accessing RemoveConfigurationManager so we can register our capabilities.

andrewlock · 2022-09-20T12:41:00Z

Thanks, I something like GetInitializedInstance will still be needed. Whenever we've tried to access TracerManager.Instance from Security it's always caused issues. So we'll still need a way of accessing RemoveConfigurationManager so we can register our capabilities.

Hmmm, that's a bit worrying 🤔 The trouble is, GetInitializedInstance is fundamentally broken in the general case, due to the fact that we can have multiple Tracer instances alive (albeit briefly), and that the RemoteConfigurationManager needs be associated with a specific tracer, so that it uses the correct exporter settings (with the current overall design, anyway)

Whenever we've tried to access TracerManager.Instance from Security it's always caused issues

I think this is what we should look into... Do you have any examples of how this has caused issues? Hopefully there's something there we can address

…ager

OmerRaviv

I only reviewed the parts that effect Live Debugger, so... not much :)

andrewlock · 2022-09-21T09:29:46Z

Benchmarks Report 🐌

Benchmarks for #3186 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	713μs	386ns	1.44μs	0.355	3.18 KB
master	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	461μs	156ns	582ns	0	2.58 KB
#3186	`WriteAndFlushEnrichedTraces`	net472	711μs	408ns	1.58μs	0.372	3.18 KB
#3186	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	471μs	203ns	788ns	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	191ns	0.16ns	0.578ns	0.0675	0	425 B
master	`AllCycleSimpleBody`	netcoreapp3.1	235ns	0.266ns	0.994ns	0.00587	0	424 B
master	`AllCycleMoreComplexBody`	net472	180ns	0.159ns	0.614ns	0.0638	0	401 B
master	`AllCycleMoreComplexBody`	netcoreapp3.1	236ns	0.217ns	0.841ns	0.00545	0	400 B
master	`BodyExtractorSimpleBody`	net472	259ns	0.321ns	1.2ns	0.0573	0	361 B
master	`BodyExtractorSimpleBody`	netcoreapp3.1	219ns	0.17ns	0.658ns	0.00374	0	272 B
master	`BodyExtractorMoreComplexBody`	net472	14.7μs	11.4ns	42.5ns	1.2	0.0219	7.62 KB
master	`BodyExtractorMoreComplexBody`	netcoreapp3.1	11.9μs	10.8ns	40.4ns	0.0894	0	6.75 KB
#3186	`AllCycleSimpleBody`	net472	183ns	0.23ns	0.862ns	0.0676	0	425 B
#3186	`AllCycleSimpleBody`	netcoreapp3.1	237ns	0.302ns	1.17ns	0.00586	0	424 B
#3186	`AllCycleMoreComplexBody`	net472	181ns	0.11ns	0.396ns	0.0638	0	401 B
#3186	`AllCycleMoreComplexBody`	netcoreapp3.1	237ns	0.261ns	0.976ns	0.00542	0	400 B
#3186	`BodyExtractorSimpleBody`	net472	253ns	0.155ns	0.599ns	0.0573	0	361 B
#3186	`BodyExtractorSimpleBody`	netcoreapp3.1	224ns	0.233ns	0.901ns	0.0036	0	272 B
#3186	`BodyExtractorMoreComplexBody`	net472	14.6μs	11ns	42.7ns	1.21	0.0218	7.62 KB
#3186	`BodyExtractorMoreComplexBody`	netcoreapp3.1	11.8μs	9.35ns	35ns	0.0886	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	179μs	134ns	517ns	0.268	20.6 KB
#3186	`SendRequest`	net472	0ns	0ns	0ns	0	0 b
#3186	`SendRequest`	netcoreapp3.1	181μs	196ns	759ns	0.271	20.59 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.86μs	1.05ns	4.08ns	0.15	0.000939	947 B
master	`ExecuteNonQuery`	netcoreapp3.1	1.39μs	0.796ns	2.98ns	0.0127	0	936 B
#3186	`ExecuteNonQuery`	net472	1.87μs	1.93ns	7.49ns	0.15	0.000933	947 B
#3186	`ExecuteNonQuery`	netcoreapp3.1	1.45μs	0.91ns	3.52ns	0.0124	0	936 B

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`CallElasticsearch`	net472	2.59μs	2.43ns	9.43ns	0.184	0.00129	1.16 KB
master	`CallElasticsearch`	netcoreapp3.1	1.53μs	0.414ns	1.49ns	0.0145	0	1.1 KB
master	`CallElasticsearchAsync`	net472	2.66μs	2.09ns	8.11ns	0.205	0	1.29 KB
master	`CallElasticsearchAsync`	netcoreapp3.1	1.64μs	0.735ns	2.65ns	0.0164	0	1.22 KB
#3186	`CallElasticsearch`	net472	2.59μs	2.28ns	8.81ns	0.183	0	1.16 KB
#3186	`CallElasticsearch`	netcoreapp3.1	1.5μs	0.798ns	3.09ns	0.015	0	1.1 KB
#3186	`CallElasticsearchAsync`	net472	2.75μs	2.22ns	8.62ns	0.204	0	1.29 KB
#3186	`CallElasticsearchAsync`	netcoreapp3.1	1.62μs	0.441ns	1.65ns	0.0163	0	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.87μs	4.48ns	17.3ns	0.225	1.42 KB
master	`ExecuteAsync`	netcoreapp3.1	1.8μs	0.769ns	2.98ns	0.018	1.34 KB
#3186	`ExecuteAsync`	net472	2.91μs	2.24ns	8.67ns	0.225	1.42 KB
#3186	`ExecuteAsync`	netcoreapp3.1	1.77μs	0.756ns	2.93ns	0.0178	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.9μs	2.99ns	11.2ns	0.44	2.77 KB
master	`SendAsync`	netcoreapp3.1	3.77μs	1.82ns	7.05ns	0.0358	2.6 KB
#3186	`SendAsync`	net472	5.98μs	3.33ns	12.9ns	0.439	2.77 KB
#3186	`SendAsync`	netcoreapp3.1	3.69μs	5.62ns	21ns	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.25μs	15.5ns	59.8ns	0.287	1.81 KB
master	`EnrichedLog`	netcoreapp3.1	2.48μs	13.5ns	109ns	0.0242	1.85 KB
#3186	`EnrichedLog`	net472	3.12μs	3.78ns	14.6ns	0.287	1.81 KB
#3186	`EnrichedLog`	netcoreapp3.1	2.49μs	4.91ns	19ns	0.0256	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	199ns	770ns	0.682	0.227	4.65 KB
master	`EnrichedLog`	netcoreapp3.1	119μs	138ns	535ns	0	0	4.49 KB
#3186	`EnrichedLog`	net472	153μs	191ns	741ns	0.683	0.228	4.65 KB
#3186	`EnrichedLog`	netcoreapp3.1	117μs	209ns	809ns	0	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.73μs	12.3ns	47.6ns	0.57	0.00292	3.59 KB
master	`EnrichedLog`	netcoreapp3.1	4.35μs	1.73ns	6.48ns	0.0531	0	3.91 KB
#3186	`EnrichedLog`	net472	5.95μs	1.82ns	6.82ns	0.568	0.00291	3.59 KB
#3186	`EnrichedLog`	netcoreapp3.1	4.41μs	2.83ns	10.6ns	0.053	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.23μs	2.36ns	9.15ns	0.217	1.37 KB
master	`SendReceive`	netcoreapp3.1	1.82μs	0.922ns	3.57ns	0.0174	1.32 KB
#3186	`SendReceive`	net472	2.25μs	2.18ns	8.17ns	0.217	1.37 KB
#3186	`SendReceive`	netcoreapp3.1	1.85μs	0.562ns	2.1ns	0.0175	1.32 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net472	5.13μs	1.7ns	6.37ns	0.353	2.23 KB
master	`EnrichedLog`	netcoreapp3.1	4.33μs	1.36ns	5.08ns	0.0239	1.8 KB
#3186	`EnrichedLog`	net472	5.21μs	1.87ns	7.26ns	0.352	2.23 KB
#3186	`EnrichedLog`	netcoreapp3.1	4.4μs	2.3ns	8.61ns	0.0243	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.24μs	0.732ns	2.74ns	0.129	810 B
master	`StartFinishSpan`	netcoreapp3.1	966ns	1.16ns	4.18ns	0.01	760 B
master	`StartFinishScope`	net472	1.41μs	1.06ns	4.12ns	0.141	891 B
master	`StartFinishScope`	netcoreapp3.1	1.05μs	0.383ns	1.38ns	0.0119	880 B
#3186	`StartFinishSpan`	net472	1.13μs	0.473ns	1.83ns	0.128	810 B
#3186	`StartFinishSpan`	netcoreapp3.1	947ns	0.319ns	1.19ns	0.0105	760 B
#3186	`StartFinishScope`	net472	1.39μs	0.713ns	2.67ns	0.141	891 B
#3186	`StartFinishScope`	netcoreapp3.1	1.1μs	0.351ns	1.27ns	0.0116	880 B

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`RunOnMethodBegin`	net472	1.51μs	1.08ns	4.17ns	0.141	891 B
master	`RunOnMethodBegin`	netcoreapp3.1	1.16μs	0.387ns	1.45ns	0.0122	880 B
#3186	`RunOnMethodBegin`	net472	1.47μs	0.751ns	2.91ns	0.142	891 B
#3186	`RunOnMethodBegin`	netcoreapp3.1	1.14μs	0.316ns	1.14ns	0.0119	880 B

andrewlock · 2022-09-21T10:39:11Z

Code Coverage Report 📊

⛔ Merging #3186 into master will will decrease line coverage by 5%
⚠️ Merging #3186 into master will will decrease branch coverage by 3%
⛔ Merging #3186 into master will will increase complexity by 20

	master	#3186	Change
Lines	`17963` / `24889`	`16837` / `24929`
Lines %	`72%`	`68%`	`-5%` ⛔
Branches	`10472` / `15254`	`10015` / `15274`
Branches %	`69%`	`66%`	`-3%` ⚠️
Complexity	`16477`	`16497`	`20` ⛔

View the full report for further details:

Datadog.Trace Breakdown ⛔

	master	#3186	Change
Lines %	`72%`	`68%`	`-5%` ⛔
Branches %	`69%`	`66%`	`-3%` ⚠️
Complexity	`16477`	`16497`	`20` ⛔

The following classes have significant coverage changes.

File	Line coverage change	Branch coverage change	Complexity change
Datadog.Trace.Debugger.PInvoke.NativeMethodProbeDefinition	`-100%` ⛔	`-100%` ⛔	`0` ✔️
Datadog.Trace.Debugger.Sink.SnapshotSink	`-100%` ⛔	`-100%` ⛔	`0` ✔️
Datadog.Trace.Debugger.Snapshots.LazySnapshotSerializerFieldsAndPropsSelector	`-100%` ⛔	`-100%` ⛔	`0` ✔️
Datadog.Trace.Debugger.Snapshots.DebuggerSnapshotCreator	`-100%` ⛔	`-68%` ⛔	`1` ⛔
BoundLineProbeLocation	`-100%` ⛔	`0%` ✔️	`0` ✔️
Datadog.Trace.Debugger.CapturedLines	`-100%` ⛔	`0%` ✔️	`0` ✔️
Datadog.Trace.Debugger.Instrumentation.DebuggerReturn	`-100%` ⛔	`0%` ✔️	`0` ✔️
Datadog.Trace.Debugger.Instrumentation.MethodMetadataInfo	`-100%` ⛔	`0%` ✔️	`0` ✔️
Datadog.Trace.Debugger.Models.LineProbeResolveResult	`-100%` ⛔	`0%` ✔️	`0` ✔️
Datadog.Trace.Debugger.PInvoke.NativeLineProbeDefinition	`-100%` ⛔	`0%` ✔️	`0` ✔️
...And 52 more

View the full reports for further details:

This comment has been minimized.

Sign in to view

robertpi force-pushed the anna/asm/rcte3-capabilities branch from 312121a to 464290f Compare September 12, 2022 09:25

This comment has been minimized.

Sign in to view

robertpi mentioned this pull request Sep 14, 2022

Implement rcte2 - Configuration apply status #3175

Merged

robertpi force-pushed the anna/asm/rcte3-capabilities branch from 8d073df to 8557cf4 Compare September 15, 2022 12:06

This comment has been minimized.

Sign in to view

robertpi force-pushed the anna/asm/rcte3-capabilities branch from 758ae6c to e7b0eb9 Compare September 16, 2022 08:09

This comment has been minimized.

Sign in to view

robertpi changed the title ~~Anna/asm/rcte3 capabilities~~ RCTE3 capabilities Sep 16, 2022

robertpi marked this pull request as ready for review September 16, 2022 10:07

robertpi requested review from a team as code owners September 16, 2022 10:07

robertpi commented Sep 16, 2022

View reviewed changes

andrewlock reviewed Sep 16, 2022

View reviewed changes

robertpi changed the title ~~RCTE3 capabilities~~ Remote Configuration Tracer Extension 3 - Remote Configuration Client capabilities Sep 16, 2022

robertpi force-pushed the anna/asm/rcte3-capabilities branch from e7b0eb9 to 2a787a8 Compare September 16, 2022 16:45

This comment has been minimized.

Sign in to view