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

Implement graceful error handling for trend charts #43090

Merged
merged 23 commits into from
May 28, 2024

Conversation

JesseSDevaney
Copy link
Contributor


Before

2024-05-23.14-56-03.mp4

After

2024-05-23.14-51-01.mp4

Desired Behavior/Acceptance Criteria

  • Trend charts no longer error when the latest value is null. computeTrend now finds the latest non-null data point and compares it with a the first non-null previous data point
  • Trend chart no longer throws an error that it does not handle. If computeTrend throws an error, displays a warning icon and error message detailing the error.
    • You can continue using the query builder (w/o needing to refresh): switching to new chart types, viewing the data as a table, etc.
  • Tests have been added to cover the above cases (compute.unit.spec.js, smartscalar.unit.spec.js, smartscalar-trend.cy.spec.js)

@JesseSDevaney JesseSDevaney added backport Automatically create PR on current release branch on merge .Team/DashViz Dashboard and Viz team labels May 23, 2024
@JesseSDevaney JesseSDevaney requested review from zbodi74, kulyk and a team May 23, 2024 19:08
@JesseSDevaney JesseSDevaney self-assigned this May 23, 2024
Copy link

github-actions bot commented May 23, 2024

Codenotify: Notifying subscribers in CODENOTIFY files for diff 5e29f43...facb385.

Notify File(s)
@alxnddr frontend/src/metabase/visualizations/components/Visualization/Visualization.jsx
frontend/src/metabase/visualizations/visualizations/SmartScalar/SmartScalar.jsx
frontend/src/metabase/visualizations/visualizations/SmartScalar/SmartScalar.unit.spec.js
frontend/src/metabase/visualizations/visualizations/SmartScalar/compute.js
frontend/src/metabase/visualizations/visualizations/SmartScalar/compute.unit.spec.js

Copy link

replay-io bot commented May 23, 2024

Status Complete ↗︎
Commit facb385
Results
2579 Passed

Copy link
Member

@alxnddr alxnddr left a comment

Choose a reason for hiding this comment

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

There was also a bit related to the inability to switch to other viz types if such error has happened. I think we should also address that even though it is not related to Trend viz. It is likely related to the logic in the Visualization.tsx component which has error local state

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an old, dead conditional. There can no longer be a null or undefined trend returned. Either computeTrend returns the actual trend, or throws an error.

