Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mask timeslider #102046

Merged
merged 20 commits into from
Jun 23, 2021
Merged

Mask timeslider #102046

merged 20 commits into from
Jun 23, 2021

Conversation

nreese
Copy link
Contributor

@nreese nreese commented Jun 14, 2021

Client side masking was removed from original timeslider implementation.

This PR brings back client side masking to timeslider with the following improvements

  • masking field is pulled from source data request meta so ILayer.syncLayerWithMB does not need to make async
  • ESSearchSource.getGeoJsonWithMeta performs a count request prior to fetching documents to know if it can pull for the entire time range or just the timeslice.

@elasticmachine
Copy link
Contributor

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

@nreese nreese requested a review from a team as a code owner June 14, 2021 16:23
Copy link
Contributor

@thomasneirynck thomasneirynck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thx!

most of this was discussed offline with @nreese, but for completeness.

The current approach is hard to generalize to .mvt based data. For mvt-data, the source cannot determine up front whether all data is on-screen. Instead, it needs to inspect the actual responses of the tiles (reading metadata from tiles is being worked on here #94811).

As next steps, we will introduce in this PR:

  • pull up determining whether data is complete into a layer-method. This can be implemented differently, based per layer type (in practice, there will be a different implementations for vector_layer and tiled_vector_layer).
  • source could stilll make a pre-fetch call to fetch all data for the entire-range, rather than just the timeslice. This would mean that es-sources with the LIMIT-scaling type can still prefetch all data for the entire range (rather than just the timeslice). This is an optimization that would only work the LIMIT-scaling-type. We determine it is valuable enough to release, mainly because a lot of the mvt-work on the ES/and Kibana sides is still in flux ([Maps] [Meta] .mvt feature completeness #79868, Support .mvt vector tile format elasticsearch#58696).

@nreese
Copy link
Contributor Author

nreese commented Jun 16, 2021

@elasticmachine merge upstream

@spalger
Copy link
Contributor

spalger commented Jun 16, 2021

jenkins, test this

(restarting due to jenkins upgrade)

@nreese
Copy link
Contributor Author

nreese commented Jun 16, 2021

@elasticmachine merge upstream

Copy link
Contributor

@thomasneirynck thomasneirynck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx, +1 on overall changes. I think we should be able to hack on tile-based data here as well (tbd).

One small bug I believe, see comments.

Copy link
Contributor

@thomasneirynck thomasneirynck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, this is a great improvement. It will make timeslider work for even more datasets.
See one comment to improve readability.

@nreese
Copy link
Contributor Author

nreese commented Jun 23, 2021

@elasticmachine merge upstream

@nreese nreese added the auto-backport Deprecated - use backport:version if exact versions are needed label Jun 23, 2021
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
maps 3.1MB 3.1MB +4.1KB

History

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

@nreese nreese merged commit 81fe541 into elastic:master Jun 23, 2021
@kibanamachine
Copy link
Contributor

💔 Backport failed

Status Branch Result
7.x Commit could not be cherrypicked due to conflicts

To backport manually run:
node scripts/backport --pr 102046

@nreese
Copy link
Contributor Author

nreese commented Jun 23, 2021

7.x backport #103132

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation release_note:enhancement v7.14.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants