Add `auto` interval to histogram AggConfig #76001

alexwizp · 2020-08-26T16:00:17Z

Closes: #75438

Summary

What was done it that PR:

Since the histogram aggConfig does already do a preflight request to determine min and max to later make sure we're not running over the histogram:maxBars setting, we can use that min/max information to easily allow an auto interval option.
Add support for intervals which less than 1. Based on Add auto interval to histogram AggConfig #75438 (comment)
Set Auto mode as a default for new aggregations in Vis Editor

Before:

After:

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

Closes: elastic#75438

alexwizp · 2020-08-26T19:33:24Z

@elasticmachine merge upstream

wylieconlon · 2020-08-26T19:40:27Z

I have reviewed the code and submitted a PR to you to change the layout and defaults to what makes sense to me: https://github.com/alexwizp/kibana/pull/4/files

The main changes are:

Auto is default
The max bars defaults to the max allowed by Kibana
When switching to a fixed interval, 0 is no longer valid

alexwizp · 2020-08-26T20:04:31Z

@wylieconlon I've reviewed your changes but if I don't understand the benefits of adding one more configuration property. I'm about useAuto. Date Histogram in default editor, TSVB and I think other places use interval field for that. I see probably only one reason, we can do it for saving value between switching isAuto. But for consistency I prefer to keep current approach.

elasticmachine · 2020-08-27T19:21:12Z

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

alexwizp · 2020-08-31T07:44:39Z

@elasticmachine merge upstream

src/plugins/data/common/search/aggs/buckets/_interval_options.ts

src/plugins/vis_default_editor/public/components/controls/number_interval.tsx

sulemanof

LGTM
Checked locally in Chrome, works fine

alexwizp · 2020-09-01T12:36:55Z

@elasticmachine merge upstream

stratoula

LGTM 🎉

wylieconlon

Please don't merge yet, the algorithm for selecting intervals doesn't look right.

src/plugins/data/common/search/aggs/buckets/lib/histogram_calculate_interval.test.ts

wylieconlon · 2020-09-01T15:40:19Z

src/plugins/data/common/search/aggs/buckets/lib/histogram_calculate_interval.ts

