Add "use time filter" option to input controls by nreese · Pull Request #15852 · elastic/kibana

nreese · 2018-01-04T18:30:57Z

partial fix for #14659

Add use time filter option to input controls. When set to true, Controls will use the global time when fetching terms and min/max values.

kobelb · 2018-01-04T19:57:31Z

Instead of using JSON.stringify to determine if the time filter's changed, can we use _.clone and _.isEqual instead?

kobelb · 2018-01-04T20:00:53Z

Are we going to miss potential new items when using a relative time range, as the this.vis.API.timeFilter.time will be equal, but the underlying data ranges will differ?

Right now there is no way knowing if the render method is getting called because the timefilter updated. I thought comparing the time would work but you are right that when data is fetched for a relative time range then this strategy breaks down. Any ideas on how to handle relative dates?

What's the downside to rendering every-time render is called? Does this cause the data to be fetched again, and we're trying to prevent this when it's unnecessary?

exactly. Each time the component is drawn, then the terms agg or min/max agg will be required.

timefilter has a getBounds method call. I can just watch the results of that instead. Also, I am going to move that logic into https://github.com/elastic/kibana/blob/master/src/ui/public/vis/update_status.js. That way the status parameter will just contain a time key that will be set to true or false.

I think we should rely on Elasticsearch to cache what it can and make our requests as fast as possible. Trying to dedupe things client side (especially per visualization) seems like it will be very tricky to get right in all scenarios if not impossible.

I think we should rely on Elasticsearch to cache what it can and make our requests as fast as possible. Trying to dedupe things client side (especially per visualization) seems like it will be very tricky to get right in all scenarios if not impossible.

Agreed. I'm not aware of us doing this for other visualizations, was there a specific reason why Input Controls should be the exception to this @nreese?

Other visualizations do have custom logic to render under certain conditions. Visualizations are passed a status object to know why render is getting called. For example, the map visualization does different things based on the contents of status.

I created a new PR that adds time to the status object so all time change logic can be removed from vis_controller.js

nreese · 2018-01-08T21:21:30Z

@kobelb I removed the time checking logic since it's now provided by the status object. Let me know if there are any other changes needed.

kobelb · 2018-01-08T21:42:31Z

@nreese when I add the input control that uses the timefilter to a Dashboard, I'm still seeing it hit the early return when subsequently searching, this is going to cause the user to not see "new" values show up in the input control.

kobelb · 2018-01-09T15:20:00Z

@nreese I stand corrected, LGTM

kobelb

LGTM

thomasneirynck

lgtm.

I'd consider explaining the option better, either with a longer label or info-hover over. I'm not sure "use time filter" will make sense to a user without context.

nreese · 2018-01-12T15:52:58Z

@thomasneirynck I think you are right that an info description is needed for better explanation. This visualization is all react based so I can not use the angular kbn-info directive. I will create an info component in EUI. Should I delay this PR until that component is available or merge this PR as is and open an issue to add the info section once the EUI component is available?

* add useTimeFilter parameter to Controls visualization * fix broken jest test * add functional tests for useTimeFilter * remove wrong comment * use _.clone and _.isEqual for time comparision * do not track time changes in vis_controller - use status.time instead

nreese · 2018-01-16T19:38:42Z

@thomasneirynck Issue #16072 created to track Use time filter description.

* add useTimeFilter parameter to Controls visualization * fix broken jest test * add functional tests for useTimeFilter * remove wrong comment * use _.clone and _.isEqual for time comparision * do not track time changes in vis_controller - use status.time instead

robcowart · 2018-02-22T16:24:01Z

@nreese. Playing around with this feature, when enabled, it looks like it causes the visualization to pickup more than just the time to determine the list of items displayed. It also restricts the list by all of the global filters. This is not necessarily a bad thing. In fact I find it useful as it behaves like https://github.com/robcowart/kibana-vis-dropdown. I do however wish that I could set this on a per element basis.

Consider this... I have a dropdown with for Hosts(A) and a dropdown for Interfaces(B). If I could set A to ignore the global filter it would give me a list of all hosts.

When I pick a host in A it would set a global filter for the host. If B is set with to respect the global filter, it would now contain a list of interfaces only from the host selected in A.

As you can see from the screenshots, I tried to achieve this by creating two separate input control visualizations. One for Nodes, one for Interfaces. However in practice this doesn't work because the visualizations will overwrite each other's filter.

So to achieve such dependency-based input controls, either...

It must be possible set whether to ignore global settings per input element within a single visualization.
or
If each input control visualization instance needs to be able to set its own independent filter instances and keep track of them. (this is how the plugin I linked to above works)

nreese · 2018-02-22T16:33:09Z

Yes, the implementation in 6.2 is broken and picks up all global attributes.

//Broken implemenation
if (!useTimeFilter) {
  searchSource.inherits(false); //Do not filter by time so can not inherit from rootSearchSource
}

This has been fixed in 6.3, and now only the time filter is added instead of just inheriting from the root search source. The bug was fixed when refactoring for nested input controls #16407.

  searchSource.inherits(false);
  searchSource.filter(() => {
    const activeFilters = [...filters];
    if (useTimeFilter) {
      activeFilters.push(kbnApi.timeFilter.get(indexPattern));
    }
    return activeFilters;
  });

Maybe nested input controls solve your other problems as well?

robcowart · 2018-02-22T16:39:24Z

Got it. Of course that begs the follow on question. Is there an issue open to optionally allow an element to apply the global filter? As I mention this could make possible dependent elements as I show above. This feature is listed in #13911, but I don't see an issue open for it.

nreese · 2018-02-22T16:56:56Z

#16061

robcowart · 2018-02-22T17:05:12Z

That isn't the same thing and doesn't achieve dependency between two input control elements. The Kibana Advanced Setting filters:pinnedByDefault to true is the first change I make after installing Kibana, so filters set by Input Controls were always pinned for me. If input controls are "fixed" to ignore the global filter, then you can't leverage the global filter, pinned or not, to achieve dependency that I can see.

* add useTimeFilter parameter to Controls visualization * fix broken jest test * add functional tests for useTimeFilter * remove wrong comment * use _.clone and _.isEqual for time comparision * do not track time changes in vis_controller - use status.time instead

nreese added :Sharing release_note:enhancement v6.2.0 v7.0.0 labels Jan 4, 2018

nreese requested review from kobelb and stacey-gammon January 4, 2018 18:30

kobelb reviewed Jan 4, 2018

View reviewed changes

nreese force-pushed the useTimefilter branch from bfe4322 to 55b1407 Compare January 8, 2018 16:43

kobelb approved these changes Jan 9, 2018

View reviewed changes

nreese requested review from thomasneirynck and removed request for stacey-gammon January 9, 2018 16:01

thomasneirynck approved these changes Jan 11, 2018

View reviewed changes

nreese requested a review from lukasolson January 11, 2018 23:20

nreese added 6 commits January 12, 2018 08:44

add useTimeFilter parameter to Controls visualization

3961c43

fix broken jest test

d1e916f

add functional tests for useTimeFilter

ab19f6b

remove wrong comment

cffffec

use _.clone and _.isEqual for time comparision

d825645

do not track time changes in vis_controller - use status.time instead

c5f4ac5

nreese force-pushed the useTimefilter branch from 4232082 to c5f4ac5 Compare January 12, 2018 15:44

nreese merged commit 0c907ba into elastic:master Jan 16, 2018

nreese mentioned this pull request Jan 16, 2018

[6.x] Add "use time filter" option to input controls (#15852) #16071

Merged

This was referenced Jan 18, 2018

Controls visualization should use time filter #14659

Closed

add timefilter to search source instead of inheriting from root search source #16159

Merged

robcowart mentioned this pull request Feb 22, 2018

Input controls dropdown - query as-you-type so all available results are shown #14529

Closed

Conversation

nreese commented Jan 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nreese commented Jan 8, 2018

Uh oh!

kobelb commented Jan 8, 2018

Uh oh!

kobelb commented Jan 9, 2018

Uh oh!

kobelb left a comment

Choose a reason for hiding this comment

Uh oh!

thomasneirynck left a comment

Choose a reason for hiding this comment

Uh oh!

nreese commented Jan 12, 2018

Uh oh!

nreese commented Jan 16, 2018

Uh oh!

robcowart commented Feb 22, 2018

Uh oh!

nreese commented Feb 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robcowart commented Feb 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nreese commented Feb 22, 2018

Uh oh!

robcowart commented Feb 22, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nreese commented Jan 4, 2018 •

edited

Loading

nreese commented Feb 22, 2018 •

edited

Loading

robcowart commented Feb 22, 2018 •

edited

Loading