[Lens] Remove Over time suggestions for numeric intervals #78442

dej611 · 2020-09-24T15:52:38Z

Summary

Current state

After some discussion there wasn't a clear consensus on the value of having a Over time suggestion for numeric histograms type of charts.

Therefore the current state is to detect this scenario and avoid to push a Over time suggestion for it.

Previous state

The aim of this PR is to improve the suggestion system for histograms not date-based, providing better Over time suggestions.
This is the continuation of #76121 .

The current state of suggestions for the Over time option is to add a new interval date dimension to the break down section of the panel, keeping other dimensions on their place.

This PR makes a distinction between interval type operations, making the suggestion system aware of the data type, pushing for a swap of the x and splitBy dimensions to propose a date histogram based chart:

This is a first attempt with it: so far I couldn't find another route than the dataType to detect the different type of intervals.

Checklist

Unit or functional tests were updated or added to match the most common scenarios

wylieconlon · 2020-09-24T19:41:51Z

@dej611 I'm interested in coming up with a solution that doesn't involve the XY chart at all. Could you take a look at the logic in indexpattern_suggestions.ts? Maybe you can avoid adding the date histogram when there is already a number histogram.

…rvals-suggestions

dej611 · 2020-10-08T12:57:22Z

Reworked the logic to tune the data table at indexpattern_suggestions.ts level. It's probably not super clean (it relies on some combination of parameters not documented) but it works.

…rvals-suggestions

dej611 · 2020-10-08T15:43:21Z

@elasticmachine merge upstream

wylieconlon · 2020-10-08T22:13:30Z

@dej611 This is looking closer, but it seems like this is still generating suggestions to insert the time field in cases where inserting the time field is not helpful. What do you think about my previous comment:

Maybe you can avoid adding the date histogram when there is already a number histogram?

…rvals-suggestions

dej611 · 2020-10-13T12:51:59Z

Talked offline, not sure to find a consensus on what it should show in this scenario.
So as for now I'll just remove the Over time suggestion in this particular scenario: note it applies only to the basic Interval mode, not the Custom intervals mode.

…istogram

…1/kibana into feature/lens/intervals-suggestions

dej611 · 2020-10-13T16:13:12Z

@elasticmachine merge upstream

wylieconlon

I tested this out locally and it did what I expected, but also seems to do other unrelated things. Can you clarify the parts that are directly related here?

wylieconlon · 2020-10-13T19:28:35Z

x-pack/plugins/lens/public/indexpattern_datasource/indexpattern_suggestions.ts

@@ -455,6 +455,10 @@ export function getDatasourceSuggestionsFromCurrentState(
          ({ name }) => name === indexPattern.timeFieldName
        );

+        const hasNumericDimension =
+          buckets.length === 1 &&
+          buckets.some((columnId) => layer.columns[columnId].dataType === 'number');


Instead of looking for numeric operations, which can include the number range aggregation too, could you filter for any existing column.scale === 'interval'? This should allow the timestamp to be added when it's a "Custom ranges" option, which is currently not added.

wylieconlon · 2020-10-13T19:28:48Z

x-pack/plugins/lens/public/indexpattern_datasource/indexpattern_suggestions.test.tsx

@@ -1381,6 +1381,51 @@ describe('IndexPattern Data Source suggestions', () => {
      );
    });

