[Maps] Fix tileload error messaging / Fix heatmap error message by thomasneirynck · Pull Request #30327 · elastic/kibana

thomasneirynck · 2019-02-06T22:19:42Z

Fixes false error messages that are most apparent in Cloud deployments.

Closes #29886
Closes #29863
Closes #29866

remove tile-error detection. It is buggy and relies on undocumented internal API from mapbox-gl. There is no real viable alternative at this point. The bugs are explained here [Maps] false negatives when checking for validity of tile service #29886 (comment) and here [Maps] false negatives when checking for validity of tile service #29886 (comment).
do not persist error state in descriptors. Error-messages will no longer reappear after saving.
refactor to use new naming scheme for app-lifecycle state in the descriptors. These should be prepended with __
tests

This also closes #30343, by introducing a default implementation of Layer#getOrdinalFields

elasticmachine · 2019-02-06T22:20:24Z

Pinging @elastic/kibana-gis

copy persistent state using new naming scheme rename error state rename previous style remove cruft remove unnecessary asyncyness

elasticmachine · 2019-02-06T23:08:48Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…load

thomasneirynck · 2019-02-07T04:21:38Z

jenkins, test this

thomasneirynck · 2019-02-07T04:29:58Z

jenkins, test this

elasticmachine · 2019-02-07T05:05:55Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

thomasneirynck · 2019-02-07T14:08:45Z

jenkins, test this

elasticmachine · 2019-02-07T15:15:58Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

nreese · 2019-02-07T16:28:11Z

Would you mind adding the fix for #30343

in layer.js add a default implementation for getOrdinalFields

async getOrdinalFields() {
  return [];
}

kindsun · 2019-02-07T16:44:42Z

-      return;
-    }
+    if (!source) {
+      const sourceDataRquest = this.getSourceDataRequest();


typo: sourceDataRquest -> sourceDataRequest

kindsun · 2019-02-07T16:49:01Z

+        minzoom: this._descriptor.minZoom,
+        maxzoom: this._descriptor.maxZoom,
+      });
      return;


Nice. Good call returning before a needless call to set layer props

oops, that's actually a bug...

lol. I was actually just staring at it coming to the same conclusion. we would be doing an extra call to set zoom but alpha isn't covered. there might be a better way to dice those up but a redundant zoom set isn't the end of the world

kindsun · 2019-02-07T17:20:25Z

In this revised workflow, if I add a URL I know won't work, the layer is still added to the TOC with no indication it's any different from valid layers. Maybe we should consider dispatching a layer remove action after a timeout callback checks the map and finds the id not present. This would keep the layer list in sync with the map and give the user a more accurate experience in the absence of error messaging.

kindsun · 2019-02-07T17:22:30Z

I think the new pattern you're using for non-persisting properties will be the key to solving #28576 too. Nice! (sorry, linked wrong issue before! hopefully less confusing now)

thomasneirynck · 2019-02-07T18:11:29Z

@aaronjcaldwell

Maybe we should consider dispatching a layer remove action after a timeout callback checks the map and finds the id not present.

The issues in Cloud are not about whether we should show an error or not, or whether a faulty layer should stay in the TOC or not. It is about not being able to reliably detect, given the limitations of the mapboxgl-api, whether a given tile-service is functional or not. There's a lot of edge-cases - sparsity in tile coverage, excessive (but expected) latency, .... - that need to be handled, beyond the scope of the bugs described in this PR.

As for detecting if the id is present in the mapbox-map. It is present because the Maps-app is explicitly adding the layer to the mapbox-map during syncing. Mapbox-gl is not removing the layer because it cannot load tiles. That would break the contract of their API.

We could take this question more generically: should faulty or dysfunctional layers stay in the TOC? This question already is sort of answered when introducing the layer-error state in #28852. There are certain very well defined conditions (e.g. cannot find index-pattern id, administrator turned off connection to EMS in the kibana.yml, ....) that the app can detect. The app can present a useful error message for those. Apart from displaying useful hints, if the app would not show the layer in the TOC, a user would not be able to manipulate their map configuration to try and fix the error. That would be a dark pattern imho to remove layers just because there are configuration mismatches, or missing backend dependencies.

nreese

lgtm
code review, tested changes in chrome

nreese · 2019-02-07T18:16:57Z

-    if (this._descriptor.dataRequests) {
-      this._dataRequests = this._descriptor.dataRequests.map(dataRequest => new DataRequest(dataRequest));
+    if (this._descriptor.__dataRequests) {
+      this._dataRequests = this._descriptor.__dataRequests.map(dataRequest => new DataRequest(dataRequest));


This seems error prone to have this._dataRequests with a single underscore and this._descriptor.__dataRequests with a double underscore. Any ideas on how to avoid?

yeah .... it looks funny.

We could just revert to single underscore, and ditch the double underscore.

On the other hand, it's a pretty common convention to prepend _ to variables of private state or internal state. But I feel like store-state is something different altogether (just gut feeling..)

Maybe we could just leave as-is for now (?)

yep, leaving as is for now sounds ok.

elasticmachine · 2019-02-07T21:35:59Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

thomasneirynck · 2019-02-07T21:42:42Z

CI is down.

jenkins, test this

elasticmachine · 2019-02-07T21:59:34Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

thomasneirynck · 2019-02-07T22:22:34Z

green CI https://kibana-ci.elastic.co/job/elastic+kibana+pull-request/11656/

…tic#30327)

elasticmachine · 2019-02-07T23:02:29Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…) (#30443)

…) (#30444)

…tic#30327)

…) (#30442)

thomasneirynck added v7.0.0 v8.0.0 v6.7.0 v7.2.0 blocker Team:Geo Former Team Label for Geo Team. Now use Team:Presentation labels Feb 6, 2019

thomasneirynck added the WIP Work in progress label Feb 6, 2019

rename datarequests

5deee72

copy persistent state using new naming scheme rename error state rename previous style remove cruft remove unnecessary asyncyness

thomasneirynck force-pushed the maps/fix_tileloads branch from c3988fc to 5deee72 Compare February 6, 2019 22:43

thomasneirynck requested review from kindsun and nreese February 6, 2019 22:45

thomasneirynck changed the title ~~Fix tileload error messaging~~ [Ma[s] Fix tileload error messaging Feb 6, 2019

thomasneirynck changed the title ~~[Ma[s] Fix tileload error messaging~~ [Maps] Fix tileload error messaging Feb 6, 2019

Merge branch 'master' of github.com:elastic/kibana into maps/fix_tile…

1128845

…load

Merge branch 'master' into maps/fix_tileload

86403e5

kindsun reviewed Feb 7, 2019

View reviewed changes

thomasneirynck removed the WIP Work in progress label Feb 7, 2019

thomasneirynck added 2 commits February 7, 2019 12:46

Merge branch 'master' into maps/fix_tileload

d431d2d

review feedback

20693bb

fix tests

b8ab7ed

thomasneirynck changed the title ~~[Maps] Fix tileload error messaging~~ [Maps] Fix tileload error messaging / Fix heatmap error message Feb 7, 2019

nreese approved these changes Feb 7, 2019

View reviewed changes

add missing feedback

88f4b6c

nreese mentioned this pull request Feb 7, 2019

[Maps] upgrade mapboxgl to 0.52.0 #30431

Merged

thomasneirynck merged commit 168d17e into elastic:master Feb 7, 2019

thomasneirynck added a commit to thomasneirynck/kibana that referenced this pull request Feb 7, 2019

[Maps] Fix tileload error messaging / Fix heatmap error message (elas…

24cf4f9

…tic#30327)

This was referenced Feb 7, 2019

[7.x] [Maps] Fix tileload error messaging / Fix heatmap error message (#30327) #30442

Merged

[7.0] [Maps] Fix tileload error messaging / Fix heatmap error message (#30327) #30443

Merged

thomasneirynck added a commit to thomasneirynck/kibana that referenced this pull request Feb 7, 2019

[Maps] Fix tileload error messaging / Fix heatmap error message (elas…

c32a89d

…tic#30327)

thomasneirynck mentioned this pull request Feb 7, 2019

[6.7] [Maps] Fix tileload error messaging / Fix heatmap error message (#30327) #30444

Merged

thomasneirynck added a commit to thomasneirynck/kibana that referenced this pull request Feb 7, 2019

[Maps] Fix tileload error messaging / Fix heatmap error message (elas…

794aa88

…tic#30327)

thomasneirynck added a commit that referenced this pull request Feb 8, 2019

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

75e6928

…) (#30443)

thomasneirynck added a commit that referenced this pull request Feb 8, 2019

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

e16f98b

…) (#30444)

lukeelmers pushed a commit to lukeelmers/kibana-factory that referenced this pull request Feb 8, 2019

[Maps] Fix tileload error messaging / Fix heatmap error message (elas…

cba941e

…tic#30327)

thomasneirynck added a commit that referenced this pull request Feb 13, 2019

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

9921387

…) (#30442)

Conversation

thomasneirynck commented Feb 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Feb 6, 2019

Uh oh!

elasticmachine commented Feb 6, 2019

💔 Build Failed

Uh oh!

thomasneirynck commented Feb 7, 2019

Uh oh!

thomasneirynck commented Feb 7, 2019

Uh oh!

elasticmachine commented Feb 7, 2019

💔 Build Failed

Uh oh!

thomasneirynck commented Feb 7, 2019

Uh oh!

elasticmachine commented Feb 7, 2019

💔 Build Failed

Uh oh!

nreese commented Feb 7, 2019

Uh oh!

kindsun Feb 7, 2019

Choose a reason for hiding this comment

Uh oh!

kindsun Feb 7, 2019

Choose a reason for hiding this comment

Uh oh!

thomasneirynck Feb 7, 2019

Choose a reason for hiding this comment

Uh oh!

kindsun Feb 7, 2019

Choose a reason for hiding this comment

Uh oh!

kindsun commented Feb 7, 2019

Uh oh!

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

Uh oh!

Uh oh!

thomasneirynck commented Feb 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nreese left a comment

Choose a reason for hiding this comment

Uh oh!

nreese Feb 7, 2019

Choose a reason for hiding this comment

Uh oh!

thomasneirynck Feb 7, 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 Feb 7, 2019

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Feb 7, 2019

💔 Build Failed

Uh oh!

thomasneirynck commented Feb 7, 2019

Uh oh!

elasticmachine commented Feb 7, 2019

💚 Build Succeeded

Uh oh!

thomasneirynck commented Feb 7, 2019

Uh oh!

elasticmachine commented Feb 7, 2019

💔 Build Failed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

thomasneirynck commented Feb 6, 2019 •

edited

Loading

kindsun commented Feb 7, 2019 •

edited

Loading

thomasneirynck commented Feb 7, 2019 •

edited

Loading

thomasneirynck Feb 7, 2019 •

edited

Loading