Skip to content

[IAST] Move analyzers init to an explicit call#5920

Merged
dromanol merged 1 commit into
masterfrom
dani/iast/hardcodedsecrets_init_fix
Aug 20, 2024
Merged

[IAST] Move analyzers init to an explicit call#5920
dromanol merged 1 commit into
masterfrom
dani/iast/hardcodedsecrets_init_fix

Conversation

@dromanol

@dromanol dromanol commented Aug 20, 2024

Copy link
Copy Markdown
Contributor

Summary of changes

Make IAST analyzers initialization explicit, and call it only in Instrumentation.Initialize to avoid multiple instances scenarios

Reason for change

A crash under these circumstances has been detected

Implementation details

Move IAST analyzers init inside Instrumentation.Initialize only to make sure they are initialized only once

Test coverage

Other details

@dromanol
dromanol requested review from a team as code owners August 20, 2024 09:46
@datadog-ddstaging

datadog-ddstaging Bot commented Aug 20, 2024 •
edited
Loading

Copy link
Copy Markdown

Datadog Report

Branch report: dani/iast/hardcodedsecrets_init_fix
Commit report: 59fb803
Test service: dd-trace-dotnet

✅ 0 Failed, 344564 Passed, 1780 Skipped, 15h 7m 14.82s Total Time

@andrewlock

andrewlock commented Aug 20, 2024

Copy link
Copy Markdown
Member

Execution-Time Benchmarks Report ⏱️

Execution-time results for samples comparing the following branches/commits:

Execution-time benchmarks measure the whole time it takes to execute a program. And are intended to measure the one-off costs. Cases where the execution time results for the PR are worse than latest master results are shown in red. The following thresholds were used for comparing the execution times:

  • Welch test with statistical test for significance of 5%
  • Only results indicating a difference greater than 5% and 5 ms are considered.

Note that these results are based on a single point-in-time result for each branch. For full results, see the dashboard.

Graphs show the p99 interval based on the mean and StdDev of the test run, as well as the mean value of the run (shown as a diamond below the graph).

@andrewlock

andrewlock commented Aug 20, 2024

Copy link
Copy Markdown
Member

Benchmarks Report for appsec 🐌

Benchmarks for #5920 compared to master:

  • 3 benchmarks are faster, with geometric mean 1.174
  • 1 benchmarks are slower, with geometric mean 1.134
  • 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.Asm.AppSecBodyBenchmark - Slower ⚠️ Same allocations ✔️

Slower ⚠️ in #5920

Benchmark diff/base Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.Asm.AppSecBodyBenchmark.ObjectExtractorSimpleBody‑netcoreapp3.1 1.134 210.62 238.88

Faster 🎉 in #5920

