[Lens] Visualization validation and better error messages #81439

dej611 · 2020-10-22T08:12:40Z

Summary

This PR is a partial take on the #80127, focused only at the visualization level, providing a better error message to the user in specific scenarios. A new PR will address also the problem at the datasource level next.
The code in this PR covers only the visualization expression building and evaluation phase.

✨ all visualization have now a getErrorMessage method
✨ some visualizations are showing a more accurate error message depending on the context, giving user's guidance on how to solve it
- if the same error happens on multiple levels, these are batched and shown in a single message
- there's a priority order for error message, so only one message per type is shown at the time to guide the user to a valid configuration
✨ initial validation logic bootstrapped at visualization level
✅ add basic testing for the visualization validation logic

XY Visualization

Datatable

Other chart types

There's currently no way to "break" other visualization types (pie and metric at the moment).

Building phase and errors

During a visualization building phase it's possible to fall into incomplete states, where no errors are shown. For an example the right hand side panel error message on the Y-axis dimension:

It would be nice to have a distinct state for this "incomplete" state from the "invalid" state, where a better message (rather than simply an error) is shown to the user.

Checklist

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Unit or functional tests were updated or added to match the most common scenarios

gchaps · 2020-10-22T22:26:13Z

As users can get details of this error in the panel on the right, how about removing "Show details of error" and going with text like this:

Missing X-axis
Layer 2 requires a field for the X-axis.

Missing X-axis
Layers 1,3 require fields for the X-axis.

Missing metric
Add a field in the Metrics panel.

@mdefazio Your thoughts?

mdefazio · 2020-10-26T12:45:46Z

As users can get details of this error in the panel on the right, how about removing "Show details of error" and going with text like this:

Missing X-axis
Layer 2 requires a field for the X-axis.

Missing X-axis
Layers 1,3 require fields for the X-axis.

Missing metric
Add a field in the Metrics panel.

@mdefazio Your thoughts?

I am in favor of removing the extra click! Am I understanding this correctly that there would also be 'lower priority' errors that aren't shown in the main portion, but would appear on the right? If so, is another line like '+2 more errors' also needed?

dej611 · 2020-10-26T13:24:43Z

Am I understanding this correctly that there would also be 'lower priority' errors that aren't shown in the main portion, but would appear on the right?

We could collect most of the errors on the right hand side in the main portion.

If so, is another line like '+2 more errors' also needed?

It could be an idea. I'll give it a try.

wylieconlon

Tested the behavior and did some code review, and I have two main issues with the current behavior:

The new messages don't appear before running expressions. I was expecting that if you get into an invalid configuration, we would prevent the expression from running while showing help text. This means that we never get errors for XY charts.
The rendering of these looks way more like an error than a helpful piece of reminder text. I would prefer having two separate designs, one for "something bad" and one for "helpful reminder".

wylieconlon · 2020-10-26T21:27:17Z

...ck/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.tsx

@@ -372,7 +388,7 @@ export const InnerVisualizationWrapper = ({
                    })}
                  </EuiButtonEmpty>

-                  {localState.expandError ? visibleErrorMessage : null}
+                  {localState.expandError ? longMessage || visibleErrorMessage : null}


I don't like the way this message is presented for anything but error messages. I would strongly prefer that we split this into 3 states, not 2:

Visualization warning with all messages visible, but not a "scary" message

Error state with warning triangle
2b. Error state with expanded details

wylieconlon · 2020-10-26T21:28:41Z

x-pack/plugins/lens/public/metric_visualization/visualization.tsx

+
+  getErrorMessage(state, frame) {
+    // Is it possible to break it?
+    return undefined;


I'm fine with requiring the function, but why return undefined;? What about no return statement?

The type system has a distinction between void and undefined values:

Type 'void' is not assignable to type '{ shortMessage: string; longMessage: string; } | undefined'.

wylieconlon · 2020-10-26T21:42:34Z

x-pack/plugins/lens/public/xy_visualization/visualization.test.ts

+          createMockFramePublicAPI()
+        )
+      ).not.toBeDefined();
+    });


