filtering (making it work with expressions) by ppisljar · Pull Request #55351 · elastic/kibana

ppisljar · 2020-01-20T19:40:07Z

Summary

Changes a bit how filtering works so it can work correctly with expressions
partly resolves #46904

esaggs adds meta information about the agg and field to the KibanaDatatable
use that meta information in createFilterFromEvent function to correctly build click filter
use that meta information in onBrush function to correctly build brush(range) filter
move filter creation inside the apply_filter_action this means that visualization doesn't emit a filter on click, but just information of what was clicked. We use this information in default apply_filter_action to correctly create the filter and apply it. This now makes it way easier to apply other actions, as its easy to extract information like cell value clicked etc.
added a new action called select_range_action which applies a range filter

FOLLOWUP:

this still doesn't solve the problem with filtering on expressions that manipulate the datatable provided by esaggs function. for example you could sum two columns into a new column and visualize it. filtering on that column will not work as meta information required is not provided.

in the future we can allow to customize default filter action for this scenarios. user could provide custom expression to build the filter which would be used when meta information provided by esaggs is not available.

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

This was checked for cross-browser compatibility, including a check against IE11
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

For maintainers

~~[ ] This was checked for breaking API changes and was labeled appropriately~~
~~[ ] This includes a feature addition or change that requires a release note and was labeled appropriately~~

elasticmachine · 2020-01-20T19:40:10Z

Pinging @elastic/kibana-app-arch (Team:AppArch)

ppisljar · 2020-01-20T19:40:35Z

src/legacy/core_plugins/data/public/search/expressions/esaggs.ts

any ideas for naming this ?

meta sgtm, but I think I would remove the _

stacey-gammon · 2020-01-22T20:03:03Z

src/legacy/core_plugins/data/public/search/expressions/esaggs.ts

meta sgtm, but I think I would remove the _

stacey-gammon · 2020-01-22T20:25:18Z

src/legacy/core_plugins/visualizations/public/embeddable/visualize_embeddable.ts

I am not sure we should separate these out instead of having a single one.

If we think the user will want something different to happen based on which one is emitted, we should separate them, but if in the users head, the same thing should happen, we should leave them together.

Right now, the user see's the same exact thing happen, the only difference here is in the code, to construct a different filter object, but in either case, a filter gets added to the view. But now, if the user wants to add a drilldown, we have two triggers, and we don't really know which one will be emitted from any given visualization. We could try to expose it but that will be difficult with lens when you don't really have the information from the renderer.

One other option is to leave these triggers separately, name them something like you suggested, SELECT_RANGE and CLICKED_DATA_POINT, and actually have each of those actions emit another trigger, maybe named ES_FILTER_SELECTED_TRIGGER. Then drilldowns actions can attach to that trigger name, and it won't matter whether it was a brush or a click, it only matters that the end result was that the user selected some filters, and any relevant drilldowns will apply.

if we wanted to do that, and eventually allow users to attach the raw URL builder drilldown where we will expose the raw data, not the es filter data, we'd need to figure out a way to batch or collect all triggers being emitted in a single event cycle. Otherwise, the user could attach an action on both CLICKED_DATA_POINT, and FILTER_SELECTED, and since they were separately emitted as part of the same event cycle, you'd see two context menu's pop up and we don't want that.

I think probably to avoid complicating drilldown feature, for phase 1, we should have at least one common trigger we can attach the drilldowns to, and not worry about having to figure out whether to attach the drilldown to the select range or data clicked trigger. Then once we expose the ability to attach actions to the triggers that have the raw data, figure out the batching then.

Thoughts?

i really like your idea about this actions emitting yet another trigger ES_FILTER_SELECTED_TRIGGER

i will leave creating another trigger ES_FILTER_SELECTED_TRIGGER out of this PR, but i agree that could be a possible solution to simplify the drilldown (or any user defined action) configuration.

flash1293 · 2020-01-23T09:59:32Z

src/legacy/core_plugins/data/public/search/expressions/esaggs.ts

Not sure whether that's out of scope for this PR, but the index pattern object is not serializable, right? It's also pretty big: https://github.com/elastic/kibana/pull/55351/files#diff-c85d790e1f9fa1b5d3c3ca92ea62bd60R1

Should this be just the id of the pattern?

I was almost going to add a comment suggesting you name this indexPatternId to avoid confusion (I thought it was the id).

wylieconlon · 2020-01-28T20:27:50Z

src/plugins/expressions/common/expression_types/kibana_datatable.ts

+export interface KibanaDatatableColumnMeta {
+  type: string;
+  indexPatternId: string;
+  params: Record<string, any>;


Suggested change

params: Record<string, any>;

aggConfig: Record<string, unknown>;

Having a generic object called params is not helpful for our use in client apps- will we need to assume that params is always the same data?

I suggest renaming to aggConfigs because that's what it appears to be in this change.

its aggConfigParams

wylieconlon · 2020-01-28T20:29:00Z

src/plugins/expressions/common/expression_types/kibana_datatable.ts

 export interface KibanaDatatableColumn {
  id: string;
  name: string;
+  meta?: KibanaDatatableColumnMeta;


This is not generic metadata about the table column any more- if it were, it would just include type. The addition of indexPattern and aggConfigs into the table is not what I would expect for all tables.

Suggestion:

Move the type parameter to the table column

Create a new table type for KibanaAggregatedTable

I agree that if we are adding a field for truly generic metadata, it should probably make the agg-specific values in KibanaDatatableColumnMeta optional, instead of assuming the only use of meta is ever going to be for aggs. KibanaAggregatedTable is an interesting idea too.

lukeelmers

I don't know a ton about the vislib legend portion and I'm only somewhat familiar with the syntax for registering custom actions, so can't really comment much on those, but I think the rest makes sense to me in general. Added a few notes.

src/legacy/core_plugins/data/public/search/expressions/utils.ts

src/legacy/core_plugins/data/public/search/expressions/esaggs.ts

src/legacy/core_plugins/data/public/search/expressions/utils.ts

lukeelmers · 2020-01-28T22:36:27Z

src/plugins/expressions/common/expression_types/kibana_datatable.ts

 export interface KibanaDatatableColumn {
  id: string;
  name: string;
+  meta?: KibanaDatatableColumnMeta;


I agree that if we are adding a field for truly generic metadata, it should probably make the agg-specific values in KibanaDatatableColumnMeta optional, instead of assuming the only use of meta is ever going to be for aggs. KibanaAggregatedTable is an interesting idea too.

src/legacy/core_plugins/data/public/search/expressions/create_filter.js

src/legacy/core_plugins/data/public/actions/value_click_action.ts

# Conflicts: # src/legacy/core_plugins/visualizations/public/embeddable/visualize_embeddable.ts # src/legacy/core_plugins/visualizations/public/np_ready/public/types/base_vis_type.js # src/legacy/ui/public/agg_types/index.ts

lukeelmers

Just to reiterate my disclaimer above:

I don't know a ton about the vislib legend portion and I'm only somewhat familiar with the syntax for registering custom actions, so can't really comment much on those

Other than that, code changes here LGTM, pending a green build from CI!

ppisljar · 2020-02-01T01:38:58Z

@elasticmachine merge upstream

ppisljar · 2020-02-03T11:46:41Z

@elasticmachine merge upstream

kibanamachine · 2020-02-03T13:04:42Z

💚 Build Succeeded

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

History

💚 Build #23885 succeeded 6515f82
💔 Build #23538 failed 04607b4
💔 Build #23329 failed 1af2600
💚 Build #23294 succeeded 9e72abb
💔 Build #23137 failed bebbbb2

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

* adding meta information to KibanaDatatable * updating filtering functions to use new information * moving filter creation to APPLY_FILTER_ACTION * adding SELECT_RANGE_ACTION and TRIGGER * making _meta optional * inlining legacy code for inspector * fixing jest tests * keeping apply_filter_action and adding value_click_action and trigger * utilities for serializing/unserializing aggConfigs * renaming prop to indexPatternId * cleanup * updating interpreter functional baselines * trying to fix tests * Fix legend tests * reverting update to multi metric screenshot * updating based on review * updating tests Co-authored-by: Nick Partridge <nick.ryan.partridge@gmail.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

stacey-gammon · 2020-02-06T22:42:37Z

src/legacy/core_plugins/data/public/actions/select_range_action.ts

+export const SELECT_RANGE_ACTION = 'SELECT_RANGE_ACTION';
+
+interface ActionContext {
+  data: any;


Can this be further typed out? The types are so critical to the trigger and what actions can be attached to it.

I'm going to create an issue to track this

wylieconlon · 2020-04-06T19:01:15Z

src/legacy/core_plugins/data/public/actions/filters/create_filters_from_event.js

+  });
  let filter = [];
-  const value = rowIndex > -1 ? table.rows[rowIndex][column.id] : cellValue;
+  const value = rowIndex > -1 ? table.rows[rowIndex][column.id] : null;


@ppisljar This is the line you are referring to in #61949, right?

yes, passing valid rowId is cruical now and regionmap was passing wrong info for a very long time.

ppisljar added v8.0.0 Team:AppArch release_note:skip Skip the PR/issue when compiling release notes v7.7.0 labels Jan 20, 2020

ppisljar requested a review from a team as a code owner January 20, 2020 19:40

ppisljar commented Jan 20, 2020

View reviewed changes

ppisljar requested a review from stacey-gammon January 20, 2020 19:44

ppisljar mentioned this pull request Jan 21, 2020

[Lens] Trigger a filter action on click in embedded visualizations for xychart #52806

Closed

ppisljar force-pushed the poc/filtering branch from 26b2c72 to 4e73315 Compare January 22, 2020 19:00

ppisljar requested a review from a team January 22, 2020 19:00

ppisljar changed the title ~~[PoC] filtering (making it work with expressions)~~ filtering (making it work with expressions) Jan 22, 2020

ppisljar force-pushed the poc/filtering branch from 975e9d9 to a67f968 Compare January 22, 2020 19:41

stacey-gammon reviewed Jan 22, 2020

View reviewed changes

flash1293 reviewed Jan 23, 2020

View reviewed changes

ppisljar force-pushed the poc/filtering branch from e317dc5 to ca586b8 Compare January 28, 2020 15:03

ppisljar added 11 commits January 28, 2020 11:05

adding meta information to KibanaDatatable

416c005

updating filtering functions to use new information

411045c

moving filter creation to APPLY_FILTER_ACTION

347547a

adding SELECT_RANGE_ACTION and TRIGGER

73c221a

making _meta optional

20d873b

inlining legacy code for inspector

3539e93

fixing jest tests

9def3bc

keeping apply_filter_action and adding value_click_action and trigger

1d499b1

utilities for serializing/unserializing aggConfigs

8b31793

renaming prop to indexPatternId

943ca0b

cleanup

650602c

ppisljar force-pushed the poc/filtering branch from ca586b8 to 650602c Compare January 28, 2020 16:09

ppisljar added 2 commits January 28, 2020 12:53

updating interpreter functional baselines

1d31a44

trying to fix tests

37a9d0c

ppisljar force-pushed the poc/filtering branch from 0586d2b to 37a9d0c Compare January 28, 2020 18:19

Fix legend tests

bebbbb2

wylieconlon reviewed Jan 28, 2020

View reviewed changes

lukeelmers reviewed Jan 28, 2020

View reviewed changes

ppisljar added 3 commits January 29, 2020 09:01

reverting update to multi metric screenshot

9e72abb

updating based on review

65f9ceb

Merge branch 'master' into poc/filtering

1af2600

# Conflicts: # src/legacy/core_plugins/visualizations/public/embeddable/visualize_embeddable.ts # src/legacy/core_plugins/visualizations/public/np_ready/public/types/base_vis_type.js # src/legacy/ui/public/agg_types/index.ts

lukeelmers approved these changes Jan 29, 2020

View reviewed changes

updating tests

04607b4

lukeelmers mentioned this pull request Jan 31, 2020

Move ui/agg_types in to shim data plugin #56353

Merged

2 tasks

Merge branch 'master' into poc/filtering

6515f82

Merge branch 'master' into poc/filtering

a040f0b

ppisljar merged commit f271365 into elastic:master Feb 3, 2020

ppisljar mentioned this pull request Feb 3, 2020

[7.x] filtering (making it work with expressions) (#55351) #56622

Merged

ppisljar added a commit that referenced this pull request Feb 3, 2020

filtering (making it work with expressions) (#55351) (#56622)

11f828c

stacey-gammon reviewed Feb 6, 2020

View reviewed changes

ppisljar mentioned this pull request Apr 6, 2020

fixing region map click filter #61949

Merged

1 task

wylieconlon reviewed Apr 6, 2020

View reviewed changes

	params: Record<string, any>;
	aggConfig: Record<string, unknown>;

Conversation

ppisljar commented Jan 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Uh oh!

elasticmachine commented Jan 20, 2020

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!

ppisljar Jan 28, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukeelmers left a comment

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!

Uh oh!

Uh oh!

lukeelmers left a comment

Choose a reason for hiding this comment

Uh oh!

ppisljar commented Feb 1, 2020

Uh oh!

ppisljar commented Feb 3, 2020

Uh oh!

kibanamachine commented Feb 3, 2020

💚 Build Succeeded

History

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!

ppisljar Apr 7, 2020 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

ppisljar commented Jan 20, 2020 •

edited

Loading

ppisljar Jan 28, 2020 •

edited

Loading

ppisljar Apr 7, 2020 •

edited

Loading