+    it('does not create an over time suggestion if tables with numeric buckets with time dimension', async () => {


Can you also add a test when the aggregation is the "Custom ranges" mode?

wylieconlon · 2020-10-13T19:31:15Z

x-pack/plugins/lens/public/indexpattern_datasource/indexpattern_suggestions.ts

@@ -665,7 +677,7 @@ function createSuggestionWithDefaultDateHistogram(
    label: i18n.translate('xpack.lens.indexpattern.suggestions.overTimeLabel', {
      defaultMessage: 'Over time',
    }),
-    changeType: 'extended',
+    changeType: shouldSwitchTimeWithBuckets ? 'reorder' : 'extended',


This suggestion should always be extended because it should always add a new column. Calling it a reorder will affect how the XY charts use the suggestion, which I don't think is intentional.

You're right. All the code in this function needs to be removed as it was shaped to make it work the initial way.

wylieconlon · 2020-10-13T19:38:43Z

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

@@ -117,8 +117,6 @@ function flipSeriesType(seriesType: SeriesType) {
      return 'bar_stacked';
    case 'bar':
      return 'bar_horizontal';
-    case 'bar_horizontal_stacked':
-      return 'bar_stacked';


This doesn't look right to me, why was this removed?

It's a duplicate. At line 116 there's exactly the same case.

wylieconlon · 2020-10-13T19:38:59Z

x-pack/plugins/lens/public/indexpattern_datasource/indexpattern_suggestions.ts

+    columnOrder: shouldSwitchTimeWithBuckets
+      ? // in this case the order is very important to assign the split accessor!
+        [newId, ...buckets, ...metrics]
+      : [...buckets, newId, ...metrics],


Is this necessary? If so, can you please write a test for it?

…1/kibana into feature/lens/intervals-suggestions

dej611 · 2020-10-16T08:38:00Z

@elasticmachine merge upstream

elasticmachine · 2020-10-16T10:47:47Z

Pinging @elastic/kibana-app (Team:KibanaApp)

wylieconlon

Tested and LGTM!

dej611 · 2020-10-19T09:48:03Z

@elasticmachine merge upstream

kibanamachine · 2020-10-19T11:21:48Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 4909c52

Metrics [docs]

async chunks size

id	before	after	diff
`lens`	1.0MB	1.0MB	+104.0B

History

💚 Build #82103 succeeded 6224b9d
💔 Build #81931 failed ae5406f
💚 Build #81552 succeeded eefab3b
💚 Build #81371 succeeded 73ead33
💚 Build #80788 succeeded ef21c11

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

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

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

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

dej611 added 2 commits September 24, 2020 15:03

✨ Make xy suggestor data type aware again

c70ca5e

✅ Add test for numeric histogram extended suggestion

405c05d

dej611 added Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 v7.10.0 labels Sep 24, 2020

dej611 requested a review from wylieconlon September 24, 2020 15:52

dej611 self-assigned this Sep 24, 2020

dej611 added 5 commits October 6, 2020 12:53

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

aa4aad1

…rvals-suggestions

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

9f33112

…rvals-suggestions

♻️ Restore previous state

b328ff8

✨ Tuning for numeric histogram

9265253

:white_mark_check: Add test for numeric histogram

3eb6809

dej611 added 2 commits October 8, 2020 15:29

🔥 Remove some development cruft

49f7458

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

24a2d61

…rvals-suggestions

Merge branch 'master' into feature/lens/intervals-suggestions

ef21c11

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

7614d1d

…rvals-suggestions

dej611 added 2 commits October 13, 2020 14:52

👌 Revisit implementation to remove over time suggestion for numeric h…

988aeec

…istogram

Merge branch 'feature/lens/intervals-suggestions' of github.com:dej61…

6f1448a

…1/kibana into feature/lens/intervals-suggestions

dej611 changed the title ~~[Lens] Improve suggestion quality for numeric intervals~~ [Lens] Remove Over time suggestions for numeric intervals Oct 13, 2020

Merge branch 'master' into feature/lens/intervals-suggestions

73ead33

wylieconlon reviewed Oct 13, 2020

View reviewed changes

dej611 added 2 commits October 14, 2020 11:34

🔥 Remove code for previous behaviour

05f68da

Merge branch 'feature/lens/intervals-suggestions' of github.com:dej61…

eefab3b

…1/kibana into feature/lens/intervals-suggestions

dej611 added the Feature:Lens label Oct 14, 2020

✅ Add custom range test case + adds better comments

ae5406f

Merge branch 'master' into feature/lens/intervals-suggestions

6224b9d

dej611 marked this pull request as ready for review October 16, 2020 10:47

dej611 requested a review from a team October 16, 2020 10:47

dej611 added the release_note:fix label Oct 16, 2020

wylieconlon approved these changes Oct 16, 2020

View reviewed changes

Merge branch 'master' into feature/lens/intervals-suggestions

4909c52

dej611 added v7.11.0 and removed v7.10.0 labels Oct 19, 2020

dej611 merged commit 7b1b36e into elastic:master Oct 19, 2020

dej611 deleted the feature/lens/intervals-suggestions branch October 19, 2020 12:45

dej611 added v7.10 v7.10.0 and removed v7.10 labels Oct 20, 2020

This was referenced Oct 20, 2020

[7.x] [Lens] Remove Over time suggestions for numeric intervals (#78442) #81117

Merged

[7.10] [Lens] Remove Over time suggestions for numeric intervals (#78442) #81118

Merged

dej611 added a commit to dej611/kibana that referenced this pull request Oct 20, 2020

[Lens] Remove Over time suggestions for numeric intervals (elastic#78442

5667e25

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

dej611 added a commit to dej611/kibana that referenced this pull request Oct 20, 2020

[Lens] Remove Over time suggestions for numeric intervals (elastic#78442

d85f32e

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

wylieconlon added release_note:skip Skip the PR/issue when compiling release notes and removed release_note:fix labels Nov 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Lens] Remove Over time suggestions for numeric intervals #78442

[Lens] Remove Over time suggestions for numeric intervals #78442

dej611 commented Sep 24, 2020 •

edited

Loading

wylieconlon commented Sep 24, 2020

dej611 commented Oct 8, 2020 •

edited

Loading

dej611 commented Oct 8, 2020

wylieconlon commented Oct 8, 2020

dej611 commented Oct 13, 2020

dej611 commented Oct 13, 2020

wylieconlon left a comment

wylieconlon Oct 13, 2020

wylieconlon Oct 13, 2020

wylieconlon Oct 13, 2020

dej611 Oct 14, 2020

wylieconlon Oct 13, 2020

dej611 Oct 14, 2020

wylieconlon Oct 13, 2020

dej611 commented Oct 16, 2020

elasticmachine commented Oct 16, 2020

wylieconlon left a comment

dej611 commented Oct 19, 2020

kibanamachine commented Oct 19, 2020

[Lens] Remove Over time suggestions for numeric intervals #78442

[Lens] Remove Over time suggestions for numeric intervals #78442

Conversation

dej611 commented Sep 24, 2020 • edited Loading

Summary

Current state

Checklist

wylieconlon commented Sep 24, 2020

dej611 commented Oct 8, 2020 • edited Loading

dej611 commented Oct 8, 2020

wylieconlon commented Oct 8, 2020

dej611 commented Oct 13, 2020

dej611 commented Oct 13, 2020

wylieconlon left a comment

Choose a reason for hiding this comment

wylieconlon Oct 13, 2020

Choose a reason for hiding this comment

wylieconlon Oct 13, 2020

Choose a reason for hiding this comment

wylieconlon Oct 13, 2020

Choose a reason for hiding this comment

dej611 Oct 14, 2020

Choose a reason for hiding this comment

wylieconlon Oct 13, 2020

Choose a reason for hiding this comment

dej611 Oct 14, 2020

Choose a reason for hiding this comment

wylieconlon Oct 13, 2020

Choose a reason for hiding this comment

dej611 commented Oct 16, 2020

elasticmachine commented Oct 16, 2020

wylieconlon left a comment

Choose a reason for hiding this comment

dej611 commented Oct 19, 2020

kibanamachine commented Oct 19, 2020

💚 Build Succeeded

Metrics [docs]

async chunks size

History

dej611 commented Sep 24, 2020 •

edited

Loading

dej611 commented Oct 8, 2020 •

edited

Loading