[Maps] Handle unavailable tilemap services by kindsun · Pull Request #28852 · elastic/kibana

kindsun · 2019-01-16T16:41:00Z

Resolves #27832 & #28119. Handles errors from:

EMS TMS
custom TMS
TMS from url
Also handles EMS disabling via configuration

A few details:

If the app is unable to obtain the EMS manifest, EMS fails immediately
Timeout for valid urls is set at 32 seconds
Errors are show both on the layer TOC/Legend and in the console. We could port messaging logic recently written for the region/coordinate maps over in a future PR

…attribution errors

… both obtaining url and tile loading

…atus

… list from reordering logic

…or logic for tile and vector layers

elasticmachine · 2019-01-16T17:21:39Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-01-16T17:56:46Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-01-17T16:17:05Z

Pinging @elastic/kibana-gis

…nsolidate redundant code

…loading via callback. Add interval to cancel timer

elasticmachine · 2019-01-17T20:20:42Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-01-17T20:55:14Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-01-17T21:33:30Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-01-17T23:02:23Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

kindsun · 2019-01-17T23:04:33Z

retest

elasticmachine · 2019-01-17T23:45:28Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

# Conflicts: # x-pack/plugins/gis/public/shared/layers/layer.js

elasticmachine · 2019-01-18T21:37:27Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

nreese

Works really well and the app now works without any EMS connection.

nreese · 2019-01-18T21:45:55Z

 export const SET_SELECTED_LAYER = 'SET_SELECTED_LAYER';
 export const UPDATE_LAYER_ORDER = 'UPDATE_LAYER_ORDER';
 export const ADD_LAYER = 'ADD_LAYER';
+export const SET_TMS_ERROR_STATUS = 'SET_TMS_ERROR_STATUS';


Can you make this a generic action for setting any error status on a layer. Not one thats tied to TMS. So maybe rename to SET_LAYER_ERROR_MESSAGE?

nreese · 2019-01-18T22:08:48Z

