New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[ASM][ATO] Collect session id at all times #6623

Merged

anna-git merged 13 commits into master from anna/asm/add-sessionid-beginning-request

Feb 20, 2025

Contributor

anna-git commented Feb 3, 2025 •

edited

Loading

Summary of changes

Collects session id in any case:

.net core: on endpoint matched
net framework: try access it through httpcontext
User doesn't need to be authenticated anymore for it to collect the session id.

Reason for change

Session id should always be collected, not only when user is authenticated.

Implementation details

there are guards for netcore as accessing Session when it's not been setup just throws. So this way we make sure it's setup for the webapp.

Test coverage

Change all snapshots.
Scrubbing pitfalls:

scrub cookies and session id for aspnet mvc because if there's a session id it will always change the cookies
scrub cookies and session id for aspnet core when user is authenticated / logged in (as it will keep changing)
other cases in aspnet core: just scrub session fingerprint

Other details

datadog-ddstaging bot commented Feb 3, 2025 •

edited

Loading

Datadog Report

Branch report: anna/asm/add-sessionid-beginning-request
Commit report: b9e859b
Test service: dd-trace-dotnet

❌ 121 Failed (0 Known Flaky), 244485 Passed, 2226 Skipped, 18h 48m 39.81s Total Time

❌ Failed Tests (121)

This report shows up to 5 failed tests.

TestAuthenticatedRequest - Datadog.Trace.Security.IntegrationTests.AspNetCore5AutoUserEventsDefaultModeSecurityEnabled - Details

Expand for error

 Results do not match.
 Differences:
 Received: Security.AspNetCore5AutoUserEvents.SecurityOn.defaultmode-TestAuthenticatedRequest.received.txt
 Verified: Security.AspNetCore5AutoUserEvents.SecurityOn.defaultmode-TestAuthenticatedRequest.verified.txt
 Received Content:
 [
   {
     TraceId: Id_1,
     SpanId: Id_2,
     Name: aspnet_core.request,
 ...

TestAuthenticatedRequest - Datadog.Trace.Security.IntegrationTests.AspNetCore5AutoUserEventsDefaultModeSecurityEnabled - Details

Expand for error

 Results do not match.
 Differences:
 Received: Security.AspNetCore5AutoUserEvents.SecurityOn.defaultmode-TestAuthenticatedRequest.received.txt
 Verified: Security.AspNetCore5AutoUserEvents.SecurityOn.defaultmode-TestAuthenticatedRequest.verified.txt
 Received Content:
 [
   {
     TraceId: Id_1,
     SpanId: Id_2,
     Name: aspnet_core.request,
 ...

TestAuthenticatedRequest - Datadog.Trace.Security.IntegrationTests.AspNetCore5AutoUserEventsDefaultModeSecurityEnabled - Details

Expand for error

 Results do not match.
 Differences:
 Received: Security.AspNetCore5AutoUserEvents.SecurityOn.defaultmode-TestAuthenticatedRequest.received.txt
 Verified: Security.AspNetCore5AutoUserEvents.SecurityOn.defaultmode-TestAuthenticatedRequest.verified.txt
 Received Content:
 [
   {
     TraceId: Id_1,
     SpanId: Id_2,
     Name: aspnet_core.request,
 ...

TestAuthenticatedRequest - Datadog.Trace.Security.IntegrationTests.AspNetCore5AutoUserEventsDefaultModeSecurityEnabled - Details

Expand for error

 Results do not match.
 Differences:
 Received: Security.AspNetCore5AutoUserEvents.SecurityOn.defaultmode-TestAuthenticatedRequest.received.txt
 Verified: Security.AspNetCore5AutoUserEvents.SecurityOn.defaultmode-TestAuthenticatedRequest.verified.txt
 Received Content:
 [
   {
     TraceId: Id_1,
     SpanId: Id_2,
     Name: aspnet_core.request,
 ...

TestAuthenticatedRequest - Datadog.Trace.Security.IntegrationTests.AspNetCore5AutoUserEventsDefaultModeSecurityEnabled - Details

Expand for error

 Results do not match.
 Differences:
 Received: Security.AspNetCore5AutoUserEvents.SecurityOn.defaultmode-TestAuthenticatedRequest.received.txt
 Verified: Security.AspNetCore5AutoUserEvents.SecurityOn.defaultmode-TestAuthenticatedRequest.verified.txt
 Received Content:
 [
   {
     TraceId: Id_1,
     SpanId: Id_2,
     Name: aspnet_core.request,
 ...

Member

andrewlock commented Feb 3, 2025 •

edited

Loading

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).

gantt
    title Execution time (ms) FakeDbCommand (.NET Framework 4.6.2) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6623) - mean (69ms)  : 66, 71
     .   : milestone, 69,
    master - mean (69ms)  : 66, 71
     .   : milestone, 69,

    section CallTarget+Inlining+NGEN
    This PR (6623) - mean (997ms)  : 978, 1016
     .   : milestone, 997,
    master - mean (995ms)  : 975, 1014
     .   : milestone, 995,

gantt
    title Execution time (ms) FakeDbCommand (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6623) - mean (102ms)  : 99, 105
     .   : milestone, 102,
    master - mean (102ms)  : 100, 104
     .   : milestone, 102,

    section CallTarget+Inlining+NGEN
    This PR (6623) - mean (670ms)  : 653, 688
     .   : milestone, 670,
    master - mean (675ms)  : 658, 693
     .   : milestone, 675,

gantt
    title Execution time (ms) FakeDbCommand (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6623) - mean (88ms)  : 86, 90
     .   : milestone, 88,
    master - mean (89ms)  : 87, 90
     .   : milestone, 89,

    section CallTarget+Inlining+NGEN
    This PR (6623) - mean (625ms)  : 607, 643
     .   : milestone, 625,
    master - mean (631ms)  : 608, 653
     .   : milestone, 631,

gantt
    title Execution time (ms) HttpMessageHandler (.NET Framework 4.6.2) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6623) - mean (191ms)  : 187, 195
     .   : milestone, 191,
    master - mean (192ms)  : 187, 196
     .   : milestone, 192,

    section CallTarget+Inlining+NGEN
    This PR (6623) - mean (1,109ms)  : 1077, 1142
     .   : milestone, 1109,
    master - mean (1,111ms)  : 1086, 1137
     .   : milestone, 1111,

gantt
    title Execution time (ms) HttpMessageHandler (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6623) - mean (270ms)  : 266, 275
     .   : milestone, 270,
    master - mean (272ms)  : 268, 275
     .   : milestone, 272,

    section CallTarget+Inlining+NGEN
    This PR (6623) - mean (862ms)  : 832, 892
     .   : milestone, 862,
    master - mean (865ms)  : 828, 903
     .   : milestone, 865,

gantt
    title Execution time (ms) HttpMessageHandler (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6623) - mean (261ms)  : 257, 266
     .   : milestone, 261,
    master - mean (262ms)  : 257, 267
     .   : milestone, 262,

    section CallTarget+Inlining+NGEN
    This PR (6623) - mean (851ms)  : 800, 903
     .   : milestone, 851,
    master - mean (847ms)  : 813, 881
     .   : milestone, 847,

Member

andrewlock commented Feb 3, 2025 •

edited

Loading

Benchmarks Report for appsec 🐌

Benchmarks for #6623 compared to master:

1 benchmarks are slower, with geometric mean 1.119
2 benchmarks have fewer allocations
1 benchmarks have more 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 #6623

Benchmark	diff/base	Base Median (ns)	Diff Median (ns)	Modality
Benchmarks.Trace.Asm.AppSecBodyBenchmark.AllCycleSimpleBody‑net6.0	1.119	186,582.68	208,778.14

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Gen 2	Allocated
master	`AllCycleSimpleBody`	net6.0	187μs	89.1ns	333ns	2.71	0	0	193.47 KB
master	`AllCycleSimpleBody`	netcoreapp3.1	292μs	136ns	510ns	2.77	0	0	200.94 KB
master	`AllCycleSimpleBody`	net472	256μs	282ns	1.09μs	36.7	2.04	0	231.51 KB
master	`AllCycleMoreComplexBody`	net6.0	198μs	88ns	329ns	2.76	0	0	196.98 KB
master	`AllCycleMoreComplexBody`	netcoreapp3.1	298μs	177ns	662ns	2.81	0	0	204.36 KB
master	`AllCycleMoreComplexBody`	net472	262μs	314ns	1.18μs	37.3	2.09	0	235.03 KB
master	`ObjectExtractorSimpleBody`	net6.0	138ns	0.145ns	0.56ns	0.00396	0	0	280 B
master	`ObjectExtractorSimpleBody`	netcoreapp3.1	210ns	0.112ns	0.405ns	0.0037	0	0	272 B
master	`ObjectExtractorSimpleBody`	net472	208ns	0.0885ns	0.343ns	0.0446	0	0	281 B
master	`ObjectExtractorMoreComplexBody`	net6.0	2.89μs	1.54ns	5.78ns	0.0534	0	0	3.78 KB
master	`ObjectExtractorMoreComplexBody`	netcoreapp3.1	3.79μs	2.27ns	8.8ns	0.0511	0	0	3.69 KB
master	`ObjectExtractorMoreComplexBody`	net472	4.23μs	3.33ns	12.5ns	0.602	0.00634	0	3.8 KB
#6623	`AllCycleSimpleBody`	net6.0	209μs	214ns	827ns	2.77	0	0	193.33 KB
#6623	`AllCycleSimpleBody`	netcoreapp3.1	317μs	144ns	540ns	2.69	0	0	200.8 KB
#6623	`AllCycleSimpleBody`	net472	279μs	297ns	1.11μs	36.6	2.1	0	231.35 KB
#6623	`AllCycleMoreComplexBody`	net6.0	215μs	83.1ns	311ns	2.79	0	0	196.83 KB
#6623	`AllCycleMoreComplexBody`	netcoreapp3.1	312μs	66.5ns	258ns	2.65	0	0	204.22 KB
#6623	`AllCycleMoreComplexBody`	net472	282μs	196ns	758ns	37.3	2.1	0	234.87 KB
#6623	`ObjectExtractorSimpleBody`	net6.0	139ns	0.142ns	0.551ns	0.00394	0	0	280 B
#6623	`ObjectExtractorSimpleBody`	netcoreapp3.1	206ns	0.177ns	0.661ns	0.00364	0	0	272 B
#6623	`ObjectExtractorSimpleBody`	net472	210ns	0.129ns	0.5ns	0.0446	0	0	281 B
#6623	`ObjectExtractorMoreComplexBody`	net6.0	2.94μs	2.65ns	9.92ns	0.0532	0	0	3.78 KB
#6623	`ObjectExtractorMoreComplexBody`	netcoreapp3.1	3.86μs	1.96ns	7.57ns	0.0501	0	0	3.69 KB
#6623	`ObjectExtractorMoreComplexBody`	net472	4.23μs	3.35ns	12.5ns	0.603	0.00633	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.4μs	9.28ns	32.1ns	0.459	0	0	32.4 KB
master	`EncodeArgs`	netcoreapp3.1	54μs	12.7ns	47.6ns	0.43	0	0	32.4 KB
master	`EncodeArgs`	net472	66.9μs	60.9ns	236ns	5.13	0.068	0	32.5 KB
master	`EncodeLegacyArgs`	net6.0	77.7μs	415ns	2.15μs	0	0	0	2.14 KB
master	`EncodeLegacyArgs`	netcoreapp3.1	105μs	98.3ns	368ns	0	0	0	2.14 KB
master	`EncodeLegacyArgs`	net472	153μs	139ns	540ns	0.306	0	0	2.15 KB
#6623	`EncodeArgs`	net6.0	36.5μs	16.5ns	63.9ns	0.455	0	0	32.4 KB
#6623	`EncodeArgs`	netcoreapp3.1	53.6μs	12.7ns	47.6ns	0.43	0	0	32.4 KB
#6623	`EncodeArgs`	net472	65.8μs	49.4ns	178ns	5.16	0.0658	0	32.5 KB
#6623	`EncodeLegacyArgs`	net6.0	74.2μs	17.1ns	59.4ns	0	0	0	2.14 KB
#6623	`EncodeLegacyArgs`	netcoreapp3.1	105μs	108ns	418ns	0	0	0	2.14 KB
#6623	`EncodeLegacyArgs`	net472	155μs	320ns	1.24μs	0.307	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	174μs	161ns	580ns	0	0	0	2.54 KB
master	`RunWafRealisticBenchmark`	netcoreapp3.1	188μs	76ns	285ns	0	0	0	2.49 KB
master	`RunWafRealisticBenchmark`	net472	199μs	37.3ns	140ns	0.396	0	0	2.55 KB
master	`RunWafRealisticBenchmarkWithAttack`	net6.0	115μs	66.6ns	240ns	0	0	0	1.57 KB
master	`RunWafRealisticBenchmarkWithAttack`	netcoreapp3.1	124μs	62.6ns	234ns	0	0	0	1.55 KB
master	`RunWafRealisticBenchmarkWithAttack`	net472	134μs	81ns	303ns	0.201	0	0	1.58 KB
#6623	`RunWafRealisticBenchmark`	net6.0	174μs	215ns	832ns	0	0	0	2.54 KB
#6623	`RunWafRealisticBenchmark`	netcoreapp3.1	188μs	66.6ns	240ns	0	0	0	2.49 KB
#6623	`RunWafRealisticBenchmark`	net472	199μs	52.1ns	202ns	0.397	0	0	2.55 KB
#6623	`RunWafRealisticBenchmarkWithAttack`	net6.0	117μs	186ns	696ns	0	0	0	1.57 KB
#6623	`RunWafRealisticBenchmarkWithAttack`	netcoreapp3.1	124μs	105ns	407ns	0	0	0	1.55 KB
#6623	`RunWafRealisticBenchmarkWithAttack`	net472	132μs	44.8ns	173ns	0.197	0	0	1.58 KB

Benchmarks.Trace.Iast.StringAspectsBenchmark - Same speed ✔️ More allocations ⚠️

More allocations ⚠️ in #6623

Benchmark	Base Allocated	Diff Allocated	Change	Change %
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatBenchmark‑net472	59.67 KB	61.77 KB	2.1 KB	3.51%

Fewer allocations 🎉 in #6623

Benchmark	Base Allocated	Diff Allocated	Change	Change %
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatAspectBenchmark‑netcoreapp3.1	253.98 KB	252.02 KB	-1.95 KB	-0.77%
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatAspectBenchmark‑net6.0	271.93 KB	254.07 KB	-17.86 KB	-6.57%

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Gen 2	Allocated
master	`StringConcatBenchmark`	net6.0	62.6μs	876ns	8.71μs	0	0	0	43.44 KB
master	`StringConcatBenchmark`	netcoreapp3.1	62.5μs	1.04μs	10.4μs	0	0	0	42.64 KB
master	`StringConcatBenchmark`	net472	37.7μs	74.7ns	279ns	0	0	0	59.67 KB
master	`StringConcatAspectBenchmark`	net6.0	299μs	4.36μs	40.7μs	0	0	0	271.93 KB
master	`StringConcatAspectBenchmark`	netcoreapp3.1	346μs	1.96μs	13.4μs	0	0	0	253.98 KB
master	`StringConcatAspectBenchmark`	net472	288μs	6.22μs	60.7μs	0	0	0	278.53 KB
#6623	`StringConcatBenchmark`	net6.0	60.1μs	780ns	7.73μs	0	0	0	43.44 KB
#6623	`StringConcatBenchmark`	netcoreapp3.1	60.2μs	723ns	7.16μs	0	0	0	42.64 KB
#6623	`StringConcatBenchmark`	net472	36.5μs	62.9ns	227ns	0	0	0	61.77 KB
#6623	`StringConcatAspectBenchmark`	net6.0	292μs	4.24μs	39.7μs	0	0	0	254.07 KB
#6623	`StringConcatAspectBenchmark`	netcoreapp3.1	340μs	1.97μs	16.6μs	0	0	0	252.02 KB
#6623	`StringConcatAspectBenchmark`	net472	289μs	5.9μs	57.8μs	0	0	0	278.53 KB

Member

andrewlock commented Feb 3, 2025 •

edited

Loading

Benchmarks Report for tracer 🐌

Benchmarks for #6623 compared to master:

2 benchmarks are faster, with geometric mean 1.172
2 benchmarks are slower, with geometric mean 1.156
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.ActivityBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Gen 2	Allocated
master	`StartStopWithChild`	net6.0	7.86μs	43ns	262ns	0.0203	0.00811	0	5.61 KB
master	`StartStopWithChild`	netcoreapp3.1	10.1μs	55.5ns	324ns	0.0199	0.00996	0	5.8 KB
master	`StartStopWithChild`	net472	15.9μs	38.1ns	142ns	1.04	0.307	0.0944	6.21 KB
#6623	`StartStopWithChild`	net6.0	8.22μs	47.8ns	441ns	0.0175	0.00875	0	5.61 KB
#6623	`StartStopWithChild`	netcoreapp3.1	10.6μs	61.1ns	504ns	0.0205	0.0102	0	5.8 KB
#6623	`StartStopWithChild`	net472	16μs	65.3ns	253ns	1.05	0.317	0.103	6.21 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Gen 2	Allocated
master	`WriteAndFlushEnrichedTraces`	net6.0	492μs	486ns	1.82μs	0	0	0	2.7 KB
master	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	700μs	282ns	1.05μs	0	0	0	2.7 KB
master	`WriteAndFlushEnrichedTraces`	net472	848μs	626ns	2.34μs	0.422	0	0	3.3 KB
#6623	`WriteAndFlushEnrichedTraces`	net6.0	513μs	269ns	1.01μs	0	0	0	2.7 KB
#6623	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	647μs	648ns	2.51μs	0	0	0	2.7 KB
#6623	`WriteAndFlushEnrichedTraces`	net472	850μs	925ns	3.58μs	0.425	0	0	3.3 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Gen 2	Allocated
master	`SendRequest`	net6.0	132μs	417ns	1.61μs	0.135	0	0	14.47 KB
master	`SendRequest`	netcoreapp3.1	147μs	396ns	1.53μs	0.22	0	0	17.27 KB
master	`SendRequest`	net472	0.00199ns	0.000288ns	0.00112ns	0	0	0	0 b
#6623	`SendRequest`	net6.0	133μs	395ns	1.53μs	0.195	0	0	14.47 KB
#6623	`SendRequest`	netcoreapp3.1	143μs	471ns	1.82μs	0.205	0	0	17.27 KB
#6623	`SendRequest`	net472	0.00152ns	0.000772ns	0.00299ns	0	0	0	0 b

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Gen 2	Allocated
master	`WriteAndFlushEnrichedTraces`	net6.0	608μs	3.32μs	18.8μs	0.604	0	0	41.87 KB
master	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	670μs	3.78μs	27.5μs	0.338	0	0	41.69 KB
master	`WriteAndFlushEnrichedTraces`	net472	840μs	2.83μs	10.6μs	8.22	2.47	0.411	53.34 KB
#6623	`WriteAndFlushEnrichedTraces`	net6.0	602μs	3.43μs	27.3μs	0.551	0	0	41.77 KB
#6623	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	653μs	2.78μs	10.4μs	0.327	0	0	41.78 KB
#6623	`WriteAndFlushEnrichedTraces`	net472	836μs	3.64μs	14.1μs	8.45	2.53	0.422	53.3 KB

Benchmarks.Trace.DbCommandBenchmark - Faster 🎉 Same allocations ✔️

Faster 🎉 in #6623

Benchmark	base/diff	Base Median (ns)	Diff Median (ns)	Modality
Benchmarks.Trace.DbCommandBenchmark.ExecuteNonQuery‑net6.0	1.122	1,442.46	1,285.29

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Gen 2	Allocated
master	`ExecuteNonQuery`	net6.0	1.44μs	2.09ns	8.1ns	0.0138	0	0	1.02 KB
master	`ExecuteNonQuery`	netcoreapp3.1	1.75μs	2.38ns	9.22ns	0.0139	0	0	1.02 KB
master	`ExecuteNonQuery`	net472	2.07μs	4.15ns	16.1ns	0.156	0.00103	0	987 B
#6623	`ExecuteNonQuery`	net6.0	1.28μs	1.56ns	6.03ns	0.0146	0	0	1.02 KB
#6623	`ExecuteNonQuery`	netcoreapp3.1	1.75μs	1.09ns	4.09ns	0.0137	0	0	1.02 KB
#6623	`ExecuteNonQuery`	net472	2.15μs	2.22ns	8.32ns	0.156	0.00107	0	987 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`	net6.0	1.21μs	0.404ns	1.51ns	0.0139	0	0	976 B
master	`CallElasticsearch`	netcoreapp3.1	1.5μs	0.958ns	3.59ns	0.0128	0	0	976 B
master	`CallElasticsearch`	net472	2.46μs	2.36ns	9.14ns	0.158	0	0	995 B
master	`CallElasticsearchAsync`	net6.0	1.37μs	1.01ns	3.76ns	0.013	0	0	952 B
master	`CallElasticsearchAsync`	netcoreapp3.1	1.71μs	0.965ns	3.61ns	0.0139	0	0	1.02 KB
master	`CallElasticsearchAsync`	net472	2.67μs	1.97ns	7.63ns	0.166	0	0	1.05 KB
#6623	`CallElasticsearch`	net6.0	1.26μs	1.57ns	5.87ns	0.0139	0	0	976 B
#6623	`CallElasticsearch`	netcoreapp3.1	1.52μs	0.539ns	1.94ns	0.0131	0	0	976 B
#6623	`CallElasticsearch`	net472	2.56μs	2.22ns	8.6ns	0.158	0	0	995 B
#6623	`CallElasticsearchAsync`	net6.0	1.38μs	0.919ns	3.56ns	0.0131	0	0	952 B
#6623	`CallElasticsearchAsync`	netcoreapp3.1	1.65μs	1.64ns	6.13ns	0.0133	0	0	1.02 KB
#6623	`CallElasticsearchAsync`	net472	2.57μs	2ns	7.76ns	0.166	0	0	1.05 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`	net6.0	1.44μs	1.02ns	3.96ns	0.013	0	0	952 B
master	`ExecuteAsync`	netcoreapp3.1	1.71μs	0.954ns	3.7ns	0.012	0	0	952 B
master	`ExecuteAsync`	net472	1.86μs	0.559ns	2.09ns	0.145	0	0	915 B
#6623	`ExecuteAsync`	net6.0	1.32μs	0.641ns	2.31ns	0.0131	0	0	952 B
#6623	`ExecuteAsync`	netcoreapp3.1	1.67μs	0.757ns	2.93ns	0.0126	0	0	952 B
#6623	`ExecuteAsync`	net472	1.9μs	0.843ns	3.16ns	0.145	0	0	915 B

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Gen 2	Allocated
master	`SendAsync`	net6.0	4.41μs	1.19ns	4.47ns	0.0331	0	0	2.31 KB
master	`SendAsync`	netcoreapp3.1	5.35μs	3.47ns	13ns	0.0374	0	0	2.85 KB
master	`SendAsync`	net472	7.37μs	2.03ns	7.85ns	0.495	0	0	3.12 KB
#6623	`SendAsync`	net6.0	4.34μs	1.01ns	3.78ns	0.0325	0	0	2.31 KB
#6623	`SendAsync`	netcoreapp3.1	5.26μs	3.88ns	15ns	0.037	0	0	2.85 KB
#6623	`SendAsync`	net472	7.43μs	1.77ns	6.84ns	0.493	0	0	3.12 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`	net6.0	1.58μs	0.493ns	1.78ns	0.023	0	0	1.64 KB
master	`EnrichedLog`	netcoreapp3.1	2.19μs	1.07ns	3.99ns	0.0228	0	0	1.64 KB
master	`EnrichedLog`	net472	2.67μs	4.35ns	16.9ns	0.25	0	0	1.57 KB
#6623	`EnrichedLog`	net6.0	1.59μs	1.03ns	3.86ns	0.023	0	0	1.64 KB
#6623	`EnrichedLog`	netcoreapp3.1	2.21μs	1.05ns	4.05ns	0.0221	0	0	1.64 KB
#6623	`EnrichedLog`	net472	2.64μs	1.29ns	4.98ns	0.249	0	0	1.57 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`	net6.0	112μs	212ns	822ns	0.0558	0	0	4.28 KB
master	`EnrichedLog`	netcoreapp3.1	117μs	148ns	573ns	0.0582	0	0	4.28 KB
master	`EnrichedLog`	net472	151μs	82.1ns	307ns	0.681	0.227	0	4.46 KB
#6623	`EnrichedLog`	net6.0	112μs	179ns	693ns	0	0	0	4.28 KB
#6623	`EnrichedLog`	netcoreapp3.1	116μs	185ns	668ns	0.0573	0	0	4.28 KB
#6623	`EnrichedLog`	net472	150μs	163ns	630ns	0.675	0.225	0	4.46 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`	net6.0	3.11μs	0.496ns	1.72ns	0.0311	0	0	2.2 KB
master	`EnrichedLog`	netcoreapp3.1	4.2μs	2.35ns	8.79ns	0.0294	0	0	2.2 KB
master	`EnrichedLog`	net472	5.03μs	1.01ns	3.79ns	0.32	0	0	2.02 KB
#6623	`EnrichedLog`	net6.0	3μs	0.888ns	3.44ns	0.0299	0	0	2.2 KB
#6623	`EnrichedLog`	netcoreapp3.1	4.01μs	1.35ns	5.03ns	0.0301	0	0	2.2 KB
#6623	`EnrichedLog`	net472	4.88μs	2.27ns	8.78ns	0.32	0	0	2.02 KB

Benchmarks.Trace.RedisBenchmark - Slower ⚠️ Same allocations ✔️

Slower ⚠️ in #6623

Benchmark	diff/base	Base Median (ns)	Diff Median (ns)	Modality
Benchmarks.Trace.RedisBenchmark.SendReceive‑net6.0	1.159	1,271.89	1,474.45

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Gen 2	Allocated
master	`SendReceive`	net6.0	1.27μs	0.38ns	1.47ns	0.0159	0	0	1.14 KB
master	`SendReceive`	netcoreapp3.1	1.77μs	0.486ns	1.68ns	0.015	0	0	1.14 KB
master	`SendReceive`	net472	2.12μs	2.85ns	11.1ns	0.183	0	0	1.16 KB
#6623	`SendReceive`	net6.0	1.48μs	0.577ns	2.24ns	0.0163	0	0	1.14 KB
#6623	`SendReceive`	netcoreapp3.1	1.79μs	1.57ns	6.1ns	0.0153	0	0	1.14 KB
#6623	`SendReceive`	net472	2.1μs	1.59ns	5.95ns	0.183	0	0	1.16 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`	net6.0	2.77μs	1.19ns	4.44ns	0.022	0	0	1.6 KB
master	`EnrichedLog`	netcoreapp3.1	3.97μs	5.6ns	20.9ns	0.0217	0	0	1.65 KB
master	`EnrichedLog`	net472	4.3μs	3.26ns	12.6ns	0.323	0	0	2.04 KB
#6623	`EnrichedLog`	net6.0	2.83μs	1.16ns	4.49ns	0.0226	0	0	1.6 KB
#6623	`EnrichedLog`	netcoreapp3.1	3.88μs	2.97ns	11.5ns	0.0212	0	0	1.65 KB
#6623	`EnrichedLog`	net472	4.37μs	2.12ns	8.22ns	0.323	0	0	2.04 KB

Benchmarks.Trace.SpanBenchmark - Slower ⚠️ Same allocations ✔️

Slower ⚠️ in #6623

Benchmark	diff/base	Base Median (ns)	Diff Median (ns)	Modality
Benchmarks.Trace.SpanBenchmark.StartFinishSpan‑net6.0	1.153	390.66	450.47

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Gen 2	Allocated
master	`StartFinishSpan`	net6.0	391ns	0.461ns	1.79ns	0.00817	0	0	576 B
master	`StartFinishSpan`	netcoreapp3.1	557ns	0.82ns	3.18ns	0.00784	0	0	576 B
master	`StartFinishSpan`	net472	584ns	1.27ns	4.91ns	0.0915	0	0	578 B
master	`StartFinishScope`	net6.0	515ns	0.487ns	1.89ns	0.00983	0	0	696 B
master	`StartFinishScope`	netcoreapp3.1	742ns	1.68ns	6.07ns	0.00932	0	0	696 B
master	`StartFinishScope`	net472	839ns	2.27ns	8.78ns	0.105	0	0	658 B
#6623	`StartFinishSpan`	net6.0	450ns	1.1ns	4.25ns	0.00817	0	0	576 B
#6623	`StartFinishSpan`	netcoreapp3.1	616ns	1.18ns	4.58ns	0.00764	0	0	576 B
#6623	`StartFinishSpan`	net472	633ns	1.1ns	4.26ns	0.0916	0	0	578 B
#6623	`StartFinishScope`	net6.0	472ns	0.802ns	3.11ns	0.00978	0	0	696 B
#6623	`StartFinishScope`	netcoreapp3.1	717ns	1.49ns	5.78ns	0.00949	0	0	696 B
#6623	`StartFinishScope`	net472	774ns	1.79ns	6.92ns	0.104	0	0	658 B

Benchmarks.Trace.TraceAnnotationsBenchmark - Faster 🎉 Same allocations ✔️

Faster 🎉 in #6623

Benchmark	base/diff	Base Median (ns)	Diff Median (ns)	Modality
Benchmarks.Trace.TraceAnnotationsBenchmark.RunOnMethodBegin‑net6.0	1.223	731.18	597.88

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Gen 2	Allocated
master	`RunOnMethodBegin`	net6.0	733ns	1.3ns	5.05ns	0.00951	0	0	696 B
master	`RunOnMethodBegin`	netcoreapp3.1	907ns	1.48ns	5.73ns	0.00936	0	0	696 B
master	`RunOnMethodBegin`	net472	1.16μs	1.52ns	5.88ns	0.105	0	0	658 B
#6623	`RunOnMethodBegin`	net6.0	598ns	0.804ns	3.11ns	0.0099	0	0	696 B
#6623	`RunOnMethodBegin`	netcoreapp3.1	905ns	1.01ns	3.79ns	0.00946	0	0	696 B
#6623	`RunOnMethodBegin`	net472	1.07μs	3.41ns	13.2ns	0.104	0	0	658 B

Contributor

github-actions bot commented Feb 5, 2025 •

edited

Loading

Snapshots difference summary

The following differences have been observed in committed snapshots. It is meant to help the reviewer.
The diff is simplistic, so please check some files anyway while we improve it.

36 occurrences of :

-      _dd.appsec.fp.session: ssn--bd9bce81-d0fff5a7-,
+      _dd.appsec.fp.session: ssn--bd9bce81-d0fff5a7-<SessionFp>,

82 occurrences of :

+      _dd.appsec.fp.session: ssn----<SessionFp>,

1 occurrences of :

-      _dd.appsec.fp.session: ssn-5860faf0---,
+      _dd.appsec.fp.session: ssn-5860faf0---<SessionFp>,

11 occurrences of :

-      _dd.appsec.fp.session: ssn-<fingerprint>,
+      _dd.appsec.fp.session: ssn-7bcd1c9f---<SessionFp>,

3 occurrences of :

-      _dd.appsec.fp.session: ssn-<fingerprint>,
+      _dd.appsec.fp.session: ssn-ef8eb89f---<SessionFp>,

50 occurrences of :

+      _dd.appsec.fp.session: ssn--<CookieFields>-<CookieValues>-<SessionFp>,

3 occurrences of :

-      _dd.appsec.s.req.cookies: [{"cookie-key":[8]}],
+      _dd.appsec.s.req.cookies: [{"ASP.NET_SessionId":[8],"cookie-key":[8]}],

anna-git force-pushed the anna/asm/add-sessionid-beginning-request branch 13 times, most recently from bc3fe93 to 6ff252f Compare

February 13, 2025 10:56

anna-git force-pushed the anna/asm/add-sessionid-beginning-request branch 2 times, most recently from a5b6a37 to 1727f6b Compare

February 13, 2025 15:17

anna-git changed the title ~~Anna/asm/add sessionid beginning request~~ [ASM][ATO] Collect session id at all times

anna-git added 7 commits

February 13, 2025 18:03


          Add session id in the main RunWaf method and a function to not over…

73f324d

…ride the sdk value


          Take session id into account when used with sdk

003d3bb


          Collect the actual SessionId

b4b2d55


          Scrub session fingerprint when no authentication

8e7ae22

Scrub session and cookies fingerprint when authentication


          Adapt Rasp snapshots

bef3d69


          Adapt rasp iast snapshots

9a5b174


          Adapt security snapshots

4ff5982

anna-git force-pushed the anna/asm/add-sessionid-beginning-request branch from 1727f6b to 4ff5982 Compare

February 13, 2025 17:09

github-actions bot added the area:asm label

anna-git marked this pull request as ready for review

February 13, 2025 19:13

anna-git requested review from a team as code owners

February 13, 2025 19:13

NachoEchevarria reviewed

View reviewed changes

tracer/src/Datadog.Trace/AppSec/Waf/Context.cs Outdated

		@@ -66,32 +66,26 @@ private Context(IntPtr contextHandle, IWaf waf, IWafLibraryInvoker wafLibraryInv
		public Dictionary<string, object> ShouldRunWith(IDatadogSecurity security, string? userId = null, string? userLogin = null, string? userSessionId = null, bool fromSdk = false)

Contributor

NachoEchevarria Feb 19, 2025

NIT: Maybe it's a little confusing having these two methods with the same name, very similar signature, and different return values?

Dictionary<string, object> ShouldRunWith(IDatadogSecurity security, string? userId = null, string? userLogin = null, string? userSessionId = null, bool fromSdk = false)

private bool ShouldRunWith(IDatadogSecurity security, UserEventsState.UserRecord? userRecord, string? value, string address, bool fromSdk)

Contributor Author

anna-git Feb 19, 2025

Good point, changed and documented in 061a364

NachoEchevarria reviewed

View reviewed changes

tracer/src/Datadog.Trace/AppSec/Waf/Context.cs Outdated

+                      string? previousValue = null;
+                      var previousValueFromSdk = false;
+                      if (userRecord.HasValue)

Contributor

NachoEchevarria Feb 19, 2025

If userRecord.HasValue is false, could we just return fromSdk and avoid the string comparison?

Contributor Author

anna-git Feb 19, 2025

Very good point, changed in c9a71ab

NachoEchevarria approved these changes

View reviewed changes

Contributor

NachoEchevarria left a comment

LGTM!

anna-git added 2 commits

February 19, 2025 12:08


          Answer to comment on confusing methods signatures and add documentation

061a364


          Answer to comment on If userRecord.HasValue is false, could we just r…

c9a71ab

…eturn fromSdk and avoid the string comparison?

andrewlock approved these changes

View reviewed changes

Member

andrewlock left a comment

Mostly just reviewed the tracing side, but LGTM

tracer/src/Datadog.Trace/AppSec/Coordinator/SecurityCoordinatorHelpers.Core.cs Outdated

                           {
                               var securityCoordinator = SecurityCoordinator.Get(security, span, transport);
                               var args = new Dictionary<string, object> { { AddressesConstants.RequestPathParams, pathParams } };
-                              var result = securityCoordinator.RunWaf(args);
+                              IResult? result;
+                              // we need these tests for netcoreapp3.1 and lower, as otherwise it throws

Member

andrewlock Feb 19, 2025

There's nothing netcore31+ specific AFAICT, so it doesn't matter, but why only those TFMs? Don't we need the check for newer versions too? 🤔

Contributor Author

anna-git Feb 19, 2025

I added the check for all versions. Funny enough, it was crashing only on older versions in integration tests so I wrote this comment. But now browsing the source code I can see that even the latest version does:

   public override ISession Session
    {
        get
        {
            var feature = SessionFeatureOrNull;
            if (feature == null)
            {
                throw new InvalidOperationException("Session has not been configured for this application " +
                    "or request.");
            }
            return feature.Session;
        }

So I'll remove the comment in af317c5 regarding netcoreapp3.1 and lower !

tracer/test/Datadog.Trace.Security.IntegrationTests/ApiSecurity/AspNetCoreApiSecurity.cs Outdated

                       Directory.CreateDirectory(LogDirectory);
                       EnvironmentHelper.CustomEnvironmentVariables.Add(ConfigurationKeys.AppSec.Rules, Path.Combine("ApiSecurity", "ruleset-with-block.json"));
                       // necessary as the developer middleware prevents the right blocking response

Member

andrewlock Feb 19, 2025

nit: no longer relevant

Suggested change

// necessary as the developer middleware prevents the right blocking response

Contributor Author

anna-git Feb 19, 2025

good catch removed in 6fd2f47

tracer/test/Datadog.Trace.Security.IntegrationTests/AspNetBase.cs Show resolved Hide resolved

tracer/src/Datadog.Trace/AppSec/Coordinator/SecurityCoordinatorHelpers.Core.cs Outdated

-                              var result = securityCoordinator.RunWaf(args);
+                              IResult? result;
+                              // we need these tests for netcoreapp3.1 and lower, as otherwise it throws
+                              if (context.Features.Get<ISessionFeature>() is not null && (context.Session?.IsAvailable ?? false))

Member

andrewlock Feb 19, 2025

Just FYI, this isn't just about whether it's enabled, it's also about whether the session middleware has executed yet. I don't think that matters here, as you're not running this until the endpoint executes, right?

Also, minor nit, but context.Session just calls context.Features.Get<ISessionFeature>() again, so you may as well reuse the value instead of calling it again I think:

Suggested change

      
                            if (context.Features.Get<ISessionFeature>() is not null && (context.Session?.IsAvailable ?? false))
          
                            if (context.Features.Get<ISessionFeature>() is { } feature && (feature.Session?.IsAvailable ?? false))

Contributor Author

anna-git Feb 19, 2025

Good point thank you! yes it's only called OnRoutingEndpointMatched, so when it's available , changed in 1c80b17

anna-git added 3 commits

February 19, 2025 14:42


          Change comment erroneous about tfm

af317c5


          remove old comment

6fd2f47


          Reuse ISessionFeature as we're testing its existence

1c80b17

lucaspimentel reviewed

View reviewed changes

tracer/src/Datadog.Trace/AppSec/Coordinator/SecurityCoordinatorHelpers.Core.cs Outdated

-                              var result = securityCoordinator.RunWaf(args);
+                              IResult? result;
+                              // we need to check context.Features.Get<ISessionFeature> as accessing the Session item if session has not been configured for the application is throwing InvalidOperationException
+                              if (context.Features.Get<ISessionFeature>() is { } feature && (feature.Session?.IsAvailable ?? false))

Member

lucaspimentel Feb 19, 2025 •

edited

Loading

Just a tiny style nit, but I'm not a fan of using ?? inside an if. I think it makes the if condition harder to read. I prefer == true or == false.

Member

lucaspimentel Feb 19, 2025

This is how I read it in my head, assuming b is nullable 😅

// set the value of a to the value of b if available, 
// or false otherwise (this is fine)
var a = b ?? false;

// if b is true (this is fine)
if (b == true) ...

// uhhh (I need to think for a second)
if (b ?? false) ...

Contributor Author

anna-git Feb 20, 2025 •

edited

Loading

actually it's a good point, it's a bit convoluted doing this, when I could just test for true. And that allows me to use the pattern even better:
if (context.Features.Get<ISessionFeature>() is { Session.IsAvailable: true } feature)
so thanks, changed in 97afde5

lucaspimentel approved these changes

View reviewed changes


          Answer to comment on falling back on false instead of testing for act…

97afde5

…ual true

anna-git merged commit 889915f into master

144 of 146 checks passed

anna-git deleted the anna/asm/add-sessionid-beginning-request branch

February 20, 2025 15:50

github-actions bot added this to the vNext-v3 milestone

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels