Scale histogram aggregation interval to avoid crashing browser #14157

nreese · 2017-09-25T18:41:05Z

@spalger's PR added the infrastructure required for aggregations to perform pre-flight requests before getting serialized. This PR uses onRequestStart to fetch the min/max before serializing the histogram aggregation. That way, the interval can be scaled to ensure that a proper value is provided to avoid creating a gazillion buckets and crashing the browser.

spalger · 2017-09-26T01:41:34Z

src/ui/public/agg_types/buckets/histogram.js

        }
      },

      {
+        name: 'max',


We should probably use a less generic param name, maybe combine min/max to autoBounds: { min/max }?

JacobBrandt · 2017-09-26T02:21:54Z

src/ui/public/agg_types/controls/number_interval.html

+    Interval
+    <kbn-info
+      placement="right"
+      info="Interval will be automatically scaled in the event that the provided internval creates more buckets than specified by Advanced Setting's 'histogram:maxBars'">


typo - internval

thomasneirynck

This change is going to make a lot of people happy, awesome!

I have mostly minor comments to improve understanding and navigability of the new methods. The onStart-hook makes a lot of sense, but I'd tighten the method signatures a little bit more explicitly.

I would also add a selenium test for this, create a bar chart or something with a very small interval. e.g. add a test-case here: https://github.com/thomasneirynck/kibana/blob/b278ad96a9c0d4e9e0fcf91efb878884d1401fff/test/functional/apps/visualize/_vertical_bar_chart.js#L112-L112

thomasneirynck · 2017-09-28T20:33:43Z

src/ui/public/agg_types/controls/number_interval.html

@@ -0,0 +1,19 @@
+<div class="form-group">
+  <label for="visEditorInterval{{agg.id}}">
+    Interval


I'd rename now to "Minimum Interval"

thomasneirynck · 2017-09-28T21:33:36Z

src/ui/public/agg_types/buckets/histogram.js

+          // ensure interval does not create too many buckets and crash browser
+          if (aggConfig.autoBounds) {
+            const range = aggConfig.autoBounds.max - aggConfig.autoBounds.min;
+            const bars = range / interval;


I'd check if interval is 0. Through the magic of JS bars is either going to be NaN or Infinity depending on the nominator, so I'd just avoid this altogether.

An option too is modify the number_interval, to disallow 0 input for the agg.params.interval value.

thomasneirynck · 2017-09-28T21:46:14Z

src/ui/public/agg_types/param_types/base.js

+   *
+   *  @returns {Promise<undefined>|undefined}
+   */
+  BaseParamType.prototype.onSearchRequestStart = function () {


I'd have this return a dummy resolved Promise iso undefined.

I'd make the function signature also more explicit

BaseParamType.prototype.onSearchRequestStart = async function (aggconfig, searchSource, searchRequest) {

Given this is a side-effect function, consider renaming as modifyAggConfigOnSearchRequestStart. This rename would also to distinguish it from AggConfig#onsearchRequestStart(searchSource,SearchRequest)

I updated the method name and added @param comments about the arguments but I could not put the arguments in the method definition since BaseParamType#modifyAggConfigOnSearchRequestStart does not use the arguments and eslint is not happy about unused arguments

thomasneirynck · 2017-09-28T21:49:17Z

src/ui/public/vis/agg_config.js

-    self.type && self.type.params.forEach(function (param) {
-      if (param.onRequest) param.onRequest(self);
-    });
+  AggConfig.prototype.onSearchRequestStart = function (...args) {


I wouldn't use the varargs and just call them what they are:
AggConfig.prototype.onSearchRequestStart = function (searchSource, searchRequest) { ...

thomasneirynck · 2017-09-28T22:00:08Z

src/ui/public/agg_types/buckets/histogram.js

+            })
+            .fetchAsRejectablePromise()
+            .then((resp) => {
+              aggConfig.autoBounds = {


We do this stuff quite a bit in Kibana, and side-effects when well managed are very useful, but I'd consider making this explicit with a getter/setter on AggConfig. It will help jumping around in the code.

So I'd add something like:
AggConfig.prototype.setAutoBounds(autoBounds) and AggConfig.prototype.getAutoBounds().

This would be similar to https://github.com/thomasneirynck/kibana/blob/1423d068f8d4d38e5c0f2087482e4a9155e77a28/src/ui/public/time_buckets/time_buckets.js#L44-L44, which I find a little more readable.

Should the getters/setter methods really be added to AggConfig since most other configs will never need autoBounds? Autobounds is added to AggConfig as a decoration https://github.com/nreese/kibana/blob/hist_pre_flight/src/ui/public/agg_types/buckets/histogram.js#L22.

I don't think this should go on the aggConfig at all, but should go on aggConfig.params. That's what the params object is for, and adding a getter/setter to a general class like AggConfig doesn't really make sense to me.

Oh, I guess you could just update your decorateAggConfig function with getAutoBounds() and setAutoBounds()...

nreese · 2017-09-28T22:10:38Z

@thomasneirynck The PR contains a selenium test. Is another needed?

thomasneirynck · 2017-09-29T01:59:34Z

@nreese ok, shame on me, I completely missed https://github.com/elastic/kibana/pull/14157/files#diff-492637ba8caab7b809370b55eecad4e1. no, that's good, sorry for run around.

nreese · 2017-09-29T21:47:01Z

@thomasneirynck Everything has been addressed and should be ready for another look

nreese · 2017-09-29T21:59:26Z

jenkins, test this

thomasneirynck

🔥 👨‍🚒

great, thx!

nreese · 2017-10-23T13:42:53Z

Failed test caused by #14503. Trying again

Jenkins, test this

… add functional test

…ters for autoBounds to AggConfig. Protect against divide by zero

ppisljar

nice! LGTM

…ic#14157) * update histogram agg to fetch min and max when search request started * scale interval when too many buckets are created * move min and max params into autoBounds param, remove typo in help text * use decorated property instead of params to avoid changing agg state, add functional test * remove sleep from functional test * make args for onSearchRequest functions specific. Add getters and setters for autoBounds to AggConfig. Protect against divide by zero * add unused arguments with eslint comment

… (#14531) * update histogram agg to fetch min and max when search request started * scale interval when too many buckets are created * move min and max params into autoBounds param, remove typo in help text * use decorated property instead of params to avoid changing agg state, add functional test * remove sleep from functional test * make args for onSearchRequest functions specific. Add getters and setters for autoBounds to AggConfig. Protect against divide by zero * add unused arguments with eslint comment

…ic#14157) * update histogram agg to fetch min and max when search request started * scale interval when too many buckets are created * move min and max params into autoBounds param, remove typo in help text * use decorated property instead of params to avoid changing agg state, add functional test * remove sleep from functional test * make args for onSearchRequest functions specific. Add getters and setters for autoBounds to AggConfig. Protect against divide by zero * add unused arguments with eslint comment

nreese added v6.0.0-rc1 v6.1.0 v7.0.0 v6.0.0 and removed v6.0.0-rc1 labels Sep 25, 2017

nreese mentioned this pull request Sep 25, 2017

Charts render slow with big number of buckets #8159

Closed

nreese requested review from thomasneirynck and ppisljar September 25, 2017 18:57

nreese added the release_note:fix label Sep 25, 2017

spalger reviewed Sep 26, 2017

View reviewed changes

JacobBrandt reviewed Sep 26, 2017

View reviewed changes

JacobBrandt mentioned this pull request Sep 26, 2017

Allow all visualizations to supply max # of items to render #14166

Closed

epixa added v6.0.0-rc2 and removed v6.0.0-rc2 labels Sep 28, 2017

thomasneirynck added Feature:Visualizations Generic visualization features (in case no more specific feature label is available) Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc and removed v6.0.0 labels Sep 28, 2017

thomasneirynck reviewed Sep 28, 2017

View reviewed changes

thomasneirynck self-requested a review October 5, 2017 16:59

thomasneirynck approved these changes Oct 5, 2017

View reviewed changes

nreese force-pushed the hist_pre_flight branch 2 times, most recently from 2e56fa1 to 2d76f03 Compare October 23, 2017 11:42

nreese added 3 commits October 23, 2017 13:53

update histogram agg to fetch min and max when search request started

6ae3c40

scale interval when too many buckets are created

483bf77

move min and max params into autoBounds param, remove typo in help text

3a85b8f

nreese added 4 commits October 23, 2017 13:53

use decorated property instead of params to avoid changing agg state,…

2e7a504

… add functional test

remove sleep from functional test

b5b3933

make args for onSearchRequest functions specific. Add getters and set…

6b0eb21

…ters for autoBounds to AggConfig. Protect against divide by zero

add unused arguments with eslint comment

abfb618

nreese force-pushed the hist_pre_flight branch from 2d76f03 to abfb618 Compare October 23, 2017 19:53

ppisljar approved these changes Oct 24, 2017

View reviewed changes

nreese merged commit 88f3af4 into elastic:master Oct 24, 2017

nreese mentioned this pull request Oct 24, 2017

[Backport] Scale histogram aggregation interval to avoid crashing browser (#14157) #14531

Merged

thomasneirynck mentioned this pull request Oct 26, 2017

Kibana gets stuck while trying to render a visualization with a lot of data #14598

Closed

kertal mentioned this pull request Mar 9, 2021

[Discover] Remove redundant execution of onRequestStart when fetching data #94093

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scale histogram aggregation interval to avoid crashing browser #14157

Scale histogram aggregation interval to avoid crashing browser #14157

nreese commented Sep 25, 2017

spalger Sep 26, 2017

JacobBrandt Sep 26, 2017

thomasneirynck left a comment •

edited

thomasneirynck Sep 28, 2017

thomasneirynck Sep 28, 2017 •

edited

thomasneirynck Sep 28, 2017 •

edited

nreese Sep 29, 2017

thomasneirynck Sep 28, 2017

thomasneirynck Sep 28, 2017

nreese Sep 28, 2017

spalger Sep 29, 2017

spalger Sep 29, 2017

nreese commented Sep 28, 2017

thomasneirynck commented Sep 29, 2017

nreese commented Sep 29, 2017

nreese commented Sep 29, 2017

thomasneirynck left a comment

nreese commented Oct 23, 2017

ppisljar left a comment

Scale histogram aggregation interval to avoid crashing browser #14157

Scale histogram aggregation interval to avoid crashing browser #14157

Conversation

nreese commented Sep 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomasneirynck left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomasneirynck Sep 28, 2017 • edited

Choose a reason for hiding this comment

thomasneirynck Sep 28, 2017 • edited

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

nreese commented Sep 28, 2017

thomasneirynck commented Sep 29, 2017

nreese commented Sep 29, 2017

nreese commented Sep 29, 2017

thomasneirynck left a comment

Choose a reason for hiding this comment

nreese commented Oct 23, 2017

ppisljar left a comment

Choose a reason for hiding this comment

thomasneirynck left a comment •

edited

thomasneirynck Sep 28, 2017 •

edited

thomasneirynck Sep 28, 2017 •

edited