[Maps] Do not check count for blended layers when layer is not visible by nreese · Pull Request #66460 · elastic/kibana

nreese · 2020-05-13T19:37:50Z

Steps to view bug:

install sample web logs and create a Documents layer with clustering enabled.
hide layer
Change time range
Notice how _search request is made for the count of documents. Hidden layers should not make any requests.

This PR fixes the issue by consolidating the layer visibility check in map action syncDataForLayer that calls syncData

elasticmachine · 2020-05-13T19:37:52Z

Pinging @elastic/kibana-gis (Team:Geo)

thomasneirynck

thanks, good catch.

Would you mind adding a test for this? imho syncData is core enough for a unit test.

x-pack/plugins/maps/public/classes/layers/blended_vector_layer/blended_vector_layer.ts

nreese · 2020-05-14T13:56:50Z

The PR moves the layer visibility check into map_actions where syncData is called. This removes all the duplicated logic of checking visibility in each syncData implementation.

I am removing 7.8.0 label since this PR is now more complicated then the simple fix.

nreese · 2020-05-14T19:32:05Z

x-pack/test/functional/es_archives/maps/kibana/data.json

        "description" : "",
        "mapStateJSON" : "{\"zoom\":4.1,\"center\":{\"lon\":-100.61091,\"lat\":33.23887},\"timeFilters\":{\"from\":\"2015-09-20T00:00:00.000Z\",\"to\":\"2015-09-24T01:00:00.000Z\"},\"refreshConfig\":{\"isPaused\":true,\"interval\":1000},\"query\":{\"query\":\"\",\"language\":\"kuery\"}}",
-        "layerListJSON" : "[{\"id\":\"0hmz5\",\"sourceDescriptor\":{\"type\":\"EMS_TMS\",\"id\":\"road_map\"},\"visible\":true,\"temporary\":false,\"style\":{\"type\":\"TILE\",\"properties\":{}},\"type\":\"TILE\",\"minZoom\":0,\"maxZoom\":24},{\"id\":\"z52lq\",\"label\":\"logstash\",\"minZoom\":0,\"maxZoom\":24,\"sourceDescriptor\":{\"id\":\"e1a5e1a6-676c-4a89-8ea9-0d91d64b73c6\",\"type\":\"ES_SEARCH\",\"geoField\":\"geo.coordinates\",\"limit\":2048,\"filterByMapBounds\":true,\"showTooltip\":true,\"tooltipProperties\":[],\"topHitsTimeField\":\"@timestamp\",\"useTopHits\":true,\"topHitsSplitField\":\"machine.os.raw\",\"topHitsSize\":2,\"indexPatternRefName\":\"layer_1_source_index_pattern\"},\"visible\":true,\"temporary\":false,\"style\":{\"type\":\"VECTOR\",\"properties\":{\"fillColor\":{\"type\":\"STATIC\",\"options\":{\"color\":\"#e6194b\"}},\"lineColor\":{\"type\":\"STATIC\",\"options\":{\"color\":\"#FFFFFF\"}},\"lineWidth\":{\"type\":\"STATIC\",\"options\":{\"size\":1}},\"iconSize\":{\"type\":\"STATIC\",\"options\":{\"size\":10}}},\"previousStyle\":null},\"type\":\"VECTOR\"}]",
+        "layerListJSON" : "[{\"id\":\"z52lq\",\"label\":\"logstash\",\"minZoom\":0,\"maxZoom\":24,\"sourceDescriptor\":{\"id\":\"e1a5e1a6-676c-4a89-8ea9-0d91d64b73c6\",\"type\":\"ES_SEARCH\",\"geoField\":\"geo.coordinates\",\"limit\":2048,\"filterByMapBounds\":true,\"showTooltip\":true,\"tooltipProperties\":[],\"topHitsTimeField\":\"@timestamp\",\"useTopHits\":true,\"topHitsSplitField\":\"machine.os.raw\",\"topHitsSize\":2,\"indexPatternRefName\":\"layer_1_source_index_pattern\"},\"visible\":true,\"temporary\":false,\"style\":{\"type\":\"VECTOR\",\"properties\":{\"fillColor\":{\"type\":\"STATIC\",\"options\":{\"color\":\"#e6194b\"}},\"lineColor\":{\"type\":\"STATIC\",\"options\":{\"color\":\"#FFFFFF\"}},\"lineWidth\":{\"type\":\"STATIC\",\"options\":{\"size\":1}},\"iconSize\":{\"type\":\"STATIC\",\"options\":{\"size\":10}}},\"previousStyle\":null},\"type\":\"VECTOR\"}]",


Removed EMS base map layer from layerList. Tests using these maps look at mapbox style. EMS base map layer bloats mapbox style making the inspector very slow to render. Removing that layer speeds up tests a lot.

thx, good call. Didn't occur to me, but when we moved to vector-tiles, this added a large amount of layers to the mb-map

nreese · 2020-05-14T22:23:15Z

@elasticmachine merge upstream

thomasneirynck

Can we export syncData and add a test to map_selectors.test.js

Since we're patching, I'd strongly recommend adding a test that verifies this change.

e.g. export syncDataForLayer from map_actions and add something like:

describe('syncDataForLayer', () => {
    let isVisible;
    let showAtZoomLevel;
    let startLoading;
    const mockLayer = {
      isVisible() {
        return isVisible;
      },
      getId() {
        return 'foobar';
      },
      showAtZoomLevel() {
        return showAtZoomLevel;
      },
      syncData() {
        startLoading = true;
      },
    };
    beforeEach(() => {
      startLoading = false;
    });

    it('should not sync when visible layerproperty is false', async () => {
      isVisible = false;
      showAtZoomLevel = true;
      const action = syncDataForLayer(mockLayer);
      await action(dispatchMock, getStoreMock);
      expect(startLoading).toEqual(false);
    });

    it('should not sync when outside zoom range', async () => {
      isVisible = true;
      showAtZoomLevel = false;
      const action = syncDataForLayer(mockLayer);
      await action(dispatchMock, getStoreMock);
      expect(startLoading).toEqual(false);
    });

    it('should only sync when both visible and in zoom range', async () => {
      isVisible = true;
      showAtZoomLevel = true;
      const action = syncDataForLayer(mockLayer);
      await action(dispatchMock, getStoreMock);
      expect(startLoading).toEqual(true);
    });
  });

thomasneirynck · 2020-05-15T18:12:39Z

x-pack/plugins/maps/public/actions/map_actions.js

