Add support for HDR percentiles in TSVB visualizations #78306

alexwizp · 2020-09-23T15:33:58Z

Closes: #64238

Summary

Having access to HDR percentiles can be more performant for some queries, as outlined in the Elasticsearch docs.

Describe a specific use case for the feature:

It would be really nice to have access to build these types of percentile aggregations in the TSVB visualization.

This is related to the more broad request #40612 of getting access to HDR support added to regular visualizations.

Screens

Percentile Agg:
Percentile Rank Agg:

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
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

Closes: elastic#64238

alexwizp · 2020-09-23T18:43:57Z

@elasticmachine merge upstream

elasticmachine · 2020-09-24T08:29:37Z

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

alexwizp · 2020-09-24T08:29:46Z

@elasticmachine merge upstream

stratoula

I see the following regression that has to do with the UI actually. On master when I select Band mode it is displayed correctly

but here it is not so good even in larger screens:

Moreover it seems that it fails for negative values. Can we restrict the user from adding them ?

...s/vis_type_timeseries/public/application/components/aggs/percentile_rank/multi_value_row.tsx

stratoula · 2020-09-24T13:10:43Z

...s/vis_type_timeseries/public/application/components/aggs/percentile_rank/percentile_rank.tsx

+            }
+          >
+            <>
+              {model.values && (


Why don't you add the whole EuiFormRow under model.values check here?

Good question, I just moved it from the other place without thinking about it. I see that model.values are not optional fo this agg and should have a value. I think it was added cause in model it's an optional value....

Fixed

stratoula · 2020-09-24T13:16:14Z

...s/vis_type_timeseries/public/application/components/aggs/percentile_rank/percentile_rank.tsx

+            <>
+              {model.values && (
+                <PercentileRankValues
+                  disableAdd={isTablePanel}


I get a warning for the key here when I add multiple Values

I don't reproduce it in master.

@stratoula you are not right, on master I see the same issue. But it's not a reason to don't solve it. Will be fixed in that PR

My mistake @alexwizp, thank you so much for fixing this

alexwizp · 2020-09-25T09:19:03Z

@elasticmachine merge upstream

alexwizp · 2020-09-26T07:38:41Z

@elasticmachine merge upstream

alexwizp · 2020-09-26T07:56:26Z

@cchaos could you please confirm that my fix which found @statula is ok from the design perspective

Before:

After:

I moved some optional for the Band mode fields into new Panel. As a result all looks fine on all screens. It also seems to me that this is more user friendly

@stratoula thank you so much, I forgot about that mode

alexwizp · 2020-09-28T08:19:45Z

@elasticmachine merge upstream

cchaos · 2020-09-28T15:46:11Z

The problem with the panel visually is that it doesn't encompass the whole setting and so it's not clear where it came from or which line will delete it.

My suggestion would be to keep it as a separate line but align it with the above inputs so that the +/delete icons are vertically centered with both lines of forms. Also make sure to use the horizontal form rows to match the above display.

alexwizp · 2020-09-29T17:19:13Z

@cchaos I still think that we should use EuiPanel. What do you think about?:

Percentile:

Percentile Rank:

cchaos · 2020-09-30T14:06:34Z

That's fine. I think we've tried to be more condensed in the past in the TSVB forms. Just be sure to continue to test in smaller window sizes.

alexwizp · 2020-09-30T18:26:18Z

@cchaos on mobile it looks like:

stratoula · 2020-10-01T06:46:42Z

@elasticmachine merge upstream

stratoula

It looks much better now @alexwizp 🙂 LGTM!

stratoula · 2020-10-01T07:30:50Z

@sulemanof can you also check this please?

sulemanof

Code LGTM, tested locally, works as expected (except the thing when I type value of numberOfSignificantValueDigits as magic 5, this breaks my local es snapshot with out of memory exception - not a problem for the PR, but locally should increase the value of memory heap ).

I have only concerns around css here, IMHO it looks strange on different screen sizes.

Screen with high resolution (1920px) - I assume forms should be responsive:

Tablet (790px):

Mobile (the only percentile will take more than screen height):

But this is definitely a question to design team @cchaos

cchaos · 2020-10-01T14:58:43Z

@sulemanof I agree, it's not great. I did emphasize making sure this layout worked well responsively. I'd consider continuing to tweak for all all screen sizes.

alexwizp · 2020-10-07T14:50:38Z

@elasticmachine merge upstream

kibanamachine · 2020-10-07T16:24:51Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: e77c5d4

Metrics [docs]

@kbn/optimizer bundle module count

id	before	after	diff
`visTypeTimeseries`	466	467	+1

async chunks size

id	before	after	diff
`visTypeTimeseries`	1.8MB	1.8MB	+2.4KB

History

💚 Build #78607 succeeded 7f0a927
💚 Build #78011 succeeded d1a772f
💔 Build #77990 failed 41794d6
💔 Build #77982 failed c4ac59b
💚 Build #77767 succeeded 29a7b53

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

* Add support for HDR percentiles in TSVB visualizations Closes: elastic#64238 * remove extra console.log * fix CI * fix PR comments * fix layout * remove legacy injectI18n * fix localization issues Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Kibana Machine <[email protected]>

* Add support for HDR percentiles in TSVB visualizations Closes: #64238 * remove extra console.log * fix CI * fix PR comments * fix layout * remove legacy injectI18n * fix localization issues Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Kibana Machine <[email protected]>

alexwizp added 4 commits September 23, 2020 18:25

Add support for HDR percentiles in TSVB visualizations

4516cb0

Closes: elastic#64238

Merge remote-tracking branch 'upstream/master' into hdr

a4c4804

remove extra console.log

32ad8f8

fix CI

1568a61

Merge branch 'master' into hdr

fe4d5e7

alexwizp requested review from timroes, stratoula, VladLasitsa, sulemanof and DianaDerevyankina September 24, 2020 08:27

alexwizp self-assigned this Sep 24, 2020

alexwizp added Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.10.0 v8.0.0 release_note:enhancement Feature:TSVB TSVB (Time Series Visual Builder) labels Sep 24, 2020

alexwizp marked this pull request as ready for review September 24, 2020 08:29

alexwizp requested a review from a team September 24, 2020 08:29

Merge branch 'master' into hdr

818e39a

stratoula reviewed Sep 24, 2020

View reviewed changes

elasticmachine and others added 2 commits September 25, 2020 05:19

Merge branch 'master' into hdr

8febb1b

fix PR comments

adbb1d7

Merge branch 'master' into hdr

1f22282

Merge branch 'master' into hdr

4ffd34f

elastic deleted a comment from elasticmachine Sep 28, 2020

alexwizp added 2 commits September 29, 2020 11:44

Merge remote-tracking branch 'upstream/master' into hdr

29a7b53

fix layout

c4ac59b

remove legacy injectI18n

41794d6

alexwizp requested a review from cchaos September 29, 2020 17:31

fix localization issues

d1a772f

Merge branch 'master' into hdr

7f0a927

stratoula approved these changes Oct 1, 2020

View reviewed changes

alexwizp added v7.11.0 and removed v7.10.0 labels Oct 1, 2020

sulemanof approved these changes Oct 1, 2020

View reviewed changes

Merge branch 'master' into hdr

e77c5d4

alexwizp merged commit 59d83e6 into elastic:master Oct 7, 2020

alexwizp mentioned this pull request Oct 7, 2020

[7.x] Add support for HDR percentiles in TSVB visualizations (#78306) #79877

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for HDR percentiles in TSVB visualizations #78306

Add support for HDR percentiles in TSVB visualizations #78306

alexwizp commented Sep 23, 2020

alexwizp commented Sep 23, 2020

elasticmachine commented Sep 24, 2020

alexwizp commented Sep 24, 2020

stratoula left a comment

stratoula Sep 24, 2020

alexwizp Sep 26, 2020

stratoula Sep 24, 2020

alexwizp Sep 24, 2020

alexwizp Sep 26, 2020

stratoula Sep 27, 2020

alexwizp commented Sep 25, 2020

alexwizp commented Sep 26, 2020

alexwizp commented Sep 26, 2020

alexwizp commented Sep 28, 2020

cchaos commented Sep 28, 2020

alexwizp commented Sep 29, 2020

cchaos commented Sep 30, 2020

alexwizp commented Sep 30, 2020

stratoula commented Oct 1, 2020

stratoula left a comment

stratoula commented Oct 1, 2020

sulemanof left a comment

cchaos commented Oct 1, 2020

alexwizp commented Oct 7, 2020

kibanamachine commented Oct 7, 2020

Add support for HDR percentiles in TSVB visualizations #78306

Add support for HDR percentiles in TSVB visualizations #78306

Conversation

alexwizp commented Sep 23, 2020

Summary

Screens

Checklist

For maintainers

alexwizp commented Sep 23, 2020

elasticmachine commented Sep 24, 2020

alexwizp commented Sep 24, 2020

stratoula left a comment

Choose a reason for hiding this comment

stratoula Sep 24, 2020

Choose a reason for hiding this comment

alexwizp Sep 26, 2020

Choose a reason for hiding this comment

stratoula Sep 24, 2020

Choose a reason for hiding this comment

alexwizp Sep 24, 2020

Choose a reason for hiding this comment

alexwizp Sep 26, 2020

Choose a reason for hiding this comment

stratoula Sep 27, 2020

Choose a reason for hiding this comment

alexwizp commented Sep 25, 2020

alexwizp commented Sep 26, 2020

alexwizp commented Sep 26, 2020

alexwizp commented Sep 28, 2020

cchaos commented Sep 28, 2020

alexwizp commented Sep 29, 2020

cchaos commented Sep 30, 2020

alexwizp commented Sep 30, 2020

stratoula commented Oct 1, 2020

stratoula left a comment

Choose a reason for hiding this comment

stratoula commented Oct 1, 2020

sulemanof left a comment

Choose a reason for hiding this comment

cchaos commented Oct 1, 2020

alexwizp commented Oct 7, 2020

kibanamachine commented Oct 7, 2020

💚 Build Succeeded

Metrics [docs]

@kbn/optimizer bundle module count

async chunks size

History