feat(core): Add experimentalExcludeSentryInternalFrames option to thirdPartyErrorFilterIntegration
#18632
Conversation
chargome
changed the title
thirdPartyErrorFilterIntegrationexperimentalExcludeSentryInternalFrames option to thirdPartyErrorFilterIntegration
| function isSentryInternalFrame(frame: StackFrame, frameIndex: number): boolean { | ||
| // Only match the last frame (index 0 in reversed stack) | ||
| if (frameIndex !== 0 || !frame.context_line || !frame.filename) { | ||
| return false; | ||
| } | ||
|
|
||
| // Filename would look something like this: 'node_modules/@sentry/browser/build/npm/esm/helpers.js' | ||
| if (!frame.filename.includes('sentry') || !frame.filename.includes('helpers')) { | ||
| return false; | ||
| } | ||
|
|
||
| // Must have context_line with the exact fn.apply pattern (case-sensitive) | ||
| if (!frame.context_line.includes(SENTRY_INTERNAL_FN_APPLY)) { | ||
| return false; | ||
| } | ||
|
|
||
| // Check pre_context array for comment pattern | ||
| if (frame.pre_context) { | ||
| const len = frame.pre_context.length; | ||
| for (let i = 0; i < len; i++) { | ||
| if (frame.pre_context[i]?.includes(SENTRY_INTERNAL_COMMENT)) { | ||
| return true; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
I know that this filter is overly strict, but I'd like to avoid false positives at all costs here. Open for suggestions on how to make this smarter though.
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
| function isSentryInternalFrame(frame: StackFrame, frameIndex: number): boolean { | ||
| // Only match the last frame (index 0 in reversed stack) | ||
| if (frameIndex !== 0 || !frame.context_line || !frame.filename) { |
There was a problem hiding this comment.
Bug: The isSentryInternalFrame function incorrectly checks for frameIndex !== 0. It should identify the last frame in the stack, not the first, to correctly filter Sentry internal frames.
Severity: CRITICAL | Confidence: High
🔍 Detailed Analysis
The isSentryInternalFrame function incorrectly checks if a frame is at index 0 (frameIndex !== 0) to identify a Sentry internal frame. This logic assumes the internal frame is always the first frame in the stack. However, the feature's goal is to filter the last frame, which is Sentry's internal wrapper. Because the stack trace is not reversed, this check is incorrect. As a result, the experimentalExcludeSentryInternalFrames option will fail to filter out Sentry's internal frames when they appear at their expected position at the end of the stack trace, defeating the purpose of the feature.
💡 Suggested Fix
Modify the condition in isSentryInternalFrame to correctly identify the last frame of the stack trace. Instead of checking frameIndex !== 0, the check should be against the total length of the frames array, for example, frameIndex === frames.length - 1.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: packages/core/src/integrations/third-party-errors-filter.ts#L117-L119
Potential issue: The `isSentryInternalFrame` function incorrectly checks if a frame is
at index 0 (`frameIndex !== 0`) to identify a Sentry internal frame. This logic assumes
the internal frame is always the first frame in the stack. However, the feature's goal
is to filter the *last* frame, which is Sentry's internal wrapper. Because the stack
trace is not reversed, this check is incorrect. As a result, the
`experimentalExcludeSentryInternalFrames` option will fail to filter out Sentry's
internal frames when they appear at their expected position at the end of the stack
trace, defeating the purpose of the feature.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 7994207
There was a problem hiding this comment.
It is checking the reversed stack
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
| if (frameIndex !== 0 || !frame.context_line || !frame.filename) { | ||
| return false; | ||
| } | ||
|
|
||
| // Filename would look something like this: 'node_modules/@sentry/browser/build/npm/esm/helpers.js' | ||
| if (!frame.filename.includes('sentry') || !frame.filename.includes('helpers')) { | ||
| return false; | ||
| } | ||
|
|
||
| // Must have context_line with the exact fn.apply pattern (case-sensitive) | ||
| if (!frame.context_line.includes(SENTRY_INTERNAL_FN_APPLY)) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
l: can we combine these cases to save some bytes?
| * If set to true, the integration will exclude frames that are internal to the Sentry SDK from the third-party frame detection. | ||
| * Note that enabling this option might lead to errors being misclassified as third-party errors. | ||
| */ | ||
| experimentalExcludeSentryInternalFrames?: boolean; |
There was a problem hiding this comment.
I'm wondering if we need the experimental prefix here at all. My thinking is that it'll cause us more bytes in the long run than to introduce the option directly. If the user who reported this is willing to test the changes, we could cut an experimental release of the SDK, let them test first and then release it. WDYT?
| * If set to true, the integration will exclude frames that are internal to the Sentry SDK from the third-party frame detection. | ||
| * Note that enabling this option might lead to errors being misclassified as third-party errors. | ||
| */ | ||
| experimentalExcludeSentryInternalFrames?: boolean; |
There was a problem hiding this comment.
l/nit (feel free to disgredard): Should we rather call this
| experimentalExcludeSentryInternalFrames?: boolean; | |
| experimentalIgnoreSentryInternalFrames?: boolean; |
? IIUC we want to avoid making decisions based on these frames so we ignore them. But really up to you, I'm fine with either.
|
It makes sense to not alter existing behaviour in the major but if this proves to be valuable, perhaps it should become default behaviour in the next major? |
Some users get flooded by third party errors despite having set their third party error filtering to
"drop-error-if-contains-third-party-frames". This comes from the fact that often the stacktrace includes our internal wrapper logic as last trace in the stack.With this PR we're trying to work around this by specifically ignoring these frames with a new opt-in mechanism. Marked this as experimental, so users will know this option might lead to errors being misclassified.
experimentalExcludeSentryInternalFramesto thethirdPartyErrorFilterIntegrationCloses #13835