Features rename by robertpi · Pull Request #3223 · DataDog/dd-trace-dotnet

robertpi · 2022-09-16T15:10:24Z

Summary of changes

We'll rename the remote config products "FEATURES" to "ASM_FEATURES" and make this product specific to ASM.

Reason for change

It will be difficult to share products between different sub-systems as we'll be ACKing each product, so if two different sub-system have a different ACK status it will create confusion.

robertpi · 2022-09-19T15:42:30Z

Convert to draft, since spec might be changing again.

andrewlock

Don't really understand how all the RCM stuff fits together tbh, but I'll take your word for it this is a sensible change 😄

andrewlock · 2022-09-21T09:28:16Z

+
+namespace Datadog.Trace.AppSec;
+
+internal class AsmRemoteConfigurationProducts


nit: could make this static too

Good point. I'll make the change in a future PR to avoid a new CI run.

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

+
+internal class AsmRemoteConfigurationProducts
+{
+    public static AsmFeaturesProduct AsmFeaturesProduct { get; } = new();


Just for my understanding, are there going to be extra ASM "products" in here too other than "features"? I guess I don't understand what the difference between a "feature" and a "product" is, if ASM is not a product?

Remote config groups it's payloads into "products". The client will subscribe to a product and changes to this product will be pushed to the subscriber.

"FEATURES" was the name of a product. (Product names are always block capitals I guess the RC team hangs out on #ALL-CAPS-TYPING) The features product was design to contain the activation flags for different the different library subsystems (ASM, live debugger etc.), but we decided sharing config between different sub-systems was a bad idea for various reasons. This meant the features became exclusively owned by ASM, so we renamed it.

Product names are always block capitals I guess the RC team hangs out on #ALL-CAPS-TYPING

😂

The features product was design to contain the activation flags for different the different library subsystems (ASM, live debugger etc.), but we decided sharing config between different sub-systems was a bad idea for various reasons. This meant the features became exclusively owned by ASM, so we renamed it.

I see, that makes sense, thanks. I misread this as an "our code only" change, as opposed to an "RCM" change.

dromanol

I liked the idea of having all the products in the same place (SharedRemoteConfiguration) instead of segregating by "area", but it's a personal preference. LGTM.

robertpi · 2022-09-21T11:37:53Z

I liked the idea of having all the products in the same place (SharedRemoteConfiguration) instead of segregating by "area", but it's a personal preference. LGTM.

I think we should group the ASM remote configs in AsmRemoteConfigurationProducts, I think it's probably better not share them with other teams since we're trying to keep the subsystems as independent as possible.

andrewlock · 2022-09-21T12:11:42Z

Benchmarks Report 🐌

Benchmarks for #3223 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
#3223	`WriteAndFlushEnrichedTraces`	net472	712μs	524ns	2.03μs	0.357	3.18 KB
#3223	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	474μs	214ns	802ns	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
#3223	`AllCycleSimpleBody`	net472	184ns	0.152ns	0.567ns	0.0676	0	425 B
#3223	`AllCycleSimpleBody`	netcoreapp3.1	240ns	0.245ns	0.949ns	0.00587	0	424 B
#3223	`AllCycleMoreComplexBody`	net472	182ns	0.212ns	0.822ns	0.0637	0	401 B
#3223	`AllCycleMoreComplexBody`	netcoreapp3.1	233ns	0.15ns	0.563ns	0.00551	0	400 B
#3223	`BodyExtractorSimpleBody`	net472	258ns	0.167ns	0.602ns	0.0574	0	361 B
#3223	`BodyExtractorSimpleBody`	netcoreapp3.1	215ns	0.237ns	0.918ns	0.00377	0	272 B
#3223	`BodyExtractorMoreComplexBody`	net472	14.8μs	9.4ns	35.2ns	1.21	0.0147	7.62 KB
#3223	`BodyExtractorMoreComplexBody`	netcoreapp3.1	11.9μs	15.1ns	58.3ns	0.0891	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	179μs	134ns	517ns	0.268	20.6 KB
#3223	`SendRequest`	net472	0ns	0ns	0ns	0	0 b
#3223	`SendRequest`	netcoreapp3.1	183μs	231ns	865ns	0.183	20.6 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
#3223	`ExecuteNonQuery`	net472	1.91μs	0.972ns	3.51ns	0.15	0.000959	947 B
#3223	`ExecuteNonQuery`	netcoreapp3.1	1.49μs	0.704ns	2.64ns	0.0127	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
#3223	`CallElasticsearch`	net472	2.54μs	1.95ns	7.54ns	0.183	0	1.16 KB
#3223	`CallElasticsearch`	netcoreapp3.1	1.56μs	0.766ns	2.97ns	0.0148	0	1.1 KB
#3223	`CallElasticsearchAsync`	net472	2.65μs	2ns	7.75ns	0.205	0	1.29 KB
#3223	`CallElasticsearchAsync`	netcoreapp3.1	1.61μs	0.614ns	2.3ns	0.0161	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
#3223	`ExecuteAsync`	net472	2.98μs	2.42ns	9.38ns	0.226	1.42 KB
#3223	`ExecuteAsync`	netcoreapp3.1	1.76μs	1.14ns	4.28ns	0.0176	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
#3223	`SendAsync`	net472	5.86μs	3.05ns	11.4ns	0.438	2.77 KB
#3223	`SendAsync`	netcoreapp3.1	3.73μs	8.16ns	31.6ns	0.0359	2.6 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`EnrichedLog`	net472	3.25μs	15.5ns	59.8ns	0.287	0	1.81 KB
master	`EnrichedLog`	netcoreapp3.1	2.48μs	13.5ns	109ns	0.0242	0	1.85 KB
#3223	`EnrichedLog`	net472	3.17μs	5.44ns	20.4ns	0.288	0.00157	1.81 KB
#3223	`EnrichedLog`	netcoreapp3.1	2.51μs	3.08ns	11.9ns	0.025	0	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
#3223	`EnrichedLog`	net472	153μs	224ns	866ns	0.682	0	4.65 KB
#3223	`EnrichedLog`	netcoreapp3.1	116μs	148ns	572ns	0.0579	0	4.5 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
#3223	`EnrichedLog`	net472	5.8μs	13.2ns	51.3ns	0.569	0.00287	3.59 KB
#3223	`EnrichedLog`	netcoreapp3.1	4.46μs	4.6ns	17.8ns	0.0524	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
#3223	`SendReceive`	net472	2.18μs	2.25ns	8.43ns	0.218	1.37 KB
#3223	`SendReceive`	netcoreapp3.1	1.88μs	0.995ns	3.85ns	0.0179	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
#3223	`EnrichedLog`	net472	5.02μs	2.81ns	10.5ns	0.353	2.23 KB
#3223	`EnrichedLog`	netcoreapp3.1	4.42μs	4.25ns	15.3ns	0.0242	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
#3223	`StartFinishSpan`	net472	1.11μs	0.481ns	1.86ns	0.128	810 B
#3223	`StartFinishSpan`	netcoreapp3.1	959ns	0.329ns	1.23ns	0.0102	760 B
#3223	`StartFinishScope`	net472	1.41μs	1.22ns	4.71ns	0.141	891 B
#3223	`StartFinishScope`	netcoreapp3.1	1.07μs	2.08ns	7.51ns	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
#3223	`RunOnMethodBegin`	net472	1.45μs	0.919ns	3.56ns	0.141	891 B
#3223	`RunOnMethodBegin`	netcoreapp3.1	1.19μs	0.369ns	1.33ns	0.0119	880 B

andrewlock · 2022-09-21T15:48:34Z

Code Coverage Report 📊

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

	master	#3223	Change
Lines	`17963` / `24889`	`16810` / `24930`
Lines %	`72%`	`67%`	`-5%` ⛔
Branches	`10472` / `15254`	`10001` / `15274`
Branches %	`69%`	`65%`	`-3%` ⚠️
Complexity	`16477`	`16497`	`20` ⛔

View the full report for further details:

Datadog.Trace Breakdown ⛔

	master	#3223	Change
Lines %	`72%`	`67%`	`-5%` ⛔
Branches %	`69%`	`65%`	`-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 51 more

The following classes were added in #3223:

File	Line coverage	Branch coverage	Complexity
Datadog.Trace.AppSec.AsmFeaturesProduct	`100%`	`100%`	`1`
Datadog.Trace.AppSec.AsmRemoteConfigurationProducts	`100%`	`100%`	`1`

2 classes were removed from Datadog.Trace in #3223

View the full reports for further details:

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

This comment has been minimized.

Sign in to view

robertpi marked this pull request as draft September 19, 2022 15:42

robertpi marked this pull request as ready for review September 21, 2022 09:25

andrewlock approved these changes Sep 21, 2022

View reviewed changes

dromanol approved these changes Sep 21, 2022

View reviewed changes

robertpi added 3 commits September 21, 2022 13:39

Features rename

56ddd91

update error message

9f55ec0

Keep Andrew happy

a2efe7f

robertpi force-pushed the robert/asm/features-product-not-shared branch from 2d6a26e to a2efe7f Compare September 21, 2022 11:41

anna-git added area:asm area:remote-config labels Sep 21, 2022

robertpi merged commit 02bcf9c into master Sep 21, 2022

robertpi deleted the robert/asm/features-product-not-shared branch September 21, 2022 15:39

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


		namespace Datadog.Trace.AppSec;

		internal class AsmRemoteConfigurationProducts

Uh oh!

Conversation

robertpi commented Sep 16, 2022

Summary of changes

Reason for change

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

robertpi commented Sep 19, 2022

Uh oh!

andrewlock left a comment

Choose a reason for hiding this comment

Uh oh!

andrewlock Sep 21, 2022

Choose a reason for hiding this comment

Uh oh!

robertpi Sep 21, 2022

Choose a reason for hiding this comment

Uh oh!

andrewlock Sep 21, 2022

Choose a reason for hiding this comment

Uh oh!

robertpi Sep 21, 2022

Choose a reason for hiding this comment

Uh oh!

andrewlock Sep 21, 2022

Choose a reason for hiding this comment

Uh oh!

dromanol left a comment

Choose a reason for hiding this comment

Uh oh!

robertpi commented Sep 21, 2022

Uh oh!

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

Code Coverage Report 📊

Datadog.Trace Breakdown ⛔

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants