Fix map updates not propagating to the dashboard by stacey-gammon · Pull Request #13589 · elastic/kibana

stacey-gammon · 2017-08-18T19:43:40Z

…ation.

stacey-gammon · 2017-08-21T15:32:30Z

@ppisljar and @nreese - While this fixes the dashboard ui state issue, I suspect I may be breaking something else with map bounds. Can one of you take a look and lmk if that is the case? I can try to add an additional test if I know what to look for that is broken.

nreese · 2017-08-21T16:17:34Z

@stacey-gammon This change breaks the filter aggregation. Since there are no mapBounds, the filter aggregation can never be generated.

There needs to be additional _tile_map functional tests to ensure that the filter aggregation is applied when isFilteredByCollar is checked. The best way to check this is to use the spy tab. The table headings should be filter geohash_grid Count Geo Centroid or the actual request could be inspected.

Additionally, it would be nice if the geo_centroid aggregation is tested to verify it is turned on/off when using setting the useGeocentroid option.

…r agg

…g temporary "sessionState" on a vis

ppisljar · 2017-08-22T08:30:24Z

thanks Stacey! seems to work OK

the failing test is probably due to not

await PageObjects.visualize.clickGo();
await PageObjects.header.waitUntilLoadingHasFinished();

after you toggle the checkbox

nreese · 2017-08-22T14:26:08Z

          const boundingBox = {};
          delete mapCollar.zoom; // zoom is not part of bounding box filter
          boundingBox[agg.getField().name] = mapCollar;
+          vis.sessionState.mapCollar = mapCollar;


A new _msearch request is created anytime the aggregation changes. By updating the mapCollar every time, the aggregation changes any time the map is moved. This results in an _msearch request on each map movement which is not the intended behavior.

vis.sessionState.mapCollar should only be updated when the mapBounds are outside of the last fetched map collar and new data needs to be fetched from elasticsearch.

add more tests to ensure getRequestAggs functions as intented

stacey-gammon · 2017-08-23T11:42:20Z

Failed on:

