front_end_error_logging #237

salelkafrawy · 2020-09-01T02:34:12Z

Hello all,

I'm running into an issue, here is the detailed explanation:

For the front end error message:

I've added a ErrorBoundary component inside static_react_task/webapp/app.jsx temporarily (once I correct the behavior of the issue I have I'll move it to mephisto-task/utils
I was trying to render a nice looking message box or popup once an error occurs.
In the test case Pratik suggested, the error does show up in the console but the componentDidCatch inside ErrorBoundary component is not called, I've tried to console.log() some messages inside that method with nothing showing up in the console. Am I missing something or it's real that ErrorBoundary could be not triggered in that test case as stated here and I just need to try a different test case?

For propagating the error from the front to the backend:

I added handleFatalError same as handleSubmit in useMephistoTask with just calling handleSubmit with a different data dictionary.
Then I added a property to the ErrorBoundary called handleError which is triggered in componentDidCatch (which is not called properly as I explained in the first part)
was what I added functioning but not running due to the issue of componentDidCatch not being triggered or do I miss something important here?

JackUrb · 2020-09-01T15:00:01Z

One issue is that react doesn't appear to be able to catch errors that happen in handlers, only in lifecycle functions: facebook/react#11409.

These kinds of errors will end up being type 2 and 3 instead.

For testing this, you should throw your error during the render method.

JackUrb

Error logging flow in general looks good. Nice job following the existing conventions for constants and post methods! Next up is going to be polishing and dealing with the global case in a way that allows users to decide if something major broke.

It might be worth some kind of heuristic on global errors, like setting a timeout when they occur, and then seeing if the user is still interacting with the app and making progress (which would imply that the error was actually minor). Any thoughts on what a good practice would be for this @pringshia?

examples/static_react_task/re-build.sh

JackUrb · 2020-09-17T17:55:09Z

examples/static_react_task/webapp/src/app.jsx

+      errorInfo: errorInfo
+    })
+    // alert Mephisto worker of a component error
+    alert("error from a component" + error)


We'll probably want to update this message once everything else is fully hashed out!

Yes. It was an indication for me that the error was raised from ErrorBoundary

I actually don't see a difference between type 2 and type 3 (global) errors. I had to place the window.eventListener('error'..) definition after the useMephistoTask as it uses handleFatalError. And even placing an error at the beginning of MainApp wouldn't fire the window.eventListener('error', ..) nor window.onerror callback as the error is place before defining any of them. So I don't know how to produce a global error that might happen right after defining the MainApp unless useMephistoTask should handle that type inside it, is that right?

Hmm, I wonder why this is happening, as <script src="wrap_crowd_source.js"></script> is defined in the <head> tag. Let me experiment a bit with this and get back to you.

seems like I was able to get type 3 errors to work:

console

examples/parlai_chat_task_demo/custom_simple/main.js

wrap_crowd_source.js

And this is from throwing the error from within a component (inside a React context).

Yes I am able to see them in the console but do you get them back in the backend? there should be a logging INFO for the error sent back to the backend

mephisto/server/architects/router/deploy/server.js

JackUrb · 2020-09-17T17:59:33Z

packages/mephisto-task/src/index.js

+   (data) => {
+       console.log('inside handleFatalError ...')
+       postErrorLog(state.agentId, data)
+//       handleSubmit(data)


We may want to consider a distinction as to whether or not the error is actually fatal - in the cases where it is, that's when we should trigger a submit. This will allow us to surface something to the user when global errors happen, without necessarily forcing them to submit?

How can I categorize the fatal errors from non fatal ones?

as a temporary step you can trigger a confirm() dialog and allow the user to choose? i don't like this because it can be a jarring experience for users, especially in the case of false positives, but it can be a good stopgap for now

I already did this for type 2 errors. Thansk

Do you mean do the confirm() inside componentDidCatch()? because I already do confirm() in the window.onerror event.

examples/static_react_task/webapp/src/app.jsx

packages/mephisto-task/src/utils.js

mephisto/server/architects/router/deploy/server.js

pringshia · 2020-09-18T18:56:13Z

examples/static_react_task/webapp/src/components/core_components.jsx

-                onClick={() => onSubmit({ rating: "good" })}
+                // Test for type 2 errors
+                onClick={()=> {throw new Error("test error event_handler");}}
+//                 onClick={() => onSubmit({ rating: "good" })}


just a reminder to revert this when we're ready to merge

to echo Jack, these errors in the callback may actually be caught as type 3 errors, instead of 2

the event handlers errors which can't be caught by React's ErrorBoundary are caught by the window.onerror event handler and those which can be caught by both the window.onerror event handler and ErrorBoundary are caught twice but this should be the case for mode=development and once the mode=production it should be caught only once in the ErrorBoundary [according to StackOverFlow]

pringshia · 2020-09-18T18:58:03Z

examples/static_react_task/webapp/src/app.jsx

+          console.log("You pressed Cancel!");
+        }
+        return true;
+})


