fix: filter non error type errors #1865
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes involve simplifying the error reporting utilities by removing certain properties from configuration objects and eliminating the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1865 +/- ##
===========================================
- Coverage 57.12% 56.85% -0.28%
===========================================
Files 476 476
Lines 16231 16232 +1
Branches 3240 3241 +1
===========================================
- Hits 9272 9228 -44
- Misses 5726 5753 +27
- Partials 1233 1251 +18 ☔ View full report in Codecov by Sentry. |
size-limit report 📦
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/analytics-js-plugins/src/errorReporting/event/event.ts (1)
Line range hint
1-124: Consider additional improvements to error handlingWhile the current changes improve error filtering, consider the following suggestions for further optimization:
Error Logging: The current implementation logs a warning when a non-error is received. Consider adding more detailed logging or telemetry to track the frequency and types of non-error objects being passed.
Performance: The
ErrorStackParser.parse()method is called for every error. For high-volume applications, this could potentially impact performance. Consider implementing a caching mechanism for parsed stack traces.Browser Compatibility: Ensure that the
isError()function (imported from./utils) is compatible with all targeted browsers, especially older versions of IE11 if that's still a requirement.Error Transformation: Consider adding a mechanism to transform non-standard error objects into standard Error objects, rather than discarding them entirely. This could help capture more meaningful error data in environments where custom error types are used.
Here's a sample implementation for error transformation:
function transformToError(maybeError: any): Error { if (isError(maybeError)) { return maybeError; } const errorMessage = typeof maybeError === 'string' ? maybeError : stringifyWithoutCircular(maybeError); return new Error(errorMessage); } // Then in normaliseError: if (!isError(maybeError)) { logger?.warn( `${ERROR_REPORTING_PLUGIN}:: ${component} received a non-error: ${stringifyWithoutCircular(maybeError)}`, ); error = transformToError(maybeError); } else { error = maybeError; }This approach would allow you to capture more information from non-standard errors while still maintaining type safety.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/analytics-js-plugins/src/errorReporting/event/event.ts (1 hunks)
🔇 Additional comments (3)
packages/analytics-js-plugins/src/errorReporting/event/event.ts (3)
78-81: Approve changes in error handling logicThe modifications to the
normaliseErrorfunction align well with the PR objectives. The function now effectively filters out non-error type errors and errors without stack traces. This simplification makes the code more robust and less prone to unexpected behavior.Benefits of these changes:
- Only valid
Errorinstances are processed, preventing the creation of erroneous error objects.- Errors without stack traces are filtered out, addressing the issue mentioned in SDK-2284.
- The code is more straightforward and easier to maintain.
Line range hint
1-124: Summary of review and next stepsThe changes in this file successfully address the PR objectives by improving error filtering in the
normaliseErrorfunction. The modifications effectively handle non-error type errors and errors without stack traces, aligning with the goals outlined in the PR summary and the related Linear task (SDK-2284).Next steps:
- Verify the impact of these changes on the overall error reporting process using the provided script.
- Consider implementing the suggested improvements for error transformation and logging.
- Ensure cross-browser compatibility, especially for IE11 as mentioned in the PR checklist.
- Run the sanity suite test cases locally to confirm no regressions have been introduced.
- Review the security checklist to ensure these changes don't introduce any vulnerabilities.
Once these steps are completed and any necessary adjustments are made, this PR should be ready for final review and merge.
78-81: Verify impact on error reporting processWhile the changes improve error filtering, we should verify that this doesn't negatively impact the error reporting process.
Please run the following script to check for any potential issues:
Please review the results to ensure that the changes don't cause any unintended side effects in the error reporting process.
✅ Verification successful
Impact on Error Reporting Process Verified Successfully
The changes to error filtering do not negatively impact the error reporting process. All existing error handling and logging mechanisms remain intact.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential issues in error handling across the codebase # Test 1: Check for uses of normaliseError function echo "Checking uses of normaliseError function:" rg "normaliseError\(" --type typescript # Test 2: Check for any custom error handling that might be affected echo "Checking for custom error handling:" rg "new Error\(" --type typescript # Test 3: Check for any error logging that might be affected echo "Checking for error logging:" rg "console\.error\(" --type typescript rg "logger.*error\(" --type typescript # Expected results: # - No unexpected uses of normaliseError # - No custom error handling that relies on non-Error objects # - No error logging that might be affected by the changesLength of output: 540
Script:
#!/bin/bash # Description: Recheck potential issues in error handling across the codebase with corrected file type patterns # Test 1: Check for uses of normaliseError function echo "Checking uses of normaliseError function:" rg "normaliseError\(" -g '*.ts*' # Test 2: Check for any custom error handling that might be affected echo "Checking for custom error handling:" rg "new Error\(" -g '*.ts*' # Test 3: Check for any error logging that might be affected echo "Checking for error logging:" rg "console\.error\(" -g '*.ts*' rg "logger.*error\(" -g '*.ts*'Length of output: 16722
|
@saikumarrs Addressed you comment |
packages/analytics-js-plugins/src/errorReporting/event/event.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- packages/analytics-js-plugins/tests/errorReporting/utils.test.ts (0 hunks)
- packages/analytics-js-plugins/src/errorReporting/event/event.ts (3 hunks)
- packages/analytics-js-plugins/src/errorReporting/index.ts (1 hunks)
- packages/analytics-js-plugins/src/errorReporting/utils.ts (0 hunks)
💤 Files not reviewed due to no reviewable changes (2)
- packages/analytics-js-plugins/tests/errorReporting/utils.test.ts
- packages/analytics-js-plugins/src/errorReporting/utils.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/analytics-js-plugins/src/errorReporting/event/event.ts
|
PR Description
This PR fixes a scenario where we create new errors from error messages if the type of the error is not an error.
Linear task (optional)
Linear task link
Cross Browser Tests
Please confirm you have tested for the following browsers:
Sanity Suite
Security
Summary by CodeRabbit
Errorinstances, improving error handling.These changes enhance the reliability and efficiency of error reporting within the application.