-
Notifications
You must be signed in to change notification settings - Fork 739
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
GSoC: Distributed error reporting #12489
base: develop
Are you sure you want to change the base?
Changes from 80 commits
d814aa1
afe3ad9
5e10f53
abb63f3
0e01b17
20e540c
e5f2b4c
8b0d8cf
1b50c66
f89c601
e49b82d
2114c4d
678d0cd
5af22b6
2e1b852
722efe7
4c62c27
a859358
fa40e38
b1c50bf
c2207f5
388dd17
26f0018
b6de284
c67851b
cc968b6
09e2af3
d103ff8
00b7be1
53ae2ad
77507a4
30c13b4
2e7539b
86b980e
a0ea7b4
cb259fa
d389b27
0dcbd4a
aa5960b
4b86a2b
1768cd2
7f88d06
2d89272
d02097f
d9ef827
984261f
44ce4e1
2798cdf
bea2e64
afef5ad
e3252ee
01474a2
d530b3a
7056944
109acd2
cd373c4
a558f37
6c4ce3a
1cc20dc
a7414c8
dea1d32
86159d0
22cfce2
15e34ba
85cf9c0
c5eb2ae
14ceceb
ee7eb3e
d8a713e
780d544
4f104ef
085cf88
5b909bc
fd9e925
ac07e7a
6939d0e
aacc517
b64c4c2
340f714
73aed79
8ab29d4
39d07ad
f996888
0c82a59
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,95 @@ | ||
import urls from 'kolibri.urls'; | ||
import Resource from '../errorReport'; | ||
import { | ||
VueErrorReport, | ||
JavascriptErrorReport, | ||
UnhandledRejectionErrorReport, | ||
} from '../../utils/errorReportUtils'; | ||
|
||
/* eslint-env jest */ | ||
jest.mock('kolibri.urls', () => ({ | ||
'kolibri:core:report': jest.fn(), | ||
})); | ||
|
||
describe('Error Report', () => { | ||
beforeEach(() => { | ||
urls['kolibri:core:report'].mockReturnValue('/api/core/report'); | ||
}); | ||
|
||
afterEach(() => { | ||
jest.clearAllMocks(); | ||
}); | ||
|
||
it('should call api/core/report with VueErrorReport data', () => { | ||
const vueError = new Error('Vue error'); | ||
vueError.stack = 'My stack trace'; | ||
const vm = { $options: { name: 'TestComponent' } }; | ||
const errorReport = new VueErrorReport(vueError, vm); | ||
|
||
const expectedData = { | ||
error_message: 'Vue error', | ||
traceback: 'My stack trace', | ||
context: { | ||
...errorReport.getContext(), | ||
component: 'TestComponent', | ||
}, | ||
}; | ||
|
||
Resource.client = jest.fn(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ideally, whenever you mock something in Python or JS, you want to ensure that the original implementation can be restored after the test is completed. That approach can keep tests from interfering with other tests, because of their use of mocks. Since this is a direct replace of |
||
Resource.report(errorReport); | ||
|
||
expect(Resource.client).toHaveBeenCalledWith({ | ||
url: '/api/core/report', | ||
method: 'post', | ||
data: expectedData, | ||
}); | ||
}); | ||
|
||
it('should call api/core/report with JavascriptErrorReport data', () => { | ||
const jsErrorEvent = { | ||
error: new Error('Javascript error'), | ||
}; | ||
jsErrorEvent.error.stack = 'My stack trace'; | ||
|
||
const errorReport = new JavascriptErrorReport(jsErrorEvent); | ||
|
||
const expectedData = { | ||
error_message: 'Javascript error', | ||
traceback: 'My stack trace', | ||
context: errorReport.getContext(), | ||
}; | ||
|
||
Resource.client = jest.fn(); | ||
Resource.report(errorReport); | ||
|
||
expect(Resource.client).toHaveBeenCalledWith({ | ||
url: '/api/core/report', | ||
method: 'post', | ||
data: expectedData, | ||
}); | ||
}); | ||
|
||
it('should call api/core/report with UnhandledRejectionErrorReport data', () => { | ||
const rejectionEvent = { | ||
reason: new Error('Unhandled rejection'), | ||
}; | ||
rejectionEvent.reason.stack = 'My stack trace'; | ||
|
||
const errorReport = new UnhandledRejectionErrorReport(rejectionEvent); | ||
|
||
const expectedData = { | ||
error_message: 'Unhandled rejection', | ||
traceback: 'My stack trace', | ||
context: errorReport.getContext(), | ||
}; | ||
|
||
Resource.client = jest.fn(); | ||
Resource.report(errorReport); | ||
|
||
expect(Resource.client).toHaveBeenCalledWith({ | ||
url: '/api/core/report', | ||
method: 'post', | ||
data: expectedData, | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
import { Resource } from 'kolibri.lib.apiResource'; | ||
import urls from 'kolibri.urls'; | ||
|
||
export default new Resource({ | ||
name: 'errorreports', | ||
report(error) { | ||
const url = urls['kolibri:core:report'](); | ||
const data = error.getErrorReport(); | ||
return this.client({ | ||
url, | ||
method: 'post', | ||
data: data, | ||
}); | ||
}, | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,8 +16,14 @@ import heartbeat from 'kolibri.heartbeat'; | |
import ContentRenderer from '../views/ContentRenderer'; | ||
import initializeTheme from '../styles/initializeTheme'; | ||
import { i18nSetup } from '../utils/i18n'; | ||
import setupPluginMediator from './pluginMediator'; | ||
import { ErrorReportResource } from '../api-resources'; | ||
import { | ||
VueErrorReport, | ||
JavascriptErrorReport, | ||
UnhandledRejectionErrorReport, | ||
} from '../utils/errorReportUtils'; | ||
import apiSpec from './apiSpec'; | ||
import setupPluginMediator from './pluginMediator'; | ||
|
||
// Do this before any async imports to ensure that public paths | ||
// are set correctly | ||
|
@@ -71,6 +77,26 @@ heartbeat.startPolling(); | |
|
||
i18nSetup().then(coreApp.ready); | ||
|
||
// these shall be responsibe for catching runtime errors | ||
Vue.config.errorHandler = function (err, vm) { | ||
logging.error(`Unexpected Error: ${err}`); | ||
const error = new VueErrorReport(err, vm); | ||
ErrorReportResource.report(error); | ||
}; | ||
|
||
window.addEventListener('error', e => { | ||
logging.error(`Unexpected Error: ${e.error}`); | ||
const error = new JavascriptErrorReport(e); | ||
ErrorReportResource.report(error); | ||
}); | ||
|
||
window.addEventListener('unhandledrejection', event => { | ||
event.preventDefault(); | ||
logging.error(`Unhandled Rejection: ${event.reason}`); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know the |
||
const error = new UnhandledRejectionErrorReport(event); | ||
ErrorReportResource.report(error); | ||
}); | ||
|
||
// This is exported by webpack as the kolibriCoreAppGlobal object, due to the 'output.library' flag | ||
// which exports the coreApp at the bottom of this file as a named global variable: | ||
// https://webpack.github.io/docs/configuration.html#output-library | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,66 @@ | ||
import { browser, os, device, isTouchDevice } from './browserInfo'; | ||
|
||
class ErrorReport { | ||
constructor(e) { | ||
this.e = e; | ||
this.context = this.getContext(); | ||
} | ||
|
||
getErrorReport() { | ||
throw new Error('getErrorReport() method must be implemented.'); | ||
} | ||
|
||
getContext() { | ||
return { | ||
browser: browser, | ||
os: os, | ||
device: { | ||
...device, | ||
is_touch_device: isTouchDevice, | ||
screen: { | ||
width: window.screen.width, | ||
height: window.screen.height, | ||
available_width: window.screen.availWidth, | ||
available_height: window.screen.availHeight, | ||
}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was discussion about using the screen size breakpoints instead of the actual width and height. Is that the case, because it doesn't look like it? The reason is that it protects privacy. Specific sizes can be used to identify users, which reduces the anonymity of the data There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, we should definitely make this update. Although I am also noticing that this file shouldn't exist, because it has been moved into the plugin to make this behaviour pluggable. |
||
}, | ||
}; | ||
} | ||
} | ||
|
||
export class VueErrorReport extends ErrorReport { | ||
constructor(e, vm) { | ||
super(e); | ||
this.vm = vm; | ||
} | ||
getErrorReport() { | ||
return { | ||
error_message: this.e.message, | ||
traceback: this.e.stack, | ||
context: { | ||
...this.context, | ||
component: this.vm.$options.name || this.vm.$options._componentTag || 'Unknown Component', | ||
}, | ||
}; | ||
} | ||
} | ||
|
||
export class JavascriptErrorReport extends ErrorReport { | ||
getErrorReport() { | ||
return { | ||
error_message: this.e.error.message, | ||
traceback: this.e.error.stack, | ||
context: this.context, | ||
}; | ||
} | ||
} | ||
|
||
export class UnhandledRejectionErrorReport extends ErrorReport { | ||
getErrorReport() { | ||
return { | ||
error_message: this.e.reason.message, | ||
traceback: this.e.reason.stack, | ||
context: this.context, | ||
}; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
import logging | ||
|
||
from django.core.exceptions import ValidationError | ||
from rest_framework import status | ||
from rest_framework.decorators import api_view | ||
from rest_framework.response import Response | ||
|
||
from .constants import FRONTEND | ||
from .models import ErrorReports | ||
from .serializers import ErrorReportSerializer | ||
|
||
|
||
logger = logging.getLogger(__name__) | ||
|
||
|
||
@api_view(["POST"]) | ||
def report(request): | ||
serializer = ErrorReportSerializer(data=request.data) | ||
if serializer.is_valid(): | ||
data = serializer.validated_data | ||
try: | ||
error = ErrorReports.insert_or_update_error( | ||
FRONTEND, | ||
data["error_message"], | ||
data["traceback"], | ||
context=data["context"], | ||
) | ||
return Response( | ||
{"error_id": error.id if error else None}, status=status.HTTP_200_OK | ||
) | ||
|
||
except (AttributeError, ValidationError) as e: | ||
logger.error("Error while saving error report: {}".format(e)) | ||
return Response( | ||
{"error": "An error occurred while saving errors."}, | ||
status=status.HTTP_400_BAD_REQUEST, | ||
) | ||
else: | ||
return Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
from django.urls import re_path | ||
|
||
from .api import report | ||
|
||
urlpatterns = [re_path(r"^report", report, name="report")] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
from django.apps import AppConfig | ||
|
||
|
||
class KolibriErrorConfig(AppConfig): | ||
name = "kolibri.core.errorreports" | ||
label = "errorreports" | ||
verbose_name = "Kolibri ErrorReports" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
FRONTEND = "frontend" | ||
BACKEND = "backend" | ||
|
||
POSSIBLE_ERRORS = [ | ||
(FRONTEND, "Frontend"), | ||
(BACKEND, "Backend"), | ||
] |
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.
I see this is creating two different pathways that hinges on the
pingback_id
. Inutils.py
, there already exists logic dependent onif "id" in data:
, which is the same condition here. It seems like this fits alongside the existing logic there.