+function syncDataForLayer(layer) {
+  return async (dispatch, getState) => {
+    const dataFilters = getDataFilters(getState());
+    if (!layer.isVisible() || !layer.showAtZoomLevel(dataFilters.zoom)) {


thanks for pulling this out here. it really helps when the business-logic is gathered in the actions rather than sprinkled across multiple classes.

thomasneirynck · 2020-05-15T18:13:46Z

x-pack/test/functional/es_archives/maps/kibana/data.json

        "description" : "",
        "mapStateJSON" : "{\"zoom\":4.1,\"center\":{\"lon\":-100.61091,\"lat\":33.23887},\"timeFilters\":{\"from\":\"2015-09-20T00:00:00.000Z\",\"to\":\"2015-09-24T01:00:00.000Z\"},\"refreshConfig\":{\"isPaused\":true,\"interval\":1000},\"query\":{\"query\":\"\",\"language\":\"kuery\"}}",
-        "layerListJSON" : "[{\"id\":\"0hmz5\",\"sourceDescriptor\":{\"type\":\"EMS_TMS\",\"id\":\"road_map\"},\"visible\":true,\"temporary\":false,\"style\":{\"type\":\"TILE\",\"properties\":{}},\"type\":\"TILE\",\"minZoom\":0,\"maxZoom\":24},{\"id\":\"z52lq\",\"label\":\"logstash\",\"minZoom\":0,\"maxZoom\":24,\"sourceDescriptor\":{\"id\":\"e1a5e1a6-676c-4a89-8ea9-0d91d64b73c6\",\"type\":\"ES_SEARCH\",\"geoField\":\"geo.coordinates\",\"limit\":2048,\"filterByMapBounds\":true,\"showTooltip\":true,\"tooltipProperties\":[],\"topHitsTimeField\":\"@timestamp\",\"useTopHits\":true,\"topHitsSplitField\":\"machine.os.raw\",\"topHitsSize\":2,\"indexPatternRefName\":\"layer_1_source_index_pattern\"},\"visible\":true,\"temporary\":false,\"style\":{\"type\":\"VECTOR\",\"properties\":{\"fillColor\":{\"type\":\"STATIC\",\"options\":{\"color\":\"#e6194b\"}},\"lineColor\":{\"type\":\"STATIC\",\"options\":{\"color\":\"#FFFFFF\"}},\"lineWidth\":{\"type\":\"STATIC\",\"options\":{\"size\":1}},\"iconSize\":{\"type\":\"STATIC\",\"options\":{\"size\":10}}},\"previousStyle\":null},\"type\":\"VECTOR\"}]",
+        "layerListJSON" : "[{\"id\":\"z52lq\",\"label\":\"logstash\",\"minZoom\":0,\"maxZoom\":24,\"sourceDescriptor\":{\"id\":\"e1a5e1a6-676c-4a89-8ea9-0d91d64b73c6\",\"type\":\"ES_SEARCH\",\"geoField\":\"geo.coordinates\",\"limit\":2048,\"filterByMapBounds\":true,\"showTooltip\":true,\"tooltipProperties\":[],\"topHitsTimeField\":\"@timestamp\",\"useTopHits\":true,\"topHitsSplitField\":\"machine.os.raw\",\"topHitsSize\":2,\"indexPatternRefName\":\"layer_1_source_index_pattern\"},\"visible\":true,\"temporary\":false,\"style\":{\"type\":\"VECTOR\",\"properties\":{\"fillColor\":{\"type\":\"STATIC\",\"options\":{\"color\":\"#e6194b\"}},\"lineColor\":{\"type\":\"STATIC\",\"options\":{\"color\":\"#FFFFFF\"}},\"lineWidth\":{\"type\":\"STATIC\",\"options\":{\"size\":1}},\"iconSize\":{\"type\":\"STATIC\",\"options\":{\"size\":10}}},\"previousStyle\":null},\"type\":\"VECTOR\"}]",


thx, good call. Didn't occur to me, but when we moved to vector-tiles, this added a large amount of layers to the mb-map

nreese · 2020-05-15T20:25:59Z

@elasticmachine merge upstream

nreese · 2020-05-15T20:55:01Z

Can we export syncData and add a test to map_actions.test.js

Those tests will not work when converted to TS. I would like to move this into another PR that starts to break map_actions into smaller files that can be converted to typescript. Then we can add tests to those smaller files. I will get a PR out for this next.

nreese · 2020-05-15T21:38:33Z

created #66821 to track unit tests one data request actions have been pulled out of map_actions

elastic#66460) * [Maps] Do not check count for blended layers when layer is not visible * move visibility logic to map_actions where syncData is called * clean up * fix syntax error * remove async action * make syncDataForAllLayers a proper redux action * simplify * fix functional test Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

#66460) (#66831) * [Maps] Do not check count for blended layers when layer is not visible * move visibility logic to map_actions where syncData is called * clean up * fix syntax error * remove async action * make syncDataForAllLayers a proper redux action * simplify * fix functional test Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

…ine-editor * 'master' of github.com:elastic/kibana: (157 commits) [ML] fix url assertion (#66850) Skip failing lens test(s). #66779 [SOM] Preserve saved object references when saving the object (#66584) Use ES API from start contract (#66157) Reorganize Management apps into Ingest, Data, Alerts and Insights, Security, Kibana, and Stack groups (#65796) [Uptime] Fix flaky navigation to certs page in tests (#66806) [Maps] Do not check count for blended layers when layer is not visible (#66460) [SIEM] Fixes glob patterns from directory changes recently for GraphQL chore(NA): bump static-fs to 1.0.2 (#66775) [Maps] Handle cross cluster index _settings resp (#66797) [SIEM][Lists] Adds 90% of the REST API and client API for exception lists and exception items allow any type for customResponseHeaders config (#66689) [APM] Disable map layout animation (#66763) [ML] Add linking to dataframe from job management tab (#65778) [Maps] Get number of categories from palette (#66454) move oss features registration to KP (#66524) [kbn/plugin-helpers] typescript-ify (#66513) Add kibana-operations as codeowners for .ci/es-snapshots and vars/ (#66746) FTR: move basic services under common folder (#66563) Migrate Beats Management UI to KP (#65791) ... # Conflicts: # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_form/pipeline_form.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_form/pipeline_form_fields.tsx

* master: [ML] fix url assertion (elastic#66850) Skip failing lens test(s). elastic#66779 [SOM] Preserve saved object references when saving the object (elastic#66584) Use ES API from start contract (elastic#66157) Reorganize Management apps into Ingest, Data, Alerts and Insights, Security, Kibana, and Stack groups (elastic#65796) [Uptime] Fix flaky navigation to certs page in tests (elastic#66806) [Maps] Do not check count for blended layers when layer is not visible (elastic#66460) [SIEM] Fixes glob patterns from directory changes recently for GraphQL chore(NA): bump static-fs to 1.0.2 (elastic#66775) [Maps] Handle cross cluster index _settings resp (elastic#66797) [SIEM][Lists] Adds 90% of the REST API and client API for exception lists and exception items allow any type for customResponseHeaders config (elastic#66689) [APM] Disable map layout animation (elastic#66763) [ML] Add linking to dataframe from job management tab (elastic#65778)

kibanamachine · 2020-05-27T17:24:51Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 3f11520

History

💚 Build #48459 succeeded 3f11520
💚 Build #48161 succeeded 98bac55
💔 Build #48084 failed 7ac07f7
💔 Build #47981 failed 382a3ce
💔 Build #47977 failed af8ff7d

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

[Maps] Do not check count for blended layers when layer is not visible

6977a16

nreese added release_note:fix Team:Geo Former Team Label for Geo Team. Now use Team:Presentation v8.0.0 v7.8.0 v7.9.0 labels May 13, 2020

nreese requested a review from thomasneirynck May 13, 2020 19:37

nreese requested a review from a team as a code owner May 13, 2020 19:37

thomasneirynck removed the release_note:fix label May 13, 2020

thomasneirynck reviewed May 13, 2020

View reviewed changes

x-pack/plugins/maps/public/classes/layers/blended_vector_layer/blended_vector_layer.ts Outdated Show resolved Hide resolved

nreese added 2 commits May 14, 2020 07:36

Merge branch 'master' of github.com:elastic/kibana into no_count_check

29d4efe

move visibility logic to map_actions where syncData is called

4bd7c9c

nreese removed the v7.8.0 label May 14, 2020

clean up

5dada11

nreese requested a review from thomasneirynck May 14, 2020 14:00

nreese added the release_note:fix label May 14, 2020

nreese added 5 commits May 14, 2020 08:11

fix syntax error

3f6e75d

remove async action

8dee6ca

make syncDataForAllLayers a proper redux action

af8ff7d

simplify

382a3ce

fix functional test

7ac07f7

nreese commented May 14, 2020

View reviewed changes

Merge branch 'master' into no_count_check

98bac55

thomasneirynck approved these changes May 15, 2020

View reviewed changes

Merge branch 'master' into no_count_check

3f11520

nreese merged commit 09c950b into elastic:master May 15, 2020

nreese mentioned this pull request May 15, 2020

[7.x] [Maps] Do not check count for blended layers when layer is not visible (#66460) #66831

Merged

nreese mentioned this pull request May 27, 2020

Blended layer requests count when layer is not visible #67499

Closed

Conversation

nreese commented May 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented May 13, 2020

Uh oh!

thomasneirynck left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nreese commented May 14, 2020

Uh oh!

nreese May 14, 2020

Choose a reason for hiding this comment

Uh oh!

thomasneirynck May 15, 2020

Choose a reason for hiding this comment

Uh oh!

nreese commented May 14, 2020

Uh oh!

thomasneirynck left a comment

Choose a reason for hiding this comment

Uh oh!

thomasneirynck May 15, 2020

Choose a reason for hiding this comment

Uh oh!

thomasneirynck May 15, 2020

Choose a reason for hiding this comment

Uh oh!

nreese commented May 15, 2020

Uh oh!

nreese commented May 15, 2020

Uh oh!

nreese commented May 15, 2020

Uh oh!

kibanamachine commented May 27, 2020

💚 Build Succeeded

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nreese commented May 13, 2020 •

edited

Loading