[ASM][ATO] Collect request headers on login user events (failures and success)#6225
Merged
Conversation
Datadog ReportBranch report: ✅ 0 Failed, 454557 Passed, 2753 Skipped, 20h 47m 21.28s Total Time New Flaky Tests (2)
|
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:
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 (6225) - mean (72ms) : 62, 82
. : milestone, 72,
master - mean (71ms) : 65, 78
. : milestone, 71,
section CallTarget+Inlining+NGEN
This PR (6225) - mean (1,016ms) : 907, 1124
. : milestone, 1016,
master - mean (981ms) : 960, 1003
. : milestone, 981,
gantt
title Execution time (ms) FakeDbCommand (.NET Core 3.1)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6225) - mean (109ms) : 106, 111
. : milestone, 109,
master - mean (108ms) : 106, 110
. : milestone, 108,
section CallTarget+Inlining+NGEN
This PR (6225) - mean (698ms) : 642, 754
. : milestone, 698,
master - mean (683ms) : 665, 702
. : milestone, 683,
gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6225) - mean (95ms) : 88, 102
. : milestone, 95,
master - mean (92ms) : 89, 95
. : milestone, 92,
section CallTarget+Inlining+NGEN
This PR (6225) - mean (702ms) : crit, 591, 813
. : crit, milestone, 702,
master - mean (638ms) : 623, 653
. : milestone, 638,
gantt
title Execution time (ms) HttpMessageHandler (.NET Framework 4.6.2)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6225) - mean (191ms) : 187, 195
. : milestone, 191,
master - mean (190ms) : 186, 194
. : milestone, 190,
section CallTarget+Inlining+NGEN
This PR (6225) - mean (1,098ms) : 1063, 1132
. : milestone, 1098,
master - mean (1,094ms) : 1072, 1115
. : milestone, 1094,
gantt
title Execution time (ms) HttpMessageHandler (.NET Core 3.1)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6225) - mean (276ms) : 271, 280
. : milestone, 276,
master - mean (276ms) : 272, 279
. : milestone, 276,
section CallTarget+Inlining+NGEN
This PR (6225) - mean (881ms) : 848, 914
. : milestone, 881,
master - mean (875ms) : 850, 900
. : milestone, 875,
gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6225) - mean (266ms) : 262, 269
. : milestone, 266,
master - mean (264ms) : 260, 268
. : milestone, 264,
section CallTarget+Inlining+NGEN
This PR (6225) - mean (859ms) : 822, 897
. : milestone, 859,
master - mean (854ms) : 825, 884
. : milestone, 854,
|
Member
Benchmarks Report for appsec 🐌Benchmarks for #6225 compared to master:
The following thresholds were used for comparing the benchmark speeds:
Allocation changes below 0.5% are ignored. Benchmark detailsBenchmarks.Trace.Asm.AppSecBodyBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.Asm.AppSecEncoderBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.Asm.AppSecWafBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.Iast.StringAspectsBenchmark - Same speed ✔️ More allocations
|
| Benchmark | Base Allocated | Diff Allocated | Change | Change % |
|---|---|---|---|---|
| Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatAspectBenchmark‑net6.0 | 253.31 KB | 254.79 KB | 1.48 KB | 0.58% |
| Benchmark | Base Allocated | Diff Allocated | Change | Change % |
|---|---|---|---|---|
| Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatBenchmark‑net472 | 59.54 KB | 57.34 KB | -2.2 KB | -3.69% |
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | StringConcatBenchmark |
net6.0 | 60.3μs | 771ns | 7.6μs | 0 | 0 | 0 | 43.44 KB |
| master | StringConcatBenchmark |
netcoreapp3.1 | 65.6μs | 833ns | 8.2μs | 0 | 0 | 0 | 42.64 KB |
| master | StringConcatBenchmark |
net472 | 37.6μs | 88.2ns | 318ns | 0 | 0 | 0 | 59.54 KB |
| master | StringConcatAspectBenchmark |
net6.0 | 305μs | 1.42μs | 5.48μs | 0 | 0 | 0 | 253.31 KB |
| master | StringConcatAspectBenchmark |
netcoreapp3.1 | 340μs | 1.45μs | 8.06μs | 0 | 0 | 0 | 253.77 KB |
| master | StringConcatAspectBenchmark |
net472 | 290μs | 6.57μs | 64μs | 0 | 0 | 0 | 278.53 KB |
| #6225 | StringConcatBenchmark |
net6.0 | 60μs | 918ns | 9.04μs | 0 | 0 | 0 | 43.44 KB |
| #6225 | StringConcatBenchmark |
netcoreapp3.1 | 63.3μs | 891ns | 8.87μs | 0 | 0 | 0 | 42.64 KB |
| #6225 | StringConcatBenchmark |
net472 | 39.1μs | 216ns | 1.39μs | 0 | 0 | 0 | 57.34 KB |
| #6225 | StringConcatAspectBenchmark |
net6.0 | 306μs | 5.16μs | 49μs | 0 | 0 | 0 | 254.79 KB |
| #6225 | StringConcatAspectBenchmark |
netcoreapp3.1 | 348μs | 2.29μs | 22.1μs | 0 | 0 | 0 | 254.2 KB |
| #6225 | StringConcatAspectBenchmark |
net472 | 295μs | 5.7μs | 55.9μs | 0 | 0 | 0 | 278.53 KB |
Member
Benchmarks Report for tracer 🐌Benchmarks for #6225 compared to master:
The following thresholds were used for comparing the benchmark speeds:
Allocation changes below 0.5% are ignored. Benchmark detailsBenchmarks.Trace.ActivityBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.AgentWriterBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.AspNetCoreBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.DbCommandBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.ElasticsearchBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.GraphQLBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.HttpClientBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.ILoggerBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.Log4netBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.NLogBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.RedisBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.SerilogBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.SpanBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.TraceAnnotationsBenchmark - Slower
|
| Benchmark | diff/base | Base Median (ns) | Diff Median (ns) | Modality |
|---|---|---|---|---|
| Benchmarks.Trace.TraceAnnotationsBenchmark.RunOnMethodBegin‑net6.0 | 1.196 | 656.26 | 784.99 |
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | RunOnMethodBegin |
net6.0 | 656ns | 0.758ns | 2.94ns | 0.00974 | 0 | 0 | 696 B |
| master | RunOnMethodBegin |
netcoreapp3.1 | 903ns | 1.41ns | 5.45ns | 0.00941 | 0 | 0 | 696 B |
| master | RunOnMethodBegin |
net472 | 1.06μs | 1.46ns | 5.66ns | 0.105 | 0 | 0 | 658 B |
| #6225 | RunOnMethodBegin |
net6.0 | 785ns | 1.11ns | 4.29ns | 0.00951 | 0 | 0 | 696 B |
| #6225 | RunOnMethodBegin |
netcoreapp3.1 | 942ns | 1.28ns | 4.95ns | 0.00935 | 0 | 0 | 696 B |
| #6225 | RunOnMethodBegin |
net472 | 1.14μs | 3.23ns | 12.5ns | 0.104 | 0 | 0 | 658 B |
Member
Throughput/Crank Report ⚡Throughput results for AspNetCoreSimpleController comparing the following branches/commits: Cases where throughput results for the PR are worse than latest master (5% drop or greater), results are shown in red. Note that these results are based on a single point-in-time result for each branch. For full results, see one of the many, many dashboards! gantt
title Throughput Linux x64 (Total requests)
dateFormat X
axisFormat %s
section Baseline
This PR (6225) (11.352M) : 0, 11351646
master (11.247M) : 0, 11246955
benchmarks/2.9.0 (11.033M) : 0, 11032866
section Automatic
This PR (6225) (7.219M) : 0, 7218764
master (7.407M) : 0, 7406927
benchmarks/2.9.0 (7.786M) : 0, 7785853
section Trace stats
master (7.695M) : 0, 7695476
section Manual
master (11.240M) : 0, 11240426
section Manual + Automatic
This PR (6225) (6.792M) : 0, 6792009
master (6.871M) : 0, 6870622
section DD_TRACE_ENABLED=0
master (10.206M) : 0, 10206195
gantt
title Throughput Linux arm64 (Total requests)
dateFormat X
axisFormat %s
section Baseline
This PR (6225) (9.656M) : 0, 9656005
master (9.563M) : 0, 9563014
benchmarks/2.9.0 (9.495M) : 0, 9494821
section Automatic
This PR (6225) (6.397M) : 0, 6396564
master (6.412M) : 0, 6411572
section Trace stats
master (6.703M) : 0, 6703390
section Manual
master (9.369M) : 0, 9369266
section Manual + Automatic
This PR (6225) (5.983M) : 0, 5982884
master (5.978M) : 0, 5977872
section DD_TRACE_ENABLED=0
master (8.996M) : 0, 8996087
gantt
title Throughput Windows x64 (Total requests)
dateFormat X
axisFormat %s
section Baseline
This PR (6225) (9.100M) : 0, 9100181
benchmarks/2.9.0 (10.020M) : 0, 10019592
section Automatic
This PR (6225) (6.063M) : 0, 6063464
benchmarks/2.9.0 (7.255M) : 0, 7255257
section Manual + Automatic
This PR (6225) (5.574M) : 0, 5573857
|
anna-git
force-pushed
the
anna/asm/ato-headers-success
branch
3 times, most recently
from
November 11, 2024 16:39
3336cdf to
b033988
Compare
Contributor
Snapshots difference summaryThe following differences have been observed in committed snapshots. It is meant to help the reviewer. 6 occurrences of : + http.request.headers.content-length: 43,
+ http.request.headers.content-type: application/x-www-form-urlencoded; charset=utf-8,
[...]
+ http.request.headers.user-agent: Mistake Not...,
+ http.request.headers.x-forwarded-for: 86.242.244.246,
4 occurrences of : + http.request.headers.content-length: 44,
+ http.request.headers.content-type: application/x-www-form-urlencoded; charset=utf-8,
[...]
+ http.request.headers.user-agent: Mistake Not...,
+ http.request.headers.x-forwarded-for: 86.242.244.246,
4 occurrences of : + http.request.headers.content-length: 45,
+ http.request.headers.content-type: application/x-www-form-urlencoded; charset=utf-8,
[...]
+ http.request.headers.user-agent: Mistake Not...,
+ http.request.headers.x-forwarded-for: 86.242.244.246,
|
anna-git
force-pushed
the
anna/asm/ato-headers-success
branch
from
November 12, 2024 10:52
601cffc to
f8c6ed2
Compare
anna-git
changed the title
anna-git
marked this pull request as ready for review
November 12, 2024 16:33
| setTag(Tags.AppSec.EventsUsers.LoginEvent.FailureSdkSource, "true"); | ||
| setTag(Tags.AppSec.EventsUsers.LoginEvent.FailureUserId, userId); | ||
| setTag(Tags.AppSec.EventsUsers.LoginEvent.FailureUserExists, exists ? "true" : "false"); | ||
| setTag(Tags.AppSec.EventsUsers.LoginEvent.FailureTrack, "true"); |
Collaborator
There was a problem hiding this comment.
NIT: While it was already there before this PR, maybe we could consider using constants for "true" and "false" to improve readability and decrease allocations (although the compiler might optimize that, but with this change we would be certain about this optimization)?
Using something like:
public static class Constants
{
public static readonly string TrueValue = "true";
public static readonly string FalseValue = "false";
}
Contributor
Author
There was a problem hiding this comment.
Thanks for the idea, I'll do this in the next PR, fixing user id collection!
Member
There was a problem hiding this comment.
Use actual constants though, not static readonly 😄
| } | ||
|
|
||
| if (span is Span internalSpan) | ||
| if (metadata != null) |
Collaborator
There was a problem hiding this comment.
Small performance boost opportunity?
if (metadata is { Count: > 0 })
{
var successTagPrefix = $"{Tags.AppSec.EventsUsers.LoginEvent.Failure}.";
foreach (var kvp in metadata)
{
setTag(successTagPrefix + kvp.Key, kvp.Value);
}
}
| setTag($"_dd.{Tags.AppSec.Events}{eventName}.sdk", "true"); | ||
|
|
||
| if (metadata != null) | ||
| if (metadata != null) |
Collaborator
There was a problem hiding this comment.
Same as suggested earlier in with LoginEvent.Failure
| setTag($"{Tags.AppSec.Events}{eventName}.{kvp.Key}", kvp.Value); | ||
| } | ||
| } | ||
| } |
Collaborator
There was a problem hiding this comment.
Should we call FillUp here too?
Contributor
Author
There was a problem hiding this comment.
good point I added it here as well in 5eb2354
anna-git
force-pushed
the
anna/asm/ato-headers-success
branch
from
November 15, 2024 14:32
f8c6ed2 to
5eb2354
Compare
andrewlock
approved these changes
Nov 22, 2024
andrewlock
left a comment
andrewlock
left a comment
Member
There was a problem hiding this comment.
LGTM in general, just a few suggestions/questions
| value = cookie.Value; | ||
| } | ||
|
|
||
| [MethodImpl(MethodImplOptions.NoInlining)] |
Member
There was a problem hiding this comment.
I assume this is to protect the CoreHttpContextStore, but is it really necessary? 🤔 If it's potentially going to crash, I would expect to see a try-catch in here...
Contributor
Author
There was a problem hiding this comment.
Seems it's necessary as if it's inlined it will try to load Microsoft.AspNetCore.Hosting.Abstractions and crashes. I dont think we can try/catch it as it's assembly loading the moment this method is being called.. (?)
We had the discussion here 🤔
|
|
||
| internal static void CollectHeaders(Span internalSpan) | ||
| { | ||
| var context = HttpContext.Current; |
Member
There was a problem hiding this comment.
I guess this is safe to call, even if you're in a console app or something? Have you tested it, or are we already doing this elsewhere anyway?
Contributor
Author
There was a problem hiding this comment.
yes it doesn't crash from the Samples.ManualInstrumentation in Net4.6.2, it just returns null
| setTag(Tags.AppSec.EventsUsers.LoginEvent.FailureSdkSource, "true"); | ||
| setTag(Tags.AppSec.EventsUsers.LoginEvent.FailureUserId, userId); | ||
| setTag(Tags.AppSec.EventsUsers.LoginEvent.FailureUserExists, exists ? "true" : "false"); | ||
| setTag(Tags.AppSec.EventsUsers.LoginEvent.FailureTrack, "true"); |
Member
There was a problem hiding this comment.
Use actual constants though, not static readonly 😄
anna-git
force-pushed
the
anna/asm/ato-headers-success
branch
from
November 25, 2024 10:25
e051989 to
8fa918f
Compare
change snapshots update snapshots
anna-git
force-pushed
the
anna/asm/ato-headers-success
branch
from
November 25, 2024 12:25
8fa918f to
4f47cce
Compare
…r Andrew's suggestion
veerbia
pushed a commit
that referenced
this pull request
Dec 16, 2024
… success) (#6225) ## Summary of changes Collect request headers on user events, whether it's a failure or success. Adapt snapshosts ## Reason for change Collect request [headers on user events](https://datadoghq.atlassian.net/wiki/spaces/SAAL/pages/2186870984/HTTP+header+collection) ## Implementation details ## 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. -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary of changes
Collect request headers on user events, whether it's a failure or success. Adapt snapshosts
Reason for change
Collect request headers on user events
Implementation details
Test coverage
Other details