Support Remote Configuration Management by shurivich · Pull Request #3077 · DataDog/dd-trace-dotnet

shurivich · 2022-08-14T14:58:22Z

Summary of changes

Add support for Support Remote Configuration management

OmerRaviv · 2022-08-15T08:46:41Z

+            var isRcmDisabled = apiResponse.StatusCode == 404;
+            if (isRcmDisabled)
+            {
+                return null;


Please document this in an XML comment and ensure all consumers are checking for null.

What happens if we received a different error status code? Won't we fail with an exception when we later call JsonConvert.DeserializeObject?

Fixed at 684ba86

It would also be good to add #nullable enable, which further documents and checks (separate PR now!)

andrewlock

I have quite a few comments, most aren't blocking 😅

The main point is

This won't handle manual configuration changes, we should refactor to move initialization into the TracerManager/TracerManagerFactory (those are not longer good names 😅)

And my main nit 😄

There's a lot of new code that doesn't have #nullable enable set, we should really try to use it where possible

andrewlock · 2022-08-23T12:01:44Z

        }

-        public static string[] AllSupportedEndpoints => SupportedDebuggerEndpoints.Concat(SupportedProbeConfigurationEndpoints).ToArray();
+        public static DiscoveryService Instance { get; private set; }


I don't think a static instance here is the right way to go. As discussed previously, the creation of the DiscoveryService should be handled in the TracerManager, as the exporter settings (and hence the IApiRequestFactory) can change after the initial Instrumentation execution.

