-
Notifications
You must be signed in to change notification settings - Fork 84
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
feat: error reporting plugin #1601
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1601 +/- ##
===========================================
+ Coverage 56.44% 56.61% +0.16%
===========================================
Files 467 471 +4
Lines 15967 16079 +112
Branches 3189 3209 +20
===========================================
+ Hits 9013 9103 +90
- Misses 5727 5761 +34
+ Partials 1227 1215 -12 ☔ View full report in Codecov by Sentry. |
size-limit report 📦
|
packages/analytics-js/src/services/ErrorHandler/ErrorHandler.ts
Outdated
Show resolved
Hide resolved
packages/analytics-js/src/services/ErrorHandler/ErrorHandler.ts
Outdated
Show resolved
Hide resolved
packages/analytics-js/src/services/ErrorHandler/processError.ts
Outdated
Show resolved
Hide resolved
packages/analytics-js/src/services/ErrorHandler/ErrorHandler.ts
Outdated
Show resolved
Hide resolved
@saikumarrs all your comments are addressed |
packages/analytics-js/src/components/pluginsManager/PluginsManager.ts
Outdated
Show resolved
Hide resolved
packages/analytics-js/src/services/ErrorHandler/ErrorHandler.ts
Outdated
Show resolved
Hide resolved
packages/analytics-js/src/services/ErrorHandler/processError.ts
Outdated
Show resolved
Hide resolved
packages/analytics-js/src/components/pluginsManager/PluginsManager.ts
Outdated
Show resolved
Hide resolved
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.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (3)
package-lock.json
is excluded by!**/package-lock.json
,!**/*.json
package.json
is excluded by!**/*.json
packages/analytics-js-plugins/package.json
is excluded by!**/*.json
Files selected for processing (5)
- packages/analytics-js-plugins/.size-limit.mjs (1 hunks)
- packages/analytics-js-plugins/rollup.config.mjs (4 hunks)
- packages/analytics-js/.size-limit.mjs (1 hunks)
- packages/analytics-js/rollup.config.mjs (9 hunks)
- sonar-project.properties (1 hunks)
Files skipped from review as they are similar to previous changes (5)
- packages/analytics-js-plugins/.size-limit.mjs
- packages/analytics-js-plugins/rollup.config.mjs
- packages/analytics-js/.size-limit.mjs
- packages/analytics-js/rollup.config.mjs
- sonar-project.properties
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.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- packages/analytics-js-plugins/.size-limit.mjs (2 hunks)
- packages/analytics-js-plugins/rollup.config.mjs (5 hunks)
- packages/analytics-js/.size-limit.mjs (2 hunks)
Files skipped from review as they are similar to previous changes (3)
- packages/analytics-js-plugins/.size-limit.mjs
- packages/analytics-js-plugins/rollup.config.mjs
- packages/analytics-js/.size-limit.mjs
packages/analytics-js/src/services/ErrorHandler/ErrorHandler.ts
Outdated
Show resolved
Hide resolved
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.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- packages/analytics-js-plugins/tests/errorReporting/index.test.ts (5 hunks)
- packages/analytics-js-plugins/src/errorReporting/index.ts (3 hunks)
- packages/analytics-js/src/services/ErrorHandler/ErrorHandler.ts (8 hunks)
Files skipped from review as they are similar to previous changes (3)
- packages/analytics-js-plugins/tests/errorReporting/index.test.ts
- packages/analytics-js-plugins/src/errorReporting/index.ts
- packages/analytics-js/src/services/ErrorHandler/ErrorHandler.ts
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.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- packages/analytics-js-plugins/tests/errorReporting/index.test.ts (5 hunks)
- packages/analytics-js-plugins/src/errorReporting/index.ts (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- packages/analytics-js-plugins/tests/errorReporting/index.test.ts
- packages/analytics-js-plugins/src/errorReporting/index.ts
packages/analytics-js/src/services/ErrorHandler/ErrorHandler.ts
Outdated
Show resolved
Hide resolved
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.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- packages/analytics-js-common/src/types/ApplicationState.ts (2 hunks)
- packages/analytics-js-plugins/tests/errorReporting/index.test.ts (5 hunks)
- packages/analytics-js-plugins/src/errorReporting/index.ts (3 hunks)
- packages/analytics-js/tests/services/ErrorHandler/ErrorHandler.test.ts (10 hunks)
- packages/analytics-js/src/components/configManager/ConfigManager.ts (3 hunks)
- packages/analytics-js/src/components/configManager/constants.ts (3 hunks)
- packages/analytics-js/src/services/ErrorHandler/ErrorHandler.ts (8 hunks)
- packages/analytics-js/src/state/slices/metrics.ts (1 hunks)
Files skipped from review as they are similar to previous changes (6)
- packages/analytics-js-common/src/types/ApplicationState.ts
- packages/analytics-js-plugins/tests/errorReporting/index.test.ts
- packages/analytics-js-plugins/src/errorReporting/index.ts
- packages/analytics-js/tests/services/ErrorHandler/ErrorHandler.test.ts
- packages/analytics-js/src/components/configManager/constants.ts
- packages/analytics-js/src/services/ErrorHandler/ErrorHandler.ts
Additional comments not posted (4)
packages/analytics-js/src/state/slices/metrics.ts (1)
10-10
: LGTM! Ensure the correct usage ofmetricsServiceUrl
.The addition of
metricsServiceUrl
looks good.Verify its usage across the codebase to ensure it is utilized correctly.
Verification successful
Verified: Proper usage of
metricsServiceUrl
The addition and usage of
metricsServiceUrl
across the codebase have been verified and are consistent with its intended purpose.
metrics.ts
: Initialized withundefined
.ConfigManager.ts
: Assigned a URL.ApplicationState.ts
: Type definition.index.ts
andindex.test.ts
: Used in error reporting and testing.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `metricsServiceUrl` in the codebase. # Test: Search for the usage of `metricsServiceUrl`. Expect: Proper usage across the codebase. rg --type python -A 5 $'metricsServiceUrl'Length of output: 72
Script:
#!/bin/bash # Description: Verify the usage of `metricsServiceUrl` in the codebase. # Test: Search for the usage of `metricsServiceUrl`. Expect: Proper usage across the codebase. rg --type ts -A 5 'metricsServiceUrl'Length of output: 2599
packages/analytics-js/src/components/configManager/ConfigManager.ts (3)
33-33
: LGTM!The import of
METRICS_SERVICE_ENDPOINT
looks good.
112-112
: LGTM! Ensure the correct definitions ofactiveDataplaneUrl
andMETRICS_SERVICE_ENDPOINT
.The assignment of
metricsServiceUrl
looks good.Verify the definitions of
activeDataplaneUrl
andMETRICS_SERVICE_ENDPOINT
to ensure they are correctly defined and used.Verification successful
LGTM! Ensure the correct definitions of
activeDataplaneUrl
andMETRICS_SERVICE_ENDPOINT
.The assignment of
metricsServiceUrl
looks good. The definitions and usage ofactiveDataplaneUrl
andMETRICS_SERVICE_ENDPOINT
are correct.
activeDataplaneUrl
is defined as aSignal<string | undefined>
and is used consistently across the codebase.METRICS_SERVICE_ENDPOINT
is defined as 'rsaMetrics' inconstants.ts
and is imported correctly inConfigManager.ts
.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the definitions of `activeDataplaneUrl` and `METRICS_SERVICE_ENDPOINT`. # Test: Search for the definitions of `activeDataplaneUrl` and `METRICS_SERVICE_ENDPOINT`. Expect: Proper definitions and usage. rg --type python -A 5 $'activeDataplaneUrl' rg --type python -A 5 $'METRICS_SERVICE_ENDPOINT'Length of output: 153
Script:
#!/bin/bash # Description: Verify the definitions of `activeDataplaneUrl` and `METRICS_SERVICE_ENDPOINT`. # Test: Search for the definitions of `activeDataplaneUrl` and `METRICS_SERVICE_ENDPOINT` in TypeScript files. Expect: Proper definitions and usage. rg --type typescript -A 5 'activeDataplaneUrl' rg --type typescript -A 5 'METRICS_SERVICE_ENDPOINT'Length of output: 169
Script:
#!/bin/bash # Description: Verify the definitions of `activeDataplaneUrl` and `METRICS_SERVICE_ENDPOINT`. # Test: Search for the definitions of `activeDataplaneUrl` and `METRICS_SERVICE_ENDPOINT` in TypeScript files. Expect: Proper definitions and usage. rg --type ts -A 5 'activeDataplaneUrl' rg --type ts -A 5 'METRICS_SERVICE_ENDPOINT'Length of output: 8860
165-165
: LGTM! Ensure theupdateReportingState
function no longer requires thelogger
parameter.The removal of
this.logger
from theupdateReportingState
function call looks good.Verify the
updateReportingState
function to ensure it no longer requires thelogger
parameter.
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.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- packages/analytics-js/src/services/ErrorHandler/ErrorHandler.ts (8 hunks)
Additional comments not posted (17)
packages/analytics-js/src/services/ErrorHandler/ErrorHandler.ts (17)
4-9
: Imports ReviewThe new imports for
ErrorType
,ErrorState
,IHttpClient
, and error processing functions are appropriate and necessary for the changes made in the file.Also applies to: 15-15, 24-28
36-36
: Initialization ofhttpClient
The
httpClient
property is correctly initialized in the constructor.
55-55
: Call tonotifyError
witherrorState
The call to
notifyError
with the newerrorState
parameter is correct and aligns with the new error handling logic.
64-64
: Call toonError
witherrorType
The calls to
onError
with the newerrorType
parameter are correct and align with the new error type system.Also applies to: 70-70
78-79
: Initialization ofhttpClient
ininit
The
httpClient
property is correctly initialized in theinit
method.
94-94
: Ensure proper error handlingThe
init
method correctly handles the initialization of the error reporting plugin and catches any errors that occur during the process.
115-115
: Addition oferrorType
parameterThe addition of the
errorType
parameter to theonError
method is correct and aligns with the new error type system.
119-119
: Use ofprocessError
The use of
processError
to handle handled exceptions is correct and ensures that errors are properly processed.Also applies to: 120-121
127-136
: Normalization of error messageThe normalization of the error message using
removeDoubleSpaces
and the creation of a newError
object with the normalized message are correct.
138-138
: Use ofgetNormalizedErrorForUnhandledError
The use of
getNormalizedErrorForUnhandledError
to handle unhandled exceptions is correct and ensures that errors are properly normalized.
141-141
: Check for error reporting enabledThe check for whether error reporting is enabled is correct and ensures that errors are only reported when error reporting is enabled.
151-157
: Buffering of errorsThe buffering of errors when the error reporting plugin is not loaded is correct and ensures that errors are not lost.
158-159
: Call tonotifyError
The call to
notifyError
when the error reporting plugin is loaded is correct and ensures that errors are reported immediately.
162-162
: Error handling inonError
The error handling in the
onError
method ensures that any errors that occur during error reporting are logged.
165-165
: Logging of handled exceptionsThe logging of handled exceptions is correct and ensures that handled exceptions are logged appropriately.
189-193
: Call toinvokeSingle
with new parametersThe call to
invokeSingle
with the new parameters in theleaveBreadcrumb
method is correct and aligns with the new error reporting logic.
206-217
: Implementation ofnotifyError
The
notifyError
method is correctly implemented and ensures that errors are notified to the error reporting plugin.
Quality Gate failedFailed conditions |
PR Description
Error reporting Plugin:
Linear task (optional)
https://linear.app/rudderstack/issue/SDK-1096/error-reporting-plugin
Cross Browser Tests
Please confirm you have tested for the following browsers:
Sanity Suite
Security
Summary by CodeRabbit
New Features
ErrorType
enumeration.Bug Fixes
Refactor
Chores