07:00:03.733        └-> should save with zoom level and load, take screenshot
07:00:03.733          └-> "before each" hook: global before each
07:00:03.738            │ debg  Kibana uiSettings are in elasticsearch and the server is reporting a green status
07:00:03.738          │ debg  in findAllByCssSelector: a.leaflet-control-zoom-in
07:00:03.995          │ debg  isGlobalLoadingIndicatorVisible
07:00:04.003          │ debg  TestSubjects.exists(globalLoadingIndicator)
07:00:04.004          │ debg  existsByDisplayedByCssSelector [data-test-subj~="globalLoadingIndicator"]
07:00:05.017          │ debg  isGlobalLoadingIndicatorHidden
07:00:05.018          │ debg  findByCssSelector [data-test-subj="globalLoadingIndicator"].ng-hide
07:00:05.027          │ debg  in findAllByCssSelector: a.leaflet-control-zoom-in
07:00:05.071          │ debg  isGlobalLoadingIndicatorVisible
07:00:05.072          │ debg  TestSubjects.exists(globalLoadingIndicator)
07:00:05.072          │ debg  existsByDisplayedByCssSelector [data-test-subj~="globalLoadingIndicator"]
07:00:06.080          │ debg  isGlobalLoadingIndicatorHidden
07:00:06.081          │ debg  findByCssSelector [data-test-subj="globalLoadingIndicator"].ng-hide
07:00:06.092          │ debg  TestSubjects.find(visualizeSaveButton)
07:00:06.093          │ debg  in displayedByCssSelector: [data-test-subj~="visualizeSaveButton"]
07:00:06.239          │ debg  saveButton button clicked
07:00:06.240          │ debg  find.byName(visTitle)
07:00:06.436          │ debg  click submit button
07:00:06.436          │ debg  TestSubjects.find(saveVisualizationButton)
07:00:06.437          │ debg  in displayedByCssSelector: [data-test-subj~="saveVisualizationButton"]
07:00:06.600          │ debg  isGlobalLoadingIndicatorVisible
07:00:06.601          │ debg  TestSubjects.exists(globalLoadingIndicator)
07:00:06.601          │ debg  existsByDisplayedByCssSelector [data-test-subj~="globalLoadingIndicator"]
07:00:06.711          │ debg  isGlobalLoadingIndicatorHidden
07:00:06.712          │ debg  findByCssSelector [data-test-subj="globalLoadingIndicator"].ng-hide
07:00:06.778          │ debg  in displayedByCssSelector: kbn-truncated.toast-message.ng-isolate-scope
07:00:06.860          │ debg  Saved viz message = Visualization Editor: Saved Visualization "Visualization TileMap"
07:00:11.750          │ debg  openSpyPanel
07:00:11.751          │ debg  TestSubjects.exists(spyModeSelect)
07:00:11.751          │ debg  existsByDisplayedByCssSelector [data-test-subj~="spyModeSelect"]
07:00:12.795          │ debg  TestSubjects.find(spyToggleButton)
07:00:12.796          │ debg  in displayedByCssSelector: [data-test-subj~="spyToggleButton"]
07:00:12.978          │ debg  TestSubjects.find(spyModeSelect)
07:00:12.981          │ debg  in displayedByCssSelector: [data-test-subj~="spyModeSelect"]
07:00:13.010          │ debg  first get the zoom level 5 page data and verify it
07:00:13.011          │ debg  TestSubjects.find(paginated-table-body)
07:00:13.011          │ debg  in displayedByCssSelector: [data-test-subj~="paginated-table-body"]
07:00:13.199          │ debg  Taking screenshot "/var/lib/jenkins/workspace/elastic+kibana+pull-request+multijob-selenium/test/functional/screenshots/failure/visualize app tile map visualize app tile map chart should save with zoom level and load, take screenshot.png"
07:00:13.401        └- ✖ fail: "visualize app tile map visualize app tile map chart should save with zoom level and load, take screenshot"
07:00:13.403        │       
07:00:13.404        │         Error: expected [ { geohash: 'dr4', count: '127', lat: 40, lon: -76 },
07:00:13.404        │         { geohash: 'dr7', count: '92', lat: 41, lon: -74 },
07:00:13.404        │         { geohash: '9q5', count: '91', lat: 34, lon: -119 },
07:00:13.404        │         { geohash: '9qc', count: '89', lat: 38, lon: -122 },
07:00:13.404        │         { geohash: 'drk', count: '87', lat: 41, lon: -73 },
07:00:13.404        │         { geohash: 'dps', count: '82', lat: 42, lon: -84 },
07:00:13.404        │         { geohash: 'dph', count: '82', lat: 40, lon: -84 },
07:00:13.404        │         { geohash: 'dp3', count: '79', lat: 41, lon: -88 },
07:00:13.404        │         { geohash: 'dpe', count: '78', lat: 42, lon: -86 },
07:00:13.404        │         { geohash: 'dp8', count: '77', lat: 43, lon: -90 } ] to sort of equal [ { geohash: '9q5', count: '91', lat: 34, lon: -119 },
07:00:13.404        │         { geohash: '9qc', count: '89', lat: 38, lon: -122 },
07:00:13.404        │         { geohash: 'dp3', count: '79', lat: 41, lon: -88 },
07:00:13.404        │         { geohash: 'dp8', count: '77', lat: 43, lon: -90 },
07:00:13.404        │         { geohash: 'dp6', count: '74', lat: 41, lon: -87 },
07:00:13.404        │         { geohash: '9qh', count: '74', lat: 34, lon: -118 },
07:00:13.404        │         { geohash: '9y7', count: '73', lat: 35, lon: -97 },
07:00:13.404        │         { geohash: '9ys', count: '71', lat: 37, lon: -95 },
07:00:13.404        │         { geohash: '9yn', count: '71', lat: 34, lon: -93 },
07:00:13.404        │         { geohash: '9q9', count: '70', lat: 37, lon: -122 } ]
07:00:13.404        │         + expected - actual
07:00:13.404        │       
07:00:13.404        │          [
07:00:13.405        │            {
07:00:13.405        │         -    "count": "127"
07:00:13.405        │         -    "geohash": "dr4"
07:00:13.405        │         -    "lat": 40
07:00:13.405        │         -    "lon": -76
07:00:13.405        │         -  }
07:00:13.405        │         -  {
07:00:13.405        │         -    "count": "92"
07:00:13.405        │         -    "geohash": "dr7"
07:00:13.405        │         -    "lat": 41
07:00:13.405        │         -    "lon": -74
07:00:13.405        │         -  }
07:00:13.405        │         -  {
07:00:13.405        │              "count": "91"
07:00:13.405        │              "geohash": "9q5"
07:00:13.405        │              "lat": 34
07:00:13.405        │              "lon": -119
07:00:13.405        │              "lat": 38
07:00:13.405        │              "lon": -122
07:00:13.405        │            }
07:00:13.405        │            {
07:00:13.405        │         -    "count": "87"
07:00:13.405        │         -    "geohash": "drk"
07:00:13.406        │         +    "count": "79"
07:00:13.406        │         +    "geohash": "dp3"
07:00:13.406        │              "lat": 41
07:00:13.406        │         -    "lon": -73
07:00:13.406        │         +    "lon": -88
07:00:13.406        │            }
07:00:13.406        │            {
07:00:13.406        │         -    "count": "82"
07:00:13.406        │         -    "geohash": "dps"
07:00:13.406        │         -    "lat": 42
07:00:13.406        │         -    "lon": -84
07:00:13.406        │         +    "count": "77"
07:00:13.406        │         +    "geohash": "dp8"
07:00:13.406        │         +    "lat": 43
07:00:13.406        │         +    "lon": -90
07:00:13.406        │            }
07:00:13.406        │            {
07:00:13.406        │         -    "count": "82"
07:00:13.406        │         -    "geohash": "dph"
07:00:13.406        │         -    "lat": 40
07:00:13.406        │         -    "lon": -84
07:00:13.406        │         +    "count": "74"
07:00:13.406        │         +    "geohash": "dp6"
07:00:13.407        │         +    "lat": 41
07:00:13.407        │         +    "lon": -87
07:00:13.407        │            }
07:00:13.407        │            {
07:00:13.407        │         -    "count": "79"
07:00:13.407        │         -    "geohash": "dp3"
07:00:13.407        │         -    "lat": 41
07:00:13.407        │         -    "lon": -88
07:00:13.407        │         +    "count": "74"
07:00:13.407        │         +    "geohash": "9qh"
07:00:13.407        │         +    "lat": 34
07:00:13.407        │         +    "lon": -118
07:00:13.407        │            }
07:00:13.407        │            {
07:00:13.407        │         -    "count": "78"
07:00:13.407        │         -    "geohash": "dpe"
07:00:13.407        │         -    "lat": 42
07:00:13.407        │         -    "lon": -86
07:00:13.407        │         +    "count": "73"
07:00:13.407        │         +    "geohash": "9y7"
07:00:13.407        │         +    "lat": 35
07:00:13.407        │         +    "lon": -97
07:00:13.407        │            }
07:00:13.408        │            {
07:00:13.408        │         -    "count": "77"
07:00:13.408        │         -    "geohash": "dp8"
07:00:13.408        │         -    "lat": 43
07:00:13.408        │         -    "lon": -90
07:00:13.408        │         +    "count": "71"
07:00:13.408        │         +    "geohash": "9ys"
07:00:13.408        │         +    "lat": 37
07:00:13.408        │         +    "lon": -95
07:00:13.408        │            }
07:00:13.408        │         +  {
07:00:13.408        │         +    "count": "71"
07:00:13.408        │         +    "geohash": "9yn"
07:00:13.408        │         +    "lat": 34
07:00:13.408        │         +    "lon": -93
07:00:13.408        │         +  }
07:00:13.408        │         +  {
07:00:13.408        │         +    "count": "70"
07:00:13.408        │         +    "geohash": "9q9"
07:00:13.408        │         +    "lat": 37
07:00:13.408        │         +    "lon": -122
07:00:13.409        │         +  }
07:00:13.409        │          ]
07:00:13.409        │         
07:00:13.409        │         at Assertion.assert (node_modules/expect.js/index.js:96:13)
07:00:13.409        │         at Assertion.eql (node_modules/expect.js/index.js:230:10)
07:00:13.409        │         at compareTableData (test/functional/apps/visualize/_tile_map.js:70:39)
07:00:13.409        │         at showData (test/functional/apps/visualize/_tile_map.js:231:11)
07:00:13.409        │         at process._tickDomainCallback (internal/process/next_tick.js:135:7)
07:00:13.409        │       
07:00:13.409        │

