[BUG] Add Data table bucket Split rows agg limit of 3 #1179

RoyiSitbon · 2022-01-25T14:03:54Z

Signed-off-by: sitbubu royi.sitbon@logz.io

Description

The default limit of 3 “Split rows” aggregated buckets isn't applied on this visualization which allows adding more buckets and can harm cluster performance.

In other types of visualizations, there is a default limit of up to 3 “Split rows” aggregated buckets, e.g. Horizontal-Bar visualizations.

We wish to have an option to configure the max,min limitation for the “Split rows” aggregated bucket under dataTable vis.
i.e. Using this configuration, we will limit the max amount to 3:

This way we will see the next result in our UI:

If we won't set any limitation, i.e:

We will have same behavior as now:

linked issue:
#1178

kavilla · 2022-01-26T06:22:02Z

Hello @RoyiSitbon,

Thanks for opening this. Will check it out, right now it's blocked because the tests are failing due to this issue.

Initial nitpick feedback would be if you can some of the content you added to the PR to the commit description that provide insight on the commit without having to come to the PR. Also, I believe this could potential to add a quick test to verify it limits to 3.

RoyiSitbon · 2022-01-26T06:46:47Z

Hello @RoyiSitbon,

Thanks for opening this. Will check it out, right now it's blocked because the tests are failing due to this issue.

Initial nitpick feedback would be if you can some of the content you added to the PR to the commit description that provide insight on the commit without having to come to the PR. Also, I believe this could potential to add a quick test to verify it limits to 3.

Hey, sure. Thanks for the feedback.

jgough · 2022-01-28T11:51:01Z

I often want to split on more than 3 fields and losing this ability would be a step backwards when data sets are small and performance is not an issue. Is there any scope of making this configurable setting with a default of 3?

jgough · 2022-01-28T11:58:57Z

The functionality of allowing more than 3 splits in data tables has been present since Kibana 4 and I use this ability regularly. Sadly I have many visualisations that would break and just not be possible unless it were possible to configure this limit.

kavilla · 2022-01-29T07:59:47Z

The functionality of allowing more than 3 splits in data tables has been present since Kibana 4 and I use this ability regularly. Sadly I have many visualisations that would break and just not be possible unless it were possible to configure this limit.

I often want to split on more than 3 fields and losing this ability would be a step backwards when data sets are small and performance is not an issue. Is there any scope of making this configurable setting with a default of 3?

I see there is probably historical reason to have this not be capped at 3.

Perhaps, @ahopp or @kgcreative, what do y'all think about this change? Does it seems like a good improvement to make all of them similar in behavior and have an override config? Or can we gather more insight on the potential impact?

kgcreative · 2022-01-31T20:36:16Z

@kavilla, @RoyiSitbon, @jgough - I would prefer to handle this in a way where we don't introduce a breaking change. I would suggest a configuration option that's empty by default to introduce a split field limit (either in stack management, or kibana yml, or somewhere else). This would allow admins to configure this limit if needed for performance reasons, but wouldn't break existing functionality.

This setting should then be enforced by any visualization that uses split fields in agg buckets.

maybe something like aggBucketSplitFieldLimit: num;, where null or 0 allows for any number of split fields.

For additional corner case handling, existing visualizations with split fields beyond N would only display N split rows.

RoyiSitbon · 2022-02-01T09:59:02Z

@kavilla, @RoyiSitbon, @jgough - I would prefer to handle this in a way where we don't introduce a breaking change. I would suggest a configuration option that's empty by default to introduce a split field limit (either in stack management, or kibana yml, or somewhere else). This would allow admins to configure this limit if needed for performance reasons, but wouldn't break existing functionality.

This setting should then be enforced by any visualization that uses split fields in agg buckets.

maybe something like aggBucketSplitFieldLimit: num;, where null or 0 allows for any number of split fields.

For additional corner case handling, existing visualizations with split fields beyond N would only display N split rows.

Sure, that sounds good to me.
I'll work on a configurable solution if agreed by all.
@kavilla @jgough

Thanks

RoyiSitbon · 2022-02-03T12:47:15Z

Hey, I changed it to be a configurable solution as talked.
Thanks

kavilla · 2022-02-15T05:38:38Z

Hi @RoyiSitbon, if you rebase the tests should pass (granted if no failures by new implementation). Would you like for me to rebase for you on your branch? Let me know. Thank you!

Also, would there be an ability to add some tests to test this new functionality.

RoyiSitbon · 2022-02-15T18:31:03Z

Hi @RoyiSitbon, if you rebase the tests should pass (granted if no failures by new implementation). Would you like for me to rebase for you on your branch? Let me know. Thank you!

Also, would there be an ability to add some tests to test this new functionality.

Hey, rebased
Also, I will try to think of a tailored test. Thank you

RoyiSitbon · 2022-02-20T13:22:30Z

Hi @RoyiSitbon, if you rebase the tests should pass (granted if no failures by new implementation). Would you like for me to rebase for you on your branch? Let me know. Thank you!

Also, would there be an ability to add some tests to test this new functionality.

