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

🪵 Add more logs and error boundaries in Label and Dashboard #1100

Merged

Conversation

JGreenlee
Copy link
Collaborator

@JGreenlee JGreenlee commented Nov 9, 2023

In effort to more effectively debug e-mission/e-mission-docs#1025 and e-mission/e-mission-docs#986 (comment):

  • Adds many logDebug statements throughout with nicer formatting
  • Wraps top-level calls in try / catch blocks, handled with displayError
  • Wraps each tab of the app in an ErrorBoundary. This means if one of them has an error on rendering (ie an error in the JSX / the React component itself, not one of our JS functions), that tab will be blank but the other tabs will not be affected.

-added several try/catches to cover all top-level calls
-added logDebug statements throughout, using `` for multi-line so they are not ugly
-replaced any uses of the old Logger service
When metrics are fetched and stored as state, we can have more log statements and a try/catch.
Comment on lines +59 to +61
label: withErrorBoundary(LabelTab),
metrics: withErrorBoundary(MetricsTab),
control: withErrorBoundary(ProfileSettings),
Copy link
Contributor

Choose a reason for hiding this comment

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

very cool!

if (mode == 'append') {
setTimelineMap(new Map([...timelineMap, ...readTimelineMap]));
} else if (mode == 'prepend') {
setTimelineMap(new Map([...readTimelineMap, ...timelineMap]));
} else if (mode == 'replace') {
setTimelineMap(readTimelineMap);
} else {
return console.error('Unknown insertion mode ' + mode);
return displayErrorMsg('Unknown insertion mode ' + mode);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: as I was looking through the logger code, I noticed that displayError displays the full stacktrace, but displayErrorMsg does not. Is there a reason why we would not want to include the backtrace?

Copy link
Contributor

Choose a reason for hiding this comment

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

@JGreenlee I'm moving this to the "cleanup" column after the merge so that we can revisit how the logger should display errors.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

displayError accepts an Error object as a parameter. displayErrorMsg is used when there is no error and we just want to popup with some text.

Maybe in cases where there is no error, we should be throwing one ourselves and then print out that stacktrace?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe in cases where there is no error, we should be throwing one ourselves and then print out that stacktrace?

If we can do so easily, I think that would be helpful. Otherwise, we have to work backwards from the message to the code, and don't really have the full trace

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The stack traces are not very useful anyway because all the JS is bundled.

Maybe we could do something like https://stackoverflow.com/questions/58448000/how-to-get-a-readable-stack-trace-for-a-webpack-production-environment-for-a-min

@shankari shankari merged commit edafd98 into e-mission:service_rewrite_2023 Nov 9, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Address code reviews with "future"
Development

Successfully merging this pull request may close these issues.

2 participants