And screenshot:

…ap-overriding-dashboard-state

stacey-gammon · 2017-08-23T17:06:19Z

Couple issues going on with the latest implementation:

Zoom values don't seem correct (or at least don't match old zoom values)
- To fix this I 'm going to go back to the old zoom calculations for now, as long as that doesn't mess up dashboard state.
Since we aren't storing mapCollar in uiState anymore, this means it will get re-calculated on a page refresh/url change. This is also a cause of the test breakage, because after the visualization save, it's recalculated. I think everything is essentially as correct as it was before, but probably due to the re-calculation of the bounds, the returned results vary slightly.

@nreese, @ppisljar - I think we should just update the data used in the test to match the new results, and live with the fact that mapCollar is recalculated on a page refresh/url change. What do you guys think?

nreese · 2017-08-23T17:33:23Z

@stacey-gammon I think its a good idea to update the test data. It makes sense that the mapCollar is recalculated on page refresh/url change. Since fresh data is being requested from ES, then this is an improvement over the old implementation since now the mapCollar is re-centered around the map when data is first pulled.

I think because mapCollar is no longer saved in uiState, the save recenters the data and we get slightly different data points from the test data. As far as my eye can tell, everything is working as intended.

stacey-gammon · 2017-08-23T19:57:53Z

I think there might be something else going on. I updated the data, but it still doesn't work. The test checks that the data goes from A -> B and back to A after zooming back out. Now it seems to go to A' -> B -> A (back to the original A data from the original test, not A').

This doesn't quite make sense. I'll have to dig more into it.

…ap-overriding-dashboard-state

stacey-gammon · 2017-08-28T19:45:39Z

There is an actual issue with this PR. The filter is lost when a visualization is saved for the first time. This does not happen on master.

On master:

On this PR:

Perhaps updating the uiState with mapBounds causes the second tabify agg response on master?

stacey-gammon · 2017-08-29T01:02:07Z

I think the problem is that we assume a visualization plugin's request has no dependency on the visualization UI, but this is not the case with tile maps.

What happens on master is that a request is made to es, with no map collar. The visualization is rendered, the map bounds are updated in uistate, this triggers a new es request, and a second request is made with the map collar filter applied this time.

Because in this PR I'm not storing the map bounds in the ui state anymore, this doesn't trigger the second es request.

Few ways I can think to fix this:

Introduce some sort of init function that is called before the request handler, which will grant access to the ui element, so the vis plugin can do what they want in there - e.g. the vis map can render an empty tile map, grab the bounds, and put that on the session state, which will then be used by the initial request.
Introduce some sort of manual way for a visualization to say "refresh the request". So instead of relying on uiState to handle this, we can trigger it manually. Maybe there is already a way to do this that I am unaware of.
Implement 2. above, and also add a way for tilemaps to skip any es requests that don't have map bounds set because it means there is no UI to render.

@ppisljar - you are probably the most familiar with the visualization plugin system. Do you have any ideas about the above? Ideally we'd be able to fix this by having all the required information before the first request goes out to es so we aren't unnecessarily sending an extra request.

cc @nreese

ppisljar · 2017-08-29T07:26:11Z

visualization constructor should probably do that ... we would need to make sure visualize waits for the constructor to finish execution ? and only then executes the request handler
is already there, you can call vis.forceReload()
introducing a flag on visualization definition skipFirstRequest should be super easy

ppisljar · 2017-08-29T07:34:08Z

i've put a PR up which introduces point 1. from above #13742
number 3. is not needed ... let me know if this would work for you.

…ap-overriding-dashboard-state

- incorporate the new init function which fixes the bug where we lose map bounds data on a fresh save - add a test that would have caught that - adjust tests due to bug where map bounds is changing slightly. File another issue for that separately as it doesn’t actually affect the users map experience.

Tests relied on my original logic of defaulting to the saved zoom state and not relying on uiState, so I went back to that logic. Also found another bug where mapZoom of 0 was being considered invalid, but it is actually a valid zoom level.

…eve circles anymore

I suspect because the page is changing, you have to keep fetching the element afresh. I don’t see this error on my local but saw it on jenkins.

…ap-overriding-dashboard-state

The function is used in the Data Table visualization where the spy pane select doesn’t exist.

thomasneirynck · 2017-09-05T10:30:46Z

jenkins, test this

stacey-gammon · 2017-09-05T17:48:20Z

jenkins, test this

issue in ES broke master builds, esvm should be rolled back now.

stacey-gammon · 2017-09-05T20:42:12Z

s3 publish error

jenkins, test this

stacey-gammon · 2017-09-06T13:39:33Z

@nreese, @ppisljar - should now be ready for a final review.

nreese

LGTM

ppisljar

LGTM

* Add failing tests * Add fix by preventing uiState from being directly updated in visualization. * Add test that would catch error caused by this PR in regards to filter agg * Fix issue with uiState triggering dirty dashboard state by introducing temporary "sessionState" on a vis * Click go after toggling the switch * add more tests to ensure getRequestAggs functions as intented * Go back to old zoom calculations. Update vis test data I think because mapCollar is no longer saved in uiState, the save recenters the data and we get slightly different data points from the test data. As far as my eye can tell, everything is working as intended. * fixes and tests - incorporate the new init function which fixes the bug where we lose map bounds data on a fresh save - add a test that would have caught that - adjust tests due to bug where map bounds is changing slightly. File another issue for that separately as it doesn’t actually affect the users map experience. * Fix tests Tests relied on my original logic of defaulting to the saved zoom state and not relying on uiState, so I went back to that logic. Also found another bug where mapZoom of 0 was being considered invalid, but it is actually a valid zoom level. * Since leaflet upgrade 'path.leaflet-clickable' can't be used to retrieve circles anymore * Avoid stale element reference I suspect because the page is changing, you have to keep fetching the element afresh. I don’t see this error on my local but saw it on jenkins. * remove spy select from PageObjects.visualize.getDataTableData The function is used in the Data Table visualization where the spy pane select doesn’t exist.

stacey-gammon added Feature:Dashboard Dashboard related features :Sharing labels Aug 18, 2017

stacey-gammon force-pushed the fix/map-overriding-dashboard-state branch from 6c66651 to 0fe1e5c Compare August 19, 2017 11:54

Add failing tests

0c62545

stacey-gammon force-pushed the fix/map-overriding-dashboard-state branch from 0fe1e5c to 0c62545 Compare August 19, 2017 11:54

Add fix by preventing uiState from being directly updated in visualiz…

c7dea45

…ation.

stacey-gammon requested review from nreese and ppisljar August 21, 2017 15:31

stacey-gammon added the review label Aug 21, 2017

Stacey Gammon added 2 commits August 21, 2017 14:12

Add test that would catch error caused by this PR in regards to filte…

19af2e4

…r agg

Fix issue with uiState triggering dirty dashboard state by introducin…

f73e1fc

…g temporary "sessionState" on a vis

Click go after toggling the switch

625ee73

nreese reviewed Aug 22, 2017

View reviewed changes

nreese and others added 2 commits August 22, 2017 11:46

add more tests to ensure getRequestAggs functions as intented

ace667c

Merge pull request #6 from nreese/more_tests2

cd982e6

add more tests to ensure getRequestAggs functions as intented

Merge branch 'master' of https://github.com/elastic/kibana into fix/m…

4f4b34e

…ap-overriding-dashboard-state

Go back to old zoom calculations. Update vis test data

699482c

I think because mapCollar is no longer saved in uiState, the save recenters the data and we get slightly different data points from the test data. As far as my eye can tell, everything is working as intended.

Merge branch 'master' of https://github.com/elastic/kibana into fix/m…

9acb054

…ap-overriding-dashboard-state

Merge branch 'master' of https://github.com/elastic/kibana into fix/m…

306f688

…ap-overriding-dashboard-state

stacey-gammon force-pushed the fix/map-overriding-dashboard-state branch from ccd1d97 to 7dea2b9 Compare August 31, 2017 15:10

Fix tests

ef591ec

Tests relied on my original logic of defaulting to the saved zoom state and not relying on uiState, so I went back to that logic. Also found another bug where mapZoom of 0 was being considered invalid, but it is actually a valid zoom level.

stacey-gammon force-pushed the fix/map-overriding-dashboard-state branch from 2b8351a to ef591ec Compare August 31, 2017 18:52

Stacey Gammon added 2 commits September 1, 2017 10:17

Since leaflet upgrade 'path.leaflet-clickable' can't be used to retri…

06ecbaf

…eve circles anymore

Avoid stale element reference

b914c5d

I suspect because the page is changing, you have to keep fetching the element afresh. I don’t see this error on my local but saw it on jenkins.

stacey-gammon force-pushed the fix/map-overriding-dashboard-state branch from e3b46b9 to b914c5d Compare September 1, 2017 16:29

Stacey Gammon added 2 commits September 1, 2017 13:28

Merge branch 'master' of https://github.com/elastic/kibana into fix/m…

b698a00

…ap-overriding-dashboard-state

remove spy select from PageObjects.visualize.getDataTableData

c64cad3

The function is used in the Data Table visualization where the spy pane select doesn’t exist.

stacey-gammon requested a review from thomasneirynck September 6, 2017 13:39

nreese approved these changes Sep 6, 2017

View reviewed changes

ppisljar approved these changes Sep 7, 2017

View reviewed changes

stacey-gammon merged commit d853fca into elastic:master Sep 7, 2017

stacey-gammon added v6.0.0-rc1 v6.1.0 labels Sep 7, 2017

This was referenced Sep 7, 2017

[6.x] Fix map updates not propagating to the dashboard (#13589) #13883

Merged

[6.0] Fix map updates not propagating to the dashboard (#13589) #13884

Merged

stacey-gammon deleted the fix/map-overriding-dashboard-state branch October 24, 2017 13:57

Conversation

stacey-gammon commented Aug 18, 2017

Uh oh!

stacey-gammon commented Aug 21, 2017

Uh oh!

nreese commented Aug 21, 2017

Uh oh!

ppisljar commented Aug 22, 2017

Uh oh!

nreese Aug 22, 2017

Choose a reason for hiding this comment

Uh oh!

stacey-gammon commented Aug 23, 2017

Uh oh!

stacey-gammon commented Aug 23, 2017

Uh oh!

nreese commented Aug 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stacey-gammon commented Aug 23, 2017

Uh oh!

stacey-gammon commented Aug 28, 2017

Uh oh!

stacey-gammon commented Aug 29, 2017

Uh oh!

ppisljar commented Aug 29, 2017

Uh oh!

ppisljar commented Aug 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thomasneirynck commented Sep 5, 2017

Uh oh!

stacey-gammon commented Sep 5, 2017

Uh oh!

stacey-gammon commented Sep 5, 2017

Uh oh!

stacey-gammon commented Sep 6, 2017

Uh oh!

nreese left a comment

Choose a reason for hiding this comment

Uh oh!

ppisljar left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nreese commented Aug 23, 2017 •

edited

Loading

ppisljar commented Aug 29, 2017 •

edited

Loading