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] Top hits per entity--change to title to use recent, minor edits #89254

Merged
merged 5 commits into from
Feb 4, 2021

Conversation

kmartastic
Copy link
Contributor

@kmartastic kmartastic commented Jan 26, 2021

Change title to use use-case centric language. Minor edits in topic.

Preview: https://kibana_89254.docs-preview.app.elstc.co/guide/en/kibana/master/maps-top-hits-aggregation.html

@kmartastic kmartastic added Team:Docs [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation v8.0.0 v7.11.0 v7.12.0 labels Jan 26, 2021
@kmartastic kmartastic requested a review from nreese January 26, 2021 00:46
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-docs (Team:Docs)

@cla-checker-service
Copy link

cla-checker-service bot commented Jan 26, 2021

💚 CLA has been signed

@kmartastic kmartastic added the release_note:skip Skip the PR/issue when compiling release notes label Jan 26, 2021
@kmartastic
Copy link
Contributor Author

Looks like my recommendation isn't a new change -- it reverts to the previous topic title (basically).

@gchaps
Copy link
Contributor

gchaps commented Jan 26, 2021

@kmartastic Thanks for the changes. Please sign the contributor agreement.

Your changes look good to me.

@@ -68,9 +68,9 @@ To enable a blended layer that dynamically shows clusters or documents:

[role="xpack"]
[[maps-top-hits-aggregation]]
=== Top hits per entity
=== Display the most recent documents
Copy link
Contributor

Choose a reason for hiding this comment

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

This title is not completely accurate. The top hits per entity is determined by the sort field and sort direction. Users can change either so that the top hits are not the most recent documents - but the most "relevent documents". Is this a concern that the title is too limiting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point.

I had the title as Display the most recent and relevant documents, but that was too long. Most relevant is better. The concern is the title is ambiguous. The goal was to make it more use-case driven.

@nreese Propose to change to: Display the most relevant documents

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the term "entity" needs to be in there somewhere. The big distinction with the top hits layer is that it grabs the top hits per entity instead of just grabbing the first 10000 matching documents.

@kmartastic kmartastic self-assigned this Jan 26, 2021
@kmartastic
Copy link
Contributor Author

@kmartastic Thanks for the changes. Please sign the contributor agreement.

Your changes look good to me.

I have signed it twice. :) What do I do now @gchaps?

@gchaps
Copy link
Contributor

gchaps commented Jan 26, 2021

cla/check

@spalger
Copy link
Contributor

spalger commented Jan 26, 2021

Hey @kmartastic, looks like your local git config setup is using the wrong email address for your commits. Looking at https://github.com/elastic/kibana/commit/a4d57f8da3e40d9489b259e38d39c36f911739a3.patch you can see that your commit is attributed to [email protected]. I recommend changing it to [email protected] and then resetting the author info with git commit --amend --no-edit --reset-author, then force push those changes to your fork and the CLA check should start working.

@gchaps
Copy link
Contributor

gchaps commented Jan 26, 2021

@kmartastic You can use the longer name on the page, and then use a shorter name in the navigation. For the shorter title, add code something like this:

++++
<titleabbrev>Display top documents</titleabbrev>
++++

@kmartastic
Copy link
Contributor Author

Hey @kmartastic, looks like your local git config setup is using the wrong email address for your commits. Looking at https://github.com/elastic/kibana/commit/a4d57f8da3e40d9489b259e38d39c36f911739a3.patch you can see that your commit is attributed to [email protected]. I recommend changing it to [email protected] and then resetting the author info with git commit --amend --no-edit --reset-author, then force push those changes to your fork and the CLA check should start working.

@spalger I hit an error trying to change the email associated with git commits.

image

I'm not sure how to proceed.

@spalger
Copy link
Contributor

spalger commented Jan 27, 2021

Hmm, where is that error coming from? The instructions I linked tell you how to set your email address that the git CLI is using to author commits you create locally, and then you'll use the git CLI to rewrite your commits to use the correct email address.

@kmartastic
Copy link
Contributor Author

kmartastic commented Jan 27, 2021

@spalger I was trying to follow these instructions: https://docs.github.com/en/github/setting-up-and-managing-your-github-user-account/adding-an-email-address-to-your-github-account

Specifically -- just trying to add the email to my account.

image

image

@spalger
Copy link
Contributor

spalger commented Jan 27, 2021

@spalger
Copy link
Contributor

spalger commented Jan 27, 2021

Under the Setting your commit email address in Git section

@kmartastic
Copy link
Contributor Author

@spalger I made the first changes -- but step 4. leads down the path to updating the profile to link contributions, which error'd.

@spalger
Copy link
Contributor

spalger commented Jan 27, 2021

Ah, gotcha, that step isn't necessary when you use the noreply.github.com email addresses github provides, sorry about that

@spalger
Copy link
Contributor

spalger commented Jan 27, 2021

Alright, looks like you merged the old commit back into your local branch, so the old commit is still in place and invalidating the CLA check. If you run the following it should fix it up:

git reset --hard 58e629ed7690feb11076a02eff461368cbe8ff32
git cherry-pick 4cef4615ab89cc0fdb6f8fcd4823d3b9289dcb4c
git push origin TopHitsPerEntity -f

@kmartastic
Copy link
Contributor Author

Thanks @spalger.

@kmartastic
Copy link
Contributor Author

@@ -68,9 +68,9 @@ To enable a blended layer that dynamically shows clusters or documents:

[role="xpack"]
[[maps-top-hits-aggregation]]
=== Top hits per entity
=== Display the most relevant entities
Copy link
Contributor

Choose a reason for hiding this comment

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

