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] add support for Top Hits to Documents source #38052

Merged
merged 18 commits into from
Jun 11, 2019

Conversation

nreese
Copy link
Contributor

@nreese nreese commented Jun 4, 2019

Replaces #33320

fixes #30738

This PR updates the Documents source to allows users to use Top hits aggregation to view the most recent documents per entity. Unlike a normal search, which just returns the first 2000 results, top hits performs a terms aggregation and then gathers documents per term. This ensures that each entity will display results and can be used for use cases like "show me the last known location for each entity".

Screen Shot 2019-06-04 at 4 32 13 PM

@nreese nreese added release_note:enhancement [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation v8.0.0 v7.3.0 labels Jun 4, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-gis

@nreese nreese mentioned this pull request Jun 4, 2019
@nreese
Copy link
Contributor Author

nreese commented Jun 4, 2019

@gchaps I need some help with the text on this PR. Do the labels in the side editor under Source settings make sense?

@nreese nreese changed the title Top hits [Maps] add support for Top Hits to Documents source Jun 4, 2019
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@nreese nreese requested a review from kindsun June 5, 2019 20:30
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Member

@nickpeihl nickpeihl left a comment

Choose a reason for hiding this comment

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

This is really cool! Tested in Firefox. I just have a couple of concerns.

  1. Since this is a terms aggregation would term a better word than entity in the source settings?
  2. It would be nice to have the more recent top hits draw on top of the older ones. See the screenshots below for example. Notice that the newer hits are drawn underneath older hits which leads me to misinterpret the direction the plane was traveling.
    Screenshot_2019-06-06 Plane Tracks - Kibana
    Screenshot_2019-06-06 Plane Tracks - Kibana(1)

@nreese
Copy link
Contributor Author

nreese commented Jun 6, 2019

Since this is a terms aggregation would term a better word than entity in the source settings?

@thomasneirynck suggested using entity in a POC review. "Let's call it "entity" (as this seems to be something we want to expand on in the stack)".

It would be nice to have the more recent top hits draw on top of the older ones

good feedback. I can sort the list by timestamp. Would you expect items across entities to be sorted so the newest always displays on top (regardless of which entity it comes from)? Or could the sort just be local to each entity?

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@nreese nreese requested a review from nickpeihl June 7, 2019 00:46
@@ -253,10 +336,30 @@ export class ESSearchSource extends AbstractESSource {
getSourceTooltipContent(sourceDataRequest) {
const featureCollection = sourceDataRequest ? sourceDataRequest.getData() : null;
const meta = sourceDataRequest ? sourceDataRequest.getMeta() : {};
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like getMeta() always minimally returns an empty object so no need for this conditional assignment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The conditional is for whether sourceDataRequest exists. Can not call getMeta on null. This statement is still needed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Member

@nickpeihl nickpeihl left a comment

Choose a reason for hiding this comment

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

lgtm.

Tested in Firefox. Newer points are now placed on top of older points.

@kindsun
Copy link
Contributor

kindsun commented Jun 11, 2019

Maybe we should consider not activating Save & Close if the user hasn't selected an entity since that shouldn't modify the layer and associated request (yet)

image

@nreese
Copy link
Contributor Author

nreese commented Jun 11, 2019

Maybe we should consider not activating Save & Close if the user hasn't selected an entity since that shouldn't modify the layer and associated request (yet)

Lets not couple that suggestion to this PR. I can open an issue for the discussion. We have the same thing when you apply dynamic styling but do not select the field yet

return {
hits: resp.hits.hits,
meta: {
areResultsTrimmed: resp.hits.total > resp.hits.hits.length
Copy link
Contributor

Choose a reason for hiding this comment

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

In case _runEsQuery throws, we might want to use _.get(resp, ....) with defaults for the values here as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If _runEsQuery throws then it won't matter because our function will throw since _runEsQuery is not wrapped in a try/catch. This is the expected behavior so vector_layer can catch the exception and call onLoadError

Copy link
Contributor

@kindsun kindsun 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, nice work!

code review. tested in chrome

@nreese nreese merged commit 230959c into elastic:master Jun 11, 2019
nreese added a commit to nreese/kibana that referenced this pull request Jun 11, 2019
* top hits

* fetch top hits

* trigger load if top hits configuration changes

* text clean-up

* add functional tests for top hits

* add functional test verify configuration updates re-fetch data

* show entity source tooltip message for top hits

* include entities cound in results trimmed message

* pass fields needed for data driven styling and joins

* fix i18n problem

* more i18n changes

* reverse hits list so most recent events are drawn on top

* review feedback

* set meta to null when no sourceDataRequest
nreese added a commit that referenced this pull request Jun 12, 2019
* top hits

* fetch top hits

* trigger load if top hits configuration changes

* text clean-up

* add functional tests for top hits

* add functional test verify configuration updates re-fetch data

* show entity source tooltip message for top hits

* include entities cound in results trimmed message

* pass fields needed for data driven styling and joins

* fix i18n problem

* more i18n changes

* reverse hits list so most recent events are drawn on top

* review feedback

* set meta to null when no sourceDataRequest
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Maps] Custom icons point styling
4 participants