[Lens] Filters aggregation #75635

mbondyra · 2020-08-21T10:09:17Z

Demo

Things to figure out:

1. Copy - in the making

AlonaNadler · 2020-08-24T19:19:46Z

Thanks @mbondyra this is a great start!!
I might have more comments after I play with it, I tried to address questions you raised in the video and in slack

if we apply filter aggregation on a field by default (in Marta's video she applies * by default), we need to help users understand what we did. Currently, the popup looks like there is no query. We should somehow specify you use all documents (*). Also in the preview maybe instead of just * explicitly saying *- all records

labels are unusually not that long, queries tend to be long. Im ok if we need to disregard the label length or add characters restriction
The query itself can be very long, any chance we increase the size of the KQL bar?
![image](https://user-images.githubusercontent.com/32550316/91085350-2c8d4400-e602-11ea-8b90-5727bd45ecc8.png*

there’s an asterisk in the design for ‘label’ - is there a tooltip over it?

not sure why we need * after the label

Scrolling problem is indeed an issue minute 3:15 in Marta's video, @Caroline any good solution in mind?

I know now we’re using ‘records’ field to make this aggregation. Could we merge it in this form and then work on removing a field from filters aggregation completely? I think we’re safe for 7.10

not sure I understand the questions. In general, I don't think search query agg works well with record fields. Users will usually use search query with one or multiple conditions. I wonder if we should allow to use records with search query aggregation

Copy: do we always call this ‘search query’ and not ‘filters aggregation’ like in Visualize?

we should start a text review document @gchaps can you help? I don't like calling it just filter in the popup left side menu, a filter has so many meanings in Kibana. So either filter aggregation of query grouping. We need to finalize on it

When having two identical queries, I add ‘[1]‘, ’[2] etc to every next query

Sounds good :)

wylieconlon · 2020-08-25T15:11:42Z

@AlonaNadler This is the first PR of several- we are expecting to do this work in several chunks before the feature freeze. The next chunk is to deal with the field selector, such as "Records," since @cchaos has asked for several changes.

cchaos · 2020-08-25T20:43:41Z

I think this PR is pretty darn close. I would only have minor feedback on the UI (when we get to the wrap-up stages).

In visualize we use "Filters aggregation". Here the copy in the design is 'search query'.

Whatever naming we do come up with, the idea is to try to embody the idea that this is a completely custom x-axis based on the query/filters that the user creates.

Long labels

Certainly even the chart has problems rendering these which is why I my original idea was to require (the asterisk) a custom label. This would force users to write a smaller label that the chart could actually display. But the way you have it wrapping even if they do type in a long custom label works for me.

Overflow bug

This is because the dropdown does not exist in a portal. @dziyanadzeraviankina is working on a PR for this.

Some responses to @AlonaNadler's review

Currently, the popup looks like there is no query. We should somehow specify you use all documents (*). Also in the preview maybe instead of just * explicitly saying *- all records

I would recommend the query being * but the label being All records (no *)

The query itself can be very long, any chance we increase the size of the KQL bar?

This automatically happens as the user types (when the input is focused) but it doesn't support the ability to grow inline.

When the config popover moves to a flyout, it will solve several problems. I think it will be easier to remove the "Field" option as it doesn't sit above the aggregations. And it will solve the popover inception and click outside handling.

gchaps · 2020-08-25T22:04:01Z

| 2. Placeholders for query and label

I'm not sure everyone knows what e.g. means. I suggest using "Example: " instead.

I've asked @debadair for suggestions for the Search query/Filters terminology in the left navigation.

I'm happy to review further if you set up a text document.

debadair · 2020-08-25T23:01:40Z

I'm not sure introducing new terminology for the filters aggregation (search query aggregation) that overloads two already overloaded terms (search and query) is a good alternative to overloading filter.
The notion of adding a Search query to a Search query also seems odd. These very common terms are used in so many different ways, it's tricky.

A "filter" in the filters bucket agg consists of a bucket name and a query that defines the criteria for including a hit in the bucket.

Each added filter (Search query) is actually a new bucket, isn't it? I don't especially like bucket as an end-user term, but it is used in Visualize. If we're introducing new terminology, maybe these are bucket filters or category filters?

AlonaNadler · 2020-08-26T04:28:38Z

@mbondyra really nice work!!

Few comments:

Moving between chart types with filter aggregations work very smooth 👍

Each added filter (Search query) is actually a new bucket, isn't it? I don't especially like bucket as an end-user term, but it is used in Visualize. If we're introducing new terminology, maybe these are bucket filters or category filters?

In many places in Lens we explicitly avoided Visualize and Elasticsearch terminology in favor of friendlier names. We are currently in pursuit of a friendlier name for the filter aggregation. bucket filters worth considering. In general,

if we don't find something we agree is an improvement we might need to ever back to filter aggs

playing with the PR one thing that I kept stumbling upon if I'm typing the query from the search bar, I'm used to clicking escape to leave the suggestions and submit the query. In Lens it closes the entire popup, I lost my queries like that multiple times

Seems like we are missing a done or submit button here

@cchaos in the query popup it hard to tell this is a search bar, perhaps like it says label we need something for the search bar. also I would use the search bar to explicitly call users to type their search.

Some of the labels on the x-axis disappeared- not sure why

cchaos · 2020-08-26T15:09:19Z

playing with the PR one thing that I kept stumbling upon if I'm typing the query from the search bar, I'm used to clicking escape to leave the suggestions and submit the query. In Lens it closes the entire popup, I lost my queries like that multiple times

I saw this happen to me too, but I'm pretty sure this will be solved when moving to the flyout because then it wouldn't be closing 2 popovers, only the search one which should automatically save. So let's wait on this issue til the flyout implementation is done.

also I would use the search bar to explicitly call users to type their search.

@AlonaNadler Can you explain further what the difference is between adding a prefix like "Search" to the input and the comment above?

The global KQL bar just has the placeholder text as "Search" vs the example that is shown in Lens (which I honestly prefer).

mbondyra · 2020-08-26T20:19:21Z

@wylieconlon @dej611 probably there'll be some small design/copy changes but I'd like to collect some feedback on technical level - I think it's ready to review/test. 🙏 🙂

mbondyra · 2020-09-09T08:48:03Z

@cchaos thanks for you help. When it comes to the naming, we've setup the copy document and @gchaps is on it. I'll pass you the link as well.
When it comes to saving the state of the other aggregations, we talked about it with Wylie and I think we should do it – but that's a bigger task. Let's talk about it.

… into lens/filters_aggregation

flash1293

Tested again and looks good to me.

We really have to work on the "Grouping" options as they can get super confusing (for example if you are working with two filters):

However it was never really working nice, so I would be fine solving this separately (I think you created an issue for this already, right?)

Edit: I just saw there is some discussion about this going on in a comment thread in this PR - if we can do some easy improvements in the scope of this PR, even better. I will keep my approval as I'm also fine with addressing it later.

cchaos

👍 LGTM! I think we'll need to re-address so a11y aspects of the dnd panels with buttons. But I think this is a repeatable pattern which we can address in a follow up

x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/dimension_popover.tsx

…onfig_panel/dimension_popover.tsx

…_frame/config_panel/dimension_popover.tsx" This reverts commit c18c658.

wylieconlon

The behavior is working exactly like I'd expect, except for the one popover state bug I found. I think this deserves more unit tests, but I am going to approve with the understanding that you can add these tests. I appreciate the issues describing more followups.

...lugins/lens/public/indexpattern_datasource/operations/definitions/filters/filter_popover.tsx

wylieconlon · 2020-09-09T18:46:22Z

...lugins/lens/public/indexpattern_datasource/operations/definitions/filters/filter_popover.tsx

+    setInputValue(value);
+  }, [value, setInputValue]);
+
+  const onChangeDebounced = React.useMemo(() => debounce(onChange, 256), [onChange]);


