Distributed error reporting: Capture more information about the error and its environment #12382

thesujai · 2024-07-01T08:48:47Z

Summary

Changes the model ErrorReports by renaming some fields like error_from to category and more.
Adds new JSON fields for backend and frontend so we can store more contexts of error into them
Update the remaining part of the code with the model changes, by update in backend error handling and frontend error handling mechanism

References

Closes #12376

Reviewer guidance

Cannot find a way yo get the node version in frontend

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-01T09:21:49Z

Build Artifacts

Asset type	Download link
PEX file	kolibri-.pex
Windows Installer (EXE)	kolibri-0.16.2b2.dev0+git.1214.gbcac96a1-windows-setup-unsigned.exe
Debian Package	kolibri_0.16.2b2.dev0+git.1214.gbcac96a1-0ubuntu1_all.deb
Mac Installer (DMG)	kolibri-0.16.2b2.dev0+git.1214.gbcac96a1-0.4.2.dmg
Android Package (APK)	kolibri-0.16.2b2.dev0+git.1214.gbcac96a1-0.1.3-debug.apk
TAR file	kolibri-0.16.2b2.dev0+git.1214.gbcac96a1.tar.gz
WHL file	kolibri-0.16.2b2.dev0+git.1214.gbcac96a1-py2.py3-none-any.whl

akolson · 2024-07-02T17:26:46Z

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

+        component: this.vm.$options.name || this.vm.$options._componentTag || 'Unknown Component',
+        browser: this.getBrowserInfo(),
+        device: this.getDeviceInfo(),
+      },


context can be movet to ErrorReport too. We can then do something like the below;

getErrorReport() { const context = this.getContext(); return { ... context: { component: this.vm.$options.name || this.vm.$options._componentTag || 'Unknown Component', ...context, }, }; }

Could even not even need to define getErrorReport in this class, instead can just define it in the parent class once, and then here we do:

getContext() { const context = super.getContext(); return { ...context, component <etc>... }; }

akolson · 2024-07-02T17:28:13Z

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

+      context: {
+        browser: this.getBrowserInfo(),
+        device: this.getDeviceInfo(),
+      },


Same here https://github.com/learningequality/kolibri/pull/12382/files#r1662911798. Though we would just assign the context

context: this.getContext()

akolson · 2024-07-02T17:28:41Z

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

+      context: {
+        browser: this.getBrowserInfo(),
+        device: this.getDeviceInfo(),
+      },


See https://github.com/learningequality/kolibri/pull/12382/files#r1662911798

akolson · 2024-07-02T17:31:08Z

kolibri/core/errorreports/schemas.py

