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

[Lens] Supports percentile_ranks aggregation #132430

Merged
merged 18 commits into from
May 31, 2022

Conversation

stratoula
Copy link
Contributor

@stratoula stratoula commented May 18, 2022

Summary

Closes #93938
Closes #45919

Adds support for percentile ranks aggregation in Lens and enables the TSVB to Lens navigation.

image

percentile

Formula usage

percentile_rank(AvgTicketPrice, value=600)

Formula documentation

I took some pieces from the ES documentation as I find it quite explanatory and easy to understand. I am open to feedback!
image

Checklist

Delete any items that are not applicable to this PR.

@@ -87,7 +87,7 @@ describe('triggerTSVBtoLensConfiguration', () => {
...model.series[0],
metrics: [
{
type: 'percentile_rank',
type: 'std_deviation',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ℹ️ This test is testing the scenario that a metric agg is not supported in Lens. As we support now the percentile_rank I changed it to std_deviation that is not supported

undefined,
timeShift
);
}
Copy link
Contributor Author

@stratoula stratoula May 20, 2022

Choose a reason for hiding this comment

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

The label is following the aggBased format
Percentile rank (value) of
image

I could remove the parenthesis but I like it that way, wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good to me!

);
}

const DEFAULT_PERCENTILE_RANKS_VALUE = 0;
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 am defaulting this to 0 as done in TSVB and Agg based.

@stratoula stratoula marked this pull request as ready for review May 20, 2022 15:29
@stratoula stratoula requested review from a team as code owners May 20, 2022 15:29
@stratoula stratoula added the Team:Visualizations Visualization editors, elastic-charts and infrastructure label May 25, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-vis-editors @elastic/kibana-vis-editors-external (Team:VisEditors)

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Added some comments

x-pack/plugins/lens/public/types.ts Outdated Show resolved Hide resolved
data-test-subj="lns-indexPattern-percentile_ranks-input"
compressed
value={inputValue ?? ''}
onChange={handleInputChange}
Copy link
Contributor

Choose a reason for hiding this comment

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

This has the same issue as percentiles in that it's not possible to order terms by a decimal point percentile value: elastic/elasticsearch#66677

The query fails if you try to do it on this PR. We can either limit it in the same way (enforce integers) or we can drop out of the terms sorting and fall back to alphabetical. What do you think @stratoula @ghudgins ? I'm not sure tbh - integers make more sense for percentile because the value range is strictly 0-100 which is not the case for percentile rank (it can have any data range so you wouldn't be able to use it for fields which have a domain of 0-1 which would be really annoying).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Awww nice catch. I was always using integer values and never caught it. Hmmm, I don't know either but I feel that as a user I would prefer adding whatever value I want and not be limited only to integers

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, we probably should fall back to alphabetical sorting (which is weird too, no easy win here)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Check this comment #132430 (comment)

undefined,
timeShift
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good to me!

@stratoula
Copy link
Contributor Author

@flash1293 about the sorting problem I did the following, tell me what you think about it:

  • If the user is using a percentile rank with an integer value I allow the sorting by column
  • If the user is using a percentile rank with a non integer value I default to alphabetical

I thought that with this implementation, only the users with the non-integer values will have this limitation. Wdyt? Otherwise I can disable the column order for all the percentile ranks.

@stratoula stratoula added v8.4.0 and removed v8.3.0 labels May 26, 2022
Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

LGMT, one minor nit. Great work!

x-pack/plugins/lens/public/types.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@jloleysens jloleysens left a comment

Choose a reason for hiding this comment

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

Changes look good to me! Also tested locally. Great work @stratoula ! I left a couple of nits.

export const aggSinglePercentileRank = (): FunctionDefinition => ({
name: aggSinglePercentileRankFnName,
help: i18n.translate('data.search.aggs.function.metrics.singlePercentileRank.help', {
defaultMessage: 'Generates a serialized agg config for a single percentile rank agg',
Copy link
Contributor

Choose a reason for hiding this comment

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

nit casing consistency with other agg fns:

Suggested change
defaultMessage: 'Generates a serialized agg config for a single percentile rank agg',
defaultMessage: 'Generates a serialized agg config for a Single Percentile Rank agg',

Copy link
Contributor Author

@stratoula stratoula May 31, 2022

Choose a reason for hiding this comment

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

I am happy to do this change @jloleysens but the single percentile follows this format https://github.com/elastic/kibana/blob/main/src/plugins/data/common/search/aggs/metrics/single_percentile_fn.ts#L28 so I think that the single percentile rank should also follow the same format. Also I think that we follow our text guildelines with this format.

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
data 513 515 +2
lens 870 871 +1
total +3

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 2863 2871 +8
lens 509 512 +3
total +11

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 +3.6KB
visTypeTimeseries 465.6KB 466.4KB +836.0B
total +4.4KB

Page load bundle

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

id before after diff
data 420.7KB 423.8KB +3.2KB
visTypeGauge 9.9KB 10.0KB +52.0B
visTypeHeatmap 10.4KB 10.5KB +26.0B
visTypeMetric 8.8KB 8.9KB +26.0B
visTypeTable 15.3KB 15.3KB +26.0B
visTypeTagcloud 5.9KB 5.9KB +26.0B
visTypeXy 43.4KB 43.5KB +104.0B
total +3.4KB
Unknown metric groups

API count

id before after diff
data 3478 3486 +8
lens 586 589 +3
total +11

History

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Lens release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.4.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Lens] Percentile rank function [Heatmap] Support percentiles & percentile ranks
5 participants