Added a new functional test with the right flow as well.

Signed-off-by: sitbubu <royi.sitbon@logz.io>

ananzh · 2022-03-03T19:33:43Z

Sorry. I commented on the wrong place: #1178 (comment)

I will re-check it.

ananzh · 2022-03-04T21:19:20Z

Hey @RoyiSitbon

After the latest change, advanced settings does not set a default split limit which is great. I have verified that. But I think the function is still not working right? For example, I set the advanced settings with/without max to limit the split to 3:

Then go to visualization, create a data table using eCommerce sample data. However, I could still split even I have 3 buckets. First, the "Split rows" option has no changes and still clickable.

Second, adding one more split is still working. We could see a descending based on product quality.

So I am bit confused here. Did I mis-use or mis-understand here? pls help

ananzh · 2022-03-05T05:07:39Z

Hey @RoyiSitbon

After the latest change, advanced settings does not set a default split limit which is great. I have verified that. But I think the function is still not working right? For example, I set the advanced settings with/without max to limit the split to 3:

Then go to visualization, create a data table using eCommerce sample data. However, I could still split even I have 3 buckets. First, the "Split rows" option has no changes and still clickable.

Second, adding one more split is still working. We could see a descending based on product quality.

So I am bit confused here. Did I mis-use or mis-understand here? pls help

My bad. It turns a cache issue and need to refresh the page to reload the settings. It is better to use requiresPageReload: true to improve user experience.

ananzh · 2022-03-05T05:12:06Z

Meanwhile, I have several questions

do other graph types besides data table also have this issue? or do they have some bucket split default limit in the code?
if other graph types have the same issue, do we or can we support them?
if no other graph types have the same issue and data table is the only type, do we need to use {"table": {"bucket":{}}}? should we modify the settings to be just bucket-min and bucket-max?

Probably we should discuss more here.

RoyiSitbon · 2022-03-06T06:41:56Z

requiresPageReload

Hey,
unlike this dynamic configuration setting, other agg types have constant settings at the moment.
The reason I implemented it like so, is because It will facilitate the implementation process afterward.
We think it will be a nice addition to integrate this feature to all vis and agg types in the system. but I guess it will be a gradual process.

kgcreative · 2022-03-10T19:57:04Z

I think it would be worth documenting all of the different visualizations that can use bucket aggregations. I think one question for the community here is if we want to have to pass all the different visualization types in order to change the split limit on all of them, or if there is an approach that can cover all visualization types, and, if desired, control individual visualizations at a more granular level.

From reading the issue, this seems to come from a desire to limit potentially expensive queries on large data sets, so it feels like a single setting to control the split limit everywhere it's used would be friendlier than having to control these individually

RoyiSitbon · 2022-03-16T07:38:02Z

I think it would be worth documenting all of the different visualizations that can use bucket aggregations. I think one question for the community here is if we want to have to pass all the different visualization types in order to change the split limit on all of them, or if there is an approach that can cover all visualization types, and, if desired, control individual visualizations at a more granular level.

From reading the issue, this seems to come from a desire to limit potentially expensive queries on large data sets, so it feels like a single setting to control the split limit everywhere it's used would be friendlier than having to control these individually

Hey @kgcreative, thanks for replying.
I understand what you are saying, who would be able to answer this type of question?
How will we understand if this solution is legit, and can be merged as-is or required to accommodate?

Thanks

ashwin-pc

Based on #1178 this does not seem to address the real problem here. If the goal is to limit expensive aggregations dynamically, this only does that for the table agg and that too for the Split Row agg type. I believe that issue needs a lil more discussion before we implement a solution. I'll call out my concerns and possible solutions in that issue.

ashwin-pc · 2022-06-03T07:13:39Z

src/plugins/vis_type_table/public/table_vis_type.ts

