Fix visualization individual timeranges #17123

timroes · 2018-03-13T15:08:04Z

Fixes #16823 and also touches the first parts of #16641

With this PR we make a step towards passing down all required information explicitly into the request handler instead of relying on searchSource inheritance or requiring the request handler to use timefilter directly to get the active bounds.

This is needed for the long term plan, to get rid of the inheritance of searchSource altogether and always require all information to be specified in the loader. In the end the dashboard embeddable should also pass us the timerange in, this is currently blocked, since panels don't rerender when the timefilter changes. This will be fixed in a later point.

This code still contains two TODOs, that will be needed to resolved at a later point.

Delete the removal of the timerange filter in courier request handler from its parent searchSource, once unify the way global context is passed down to visualize #16641 is completely fixed and we don't have inheritance anymore.
Remove reading out the timefilter.time in visualize.js as soon as the dashboard embeddables always pass us the time.

API changes

This also changes the API of request handlers. A request handler, will now be passed in the following parameters:

requestHandler(vis, params), with params containing the following keys: appState, uiState, queryFilter, searchSource, timeRange.

Be aware, that searchSource and queryFilter will be removed in the future and instead the query and all filters will be passed as separate keys explicitly into the params.

The timeRange should ALWAYS be used, and no request handler should be talking to the timefilter directly anymore. The timeRange is an object containing at least a from and now key, which can be strings in the datemath syntax. To get timestamps either use the datemath library directly or use timefilter.calculateBounds(timeRange) instead.

Note to QA

This PR changes the basic usage of time range in all visualizations. It has a high potential of breaking existing functionality, since this change affects every single visualization. If you find any errors, that could be related to the time range of the visualization not being used correctly, please feel free to suspect this PR and contact me immediately to help debugging it.

Update: I'am already marking that as a blocker (though this should all be done till feature freeze most likely anyway), since we would introduce a regression when using the vis loader, if this isn't merged, and specifying time ranges, wouldn't work anymore.

elasticmachine · 2018-03-13T16:04:22Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-03-13T16:27:34Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

timroes · 2018-03-13T19:02:19Z

Jenkins, test this - very weird test failure calculateBounds is not a function even though it works locally 100% fine, and I don't see what wrong object it could have.

nyurik · 2018-03-13T19:21:54Z

Vega portion seems reasonable, thx!

elasticmachine · 2018-03-13T19:46:30Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-03-14T10:11:40Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-03-15T22:34:00Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-03-16T12:16:04Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

timroes · 2018-03-19T14:10:37Z

Jenkins, test this (somehow the push didn't trigger a build)

timroes · 2018-03-19T14:23:25Z

Jenkins, test this

elasticmachine · 2018-03-19T15:06:08Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-03-20T11:08:06Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

timroes · 2018-03-20T14:45:40Z

Jenkins, test this - just to make sure it's a consistent failure

elasticmachine · 2018-03-20T15:31:50Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-03-21T12:19:26Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

timroes · 2018-03-21T13:03:12Z

Jenkins, test this

elasticmachine · 2018-03-21T13:50:09Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

timroes · 2018-03-21T15:23:32Z

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

@@ -699,6 +699,10 @@ function discoverController(
        return $scope.vis.getAggConfig().toDsl();
      });
    }
+


Since the date histogram in the chart looks into this variable expected to be set by visualize.js we need to set it explicitly in the places we use <visualization> directly. This is currently only discover. I think in the long run with the new chart component in EUI we should rather use the chart component directly and not any part of the visualize stack anymore in Discover, since discover and visualize already have a ton of special handling just for this one chart.

timroes · 2018-03-21T15:23:54Z

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

@@ -71,6 +71,7 @@
    app-state="state"
    editor-mode="chrome.getVisible()"
    show-spy-panel="chrome.getVisible()"
+    time-range="timeRange"


Pass down the timeRange explicitly from the editor app.

timroes · 2018-03-21T15:25:26Z

src/core_plugins/metrics/public/kbn_vis_types/request_handler.js

