-
Notifications
You must be signed in to change notification settings - Fork 150
[AAP] Enable by default API Security Endpoints collection when appsec enabled #7569
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.
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
Conversation
andrewlock
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have benchmarks and data about the impact this will have? This would be enabled for all customers, right?
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.8)
dateFormat X
axisFormat %s
todayMarker off
section Bailout
This PR (7569) - mean (72ms) : 71, 73
. : milestone, 72,
master - mean (72ms) : 71, 73
. : milestone, 72,
section Baseline
This PR (7569) - mean (68ms) : 66, 71
. : milestone, 68,
master - mean (68ms) : 66, 70
. : milestone, 68,
section CallTarget+Inlining+NGEN
This PR (7569) - mean (1,050ms) : 997, 1104
. : milestone, 1050,
master - mean (1,044ms) : 1007, 1082
. : milestone, 1044,
gantt
title Execution time (ms) FakeDbCommand (.NET Core 3.1)
dateFormat X
axisFormat %s
todayMarker off
section Bailout
This PR (7569) - mean (107ms) : 106, 108
. : milestone, 107,
master - mean (106ms) : 105, 108
. : milestone, 106,
section Baseline
This PR (7569) - mean (106ms) : 104, 109
. : milestone, 106,
master - mean (106ms) : 103, 108
. : milestone, 106,
section CallTarget+Inlining+NGEN
This PR (7569) - mean (739ms) : 719, 759
. : milestone, 739,
master - mean (745ms) : 715, 775
. : milestone, 745,
gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat X
axisFormat %s
todayMarker off
section Bailout
This PR (7569) - mean (101ms) : 100, 102
. : milestone, 101,
master - mean (100ms) : 99, 101
. : milestone, 100,
section Baseline
This PR (7569) - mean (100ms) : 98, 102
. : milestone, 100,
master - mean (99ms) : 97, 102
. : milestone, 99,
section CallTarget+Inlining+NGEN
This PR (7569) - mean (770ms) : 722, 819
. : milestone, 770,
master - mean (779ms) : 746, 812
. : milestone, 779,
gantt
title Execution time (ms) FakeDbCommand (.NET 8)
dateFormat X
axisFormat %s
todayMarker off
section Bailout
This PR (7569) - mean (93ms) : 92, 94
. : milestone, 93,
master - mean (92ms) : 91, 93
. : milestone, 92,
section Baseline
This PR (7569) - mean (92ms) : 90, 95
. : milestone, 92,
master - mean (92ms) : 90, 94
. : milestone, 92,
section CallTarget+Inlining+NGEN
This PR (7569) - mean (659ms) : 646, 672
. : milestone, 659,
master - mean (660ms) : 644, 675
. : milestone, 660,
gantt
title Execution time (ms) HttpMessageHandler (.NET Framework 4.8)
dateFormat X
axisFormat %s
todayMarker off
section Bailout
This PR (7569) - mean (210ms) : 198, 223
. : milestone, 210,
master - mean (206ms) : 195, 217
. : milestone, 206,
section Baseline
This PR (7569) - mean (205ms) : 195, 216
. : milestone, 205,
master - mean (203ms) : 191, 215
. : milestone, 203,
section CallTarget+Inlining+NGEN
This PR (7569) - mean (1,223ms) : 1151, 1295
. : milestone, 1223,
master - mean (1,214ms) : 1149, 1278
. : milestone, 1214,
gantt
title Execution time (ms) HttpMessageHandler (.NET Core 3.1)
dateFormat X
axisFormat %s
todayMarker off
section Bailout
This PR (7569) - mean (303ms) : 288, 317
. : milestone, 303,
master - mean (294ms) : 278, 311
. : milestone, 294,
section Baseline
This PR (7569) - mean (296ms) : 279, 312
. : milestone, 296,
master - mean (293ms) : 277, 308
. : milestone, 293,
section CallTarget+Inlining+NGEN
This PR (7569) - mean (993ms) : 951, 1034
. : milestone, 993,
master - mean (993ms) : 954, 1031
. : milestone, 993,
gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat X
axisFormat %s
todayMarker off
section Bailout
This PR (7569) - mean (301ms) : 278, 323
. : milestone, 301,
master - mean (302ms) : 286, 318
. : milestone, 302,
section Baseline
This PR (7569) - mean (300ms) : 283, 317
. : milestone, 300,
master - mean (300ms) : 282, 319
. : milestone, 300,
section CallTarget+Inlining+NGEN
This PR (7569) - mean (1,035ms) : 990, 1079
. : milestone, 1035,
master - mean (1,044ms) : 991, 1098
. : milestone, 1044,
gantt
title Execution time (ms) HttpMessageHandler (.NET 8)
dateFormat X
axisFormat %s
todayMarker off
section Bailout
This PR (7569) - mean (284ms) : 272, 296
. : milestone, 284,
master - mean (289ms) : 271, 306
. : milestone, 289,
section Baseline
This PR (7569) - mean (283ms) : 272, 294
. : milestone, 283,
master - mean (288ms) : 270, 307
. : milestone, 288,
section CallTarget+Inlining+NGEN
This PR (7569) - mean (890ms) : 839, 941
. : milestone, 890,
master - mean (925ms) : 846, 1004
. : milestone, 925,
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting
This comment has been minimized.
This comment has been minimized.
|
|
||
| ApiSecurityEndpointCollectionEnabled = config.WithKeys(ConfigurationKeys.AppSec.ApiSecurityEndpointCollectionEnabled) | ||
| .AsBool(false); | ||
| .AsBool(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is only enabled when ASM is enabled, then it's your call, as discussed
It's fine if it's only default on for ASM customers
robertpi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems this will only be active when appsec is active - which is what we where asked to do.
Summary of changes
This PR enable by default the feature to collect all endpoints at startup when appsec is enabled.
Other details
See #6733 #7317 for implementation.