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.1204.ge0e0412c-windows-setup-unsigned.exe
Debian Package	kolibri_0.16.2b2.dev0+git.1204.ge0e0412c-0ubuntu1_all.deb
Mac Installer (DMG)	kolibri-0.16.2b2.dev0+git.1204.ge0e0412c-0.4.2.dmg
Android Package (APK)	kolibri-0.16.2b2.dev0+git.1204.ge0e0412c-0.1.3-debug.apk
TAR file	kolibri-0.16.2b2.dev0+git.1204.ge0e0412c.tar.gz
WHL file	kolibri-0.16.2b2.dev0+git.1204.ge0e0412c-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

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

thesujai added 5 commits July 2, 2024 00:06

update ErrorReports for new fields

d7fce7d

add context field in errorreports/report/

0ec1771

update middleware to capture more fields

5a8ace1

update frontend erroreports to capture more fields

c713fad

update erroreports task to report more fields

da7f8f0

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

add test for tasks

cebe114

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

akolson reviewed Jul 2, 2024

View reviewed changes

thesujai added 5 commits July 3, 2024 23:18

update schema of frontend

cc5e4fe

add installation_type in tasks

b6bbf00

use ua-parser for the device and os

a25275f

add os in context_frontend

69508f6

revert

89614a2

akolson reviewed Jul 10, 2024

View reviewed changes

thesujai added 2 commits July 11, 2024 17:37

clarity

1d5de65

format python version

54730a7

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

thesujai added 2 commits July 15, 2024 13:15

use definations for schema

b267dbd

seeprate device and isTouchDevice

4c86fd2

akolson reviewed Jul 16, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

thesujai commented Jul 1, 2024

github-actions bot commented Jul 1, 2024 •

edited

Loading

akolson Jul 2, 2024

rtibbles Jul 2, 2024

akolson Jul 2, 2024

akolson Jul 2, 2024

akolson Jul 2, 2024 •

edited

Loading

akolson Jul 2, 2024

akolson Jul 2, 2024

akolson Jul 2, 2024

thesujai Jul 2, 2024

akolson Jul 2, 2024

akolson Jul 2, 2024

akolson Jul 2, 2024

akolson Jul 2, 2024

rtibbles Jul 2, 2024

akolson Jul 2, 2024

akolson Jul 2, 2024

akolson Jul 2, 2024

akolson Jul 2, 2024

akolson Jul 2, 2024

akolson Jul 10, 2024

akolson Jul 12, 2024 •

edited

Loading

akolson Jul 12, 2024

akolson Jul 12, 2024

akolson Jul 12, 2024 •

edited

Loading

akolson Jul 12, 2024

akolson Jul 12, 2024

akolson Jul 12, 2024

akolson Jul 12, 2024

akolson left a comment

akolson Jul 16, 2024

thesujai Jul 16, 2024

thesujai Jul 16, 2024

		@@ -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

Are you sure you want to change the base?

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

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