Skip to content
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

front_end_error_logging #237

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions examples/static_react_task/webapp/src/app.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,18 +9,20 @@
import React from "react";
import ReactDOM from "react-dom";
import { BaseFrontend, LoadingScreen } from "./components/core_components.jsx";
import { useMephistoTask } from "mephisto-task";
import { useMephistoTask, ErrorBoundary } from "mephisto-task";

/* ================= Application Components ================= */

function MainApp() {

const {
blockedReason,
blockedExplanation,
isPreview,
isLoading,
initialTaskData,
handleSubmit,
handleFatalError,
isOnboarding,
} = useMephistoTask();

Expand Down Expand Up @@ -50,14 +52,15 @@ function MainApp() {
</section>
);
}

return (
<div>
<ErrorBoundary handleError={handleFatalError}>
<BaseFrontend
taskData={initialTaskData}
onSubmit={handleSubmit}
isOnboarding={isOnboarding}
/>
</ErrorBoundary>
</div>
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,10 @@ function SimpleFrontend({ taskData, isOnboarding, onSubmit }) {
if (isOnboarding) {
return <OnboardingComponent onSubmit={onSubmit} />;
}

// test case for Type 1 error
// throw new Error('Test SimpleFrontend component error!');

return (
<div>
<Directions>
Expand Down
10 changes: 10 additions & 0 deletions mephisto/core/supervisor.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
PACKET_TYPE_SUBMIT_ONBOARDING,
PACKET_TYPE_REQUEST_ACTION,
PACKET_TYPE_UPDATE_AGENT_STATUS,
PACKET_TYPE_ERROR_LOG,
)
from mephisto.data_model.worker import Worker
from mephisto.data_model.qualification import worker_is_qualified
Expand Down Expand Up @@ -661,13 +662,22 @@ def _get_init_data(self, packet, channel_info: ChannelInfo):
packet.receiver_id = agent_id
agent_info.agent.pending_observations.append(packet)

@staticmethod
def _log_frontend_error(packet):
error_msg = packet.data['final_data'].get("errorMsg")
error_stack = packet.data['final_data'].get("error")
logger.info(f"[FRONT_END_ERROR]: {error_msg}")
logger.info(f"[FRONT_END_ERROR_Trace]: {error_stack}")

def _on_message(self, packet: Packet, channel_info: ChannelInfo):
"""Handle incoming messages from the channel"""
# TODO(#102) this method currently assumes that the packet's sender_id will
# always be a valid agent in our list of agent_infos. At the moment this
# is a valid assumption, but will not be on recovery from catastrophic failure.
if packet.type == PACKET_TYPE_AGENT_ACTION:
self._on_act(packet, channel_info)
elif packet.type == PACKET_TYPE_ERROR_LOG:
self._log_frontend_error(packet)
elif packet.type == PACKET_TYPE_NEW_AGENT:
self._register_agent(packet, channel_info)
elif packet.type == PACKET_TYPE_SUBMIT_ONBOARDING:
Expand Down
1 change: 1 addition & 0 deletions mephisto/data_model/packet.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
PACKET_TYPE_ALIVE = "alive"
PACKET_TYPE_PROVIDER_DETAILS = "provider_details"
PACKET_TYPE_SUBMIT_ONBOARDING = "submit_onboarding"
PACKET_TYPE_ERROR_LOG = "log_error"


class Packet:
Expand Down
20 changes: 20 additions & 0 deletions mephisto/providers/mock/wrap_crowd_source.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// MOCK IMPLEMENTATION
function getWorkerName() {
Expand Down Expand Up @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@pringshia pringshia Oct 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


if (event.error.hasBeenCaught !== undefined){
return false
}
event.error.hasBeenCaught = true
if (!auto_submit) {
if (confirm("Do you want to report the error?")) {
prompt('send the following error to the email address: '+
'[email address]', JSON.stringify(event.error.message))
Comment on lines +67 to +68
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@JackUrb JackUrb Oct 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Auto-submit. Display the message "An error has occurred. This has been logged to the requester automatically."
  2. 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.
  3. 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.

}
}
else {
console.log("sending to email address: ####")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
return true;
})
15 changes: 15 additions & 0 deletions mephisto/server/architects/router/deploy/server.js
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ const PACKET_TYPE_ALIVE = 'alive'
const PACKET_TYPE_PROVIDER_DETAILS = 'provider_details'
const PACKET_TYPE_SUBMIT_ONBOARDING = 'submit_onboarding'
const PACKET_TYPE_HEARTBEAT = 'heartbeat'
const PACKET_TYPE_ERROR_LOG = 'log_error'

// State for agents tracked by the server
class LocalAgentState {
Expand Down Expand Up @@ -301,6 +302,8 @@ wss.on('connection', function(socket) {
update_wanted_acts(packet.sender_id, false);
send_status_for_agent(packet.sender_id);
}
} else if (packet['packet_type'] == PACKET_TYPE_ERROR_LOG) {
handle_forward(packet);
} else if (packet['packet_type'] == PACKET_TYPE_ALIVE) {
debug_log('Agent alive: ', packet);
handle_alive(socket, packet);
Expand Down Expand Up @@ -483,6 +486,18 @@ app.post('/submit_task', upload.any(), function(req, res) {
}
});

app.post('/log_error', function(req, res) {
const { USED_AGENT_ID: agent_id, ...provider_data } = req.body
let log_packet = {
packet_type: PACKET_TYPE_ERROR_LOG,
sender_id: agent_id,
receiver_id: SYSTEM_SOCKET_ID,
data: provider_data,
};
_send_message(mephisto_socket, log_packet);
res.json({status: 'Error log sent!'})
});

// Quick status check for this server
app.get('/is_alive', function(req, res) {
res.json({status: 'Alive!'});
Expand Down
11 changes: 10 additions & 1 deletion packages/mephisto-task/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,10 @@ import {
isMobile,
getInitTaskData,
postCompleteTask,
postErrorLog,
postCompleteOnboarding,
getBlockedExplanation,
ErrorBoundary,
} from "./utils";

export * from "./MephistoContext";
Expand Down Expand Up @@ -68,6 +70,12 @@ const useMephistoTask = function () {
[state.agentId]
);

const handleFatalError = React.useCallback(
(data) => {
postErrorLog(state.agentId, data)
},
[state.agentId]);

function handleIncomingTaskConfig(taskConfig) {
if (taskConfig.block_mobile && isMobile()) {
setState({ blockedReason: "no_mobile" });
Expand Down Expand Up @@ -113,7 +121,8 @@ const useMephistoTask = function () {
blockedExplanation:
state.blockedReason && getBlockedExplanation(state.blockedReason),
handleSubmit,
handleFatalError,
};
};

export { useMephistoTask };
export { useMephistoTask , ErrorBoundary};
48 changes: 47 additions & 1 deletion packages/mephisto-task/src/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,11 @@
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

import React from "react";
import Bowser from "bowser";
const axios = require("axios");


/*
The following global methods are to be specified in wrap_crowd_source.js
They are sideloaded and exposed as global import during the build process:
Expand Down Expand Up @@ -132,6 +133,16 @@ export function postCompleteTask(agent_id, complete_data) {
});
}

export function postErrorLog(agent_id, complete_data) {
return postData("/log_error", {
USED_AGENT_ID: agent_id,
final_data: complete_data,
})
.then(function (data) {
console.log("Error log sent to server");
});
salelkafrawy marked this conversation as resolved.
Show resolved Hide resolved
}

export function getBlockedExplanation(reason) {
const explanations = {
no_mobile:
Expand All @@ -150,3 +161,38 @@ export function getBlockedExplanation(reason) {
return `Sorry, you are not able to work on this task. (code: ${reason})`;
}
}

export class ErrorBoundary extends React.Component {

state = { error: null, errorInfo: null };

componentDidCatch(error, errorInfo) {
// Catch errors in any components below and re-render with error message
this.setState({
error: error,
errorInfo: errorInfo
})
// alert Mephisto worker of a component error
alert("Error from the frontend occurred: " + error)
// pass the error and errorInfo to the backend through /submit_task endpoint
this.props.handleError({error:error.message, errorInfo:errorInfo})
}

render() {
if (this.state.errorInfo) {
// Error path
return (
<div>
<h2>Something went wrong.</h2>
<details style={{ whiteSpace: 'pre-wrap' }}>
{this.state.error && this.state.error.toString()}
<br />
{this.state.errorInfo.componentStack}
</details>
</div>
);
}
// Normally, just render children
return this.props.children;
}
}