I'm seeing console warnings caused by slowness in the onChange function here: [Violation] 'setTimeout' handler took 169ms

These warnings happen in most of places when we modify the state so deeply. I think We need more top-down approach here instead of just trying to patch this single place.

x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/filters/filters.tsx

wylieconlon · 2020-09-09T19:06:14Z

...lugins/lens/public/indexpattern_datasource/operations/definitions/filters/filter_popover.tsx

+import { IndexPattern } from '../../../types';
+import { QueryStringInput, Query } from '../../../../../../../../src/plugins/data/public';
+
+export const FilterPopover = ({


This UI component appears to have no tests. I think the minimum level of testing might be something around the popover handling because I found a weird behavior.

wylieconlon · 2020-09-09T19:06:38Z

.../plugins/lens/public/indexpattern_datasource/operations/definitions/filters/filters.test.tsx

+      });
+    });
+
+    describe('Modify search query', () => {


No tests around the popover UI or drag and drop?

...lugins/lens/public/indexpattern_datasource/operations/definitions/filters/filter_popover.tsx

wylieconlon · 2020-09-09T19:32:51Z

@mbondyra One more unexpected behavior I found (which is the same as Visualize) is that when I click on a bar in the chart, the filter is using the DSL instead of showing a nice editor. Since this is the current behavior I think it's fine, but if there's anything you can see to easily improve the behavior it would be nice to have.

…ters_aggregation

kibanamachine · 2020-09-10T18:40:27Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 2978769

Build metrics

@kbn/optimizer bundle module count

id	value	diff	baseline
lens	415	+160	255

async chunks size

id	value	diff	baseline
lens	25.9KB	+1.0B	25.9KB

page load bundle size

id	value	diff	baseline
data	1.4MB	+743.0B	1.4MB
lens	1.0MB	+172.9KB	874.0KB
total		+173.6KB

History

💔 Build #73757 failed e47402fdb28ebdfa2bc767e840c4759d6b16b184
💔 Build #73697 failed eebbbdb
💔 Build #73613 failed 5cf7490
💚 Build #73486 succeeded 871ec5b
💚 Build #73473 succeeded c18c658

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

…s-for-710 * 'master' of github.com:elastic/kibana: (65 commits) Separate url forwarding logic and legacy services (elastic#76892) Bump yargs-parser to v13.1.2+ (elastic#77009) [Ingest Manager] Shared Fleet agent policy action (elastic#76013) [Search] Re-add support for aborting when a connection is closed (elastic#76470) [Search] Remove long-running query pop-up (elastic#75385) [Monitoring] Fix UI error when alerting is not available (elastic#77179) do not log plugin id format warning in dist mode (elastic#77134) [ML] Improving client side error handling (elastic#76743) [Alerting][Connectors] Refactor IBM Resilient: Generic Implementation (phase one) (elastic#74357) [Docs] some basic searchsource api docs (elastic#77038) add cGroupOverrides to the legacy config (elastic#77180) Change saved object bulkUpdate to work across multiple namespaces (elastic#75478) [Security Solution][Resolver] Replace Selectable popover with badges (elastic#76997) Removing ml-state index from archive (elastic#77143) [Security Solution] Add unit tests for histograms (elastic#77081) [Lens] Filters aggregation (elastic#75635) [Enterprise Search] Update WS Overview logic to use new config data (elastic#77122) Cleanup type output before building new types (elastic#77211) [Security Solution] Use safe type in resolver backend (elastic#76969) Use proper lodash syntax (elastic#77105) ... # Conflicts: # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/node_allocation.tsx

mbondyra added release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 Feature:Lens v7.10.0 labels Aug 21, 2020

mbondyra force-pushed the lens/filters_aggregation branch from 4e7c26b to c5f18b4 Compare August 24, 2020 12:31

elastic deleted a comment from kibanamachine Aug 25, 2020

mbondyra force-pushed the lens/filters_aggregation branch from 6c41209 to a2d5d71 Compare August 25, 2020 11:44

debadair closed this Aug 25, 2020

debadair reopened this Aug 25, 2020

mbondyra force-pushed the lens/filters_aggregation branch from a2d5d71 to 67172bf Compare August 26, 2020 07:18

mbondyra added 5 commits August 26, 2020 15:58

checkpoint

e82e536

filters aggregation tweaks and types

47ba9df

refactoring

38fb236

style fixes

be5ae8d

addressing feedback

8d1227d

tests added

66aada5

mbondyra force-pushed the lens/filters_aggregation branch from 67172bf to 66aada5 Compare August 26, 2020 20:16

elastic deleted a comment from kibanamachine Aug 26, 2020

mbondyra marked this pull request as ready for review August 26, 2020 20:19

mbondyra requested a review from a team August 26, 2020 20:19

mbondyra requested a review from a team as a code owner August 26, 2020 20:19

Merge branch 'lens/filters_aggregation' of github.com:mbondyra/kibana…

81c4080

… into lens/filters_aggregation

mbondyra requested review from cchaos, wylieconlon and flash1293 September 9, 2020 12:35

flash1293 approved these changes Sep 9, 2020

View reviewed changes

cchaos added 2 commits September 9, 2020 12:25

Fixing word-wrap and adding more titles

3a29c0f

Cleanup

b539a0f

cchaos approved these changes Sep 9, 2020

View reviewed changes

dej611 reviewed Sep 9, 2020

View reviewed changes

x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/dimension_popover.tsx Show resolved Hide resolved

mbondyra and others added 2 commits September 9, 2020 19:24

Update x-pack/plugins/lens/public/editor_frame_service/editor_frame/c…

c18c658

…onfig_panel/dimension_popover.tsx

Revert "Update x-pack/plugins/lens/public/editor_frame_service/editor…

871ec5b

…_frame/config_panel/dimension_popover.tsx" This reverts commit c18c658.

wylieconlon approved these changes Sep 9, 2020

View reviewed changes

wylieconlon reviewed Sep 9, 2020

View reviewed changes

...lugins/lens/public/indexpattern_datasource/operations/definitions/filters/filter_popover.tsx Outdated Show resolved Hide resolved

mbondyra added 5 commits September 10, 2020 10:38

refactor: copy

5cf7490

added tests

e95af12

type

eebbbdb

Merge commit '100afab3085e735fa0624a07a54a7f752d92935f' into lens/fil…

e0058e2

…ters_aggregation

useDebounce properly this time

2978769

mbondyra force-pushed the lens/filters_aggregation branch from e47402f to 2978769 Compare September 10, 2020 16:59

mbondyra merged commit 0c678eb into elastic:master Sep 10, 2020

mbondyra deleted the lens/filters_aggregation branch September 10, 2020 19:16

mbondyra mentioned this pull request Sep 10, 2020

[7.x] [Lens] Filters aggregation (#75635) #77242

Merged

mbondyra added a commit to mbondyra/kibana that referenced this pull request Sep 10, 2020

[Lens] Filters aggregation (elastic#75635)

e10d13f

mbondyra added a commit that referenced this pull request Sep 10, 2020

[Lens] Filters aggregation (#75635) (#77242)

6a91490

timroes mentioned this pull request Jan 13, 2021

Change the order of filter aggs #3210

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Lens] Filters aggregation #75635

[Lens] Filters aggregation #75635

mbondyra commented Aug 21, 2020 •

edited

Loading

AlonaNadler commented Aug 24, 2020

wylieconlon commented Aug 25, 2020

cchaos commented Aug 25, 2020

gchaps commented Aug 25, 2020

debadair commented Aug 25, 2020 •

edited

Loading

AlonaNadler commented Aug 26, 2020 •

edited

Loading

cchaos commented Aug 26, 2020

mbondyra commented Aug 26, 2020

mbondyra commented Sep 9, 2020 •

edited

Loading

flash1293 left a comment •

edited

Loading

cchaos left a comment

wylieconlon left a comment

wylieconlon Sep 9, 2020

mbondyra Sep 10, 2020

wylieconlon Sep 9, 2020

wylieconlon Sep 9, 2020

wylieconlon commented Sep 9, 2020

kibanamachine commented Sep 10, 2020

[Lens] Filters aggregation #75635

[Lens] Filters aggregation #75635

Conversation

mbondyra commented Aug 21, 2020 • edited Loading

Demo

Things to figure out:

1. Copy - in the making

AlonaNadler commented Aug 24, 2020

wylieconlon commented Aug 25, 2020

cchaos commented Aug 25, 2020

gchaps commented Aug 25, 2020

debadair commented Aug 25, 2020 • edited Loading

AlonaNadler commented Aug 26, 2020 • edited Loading

cchaos commented Aug 26, 2020

mbondyra commented Aug 26, 2020

mbondyra commented Sep 9, 2020 • edited Loading

flash1293 left a comment • edited Loading

Choose a reason for hiding this comment

cchaos left a comment

Choose a reason for hiding this comment

wylieconlon left a comment

Choose a reason for hiding this comment

wylieconlon Sep 9, 2020

Choose a reason for hiding this comment

mbondyra Sep 10, 2020

Choose a reason for hiding this comment

wylieconlon Sep 9, 2020

Choose a reason for hiding this comment

wylieconlon Sep 9, 2020

Choose a reason for hiding this comment

wylieconlon commented Sep 9, 2020

kibanamachine commented Sep 10, 2020

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

async chunks size

page load bundle size

History

mbondyra commented Aug 21, 2020 •

edited

Loading

debadair commented Aug 25, 2020 •

edited

Loading

AlonaNadler commented Aug 26, 2020 •

edited

Loading

mbondyra commented Sep 9, 2020 •

edited

Loading

flash1293 left a comment •

edited

Loading