Implement rcte2 - Configuration apply status by robertpi · Pull Request #3175 · DataDog/dd-trace-dotnet

robertpi · 2022-09-06T14:48:56Z

Summary of changes

A system that allows a remote config client to acknowledge each file provided by RC.

Reason for change

It will allows us to support customers using remote config more easily.

Implementation details

As remote config is event based, acknowledging a file relies on the subscriber of the products event to call a method on the event args class to acknowledge successful application of the config. If an error was thrown then the config will automatically be set into an error state. A subscribe can also explicitly set an error if they whish.

Test coverage

The feature is covered by an integration test, which includes a new mechanism for recovering the remote config request.

andrewlock

Mostly minor comments, but I'd prefer that we didn't use the file-based RCM for testing, and instead added the expected responses to the Mock Agent

andrewlock · 2022-09-07T11:27:54Z

+            // can only move from unack to ack
+            if (applyDetails.ApplyState == ApplyStates.UNACKNOWLEDGED)
+            {
+                applyDetails.ApplyState = ApplyStates.ACKNOWLEDGED;
+            }


What if it was an error before? we leave it as errored, even though it's now acknowledged? That differs with the behaviour in RemoteConfigurationCache AFAICT?

If an error occurred, I would rather know about the error than if it was acknowledged as well, though in an ideal world we'd know about both, but that's not possible with current interface to the backend.

That's fair enough, my bigger question/concern was the disparity with RemoteConfigurationCache, and whether that's an issue

This bit of code is all called if the remote config client detects a change in the message. I believe it actually plays nicely with resetting RemoteConfigurationCache , each time the messages change. On each remote config message change we:

reset the cache so ApplyState passes to Unack

attempt to apply all the new state to all listeners

update ApplyState according to what happened in the listener

However, I do agree this creates a coupling between the cache and the way ApplyState is handled that is rather delicate, but not sure how this could be improved?

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

… fixed delay