@@ -8,25 +8,25 @@ const MetricsRequestHandlerProvider = function (Private, Notifier, config, timef

  return {
    name: 'metrics',
-    handler: function (vis, appState, uiState) {
+    handler: function (vis, { uiState, timeRange }) {


Make TSVB request handler only use the passed timeRange and not looking into timefilter directly anymore.

timroes · 2018-03-21T15:26:17Z

src/core_plugins/timelion/public/vis/timelion_request_handler.js

@@ -12,28 +12,20 @@ const TimelionRequestHandlerProvider = function (Private, Notifier, $http, $root

  return {
    name: 'timelion',
-    handler: function (vis /*, appState, uiState, queryFilter*/) {
+    handler: function (vis, { timeRange }) {


Make timelion request handler only use the explicitly passed timeRange instead of accessing timefilter.time directly.

timroes · 2018-03-21T15:26:45Z

src/core_plugins/vega/public/vega_request_handler.js

@@ -13,7 +13,8 @@ export function VegaRequestHandlerProvider(Private, es, timefilter, serviceSetti

    name: 'vega',

-    handler(vis) {
+    handler(vis, { timeRange }) {
+      timeCache.setTimeRange(timeRange);


Pass in the timeRange that the vega request handler got into the timecache, that is used.

timroes · 2018-03-21T15:28:39Z

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

-    handler: function (vis, appState, uiState, queryFilter, searchSource) {
+    handler: function (vis, { appState, queryFilter, searchSource, timeRange }) {
+
+      searchSource.filter(() => {


Move applying the timeRange filter on searchSource into the actual courier request handler.

In the long run we want to remove searchSource altogether from visualize, since this is courier request handler specific.

stacey-gammon · 2018-03-21T18:47:46Z

I think tilemaps might need some extra logic

on master, the map is empty

... btw this works on a hard refresh, to repro, zoom in from a visualization or using the time range while the dashboard is open.

stacey-gammon · 2018-03-21T18:56:33Z

Everything else seems to be working well! btw reporting dashboard snapshots would have been great for this change. Probably would have caught the tilemap thing, too bad they are turned off! I'll get them back on at some point.

timroes · 2018-03-22T10:38:16Z

@stacey-gammon I think that's actually a bug on master in the maps and not within this change. Whenever I try to change timeranges from a range that had data to a range that doesn't have data, the maps are just ignoring the new data and keep their state (because isDataUsable returns false for the new data).

Could you please check if you can reproduce it on master, too with that information? In that case I would just open a separate ticket for that. Switching from one range with data to another with data seems to work fine for me.

stacey-gammon

yup you're right, bug on master! lgtm

timroes · 2018-03-22T13:03:45Z

Opened #17327 for that issue.

thomasneirynck

to confirm: as for the remaining map-bug, I'm wrapping this up in a larger PR with another bug fix #17263.

* Fix visualization individual timeranges * Make Vega request handler use passed in timeRange * Remove unneeded private variable * Use timeRange for courier caching check * Fix developer documentation * Fix date_histogram * Fix issue * Fix broken tests * Fix issue in discover visualization * Fix vega tests * Fix issue with saved search visualizations * Update timeRange correctly in editor

elasticmachine · 2018-03-27T10:47:30Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

timroes added bug Fixes for quality problems that affect the customer experience WIP Work in progress Feature:Visualizations Generic visualization features (in case no more specific feature label is available) labels Mar 13, 2018

timroes added v7.0.0 v6.3.0 blocker labels Mar 16, 2018

timroes force-pushed the vis-timeranges branch from 9c8d3af to f332911 Compare March 16, 2018 11:49

timroes and others added 11 commits March 20, 2018 08:21

Fix visualization individual timeranges

bae4f14

Make Vega request handler use passed in timeRange

2661482

Remove unneeded private variable

e59c43e

Use timeRange for courier caching check

105c1cd

Fix developer documentation

877a7e3

Fix date_histogram

78289cd

Fix issue

7a579b9

Fix broken tests

edd5482

Fix issue in discover visualization

28291a0

Fix vega tests

ba9a882

Fix issue with saved search visualizations

0e3b5f1

timroes force-pushed the vis-timeranges branch from aa5c506 to 0e3b5f1 Compare March 20, 2018 10:21

Update timeRange correctly in editor

5b65c91

thomasneirynck self-requested a review March 21, 2018 14:28

timroes requested review from kobelb and stacey-gammon March 21, 2018 15:17

timroes commented Mar 21, 2018

View reviewed changes

timroes added review and removed WIP Work in progress labels Mar 21, 2018

stacey-gammon approved these changes Mar 22, 2018

View reviewed changes

thomasneirynck approved these changes Mar 23, 2018

View reviewed changes

timroes merged commit a2fd080 into elastic:master Mar 25, 2018

timroes deleted the vis-timeranges branch March 25, 2018 18:27

timroes mentioned this pull request Mar 25, 2018

[6.x] Fix visualization individual timeranges (#17123) #17383

Merged

This was referenced Apr 10, 2018

Filters applied to visualizations are lost when viewing on a dashboard #17644

Closed

New type of filter to get all the values for a field within the current context #17571

Closed

LeeDr mentioned this pull request May 18, 2018

filter on time field isn't applied to visualizations #19216

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix visualization individual timeranges #17123

Fix visualization individual timeranges #17123

timroes commented Mar 13, 2018 •

edited

Loading

elasticmachine commented Mar 13, 2018

elasticmachine commented Mar 13, 2018

timroes commented Mar 13, 2018

nyurik commented Mar 13, 2018

elasticmachine commented Mar 13, 2018

elasticmachine commented Mar 14, 2018

elasticmachine commented Mar 15, 2018

elasticmachine commented Mar 16, 2018

timroes commented Mar 19, 2018 •

edited

Loading

timroes commented Mar 19, 2018

elasticmachine commented Mar 19, 2018

elasticmachine commented Mar 20, 2018

timroes commented Mar 20, 2018

elasticmachine commented Mar 20, 2018

elasticmachine commented Mar 21, 2018

timroes commented Mar 21, 2018

elasticmachine commented Mar 21, 2018

timroes Mar 21, 2018

timroes Mar 21, 2018

timroes Mar 21, 2018

timroes Mar 21, 2018

timroes Mar 21, 2018

timroes Mar 21, 2018

stacey-gammon commented Mar 21, 2018 •

edited

Loading

stacey-gammon commented Mar 21, 2018

timroes commented Mar 22, 2018

stacey-gammon left a comment

timroes commented Mar 22, 2018

thomasneirynck left a comment

elasticmachine commented Mar 27, 2018

Fix visualization individual timeranges #17123

Fix visualization individual timeranges #17123

Conversation

timroes commented Mar 13, 2018 • edited Loading

API changes

Note to QA

elasticmachine commented Mar 13, 2018

💔 Build Failed

elasticmachine commented Mar 13, 2018

💔 Build Failed

timroes commented Mar 13, 2018

nyurik commented Mar 13, 2018

elasticmachine commented Mar 13, 2018

💔 Build Failed

elasticmachine commented Mar 14, 2018

💔 Build Failed

elasticmachine commented Mar 15, 2018

💔 Build Failed

elasticmachine commented Mar 16, 2018

💔 Build Failed

timroes commented Mar 19, 2018 • edited Loading

timroes commented Mar 19, 2018

elasticmachine commented Mar 19, 2018

💔 Build Failed

elasticmachine commented Mar 20, 2018

💔 Build Failed

timroes commented Mar 20, 2018

elasticmachine commented Mar 20, 2018

💔 Build Failed

elasticmachine commented Mar 21, 2018

💔 Build Failed

timroes commented Mar 21, 2018

elasticmachine commented Mar 21, 2018

💚 Build Succeeded

timroes Mar 21, 2018

Choose a reason for hiding this comment

timroes Mar 21, 2018

Choose a reason for hiding this comment

timroes Mar 21, 2018

Choose a reason for hiding this comment

timroes Mar 21, 2018

Choose a reason for hiding this comment

timroes Mar 21, 2018

Choose a reason for hiding this comment

timroes Mar 21, 2018

Choose a reason for hiding this comment

stacey-gammon commented Mar 21, 2018 • edited Loading

stacey-gammon commented Mar 21, 2018

timroes commented Mar 22, 2018

stacey-gammon left a comment

Choose a reason for hiding this comment

timroes commented Mar 22, 2018

thomasneirynck left a comment

Choose a reason for hiding this comment

elasticmachine commented Mar 27, 2018

💚 Build Succeeded

timroes commented Mar 13, 2018 •

edited

Loading

timroes commented Mar 19, 2018 •

edited

Loading

stacey-gammon commented Mar 21, 2018 •

edited

Loading