Comment on lines +179 to +187
const latestRowIndex = _.findLastIndex(rows, row => {
const date = row[dimensionColIndex];
const value = row[metricColIndex];

return !isEmpty(value) && !isEmpty(date);
});
if (latestRowIndex === -1) {
throw Error("No rows contain a valid value.");
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the fix for the latest data point being null. If it is, we skip it and find the most recent non-null data point.

const date = rows[latestRowIndex][dimensionColIndex];
const value = rows[latestRowIndex][metricColIndex];
if (isEmpty(value) || isEmpty(date)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This conditional is no longer needed because of the fix above

Comment on lines +311 to +328
const previousRowIndex = _.findLastIndex(rows, (row, i) => {
if (i >= nextValueRowIndex) {
return false;
}

const date = row[dimensionColIndex];
const value = row[metricColIndex];

return !isEmpty(value) && !isEmpty(date);
});

// if no row exists with non-null date and non-null value
if (isEmpty(previousRow)) {
if (previousRowIndex === -1) {
return null;
}

const prevDate = previousRow[dimensionColIndex];
const prevValue = previousRow[metricColIndex];
const prevDate = rows[previousRowIndex][dimensionColIndex];
const prevValue = rows[previousRowIndex][metricColIndex];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored this to have the same structure as getCurrentMetricData does.

@@ -467,7 +471,7 @@ class Visualization extends PureComponent {
(replacementContent && (dashcard.size_y !== 1 || isMobile) && !isAction);

return (
<ErrorBoundary>
<ErrorBoundary onError={this.onRenderError}>
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 reason users could not switch between visualizations or to the table view was because this ErrorBoundary was catching the errors, but no function was provided to pass off that error information to the Visualization component. Because it was not passed off, ErrorBoundary held onto its local state of hasError and therefore would not render its children.

Comment on lines 323 to 326
if (error?.message) {
return this.setState({ error: error.message });
}

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 ErrorView component within Visualization.jsx expects the error to be a string. So we need to check if the error is a regular error object and only pass the string message.

@JesseSDevaney JesseSDevaney requested review from alxnddr and removed request for kulyk May 24, 2024 19:01
@JesseSDevaney
Copy link
Contributor Author

New After (trend chart)

2024-05-24.15-03-02.mp4
  • What an actual trend chart error would look like in app (dev mode react does not show errors if they hit ErrorBoundary):
    Screenshot from 2024-05-24 15-07-34

  • What the new static-viz error will look like with the added error message:
    Screenshot from 2024-05-24 15-04-19

How Visualization now handles errors that hit ErrorBoundary

2024-05-24.15-08-14.mp4

Copy link
Contributor Author

@JesseSDevaney JesseSDevaney May 24, 2024

Choose a reason for hiding this comment

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

A generic error component was added to this on top of the regular error component, because they are serving different needs.

The regular error component contains errors that we are expecting to be thrown in certain conditions, and because of that, we have a particular error message we are wanting to display.

The generic error component is for errors that we are not expecting to be thrown and therefore have no specific message to display. We should still allow the user the option to get diagnostic info in order to help them out in the event that unintended errors are thrown.

@JesseSDevaney JesseSDevaney merged commit 8770ea2 into master May 28, 2024
109 checks passed
@JesseSDevaney JesseSDevaney deleted the fix-42948/trend-charts-should-degrade-gracefully branch May 28, 2024 15:41
Copy link

@JesseSDevaney Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone?

github-actions bot pushed a commit that referenced this pull request May 28, 2024
* fix trend charts erroring on latest value being null

* refactor previousRowIndex calculation

* handle errors gracefully

* refactor error messages

* update computeTrend unit tests

* add error handling integration test

* add E2E test to check that errors are gracefully handled

* add integration test to enforce the fix

* fix static viz

* display more meaningful error on static-viz trend charts

* move error handling out of trend chart and fix error handling in Visualization.tsx

* update compute.unit.spec.js

* update SmartScalar.unit.spec.js

* use separate error handler for ErrorBoundary errors in Visualization.jsx

* update E2E test

* update error handling

* update compute.unit.spec.js

* fix react state change error in render function

* show generic error message for ErrorBoundary caught errors

* revert ErrorBoundary changes

* remove generic error border for visualizations
@JesseSDevaney
Copy link
Contributor Author

@metabase-bot backport release-x.49.x

Copy link

@JesseSDevaney something went wrong while creating a backport [Logs]

metabase-bot bot added a commit that referenced this pull request May 28, 2024
* fix trend charts erroring on latest value being null

* refactor previousRowIndex calculation

* handle errors gracefully

* refactor error messages

* update computeTrend unit tests

* add error handling integration test

* add E2E test to check that errors are gracefully handled

* add integration test to enforce the fix

* fix static viz

* display more meaningful error on static-viz trend charts

* move error handling out of trend chart and fix error handling in Visualization.tsx

* update compute.unit.spec.js

* update SmartScalar.unit.spec.js

* use separate error handler for ErrorBoundary errors in Visualization.jsx

* update E2E test

* update error handling

* update compute.unit.spec.js

* fix react state change error in render function

* show generic error message for ErrorBoundary caught errors

* revert ErrorBoundary changes

* remove generic error border for visualizations

Co-authored-by: Jesse Devaney <22608765+JesseSDevaney@users.noreply.github.com>
JesseSDevaney added a commit that referenced this pull request May 28, 2024
* fix trend charts erroring on latest value being null

* refactor previousRowIndex calculation

* handle errors gracefully

* refactor error messages

* update computeTrend unit tests

* add error handling integration test

* add E2E test to check that errors are gracefully handled

* add integration test to enforce the fix

* fix static viz

* display more meaningful error on static-viz trend charts

* move error handling out of trend chart and fix error handling in Visualization.tsx

* update compute.unit.spec.js

* update SmartScalar.unit.spec.js

* use separate error handler for ErrorBoundary errors in Visualization.jsx

* update E2E test

* update error handling

* update compute.unit.spec.js

* fix react state change error in render function

* show generic error message for ErrorBoundary caught errors

* revert ErrorBoundary changes

* remove generic error border for visualizations
rafpaf pushed a commit that referenced this pull request May 28, 2024
* fix trend charts erroring on latest value being null

* refactor previousRowIndex calculation

* handle errors gracefully

* refactor error messages

* update computeTrend unit tests

* add error handling integration test

* add E2E test to check that errors are gracefully handled

* add integration test to enforce the fix

* fix static viz

* display more meaningful error on static-viz trend charts

* move error handling out of trend chart and fix error handling in Visualization.tsx

* update compute.unit.spec.js

* update SmartScalar.unit.spec.js

* use separate error handler for ErrorBoundary errors in Visualization.jsx

* update E2E test

* update error handling

* update compute.unit.spec.js

* fix react state change error in render function

* show generic error message for ErrorBoundary caught errors

* revert ErrorBoundary changes

* remove generic error border for visualizations

Co-authored-by: Jesse Devaney <22608765+JesseSDevaney@users.noreply.github.com>
JesseSDevaney added a commit that referenced this pull request May 28, 2024
* fix trend charts erroring on latest value being null

* refactor previousRowIndex calculation

* handle errors gracefully

* refactor error messages

* update computeTrend unit tests

* add error handling integration test

* add E2E test to check that errors are gracefully handled

* add integration test to enforce the fix

* fix static viz

* display more meaningful error on static-viz trend charts

* move error handling out of trend chart and fix error handling in Visualization.tsx

* update compute.unit.spec.js

* update SmartScalar.unit.spec.js

* use separate error handler for ErrorBoundary errors in Visualization.jsx

* update E2E test

* update error handling

* update compute.unit.spec.js

* fix react state change error in render function

* show generic error message for ErrorBoundary caught errors

* revert ErrorBoundary changes

* remove generic error border for visualizations
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport Automatically create PR on current release branch on merge .Team/DashViz Dashboard and Viz team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Trend charts should break down gracefully on missing data
2 participants