Exposing the DiscoveryService on TracerManager removes the need for a static instance and the GlobalLock (that's handled in TracerManager).

I'm happy for this refactoring to be in a follow-up PR if preferrable

Yes, we definitely should do it in the following PR

andrewlock · 2022-08-23T12:04:42Z

+                    using var response = await api.GetAsync().ConfigureAwait(false);
+                    if (response.StatusCode == 200)
+                    {
+                        var content = await response.ReadAsStringAsync().ConfigureAwait(false);


nit: seeing as we're deserializing this straight to JSON, we could skip the string allocation and use JSON.NET's Stream support instead to deserialize it directly.

Again, could be a follow up, I just think we need to be careful about allocations in this code as it will now run by default, on app startup

I agree with you, we should do it, but in a follow up PR.
I will be happy to do this.

andrewlock · 2022-08-23T12:07:49Z

 #endif

-        internal static void InitializeLiveDebugger()
+        private static void InitRemoteConfigurationManagement(Tracer tracer)


This code should all be wrapped up in TracerManager, as settings could change.

Agree, will do in a follow up PR.

andrewlock · 2022-08-23T12:15:26Z

        }

-        public static LiveDebugger Instance => LazyInstance.Value;
+        public static LiveDebugger Instance { get; private set; }


This may be another case where this needs to be handled and exposed on the TracerManager, as settings could change. TracerManager looking less and less like the right name...🙈

Same as above.
We can start to think about new name 😄

andrewlock · 2022-08-23T12:16:49Z

+                AppDomain.CurrentDomain.AssemblyLoad += (sender, args) => CheckUnboundProbes();
+                AppDomain.CurrentDomain.DomainUnload += (sender, args) => _lineProbeResolver.OnDomainUnloaded();


Can we not hook into the LifetimeManager here?

@OmerRaviv @GreenMatan WDYT?

Great suggestion, makes total sense.
Just to point out that this should only apply to line 105 - line 104 isn't really relevant for LifetimeManager AFAICT.

BTW @GreenMatan note that we still don't have any tests whatsoever for the debugger working properly in a multi AppDomain scenario, so things like "method probe exists on the same method in both AppDomains A & B, AppDomain B is then unloaded" is a completely untested code path currently. I'll raise the priority of that JIRA ticket.

Either way, this is existing implementation, so I think we can safely push to a follow up PR 🙂

andrewlock · 2022-08-23T13:17:37Z

+            var isRcmDisabled = apiResponse.StatusCode == 404;
+            if (isRcmDisabled)
+            {
+                return null;


It would also be good to add #nullable enable, which further documents and checks (separate PR now!)

andrewlock · 2022-08-23T13:22:16Z

+                return null;
+            }
+
+            var content = await apiResponse.ReadAsStringAsync().ConfigureAwait(false);


nit: if we're deserializing straight from the stream, then we shouldn't convert to a string first. We should probably add a GetStream() method to IApiResponse to avoid the extra unnecessary allocations. For a separate PR though

Good idea, however I think we should add it in separate PR.
This one is already too big 😄

Agreed, separate PR

dromanol

Great job, IMHO. But I think on-the-fly registration / unregistration of products should be addressed.

Co-authored-by: Andrew Lock <[email protected]>

andrewlock · 2022-08-24T10:54:04Z

Benchmarks Report 🐌

Benchmarks for #3077 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	730μs	359ns	1.39μs	0.361	3.18 KB
master	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	452μs	1.11μs	4.31μs	0	2.58 KB
#3077	`WriteAndFlushEnrichedTraces`	net472	711μs	475ns	1.84μs	0.355	3.18 KB
#3077	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	461μ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	181ns	0.199ns	0.77ns	0.0676	9.21E-05	425 B
master	`AllCycleSimpleBody`	netcoreapp3.1	236ns	0.314ns	1.13ns	0.00583	0	424 B
master	`AllCycleMoreComplexBody`	net472	190ns	0.199ns	0.771ns	0.0637	0	401 B
master	`AllCycleMoreComplexBody`	netcoreapp3.1	236ns	0.295ns	1.14ns	0.00538	0	400 B
master	`BodyExtractorSimpleBody`	net472	251ns	0.22ns	0.823ns	0.0573	0	361 B
master	`BodyExtractorSimpleBody`	netcoreapp3.1	229ns	0.478ns	1.79ns	0.00366	0	272 B
master	`BodyExtractorMoreComplexBody`	net472	15.1μs	12.5ns	46.8ns	1.21	0.015	7.62 KB
master	`BodyExtractorMoreComplexBody`	netcoreapp3.1	12μs	10.7ns	38.5ns	0.0902	0	6.75 KB
#3077	`AllCycleSimpleBody`	net472	185ns	0.112ns	0.433ns	0.0675	0	425 B
#3077	`AllCycleSimpleBody`	netcoreapp3.1	242ns	0.337ns	1.26ns	0.00584	0	424 B
#3077	`AllCycleMoreComplexBody`	net472	185ns	0.139ns	0.521ns	0.0638	0	401 B
#3077	`AllCycleMoreComplexBody`	netcoreapp3.1	237ns	0.385ns	1.49ns	0.00535	0	400 B
#3077	`BodyExtractorSimpleBody`	net472	251ns	0.258ns	0.965ns	0.0574	0	361 B
#3077	`BodyExtractorSimpleBody`	netcoreapp3.1	230ns	0.247ns	0.956ns	0.00368	0	272 B
#3077	`BodyExtractorMoreComplexBody`	net472	14.6μs	13ns	50.3ns	1.21	0.0218	7.62 KB
#3077	`BodyExtractorMoreComplexBody`	netcoreapp3.1	12.4μs	15.3ns	57.3ns	0.0869	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	121ns	468ns	0.272	20.57 KB
#3077	`SendRequest`	net472	0ns	0ns	0ns	0	0 b
#3077	`SendRequest`	netcoreapp3.1	180μs	140ns	543ns	0.271	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.75μs	0.793ns	2.97ns	0.15	0.000895	947 B
master	`ExecuteNonQuery`	netcoreapp3.1	1.41μs	4.13ns	15.5ns	0.0126	0	936 B
#3077	`ExecuteNonQuery`	net472	1.79μs	0.448ns	1.74ns	0.15	0.000898	947 B
#3077	`ExecuteNonQuery`	netcoreapp3.1	1.36μs	0.267ns	0.999ns	0.0129	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.44μs	0.844ns	3.16ns	0.183	1.16 KB
master	`CallElasticsearch`	netcoreapp3.1	1.55μs	1.46ns	5.64ns	0.0147	1.1 KB
master	`CallElasticsearchAsync`	net472	2.66μs	0.498ns	1.87ns	0.205	1.29 KB
master	`CallElasticsearchAsync`	netcoreapp3.1	1.56μs	0.336ns	1.26ns	0.0164	1.22 KB
#3077	`CallElasticsearch`	net472	2.34μs	0.701ns	2.62ns	0.183	1.16 KB
#3077	`CallElasticsearch`	netcoreapp3.1	1.51μs	1.2ns	4.64ns	0.0143	1.1 KB
#3077	`CallElasticsearchAsync`	net472	2.57μs	0.701ns	2.71ns	0.205	1.29 KB
#3077	`CallElasticsearchAsync`	netcoreapp3.1	1.6μs	0.748ns	2.8ns	0.0167	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.52μs	5.32ns	19.9ns	0.224	1.41 KB
master	`ExecuteAsync`	netcoreapp3.1	1.68μs	2.78ns	10.4ns	0.0186	1.34 KB
#3077	`ExecuteAsync`	net472	2.61μs	10.9ns	42.2ns	0.224	1.41 KB
#3077	`ExecuteAsync`	netcoreapp3.1	1.66μs	4.66ns	17.4ns	0.0181	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.67μs	13.8ns	53.4ns	0.438	2.77 KB
master	`SendAsync`	netcoreapp3.1	3.62μs	9.19ns	34.4ns	0.0345	2.6 KB
#3077	`SendAsync`	net472	5.65μs	15.9ns	61.7ns	0.438	2.77 KB
#3077	`SendAsync`	netcoreapp3.1	3.54μs	11.2ns	43.5ns	0.035	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.11μs	1.07ns	4.01ns	0.288	1.81 KB
master	`EnrichedLog`	netcoreapp3.1	2.34μs	0.734ns	2.65ns	0.0246	1.85 KB
#3077	`EnrichedLog`	net472	3.03μs	3.06ns	11ns	0.287	1.81 KB
#3077	`EnrichedLog`	netcoreapp3.1	2.46μs	1.79ns	6.68ns	0.0259	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	150μs	158ns	611ns	0.68	0.227	4.65 KB
master	`EnrichedLog`	netcoreapp3.1	114μs	122ns	458ns	0.057	0	4.49 KB
#3077	`EnrichedLog`	net472	151μs	166ns	623ns	0.69	0.23	4.65 KB
#3077	`EnrichedLog`	netcoreapp3.1	117μs	161ns	624ns	0.0581	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.52μs	11.1ns	43ns	0.569	0.00278	3.59 KB
master	`EnrichedLog`	netcoreapp3.1	4.41μs	10.5ns	40.8ns	0.0535	0	3.91 KB
#3077	`EnrichedLog`	net472	5.46μs	14.4ns	55.6ns	0.567	0.0027	3.59 KB
#3077	`EnrichedLog`	netcoreapp3.1	4.5μs	12.4ns	47.9ns	0.0532	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.26μs	0.56ns	2.1ns	0.218	1.37 KB
master	`SendReceive`	netcoreapp3.1	1.74μs	0.587ns	2.2ns	0.0175	1.32 KB
#3077	`SendReceive`	net472	2.23μs	1.14ns	4.26ns	0.217	1.37 KB
#3077	`SendReceive`	netcoreapp3.1	1.73μs	1.09ns	4.24ns	0.0182	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.17μs	1.76ns	6.83ns	0.354	2.23 KB
master	`EnrichedLog`	netcoreapp3.1	4.18μs	1.73ns	6.69ns	0.023	1.8 KB
#3077	`EnrichedLog`	net472	4.97μs	2.62ns	10.2ns	0.352	2.23 KB
#3077	`EnrichedLog`	netcoreapp3.1	4.37μs	1.67ns	6.48ns	0.0241	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.18μs	0.426ns	1.65ns	0.128	810 B
master	`StartFinishSpan`	netcoreapp3.1	908ns	1.18ns	4.41ns	0.0104	760 B
master	`StartFinishScope`	net472	1.35μs	0.611ns	2.29ns	0.141	891 B
master	`StartFinishScope`	netcoreapp3.1	1.11μs	0.523ns	1.89ns	0.0122	880 B
#3077	`StartFinishSpan`	net472	1.08μs	0.571ns	2.21ns	0.128	810 B
#3077	`StartFinishSpan`	netcoreapp3.1	925ns	0.36ns	1.39ns	0.0101	760 B
#3077	`StartFinishScope`	net472	1.38μs	0.299ns	1.08ns	0.141	891 B
#3077	`StartFinishScope`	netcoreapp3.1	1.11μs	0.547ns	2.12ns	0.0117	880 B

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`RunOnMethodBegin`	net472	1.48μs	0.483ns	1.81ns	0.141	891 B
master	`RunOnMethodBegin`	netcoreapp3.1	1.11μs	0.571ns	2.21ns	0.0122	880 B
#3077	`RunOnMethodBegin`	net472	1.55μs	0.308ns	1.19ns	0.141	891 B
#3077	`RunOnMethodBegin`	netcoreapp3.1	1.11μs	1.56ns	5.82ns	0.0116	880 B

andrewlock · 2022-08-24T12:08:51Z

Code Coverage Report 📊

✔️ Merging #3077 into master will not change line coverage
✔️ Merging #3077 into master will not change branch coverage
⛔ Merging #3077 into master will will increase complexity by 75

	master	#3077	Change
Lines	`17023` / `23379`	`17160` / `23534`
Lines %	`73%`	`73%`	`0%` ✔️
Branches	`10161` / `14458`	`10219` / `14538`
Branches %	`70%`	`70%`	`0%` ✔️
Complexity	`15565`	`15640`	`75` ⛔

View the full report for further details:

Datadog.Trace Breakdown ✔️

	master	#3077	Change
Lines %	`73%`	`73%`	`0%` ✔️
Branches %	`70%`	`70%`	`0%` ✔️
Complexity	`15565`	`15640`	`75` ⛔

The following classes have significant coverage changes.

File	Line coverage change	Branch coverage change	Complexity change
Datadog.Trace.Ci.GitInfo	`-17%` ⛔	`-11%` ⛔	`0` ✔️
Datadog.Trace.Agent.DiscoveryService.DiscoveryService	`-10%` ⛔	`0%` ✔️	`5` ⛔
Datadog.Trace.Debugger.Configurations.Models.Where	`0%` ✔️	`-6%` ⛔	`0` ✔️
Datadog.Trace.Ci.CIVisibility	`5%` ✔️	`6%` ✔️	`0` ✔️
Datadog.Trace.Debugger.Helpers.StringExtensions	`100%` ✔️	`0%` ✔️	`0` ✔️

The following classes were added in #3077:

File	Line coverage	Branch coverage	Complexity
Datadog.Trace.Debugger.LiveDebuggerFactory	`100%`	`100%`	`2`
Datadog.Trace.Debugger.LiveDebuggerProduct	`100%`	`50%`	`5`
Datadog.Trace.RemoteConfigurationManagement.Json.TufRootBase64Converter	`86%`	`50%`	`5`
Datadog.Trace.RemoteConfigurationManagement.Product	`90%`	`90%`	`12`
Datadog.Trace.RemoteConfigurationManagement.ProductConfigChangedEventArgs	`100%`	`100%`	`3`
...And 15 more

6 classes were removed from Datadog.Trace in #3077

View the full reports for further details:

shurivich requested review from a team as code owners August 14, 2022 14:58

shurivich marked this pull request as draft August 14, 2022 14:59

shurivich force-pushed the shurivich/rcm_client branch from d07b658 to b2d2ed3 Compare August 14, 2022 15:00

shurivich self-assigned this Aug 14, 2022

shurivich added the area:debugger label Aug 14, 2022

OmerRaviv changed the title ~~Support Remote Configuration management~~ Support Remote Configuration Management Aug 14, 2022