@@ -45,6 +45,10 @@ export function getTableVisTypeDefinition(
  core: CoreSetup,
  context: PluginInitializerContext
 ): BaseVisTypeOptions {
+  const tableBucketAggAmounts = core.uiSettings.get('visualize:aggAmounts')?.table?.bucket;


I'm a lil confused by this change. This UI setting only applies to the table viz since its used in the table_vis_type. But the setting is labeled as a way to dynamically set the min and max agg limits for the bucket viz. That is not the case. Am i missing something?

ashwin-pc · 2022-06-03T07:14:36Z

src/plugins/visualizations/server/plugin.ts

+      [VISUALIZE_AGG_AMOUNTS]: {
+        // currently only supports dataTable vis
+        name: i18n.translate('visualizations.advancedSettings.visualizeAggAmountsTitle', {
+          defaultMessage: `Set bucket's aggregation min, max level limitation`,


Any reason to only limit the bucket agg and not the metric agg too?

joshuarrrr · 2022-06-22T19:02:02Z

@RoyiSitbon I'm going to convert this to a draft until we can come to consensus on a more general approach as discussed in #1178.

seanneumann · 2022-09-26T20:39:37Z

We are closing this PR because what it says and does is very different. Next steps to resolve the discussion on #1178. After that feel free to open a new PR.

RoyiSitbon requested a review from a team as a code owner January 25, 2022 14:03

kavilla linked an issue Jan 26, 2022 that may be closed by this pull request

[BUG] Data Table Visualisation - The default limit of 3 "Split rows" aggregated buckets isn't applied #1178

Open

RoyiSitbon changed the title ~~[2.x][BUG] Add Data table bucket Split rows agg limit of 3~~ [BUG] Add Data table bucket Split rows agg limit of 3 Feb 3, 2022

RoyiSitbon force-pushed the addAggBucketSplitRowLimit branch from f75cdfa to b797497 Compare February 15, 2022 18:27

RoyiSitbon force-pushed the addAggBucketSplitRowLimit branch 2 times, most recently from c3a8a60 to 734ccea Compare February 20, 2022 13:46

kavilla assigned ananzh Feb 21, 2022

RoyiSitbon added 4 commits February 28, 2022 09:39

Add data table bucket agg split row limit to 3

8d3b782

Signed-off-by: sitbubu <royi.sitbon@logz.io>

Allow dataTable bucket min,max to be configurable

8f6339b

Signed-off-by: sitbubu <royi.sitbon@logz.io>

Devised new e2e with right flow + bugfix

e00e941

Signed-off-by: sitbubu <royi.sitbon@logz.io>

Set no bucket limitation as default

7fc8d18

Signed-off-by: sitbubu <royi.sitbon@logz.io>

RoyiSitbon force-pushed the addAggBucketSplitRowLimit branch from 734ccea to 7fc8d18 Compare February 28, 2022 07:58

Add json as type + fix test

f16897e

Signed-off-by: sitbubu <royi.sitbon@logz.io>

kavilla assigned kgcreative Apr 25, 2022

kavilla unassigned ananzh May 9, 2022

ashwin-pc mentioned this pull request Jun 3, 2022

[BUG] Data Table Visualisation - The default limit of 3 "Split rows" aggregated buckets isn't applied #1178

Open

ashwin-pc requested changes Jun 3, 2022

View reviewed changes

joshuarrrr marked this pull request as draft June 22, 2022 19:02

seanneumann closed this Sep 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BUG] Add Data table bucket Split rows agg limit of 3 #1179

[BUG] Add Data table bucket Split rows agg limit of 3 #1179

RoyiSitbon commented Jan 25, 2022 •

edited

Loading

kavilla commented Jan 26, 2022

RoyiSitbon commented Jan 26, 2022

jgough commented Jan 28, 2022

jgough commented Jan 28, 2022

kavilla commented Jan 29, 2022

kgcreative commented Jan 31, 2022 •

edited

Loading

RoyiSitbon commented Feb 1, 2022

RoyiSitbon commented Feb 3, 2022

kavilla commented Feb 15, 2022 •

edited

Loading

RoyiSitbon commented Feb 15, 2022

RoyiSitbon commented Feb 20, 2022

ananzh commented Mar 3, 2022

ananzh commented Mar 4, 2022

ananzh commented Mar 5, 2022

ananzh commented Mar 5, 2022 •

edited

Loading

RoyiSitbon commented Mar 6, 2022

kgcreative commented Mar 10, 2022

RoyiSitbon commented Mar 16, 2022

ashwin-pc left a comment

ashwin-pc Jun 3, 2022

ashwin-pc Jun 3, 2022

joshuarrrr commented Jun 22, 2022

seanneumann commented Sep 26, 2022

[BUG] Add Data table bucket Split rows agg limit of 3 #1179

[BUG] Add Data table bucket Split rows agg limit of 3 #1179

Conversation

RoyiSitbon commented Jan 25, 2022 • edited Loading

Description

kavilla commented Jan 26, 2022

RoyiSitbon commented Jan 26, 2022

jgough commented Jan 28, 2022

jgough commented Jan 28, 2022

kavilla commented Jan 29, 2022

kgcreative commented Jan 31, 2022 • edited Loading

RoyiSitbon commented Feb 1, 2022

RoyiSitbon commented Feb 3, 2022

kavilla commented Feb 15, 2022 • edited Loading

RoyiSitbon commented Feb 15, 2022

RoyiSitbon commented Feb 20, 2022

ananzh commented Mar 3, 2022

ananzh commented Mar 4, 2022

ananzh commented Mar 5, 2022

ananzh commented Mar 5, 2022 • edited Loading

RoyiSitbon commented Mar 6, 2022

kgcreative commented Mar 10, 2022

RoyiSitbon commented Mar 16, 2022

ashwin-pc left a comment

Choose a reason for hiding this comment

ashwin-pc Jun 3, 2022

Choose a reason for hiding this comment

ashwin-pc Jun 3, 2022

Choose a reason for hiding this comment

joshuarrrr commented Jun 22, 2022

seanneumann commented Sep 26, 2022

RoyiSitbon commented Jan 25, 2022 •

edited

Loading

kgcreative commented Jan 31, 2022 •

edited

Loading

kavilla commented Feb 15, 2022 •

edited

Loading

ananzh commented Mar 5, 2022 •

edited

Loading