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.

Sign up for GitHub

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

Jump to bottom

[lens] Add significant terms support as ranking function in Top Values #158962

Merged
merged 15 commits into from
Jun 8, 2023
Merged

[lens] Add significant terms support as ranking function in Top Values #158962

merged 15 commits into from
Jun 8, 2023

Conversation

nreese
Copy link
Contributor

@nreese nreese commented Jun 2, 2023 •
edited by stratoula
Loading

Part of #154307
Closes #116421

PR adds Significance selection to Rank by select. When selected, top values are fetched with significant_terms aggregation

Screen Shot 2023-06-02 at 12 56 29 PM

@nreese nreese changed the title [lens] add support for significant terms aggregation [lens] significant terms Jun 2, 2023
@nreese
Copy link
Contributor Author

nreese commented Jun 5, 2023

@elasticmachine merge upstream

@nreese nreese marked this pull request as ready for review June 5, 2023 17:30
@nreese nreese requested review from a team as code owners June 5, 2023 17:30
@nreese nreese added release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.9.0 labels Jun 5, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-visualizations @elastic/kibana-visualizations-external (Team:Visualizations)

dej611
dej611 reviewed Jun 6, 2023
View reviewed changes
Copy link
Contributor

@dej611 dej611 left a comment

Choose a reason for hiding this comment

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

Tested locally with Safari and it works great 👍

I would just suggest to add few unit tests for Significant ranking criteria and checking that few options (add term, group rest as Other, etc...) are disabled.
Very minor nitpick on the code review.

x-pack/plugins/lens/public/datasources/form_based/operations/definitions/terms/index.tsx Outdated Show resolved Hide resolved
@kertal kertal requested a review from lukasolson June 6, 2023 09:37
@nreese
Copy link
Contributor Author

nreese commented Jun 6, 2023

@elasticmachine merge upstream

dej611
dej611 approved these changes Jun 6, 2023
View reviewed changes
Copy link
Contributor

@dej611 dej611 left a comment

Choose a reason for hiding this comment

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

Code review

@dej611 dej611 changed the title [lens] significant terms [lens] Add significant terms support as ranking function in Top Values Jun 6, 2023
@stratoula
Copy link
Contributor

@dej611 have you also tested it or do you want me to also do a testing here?

@dej611
Copy link
Contributor

dej611 commented Jun 6, 2023

I did test it before locally. I've just approved now as I saw the new tiny refactoring landing

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
data 2577 2578 +1

Async chunks

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

id before after diff
lens 1.2MB 1.2MB +1.2KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
data 404.1KB 404.4KB +294.0B
Unknown metric groups

API count

id before after diff
data 3301 3302 +1

ESLint disabled line counts

id before after diff
enterpriseSearch 19 21 +2
securitySolution 413 417 +4
total +6

Total ESLint disabled count

id before after diff
enterpriseSearch 20 22 +2
securitySolution 497 501 +4
total +6

History

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

davismcphee
davismcphee approved these changes Jun 8, 2023
View reviewed changes
Copy link
Contributor

@davismcphee davismcphee left a comment

Choose a reason for hiding this comment

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

Code-only review, data/search changes LGTM

@nreese nreese merged commit 59eb4ea into elastic:main Jun 8, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jun 8, 2023
saarikabhasi pushed a commit to saarikabhasi/kibana that referenced this pull request Jun 14, 2023
@nreese @kibanamachine @saarikabhasi
[lens] Add significant terms support as ranking function in Top Values (
612f5e4 
elastic#158962)

Part of elastic#154307
Closes elastic#116421

PR adds `Significance` selection to `Rank by` select. When selected, top
values are fetched with [significant_terms
aggregation](https://www.elastic.co/guide/en/elasticsearch/reference/current/search-aggregations-bucket-significantterms-aggregation.html)

<img width="600" alt="Screen Shot 2023-06-02 at 12 56 29 PM"
src="https://github.com/elastic/kibana/assets/373691/b6a4f70a-4121-4c38-b6d5-7d46dc48ab1a">

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dej611 dej611 dej611 approved these changes

@davismcphee davismcphee davismcphee approved these changes

@lukasolson lukasolson Awaiting requested review from lukasolson

Assignees
No one assigned
Labels
backport:skip This commit does not require backporting Feature:Lens release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.9.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Lens] Support significant terms
7 participants
@nreese @elasticmachine @stratoula @dej611 @kibana-ci @davismcphee @kibanamachine