Benchmark base/diff Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.Asm.AppSecBodyBenchmark.ObjectExtractorMoreComplexBody‑net472 1.172 4,439.31 3,788.06
Benchmarks.Trace.Asm.AppSecBodyBenchmark.ObjectExtractorSimpleBody‑net472 1.157 196.29 169.61

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master AllCycleSimpleBody net6.0 74.3μs 109ns 422ns 0.0749 0 0 6 KB
master AllCycleSimpleBody netcoreapp3.1 62μs 76.1ns 295ns 0.0939 0 0 6.95 KB
master AllCycleSimpleBody net472 49.4μs 55.2ns 214ns 1.31 0 0 8.34 KB
master AllCycleMoreComplexBody net6.0 78.8μs 76.9ns 298ns 0.118 0 0 9.51 KB
master AllCycleMoreComplexBody netcoreapp3.1 69.2μs 67ns 260ns 0.138 0 0 10.36 KB
master AllCycleMoreComplexBody net472 55.9μs 65.8ns 246ns 1.87 0.0279 0 11.85 KB
master ObjectExtractorSimpleBody net6.0 140ns 0.117ns 0.452ns 0.00397 0 0 280 B
master ObjectExtractorSimpleBody netcoreapp3.1 210ns 0.136ns 0.526ns 0.0037 0 0 272 B
master ObjectExtractorSimpleBody net472 196ns 0.132ns 0.511ns 0.0446 0 0 281 B
master ObjectExtractorMoreComplexBody net6.0 3.06μs 1.67ns 6.24ns 0.0535 0 0 3.78 KB
master ObjectExtractorMoreComplexBody netcoreapp3.1 3.96μs 1.82ns 7.04ns 0.0495 0 0 3.69 KB
master ObjectExtractorMoreComplexBody net472 4.44μs 2.34ns 8.75ns 0.602 0.00666 0 3.8 KB
#5920 AllCycleSimpleBody net6.0 72.7μs 79.9ns 299ns 0.0731 0 0 6.01 KB
#5920 AllCycleSimpleBody netcoreapp3.1 62.2μs 81.9ns 306ns 0.0929 0 0 6.95 KB
#5920 AllCycleSimpleBody net472 49μs 52.6ns 189ns 1.3 0 0 8.34 KB
#5920 AllCycleMoreComplexBody net6.0 78.8μs 79.4ns 297ns 0.121 0 0 9.51 KB
#5920 AllCycleMoreComplexBody netcoreapp3.1 70.5μs 89.6ns 347ns 0.105 0 0 10.36 KB
#5920 AllCycleMoreComplexBody net472 55.8μs 43.2ns 167ns 1.86 0.0282 0 11.85 KB
#5920 ObjectExtractorSimpleBody net6.0 142ns 0.188ns 0.702ns 0.00394 0 0 280 B
#5920 ObjectExtractorSimpleBody netcoreapp3.1 239ns 0.163ns 0.586ns 0.00373 0 0 272 B
#5920 ObjectExtractorSimpleBody net472 169ns 0.34ns 1.32ns 0.0446 0 0 281 B
#5920 ObjectExtractorMoreComplexBody net6.0 3.04μs 2.12ns 7.94ns 0.054 0 0 3.78 KB
#5920 ObjectExtractorMoreComplexBody netcoreapp3.1 3.84μs 2.73ns 10.6ns 0.0499 0 0 3.69 KB
#5920 ObjectExtractorMoreComplexBody net472 3.79μs 2.58ns 9.64ns 0.603 0.00569 0 3.8 KB
Benchmarks.Trace.Asm.AppSecEncoderBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master EncodeArgs net6.0 38.1μs 22.9ns 88.8ns 0.454 0 0 32.4 KB
master EncodeArgs netcoreapp3.1 54.7μs 31.7ns 123ns 0.433 0 0 32.4 KB
master EncodeArgs net472 65.9μs 29.5ns 114ns 5.15 0.0664 0 32.5 KB
master EncodeLegacyArgs net6.0 80.5μs 172ns 667ns 0.0399 0 0 2.14 KB
master EncodeLegacyArgs netcoreapp3.1 108μs 109ns 420ns 0 0 0 2.14 KB
master EncodeLegacyArgs net472 151μs 33.6ns 126ns 0.302 0 0 2.15 KB
#5920 EncodeArgs net6.0 37.9μs 18ns 69.7ns 0.456 0 0 32.4 KB
#5920 EncodeArgs netcoreapp3.1 54.9μs 15.1ns 54.4ns 0.438 0 0 32.4 KB
#5920 EncodeArgs net472 66.7μs 42.3ns 164ns 5.16 0.0666 0 32.5 KB
#5920 EncodeLegacyArgs net6.0 77.2μs 422ns 2.43μs 0 0 0 2.14 KB
#5920 EncodeLegacyArgs netcoreapp3.1 105μs 254ns 982ns 0 0 0 2.15 KB
#5920 EncodeLegacyArgs net472 152μs 54ns 209ns 0.305 0 0 2.15 KB
Benchmarks.Trace.Asm.AppSecWafBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master RunWafRealisticBenchmark net6.0 188μs 67.1ns 251ns 0 0 0 2.42 KB
master RunWafRealisticBenchmark netcoreapp3.1 197μs 124ns 447ns 0 0 0 2.37 KB
master RunWafRealisticBenchmark net472 211μs 49.4ns 178ns 0.316 0 0 2.43 KB
master RunWafRealisticBenchmarkWithAttack net6.0 122μs 105ns 407ns 0 0 0 1.46 KB
master RunWafRealisticBenchmarkWithAttack netcoreapp3.1 131μs 70ns 271ns 0 0 0 1.45 KB
master RunWafRealisticBenchmarkWithAttack net472 140μs 38.4ns 144ns 0.21 0 0 1.48 KB
#5920 RunWafRealisticBenchmark net6.0 185μs 32.2ns 120ns 0 0 0 2.42 KB
#5920 RunWafRealisticBenchmark netcoreapp3.1 196μs 110ns 426ns 0 0 0 2.37 KB
#5920 RunWafRealisticBenchmark net472 209μs 51.8ns 194ns 0.314 0 0 2.43 KB
#5920 RunWafRealisticBenchmarkWithAttack net6.0 122μs 83.6ns 324ns 0 0 0 1.46 KB
#5920 RunWafRealisticBenchmarkWithAttack netcoreapp3.1 130μs 52.5ns 189ns 0 0 0 1.45 KB
#5920 RunWafRealisticBenchmarkWithAttack net472 139μs 45.7ns 171ns 0.209 0 0 1.48 KB
Benchmarks.Trace.Iast.StringAspectsBenchmark - Faster 🎉 Same allocations ✔️

Faster 🎉 in #5920

Benchmark base/diff Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatBenchmark‑netcoreapp3.1 1.192 63,200.00 53,000.00 bimodal

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master StringConcatBenchmark net6.0 59.9μs 752ns 7.44μs 0 0 0 43.44 KB
master StringConcatBenchmark netcoreapp3.1 64.1μs 911ns 8.93μs 0 0 0 42.64 KB
master StringConcatBenchmark net472 37μs 120ns 432ns 0 0 0 59.04 KB
master StringConcatAspectBenchmark net6.0 319μs 1.76μs 11.2μs 0 0 0 264.42 KB
master StringConcatAspectBenchmark netcoreapp3.1 336μs 1.21μs 4.37μs 0 0 0 255.34 KB
master StringConcatAspectBenchmark net472 277μs 5.54μs 54μs 0 0 0 278.53 KB
#5920 StringConcatBenchmark net6.0 61.1μs 730ns 7.23μs 0 0 0 43.44 KB
#5920 StringConcatBenchmark netcoreapp3.1 53.1μs 293ns 1.73μs 0 0 0 42.64 KB
#5920 StringConcatBenchmark net472 37μs 85.8ns 321ns 0 0 0 59.04 KB
#5920 StringConcatAspectBenchmark net6.0 305μs 1.6μs 10.8μs 0 0 0 264.1 KB
#5920 StringConcatAspectBenchmark netcoreapp3.1 336μs 1.76μs 12.7μs 0 0 0 254.33 KB
#5920 StringConcatAspectBenchmark net472 292μs 5.62μs 54.8μs 0 0 0 278.53 KB

@dromanol
dromanol merged commit 6db0ba1 into master Aug 20, 2024
@dromanol
dromanol deleted the dani/iast/hardcodedsecrets_init_fix branch August 20, 2024 14:19
@github-actions github-actions Bot added this to the vNext-v3 milestone Aug 20, 2024
andrewlock pushed a commit that referenced this pull request Aug 23, 2024
@dromanol @andrewlock
[IAST] Move analyzers init to an explicit call (#5920)
ace0982 
## Summary of changes
Make IAST analyzers initialization explicit, and call it only in
`Instrumentation.Initialize` to avoid multiple instances scenarios

## Reason for change
A crash under these circumstances has been detected

## Implementation details
Move IAST analyzers init inside `Instrumentation.Initialize` only to
make sure they are initialized only once

## Test coverage

## Other details
<!-- Fixes #{issue} -->

<!-- ⚠️ Note: where possible, please obtain 2 approvals prior to
merging. Unless CODEOWNERS specifies otherwise, for external teams it is
typically best to have one review from a team member, and one review
from apm-dotnet. Trivial changes do not require 2 reviews. -->
@andrewlock andrewlock mentioned this pull request Aug 23, 2024
Merged
andrewlock added a commit that referenced this pull request Aug 23, 2024
@andrewlock @dromanol
[IAST] Move analyzers init to an explicit call (#5920 -> v2) (#5937)
ba1be9a 
## Summary of changes
Make IAST analyzers initialization explicit, and call it only in
`Instrumentation.Initialize` to avoid multiple instances scenarios

## Reason for change
A crash under these circumstances has been detected

## Implementation details
Move IAST analyzers init inside `Instrumentation.Initialize` only to
make sure they are initialized only once

## Test coverage

## Other details
Backport of #5920

Co-authored-by: Daniel Romano <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@anna-git anna-git anna-git approved these changes
+1 more reviewer
@kevingosse kevingosse kevingosse approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area:asm-iast identified-by:crashtracking

Projects

None yet

Milestone

3.2.0

Development

Successfully merging this pull request may close these issues.

4 participants

@dromanol @andrewlock @anna-git @kevingosse