-
Notifications
You must be signed in to change notification settings - Fork 306
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
Fix stack trace collection for appsec events #4410
Conversation
Overall package sizeSelf size: 6.71 MB Dependency sizes
🤖 This report was automatically generated by heaviest-objects-in-the-universe |
5f98e21
to
8ed8192
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4410 +/- ##
===========================================
- Coverage 80.42% 69.19% -11.24%
===========================================
Files 3 1 -2
Lines 373 198 -175
Branches 33 33
===========================================
- Hits 300 137 -163
+ Misses 73 61 -12 ☔ View full report in Codecov by Sentry. |
BenchmarksBenchmark execution time: 2024-06-18 15:04:57 Comparing candidate commit 8ed8192 in PR branch Found 1 performance improvements and 0 performance regressions! Performance is the same for 257 metrics, 8 unstable metrics. scenario:plugin-graphql-with-depth-and-collapse-on-18
|
@@ -27,19 +27,17 @@ function getCallSiteList (maxDepth = 100) { | |||
} | |||
|
|||
function filterOutFramesFromLibrary (callSiteList) { | |||
return callSiteList.filter(callSite => !callSite.getFileName().includes(ddBasePath)) | |||
return callSiteList.filter(callSite => !callSite.getFileName()?.startsWith(ddBasePath)) |
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.
adding that optional chaining has a side effect:
if startsWith is not a method of the intermediary value, that means it's probably not a string, so null or undefined ? if that's the case the !
at the start of the arrow function will turn it into a true, and filter() will keep it in the array, i don't think that's intended right ? or what was your thought behind adding the optional chaining ?
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.
It's intended. There are some frames - like the ones the call take place in code defined by a call to eval - for what getFileName()
returns null.
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.
And do we want to keep those in callSiteList ?
* Fix null filename on frame filtering * Use startsWith instead of includes on callSite filename check * Rework stack trace reporting when callistelist is undefined * Avoid checking for max stack trace depth twice * Use Array(n).fill() instead of [...Array(n).keys()] * Fix linting
* Fix null filename on frame filtering * Use startsWith instead of includes on callSite filename check * Rework stack trace reporting when callistelist is undefined * Avoid checking for max stack trace depth twice * Use Array(n).fill() instead of [...Array(n).keys()] * Fix linting
* Fix null filename on frame filtering * Use startsWith instead of includes on callSite filename check * Rework stack trace reporting when callistelist is undefined * Avoid checking for max stack trace depth twice * Use Array(n).fill() instead of [...Array(n).keys()] * Fix linting
* Fix null filename on frame filtering * Use startsWith instead of includes on callSite filename check * Rework stack trace reporting when callistelist is undefined * Avoid checking for max stack trace depth twice * Use Array(n).fill() instead of [...Array(n).keys()] * Fix linting
What does this PR do?
Fix and address some issues with stack trace collection.
Motivation
Have a more robust stack trace collection