Add simple JS error tracking via custom wrapper and Tracks #44266
Conversation
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
github-actions
bot
added
[Package] My Jetpack
[Plugin] Backup
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Backup plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Boost plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Search plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Social plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Starter Plugin plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Protect plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Videopress plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
github-actions
bot
added
the
[Status] Needs Author Reply
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Code Coverage SummaryCoverage changed in 2 files.
2 files are newly checked for coverage.
Full summary · PHP report · JS report If appropriate, add one of these labels to override the failing coverage check:
Covered by non-unit tests
|
Moved the initializeErrorTracker function from admin.jsx to a new utils/error-tracker.js file for better modularity and maintainability. Updated imports to use the new location.
…e docs Renamed jp-js-error-tracker.js to jp-js-error-tracker/index.js for better module organization and updated the script registration path in class-initializer.php accordingly.
Introduces documentation detailing features, usage, error data structure, configuration, and security considerations for the My Jetpack JavaScript error tracking solution.
manzoorwanijk
removed
the
[Status] Needs Author Reply
manzoorwanijk
added
[Type] Enhancement
There was a problem hiding this comment.
I did a light review and left some comments inline. Here are a few more comments:
- Any idea of what kind of volume you might get? It might be good to limit to some specific error types to start.
- One limitation of not using a custom endpoint is not being able to control things on the receiving side (e.g. filtering the type of things we collect on the fly rather than waiting for each release).
- Batching errors into a single call would be a nice way to limit requests sent, but that'd probably also best be done via a custom endpoint.
- It might be worth splitting this into a separate package down the road. The downside would be that people might actually consume it without care. 😄
- Can we add a few simple unit tests (e.g. run JS with no error and run JS with an error)? I'm not sure how easy that would be to mock.
|
|
||
| const clearErrors = useCallback( () => { | ||
| if ( window.myJetpackErrorTracker ) { | ||
| window.myJetpackErrorTracker.clearErrors(); |
There was a problem hiding this comment.
It was defined in its first iteration but remained in this test component which is going to be removed before merging this.
| const history = window.myJetpackErrorTracker.getErrorHistory(); | ||
| const count = window.myJetpackErrorTracker.getErrorCount(); |
|
|
||
| - **Modern Browsers**: Full feature support including Performance Observer | ||
| - **Legacy Browsers**: Graceful degradation with core error tracking | ||
| - **Crypto API**: Uses secure random generation when available, falls back to Math.random() |
There was a problem hiding this comment.
I don't understand why we'd need cryptographically-secure randomness here. We're just generating an arbitrary session ID to connect Tracks events.
There was a problem hiding this comment.
It was Claude responding to #44266 (review) but yes I agree that it's not a big deal here.
There was a problem hiding this comment.
I'd either stick with Math.random() (as that should be adequate for our needs). If it's not adequate, we shouldn't be allowing it as a fallback.
There was a problem hiding this comment.
Updated to stick to Math.random
| this.pageLoadTime = Date.now(); | ||
| this.errorCount = 0; | ||
| this.throttledErrors = new Map(); | ||
| this.throttleMs = 1000; // Fixed throttle time |
There was a problem hiding this comment.
The docs indicate this is configurable, but it seems to be hard-coded? Also, it might be wise to be a bit more conservative at 5s to start.
There was a problem hiding this comment.
Updated the code to reflect the docs
| this.onError = onError; | ||
| this.sessionId = this.generateSessionId(); | ||
| this.pageLoadTime = Date.now(); | ||
| this.errorCount = 0; |
There was a problem hiding this comment.
I wonder if we should have a maxErrors as well, rolling off the oldest errors to prevent this from getting too large. However, in order to do that properly we'd need an accurate stack of all errors, not just unique ones.
| const self = this; | ||
|
|
||
| // Monitor fetch | ||
| if ( window.fetch ) { |
There was a problem hiding this comment.
This should be available in any modern browser.
I'll also note that it'd be good to have a way to prevent one from double-monkeypatching the function. Right now each time you run setupNetworkMonitoring() the call will get more deeply nested; we shouldn't allow it to run more than once.
One solution would be to store the original fetch function as a new property in window, and then check for that property prior to the monkeypatch.
There was a problem hiding this comment.
That is a great point. I have updated it to handle that case.
projects/packages/my-jetpack/src/js-static/jp-js-error-tracker/index.js
Outdated
Show resolved
Hide resolved
| ## Browser Support | ||
|
|
||
| - **Modern Browsers**: Full feature support including Performance Observer | ||
| - **Legacy Browsers**: Graceful degradation with core error tracking |
There was a problem hiding this comment.
In my opinion, it's probably not worth adding any support or fallbacks for legacy browsers. Worst case, the data doesn't send. There's no change to the user experience, and it's not like we're going to be expanding support to older browsers in the future.
projects/packages/my-jetpack/src/js-static/jp-js-error-tracker/index.js
Dismissed
Show dismissed
Hide dismissed
Completes MYJP-223
Proposed Changes
This PR introduces comprehensive JavaScript error tracking to the My Jetpack interface using a custom error monitoring solution that integrates with Jetpack Analytics.
Features Added
Custom JPJSErrorTracker Library (
projects/packages/my-jetpack/src/js-static/jp-js-error-tracker/index.js):crypto.getRandomValues()My Jetpack Integration (
projects/packages/my-jetpack/_inc/utils/error-tracker.js):jetpack_my_jetpack_js_erroreventsTesting Component (
projects/packages/my-jetpack/components/error-tracker-test/index.jsx):Technical Implementation
class-initializer.php:267admin.jsx:115before React renderingOther information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Yes,
Error Data Collected
Each error includes comprehensive context:
Testing instructions:
jetpack_my_jetpack_js_errorevents)