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

[Maps] Blended layer that switches between documents and clusters #57879

Merged
merged 38 commits into from
Mar 18, 2020

Conversation

nreese
Copy link
Contributor

@nreese nreese commented Feb 18, 2020

This PR adds a new layer type, BLENDED_VECTOR. The layer type will dynamically show clusters or raw documents depending on how many features are visible in the current view box.

You can create a blended layer by selected Documents source and then selecting the dynamic clustering option under the new Scaling section.

Screen Shot 2020-02-19 at 9 39 52 AM

@nreese nreese added WIP Work in progress [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation enhancement New value added to drive a business result v8.0.0 v7.7.0 labels Feb 18, 2020
@nreese nreese requested a review from a team as a code owner February 18, 2020 16:40
@elasticmachine
Copy link
Contributor

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

@nreese
Copy link
Contributor Author

nreese commented Feb 19, 2020

@thomasneirynck @jsanz I added you for review even though this PR is still WIP. From a Proof of Concept perspective, the PR is complete. Could you please review the functionality from a usability perspective. Then, if this is the direction we think we should continue, I can finish up the PR and get it ready for code review.

@alexfrancoeur
Copy link

Hey @nreese, this functionality is awesome. I have some comments around the UX and would be interested in hearing thoughts from you and the group on them.

  • The current implementation generally feels a bit hidden for the functionality it offers
  • Explicitly calling out the 10k limit seemed odd, that's not something most users are aware of initially. If we do feel like we need to present this, it might be better to make it more subtle. Maybe as subtext, an info button or something in addition to the label it self
  • There might also be value in customizing this limit, maxing out at 10k
  • The other functionality that was missing for me was the option to configure my "clustering", "grid" or "heatmap". We default to clusters, but if we're providing this blended option, maybe we should have similar controls that we do in the grid agg layer.
  • To me, it almost feels like this functionality should be the default experience for adding a layer. Any new user would probably expect this experience and any mature maps user, would want to customize and have more control over when / how layers are shown
  • I wonder if it makes sense to approach this change with a broader change to layers. I know we have had discussions around grouping layer types or minimizing the selection available. Some high level Elasticsearch layer with a selection for "Blended", "Heatmap", "Cluster", "Shapes", "Points", where "Blended" is the default layer of choice could be interesting. It might be worth having a brainstorm session with design on this.

Should we sync live on this? I'm always one for progress over perfection, but if we want this layer to be used I think we need find more ways to expose the functionality.

@nreese
Copy link
Contributor Author

nreese commented Feb 19, 2020

I wonder if it makes sense to approach this change with a broader change to layers. I know we have had discussions around grouping layer types or minimizing the selection available. Some high level Elasticsearch layer with a selection for "Blended", "Heatmap", "Cluster", "Shapes", "Points", where "Blended" is the default layer of choice could be interesting. It might be worth having a brainstorm session with design on this

We had a sync on this and decided the best course of action given existing saved objects was to enhance the documents source. We did not want to redesign the entire source selection to get this feature in.

@jsanz
Copy link
Member

jsanz commented Feb 20, 2020

I agree with @alexfrancoeur that it would be great to have the chance to select the type of aggregation we want for the low zoom levels. Also, the 10K limit feels much like an implementation detail and we could present to the user the configuration for the layer in a more visual way. Related with that, when the layer changes between modes we could have some visual indication after the layer name, as we do now for other situations.

Have you already put logic on the legend for the aggregated layer based on the document source definition? Something like:

I see users wanting to do different things, so this should not be enforced but proposed, or maybe we can do nothing, and let the user decide how they want to symbolize the aggregation.

Also, if we decide to apply some defaults, and since we could potentially apply this based on document source properties I guess we would need to set up a priority for properties like: first color, then size, then label text. etc.

@nreese
Copy link
Contributor Author

nreese commented Mar 14, 2020

@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.

I think this is really excellent addition. It's the first feature that explicitly proposes a solution to the scalability issues with the default ES response limits. It automates a work-flow we now expect users to perform manually, and it retains Kibana's real-time searchability and filtering.


as for earlier comments:

The value is pulled from index setting maxResultWindow so its not as arbitrary as it seems

Agreed that we can call out the explicit limit. But could we provide a link to the docs? I think it's fair to point users to this setting and nudge them to either increase/decrease this limit (e.g. in dev-console).

I would like to avoid making it the default capability for a few reasons.

I agree with @alexfrancoeur that a user would benefit from this being the default behavior. When the nr-of-docs exceeds 10k, the user already pays a usability-tax.

Either:

    1. existing: the documents just get magically dropped from the view (which can create significant gaps in coverage).
    1. proposed: the performance of the extra round-trip of the count request

When docs exceed 10k, I think it would benefit the add-source wizard to present this choice up-front. In practice, that entire "scaling"-card would be presented in the add-source. Of course, users can always course-correct and change this setting in the layer-editor.

Now is not the time to re-design the entire layer creation process.

+1. Let's follow up on this as separate effort (e.g. as a top-level ask, for example when adding templated layers for ML, APM, ...).


More feedback forthcoming, but just wanted to start participation on this.

@thomasneirynck thomasneirynck self-requested a review March 16, 2020 03:20
@nreese
Copy link
Contributor Author

nreese commented Mar 16, 2020

When docs exceed 10k, I think it would benefit the add-source wizard to present this choice up-front. In practice, that entire "scaling"-card would be presented in the add-source. Of course, users can always course-correct and change this setting in the layer-editor.

agreed but I believe this should be done in a separate PR. Should I create an issue for this?

@nreese
Copy link
Contributor Author

nreese commented Mar 16, 2020

@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.

initial comments

x-pack/legacy/plugins/maps/common/descriptor_types.d.ts Outdated Show resolved Hide resolved
@@ -333,10 +342,6 @@ export class AbstractLayer {
return [];
}

async getFields() {
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks, +1

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.

lgtm. great feature!

it would be nice if we can work out details still in subsequent PRs:

  • improve wording of new "scaling"-card
  • move some (or all) of these scaling-settings into the add-source wizard so this functionality surfaces better (similar to how dynamic bounded-queries are added there)

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@nreese nreese merged commit 7e085ea into elastic:master Mar 18, 2020
nreese added a commit to nreese/kibana that referenced this pull request Mar 18, 2020
…astic#57879)

* [Maps] Blended layer that switches between documents and clusters

* change layer type when scalingType changes

* getSource

* use cluster source when count exceeds value

* ensure doc source stays in editor

* start creating cluster style

* pass all parts of style descriptor

* get toggling between sources working

* derive cluster style from document style

* remove references to METRIC_TYPE

* fix import

* start typescripting blended_vector_layer

* more typescript work

* last of the TS errors

* add migration to convert useTopTerm to scalingType

* clean up

* remove MapSavedObject work since its in a seperate PR now

* fix EsSearchSource update editor jest test

* fix map_selector jest test

* move mutable state out of BlendedVectorLayer

* one more change for removing mutable BlendedVectorLayer state

* integrate newly merged MapSavedObjectAttributes type

* review feedback

* use data request for fetching feature count

* add functional test

* fix functional test

* review feedback

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
@thomasneirynck thomasneirynck moved this from In progress to Done in Maps Mar 18, 2020
nreese added a commit that referenced this pull request Mar 18, 2020
…7879) (#60550)

* [Maps] Blended layer that switches between documents and clusters

* change layer type when scalingType changes

* getSource

* use cluster source when count exceeds value

* ensure doc source stays in editor

* start creating cluster style

* pass all parts of style descriptor

* get toggling between sources working

* derive cluster style from document style

* remove references to METRIC_TYPE

* fix import

* start typescripting blended_vector_layer

* more typescript work

* last of the TS errors

* add migration to convert useTopTerm to scalingType

* clean up

* remove MapSavedObject work since its in a seperate PR now

* fix EsSearchSource update editor jest test

* fix map_selector jest test

* move mutable state out of BlendedVectorLayer

* one more change for removing mutable BlendedVectorLayer state

* integrate newly merged MapSavedObjectAttributes type

* review feedback

* use data request for fetching feature count

* add functional test

* fix functional test

* review feedback

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Maps
  
Done (current version)
Development

Successfully merging this pull request may close these issues.

None yet

6 participants