[OnWeek][Discover] Allow to change current sample size and save it with a saved search by jughosta · Pull Request #157269 · elastic/kibana

jughosta · 2023-05-10T13:39:20Z

Summary

This PR allows to change current sample size right from Discover page, no need to modify the global default value.

Saved search panels on Dashboard will also use the saved value to fetch only the requested sample size. This customisation was requested by many customers as it will allow to load Dashboards faster.

Current range for the slider: from 10 to 1000 (with a step 10).

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
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

…ed search

…-sample-size

jughosta · 2023-05-16T15:55:27Z

@andreadelrio What do you think about this UI/UX addition (settings icon + a slider)? What else could fit here to enable this functionality for users?

andreadelrio · 2023-05-17T01:38:58Z

@andreadelrio What do you think about this UI/UX addition (settings icon + a slider)? What else could fit here to enable this functionality for users?

@jughosta I think what you have works. However, if you wanted to have higher discoverability you could place this on the left side instead like so:

…-sample-size

jughosta · 2023-05-22T17:30:31Z

@andreadelrio Thanks! I pushed the updates. What was the name of the icon you used? The closest one I could find is visTable.

Discover page:

Dashboard view:

andreadelrio · 2023-05-22T17:35:57Z

@andreadelrio Thanks! I pushed the updates. What was the name of the icon you used? The closest one I could find is visTable.

Discover page:

Dashboard view:

@jughosta the icon name is editorTable.

andreadelrio · 2023-05-22T17:38:08Z

@andreadelrio What do you think about this UI/UX addition (settings icon + a slider)? What else could fit here to enable this functionality for users?

@jughosta I think what you have works. However, if you wanted to have higher discoverability you could place this on the left side instead like so:

@gchaps can you let us know if this label is ok for a button? in particular shortening "maximum" to "max". Thanks

gchaps · 2023-05-24T22:04:43Z

Yes, using 'max' works.

…-sample-size # Conflicts: # src/core/server/integration_tests/saved_objects/migrations/group2/check_registered_types.test.ts

elasticmachine · 2023-10-06T15:36:23Z

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

opauloh · 2023-10-06T16:51:49Z

packages/kbn-unified-data-table/src/components/data_table_additional_display_settings.tsx

+        compressed
+        fullWidth
+        min={1}
+        max={MAX_ALLOWED_SAMPLE_SIZE}


Hey, I'm wondering if the current input might give the impression that it can accept really big numbers for sample size:

And since there is this limit, what about changing to EuiSlider as was previously suggested? Keeping the validations of the onChangeActiveSampleSize method, as EuiSlider doesn't handle that.

@opauloh Thanks for your feedback! The slider was not really helpful when having a step of 1, so an input would work better. But there is another option: to have a slider with a step 10 starting from 10. Updated the PR via 7feb2ae

LGTM 🚀

Thanks for updating it @jughosta ! I liked how it's visually clear now that it can go up to the max limit, and that the default selection is in between!

Agree on changing the steps, a step of 10 makes way more sense for this purpose and improves the usability!

opauloh

CloudSecurityDataTable changes LGTM

TinaHeiligers

LGTM regarding updates to the SO

TinaHeiligers · 2023-10-06T19:09:30Z

src/plugins/discover/public/application/main/components/layout/discover_documents.tsx

  const hideAnnouncements = useMemo(() => uiSettings.get(HIDE_ANNOUNCEMENTS), [uiSettings]);
  const isLegacy = useMemo(() => uiSettings.get(DOC_TABLE_LEGACY), [uiSettings]);
-  const sampleSize = useMemo(() => uiSettings.get(SAMPLE_SIZE_SETTING), [uiSettings]);
+  const defaultSampleSize = useMemo(() => uiSettings.get(SAMPLE_SIZE_SETTING), [uiSettings]);


Is there a max limit? If there isn't, we need to have one set. Users often don't know the effects of loading too many documents.

What's the default for sample size? One test uses 70, is that it?

Hi @TinaHeiligers,

Is there a max limit? If there isn't, we need to have one set. Users often don't know the effects of loading too many documents.

Yes, currently it's between 10 and 1000. Users can now also press "Load more" for loading next pages (PR).

What's the default for sample size? One test uses 70, is that it?

It's 500 by default.

TinaHeiligers · 2023-10-06T19:13:26Z

src/plugins/saved_search/server/saved_objects/search.ts

 import { MigrateFunctionsObject } from '@kbn/kibana-utils-plugin/common';
-import { VIEW_MODE } from '../../common';
 import { getAllMigrations } from './search_migrations';
+import { SCHEMA_SEARCH_V8_8_0, SCHEMA_SEARCH_V8_12_0 } from './schema';


davismcphee

Awesome work! Code looks good, and it worked well during testing 👍

The only thing I feel could be improved is that when I set the default sample size to a value greater than 1000, then change the sample size to a custom one, I'm unable to get the default sample size back unless I click the "New" button:

