GSoC: Distributed error reporting #12489

akolson · 2024-07-25T19:04:25Z

Summary

This pr implements the distributed error reporting feature as part of the Google Summer of Code(GSoC) 2024 program. See the epic for details.

References

Closes #12214

Reviewer guidance

All tests associated with the implementation must run successfully

Testing checklist

Contributor has fully tested the PR manually
If there are any front-end changes, before/after screenshots are included
Critical user journeys are covered by Gherkin stories
Critical and brittle code paths are covered by unit tests

PR process

PR has the correct target branch and milestone
PR has 'needs review' or 'work-in-progress' label
If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
If this is an important user-facing change, PR or related issue has a 'changelog' label
If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

Automated test coverage is satisfactory
PR is fully functional
PR has been tested for accessibility regressions
External dependency files were updated if necessary (yarn and pip)
Documentation is updated
Contributor is in AUTHORS.md

github-actions · 2024-07-25T19:36:02Z

Build Artifacts

kolibri/core/errorreports/api.py

bjester

Main blockers: we should add more logic to remove parameters like passwords from request data, and we should have request timeouts configured on the report requests

bjester · 2024-10-08T17:51:06Z

kolibri/core/assets/src/api-resources/__tests__/errorReport.test.js

+      },
+    };
+
+    Resource.client = jest.fn();


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.client, there isn't a way for it to be restored. So it would be better to use mock.spyOn or mock.replaceProperty here, and do so in the beforeEach. Then instead of clearAllMocks in afterEach (which only clears the mock state), I would suggest using restoreAllMocks as that would ensure any mocks are restored to what they should be (assuming the appropriate approach was used to create the mock in the first place).

bjester · 2024-10-08T17:55:15Z

kolibri/core/assets/src/utils/errorReportUtils.js

+          height: window.screen.height,
+          available_width: window.screen.availWidth,
+          available_height: window.screen.availHeight,
+        },


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

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.

bjester · 2024-10-08T17:56:28Z

kolibri/core/errorreports/middleware.py

+    request_headers.pop("Cookie", None)
+
+    request_get = dict(request.GET)
+    request_get.pop("token", None)


In addition, probably for POST, we should ensure passwords are not sent?

bjester · 2024-10-08T17:58:02Z

kolibri/core/errorreports/models.py

+                error_report.context = context
+
+        error_report.save()
+        logger.error("ErrorReports: Database updated.")


This feels more like info-type logging?

bjester · 2024-10-08T18:01:37Z

kolibri/core/analytics/tasks.py

-        ping_once(started, server=server)
+        pingback_id = ping_once(started, server=server)
+        if pingback_id:
+            ping_error_reports.enqueue(args=(server, pingback_id))


I see this is creating two different pathways that hinges on the pingback_id. In utils.py, there already exists logic dependent on if "id" in data:, which is the same condition here. It seems like this fits alongside the existing logic there.

bjester · 2024-10-08T18:07:07Z

kolibri/core/assets/src/core-app/index.js