I know you were talking about this with @timroes, but I would like to get a summary of the decision about this one. I can imagine several ways of solving it, including putting it off for another PR.

I had an idea about, maybe it can be implemented in here. @timroes opened the #81789 issue about meanwhile.

wylieconlon · 2020-10-26T21:44:52Z

x-pack/plugins/lens/public/xy_visualization/visualization.test.ts

+        shortMessage: 'Some layers are missing the X dimension',
+        longMessage: 'Layers 2, 3 have no dimension field set for the X axis',
+      });
+    });


I wasn't able to reproduce this error

wylieconlon · 2020-10-26T21:45:03Z

x-pack/plugins/lens/public/xy_visualization/visualization.test.ts

+      ).toEqual({
+        shortMessage: 'Some layers are missing the Y dimension',
+        longMessage: 'Layer 1 has no dimension field set for the Y axis',
+      });


I wasn't able to reproduce this error

flash1293 · 2020-10-27T10:40:30Z

Discussed this with @dej611 and to keep this PR focus, we concluded on this approach:

User changes something about configuration (a.k.a there's a state update)
Call getErrorMessages of datasource
Call getErrorMessages of visualization
If one of them returns error messages, show them in the workspace (example: inconsistent configuration error, fixable by the user by changing configuration)
If there are no error messages, build the expression
If building fails for some reason, show the exception error message in the workspace (example: buggy datasource/visualization, likely not fixable by the user, open an issue)
If building expression works, execute expression
If execution fails, show exception error message in workspace (example: runtime error, problem outside of Lens - index pattern issues, network issues, permission issues, ...)

As there are open questions about how missing dimension should be treated, let's start by just handling the case of broken field references in indexpattern datasource operations due to indexpattern updates. This can be done using this helper:

kibana/x-pack/plugins/lens/public/indexpattern_datasource/utils.ts

Line 46 in 8d7f2d0

export function hasInvalidReference(state: IndexPatternPrivateState) {

For visualizations, let's roll out usage of this new API in a separate PR

dej611 · 2020-10-27T17:57:50Z

Improved validation based on some of the feedback:

✨ getErrorMessage = > getErrorMessages with additional + X errors as suggested by @mdefazio

✨ New basic Datasource.getErrorMessages for invalid references as suggested by @flash1293

✨ Run the validations before building the expression as suggested by @wylieconlon
🐛 Y Axis only is possible as long as other layers do not go in conflict as suggested by @timroes in [Lens] Don't show x-dimension required when not required #81789

…lay-error-workspace

gchaps

I think we can reduce the text. Here is my suggestion.

First example

Invalid configuration
Layer 1 requires a field for the X-axis.
+1 error

"+1 error" might be easier to parse on a separate line

Singe invalid reference

Invalid configuration
Layer 1 has an invalid reference.

Multiple invalid references

Invalid configuration
Layer 1 has invalid references.

…lay-error-workspace

dej611 · 2020-10-28T12:01:06Z

Updated the layout in case of more errors:

mdefazio · 2020-10-28T13:34:05Z

Updated the layout in case of more errors:

Can we center the '+1 more' like we have the other messages?

flash1293 · 2020-11-03T08:10:10Z

@elasticmachine merge upstream

flash1293 · 2020-11-03T08:13:59Z

x-pack/plugins/lens/public/xy_visualization/to_expression.ts

@@ -26,8 +26,8 @@ export const toExpression = (
  state.layers.forEach((layer) => {
    metadata[layer.layerId] = {};
    const datasource = datasourceLayers[layer.layerId];
-    datasource.getTableSpec().forEach((column) => {
-      const operation = datasourceLayers[layer.layerId].getOperationForColumnId(column.columnId);
+    datasource?.getTableSpec().forEach((column) => {


Why are these changes necessary? I see how they are fixing the functional test failure, but it's very concerning this is necessary. toExpression should never be called without consistent datasource layers. I can't figure out what change in your PR caused that to happen.

flash1293

Please check the comment I left about the hook dependencies.

This is what I did:

Reverted changes in x-pack/plugins/lens/public/xy_visualization/to_expression.ts
Follow instructions of failing functional test in

kibana/x-pack/test/functional/apps/lens/smokescreen.ts

Line 174 in bf73bcf

it('should transition from a multi-layer stacked bar to donut chart using suggestions', async () => {

("should transition from a multi-layer stacked bar to donut chart using suggestions")
Lens breaks
Remove frame from the dependency list of the suggestion useMemo hook
Follow instructions of failing functional test again
Switch works fine

It seems like we have some issue in there in how the UI updates (and an inconsistent state during switching) but let's move the fix for that to another PR to get these changes in.

flash1293 · 2020-11-03T08:16:09Z

x-pack/plugins/lens/public/editor_frame_service/editor_frame/suggestion_panel.tsx

+        const suggestionVisualization = visualizationMap[visualizationId];
+        // filter out visualizations with errors
+        return (
+          suggestionVisualization.getErrorMessages(suggestionVisualizationState, frame) == null


This is not checking whether the datasource contains errors - we should use the same logic here as for the workspace panel.

The check is below the new suggestion creation.
It may make sense to exit earlier one if that fails before producing new suggestions: I'll push an update.

flash1293 · 2020-11-03T08:18:34Z

x-pack/plugins/lens/public/editor_frame_service/editor_frame/suggestion_panel.tsx

@@ -202,14 +232,19 @@ export function SuggestionPanel({
          )
        : undefined;

-    return { suggestions: newSuggestions, currentStateExpression: newStateExpression };
-    // eslint-disable-next-line react-hooks/exhaustive-deps


This was kept on purpose - otherwise the suggestions are recalculated too often because the frame object gets recreated on each render. This hook should not depend on the frame object. Maybe this is causing the issue with toExpression I mentioned in the other comment.

flash1293 · 2020-11-03T08:32:07Z

I tracked the error down - it's happening because the suggestion panel is re-rendering with the datasource switch already applied but the visualization state update still pending. This causes the xy visualization trying to render the two layer visualization state based on the already one layered datasource state (which crashes). I will open a separate issue for this, for this PR, please remove frame from the useMemo dependency list and revert the to_expression changes to keep the current approach working.

flash1293 · 2020-11-03T09:50:22Z

...ck/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.tsx

+    activeDatasourceId,
+    datasourceMap,
+    datasourceStates,
+    framePublicAPI,


SHouldn't be part of dependency list

flash1293 · 2020-11-03T09:50:55Z

x-pack/plugins/lens/public/editor_frame_service/editor_frame/suggestion_panel.tsx

    const newSuggestions = getSuggestions({
      datasourceMap,
      datasourceStates: currentDatasourceStates,
      visualizationMap,
      activeVisualizationId: currentVisualizationId,
      visualizationState: currentVisualizationState,
    })
+      .filter(({ visualizationId, visualizationState: suggestionVisualizationState }) => {


New order:

filter out hidden suggestions

filter out invalid suggestions

slice

map preview expression

…gle place

flash1293

Tested and this LGTM, thanks a lot for all the iterations.

As we were concerned about performance, I tested this PR with a metricbeat indexpattern (4k fields). It's a little slow, but the validation is not a terrible contributor - in a simple drop event, it's only contributing 0.4ms, while getOperationSupportMatrix takes 50ms. If we want to optimize, we should probably start there.

…lay-error-workspace

flash1293 · 2020-11-04T14:06:01Z

x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.tsx

@@ -335,6 +341,81 @@ export function getIndexPatternDatasource({
    },
    getDatasourceSuggestionsFromCurrentState,
    getDatasourceSuggestionsForVisualizeField,
+
+    getErrorMessages(state) {
+      if (state) {


minor nit: This function can be unnested by returning early making it a little easier to follow:

if (!state) return; const invalidLayers = getInvalidReferences(state); if (invaludLayers.length === 0) return; ...

kibanamachine · 2020-11-04T16:04:34Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 8ccd1d4

Metrics [docs]

async chunks size

id	before	after	diff
`lens`	1020.4KB	1.0MB	+12.5KB

History

💚 Build #85782 succeeded 11caf4a
💔 Build #85771 failed cdb7054
💚 Build #85476 succeeded 37dca5a
💔 Build #85055 failed 4b3c36a
💔 Build #84882 failed 5e265ae

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

wylieconlon

LGTM!

wylieconlon · 2020-11-04T17:23:58Z

x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.test.ts

+      });
+    });
+
+    it('should detect and batch missing references in a layer', () => {


I was able to test this, and also validated that if I didn't refresh the index pattern we would get a request error.

* master: (127 commits) [ILM] Fix breadcrumbs (elastic#82594) [UX]Swap env filter with percentile (elastic#82246) Add platform's missing READMEs (elastic#82268) [Discover] Adding uiMetric to track Visualize link click (elastic#82344) [Search] Add used index pattern name to the search agg error field (elastic#82604) improve client-side SO client get pooling (elastic#82603) [Security Solution] Unskips Overview tests (elastic#82459) Embeddables/migrations (elastic#82296) [Enterprise Search] Refactor product server route registrations to their own files/folders (elastic#82663) Moving reinstall function outside of promise.all (elastic#82672) Load choropleth layer correctly (elastic#82628) Master backport elastic#81233 (elastic#82642) [Fleet] Allow snake cased Kibana assets (elastic#77515) Reduce saved objects authorization checks (elastic#82204) [data.search] Add request handler context and asScoped pattern (elastic#80775) [ML] Fixes formatting of fields in index data visualizer (elastic#82593) Usage collector readme (elastic#82548) [Lens] Visualization validation and better error messages (elastic#81439) [ML] Add annotation markers to time series brush area to indicate annotations exist outside of selected range (elastic#81490) chore(NA): install microdnf in UBI docker build only (elastic#82611) ...

) Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

…) (#82811) Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

dej611 added 5 commits October 20, 2020 18:47

✨ First pass with visualization validation + error messages

1bed7c0

🔥 Remove indexpattern error handling for now

bb93701

🏷️ Fix type issues

0aea6c5

✅ Add getErrorMessage test for data table

d389d85

✅ Add tests for pie and metric error messages

4c48b23

dej611 added release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 Feature:Lens v7.11.0 labels Oct 22, 2020

dej611 requested a review from gchaps October 22, 2020 08:12

dej611 added 3 commits October 22, 2020 12:36

🌐 Fix i18n checks issues

91a700a

🐛 Fix last issue

b1d71dd

✅ Add more tests for the XY visualization validation code

be2312d

wylieconlon reviewed Oct 26, 2020

View reviewed changes

dej611 added 2 commits October 27, 2020 18:49

👌 Included all feedback from first review

c64603a

✏️ Off by one message

8e77401

dej611 added 2 commits October 27, 2020 18:59

Merge remote-tracking branch 'upstream/master' into feature/lens/disp…

64a22ef

…lay-error-workspace

🌐 Fix i18n duplicate id

7089b2f

gchaps reviewed Oct 27, 2020

View reviewed changes

dej611 added 3 commits October 28, 2020 10:31

🌐 Fix last i18n issue

86cc058

Merge remote-tracking branch 'upstream/master' into feature/lens/disp…

4e0c179

…lay-error-workspace

🐛 Fixed a hook reflow issue

6672302

Merge branch 'master' into feature/lens/display-error-workspace

33b71fa

flash1293 reviewed Nov 3, 2020

View reviewed changes

flash1293 requested changes Nov 3, 2020

View reviewed changes

🐛 Fix suggestions switching bug

0a0f5d3

flash1293 reviewed Nov 3, 2020

View reviewed changes

dej611 added 2 commits November 3, 2020 11:52

:refactor: Add more validation + refactored validation logic in a sin…

37dca5a

…gle place

✏️ Add note about lint hooks disable rule

fa32b19

flash1293 approved these changes Nov 3, 2020

View reviewed changes

dej611 added 3 commits November 4, 2020 12:22

Merge remote-tracking branch 'upstream/master' into feature/lens/disp…

1a440a9

…lay-error-workspace

🚨 Fix linting issue

cdb7054

✅ Align tests with new API

11caf4a

flash1293 reviewed Nov 4, 2020

View reviewed changes

👌 Early exists added

8ccd1d4

dej611 mentioned this pull request Nov 4, 2020

[Lens] Extend Datasource props validation with VisualizationGroups #82607

Merged

wylieconlon approved these changes Nov 4, 2020

View reviewed changes

This was linked to issues Nov 4, 2020

[Lens] Allow datasource and visualization to display error in workspace #80127

Closed

[Lens] Don't show x-dimension required when not required #81789

Closed

dej611 merged commit 53ea090 into elastic:master Nov 4, 2020

dej611 deleted the feature/lens/display-error-workspace branch November 4, 2020 17:28

dej611 mentioned this pull request Nov 6, 2020

[7.x] [Lens] Visualization validation and better error messages (#81439) #82811

Merged

dej611 added a commit to dej611/kibana that referenced this pull request Nov 6, 2020

[Lens] Visualization validation and better error messages (elastic#81439

9a8ae03

) Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

dej611 mentioned this pull request Nov 6, 2020

[Lens] Performance refactoring for indexpattern fast lookup and Operation support matrix computation #82829

Merged

1 task

This was referenced Nov 17, 2020

[Lens] An error page is displayed when you manually erase out the fields on the X/Y axis which could be improvized. #83466

Closed

[Lens] Default timefield as a missing field is causing an esaggs error #84058

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Lens] Visualization validation and better error messages #81439

[Lens] Visualization validation and better error messages #81439

dej611 commented Oct 22, 2020

gchaps commented Oct 22, 2020

mdefazio commented Oct 26, 2020

dej611 commented Oct 26, 2020

wylieconlon left a comment

wylieconlon Oct 26, 2020

wylieconlon Oct 26, 2020

dej611 Oct 27, 2020

wylieconlon Oct 26, 2020

dej611 Oct 27, 2020

wylieconlon Oct 26, 2020

wylieconlon Oct 26, 2020

flash1293 commented Oct 27, 2020 •

edited

Loading

dej611 commented Oct 27, 2020

gchaps left a comment

dej611 commented Oct 28, 2020

mdefazio commented Oct 28, 2020

flash1293 commented Nov 3, 2020

flash1293 Nov 3, 2020

flash1293 left a comment

flash1293 Nov 3, 2020 •

edited

Loading

dej611 Nov 3, 2020

flash1293 Nov 3, 2020

flash1293 commented Nov 3, 2020 •

edited

Loading

flash1293 Nov 3, 2020

flash1293 Nov 3, 2020

flash1293 left a comment

flash1293 Nov 4, 2020 •

edited

Loading

kibanamachine commented Nov 4, 2020

wylieconlon left a comment

wylieconlon Nov 4, 2020

[Lens] Visualization validation and better error messages #81439

[Lens] Visualization validation and better error messages #81439

Conversation

dej611 commented Oct 22, 2020

Summary

XY Visualization

Datatable

Other chart types

Building phase and errors

Checklist

gchaps commented Oct 22, 2020

mdefazio commented Oct 26, 2020

dej611 commented Oct 26, 2020

wylieconlon 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

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

flash1293 commented Oct 27, 2020 • edited Loading

dej611 commented Oct 27, 2020

gchaps left a comment

Choose a reason for hiding this comment

dej611 commented Oct 28, 2020

mdefazio commented Oct 28, 2020

flash1293 commented Nov 3, 2020

Choose a reason for hiding this comment

flash1293 left a comment

Choose a reason for hiding this comment

flash1293 Nov 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flash1293 commented Nov 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flash1293 left a comment

Choose a reason for hiding this comment

flash1293 Nov 4, 2020 • edited Loading

Choose a reason for hiding this comment

kibanamachine commented Nov 4, 2020

💚 Build Succeeded

Metrics [docs]

async chunks size

History

wylieconlon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flash1293 commented Oct 27, 2020 •

edited

Loading

flash1293 Nov 3, 2020 •

edited

Loading

flash1293 commented Nov 3, 2020 •

edited

Loading

flash1293 Nov 4, 2020 •

edited

Loading