[context view] Apply filters to the context query #11466

weltenwort · 2017-04-27T11:20:36Z

Summary: This adds the ability to display a filter bar in the Context view and to apply those filters to the queries. It also modifies the link from the Discover view to the Context view to copy the currently defined filters when switching. New filters can be added from within the Context view using the icons in the expanded detail rows.

resolves #10954
relates to #275
builds upon #9198 and #11127

Progress

Add filter bar
Enable filter icons in detail view
Copy disabled filters from Discover
Add functional tests

Screenshots

Bargs

This looks really good already!

I only noticed one issue: pinning a filter in the context view throws an error and causes the page to reload.

I think the only other outstanding items are disabling the filters by default and re-adding the filter buttons on the field rows.

weltenwort · 2017-05-05T13:22:41Z

@Bargs I think it is in a state that justifies another look

Bargs

Functionality works well, overall the code looks great! Left just a few nitpicky questions.

Bargs · 2017-05-08T17:49:35Z

src/core_plugins/kibana/public/context/query_parameters/actions.js

@@ -43,7 +46,13 @@ export function QueryParameterActionsProvider() {
    )
  );

+  const addTermsFilter = (state) => (field, values, operation) => {


"termsFilter" is a bit of an overloaded (ahem) term since there's an actual Elasticsearch query type called "terms". Is there something you were trying to convey with this name that "addFilter" doesn't?

I actually intended to convey that this adds a terms filter in the ES sense, but just noticed that it actually adds either an exists or a phrase filter. So the name is really misleading. I'll change it to addFilter.

Bargs · 2017-05-08T17:55:27Z

src/core_plugins/kibana/public/context/query_parameters/actions.js

@@ -43,7 +46,13 @@ export function QueryParameterActionsProvider() {
    )
  );

+  const addTermsFilter = (state) => (field, values, operation) => {
+    const { queryParameters: { indexPatternId } } = state;


Does this provide a benefit over const indexPatternId = state.queryParameters.indexPatternId? Does it not throw if queryParameters is undefined or something? Just trying to understand the preference for destructing here since a regular assignment seems much more readable to me.

This just stems from the attempt to be consistent with the structure of the other action creators that fetch more than one value from the state. You are correct in that it does not contribute to readability here.

Ah ok, I just wasn't sure if there was some benefit I wasn't aware of.

Bargs · 2017-05-08T17:58:02Z

src/core_plugins/kibana/public/context/query_parameters/actions.js

@@ -43,7 +46,13 @@ export function QueryParameterActionsProvider() {
    )
  );

+  const addTermsFilter = (state) => (field, values, operation) => {
+    const { queryParameters: { indexPatternId } } = state;
+    filterManager.add(field, values, operation, indexPatternId);


Should we also call $scope.indexPattern.popularizeField(field, 1); like in discover?

I'm not sure how useful it would be to influence the ordering of the fields in the Discover field chooser via some action in the Context view. It might actually cause more confusion. 🤔

@tbragin what is your opinion?

We do it in Dashboard too fwiw. While ordering in the Discover sidebar is the only external effect at the moment, I always thought of field popularity as a more general purpose concept, a way to track how often fields are used across apps. But, we use it pretty inconsistently ¯_(ツ)_/¯

I don't see a problem with actions in context view influencing field popularity.

So while trying to add this I noticed that bumping the field popularity when adding a filter never worked in 5.x, because indexPattern.popularizeField() was always passed an incorrect argument for filtering operations. It only worked for column adding/removing. Do we still want to add it to the context view and fix it in other places?

Bargs · 2017-05-08T17:58:27Z

src/ui/public/filter_bar/lib/disable_filter.js

+export function setFilterDisabled(filter, disabled) {
+  const { meta = {} } = filter;
+
+  return {


Should this return a deep clone?

I have found that the best way to use javascript objects immutably is to return new objects when they are modified and original objects when they are not. This has the effect, that cheap comparisons by identity work well to detect changes while still keeping memory overhead at bay. It almost resembles structural sharing as used by the popular persistent data structure implementations and is in line with the best practice used by the redux and react communities.

The obvious downside is that it does not protect you against "malicious" code that modifies deep subtrees in-place, but I hope we will rather change that code than resolve to overly defensive programming.

I'm not sure I buy it, but it sounds like a debate not worth getting into here :) let's leave it as is.

Bargs · 2017-05-08T17:59:09Z

src/ui/public/filter_bar/lib/disable_filter.js

@@ -0,0 +1,25 @@
+export function disableFilter(filter) {


Could you add a few unit tests for the functions in this module?

weltenwort · 2017-05-15T13:44:14Z

the test failures seem to be unrelated

@Bargs I hope to have addressed your points

Bargs · 2017-05-15T15:14:37Z

LGTM

lukasolson

Awesome improvement! I found what I think are a couple of issues:

When I create a filter, then pin it, then switch to the context view, the filter appears to be enabled, but it really isn't. If I disable it then re-enable it, it actually becomes enabled:

Also, when I go to the context view, it appears that the filter in/out backgrounds persist, making white boxes in each of the columns of the anchor row:

When I create a filter in discover, then click the back button, the filter gets removed, but it doesn't navigate anywhere. When I go to the context view, then create a filter, then click the back button, I'm returned to the discover view. However, if I'm in the context view, then create a filter, then disable the filter, then click the back button, I stay in the context view but the filter is re-enabled:

weltenwort · 2017-05-16T11:19:15Z

Thanks for the thorough testing, @lukasolson!

I hope to have addressed the inconsistencies in the filter behavior. The broken
rendering of the filter icons in the table is occurring in Discover on master too, so I
will fix that in a separate PR.

weltenwort · 2017-05-16T15:04:47Z

@lukasolson here is the promised PR: #11819

alexfrancoeur · 2017-05-17T11:21:27Z

@weltenwort this looks great! I was hoping you could provide some clarification on a few items.

It appears with this workflow, any pre-defined filters will automatically be disabled. I understand this workflow and think it makes sense for viewing surrounding documents. Changes made to these filters are local to the context viewer as well. Pinned filters seem to be the exception here. If a pinned filter is enabled is the expectation that they are enabled upon entering the context viewer as well? Or are they expected to follow suite and disable as non-pinned filters do? I believe Lukas ran into the same issue here. I'm just wondering what the expected behavior is. It's a little odd that pinned filters keep their state when entering the context viewer but other filters do not. At the same time, the alternative of automatically disabling a global pinned filter doesn't necessarily make sense either.

This is minor and can be addressed with platform changes later on, but there is no way to add a breadcrumb at the moment is that correct? It can be a bit annoying to hit the back button for each filter change to return to discover and I don't believe it's always the users first inclination to click the discover button in the left hand nav.

@lukasolson I'm wondering, since we are targeting both this and #11375 for 5.5 - are there any concerns combining the two? The ability to add new filters directly from the menu will make this view even more powerful - I'm just wondering if there are any conflicts that we might want to get ahead of. For example - from a user experience perspective the filters are all automatically disabled upon entering the context view. In it's current state, there is an action to enable or disable all. I believe these bulk actions are lost with the new filtering capabilities but correct me if I'm wrong.

weltenwort · 2017-05-17T12:35:43Z

If a pinned filter is enabled is the expectation that they are enabled upon entering the context viewer as well?

That is what I intended here. (I hopefully fixed the problem that they appeared to be enabled but were not applied.) It kind of makes sense given the semantics of pinned filters (in so far as the pinned filters themselves make sense 😉). This is certainly up for discussion if there are some clear arguments and use-cases for different behavior.

breadcrumbs

The breadcrumbs were removed as a result of the discussion in #9198, because we did not want to represent history with them. This becomes especially important with the new accessibility criteria that the design team is currently trying to make Kibana comply with. Maybe they have something to say about this.

filter editors from #11375

We are not expecting any particular difficulties here. Since the context app is just consuming the filter bar as-is, improvements made to it should "just work", assuming that the interface via the queryFilter service remains stable.

lukasolson

LGTM!

Since the index pattern object is not clonable and serializable, only the index pattern id is stored in the state now.

In order to prevent special treatment in the context app component of filters from the app state and the global state depending on their origin, reading the filters is now moved up to the `ContextAppRouteController`. While doing so `ContextAppRouteController` does not handle `appState.filters` anymore, but delegates all access to the `queryFilter` service. Writing filter changes stays unchanged, as it was already fully delegated to `queryFilter`.

Backports PR elastic#11466 This adds the ability to display a filter bar in the Context view and to apply those filters to the queries. It also modifies the link from the Discover view to the Context view to copy the currently defined filters when switching. New filters can be added from within the Context view using the icons in the expanded detail rows.

Backports PR #11466 This adds the ability to display a filter bar in the Context view and to apply those filters to the queries. It also modifies the link from the Discover view to the Context view to copy the currently defined filters when switching. New filters can be added from within the Context view using the icons in the expanded detail rows.

This adds the ability to display a filter bar in the Context view and to apply those filters to the queries. It also modifies the link from the Discover view to the Context view to copy the currently defined filters when switching. New filters can be added from within the Context view using the icons in the expanded detail rows.

weltenwort added Feature:Discover Discover Application :Discovery release_note:enhancement WIP Work in progress v5.5.0 v6.0.0 labels Apr 27, 2017

weltenwort self-assigned this Apr 27, 2017

Bargs suggested changes Apr 28, 2017

View reviewed changes

tbragin mentioned this pull request May 1, 2017

Show event context #9198

Merged

13 tasks

weltenwort force-pushed the enhancement/context-filter-bar branch 2 times, most recently from d81a08f to f0152d0 Compare May 3, 2017 17:08

weltenwort mentioned this pull request May 4, 2017

Extract log event context #275

Closed

Bargs suggested changes May 8, 2017

View reviewed changes

weltenwort added the roadmap label May 10, 2017

weltenwort force-pushed the enhancement/context-filter-bar branch 3 times, most recently from 7bf5a36 to 9ad7933 Compare May 15, 2017 12:39

weltenwort changed the title ~~[WIP] [context view] Apply filters to the context query~~ [context view] Apply filters to the context query May 15, 2017

weltenwort requested a review from lukasolson May 15, 2017 13:44

Bargs approved these changes May 15, 2017

View reviewed changes

lukasolson requested changes May 15, 2017

View reviewed changes

weltenwort force-pushed the enhancement/context-filter-bar branch from 9ad7933 to ef09aa6 Compare May 16, 2017 11:12

epixa added the v6.0.0-alpha2 label May 17, 2017

lukasolson approved these changes May 17, 2017

View reviewed changes

weltenwort added 14 commits May 18, 2017 10:29

Include filters in the context link

097fdd8

Only store the index pattern id in the state

42d51a9

Since the index pattern object is not clonable and serializable, only the index pattern id is stored in the state now.

Apply filters to the context view

5e12a8c

Adapt tests

db77787

Enable per-field filter controls in context view

95b3b66

Pass filters to context in a disabled state

0d17693

Rename action creator addTermsFilter to addFilter

48669b4

Remove unnecessary destructuring

291135b

Add tests for filter activation manipulation functions

85774eb

Add functional test for copying filters to context view

d2070f8

Fix misleading comments in functional test

d13e6a5

Add functional test for filter bar interaction

ca27c52

Popularize fields when adding a filter

cd5d987

weltenwort force-pushed the enhancement/context-filter-bar branch from ef09aa6 to cd5d987 Compare May 18, 2017 08:30

weltenwort merged commit bc64b9a into elastic:master May 18, 2017

weltenwort mentioned this pull request May 18, 2017

[5.x] [context view] Apply filters to the context query #11890

Merged

epixa removed the WIP Work in progress label Jun 2, 2017

weltenwort mentioned this pull request Jun 12, 2017

[docs] Add filtering capabilities to context documentation #12271

Merged

jasonrhodes mentioned this pull request Jul 10, 2019

[Logs UI] View in context #40808

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[context view] Apply filters to the context query #11466

[context view] Apply filters to the context query #11466

weltenwort commented Apr 27, 2017 •

edited

Loading

Bargs left a comment

weltenwort commented May 5, 2017

Bargs left a comment

Bargs May 8, 2017

weltenwort May 9, 2017

Bargs May 8, 2017

weltenwort May 11, 2017

Bargs May 11, 2017

Bargs May 8, 2017

weltenwort May 11, 2017

Bargs May 11, 2017

tbragin May 16, 2017

weltenwort May 16, 2017

Bargs May 8, 2017

weltenwort May 11, 2017

Bargs May 11, 2017

Bargs May 8, 2017

weltenwort commented May 15, 2017

Bargs commented May 15, 2017

lukasolson left a comment

weltenwort commented May 16, 2017

weltenwort commented May 16, 2017

alexfrancoeur commented May 17, 2017

weltenwort commented May 17, 2017 •

edited

Loading

lukasolson left a comment

[context view] Apply filters to the context query #11466

[context view] Apply filters to the context query #11466

Conversation

weltenwort commented Apr 27, 2017 • edited Loading

Progress

Screenshots

Bargs left a comment

Choose a reason for hiding this comment

weltenwort commented May 5, 2017

Bargs left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weltenwort commented May 15, 2017

Bargs commented May 15, 2017

lukasolson left a comment

Choose a reason for hiding this comment

weltenwort commented May 16, 2017

weltenwort commented May 16, 2017

alexfrancoeur commented May 17, 2017

weltenwort commented May 17, 2017 • edited Loading

lukasolson left a comment

Choose a reason for hiding this comment

weltenwort commented Apr 27, 2017 •

edited

Loading

weltenwort commented May 17, 2017 •

edited

Loading