How about "Display the most relevant documents per entities"? The layer is not displaying entities but the top documents per entity.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to come up with a more specific word for entity?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like Nathan's suggestion; I think we should use entity.

Copy link
Contributor

Choose a reason for hiding this comment

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

++

@kmartastic
Copy link
Contributor Author

@nreese please review.


You can display the most relevant documents per entity, for example, the most recent GPS tracks per flight.
Use *Top hits per entity* to display the most relevant documents per entity, for example, the most recent GPS tracks per flight route.
Copy link
Contributor

Choose a reason for hiding this comment

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

Now title is repeat of this sentence. Is that a problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you think @gchaps?

I don't think it's a problem, but it's obviously not normal practice.

Copy link
Contributor

Choose a reason for hiding this comment

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

It does feel a little awkward to have"per entity" appear three times before the second sentence. What about removing that sentence and going with this:

Elasticsearch groups your data using a terms aggregation, and then accumulates the most relevant documents based on sort order for each entry using a top hits metric aggregation.

To view the top hits per entity:

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't loss the example, "for example, the most recent GPS tracks per flight route." needs to be maintained

Copy link
Contributor

@gchaps gchaps Jan 29, 2021

Choose a reason for hiding this comment

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

Elasticsearch groups your data using a terms aggregation, and then accumulates the most relevant documents based on sort order for each entry using a top hits metric aggregation. This enables you to view metrics such as the most recent GPS tracks per flight route.

To view the top hits per entity:

Copy link
Contributor

Choose a reason for hiding this comment

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

Can't use the word "metric" because its not an aggregation. Unlike an aggregation, top hits is actually showing documents, just a very small number of documents per entity, where entity is an airplane or truck.

Copy link
Contributor

@gchaps gchaps Jan 29, 2021

Choose a reason for hiding this comment

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

metrics > top hits?

If that doesn't work, let's go with the original text.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the text is fine because Tops hits per entity is a UI command.

Use Top hits per entity to display the most relevant documents per entity, for example, the most recent GPS tracks per flight route.

@kmartastic kmartastic requested review from nreese and gchaps January 29, 2021 21:24
Copy link
Contributor

@gchaps gchaps left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

LGTM

@kmartastic
Copy link
Contributor Author

@elasticmachine merge upstream

@kmartastic kmartastic merged commit da9c4a8 into elastic:master Feb 4, 2021
gmmorris added a commit to gmmorris/kibana that referenced this pull request Feb 4, 2021
* master: (244 commits)
  [maps] Top hits per entity--change to title to use recent, minor edits (elastic#89254)
  [DOCS] Update installation details (elastic#90354)
  RFC for automatically generated typescript API documentation for every plugins public services, types, and functionality (elastic#86704)
  Elastic Maps Server config is `host` not `hostname` (elastic#90234)
  Use doc link services in index pattern management (elastic#89937)
  [Fleet] Managed Agent Policy (elastic#88688)
  [Workplace Search] Fix Source Settings bug  (elastic#90242)
  [Enterprise Search] Refactor MockRouter test helper to not store payload (elastic#90206)
  Use doc link service in more Stack Monitoring pages (elastic#89050)
  [App Search] Relevance Tuning logic - actions and selectors only, no listeners (elastic#89313)
  Remove UI filters from UI (elastic#89793)
  Use newfeed.service config for all newsfeeds (elastic#90252)
  skip flaky suite (elastic#85086)
  Add readme to geo containment alert covering test alert setup (elastic#89625)
  [APM] Enabling yesterday option when 24 hours is selected (elastic#90017)
  Test user for maps tests under import geoJSON tests (elastic#86015)
  [Lens] Hide column in table (elastic#88680)
  [Security Solution][Detections] Reduce detection engine reliance on _source (elastic#89371)
  [Discover] Minor cleanup (elastic#90260)
  [Search Session][Management] Rename "cancel" button and delete "Reload" button (elastic#90015)
  ...
kmartastic added a commit to kmartastic/kibana that referenced this pull request Feb 5, 2021
elastic#89254)

* [maps] Top hits per entity--change to title to use recent, minor edits

* Updated TopHitsPerEntity title and description to use the term relevant

* updating top hits per entity topic to new title

Co-authored-by: Kent Marten <[email protected]>
Co-authored-by: Kibana Machine <[email protected]>
kmartastic added a commit to kmartastic/kibana that referenced this pull request Feb 5, 2021
elastic#89254)

* [maps] Top hits per entity--change to title to use recent, minor edits

* Updated TopHitsPerEntity title and description to use the term relevant

* updating top hits per entity topic to new title

Co-authored-by: Kent Marten <[email protected]>
Co-authored-by: Kibana Machine <[email protected]>
kmartastic added a commit that referenced this pull request Feb 5, 2021
#89254) (#90366)

* [maps] Top hits per entity--change to title to use recent, minor edits

* Updated TopHitsPerEntity title and description to use the term relevant

* updating top hits per entity topic to new title

Co-authored-by: Kent Marten <[email protected]>
Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Kent Marten <[email protected]>
Co-authored-by: Kibana Machine <[email protected]>
kmartastic added a commit that referenced this pull request Feb 5, 2021
#89254) (#90367)

* [maps] Top hits per entity--change to title to use recent, minor edits

* Updated TopHitsPerEntity title and description to use the term relevant

* updating top hits per entity topic to new title

Co-authored-by: Kent Marten <[email protected]>
Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Kent Marten <[email protected]>
Co-authored-by: Kibana Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation release_note:skip Skip the PR/issue when compiling release notes Team:Docs v7.11.0 v7.12.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants