[Appsec]Benchmarks by anna-git · Pull Request #2537 · DataDog/dd-trace-dotnet

anna-git · 2022-03-07T09:40:05Z

Summary of changes

Reason for change

Implementation details

Test coverage

Other details

robertpi · 2022-03-07T10:01:02Z

        {
            // Build the actual integration test projects for Any CPU
-            var integrationTestProjects = TracerDirectory.GlobFiles("test/*.IntegrationTests/*.csproj");
+            var integrationTestProjects = TracerDirectory.GlobFiles("test/*.Security.IntegrationTests/*.csproj");


Looks like an error.

robertpi

I think there's a minor fix needed, but otherwise looks good.

andrewlock

Just some minor suggestions/questions

andrewlock · 2022-03-07T14:19:56Z

@@ -0,0 +1,5 @@
+{
+  "DD_APPSEC_ENABLED": true,
+  "DD_DOTNET_TRACER_HOME": "C:\Repositories\dd-trace-dotnet\tracer\bin\dd-tracer-home\win-x64",


Should this really be hard-coded? 🤔

indeed I forgot to change it, removed the file and setting environment variables in the constructor, thanks

andrewlock · 2022-03-07T14:24:50Z

+#if !NETFRAMEWORK
+    public class HttpContextMock : HttpContext
+    {
+        private readonly HttpResponseMock responseMock;
+
+        public HttpContextMock(HttpResponseMock responseMock)
+        {
+            this.responseMock = responseMock;
+        }
+        public override ConnectionInfo Connection => throw new NotImplementedException();


I'm not sure all this is necessary? I think you can use DefaultHttpContext?

yes I wasnt sure how to make sure the response was disposing the waf pointers, but with disposing them manually now in 8eda4fe, no need for all this anymore thanks

andrewlock · 2022-03-07T14:27:10Z

+        [GlobalSetup]
+        public void Setup()
+        {
+            security = Security.Instance;
+        }


nit: rather than using GlobalSetup we typically use a static constructor. Nothing wrong with this AFAIK, just wondered if there was a specific reason for this approach?

there was no reason , as I had problems initializing Security, I tried this and left it 😅

fixed in 2dd5587

andrewlock · 2022-03-07T14:27:43Z

+        [Benchmark]
+        public void AllCycleSimpleBody()
+        {
+#if NETFRAMEWORK
+            Debugger.Break();
+            using var ms = new MemoryStream();
+            using var sw = new StreamWriter(ms);
+            var httpContextMock = new HttpContext(new HttpRequest(string.Empty, string.Empty, string.Empty), new HttpResponse(sw));
+            security.InstrumentationGateway.RaiseBodyAvailable(httpContextMock, new Span(new SpanContext(1, 1), DateTimeOffset.UtcNow), new { });
+#else
+            var httpContextMock = new HttpContextMock(new HttpResponseMock());
+            security.InstrumentationGateway.RaiseBodyAvailable(httpContextMock, new Span(new SpanContext(1, 1), DateTimeOffset.UtcNow), new { });
+#endif
+        }


It would be nice to be able to compare these to benchmarks where security is disabled, do we do that currently?

we have that in the crank but this was more to benchmark the body, and in particular, the body reflection algorithm.

andrewlock · 2022-03-07T14:28:22Z

+            using var ms = new MemoryStream();
+            using var sw = new StreamWriter(ms);
+            var httpContextMock = new HttpContext(new HttpRequest(string.Empty, string.Empty, string.Empty), new HttpResponse(sw));


I wonder if this setup should be outside the benchmark itself? Probably not necessary though.

thanks! Fixed in 8eda4fe, resources are then disposed after the execution

* first appsec benchmark * Remove required filter and add more benchmarks * Fix benchmarks for .NET framework * add a more complex object * fix build steps * Env variables * fix benchmarks * Init httpcontext outside of benchmark * remove global setup Co-authored-by: Robert Pickering <[email protected]>

* Failing test for framework * Adding extractor for body model and event * Subscribing to body available in security * add content type for post request * Read the parsed body in .NET Framework Includes: * Win Forms * WebApi * MVC * Read the parsed body in Net Core (#2501) Adding a more complex model Removing tests with block enabled Removing tests with block enabled Renaming samples wth security prefix to avoid nuke confusion Changing sln accordingly * Adapt build steps for linux and rename integration right * Changing snapshot files to match new ressponse content length * Publish views dll for linux cause we use the publish folder for inte test fix build * Start of unit tests for parsed body * Unit tests and handle some serialization corner cases * Move code from IG into BodyExtractor.cs * Better detection of primatives Detect object graphs with cycles Only look at instance properties * Use fields instead of property to avoid side effects Use fields instead of property to avoid side effects Fix possible null ref * [Appsec]Benchmarks (#2537) * first appsec benchmark * Remove required filter and add more benchmarks * Fix benchmarks for .NET framework * add a more complex object * fix build steps * Env variables * fix benchmarks * Init httpcontext outside of benchmark * remove global setup Co-authored-by: Robert Pickering <[email protected]> * Adding enum to body extractor * Remove unneeded code containing a TODO Adding verified files * First response to comments * Fixing duck typing Factorize peek and pop with Func remove duck type on real type * Repsonse to comments Response to comments * Fix integration tests * Fix called several times if 2 binding sources with different id , fix derived attribute * Adding instrumentation scenario filter to instrumentations * Readonlydictionary and reuse the empty one Co-authored-by: Anna <[email protected]>

anna-git and others added 4 commits March 4, 2022 17:20

first appsec benchmark

02f3362

Remove required filter and add more benchmarks

60387ad

Fix benchmarks for .NET framework

b39851b

add a more complex object

4c2dfea

anna-git requested a review from a team as a code owner March 7, 2022 09:40

anna-git requested review from robertpi and removed request for a team March 7, 2022 09:40

robertpi reviewed Mar 7, 2022

View reviewed changes

robertpi approved these changes Mar 7, 2022

View reviewed changes

anna-git added 2 commits March 7, 2022 11:05

fix build steps

f84a8f4

Env variables

3a4cda2

andrewlock approved these changes Mar 7, 2022

View reviewed changes

anna-git added 3 commits March 7, 2022 18:42

fix benchmarks

aef21fb

Init httpcontext outside of benchmark

8eda4fe

remove global setup

2dd5587

anna-git merged commit 8eb45a5 into appsec/parsed-body-common Mar 7, 2022

anna-git deleted the appsec/anna/parsed-body-common-benchmarks branch March 7, 2022 19:26

Uh oh!

Conversation

anna-git commented Mar 7, 2022

Summary of changes

Reason for change

Implementation details

Test coverage

Other details

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robertpi left a comment

Choose a reason for hiding this comment

Uh oh!

andrewlock 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!

anna-git Mar 7, 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

anna-git Mar 7, 2022 •

edited

Loading