+  return interval;
+};
+
+const calculateAutoInterval = (


These functions are not behaving the way I expected, as described here: #75438 (comment)

I'll comment separately with tweaks.

wylieconlon · 2020-09-01T16:46:04Z

@elasticmachine merge upstream

wylieconlon · 2020-09-01T16:50:31Z

@alexwizp There are several differences between the algorithm you've implemented here and the one that I was expecting. The main one is that I'm optimizing to pick "simple" intervals which are multiples of 10, 2 or 5, while yours is not. So with your bucketing, CPU values are bucketed into intervals of 0.3:

While mine is choosing 0.2:

I'm also not having any special cases for integers vs floats, they should be treated identically from my perspective.

alexwizp#5

…ulate_interval.test.ts Co-authored-by: Wylie Conlon <[email protected]>

alexwizp · 2020-09-01T18:00:15Z

@wylieconlon Honestly I don't understand why we should pick "simple" intervals which are multiples of 10, 2 or 5. it's just one of the possible variant. If we are talking about an example above for me 0.3 and 0.2 both valid values for auto mode.

@lukeelmers could you please have a look?

lukeelmers

If we are talking about an example above for me 0.3 and 0.2 both valid values for auto mode.

It makes sense to me that we'd make some attempt at providing "pretty" intervals when somebody selects auto. IMO it's easier for someone to visually digest multiples of 10 (or 2, 5, 10) than another arbitrary interval.

Overall the code here LGTM & the math makes sense, however as @wylieconlon points out this isn't exactly the algorithm that he proposed in the issue... so if we want to deviate from that proposal I think we'd need to discuss further to make sure everyone is on the same page.

Otherwise, I'm comfortable with merging once this aligns with the original plan and we add a few more clarifying comments in the code.

src/plugins/data/common/search/aggs/buckets/histogram.ts

src/plugins/vis_default_editor/public/components/controls/max_bars.tsx

lukeelmers · 2020-09-01T18:50:54Z

src/plugins/data/common/search/aggs/buckets/lib/histogram_calculate_interval.ts

+  return roundInterval(diff / bars);
+};
+
+export const calculateHistogramInterval = ({


This is the type of code that can be really hard to maintain if you have to read through the math without any background and grok what's going on.

For future reference & to make maintenance easier, it would be great if we could add some detailed comments to each of the functions in this file, explaining the algorithm at a high level, what each function does, and why.

Good idea, I've added a textual description of @wylieconlon 's original algorithm 11f6ec8

Co-authored-by: Luke Elmers <[email protected]>

alexwizp · 2020-09-01T19:01:08Z

Algorithm of @wylieconlon works better for some cases. I think we should choose it. Thank you again.
PR was updated

lukeelmers

I'm good with these changes once we get a green build, but please wait for a final LGTM from Wylie before merging.

wylieconlon

Changes LGTM!

src/plugins/vis_default_editor/public/components/controls/max_bars.tsx

src/plugins/data/common/search/aggs/buckets/lib/histogram_calculate_interval.ts

…ulate_interval.ts Co-authored-by: Wylie Conlon <[email protected]>

kibanamachine · 2020-09-01T22:12:07Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 25e9aef

Build metrics

@kbn/optimizer bundle module count

id	value	diff	baseline
charts	269	+1	268
data	545	+1	544
regionMap	252	+1	251
tileMap	258	+1	257
visTypeMarkdown	259	+1	258
visTypeMetric	261	+1	260
visTypeTable	278	+1	277
visTypeTagcloud	262	+1	261
visTypeTimelion	281	+1	280
visTypeVega	312	+1	311
visTypeVislib	508	+1	507
visualize	316	+1	315
total		+12

async chunks size

id	value	diff	baseline
visualize	697.6KB	+3.9KB	693.8KB

page load bundle size

id	value	diff	baseline
charts	877.5KB	+3.9KB	873.6KB
data	1.4MB	+3.2KB	1.4MB
regionMap	836.3KB	+3.9KB	832.4KB
tileMap	847.9KB	+3.9KB	844.1KB
visTypeMarkdown	560.0KB	+3.9KB	556.1KB
visTypeMetric	584.7KB	+3.9KB	580.9KB
visTypeTable	608.8KB	+3.9KB	604.9KB
visTypeTagcloud	841.2KB	+3.9KB	837.3KB
visTypeTimelion	716.0KB	+3.9KB	712.1KB
visTypeVega	665.5KB	+3.9KB	661.7KB
visTypeVislib	1.3MB	+3.9KB	1.3MB
total		+41.8KB

oss distributable file count

id	value	diff	baseline
total	27307	+1	27306

distributable file count

id	value	diff	baseline
total	45470	+1	45469

History

💚 Build #71682 succeeded 11f6ec8
💚 Build #71679 succeeded 6101aa3
💚 Build #71651 succeeded 0b3259d
💚 Build #71627 succeeded 86d88f7
💚 Build #71540 succeeded 09c2eac

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

* Add `auto` interval to histogram AggConfig Closes: elastic#75438 * fix JEST * update UI * add tests * some changes * small changes * cleanup code * fix PR comment * fix PR comments * fix PR comment * Update src/plugins/data/common/search/aggs/buckets/lib/histogram_calculate_interval.test.ts Co-authored-by: Wylie Conlon <[email protected]> * Change algorithm for auto interval * Update src/plugins/data/common/search/aggs/buckets/histogram.ts Co-authored-by: Luke Elmers <[email protected]> * added some comments * Update src/plugins/data/common/search/aggs/buckets/lib/histogram_calculate_interval.ts Co-authored-by: Wylie Conlon <[email protected]> Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Wylie Conlon <[email protected]> Co-authored-by: Luke Elmers <[email protected]>

* Add `auto` interval to histogram AggConfig Closes: #75438 * fix JEST * update UI * add tests * some changes * small changes * cleanup code * fix PR comment * fix PR comments * fix PR comment * Update src/plugins/data/common/search/aggs/buckets/lib/histogram_calculate_interval.test.ts Co-authored-by: Wylie Conlon <[email protected]> * Change algorithm for auto interval * Update src/plugins/data/common/search/aggs/buckets/histogram.ts Co-authored-by: Luke Elmers <[email protected]> * added some comments * Update src/plugins/data/common/search/aggs/buckets/lib/histogram_calculate_interval.ts Co-authored-by: Wylie Conlon <[email protected]> Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Wylie Conlon <[email protected]> Co-authored-by: Luke Elmers <[email protected]> Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Wylie Conlon <[email protected]> Co-authored-by: Luke Elmers <[email protected]>

@ts-expect-error

* master: (223 commits) skip flaky suite (elastic#75724) [Reporting] Add functional test for Reports in non-default spaces (elastic#76053) [Enterprise Search] Fix various icons in dark mode (elastic#76430) skip flaky suite (elastic#76245) Add `auto` interval to histogram AggConfig (elastic#76001) [Resolver] generator uses setup_node_env (elastic#76422) [Ingest Manager] Support both zip & tar archives from Registry (elastic#76197) [Ingest Manager] Improve agent vs kibana version checks (elastic#76238) Manually building `KueryNode` for Fleet's routes (elastic#75693) remove dupe tinymath section (elastic#76093) Create APM issue template (elastic#76362) Delete unused file. (elastic#76386) [SECURITY_SOLUTION][ENDPOINT] Trusted Apps Create API (elastic#76178) [Detections Engine] Add Alert actions to the Timeline (elastic#73228) [Dashboard First] Library Notification (elastic#76122) [Maps] Add mvt support for ES doc sources (elastic#75698) Add setHeaderActionMenu API to AppMountParameters (elastic#75422) [ML] Remove "Are you sure" from data frame analytics jobs (elastic#76214) [yarn] remove typings-tester, use @ts-expect-error (elastic#76341) [Reporting/CSV] Do not fail the job if scroll ID can not be cleared (elastic#76014) ...

alexwizp added 2 commits August 26, 2020 18:56

Add auto interval to histogram AggConfig

4398245

Closes: elastic#75438

fix JEST

879f8d7

Merge branch 'master' into 75438

827c2a3

alexwizp added 3 commits August 27, 2020 13:45

update UI

7fcd6ae

Merge remote-tracking branch 'upstream/master' into 75438

e6e25fb

add tests

727599f

alexwizp self-assigned this Aug 27, 2020

alexwizp added the Team:Visualizations Visualization editors, elastic-charts and infrastructure label Aug 27, 2020

alexwizp added v7.10.0 v8.0.0 Feature:Vis Editor Visualization editor issues release_note:enhancement labels Aug 27, 2020

alexwizp added 2 commits August 27, 2020 17:32

some changes

4f13f06

Merge remote-tracking branch 'upstream/master' into 75438

45cfa92

alexwizp requested review from sulemanof, lukeelmers and wylieconlon August 27, 2020 14:34

small changes

86cdd45

dej611 mentioned this pull request Aug 27, 2020

[Lens] Combined histogram/range aggregation for numbers #76121

Merged

20 tasks

alexwizp marked this pull request as ready for review August 27, 2020 19:21

alexwizp requested a review from a team August 27, 2020 19:21

alexwizp requested a review from a team as a code owner August 27, 2020 19:21

cleanup code

70981b3

Merge branch 'master' into 75438

f70277c

sulemanof suggested changes Aug 31, 2020

View reviewed changes

fix PR comment

8004b28

sulemanof approved these changes Aug 31, 2020

View reviewed changes

Merge branch 'master' into 75438

09c2eac

stratoula approved these changes Sep 1, 2020

View reviewed changes

wylieconlon suggested changes Sep 1, 2020

View reviewed changes

Merge branch 'master' into 75438

86d88f7

Update src/plugins/data/common/search/aggs/buckets/lib/histogram_calc…

0b3259d

…ulate_interval.test.ts Co-authored-by: Wylie Conlon <[email protected]>

Change algorithm for auto interval

4b2ae64

lukeelmers reviewed Sep 1, 2020

View reviewed changes

Update src/plugins/data/common/search/aggs/buckets/histogram.ts

6101aa3

Co-authored-by: Luke Elmers <[email protected]>

added some comments

11f6ec8

lukeelmers approved these changes Sep 1, 2020

View reviewed changes

wylieconlon approved these changes Sep 1, 2020

View reviewed changes

src/plugins/vis_default_editor/public/components/controls/max_bars.tsx Show resolved Hide resolved

src/plugins/data/common/search/aggs/buckets/lib/histogram_calculate_interval.ts Outdated Show resolved Hide resolved

Update src/plugins/data/common/search/aggs/buckets/lib/histogram_calc…

25e9aef

…ulate_interval.ts Co-authored-by: Wylie Conlon <[email protected]>

alexwizp merged commit 7d82e27 into elastic:master Sep 1, 2020

alexwizp mentioned this pull request Sep 1, 2020

[7.x] Add auto interval to histogram AggConfig (#76001) #76449

Merged

wylieconlon mentioned this pull request Sep 1, 2020

[Lens] Create combined histogram/range aggregation for numbers #59424

Closed

timroes mentioned this pull request Sep 10, 2020

Visualize: Bad request when working with histogram aggregation #77023

Closed

wylieconlon mentioned this pull request Jun 4, 2021

Automatically generate ranges for range/histogram agg #3757

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `auto` interval to histogram AggConfig #76001

Add `auto` interval to histogram AggConfig #76001

alexwizp commented Aug 26, 2020 •

edited

Loading

alexwizp commented Aug 26, 2020

wylieconlon commented Aug 26, 2020

alexwizp commented Aug 26, 2020

elasticmachine commented Aug 27, 2020

alexwizp commented Aug 31, 2020

sulemanof left a comment

alexwizp commented Sep 1, 2020

stratoula left a comment

wylieconlon left a comment

wylieconlon Sep 1, 2020

wylieconlon commented Sep 1, 2020

wylieconlon commented Sep 1, 2020

alexwizp commented Sep 1, 2020

lukeelmers left a comment

lukeelmers Sep 1, 2020

alexwizp Sep 1, 2020

alexwizp commented Sep 1, 2020

lukeelmers left a comment

wylieconlon left a comment

kibanamachine commented Sep 1, 2020

Add auto interval to histogram AggConfig #76001

Add auto interval to histogram AggConfig #76001

Conversation

alexwizp commented Aug 26, 2020 • edited Loading

Summary

Checklist

For maintainers

alexwizp commented Aug 26, 2020

wylieconlon commented Aug 26, 2020

alexwizp commented Aug 26, 2020

elasticmachine commented Aug 27, 2020

alexwizp commented Aug 31, 2020

sulemanof left a comment

Choose a reason for hiding this comment

alexwizp commented Sep 1, 2020

stratoula left a comment

Choose a reason for hiding this comment

wylieconlon left a comment

Choose a reason for hiding this comment

wylieconlon Sep 1, 2020

Choose a reason for hiding this comment

wylieconlon commented Sep 1, 2020

wylieconlon commented Sep 1, 2020

alexwizp commented Sep 1, 2020

lukeelmers left a comment

Choose a reason for hiding this comment

lukeelmers Sep 1, 2020

Choose a reason for hiding this comment

alexwizp Sep 1, 2020

Choose a reason for hiding this comment

alexwizp commented Sep 1, 2020

lukeelmers left a comment

Choose a reason for hiding this comment

wylieconlon left a comment

Choose a reason for hiding this comment

kibanamachine commented Sep 1, 2020

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

async chunks size

page load bundle size

oss distributable file count

distributable file count

History

Add `auto` interval to histogram AggConfig #76001

Add `auto` interval to histogram AggConfig #76001

alexwizp commented Aug 26, 2020 •

edited

Loading