-    layerList.forEach((layer) => {
-      layer.syncLayerWithMB(this._mbMap);
+    layerList.forEach(layer => {
+      if (!layer.dataHasLoadError()) {


dataHasLoadError maybe this should be more generic and just be hasErrors? That way it will cover all errors, not just load errors.

nreese · 2019-01-18T22:09:47Z

+      if (!layer.dataHasLoadError()) {
+        Promise.resolve(layer.syncLayerWithMB(this._mbMap))
+          .catch(({ message }) => {
+            switch(layer._descriptor.type) {


Is this switch needed? I would say all errors have the same action - update the layer state with the error message

nreese · 2019-01-18T22:11:14Z

    return this._source.renderSourceSettingsEditor({ onChange });
  }

+  _findDataRequestForSource(sourceDataId) {


Delete unused method

nreese · 2019-01-18T22:14:56Z


  dataHasLoadError() {
-    return this._dataRequests.some(dataRequest => dataRequest.hasLoadError());
+    return this._descriptor && this._descriptor.errorState || false;


Should errorState be errorMessage? What is the difference between the two?

Error state is a boolean value. When an error occurs, the layer is put in errorState and is assigned an errorMessage, assuming there is one. I could change it to something like isInErrorState if we wanted to be explicit on the boolean status

I would rename to isInErrorState, that makes a lot of sense. Also this._descriptor.errorState || false should use lodash _.get to avoid problems like this #29275

nreese · 2019-01-18T22:16:36Z

-        return dataRequest._descriptor.dataLoadError;
-      });
-    return loadErrors.join(',');
+    return this.dataHasLoadError() ? this._descriptor.errorMessage : '';


Maybe errorMessage should be an array errors so multiple message could be displayed. For example lets say you had a EMS vector layer with a join. Then EMS is not available and the index pattern of the join got deleted. Should have two messages - 1) Can not load EMS vector layer. 2) Can not access index pattern X

I'm going to hold off on this since we're primarily focused on TMS for this issue, which should have limited errors. I do think our messaging deserves further fleshing out. If we don't already have one, we likely need a future issue around better error messaging to the user. We have fairly robust messaging for the legacy maps now (some of it pending merge). We could port that over and rethink how errors are relayed in the process.

nreese · 2019-01-18T22:19:03Z

      return service.id === this._descriptor.id;
    });
+    if (!emsTmsService) {
+      console.error(`EMS TMS Service: ${this._descriptor.id} currently unavailable`);


Not sure this console error is needed. Just going to spam the console. The server should log an error if it can not reach EMS. Not need for the console to log errors since there will be something visible to the user through the UI

nreese · 2019-01-18T22:19:40Z

+    let attributions;
+    try {
+      service = this._getTMSOptions();
+      attributions = service.attributionMarkdown.split('|');


Need to check that service exists because _getTMSOptions can return null

nreese · 2019-01-18T22:33:08Z

+        map.off('dataloading');
+      };
+
+      checkInterval = setInterval(() => {


Why is this interval needed? Isn't the timer sufficient?

Running timers stress me out :), especially ones that run for 32 seconds as this one does. I added this on a second pass to clear the timer as soon as the tiles have loaded rather than keep running. I don't mind removing it though. It's a little cluttery and we can always bring it back if we want.

nreese · 2019-01-18T22:36:41Z

+      }, 1000);
+      tileLoadTimer = setTimeout(() => {
+        if (!tileLoad) {
+          try {


Why do you throw from within a try and then catch it? Could just just be

if (!tileLoadCount) { reject(new Error(`Tiles from "${url}" could not be loaded`)); return; } resolve();

…Disabling # Conflicts: # x-pack/plugins/gis/public/shared/layers/layer.js # x-pack/plugins/gis/public/shared/layers/tile_layer.js # x-pack/plugins/gis/public/shared/layers/util/data_request.js # x-pack/plugins/gis/public/shared/layers/vector_layer.js

nreese · 2019-01-24T20:43:04Z

-  dataHasLoadError() {
-    return this._dataRequests.some(dataRequest => dataRequest.hasLoadError());
+  hasErrors() {
+    return _.get(this._descriptor, 'isInErrorState', false);


Is isInErrorState needed? Why not just check if this._descriptor.errorMessage exists or not?

Not needed, just preferred

nreese · 2019-01-24T20:46:35Z


  _getTMSOptions() {
-    return this._emsTileServices.find(service => {
+    if(!this._emsTileServices || !this._emsTileServices.length) {


This method can be simplified to just

_getTMSOptions() { if(!this._emsTileServices) { return; } return this._emsTileServices.find(service => { return service.id === this._descriptor.id; }); }

nreese · 2019-01-24T20:52:00Z

-    });
+    let service;
+    let attributions;
+    try {


What is this try/catch block going to catch? I don't think its needed since this._getTMSOptions does not throw and error and service.attributionMarkdown.split should not throw an error. This entire method code be rewritten to the below which I think is much more readable

async getAttributions() { const service = this._getTMSOptions(); if (!service || !service.attributionMarkdown) { return []; } return service.attributionMarkdown.split('|').map((attribution) => { attribution = attribution.trim(); //this assumes attribution is plain markdown link const extractLink = /\[(.*)\]$(.*)$/; const result = extractLink.exec(attribution); return { label: result ? result[1] : null, url: result ? result[2] : null }; }); }

There was an error getting thrown specific to attributions, otherwise I wouldn't have modified it at all since it's not directly related. I believe it was attempting to create attributions on unresolved layers

But where is the error getting thrown? Is it getting thrown by _getTMSOptions? This method needs some clean-up. It is very difficult to read and has some problems. For example, if an exception is thrown and attributions is not set when you will call attributions.map with will throw an exception. Also, your return returns mixed types. Sometimes it will return an array and sometimes it will return an empty string. It should return an [] if there are not attributes or there are problems getting them. The implementation I provided should be fine and I don't think needs to be a try/catch block

nreese · 2019-01-24T20:52:28Z

 export class TileLayer extends ALayer {

  static type = "TILE";
+  TMS_LOAD_TIMEOUT = 32000;


Where did 32 seconds come from?

Offline discussion with @thomasneirynck but for resolution of the TMS layers in the legacy map apps. For consistency, I made the timeout the same here

elasticmachine · 2019-01-24T20:55:50Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-01-25T18:14:46Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

kindsun · 2019-01-25T18:20:37Z

retest

elasticmachine · 2019-01-25T19:01:50Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…Disabling # Conflicts: # x-pack/plugins/gis/public/components/widget_overlay/layer_control/layer_toc/toc_entry/view.js

elasticmachine · 2019-01-25T21:26:24Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

nreese

Looks like some dataHasLoadError and getDataLoadError function names snuck in with latest rebase

https://github.com/elastic/kibana/blob/master/x-pack/plugins/gis/public/shared/components/layer_toc_actions.js#L80

nreese · 2019-01-25T22:01:44Z

  static createDescriptor(options = {}) {
    const layerDescriptor = { ...options };

+    layerDescriptor.source = this._source;


Why are you setting descriptor.source to an object instance? I thought the descriptors just held pure data and not instances. I don't think this is needed and may cause problems when storing the layer in the store since this._source is not serializable.

nreese · 2019-01-25T22:02:56Z

+    if (!this.hasErrors()) {
+      return await this._source.getAttributions();
+    }
+    return '';


Should return [] to be consistent with this._source.getAttributions()

nreese · 2019-01-25T22:06:02Z

 export class TileLayer extends AbstractLayer {

  static type = "TILE";
+  TMS_LOAD_TIMEOUT = 32000;


right now TMS_LOAD_TIMEOUT on this is editable. Move this out of class and make const

nreese · 2019-01-25T22:10:12Z

+    map.on('dataloading', ({ tile }) => {
+      if (tile && tile.request) {
+        // If at least one tile loads, endpoint/resource is valid
+        tile.request.onloadend = ({ loaded }) => loaded && (tileLoad = true);


tile.request.onloadend = ({ loaded }) => loaded && (tileLoad = true); is difficult to read in a single line. How about breaking it out so its easier to read and under stand that tileLoad is getting set to true

tile.request.onloadend = ({ loaded }) => { if (loaded) { tileLoad = true; } };

nreese · 2019-01-25T22:27:10Z

+    }
+
+    let url;
+    return new Promise((resolve, reject) => {


Nothing in this first promise block is async. Why does the first new Promise exist? It really hurts readability to have all of these nested Promises and .then callbacks.

This function can be rewritten to the below which is much easier to follow. If this._source.getUrlTemplate or await this._tileLoadErrorTracker(mbMap, url) fail the function will throw an error which will be catch by the caller x-pack/plugins/gis/public/components/map/mb/view.js

async syncLayerWithMB(mbMap) { const source = mbMap.getSource(this.getId()); const layerId = this.getId() + '_raster'; if (source) { return; } const url = this._source.getUrlTemplate(); const sourceId = this.getId(); mbMap.addSource(sourceId, { type: 'raster', tiles: [url], tileSize: 256, scheme: 'xyz', }); mbMap.addLayer({ id: layerId, type: 'raster', source: sourceId, minzoom: 0, maxzoom: 22, }); await this._tileLoadErrorTracker(mbMap, url); mbMap.setLayoutProperty(layerId, 'visibility', this.isVisible() ? 'visible' : 'none'); mbMap.setLayerZoomRange(layerId, this._descriptor.minZoom, this._descriptor.maxZoom); this._style && this._style.setMBPaintProperties({ alpha: this.getAlpha(), mbMap, layerId, }); }

elasticmachine · 2019-01-28T16:58:25Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

thomasneirynck

See comment. I'm running into the same for #29367.

If we change the signature to async of the layer syncing with mapbox-gl, the client-code needs to handle this asynchronous nature.

thomasneirynck · 2019-01-28T17:21:53Z

+    });
+  }
+
+  async syncLayerWithMB(mbMap) {


If we make this signature async, than the client-code should take into account the async-nature of this method.

In practice, the completion of this method (https://github.com/thomasneirynck/kibana/blob/93fe5b7b2f566cc4519cc257834b110eb8f8248b/x-pack/plugins/gis/public/components/map/mb/view.js#L194) now no longer guarantees that the map has synced.

thomasneirynck · 2019-01-28T17:23:39Z

-      layer.syncLayerWithMB(this._mbMap);
+    layerList.forEach(layer => {
+      if (!layer.hasErrors()) {
+        Promise.resolve(layer.syncLayerWithMB(this._mbMap))


This causes the syncing operation to unhook from the callstack, and completion is not guaranteed.

thomasneirynck

👍

* Update ems utils to better handle no service results. Prevent excess attribution errors * Update tile layer sync to return promise and handle errors related to both obtaining url and tile loading * Add flow for updating tms layers with error status/message * Handle promises, if returned, on syncLayerWithMB. Update TMS error status * Exclude layers that mapbox didn't add to map but are tracked in layer list from reordering logic * Move datarequest handling to vector layer. Use relevant data load/error logic for tile and vector layers * Don't try to get attributions on errored layer * Handle 'includeElasticMapsService' configuration * Move data requests back to layer level for heatmap usage * Update all layers to set top-level layer error status and message. Consolidate redundant code * Update tile sync function to more reliably confirm load status after loading via callback. Add interval to cancel timer * Remove unnecessary, and annoying, clear temp layers on tms error * Clean up * More clean up * Review feedback * Review feedback. Test cleanup * Test fixes and review feedback

kindsun added 7 commits January 16, 2019 08:59

Update ems utils to better handle no service results. Prevent excess …

e482029

…attribution errors

Update tile layer sync to return promise and handle errors related to…

6b911c3

… both obtaining url and tile loading

Add flow for updating tms layers with error status/message

abafa1c

Handle promises, if returned, on syncLayerWithMB. Update TMS error st…

ad4184b

…atus

Exclude layers that mapbox didn't add to map but are tracked in layer…

58651ae

… list from reordering logic

Move datarequest handling to vector layer. Use relevant data load/err…

bfc1db4

…or logic for tile and vector layers

Don't try to get attributions on errored layer

4a6f50e

kindsun changed the title ~~Fix/maps app handle disabling~~ Handle unavailable tilemap services Jan 16, 2019

Handle 'includeElasticMapsService' configuration

3b2bba5

kindsun requested a review from thomasneirynck January 16, 2019 17:13

nreese self-requested a review January 16, 2019 22:03

thomasneirynck added Project:Accessibility v7.0.0 Team:Geo Former Team Label for Geo Team. Now use Team:Presentation v6.7.0 labels Jan 17, 2019

thomasneirynck removed the Project:Accessibility label Jan 17, 2019

kindsun added 4 commits January 17, 2019 12:39

Move data requests back to layer level for heatmap usage

43a021f

Update all layers to set top-level layer error status and message. Co…

ab80f12

…nsolidate redundant code

Update tile sync function to more reliably confirm load status after …

b4c16ce

…loading via callback. Add interval to cancel timer

Remove unnecessary, and annoying, clear temp layers on tms error

76b357c

Clean up

eca78ef

More clean up

7b75249

Merge branch 'master' into fix/mapsAppHandleDisabling

0c3dd1f

# Conflicts: # x-pack/plugins/gis/public/shared/layers/layer.js

nreese reviewed Jan 18, 2019

View reviewed changes

kindsun added 2 commits January 24, 2019 12:22

Review feedback

adf4962

nreese reviewed Jan 24, 2019

View reviewed changes

Review feedback. Test cleanup

0a205a3

Merge remote-tracking branch 'upstream/master' into fix/mapsAppHandle…

11fcf24

…Disabling # Conflicts: # x-pack/plugins/gis/public/components/widget_overlay/layer_control/layer_toc/toc_entry/view.js

nreese reviewed Jan 25, 2019

View reviewed changes

Test fixes and review feedback

14e94cb

thomasneirynck reviewed Jan 28, 2019

View reviewed changes

thomasneirynck changed the title ~~Handle unavailable tilemap services~~ [Maps] Handle unavailable tilemap services Jan 28, 2019

thomasneirynck approved these changes Jan 28, 2019

View reviewed changes

kindsun merged commit 20c5c3b into elastic:master Jan 28, 2019

kindsun mentioned this pull request Jan 29, 2019

[6.x] [Maps] Handle unavailable tilemap services (#28852) #29542

Merged

nreese mentioned this pull request Feb 1, 2019

[Maps] Apply tile styles before waiting for _tileLoadErrorTracker timer to expire #29798

Merged

thomasneirynck mentioned this pull request Feb 7, 2019

[Maps] Fix tileload error messaging / Fix heatmap error message #30327

Merged

4 tasks

Conversation

kindsun commented Jan 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Jan 16, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Jan 16, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Jan 17, 2019

Uh oh!

elasticmachine commented Jan 17, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Jan 17, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Jan 17, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Jan 17, 2019

💔 Build Failed

Uh oh!

kindsun commented Jan 17, 2019

Uh oh!

elasticmachine commented Jan 17, 2019

💚 Build Succeeded

Uh oh!

elasticmachine commented Jan 18, 2019

💚 Build Succeeded

Uh oh!

nreese left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

kindsun Jan 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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 Jan 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

kindsun commented Jan 16, 2019 •

edited

Loading

nreese Jan 18, 2019 •

edited

Loading

nreese Jan 18, 2019 •

edited

Loading

kindsun Jan 24, 2019 •

edited

Loading

nreese Jan 24, 2019 •

edited

Loading

nreese Jan 25, 2019 •

edited

Loading