explicitly passing filters and query to visualize #19172

ppisljar · 2018-05-17T12:47:11Z

filters and query are explicitly passed down to dashboard embeddables (visualize and saved search)

rootSearchSource is removed, timeFilter needs to be passed explicitly in to each embeddable or set on each searchSource
appSearchSource is removed, query and filters need to be passed explicitly to each embeddable
visualize no longer needs to watch appState for changes
visualization searchSource no longer inherits from rootSearchSource (however it still uses inheritance for saved searches or filters/query saved with visualization.

resolves #16641, resolves #16595

stacey-gammon · 2018-05-21T17:23:37Z

src/core_plugins/kibana/public/dashboard/dashboard_state_manager.js

@@ -177,6 +181,14 @@ export class DashboardStateManager {
    if (getDescription(state) !== this.getDescription()) {
      store.dispatch(updateDescription(this.getDescription()));
    }
+
+    if (getFilters(state) !== this.getFilterState().filterBars) {


This will always return not equal:

See getFilterBarChanged function in this class for a better way to compare them. (Unfortunately I'm not sure there are any tests to make sure this isn't overly triggered...)

stacey-gammon · 2018-05-21T17:28:14Z

src/core_plugins/kibana/public/discover/embeddable/search_embeddable.js

    this.searchScope.columns = this.customization.columns || this.savedSearch.columns;
    this.searchScope.sort = this.customization.sort || this.savedSearch.sort;
    this.searchScope.sharedItemTitle = this.panelTitle;
+
+    this.searchScope.searchSource.getParent().set('filter', this.filters);


I think you are losing the time range filter. Unfortunately the passed down time range is not being picked up by saved searches yet. Was something I was planning to get to at some point... looks like you are going to beat me to it though!

Why setting it on the parent and not this.searchScope.searchSource? Could you clarify a bit what search source is which in this case?

we created a searchsource 'stack' (check below) where timeRangeSearchSource is the root and filterSearchSouce inherits from it and our saved searchSource inherits from the filterSearchSource

savedSearchSource will have filters and query saved with the search

filterSearchSource will get dashboard filters and query

timeRangeSearchSource will get the time range filter

we do this as we don't want to handle merging of filters and queries (if we would have only one level) and we let the searchsource handle it.

Couldn't we change the inheritance order of the search sources to be: timeRangeSearchSource ← savedSearchSource ← filterSearchSource (i.e. put the saved search source in the middle).

That way we could safely use this.searchScope.searchSource instead of needing this kind of implementation detail based call to getParent()? Also for the actual request, the inheritance order of those three shouldn't matter?

elasticmachine · 2018-05-21T18:05:39Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-05-21T18:17:32Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-05-21T21:09:42Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-05-22T14:42:30Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-05-22T15:45:55Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-05-23T19:07:37Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

timroes

I think the way itself look good to go, though we should carefully also test every of this scenarios. I would also be interested in @spalger overall thoughts about that, and we should at least when we are done with it (maybe already before for feedback) get a review by @stacey-gammon and @Bargs

timroes · 2018-05-24T11:21:47Z

src/core_plugins/kibana/public/discover/controllers/discover.js

@@ -182,6 +182,17 @@ function discoverController(
    .highlightAll(true)
    .version(true);

+  // searchSource which applies time range
+  const timeRangeSearchSource = savedSearch.searchSource.clone();


Do we actually want to clone here, and not just creating a new instance? I've got the feeling except the filter (and maybe the index) we don't want to have anything from that savedSearch.searchSource? Wouldn't it be easier to create a new one, and copy this two things over rather than cloning and resetting some things (like query) below, but might forget to reset some important things?

what are the things you wouldn't want ? when i was thinking about it it was either:

create a new one and make sure the parameters are set correctly (index ... ? not sure if anything else is needed ?)

clone, and make sure the parameters are set correctly <-- sounded safer, as i don't need to worry about all parameters + i don't need to create a new SearchSource which means i don't need access to Private

I agree with @timroes and think we should be explicit about what we want to copy over, rather than cloning and unsetting things.

How do you feel about these options?

// create a new searchSource, without needing Private const timeRangeSearchSource = new savedSearch.searchSource.constructor() // or a little less nasty, add a `.new()` method const timeRangeSearchSource = savedSearch.searchSource.new() // or add a `.reset()` method so we don't have to know what to unset const timeRangeSearchSource = savedSearch.searchSource.clone().reset()

timroes · 2018-05-24T11:22:56Z

src/core_plugins/kibana/public/discover/embeddable/search_embeddable.js

    this.searchScope.columns = this.customization.columns || this.savedSearch.columns;
    this.searchScope.sort = this.customization.sort || this.savedSearch.sort;
    this.searchScope.sharedItemTitle = this.panelTitle;
+
+    this.searchScope.searchSource.getParent().set('filter', this.filters);


Why setting it on the parent and not this.searchScope.searchSource? Could you clarify a bit what search source is which in this case?

timroes · 2018-05-24T11:23:29Z

src/core_plugins/kibana/public/discover/embeddable/search_embeddable.js

    this.searchScope.description = this.savedSearch.description;
    this.searchScope.searchSource = this.savedSearch.searchSource;

+    const timeRangeSearchSource = this.searchScope.searchSource.clone();


Same question as above: cloning vs creating new one?

same answer as above :)

timroes · 2018-05-24T11:24:11Z

src/core_plugins/kibana/public/discover/embeddable/search_embeddable.js

+    });
+
+    const filtersSearchSource = this.searchScope.searchSource.clone();
+    filtersSearchSource.inherits(timeRangeSearchSource);


This one should still get this.filters and this.query in it?

yes, this one will receive filters and query

timroes · 2018-05-24T11:24:55Z

src/core_plugins/kibana/public/visualize/editor/editor.js

@@ -220,6 +220,7 @@ function VisEditor($scope, $route, timefilter, AppState, $window, kbnUrl, courie

    const updateTimeRange = () => {
      $scope.timeRange = timefilter.time;
+      $scope.vis.forceReload();


As discussed we should instead listen in visualize on the changes of filter/queries/timerange

the $watch is there now .... however removing forceReload doesn't work (watch is never triggered) .... i didn't dig into what exactly is preventing it to work that way .... any ideas ?

timroes · 2018-05-24T11:26:20Z

src/ui/public/timefilter/get_time.js

+import _ from 'lodash';
+import dateMath from '@kbn/datemath';
+
+export function calculateBounds(timeRange, forceNow) {


I would be more explicit here for better readability and make forceNow = false

forceNow should not be false, but undefined or a date ?

I personally find unnamed boolean arguments hard to understand, what if we just allowed dateMathOptions:

export function calculateBounds(timeRange, dateMathOptions = {}) { return { min: dateMath.parse(timeRange.from, { ...dateMathOptions }), max: dateMath.parse(timeRange.to, { roundUp: true, ...dateMathOptions }) }; } // ... somewhere else ... calculateBounds(timeRange, { forceNow: true }) // vs calculateBounds(timeRange, true) ``

timroes · 2018-05-24T11:27:38Z

src/ui/public/vis/request_handlers/courier.js

+      // in editor mode we want to set filter and query on searchSource so they get
+      // stored with visualization, however on dashboard we set them on requestSearchSource
+      // so we don't override the stored ones
+      if (vis.editorMode) {


I think it's okay for now, but we should still figure out, if we can find a better way than that.

I feel like the editor just shouldn't pass in the filters or query to <visualize>, instead the editor will set them on the vis searchSource and <visualize> will default to no extra filters or queries.

timroes · 2018-05-24T11:30:38Z

src/ui/public/visualize/visualize.js

@@ -133,9 +135,8 @@ uiModules
          if ($scope.editorMode) {


Do we still need this appState.vis in there? Shouldn't that rather be something that the visualize editor directive now takes care about (and rather expose a callback we'll trigger every time we start a fetch?

Also why do we in general need to save the app state when we do a fetch?

we need to propagate changes to the URL ...

i would prefer if visualize wouldn't know about appState but would rather receive a callback which it should call on props updates, and then visualize app can figure out if it wants to add it to url or not.

however i would suggest in a separate PR ?

timroes · 2018-05-24T11:31:25Z

src/ui/public/visualize/visualize.js

-            stateMonitor.destroy();
-          });
-        }
+        $scope.$watchGroup(['filters', 'query', 'timeRange'], fetch);


Shouldn't that be $scope.fetch?

oh .... i guess thats the reason watch is not triggering :D lol, thanks!

elasticmachine · 2018-05-24T13:58:09Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-05-24T19:12:12Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-06-08T18:48:56Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-06-08T21:22:36Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

stacey-gammon · 2018-06-12T15:11:20Z

hmm, synced with the latest and also cannot repro that bug anymore @ppisljar.

spalger

I'm super happy to see this working with so few lines changed (by some standards). Couple nits/questions but I pulled the code and things seem to work great!

spalger · 2018-06-12T23:37:52Z

src/ui/public/vis/request_handlers/courier.js

-              if (_.has(agg, 'type.postFlightRequest')) {
-                const nestedSearchSource = new SearchSource().inherits(requestSearchSource);
-                resp = await agg.type.postFlightRequest(resp, vis.aggs, agg, nestedSearchSource);
+      return new Promise(async (resolve, reject) => {


No need for async here

spalger · 2018-06-12T23:37:57Z

src/ui/public/vis/request_handlers/courier.js

-        searchSource.set('filter', queryFilter.getFilters());
-        searchSource.set('query', appState.query);
-      }
+      requestSearchSource.set('filter', filters);


Is this resolved? Best I can tell this is working.

spalger · 2018-06-12T23:45:24Z

src/ui/public/timefilter/get_time.js

+
+export function getTime(indexPattern, timeRange, forceNow) {
+  if (!indexPattern) {
+    //in CI, we sometimes seem to fail here.


😬 Can you justify this a little better please? Not sure what this means by "sometimes seem to fail", but if you mean that some tests aren't properly stubbing indexPattern and that's causing issues I think we should update the indexPattern stubs, or stub out this method in tests, or something other than return undefined if an index patter isn't passed. This type of silent failing has a tendency to bite back in weird ways down the road.

no idea, just extracted this to its own file, i didn't write this code :)

elasticmachine · 2018-06-13T06:02:59Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

All changes implemented

elasticmachine · 2018-06-13T09:31:55Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

ppisljar added WIP Work in progress Feature:Visualizations Generic visualization features (in case no more specific feature label is available) labels May 17, 2018

ppisljar force-pushed the fix/explicitPassing branch 3 times, most recently from 863c153 to c5473b9 Compare May 21, 2018 15:44

elastic deleted a comment from elasticmachine May 21, 2018

ppisljar force-pushed the fix/explicitPassing branch 2 times, most recently from a0e0dec to 90e4013 Compare May 21, 2018 17:17

stacey-gammon reviewed May 21, 2018

View reviewed changes

timroes mentioned this pull request May 23, 2018

Filters are not working on iFrames on visualizations #19334

Closed

ppisljar force-pushed the fix/explicitPassing branch from f986fa3 to 397f3cc Compare May 23, 2018 18:07

timroes reviewed May 24, 2018

View reviewed changes

ppisljar added review v6.4.0 and removed WIP Work in progress labels May 24, 2018

ppisljar changed the title ~~WIP: explicitly passing filters and query to visualize~~ explicitly passing filters and query to visualize May 24, 2018

ppisljar requested review from spalger and Bargs May 24, 2018 18:27

ppisljar added 4 commits June 8, 2018 11:10

applying feedback from timroes

ed38367

merging with master

b6800e7

fixing issues tim found

3a7fe8c

does this fix all the weird issues ?

c8822cd

ppisljar force-pushed the fix/explicitPassing branch from 024b516 to c8822cd Compare June 8, 2018 17:38

last suggestion by tim

c6db1fe

stacey-gammon approved these changes Jun 12, 2018

View reviewed changes

spalger approved these changes Jun 13, 2018

View reviewed changes

removing unnecesarry async

0388f37

ppisljar mentioned this pull request Jun 13, 2018

fixing embed mode #16642

Closed

adding documentation

1e9ebfe

ppisljar merged commit 88bb012 into elastic:master Jun 13, 2018

ppisljar mentioned this pull request Jun 13, 2018

[6.x] explicitly passing filters and query to visualize (#19172) #19848

Merged

ppisljar added a commit to ppisljar/kibana that referenced this pull request Jun 13, 2018

explicitly passing filters and query to visualize (elastic#19172)

bc294b1

ppisljar deleted the fix/explicitPassing branch June 13, 2018 09:40

ppisljar added a commit that referenced this pull request Jun 13, 2018

explicitly passing filters and query to visualize (#19172) (#19848)

21e449b

maryia-lapata pushed a commit to maryia-lapata/kibana that referenced this pull request Jun 25, 2018

explicitly passing filters and query to visualize (elastic#19172)

1dba5aa

Bargs mentioned this pull request Aug 10, 2018

If 'ignoreFilterIfFieldNotInIndex' set to 'true', global time filter is not working in Discover. #21358

Closed

ppisljar restored the fix/explicitPassing branch September 26, 2018 10:08

timroes mentioned this pull request Nov 14, 2018

[6.4.0] $rootScope.$listen is not a function error while trying to get VisTypes list #23278

Closed

timroes mentioned this pull request Dec 12, 2018

Use the Canvas rendering pipeline/expressions for all visualizations #19813

Closed

58 tasks

This was referenced Dec 14, 2018

Clicking the search button in dashboard no longer forces refresh #23329

Closed

Fix dashboard to refresh visualizations when the refresh button is clicked #27260

Closed

stacey-gammon mentioned this pull request Dec 18, 2018

Fix dashboard to refresh visualizations when the refresh button is clicked #27353

Merged

7 tasks

explicitly passing filters and query to visualize #19172

explicitly passing filters and query to visualize #19172

Conversation

ppisljar commented May 17, 2018 • edited Loading

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

elasticmachine commented May 21, 2018

💔 Build Failed

elasticmachine commented May 21, 2018

💔 Build Failed

elasticmachine commented May 21, 2018

💔 Build Failed

elasticmachine commented May 22, 2018

💔 Build Failed

elasticmachine commented May 22, 2018

💔 Build Failed

elasticmachine commented May 23, 2018

💔 Build Failed

timroes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalger May 25, 2018 • edited Loading

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

spalger May 25, 2018 • edited Loading

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

elasticmachine commented May 24, 2018

💔 Build Failed

elasticmachine commented May 24, 2018

💔 Build Failed

elasticmachine commented Jun 8, 2018

💚 Build Succeeded

elasticmachine commented Jun 8, 2018

💚 Build Succeeded

stacey-gammon commented Jun 12, 2018

spalger 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

elasticmachine commented Jun 13, 2018

💚 Build Succeeded

elasticmachine commented Jun 13, 2018

💚 Build Succeeded

ppisljar commented May 17, 2018 •

edited

Loading

spalger May 25, 2018 •

edited

Loading

spalger May 25, 2018 •

edited

Loading