dromanol · 2022-09-12T14:06:01Z

                {
-                    configurations ??= new List<byte[]>();
-                    configurations.Add(config.Contents);
+                    filteredConfigs ??= new List<NamedRawFile>();


Never seen this compact operator before. #TIL

dromanol · 2022-09-12T14:14:21Z

+            var request1 = await agent.WaitRcmRequestAndReturnLast();
+            CheckAckState(request1, expectedState, null, "First RCM call");
+            // even the request show the applied state seems extra time is needed before it's active
+            await Task.Delay(1500);


Could this be executed inside WaitRcmRequestAndReturnLast?

It could but it's not needed in TestRemoteConfigError(), for example, because we don't do any further calls.

I'm quite surprised it's needed at all, since if the client state has come through I thought that meant the config had been applied, so further waiting shouldn't be necessary. It might be interesting to find out at some point why it is necessary.

dromanol

LGTM. Furthermore, I guess Rcm test flakyness will be gone with the introduced changes.

OmerRaviv · 2022-09-14T07:22:53Z


        private void AcceptConfiguration(ProductConfigChangedEventArgs args)
        {
+            Log.Information("AcceptConfiguration: Starting ...");


Do we want to keep this logging here? Seems a bit chatty (these would pop up in logs every 5 seconds I assume?)

Or otherwise maybe just reduce log verbosity?

Thanks, these were for my own debugging, I deleted most of them, seems these two slipped thought. Fixed in ae8a05e.

Cool, thanks! Note that ae8a05e didn't include this deletion, maybe you haven't pushed the right commit in yet?

OmerRaviv

LGTM

OmerRaviv · 2022-09-14T10:01:11Z

+
+        private void CheckAckState(GetRcmRequest request, uint expectedState, string expectedError, string message)
+        {
+            var state = request?.Client?.State?.ConfigStates?.FirstOrDefault(x => x.Product == "FEATURES");


Suggested change

var state = request?.Client?.State?.ConfigStates?.FirstOrDefault(x => x.Product == "FEATURES");

var state = request?.Client?.State?.ConfigStates?.SingleOrDefault(x => x.Product == "FEATURES");

Will add to #3186

andrewlock · 2022-09-14T10:12:48Z

Benchmarks Report 🐌

Benchmarks for #3175 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	720μs	916ns	3.55μs	0.359	3.18 KB
master	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	458μs	649ns	2.52μs	0	2.58 KB
#3175	`WriteAndFlushEnrichedTraces`	net472	722μs	791ns	3.06μs	0.357	3.18 KB
#3175	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	464μs	588ns	2.28μ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	190ns	0.13ns	0.467ns	0.0676	0	425 B
master	`AllCycleSimpleBody`	netcoreapp3.1	236ns	0.322ns	1.25ns	0.00581	0	424 B
master	`AllCycleMoreComplexBody`	net472	188ns	0.105ns	0.377ns	0.0638	0	401 B
master	`AllCycleMoreComplexBody`	netcoreapp3.1	236ns	0.297ns	1.11ns	0.00546	0	400 B
master	`BodyExtractorSimpleBody`	net472	256ns	0.244ns	0.946ns	0.0573	0	361 B
master	`BodyExtractorSimpleBody`	netcoreapp3.1	222ns	0.191ns	0.714ns	0.00369	0	272 B
master	`BodyExtractorMoreComplexBody`	net472	14.4μs	17.1ns	66.1ns	1.21	0.0217	7.62 KB
master	`BodyExtractorMoreComplexBody`	netcoreapp3.1	12.4μs	17.2ns	64.2ns	0.0869	0	6.75 KB
#3175	`AllCycleSimpleBody`	net472	189ns	0.134ns	0.5ns	0.0675	0	425 B
#3175	`AllCycleSimpleBody`	netcoreapp3.1	239ns	0.292ns	1.13ns	0.0058	0	424 B
#3175	`AllCycleMoreComplexBody`	net472	181ns	0.0756ns	0.273ns	0.0637	0	401 B
#3175	`AllCycleMoreComplexBody`	netcoreapp3.1	233ns	0.303ns	1.09ns	0.00551	0	400 B
#3175	`BodyExtractorSimpleBody`	net472	258ns	0.219ns	0.819ns	0.0574	0	361 B
#3175	`BodyExtractorSimpleBody`	netcoreapp3.1	213ns	0.261ns	1.01ns	0.00375	0	272 B
#3175	`BodyExtractorMoreComplexBody`	net472	14.6μs	15.3ns	59.1ns	1.2	0.0219	7.62 KB
#3175	`BodyExtractorMoreComplexBody`	netcoreapp3.1	12.4μs	7.93ns	29.7ns	0.0924	0	6.75 KB

Benchmarks.Trace.AspNetCoreBenchmark - Same speed ✔️ 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	316ns	1.23μs	0.271	20.6 KB
#3175	`SendRequest`	net472	0ns	0ns	0ns	0	0 b
#3175	`SendRequest`	netcoreapp3.1	183μs	208ns	805ns	0.181	20.58 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`ExecuteNonQuery`	net472	1.73μs	2.1ns	7.84ns	0.15	947 B
master	`ExecuteNonQuery`	netcoreapp3.1	1.43μs	0.867ns	3.24ns	0.0122	936 B
#3175	`ExecuteNonQuery`	net472	1.87μs	1.75ns	6.76ns	0.15	947 B
#3175	`ExecuteNonQuery`	netcoreapp3.1	1.42μs	1.2ns	4.65ns	0.0122	936 B

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`CallElasticsearch`	net472	2.52μs	2.8ns	10.8ns	0.183	1.16 KB
master	`CallElasticsearch`	netcoreapp3.1	1.57μs	0.913ns	3.42ns	0.0149	1.1 KB
master	`CallElasticsearchAsync`	net472	2.73μs	1.81ns	7.01ns	0.204	1.29 KB
master	`CallElasticsearchAsync`	netcoreapp3.1	1.61μs	0.714ns	2.67ns	0.0162	1.22 KB
#3175	`CallElasticsearch`	net472	2.67μs	2.68ns	10.4ns	0.183	1.16 KB
#3175	`CallElasticsearch`	netcoreapp3.1	1.58μs	0.683ns	2.56ns	0.0142	1.1 KB
#3175	`CallElasticsearchAsync`	net472	2.71μs	2.15ns	8.32ns	0.204	1.29 KB
#3175	`CallElasticsearchAsync`	netcoreapp3.1	1.71μs	0.594ns	2.22ns	0.0163	1.22 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`ExecuteAsync`	net472	2.72μs	7.54ns	28.2ns	0.225	0.00135	1.42 KB
master	`ExecuteAsync`	netcoreapp3.1	1.76μs	3.48ns	13.5ns	0.0185	0	1.34 KB
#3175	`ExecuteAsync`	net472	2.82μs	7.06ns	27.4ns	0.226	0	1.42 KB
#3175	`ExecuteAsync`	netcoreapp3.1	1.73μs	3.86ns	15ns	0.0179	0	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.67μs	8.76ns	33.9ns	0.44	0	2.77 KB
master	`SendAsync`	netcoreapp3.1	3.64μs	7.35ns	27.5ns	0.0344	0.00181	2.6 KB
#3175	`SendAsync`	net472	5.74μs	13ns	48.8ns	0.438	0.00286	2.77 KB
#3175	`SendAsync`	netcoreapp3.1	3.57μs	8.57ns	33.2ns	0.0355	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.22μs	1.67ns	6.49ns	0.287	1.81 KB
master	`EnrichedLog`	netcoreapp3.1	2.59μs	0.974ns	3.51ns	0.0254	1.85 KB
#3175	`EnrichedLog`	net472	3.09μs	4.07ns	15.8ns	0.287	1.81 KB
#3175	`EnrichedLog`	netcoreapp3.1	2.48μs	0.963ns	3.6ns	0.0248	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	153μs	216ns	835ns	0.684	0.076	4.66 KB
master	`EnrichedLog`	netcoreapp3.1	118μs	304ns	1.18μs	0.0586	0	4.49 KB
#3175	`EnrichedLog`	net472	153μs	178ns	689ns	0.692	0.231	4.65 KB
#3175	`EnrichedLog`	netcoreapp3.1	118μs	125ns	485ns	0.0587	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.59μs	10.3ns	37.1ns	0.569	0.0028	3.59 KB
master	`EnrichedLog`	netcoreapp3.1	4.28μs	7.24ns	27.1ns	0.0525	0.0021	3.91 KB
#3175	`EnrichedLog`	net472	5.55μs	11ns	41ns	0.57	0.00275	3.59 KB
#3175	`EnrichedLog`	netcoreapp3.1	4.33μs	8.58ns	33.2ns	0.054	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.37μs	1.15ns	4.29ns	0.218	1.37 KB
master	`SendReceive`	netcoreapp3.1	1.88μs	1.97ns	7.36ns	0.0181	1.32 KB
#3175	`SendReceive`	net472	2.31μs	2.4ns	9.3ns	0.217	1.37 KB
#3175	`SendReceive`	netcoreapp3.1	1.87μs	0.517ns	1.93ns	0.0177	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.06μs	3.59ns	13.4ns	0.354	0.00253	2.23 KB
master	`EnrichedLog`	netcoreapp3.1	4.36μs	1.32ns	5.12ns	0.0239	0	1.8 KB
#3175	`EnrichedLog`	net472	5.11μs	2.37ns	8.86ns	0.354	0	2.23 KB
#3175	`EnrichedLog`	netcoreapp3.1	4.32μs	1.82ns	6.83ns	0.024	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.17μs	0.483ns	1.81ns	0.129	810 B
master	`StartFinishSpan`	netcoreapp3.1	956ns	0.403ns	1.45ns	0.00996	760 B
master	`StartFinishScope`	net472	1.41μs	0.587ns	2.27ns	0.142	891 B
master	`StartFinishScope`	netcoreapp3.1	1.07μs	0.637ns	2.3ns	0.0119	880 B
#3175	`StartFinishSpan`	net472	1.15μs	0.374ns	1.4ns	0.129	810 B
#3175	`StartFinishSpan`	netcoreapp3.1	919ns	0.418ns	1.51ns	0.0106	760 B
#3175	`StartFinishScope`	net472	1.4μs	0.692ns	2.68ns	0.141	891 B
#3175	`StartFinishScope`	netcoreapp3.1	1.08μs	0.178ns	0.615ns	0.0119	880 B

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`RunOnMethodBegin`	net472	1.5μs	1.05ns	4.07ns	0.142	891 B
master	`RunOnMethodBegin`	netcoreapp3.1	1.13μs	0.639ns	2.39ns	0.0119	880 B
#3175	`RunOnMethodBegin`	net472	1.53μs	0.668ns	2.59ns	0.141	891 B
#3175	`RunOnMethodBegin`	netcoreapp3.1	1.18μs	0.492ns	1.91ns	0.0119	880 B

andrewlock · 2022-09-14T11:15:25Z

Code Coverage Report 📊

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

	master	#3175	Change
Lines	`17971` / `24517`	`17982` / `24566`
Lines %	`73%`	`73%`	`0%` ✔️
Branches	`10585` / `15050`	`10600` / `15056`
Branches %	`70%`	`70%`	`0%` ✔️
Complexity	`16261`	`16280`	`19` ⛔

View the full report for further details:

Datadog.Trace Breakdown ✔️

	master	#3175	Change
Lines %	`73%`	`73%`	`0%` ✔️
Branches %	`70%`	`70%`	`0%` ✔️
Complexity	`16261`	`16280`	`19` ⛔

The following classes have significant coverage changes.

File	Line coverage change	Branch coverage change	Complexity change
Datadog.Trace.RemoteConfigurationManagement.RemoteConfigurationCache	`-86%` ⛔	`0%` ✔️	`2` ⛔
Datadog.Trace.RemoteConfigurationManagement.ProductConfigChangedEventArgs	`-68%` ⛔	`-67%` ⛔	`6` ⛔
Datadog.Trace.RemoteConfigurationManagement.Product	`-31%` ⛔	`-29%` ⛔	`9` ⛔
Datadog.Trace.Ci.GitInfo	`-17%` ⛔	`-11%` ⛔	`0` ✔️
Datadog.Trace.Ci.CIVisibility	`5%` ✔️	`6%` ✔️	`0` ✔️

The following classes were added in #3175:

File	Line coverage	Branch coverage	Complexity
Datadog.Trace.RemoteConfigurationManagement.ApplyDetails	`0%`	`100%`	`2`
Datadog.Trace.RemoteConfigurationManagement.NamedRawFile	`100%`	`100%`	`1`
Datadog.Trace.RemoteConfigurationManagement.NamedTypedFile`1	`100%`	`100%`	`1`

View the full reports for further details:

robertpi requested review from a team as code owners September 6, 2022 14:48

robertpi force-pushed the robert/asm/rcte2 branch from 141a21f to ae1aed7 Compare September 6, 2022 14:50

This comment has been minimized.

Sign in to view

andrewlock reviewed Sep 7, 2022

View reviewed changes

robertpi force-pushed the robert/asm/rcte2 branch from e7f037b to 7c1753d Compare September 7, 2022 11:50

This comment has been minimized.

Sign in to view

robertpi and others added 10 commits September 12, 2022 10:36

Rough cut of RCTE2 ack might look like

612b4a7

Tests for the ACK

b59e194

convert small data holding containers to structs

b5274bb

Fix changes broken my last commit and some minor refactoring

664988f

Apply suggestions from code review

c9d3dfa

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

fix type and use nullable where we can

d1b2433

remove file that reappeared due to rebase

89b60be

correct data reterival

1bfd873

attempt to avoid flake by waiting a correct ack message rather than a…

8b3ff5d

… fixed delay

correct rebase issue

7133598

robertpi force-pushed the robert/asm/rcte2 branch from 8bdccfd to 7133598 Compare September 12, 2022 08:54

This comment has been minimized.

Sign in to view

Up wait time to reduce flake

22df180

This comment has been minimized.

Sign in to view

dromanol reviewed Sep 12, 2022

View reviewed changes

dromanol approved these changes Sep 12, 2022

View reviewed changes

OmerRaviv reviewed Sep 14, 2022

View reviewed changes

Comment thread tracer/src/Datadog.Trace/RemoteConfigurationManagement/RemoteConfigurationSettings.cs Outdated

OmerRaviv reviewed Sep 14, 2022

View reviewed changes

Comment thread tracer/test/Datadog.Trace.Security.IntegrationTests/AspNetCore5AsmToggle.cs

OmerRaviv reviewed Sep 14, 2022

View reviewed changes

Comment thread tracer/src/Datadog.Trace/RemoteConfigurationManagement/RemoteConfigurationCache.cs

OmerRaviv approved these changes Sep 14, 2022

View reviewed changes

fix issues raised by Omer

ae8a05e

OmerRaviv reviewed Sep 14, 2022

View reviewed changes

Comment thread tracer/test/Datadog.Trace.Security.IntegrationTests/AspNetCore5AsmToggle.cs

robertpi merged commit f856a98 into master Sep 14, 2022

robertpi deleted the robert/asm/rcte2 branch September 14, 2022 15:10

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

	var state = request?.Client?.State?.ConfigStates?.FirstOrDefault(x => x.Product == "FEATURES");
	var state = request?.Client?.State?.ConfigStates?.SingleOrDefault(x => x.Product == "FEATURES");

Uh oh!

Conversation

robertpi commented Sep 6, 2022

Summary of changes

Reason for change

Implementation details

Test coverage

Uh oh!

This comment has been minimized.

This comment has been minimized.

andrewlock left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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.

Choose a reason for hiding this comment

Uh oh!

dromanol Sep 12, 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!

Choose a reason for hiding this comment

Uh oh!

dromanol left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

Uh oh!

Uh oh!

Uh oh!

OmerRaviv left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

dromanol Sep 12, 2022 •

edited

Loading

OmerRaviv Sep 14, 2022 •

edited

Loading