[Lens] Add percentile function #86490
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
flash1293
added
enhancement
|
Pinging @elastic/kibana-app (Team:KibanaApp) |
flash1293
added
release_note:enhancement
and removed
enhancement
wylieconlon
approved these changes
Dec 22, 2020
x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/percentile.tsx
Outdated
Show resolved
Hide resolved
| !Number.isNaN(inputValueAsNumber) && | ||
| inputValueAsNumber >= 0 && | ||
| inputValueAsNumber <= 100 && | ||
| inputValueAsNumber % 1 === 0; |
There was a problem hiding this comment.
Can you move some of the basic integer checking into helpers? We're using it in 3 places now.
Also, why is 0 a valid percentile?
There was a problem hiding this comment.
Also, why is 0 a valid percentile?
Interesting, Elasticsearch treats the 0th percentile as min and 100th percentile as max, but according to wikipedia this is not how it works in general:
Note that in theory the 0th percentile falls at negative infinity and the 100th percentile at positive infinity, although in
many practical applications, such as test results, natural lower and/or upper limits are enforced.
I suggest we follow the common definition of percentile here and make the allowed range 1-99 inclusive, wdyt? I would also be fine with following Elasticsearch
x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/percentile.tsx
Show resolved
Hide resolved
Comment on lines
182
to
183
| min={0} | ||
| max={100} |
There was a problem hiding this comment.
Suggested change
| min={0} | |
| max={100} | |
| min={1} | |
| max={100} | |
| step={1} |
dej611
reviewed
Dec 23, 2020
| lowerBound?: number | ||
| ) { | ||
| const inputValueAsNumber = Number(inputValue); | ||
| // an input is value if it's not an empty string, parses to a valid number, is between 0 and 100 (inclusive) |
There was a problem hiding this comment.
Where is checked for 0 to 100 inclusiveness?
There was a problem hiding this comment.
The comment was outdated, removed it
Comment on lines
55
to
56
| // on initialization values can be null (from the Infinity serialization), so handle it correctly | ||
| // or they will be casted to 0 by the editor ( see #78867 ) |
There was a problem hiding this comment.
Mind this comment was addressing the isValidNumber implementation here. Probably it needs to be removed
| setInputValue(val); | ||
| }, []); | ||
| return ( | ||
| <> |
There was a problem hiding this comment.
Just a leftover, thanks
| // and is an integer | ||
| const inputValueIsValid = isValidNumber(inputValue, true, 99, 1); | ||
|
|
||
| useDebounce( |
There was a problem hiding this comment.
Mind that this useDebounce implementation triggers two times at bootstrap: there's the useDebounceWithOptions version which can skip the first render
flash1293
added a commit
to flash1293/kibana
that referenced
this pull request
Dec 23, 2020
flash1293
added a commit
that referenced
this pull request
Dec 23, 2020
timroes
added
the
release_note:feature
💔 Build Failed
Failed CI Steps
Test FailuresX-Pack Detection Engine API Integration Tests.x-pack/test/detection_engine_api_integration/security_and_spaces/tests/add_actions·ts.detection engine api security and spaces enabled add_actions adding actions should be able to create a new webhook action and attach it to a ruleStandard OutStack TraceX-Pack Detection Engine API Integration Tests.x-pack/test/detection_engine_api_integration/security_and_spaces/tests/add_actions·ts.detection engine api security and spaces enabled add_actions adding actions should be able to create a new webhook action and attach it to a ruleStandard OutStack TraceChrome X-Pack UI Functional Tests.x-pack/test/functional/apps/api_keys/home_page·ts.API Keys app Home page Loads the appStandard OutStack Traceand 5 more failures, only showing the first 3. Metrics [docs]Module Count
Async chunks
Distributable file count
History
To update your PR or re-run it, just comment with: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #74574
Release Notes: Adds percentile to Lens.
Behavior
This PR adds a single level percentile operation. It works very similar to the median operation, including an input to enter the desired percentile. On creating a new column, it defaults to the 95th percentile. The percentile input is a debounced number input. Changes are validated to be a valid integer between 0 and 100 (inclusively). Changing the value also updates the label as long as no custom label has been set. If a top values function is used, it is sorted by a percentile column if available.
Non-integer percentiles are disallowed because Elasticsearch can't order a terms agg by them (elastic/elasticsearch#66677) - in this case it felt more important to me to keep the default sorting behavior we are using for other metrics as well than to allow decimal points in the percentile.
Technical changes
percentilegetEsAggsSuffix. This is necessary because both the bucket path for sorting the terms agg and the column containing the result of the percentile calculation include a suffix (.<percentile>, e.g..95). This is implemented by the percentile operation and used by the terms agg (to set theorderByargument) and thetoExpressionfunction (to map the column correctly). If we implement single value percentile rank at some point, it can use the some logictoEsAggsFnfunction - this is required so top values can look up the referenced metric column to get the suffix for building the order by pathConsiderations
I considered using a slider component for the value as the allowed range is known, but I decided against it because IMHO when using percentiles the exact value matters and is known before configuring it. Because of this, it felt strange to me using a slider to configure it.