Skip to content

Fix fairly subtle bug with remote config feature ASM_DD#3344

Merged
robertpi merged 3 commits into
masterfrom
robert/asm/fix-rc-asm-dd
Oct 17, 2022
Merged

Fix fairly subtle bug with remote config feature ASM_DD#3344
robertpi merged 3 commits into
masterfrom
robert/asm/fix-rc-asm-dd

Conversation

@robertpi

@robertpi robertpi commented Oct 12, 2022

Copy link
Copy Markdown
Member

Summary of changes

Handle how the data is formatted in production.

Reason for change

The feature completely broken without this fix.

Implementation details

Our integration had a subtle difference in the test data sent from the actual data in prod. The Integration tests we were sending everything wrapped in double quotes:

"{  "version": "2.2",  "metadata": {   "rules_version": "1.4.2"  }, ... "

In prod the data looked like:

{  "version": "2.2",  "metadata": {   "rules_version": "1.4.2"  }, ...

@robertpi
robertpi requested review from a team as code owners October 12, 2022 12:37
andrewlock
andrewlock approved these changes Oct 12, 2022
View reviewed changes

@andrewlock andrewlock left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have we tried testing this manually locally? Given this is the second time it's broken, feels like we should make sure it definitely works...

Comment thread tracer/src/Datadog.Trace/RemoteConfigurationManagement/Transport/RemoteConfigurationApi.cs Outdated
@andrewlock

This comment has been minimized.

Sign in to view
@robertpi

robertpi commented Oct 12, 2022

Copy link
Copy Markdown
Member Author

Have we tried testing this manually locally? Given this is the second time it's broken, feels like we should make sure it definitely works...

The end points for full end to end manual testing are only just becoming available, that's what a lot of these bugs are cropping up now.

@andrewlock

andrewlock commented Oct 12, 2022

Copy link
Copy Markdown
Member

Benchmarks Report 🐌

Benchmarks for #3344 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 Gen 1 Gen 2 Allocated
master WriteAndFlushEnrichedTraces net472 790μs 591ns 2.29μs 0.396 0 0 3.22 KB
master WriteAndFlushEnrichedTraces netcoreapp3.1 538μs 224ns 808ns 0 0 0 2.63 KB
#3344 WriteAndFlushEnrichedTraces net472 786μs 730ns 2.83μs 0.398 0 0 3.22 KB
#3344 WriteAndFlushEnrichedTraces netcoreapp3.1 537μs 153ns 572ns 0 0 0 2.63 KB
Benchmarks.Trace.AppSecBodyBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master AllCycleSimpleBody net472 197ns 0.0724ns 0.28ns 0.0726 0 0 457 B
master AllCycleSimpleBody netcoreapp3.1 254ns 0.193ns 0.749ns 0.00625 0 0 456 B
master AllCycleMoreComplexBody net472 196ns 0.0741ns 0.277ns 0.0688 0 0 433 B
master AllCycleMoreComplexBody netcoreapp3.1 250ns 0.335ns 1.3ns 0.00592 0 0 432 B
master BodyExtractorSimpleBody net472 273ns 0.125ns 0.449ns 0.0573 0 0 361 B
master BodyExtractorSimpleBody netcoreapp3.1 240ns 0.243ns 0.909ns 0.00374 0 0 272 B
master BodyExtractorMoreComplexBody net472 15.8μs 16.2ns 62.9ns 1.2 0.0157 0 7.62 KB
master BodyExtractorMoreComplexBody netcoreapp3.1 12.8μs 5.76ns 21.5ns 0.0891 0 0 6.75 KB
#3344 AllCycleSimpleBody net472 205ns 0.106ns 0.41ns 0.0726 0 0 457 B
#3344 AllCycleSimpleBody netcoreapp3.1 253ns 0.117ns 0.439ns 0.00633 0 0 456 B
#3344 AllCycleMoreComplexBody net472 196ns 0.126ns 0.489ns 0.0688 0 0 433 B
#3344 AllCycleMoreComplexBody netcoreapp3.1 254ns 1.21ns 4.52ns 0.00594 0 0 432 B
#3344 BodyExtractorSimpleBody net472 272ns 0.144ns 0.539ns 0.0573 0 0 361 B
#3344 BodyExtractorSimpleBody netcoreapp3.1 243ns 0.118ns 0.442ns 0.00375 0 0 272 B
#3344 BodyExtractorMoreComplexBody net472 16.2μs 15.1ns 58.6ns 1.21 0.016 0 7.62 KB
#3344 BodyExtractorMoreComplexBody netcoreapp3.1 12.8μs 3.92ns 15.2ns 0.0894 0 0 6.75 KB
Benchmarks.Trace.AspNetCoreBenchmark - Unknown 🤷 Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master SendRequest net472 0ns 0ns 0ns 0 0 0 0 b
master SendRequest netcoreapp3.1 181μs 402ns 1.56μs 0.269 0 0 20.68 KB
#3344 SendRequest net472 0ns 0ns 0ns 0 0 0 0 b
#3344 SendRequest netcoreapp3.1 180μs 215ns 804ns 0.269 0 0 20.68 KB
Benchmarks.Trace.DbCommandBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master ExecuteNonQuery net472 1.8μs 1.15ns 4.32ns 0.155 0.000892 0 979 B
master ExecuteNonQuery netcoreapp3.1 1.39μs 0.546ns 2.11ns 0.0132 0 0 968 B
#3344 ExecuteNonQuery net472 1.76μs 0.917ns 3.43ns 0.155 0.000885 0 979 B
#3344 ExecuteNonQuery netcoreapp3.1 1.48μs 0.51ns 1.76ns 0.0127 0 0 968 B
Benchmarks.Trace.ElasticsearchBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master CallElasticsearch net472 2.48μs 0.903ns 3.5ns 0.189 0 0 1.19 KB
master CallElasticsearch netcoreapp3.1 1.65μs 0.747ns 2.8ns 0.0149 0 0 1.13 KB
master CallElasticsearchAsync net472 2.55μs 1.04ns 3.74ns 0.21 0 0 1.32 KB
master CallElasticsearchAsync netcoreapp3.1 1.66μs 0.502ns 1.81ns 0.0166 0 0 1.25 KB
#3344 CallElasticsearch net472 2.56μs 0.719ns 2.78ns 0.188 0 0 1.19 KB
#3344 CallElasticsearch netcoreapp3.1 1.57μs 0.651ns 2.44ns 0.0148 0 0 1.13 KB
#3344 CallElasticsearchAsync net472 2.64μs 0.94ns 3.52ns 0.21 0 0 1.32 KB
#3344 CallElasticsearchAsync netcoreapp3.1 1.64μs 1.47ns 5.52ns 0.0163 0 0 1.25 KB
Benchmarks.Trace.GraphQLBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master ExecuteAsync net472 2.71μs 1.8ns 6.74ns 0.23 0 0 1.45 KB
master ExecuteAsync netcoreapp3.1 1.75μs 0.249ns 0.933ns 0.0181 0 0 1.38 KB
#3344 ExecuteAsync net472 2.83μs 1.17ns 4.21ns 0.231 0 0 1.45 KB
#3344 ExecuteAsync netcoreapp3.1 1.72μs 0.731ns 2.74ns 0.0189 0 0 1.38 KB
Benchmarks.Trace.HttpClientBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master SendAsync net472 5.8μs 1.89ns 7.06ns 0.443 0 0 2.8 KB
master SendAsync netcoreapp3.1 3.64μs 1ns 3.75ns 0.0346 0 0 2.63 KB
#3344 SendAsync net472 5.93μs 2.41ns 9.01ns 0.443 0 0 2.8 KB
#3344 SendAsync netcoreapp3.1 3.72μs 1.29ns 4.64ns 0.0353 0 0 2.63 KB
Benchmarks.Trace.ILoggerBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master EnrichedLog net472 3.22μs 4.25ns 16.5ns 0.298 0 0 1.88 KB
master EnrichedLog netcoreapp3.1 2.58μs 1.14ns 4.27ns 0.0259 0 0 1.91 KB
#3344 EnrichedLog net472 3.14μs 3.37ns 13.1ns 0.298 0 0 1.88 KB
#3344 EnrichedLog netcoreapp3.1 2.52μs 0.958ns 3.58ns 0.0266 0 0 1.91 KB
Benchmarks.Trace.Log4netBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master EnrichedLog net472 150μs 278ns 1.08μs 0.679 0.226 0 4.72 KB
master EnrichedLog netcoreapp3.1 118μs 206ns 741ns 0 0 0 4.55 KB
#3344 EnrichedLog net472 153μs 189ns 733ns 0.69 0.23 0 4.72 KB
#3344 EnrichedLog netcoreapp3.1 119μs 164ns 634ns 0.0595 0 0 4.55 KB
Benchmarks.Trace.NLogBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master EnrichedLog net472 5.81μs 2.08ns 7.5ns 0.579 0.00291 0 3.65 KB
master EnrichedLog netcoreapp3.1 4.44μs 1.77ns 6.84ns 0.0533 0 0 3.98 KB
#3344 EnrichedLog net472 5.8μs 2.13ns 8.26ns 0.579 0.00289 0 3.65 KB
#3344 EnrichedLog netcoreapp3.1 4.7μs 1.46ns 5.04ns 0.0538 0 0 3.98 KB
Benchmarks.Trace.RedisBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master SendReceive net472 2.29μs 2.52ns 9.74ns 0.223 0 0 1.4 KB
master SendReceive netcoreapp3.1 1.79μs 1.14ns 4.27ns 0.0179 0 0 1.35 KB
#3344 SendReceive net472 2.27μs 3.08ns 10.7ns 0.222 0 0 1.4 KB
#3344 SendReceive netcoreapp3.1 1.81μs 0.579ns 2.24ns 0.0181 0 0 1.35 KB
Benchmarks.Trace.SerilogBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master EnrichedLog net472 5.11μs 2.61ns 9.77ns 0.364 0 0 2.3 KB
master EnrichedLog netcoreapp3.1 4.28μs 1.4ns 5.24ns 0.0256 0 0 1.86 KB
#3344 EnrichedLog net472 5.13μs 1.9ns 7.34ns 0.363 0 0 2.3 KB
#3344 EnrichedLog netcoreapp3.1 4.44μs 1.53ns 5.92ns 0.0243 0 0 1.86 KB
Benchmarks.Trace.SpanBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master StartFinishSpan net472 1.17μs 0.58ns 2.25ns 0.133 0 0 842 B
master StartFinishSpan netcoreapp3.1 973ns 0.434ns 1.68ns 0.0106 0 0 792 B
master StartFinishScope net472 1.46μs 1.12ns 4.36ns 0.146 0 0 923 B
master StartFinishScope netcoreapp3.1 1.09μs 0.473ns 1.77ns 0.0125 0 0 912 B
#3344 StartFinishSpan net472 1.2μs 0.341ns 1.23ns 0.134 0 0 842 B
#3344 StartFinishSpan netcoreapp3.1 977ns 0.503ns 1.88ns 0.0108 0 0 792 B
#3344 StartFinishScope net472 1.42μs 0.963ns 3.73ns 0.146 0 0 923 B
#3344 StartFinishScope netcoreapp3.1 1.13μs 0.473ns 1.71ns 0.0124 0 0 912 B
Benchmarks.Trace.TraceAnnotationsBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master RunOnMethodBegin net472 1.47μs 0.451ns 1.75ns 0.146 0 0 923 B
master RunOnMethodBegin netcoreapp3.1 1.22μs 0.455ns 1.57ns 0.0123 0 0 912 B
#3344 RunOnMethodBegin net472 1.5μs 0.354ns 1.37ns 0.146 0 0 923 B
#3344 RunOnMethodBegin netcoreapp3.1 1.15μs 0.498ns 1.86ns 0.0126 0 0 912 B

@andrewlock

andrewlock commented Oct 17, 2022

Copy link
Copy Markdown
Member

Code Coverage Report 📊

✔️ Merging #3344 into master will not change line coverage
✔️ Merging #3344 into master will not change branch coverage
✔️ Merging #3344 into master will will decrease complexity by 190

master #3344 Change
Lines 19543 / 27004 19400 / 26728
Lines % 72% 73% 0% ✔️
Branches 11598 / 16782 11543 / 16631
Branches % 69% 69% 0% ✔️
Complexity 18158 17968 -190 ✔️

View the full report for further details:

Datadog.Trace Breakdown ✔️

master #3344 Change
Lines % 72% 73% 0% ✔️
Branches % 69% 69% 0% ✔️
Complexity 18158 17968 -190 ✔️

The following classes have significant coverage changes.

File Line coverage change Branch coverage change Complexity change
Datadog.Trace.RemoteConfigurationManagement.ProductConfigChangedEventArgs -5% ⛔ -8% ⛔ 2 ⛔
Datadog.Trace.TraceContext -4% ⚠️ -6% ⛔ 14 ⛔
Datadog.Trace.ClrProfiler.AutoInstrumentation.Testing.MsTestV2.MsTestIntegration 3% ✔️ 11% ✔️ -10 ✔️
Datadog.Trace.Debugger.PInvoke.DebuggerNativeMethods 6% ✔️ 0% ✔️ 0 ✔️
Datadog.Trace.ClrProfiler.AutoInstrumentation.Http.HttpClient.WinHttpHandler.WinHttpHandlerIntegration 100% ✔️ 0% ✔️ 0 ✔️

20 classes were removed from Datadog.Trace in #3344

View the full reports for further details:

@robertpi
robertpi merged commit e439b59 into master Oct 17, 2022
@robertpi
robertpi deleted the robert/asm/fix-rc-asm-dd branch October 17, 2022 14:17
@github-actions github-actions Bot added this to the vNext milestone Oct 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zacharycmontoya zacharycmontoya zacharycmontoya approved these changes
@andrewlock andrewlock andrewlock approved these changes
@dromanol dromanol dromanol approved these changes

Assignees

No one assigned

Labels

area:remote-config

Projects

None yet

Milestone

2.18.0

Development

Successfully merging this pull request may close these issues.

5 participants

@robertpi @andrewlock @zacharycmontoya @dromanol @anna-git