once this seems to be working correctly, let's move it into wrap_crowd_source.js so all tasks automatically inherit this functionality

pringshia · 2020-09-18T18:59:53Z

examples/static_react_task/webapp/src/app.jsx

@@ -11,19 +11,76 @@ import ReactDOM from "react-dom";
 import { BaseFrontend, LoadingScreen } from "./components/core_components.jsx";
 import { useMephistoTask } from "mephisto-task";

+class ErrorBoundary extends React.Component {


great first step in implementing this. the next step would be to move this component into mephisto-task and import it into this project from there. this will allow us to have a reusable mephisto component we can use from other tasks

pringshia · 2020-09-18T19:02:51Z

As Jack mentioned componentDidCatch is not being triggered because the error is being thrown from an onClick event handler, which gets run in a different execution context. What you'd want to do instead if throw an error from within the render method (that is not triggered via an event handler).

salelkafrawy · 2020-09-24T01:08:58Z

I've polished the debugging logs and script, and re-located the error handling components (window.onerror event and ErrorBoundary react component) to be inside useMephistoTask and utils.js respectively.

I tried to add window.onerror to wrap_crowd_source as you said @pringshia but I need handleFatalError from useMephistoTask and also I didn't understand how this can be used for other providers since wrap_crowd_source is per provider. Did you mean to make it specific for every provider?

pringshia · 2020-10-06T18:56:06Z

Are you still planning on adding a const to the top of wrap_crowd_source.js to flag between the two different behaviors we discussed? (i.e. auto-submit on error vs. prompt to submit on error).

pringshia · 2020-10-08T17:16:44Z

mephisto/providers/mock/wrap_crowd_source.js

@@ -15,6 +15,7 @@ for both to be able to register them with the backend database.
 Returning None for the assignment_id means that the task is being
 previewed by the given worker.
 \------------------------------------------*/
+auto_submit = false


looks good, I would make this ALL_CAPS as such and declare it as a const

pringshia · 2020-10-08T17:17:29Z

mephisto/providers/mock/wrap_crowd_source.js

+            }
+  }
+  else {
+    console.log("sending to email address: ####")


we can wire up this portion to the /log_error endpoint you created?

JackUrb · 2020-10-08T18:32:21Z

mephisto/providers/mock/wrap_crowd_source.js

+          prompt('send the following error to the email address: '+
+          '[email address]', JSON.stringify(event.error.message))


Sorry a bit out of context here - why are we asking for an email address?

Pratik told me there should be a choice whether for the user to auto submit the error (wiring up /log_error) or show them an email and a copy-enabled version of the error message to send that error by themselves to an email address.

Understood - generally we may have a case where we want the submit to be optional but not explicitly state the contact info (for a case where people may not want to expose their actual email, but instead get contacted through the site). So really there should be three options:

Auto-submit. Display the message "An error has occurred. This has been logged to the requester automatically."

Don't auto-submit. Display the message. "We think an error has occurred. If the task is broken, please send this context to the requester.

Don't auto-submit. Same message as the above, but with an email instead of "to the requester"

Edit: I'm realizing this is attached to a specific wrap_crowd_source.js file, meaning the scenario here is fixed.

JackUrb · 2020-10-08T18:49:44Z

mephisto/providers/mock/wrap_crowd_source.js

@@ -53,3 +54,22 @@ function handleSubmitToProvider(task_data) {
    alert("The task has been submitted! Data: " + JSON.stringify(task_data))
    return true;
 }
+
+// Adding event listener instead of using window.onerror prevents the error to be caught twice
+window.addEventListener('error', function (event) {


@pringshia is there a reason this listener is defined in wrap_crowd_source.js and not as a part of mephisto-task?

the first place I put it inside mephisto-task but Pratik said it's better to be inside each provider separately but I didn't find it convenience because we will need to put it in each Provider's wrap_crowd_source.js, @pringshia could you elaborate why is this better?

@JackUrb - The reason for putting this in wrap_crowd_source.js as opposed to mephisto-task is so that this global catch-all error handling (list as #3 here: #231) can also apply to:

Errors in static HTML tasks

Errors that may occur at the React library level, before the application code has had a chance to run (maybe related due to a build linking / webpack error?)

facebook-github-bot · 2020-10-13T21:15:54Z

Hi @saraebrahim!

Thank you for your pull request. We require contributors to sign our Contributor License Agreement, and yours needs attention.

You currently have a record in our system, but we do not have a signature on file.

In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

JackUrb · 2020-12-30T18:35:24Z

Changes in here are all included in #343, so I'm closing this version.

salelkafrawy requested review from pringshia and JackUrb September 1, 2020 02:34

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 1, 2020

JackUrb reviewed Sep 17, 2020

View reviewed changes

pringshia reviewed Sep 18, 2020

View reviewed changes

examples/static_react_task/webapp/src/app.jsx Outdated Show resolved Hide resolved

pringshia reviewed Sep 18, 2020

View reviewed changes

packages/mephisto-task/src/utils.js Show resolved Hide resolved

pringshia reviewed Sep 18, 2020

View reviewed changes

mephisto/server/architects/router/deploy/server.js Outdated Show resolved Hide resolved

pringshia reviewed Sep 18, 2020

View reviewed changes

salelkafrawy added 5 commits September 23, 2020 10:47

initial attempt with first type of error

15dd8a5

front and backend error with handleSubmit

5e61496

new endpoint for frontend error log to server

7ee36b6

fix some nits

b5e76db

organizing error handlers final location

3395550

salelkafrawy force-pushed the sara_front_end_error branch from 6caa1c4 to 3395550 Compare September 24, 2020 01:05

salelkafrawy requested review from JackUrb and pringshia September 24, 2020 01:09

salelkafrawy changed the title ~~[WIP] front_end_error_logging~~ front_end_error_logging Sep 26, 2020

add window.addEventListener("error") to mock wrap_crowd_source.js

4bcf6d6

adding auto-submit boolean in window error event

b9c464b

pringshia reviewed Oct 8, 2020

View reviewed changes

JackUrb reviewed Oct 8, 2020

View reviewed changes

pringshia linked an issue Oct 12, 2020 that may be closed by this pull request

Add front-end error logging support to the Web UI #231

Closed

salelkafrawy closed this Nov 10, 2020

JackUrb reopened this Nov 11, 2020

pringshia mentioned this pull request Dec 17, 2020

UI Error Logging #343

Merged

JackUrb closed this Dec 30, 2020

JackUrb deleted the sara_front_end_error branch January 6, 2021 21:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

front_end_error_logging #237

front_end_error_logging #237

salelkafrawy commented Sep 1, 2020

JackUrb commented Sep 1, 2020

JackUrb left a comment

JackUrb Sep 17, 2020

salelkafrawy Sep 18, 2020

salelkafrawy Sep 18, 2020

pringshia Sep 23, 2020 •

edited

Loading

pringshia Sep 23, 2020 •

edited

Loading

pringshia Sep 23, 2020 •

edited

Loading

salelkafrawy Sep 23, 2020

JackUrb Sep 17, 2020

salelkafrawy Sep 18, 2020

pringshia Sep 23, 2020 •

edited

Loading

salelkafrawy Sep 23, 2020

salelkafrawy Sep 23, 2020

pringshia Sep 18, 2020

pringshia Sep 18, 2020

salelkafrawy Sep 24, 2020

pringshia Sep 18, 2020

pringshia Sep 18, 2020 •

edited

Loading

pringshia commented Sep 18, 2020

salelkafrawy commented Sep 24, 2020

pringshia commented Oct 6, 2020

pringshia Oct 8, 2020

pringshia Oct 8, 2020

JackUrb Oct 8, 2020

salelkafrawy Oct 8, 2020

JackUrb Oct 8, 2020 •

edited

Loading

JackUrb Oct 8, 2020

salelkafrawy Oct 8, 2020

pringshia Oct 12, 2020 •

edited

Loading

facebook-github-bot commented Oct 13, 2020

JackUrb commented Dec 30, 2020

		prompt('send the following error to the email address: '+
		'[email address]', JSON.stringify(event.error.message))

front_end_error_logging #237

front_end_error_logging #237

Conversation

salelkafrawy commented Sep 1, 2020

JackUrb commented Sep 1, 2020

JackUrb 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

pringshia Sep 23, 2020 • edited Loading

Choose a reason for hiding this comment

pringshia Sep 23, 2020 • edited Loading

Choose a reason for hiding this comment

console

examples/parlai_chat_task_demo/custom_simple/main.js

wrap_crowd_source.js

pringshia Sep 23, 2020 • 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

pringshia Sep 23, 2020 • 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

pringshia Sep 18, 2020 • edited Loading

Choose a reason for hiding this comment

pringshia commented Sep 18, 2020

salelkafrawy commented Sep 24, 2020

pringshia commented Oct 6, 2020

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

JackUrb Oct 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pringshia Oct 12, 2020 • edited Loading

Choose a reason for hiding this comment

facebook-github-bot commented Oct 13, 2020

JackUrb commented Dec 30, 2020

pringshia Sep 23, 2020 •

edited

Loading

pringshia Sep 23, 2020 •

edited

Loading

pringshia Sep 23, 2020 •

edited

Loading

pringshia Sep 23, 2020 •

edited

Loading

pringshia Sep 18, 2020 •

edited

Loading

JackUrb Oct 8, 2020 •

edited

Loading

pringshia Oct 12, 2020 •

edited

Loading