A couple of options I think are worth considering:

Make it so that clearing the sample size input resets it to the default.
Dynamically set the max sample size based on the greater of the default value or 1000.

Wdyt?

davismcphee · 2023-10-12T22:09:15Z

packages/kbn-unified-data-table/src/components/data_table.tsx

+    }
+
+    if (onUpdateSampleSize) {
+      options.allowResetButton = false;


Why do we set this to false?

@davismcphee Since we have issues with the button #131130 and now we introduce additional settings which are not handled by eui code, we better hide the button. Originally suggested by @kertal

yes, it did not work as expected, and introducing new settings, wouldn't make it better, so I suggested to remove it (not having a good way to handle it, at least back in the days)

Ok, makes sense to remove it for now if it wasn't working as intended. Might be worth figuring out if we can fix it and add it back in later though. It would be convenient to be able to reset the sample size back to the saved search default after changing it on a dashboard and saving, which persists the updated value with the dashboard.

src/plugins/discover/public/embeddable/saved_search_embeddable.tsx

src/plugins/saved_search/common/content_management/v1/cm_services.ts

src/plugins/saved_search/server/saved_objects/schema.ts

test/functional/apps/discover/group2/_data_grid_row_height.ts

test/functional/apps/discover/group2/_data_grid_sample_size.ts

kertal · 2023-10-13T06:37:58Z

packages/kbn-unified-data-table/src/components/data_table_additional_display_settings.tsx

+import { i18n } from '@kbn/i18n';
+import { debounce } from 'lodash';
+
+export const MAX_ALLOWED_SAMPLE_SIZE = 1000;


Shouldn't we set to 500, which is the default in Advanced settings, and limit the max to discover:sampleSize? We now have the option to load more anyway. I've configured 2000 for this setting, still was was 1000. So I think taking discover:sampleSize into consideration makes sense, also it should be used e.g. when an admin decides to go back from 1000 to 500 , the user configured 990, it could make sense to limit the request to 500 in this case, wdyt?
This is what came to my mind while testing it. Apart from that: 🎉

@kertal That's a good idea! The only issues is that if we base the max size on discover:sampleSize then we can't set it for the schema validation. Also users can even now update it in the URL and request more than the configured discover:sampleSize, would you limit that too?

We could define 10_000 limit for the schema and in UI use min of 10_000 and discover:sampleSize as max limit.

A hard limit of 10000 could be a good idea for the schema ... however we should make sure the number requested from ES should be limited by the sampleSize. Because in the end, this it was matters in terms of traffic and performance.

@kertal I pushed changes for it 9766615

Could you please check if it's close to what you meant?

i will, thx!

Tested and it's exactly what I meant! Thx 🙏 !

jughosta · 2023-10-13T13:31:51Z

@davismcphee

The only thing I feel could be improved is that when I set the default sample size to a value greater than 1000, then change the sample size to a custom one, I'm unable to get the default sample size back unless I click the "New" button

When a too large number is entered, we show a validation error. It's not applied to the state. Closing the settings popover will take you back. Yes, we don't provide a way to go to a default value if user moves the slider after that, is it an issue? With the latest changes, the default value becomes the slider's max value.

kibana-ci · 2023-10-17T10:57:07Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: c25f598

Failed CI Steps

Defend Workflows Cypress Tests on Serverless #2

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`cloudSecurityPosture`	379	380	+1
`discover`	675	677	+2
`logExplorer`	454	455	+1
total			+4

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`@kbn/unified-data-table`	43	44	+1
`savedSearch`	71	74	+3
total			+4

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`cloudSecurityPosture`	399.5KB	400.4KB	+977.0B
`discover`	575.0KB	577.4KB	+2.4KB
total			+3.3KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`discover`	30.9KB	31.1KB	+141.0B
`savedSearch`	10.9KB	11.1KB	+252.0B
total			+393.0B

Unknown metric groups

API count

id	before	after	diff
`@kbn/unified-data-table`	100	103	+3
`discover`	118	119	+1
`savedSearch`	72	75	+3
total			+7

History

💚 Build #168063 succeeded 1bbb2e6
💛 Build #167833 was flaky cc1fbc0
💔 Build #167785 failed d6c17ad
💔 Build #167761 failed b398fdd
💔 Build #167637 failed 9d38df8
💛 Build #166227 was flaky 7feb2ae

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

cc @jughosta

davismcphee

The latest changes look good, and I tested again and it's working great. Thanks for the work on this, another useful new feature! 🎉

Yes, we don't provide a way to go to a default value if user moves the slider after that, is it an issue? With the latest changes, the default value becomes the slider's max value.

I left a comment explaining why the reset functionality would be useful, but I think it's ok for now. Otherwise the change to make the default value the max addresses my concern 👍

davismcphee · 2023-10-19T00:01:13Z

packages/kbn-unified-data-table/src/components/data_table.tsx

+    }
+
+    if (onUpdateSampleSize) {
+      options.allowResetButton = false;


Ok, makes sense to remove it for now if it wasn't working as intended. Might be worth figuring out if we can fix it and add it back in later though. It would be convenient to be able to reset the sample size back to the saved search default after changing it on a dashboard and saving, which persists the updated value with the dashboard.

[Discover] Allow to change current sample size and save it with a sav…

fd399ce

…ed search

jughosta changed the title ~~[Discover] Allow to change current sample size and save it with a saved search~~ [OnWeek][Discover] Allow to change current sample size and save it with a saved search May 10, 2023

jughosta added 8 commits May 10, 2023 16:21

[Discover] Search embaddable supports a custom sample size

6fdae5c

Merge remote-tracking branch 'upstream/main' into onweek-configurable…

fcb469b

…-sample-size

[Discover] Update test

5ca785c

[Discover] Fix context page

8d80dbe

[Discover] Disable in SQL mode

8150dd7

[Discover] Fix redundant state updates

517cd51

[Discover] Fix the max value

e39f191

[Discover] Discard old value

b25b974

jughosta self-assigned this May 11, 2023

jughosta added release_note:enhancement Feature:Discover Discover Application backport:skip This PR does not require backporting Team:DataDiscovery Discover, search (data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. t// labels May 11, 2023

jughosta added 3 commits May 11, 2023 17:53

Merge branch 'main' into onweek-configurable-sample-size

469a3ac

[Discover] Fix the default case

1fbf4eb

Merge branch 'main' into onweek-configurable-sample-size

f070a1c

jughosta added 2 commits May 22, 2023 18:06

Merge remote-tracking branch 'upstream/main' into onweek-configurable…

f5b6ae7

…-sample-size

[Discover] Update UI

96572bb

[Discover] Update icon

05a72b7

jughosta added 3 commits June 8, 2023 10:53

Merge remote-tracking branch 'upstream/main' into onweek-configurable…

2095b0c

…-sample-size # Conflicts: # src/core/server/integration_tests/saved_objects/migrations/group2/check_registered_types.test.ts

[Discover] Fix tests

ba2297d

Merge branch 'main' into onweek-configurable-sample-size

0755a37

jughosta marked this pull request as ready for review October 6, 2023 15:36

jughosta requested review from a team as code owners October 6, 2023 15:36

opauloh reviewed Oct 6, 2023

View reviewed changes

opauloh approved these changes Oct 6, 2023

View reviewed changes

TinaHeiligers approved these changes Oct 6, 2023

View reviewed changes

jughosta added 2 commits October 9, 2023 10:10

Merge branch 'main' into onweek-configurable-sample-size

b58bff1

[Discover] Switch back to EuiRange

7feb2ae

davismcphee reviewed Oct 13, 2023

View reviewed changes

kertal reviewed Oct 13, 2023

View reviewed changes

jughosta added 3 commits October 13, 2023 10:51

Merge branch 'main' into onweek-configurable-sample-size

9d38df8

[Discover] Limit fetched sample by settings and 10_000

9766615

[Discover] Add tests

b398fdd

jughosta added 4 commits October 13, 2023 15:50

[Discover] Fix tests

d6c17ad

Merge branch 'main' into onweek-configurable-sample-size

cc1fbc0

Merge branch 'main' into onweek-configurable-sample-size

1bbb2e6

Merge branch 'main' into onweek-configurable-sample-size

c25f598

davismcphee approved these changes Oct 19, 2023

View reviewed changes

jughosta merged commit 6038f92 into elastic:main Oct 19, 2023

kibanamachine added the v8.12.0 label Oct 19, 2023

jughosta mentioned this pull request Oct 19, 2023

[Discover] Support "Load more" in saved search embeddable #169338

Open

opauloh mentioned this pull request Oct 19, 2023

[Cloud Security] [DataTable] Enable more UnifiedDataTable features #168620

Closed

2 tasks

This was referenced Nov 14, 2023

Configurable page size, number of pages, and field trim for doc table #4060

Closed

Per-panel saved search sample size #3220

Closed

Conversation

jughosta commented May 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

jughosta commented May 16, 2023

Uh oh!

andreadelrio commented May 17, 2023

Uh oh!

jughosta commented May 22, 2023

Uh oh!

andreadelrio commented May 22, 2023

Uh oh!

andreadelrio commented May 22, 2023

Uh oh!

gchaps commented May 24, 2023

Uh oh!

elasticmachine commented Oct 6, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

opauloh left a comment

Choose a reason for hiding this comment

Uh oh!

TinaHeiligers left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davismcphee left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jughosta Oct 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jughosta commented Oct 13, 2023

Uh oh!

kibana-ci commented Oct 17, 2023

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Page load bundle

jughosta commented May 10, 2023 •

edited

Loading

jughosta Oct 13, 2023 •

edited

Loading