[Lens] Add support for decimals in percentiles #165703

dej611 · 2023-09-05T14:17:08Z

Summary

Fixes #98853

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

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.

When trying to add more digits than what is supported then it will show the input as invalid:

Also it works now as custom ranking column:

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.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Unit or functional tests were updated or added to match the most common scenarios

elasticmachine · 2023-09-08T13:54:55Z

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

stratoula · 2023-09-08T14:29:17Z

This works great, I just feel that the notification needs some refinement.

@MichaelMarcialis @amyjtechwriter can you help us here?

This notification appears in the following scenario. I have a percentile with an integer value and a top values dimension with rank by the metric(percentiles) and then go back to the metric and change the percentile from an integer to a decima value. At this point we want to notify the users that we changed automatically the rank by to alphabetical (ES limitation elastic/elasticsearch#66677)

timductive · 2023-09-11T16:16:52Z

x-pack/plugins/lens/public/datasources/form_based/operations/definitions/helpers.tsx

 export function isValidNumber(
  inputValue: string | number | null | undefined,
  integer?: boolean,
  upperBound?: number,
-  lowerBound?: number
+  lowerBound?: number,
+  digits: number = 2


why are we limiting it to 2 decimals? I could imagine an SRE wanting up to 4.

Yes I agree with that comment, do we need a limit?

I see what you mean. It's a simple constant change, so I'll update.

Yes I agree with that comment, do we need a limit?

Unfortunately yes. I've asked to add the support to any but it is not coming in Range components: elastic/eui#5703

timductive · 2023-09-11T16:20:09Z

x-pack/plugins/lens/public/datasources/form_based/dimension_panel/dimension_editor.tsx

+            defaultMessage: 'Top values ranking reset',
+          }),
+          text: i18n.translate('xpack.lens.uiInfo.rankingResetToAlphabetical', {
+            defaultMessage: 'The use of decimals for this function is not supported for ranking.',


Maybe something like "Ranking reset to alphabetical rank. Please only use whole numbers to rank by percentile."

Updated with latest suggestions by @gchaps

timductive · 2023-09-11T16:23:24Z

When trying to add more digits than what is supported then it will show the input as invalid:
Screenshot 2023-09-05 at 12 24 03

This message could also use some help. Can we override the text here to say something closer to "Please limit to 2 decimal places"

gchaps · 2023-09-11T20:39:55Z

Here are some suggestions for the two messages.

Suggestions for the ranking message

Ranking changed to alphabetical
To rank by percentile, use whole numbers only.

Suggestions for the decimal place meesage:

Only 2 numbers allowed after the decimal point.
Only 2 decimal places allowed.

MichaelMarcialis · 2023-09-11T21:07:53Z

When trying to add more digits than what is supported then it will show the input as invalid:

@dej611: Rather than showing the input as invalid (and asking that the user manually fix the problem themselves), would it be better for us to round to the nearest valid decimal on blur for the user? For example, if user enters 99.568180, the value automatically changes to 99.57 on blur. I feel like that may be a better user experience.

This notification appears in the following scenario. I have a percentile with an integer value and a top values dimension with rank by the metric(percentiles) and then go back to the metric and change the percentile from an integer to a decima value. At this point we want to notify the users that we changed automatically the rank by to alphabetical (ES limitation elastic/elasticsearch#66677)

@dej611, @stratoula, @timductive: Are we absolutely certain we want to automatically change the top value dimension ranking when the user attempts to enter a decimal value for the metric percentile? I'm not an expert on these more advanced aggregations, but to my mind that feels backwards. I think it may make more sense and be less destructive to simply not allow users to enter a percentile with a decimal value if/when a top value dimension ranking is set to rank by metric. Per my comment above, could we not simply round any decimal entered to the nearest whole number automatically on blur (as well as conditionally add help text below the form element when it is detected that a top value dimension is ranked by metric and indicate to users that decimals are not supported)?

In that case, we wouldn't need this toast at all.

stratoula · 2023-09-12T06:07:21Z

Per my comment above, could we not simply round any decimal entered to the nearest whole number automatically on blur (as well as conditionally add help text below the form element when it is detected that a top value dimension is ranked by metric and indicate to users that decimals are not supported)?

Not sure how it will work on blur because if I am not mistaken the request will be sent in ES and it will fail. Except from that we could not allow decimals if a top values breakdown rank by metric is detected. The problem I see in this approach is that the users need to go then to top values, fix the ranking and then change the decimals. With the current approach we do it automatically. Decimals are important in percentiles and percentiles ranks. If a user wants 99.9 they are not going to be happy with 99. We are already doing this in percentile ranks btw and we didn't have any complain afaik.

dej611 · 2023-09-12T09:11:03Z

why are we limiting it to 2 decimals? I could imagine an SRE wanting up to 4.

Latest changes raised the limit to 4 decimals. (Note while testing inline: there's currently an EUI bug with integer check and 8 digits: elastic/eui#7180 )
The generic invalid message has also been updated:

This message could also use some help. Can we override the text here to say something closer to "Please limit to 2 decimal places"

Updated:

Here are some suggestions for the two messages.
Suggestions for the ranking message

Ranking changed to alphabetical
To rank by percentile, use whole numbers only.

Done

dej611 · 2023-09-12T09:12:34Z

The problem I see in this approach is that the users need to go then to top values, fix the ranking and then change the decimals. With the current approach we do it automatically.

Also the other way around (decimals => integer) will automagically update the ranking logic (this has always been like that in Lens).

kibana-ci · 2023-09-12T10:53:57Z

💚 Build Succeeded

Buildkite Build
Commit: 866bf01

Metrics [docs]

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.4MB	1.4MB	+1.7KB
`visDefaultEditor`	141.0KB	141.0KB	+39.0B
`visualizations`	264.7KB	264.7KB	+79.0B
total			+1.8KB

History

💔 Build #158248 failed 4562655
💚 Build #157879 succeeded bf07c1c
💔 Build #157651 failed 20a8691
💔 Build #157619 failed dffd07e
💔 Build #157582 failed 136de0b

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

stratoula

LGTM, this works great! I tested the new changes in Chrome, I really like the new message!

MichaelMarcialis · 2023-09-12T13:41:35Z

Not sure how it will work on blur because if I am not mistaken the request will be sent in ES and it will fail.

@stratoula: Are we completely unable to intercept and check the value before it is sent to ES?

The problem I see in this approach is that the users need to go then to top values, fix the ranking and then change the decimals. With the current approach we do it automatically. Decimals are important in percentiles and percentiles ranks. If a user wants 99.9 they are not going to be happy with 99.

@stratoula: I can understand that desire to make the process as automated as possible. But personally, I'd rather be notified of what needs to change in this case rather than having that ranking change forced upon me. I don't believe use of a decimal percentile is a valid indicator that the user also wishes to change their top value ranking. I think it's more likely that the user simply isn't aware of the limitation that they can't use decimal percentiles in that specific scenario.

stratoula · 2023-09-12T14:17:22Z

I don't believe use of a decimal percentile is a valid indicator that the user also wishes to change their top value ranking. I think it's more likely that the user simply isn't aware of the limitation that they can't use decimal percentiles in that specific scenario.

Absolutely, you are def right here. I guess it depends on the user and what they want to do after all. We are using this kind of automations on other places too.

We can go with the current experience as is the same with the one we have on percentile ranks and if we get any complains we can adjust.

@timductive @ninoslavmiskovic wdyt ?

ninoslavmiskovic · 2023-09-13T05:31:15Z

I like the automated approach and believe this would be sufficient for the user. The granularity is important in this case and I'm not sure the user would take the round trip based on a toast, if we prompt the user, but rather be unsatisfied.

So I am ++ on automation and nice with the 4 digits.

dej611 added 4 commits September 5, 2023 15:59

✨ Add support for decimals percentiles

0f90506

✨ Reset ranking on decimals in percentile

d4397df

💄 Show toast on unsupported ranking column

0be13e8

✅ Add test

d7a881f

dej611 added release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure Feature:Lens v8.11.0 labels Sep 5, 2023

dej611 added 10 commits September 5, 2023 17:18

✅ Fix test

72cd932

✅ Fix test

62625b3

🐛 Fix bug with optimize aggs

949a963

🐛 Enable convert for percentiles with decimals

2d691a8

✨ Enable FTR testing for percentiles

fad8ad2

✅ Add test for multiple optimized percentiels

03a6ee5

✅ Add test for percetiles convert

c90c8cb

✅ fix tests

4412323

✅ Fix tests

136de0b

✅ Fix tests

dffd07e

dej611 marked this pull request as ready for review September 8, 2023 13:54

dej611 requested a review from a team as a code owner September 8, 2023 13:54

Merge branch 'main' into feature/98853

20a8691

Merge branch 'main' into feature/98853

bf07c1c

timductive reviewed Sep 11, 2023

View reviewed changes

👌 Integrated feddback

4562655

🔥 remove old translations

866bf01

stratoula approved these changes Sep 12, 2023

View reviewed changes

dej611 merged commit b796f13 into elastic:main Sep 12, 2023

kibanamachine added the backport:skip This commit does not require backporting label Sep 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Lens] Add support for decimals in percentiles #165703

[Lens] Add support for decimals in percentiles #165703

dej611 commented Sep 5, 2023 •

edited

Loading

elasticmachine commented Sep 8, 2023

stratoula commented Sep 8, 2023 •

edited

Loading

timductive Sep 11, 2023

stratoula Sep 12, 2023

dej611 Sep 12, 2023 •

edited

Loading

dej611 Sep 12, 2023

timductive Sep 11, 2023

dej611 Sep 12, 2023

timductive commented Sep 11, 2023

gchaps commented Sep 11, 2023

MichaelMarcialis commented Sep 11, 2023 •

edited

Loading

stratoula commented Sep 12, 2023

dej611 commented Sep 12, 2023

dej611 commented Sep 12, 2023

kibana-ci commented Sep 12, 2023

stratoula left a comment

MichaelMarcialis commented Sep 12, 2023

stratoula commented Sep 12, 2023 •

edited

Loading

ninoslavmiskovic commented Sep 13, 2023

[Lens] Add support for decimals in percentiles #165703

[Lens] Add support for decimals in percentiles #165703

Conversation

dej611 commented Sep 5, 2023 • edited Loading

Summary

Checklist

elasticmachine commented Sep 8, 2023

stratoula commented Sep 8, 2023 • edited Loading

timductive Sep 11, 2023

Choose a reason for hiding this comment

stratoula Sep 12, 2023

Choose a reason for hiding this comment

dej611 Sep 12, 2023 • edited Loading

Choose a reason for hiding this comment

dej611 Sep 12, 2023

Choose a reason for hiding this comment

timductive Sep 11, 2023

Choose a reason for hiding this comment

dej611 Sep 12, 2023

Choose a reason for hiding this comment

timductive commented Sep 11, 2023

gchaps commented Sep 11, 2023

MichaelMarcialis commented Sep 11, 2023 • edited Loading

stratoula commented Sep 12, 2023

dej611 commented Sep 12, 2023

dej611 commented Sep 12, 2023

kibana-ci commented Sep 12, 2023

💚 Build Succeeded

Metrics [docs]

Async chunks

History

stratoula left a comment

Choose a reason for hiding this comment

MichaelMarcialis commented Sep 12, 2023

stratoula commented Sep 12, 2023 • edited Loading

ninoslavmiskovic commented Sep 13, 2023

dej611 commented Sep 5, 2023 •

edited

Loading

stratoula commented Sep 8, 2023 •

edited

Loading

dej611 Sep 12, 2023 •

edited

Loading

MichaelMarcialis commented Sep 11, 2023 •

edited

Loading

stratoula commented Sep 12, 2023 •

edited

Loading