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] Allow decimal percentiles like 99.5 #98853

Closed
wylieconlon opened this issue Apr 29, 2021 · 5 comments · Fixed by #165703
Closed

[Lens] Allow decimal percentiles like 99.5 #98853

wylieconlon opened this issue Apr 29, 2021 · 5 comments · Fixed by #165703
Assignees
@dej611
Labels
enhancement New value added to drive a business result Feature:Lens impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. Team:Visualizations Visualization editors, elastic-charts and infrastructure

Comments

@wylieconlon
Copy link
Contributor

Currently the Percentile function validates that the percentile is an integer, but Elasticsearch supports decimal percentiles. We should still validate that the input is a valid number, but this is a change to the validation logic only.
@wylieconlon wylieconlon added good first issue low hanging fruit enhancement New value added to drive a business result Team:Visualizations Visualization editors, elastic-charts and infrastructure Feature:Lens labels Apr 29, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@flash1293
Copy link
Contributor

flash1293 commented Apr 30, 2021 •
edited
Loading

I should have documented this somewhere. When implementing this feature we decided to go with the current approach because decimal percentiles have a limitation - terms aggs can't be sorted by them: elastic/elasticsearch#66677

So the trade-off here is: Confusing user behavior because top values suddenly can't be sorted by percentile anymore for no clear reason or limiting the user and not allowing decimal percentiles in the first place.

Happy to open the discussion again whether we made the right trade-off here.

I'm going to remove good first issue and fix-it week label until we decided what we want to do.

@flash1293 flash1293 mentioned this issue Apr 30, 2021
Closed
@wylieconlon
Copy link
Contributor Author

Good clarification @flash1293, it looks like you did write this in the original PR for percentiles but I forgot about this. I would consider this feature blocked until we can get the ES sorting, as it would be a very bad experience.

@stratoula stratoula added the impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. label Feb 15, 2023
@stratoula
Copy link
Contributor

We can do the same as I did on the percentile ranks #132430 (comment)

@dej611
Copy link
Contributor

dej611 commented Aug 8, 2023

It would be nice to notify the user of such change.
Perhaps on panel close if a decimal percentile is saved a toast could notify the user about the ranking change?
The same UX is used for the random sampling when picking the min/max function, so it's at least consistent.

@stratoula stratoula added impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. and removed blocked impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. labels Aug 28, 2023
@dej611 dej611 mentioned this issue Sep 5, 2023
Merged
2 tasks
@dej611 dej611 self-assigned this Sep 5, 2023
dej611 added a commit that referenced this issue Sep 12, 2023
@dej611 @stratoula
[Lens] Add support for decimals in percentiles (#165703)
b796f13 
## Summary

Fixes #98853

This PR adds support for decimals (2 digits) in percentile operation.


![percentile_decimals_support](https://github.com/elastic/kibana/assets/924948/cd0d2901-ba6f-452e-955c-f9d774a4e27f)

Features:
* ✨ Add decimals support in percentile
  * 🐛 Fixed aggs optimization to work with decimals
* 💄 Show Toast for ranking reset when using decimals in both
percentile and percentile rank
* ✅ Extended `isValidNumber` to support digits check and added unit
tests for it
* ♻️ Added support also to `convert to Lens` feature

Added both unit and functional tests.


![percentile_rank_toast](https://github.com/elastic/kibana/assets/924948/a9be1f9f-a1b1-4f9f-90dc-55e2af8933e1)

When trying to add more digits than what is supported then it will show
the input as invalid:
<img width="347" alt="Screenshot 2023-09-05 at 12 24 03"
src="https://github.com/elastic/kibana/assets/924948/3c38474f-b78f-4144-bca7-3dc192313c09">

Also it works now as custom ranking column:
<img width="264" alt="Screenshot 2023-09-05 at 16 14 25"
src="https://github.com/elastic/kibana/assets/924948/cb7be312-7f7b-4dc1-95a3-d893de344585">
<img width="264" alt="Screenshot 2023-09-05 at 16 14 20"
src="https://github.com/elastic/kibana/assets/924948/1c13f66e-da78-4df6-bb4f-e811d47b66d5">

**Notes**: need to specify exact digits in percentile (2) because the
`any` step is not supported and need to specify a number. I guess
alternatives here are to either extend it to 4 digits or make it a
configurable thing.

### Checklist

Delete any items that are not applicable to this PR.

- [x] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

---------

Co-authored-by: Stratoula Kalafateli <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@dej611 dej611

Labels
enhancement New value added to drive a business result Feature:Lens impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. Team:Visualizations Visualization editors, elastic-charts and infrastructure
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[Lens] Add support for decimals in percentiles dej611/kibana
5 participants
@wylieconlon @dej611 @flash1293 @elasticmachine @stratoula