+        "device": {
+            "type": "object",
+            "properties": {
+                "type": {"type": "string"},  # "desktop", "tablet", "mobile"


With json schema we can ensure the device is from a predefined list of device types

akolson · 2024-07-02T17:35:53Z

kolibri/core/errorreports/schemas.py

+                "url": {"type": "string", "optional": True},
+                "method": {"type": "string", "optional": True},
+                "headers": {"type": "object", "optional": True},
+                "body": {"type": "string", "optional": True},


I think properties are optional by default so there is not need to explicitly set them as optional. So you might as well do away with the optional flag?

Also, we can use the required property that specifies the required fields instead where necessary

(There are other sections in the schema with the same)

@akolson I too had a doubt about this. But this comment suggested otherwise:

kolibri/kolibri/core/auth/models.py

Line 90 in dc7a843

# '"optional":True' is obsolete but needed while we keep using an

Since the use of the optional property is specific to a particular version of jsonschema, we'll carry on with using it, but create a follow up issue to to update all jsonschemas as python 2.7 support has been dropped (the main reason the current version of jsonschema is used)

akolson · 2024-07-02T19:11:42Z

kolibri/core/errorreports/schemas.py

+                "platform": {
+                    "type": "string",
+                    "optional": True,
+                },  # "windows", "mac", "linux", "android", "ios"


With json schema we can ensure the platform is from a predefined list of platforms

akolson · 2024-07-02T20:19:29Z

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

@@ -6,13 +6,35 @@ class ErrorReport {
  getErrorReport() {
    throw new Error('getErrorReport() method must be implemented.');
  }
+  getDeviceInfo() {
+    return {
+      type: /Mobi|Android/i.test(navigator.userAgent) ? 'Mobile' : 'Desktop',


This will return the wrong type if kolibri is used on a tablet for example.

(Just noting that this is quite an unreliable way to check for device type)

Yes, definitely just relying on the values already defined in browserInfo.js seems like the right way forward here, no need to write new code when we already have robust parsing of the userAgent.

akolson · 2024-07-02T20:20:30Z

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

+  getDeviceInfo() {
+    return {
+      type: /Mobi|Android/i.test(navigator.userAgent) ? 'Mobile' : 'Desktop',
+      platform: navigator.platform,


We can use os in browserInfo.js

akolson · 2024-07-02T20:21:10Z

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

  getErrorReport() {
    return {
      error_message: this.e.message,
      traceback: this.e.stack,
+      context: {
+        component: this.vm.$options.name || this.vm.$options._componentTag || 'Unknown Component',
+        browser: this.getBrowserInfo(),


We can use browser in browserInfo.js

akolson · 2024-07-02T21:23:54Z

kolibri/core/errorreports/middleware.py

+
+
+def get_python_version():
+    return version.split()[0]


I think this would only pick the first version of the tuple. See here

akolson · 2024-07-02T21:31:59Z

kolibri/core/errorreports/models.py

@@ -47,25 +54,55 @@ def allow_migrate(self, db, app_label, model_name=None, **hints):


 class ErrorReports(models.Model):


Also noting that

kolibri/kolibri/core/device/api.py

Line 201 in 2418619

info["installer"] = installation_type()

is a very important piece of information to store.

Similarly, we should store is_touch_device to clearly distinguish between touch and non touch devices

akolson · 2024-07-10T14:08:36Z

kolibri/core/errorreports/tasks.py

        errors_json = serialize_error_reports_to_json_response(errors)
+
        requests.post(
            join_url(server, "/api/v1/errors/"),


Let's change the resource to /api/v1/errors/report/ for clarity.

akolson · 2024-07-12T12:17:37Z

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

+device.type = device.type || 'unknown';
+device.model = device.model || 'unknown';
+device.vendor = device.vendor || 'unknown';
+export const deviceWithTouch = {


I think this would be superfluous!
I think it would be more straight forward to just export device info only. I dont see the need to bundle it up with the device info.

I think the below would suffice

export const device = info.device;

Also deviceWithTouch could be misinterpreted to mean that it represents an object with touch capabilities which is not really the case -- isTouchDevice is a runtime value.

akolson · 2024-07-12T12:19:08Z

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

+// this is better than undefined
+device.type = device.type || 'unknown';
+device.model = device.model || 'unknown';
+device.vendor = device.vendor || 'unknown';


I am not sure there any value add of doing this!

akolson · 2024-07-12T12:26:05Z

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

@@ -1,3 +1,5 @@
+import { browser, os, deviceWithTouch } from './browserInfo';


Lets import device and isTouchDevice instead of deviceWithTouch here. See https://github.com/learningequality/kolibri/pull/12382/files#r1675763461

akolson · 2024-07-12T12:28:41Z

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

+      browser: browser,
+      os: os,
+      device: {
+        ...deviceWithTouch,


instead of deviceWithTouch, we would have

device: { ...device, isTouchDevice, screen: { ... } }

akolson · 2024-07-12T12:54:31Z

kolibri/core/errorreports/schemas.py

+                "name": {"type": "string", "optional": True},
+                "major": {"type": "string", "optional": True},
+                "minor": {"type": "string", "optional": True},
+                "patch": {"type": "string", "optional": True},


These can de defined as a property and referenced instead...

akolson · 2024-07-12T12:54:53Z

kolibri/core/errorreports/schemas.py

+                "name": {"type": "string", "optional": True},
+                "major": {"type": "string", "optional": True},
+                "minor": {"type": "string", "optional": True},
+                "patch": {"type": "string", "optional": True},


See https://github.com/learningequality/kolibri/pull/12382/files#r1675804546

For ecample it would mean adding definitions

.... "definitions": { "versionInfo": { "type": "object", "properties": { "name": {"type": "string", "optional": True}, "major": {"type": "string", "optional": True}, "minor": {"type": "string", "optional": True}, "patch": {"type": "string", "optional": True}, } } }

and then use it as below;

"os": { "$ref": "#/definitions/versionInfo", }, ... "browser": { "$ref": "#/definitions/versionInfo", }

akolson

Hi @thesujai! Great work on the changes, The pr is now taking shape! I left a few comments that would be good to implement. Thanks

akolson · 2024-07-16T14:54:06Z

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

@@ -64,6 +64,13 @@ export const os = {
  patch: osVersion[2],
 };

+// Device info
+export const device = {
+  type: info.device.type || 'desktop',


Why are we defaulting to 'desktop'?

The ua-parser library gives undefined for desktop. As according to them desktop is not an device type.

faisalman/ua-parser-js#182

akolson

LGTM! Thanks @thesujai. @rtibbles any other further comments before the merge?

…d, remove mark_as_reported

…ave method

… method

rtibbles

Sorry, I had missed the 500 errors previously, these should not be explicitly raised, and instead should raise a 400 error, ideally with a specific description of why the request was rejected. Will need to update the tests accordingly.

rtibbles · 2024-08-01T22:06:02Z

kolibri/core/errorreports/api.py

            logger.error("Error while saving error report: {}".format(e))
            return Response(
-                {"error": "Error while saving error report"},
+                {"error": "Error while saving error report: {}".format(e)},
                status=status.HTTP_500_INTERNAL_SERVER_ERROR,


Sorry, I had missed this previously - we should never deliberately raise a 500 error, it indicates that something has gone wrong and is unhandled. Instead, this should be a 400 error.

rtibbles · 2024-08-01T22:07:12Z

kolibri/core/errorreports/api.py

            )
            return Response(
                {"error_id": error.id if error else None}, status=status.HTTP_200_OK
            )
-        except (AttributeError, Exception) as e:
+
+        except (AttributeError, ValidationError, Exception) as e:


We shouldn't catch a bare Exception here - AttributeError and ValidationErrors we expect, because we are accessing specific attributes and validation the data.

rtibbles · 2024-08-01T22:08:47Z

This is looking good in general - and I think most of the previous request for changes have been addressed, I just noticed one more thing around error handling.

thesujai · 2024-08-02T05:41:19Z

Updated the PR

rtibbles

All my comments have been addressed!

akolson

The updates and code generally looks cleaner. Thanks @thesujai.

github-actions bot added DEV: backend Python, databases, networking, filesystem... DEV: frontend SIZE: large labels Jul 1, 2024

thesujai changed the title ~~Distributed error reporting task7~~ Distributed error reporting: Capture more information about the error and its environment Jul 1, 2024

akolson force-pushed the distributed-error-reporting-task7 branch from 6459176 to da7f8f0 Compare July 1, 2024 21:20

akolson requested review from rtibbles and akolson July 2, 2024 16:51

akolson reviewed Jul 2, 2024

View reviewed changes

akolson reviewed Jul 10, 2024

View reviewed changes

akolson self-requested a review July 12, 2024 12:07

akolson reviewed Jul 12, 2024

View reviewed changes

akolson requested changes Jul 12, 2024

View reviewed changes

akolson reviewed Jul 16, 2024

View reviewed changes

akolson reviewed Jul 17, 2024

View reviewed changes

thesujai added 20 commits July 29, 2024 14:32

update middleware to capture more fields

44ce4e1

update frontend erroreports to capture more fields

2798cdf

update erroreports task to report more fields

bea2e64

add test for tasks

afef5ad

update schema of frontend

e3252ee

add installation_type in tasks

01474a2

use ua-parser for the device and os

d530b3a

add os in context_frontend

7056944

revert

109acd2

clarity

cd373c4

format python version

a558f37

use definations for schema

6c4ce3a

seeprate device and isTouchDevice

1cc20dc

change screen object and move getContext to parent method

a7414c8

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

dea1d32

…d, remove mark_as_reported

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

86159d0

…ave method

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

22cfce2

… method

use single context

15e34ba

add more screen info in schemas and remove default schemas

85cf9c0

add query_params and improve packages retreival

c5eb2ae

thesujai force-pushed the distributed-error-reporting-task7 branch from b10318b to c5eb2ae Compare July 29, 2024 09:06

Add pingback_id and request_time

14ceceb

rtibbles requested changes Aug 1, 2024

View reviewed changes

raise 400 instead of 500

ee7eb3e

rtibbles approved these changes Aug 6, 2024

View reviewed changes

akolson approved these changes Aug 7, 2024

View reviewed changes

akolson merged commit d8a713e into learningequality:distributed-error-reporting Aug 7, 2024
10 checks passed

akolson mentioned this pull request Aug 7, 2024

Update the ErrorReports model to have more context on errors #12376

Closed

		@@ -47,25 +54,55 @@ def allow_migrate(self, db, app_label, model_name=None, **hints):


		class ErrorReports(models.Model):

		@@ -1,3 +1,5 @@
		import { browser, os, deviceWithTouch } from './browserInfo';

Distributed error reporting: Capture more information about the error and its environment #12382

Distributed error reporting: Capture more information about the error and its environment #12382

Conversation

thesujai commented Jul 1, 2024

Summary

References

Reviewer guidance

Testing checklist

PR process

Reviewer checklist

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

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

akolson Jul 2, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akolson Jul 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akolson Jul 12, 2024 • edited Loading

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

akolson 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

akolson left a comment

Choose a reason for hiding this comment

rtibbles left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtibbles commented Aug 1, 2024

thesujai commented Aug 2, 2024

rtibbles left a comment

Choose a reason for hiding this comment

akolson left a comment

Choose a reason for hiding this comment

github-actions bot commented Jul 1, 2024 •

edited

Loading

akolson Jul 2, 2024 •

edited

Loading

akolson Jul 12, 2024 •

edited

Loading

akolson Jul 12, 2024 •

edited

Loading