+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}`);


I know the unhandledrejection listener will prevent default logging of the error, so in regards to that and the other logging statements, I'm concerned whether these are suppressing necessary log information, i.e. a stack trace, that developers would need? If logging.error outputs a stack trace, that may not be the same trace as the error itself.

Is there a way to check whether we are in a prod or dev env in the frontend? If yes then we can skip the preventDefault and use logging.debug instead when in dev mode

On second thought, as error_reports is a plugin now, can we unplug it by default in dev environment?

Unplugging the plugin sounds fine for development. Although, having a proper stack trace logged to the console is helpful regardless of environment. On line 40 of this file, you can see that the it configures the logger based off environment.

What I'm trying to point out is if logging.error eventually feeds into console.error, the stack trace it prints would likely be different for:

`Unhandled Rejection: ${event.reason}`

then it would be for:

event.reason

assuming event.reason is an Error object.

bjester · 2024-10-08T18:09:52Z

kolibri/core/errorreports/tasks.py

+            join_url(server, "/api/v1/errors/report/"),
+            data=errors_json,
+            headers={"Content-Type": "application/json"},
+        )


Since this is using raw python requests, lets ensure this has explicit timeouts configured, and ideally separate timeouts for connection vs request.

Can we use NetworkClient here ? The other ping-back task already uses NetworkClient

Yeah if it's compatible, using the NetworkClient is just fine!

thesujai · 2025-02-21T05:30:41Z

I am just seeing the reviews here

add tests for model methods

Add test for error-report middleware

move POSSIBLE_ERRORS to contants.py

add API for frontend error report testcase for frontendreport view

…or()

tests for error_report task add error report task

…SONEncoder

…d, remove mark_as_reported

…ave method

… method

remove installation_type and release_version\n move request_time_to_error to context\n remove sensitive info from the requests info\n only use traceback and error_msg to fingerprint an error_report

akolson added DEV: backend Python, databases, networking, filesystem... gsoc A GSoC project task labels Jul 25, 2024

akolson added this to the Distributed Error Reporting milestone Jul 25, 2024

github-actions bot added the DEV: frontend label Jul 25, 2024

akolson changed the title ~~Distributed error reporting~~ GSoC: Distributed error reporting Jul 29, 2024

github-advanced-security bot found potential problems Aug 7, 2024

View reviewed changes

kolibri/core/errorreports/api.py Fixed Show fixed Hide fixed

rtibbles assigned bjester and LianaHarris360 Sep 24, 2024

bjester requested changes Oct 8, 2024

View reviewed changes

rtibbles added the DEV: Core JS API Changes related to, or to the Core JS API label Nov 5, 2024

thesujai added 18 commits March 10, 2025 08:42

create new errorreports app and database for writing reports to

d05a768

add ErrorReports model with and its class methods

f3e18ec

add tests for model methods

Add middleware for handling runtime errors

429c42a

Add test for error-report middleware

Simplify calling insert_or_update_error and tests

bbf15e9

put all the constants together in errorreports

06593ed

move POSSIBLE_ERRORS to contants.py

improve testcase for middleware

e8b9f9d

add serializer ErrorReprotsSerializers:frontend data validation

27e4d71

add API for frontend error report testcase for frontendreport view

make error_from default to 'frontend'

de95e45

simplify API: remove conditioning before calling insert_or_update_err…

5f435e7

…or()

name changes

766c324

expect (AttributeError, Exception) while calling insert_or_update

45cd9c0

test for anything other than AttributeError or Exception can be caught

7b2bb54

create task ping_error_report

8f47dfe

tests for error_report task add error report task

improve code

dfd9d6e

improvise: remove mark_as_sent to use update on queryset, use DjangoJ…

677b985

…SONEncoder

remove mark_errors_as_sent

85a15af

update ErrorReports for new fields

e42837c

add context field in errorreports/report/

61ba3b5

thesujai and others added 24 commits March 10, 2025 14:56

update middleware to capture more fields

0726d38

update erroreports task to report more fields

52bd259

add test for tasks

3076a92

update schema of frontend

eeff206

add installation_type in tasks

d95a2b9

use ua-parser for the device and os

f363395

add os in context_frontend

fa2221b

revert

745591e

clarity

817435b

format python version

538f20e

use definations for schema

9894ea3

seeprate device and isTouchDevice

f6e93ce

changes: single context instead of two, full version instead of parse…

8f75d5d

…d, remove mark_as_reported

changes: importlib instead of pkg_resources and pass context to the s…

5659d98

…ave method

changes: modelserializer instead of regular, pass context to the save…

1862489

… method

use single context

25027ee

add more screen info in schemas and remove default schemas

7701fa1

add query_params and improve packages retreival

534ed36

Add pingback_id and request_time

58084c0

raise 400 instead of 500

b7aea46

reruns migrations

49bcd82

Removes information exposure through exception

5ee68a7

refactor stuffs

cb6d7f0

remove installation_type and release_version\n move request_time_to_error to context\n remove sensitive info from the requests info\n only use traceback and error_msg to fingerprint an error_report

use get_or_create() with defaults arg

31f1c86

rtibbles force-pushed the distributed-error-reporting branch from 0c82a59 to 1d90422 Compare March 10, 2025 21:56

rtibbles added 2 commits March 10, 2025 16:07

Make error reporting pluggable.

1dfab60

Add error capturing for tasks.

97a325d

rtibbles force-pushed the distributed-error-reporting branch from 716e662 to b5ba36f Compare March 11, 2025 00:23

Refactor to standardize naming of core app and model.

8b3b22a

rtibbles force-pushed the distributed-error-reporting branch from d408e96 to 8b3b22a Compare March 11, 2025 00:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GSoC: Distributed error reporting #12489

GSoC: Distributed error reporting #12489

akolson commented Jul 25, 2024 •

edited

Loading

github-actions bot commented Jul 25, 2024 •

edited

Loading

bjester left a comment

bjester Oct 8, 2024

bjester Oct 8, 2024

rtibbles Nov 5, 2024

bjester Oct 8, 2024

bjester Oct 8, 2024

bjester Oct 8, 2024

bjester Oct 8, 2024

thesujai Feb 21, 2025

thesujai Feb 21, 2025

bjester Feb 24, 2025

bjester Oct 8, 2024

thesujai Feb 21, 2025

bjester Feb 24, 2025

thesujai commented Feb 21, 2025

GSoC: Distributed error reporting #12489

Are you sure you want to change the base?

GSoC: Distributed error reporting #12489

Conversation

akolson commented Jul 25, 2024 • edited Loading

Summary

References

Reviewer guidance

Testing checklist

PR process

Reviewer checklist

github-actions bot commented Jul 25, 2024 • edited Loading

bjester left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thesujai commented Feb 21, 2025

akolson commented Jul 25, 2024 •

edited

Loading

github-actions bot commented Jul 25, 2024 •

edited

Loading