Skip to content

[Lens] Legend Statistics feature#182357

Merged
mbondyra merged 30 commits intoelastic:mainfrom
mbondyra:lens/toolbar_redesign_legend_stats
Jun 19, 2024
Merged

[Lens] Legend Statistics feature#182357
mbondyra merged 30 commits intoelastic:mainfrom
mbondyra:lens/toolbar_redesign_legend_stats

Conversation

@mbondyra
Copy link
Copy Markdown
Contributor

@mbondyra mbondyra commented May 2, 2024 •
edited
Loading

Summary

Fixes #183887

Screenshot 2024-06-09 at 21 11 33 Screenshot 2024-06-09 at 21 11 38
  • popover width = 500 px
  • new combobox legend values component added
  • switch show value removed
  • location and alignment is in a single for row
  • auto legend width option to allow users to have the legend automatically size based its contents - not there yet
  • no limit imposed of the number of values
  • when a value is selected, list or table appears
  • when the list is selected, label truncations is not offered and legend items should be forced to have truncation on and clamped to a single line.
  • adds telemetry too - adds events on save if the legend statistics change:

lens_legend_stats- triggered if any stats is in legend

lens_legend_stats_${TYPE} - triggered for specific types, if user has 2 statistics (eg. AVG and MIN) two events are triggered.

and counting how many they use
lens_legend_stats_amount_1
lens_legend_stats_amount_2
lens_legend_stats_amount_3
lens_legend_stats_amount_4_7
lens_legend_stats_amount_above_8

@mbondyra mbondyra added Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// release_note:skip Skip the PR/issue when compiling release notes Feature:Lens v8.15.0 labels May 2, 2024
@mbondyra mbondyra force-pushed the lens/toolbar_redesign_legend_stats branch 5 times, most recently from 8484cff to 26459f8 Compare May 5, 2024 19:11
@mbondyra mbondyra changed the title [Lens] legend redesign in preparation for legend stats [Lens] legend popover redesign in preparation for legend stats May 13, 2024
@mbondyra mbondyra force-pushed the lens/toolbar_redesign_legend_stats branch 2 times, most recently from 285a428 to b49431a Compare May 27, 2024 20:14
@elastic elastic deleted a comment from kibana-ci May 27, 2024
@mbondyra mbondyra force-pushed the lens/toolbar_redesign_legend_stats branch 2 times, most recently from b22aa80 to d1d4681 Compare May 28, 2024 18:57
@mbondyra mbondyra changed the title [Lens] legend popover redesign in preparation for legend stats [Lens] legend popover redesign for legend stats May 28, 2024
@mbondyra mbondyra added release_note:enhancement release_note:feature Makes this part of the condensed release notes and removed release_note:skip Skip the PR/issue when compiling release notes release_note:enhancement labels May 29, 2024
@mbondyra mbondyra force-pushed the lens/toolbar_redesign_legend_stats branch from d1d4681 to e3ae1e2 Compare May 29, 2024 20:15
@markov00 markov00 self-assigned this May 30, 2024
@mbondyra mbondyra marked this pull request as ready for review June 17, 2024 13:12
@mbondyra mbondyra requested a review from a team as a code owner June 17, 2024 13:12
@mbondyra mbondyra requested a review from a team June 17, 2024 13:12
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Jun 17, 2024

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

@elastic elastic deleted a comment from kibana-ci Jun 17, 2024
@mbondyra
Copy link
Copy Markdown
Contributor Author

mbondyra commented Jun 17, 2024

/ci

dej611
dej611 reviewed Jun 17, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@dej611 dej611 left a comment

Choose a reason for hiding this comment

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

The ability to define legend stats for a single series without bucketing is a bit confusing to me:

Screenshot 2024-06-17 at 16 00 25 Screenshot 2024-06-17 at 15 59 24

Is this an expected behaviour or a bug?

Another confusing behaviour, to me, is the fact that Extra large size, when stats are enabled, leads to a shorter layout than Auto:

Screenshot 2024-06-17 at 15 54 14 Screenshot 2024-06-17 at 15 54 05

With percentage charts there are another couple of interesting things (not blocker for this PR, but nice to have a follow up):

  • Diff and Diff % in this case are a bit confusing in the table
  • It would be nice to limit the values to 2 digits by default (see the last row in the picture below)
Screenshot 2024-06-17 at 16 45 19

nickofthyme
nickofthyme reviewed Jun 18, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@nickofthyme nickofthyme left a comment

Choose a reason for hiding this comment

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

Code changes mostly LGTM, just a few comments.

Playing around with the new options feels good. Still testing a few edges before approving.

@gvnmagni
Copy link
Copy Markdown

gvnmagni commented Jun 18, 2024

One little detail that I noticed. When we pick Current or last value from the list and it is the only stat selected, we don't display a table while we just replicate the Lens behavior that we currently have and we display that value next to data series name. As soon as we add an additional stat we display a table

Is it working this way in order to replicate our current behavior and avoid breaking changes or is it something unexpected?

Only stat displayed: Current or last value
Screenshot 2024-06-18 at 10 55 29

Additional stat displayed
Screenshot 2024-06-18 at 10 55 42

@mbondyra
Copy link
Copy Markdown
Contributor Author

mbondyra commented Jun 18, 2024 •
edited
Loading

@dej611

The ability to define legend stats for a single series without bucketing is a bit confusing to me:

Fixed, that's not possible anymore

Another confusing behaviour, to me, is the fact that Extra large size, when stats are enabled, leads to a shorter layout than Auto:

That's because Extra large is just a number (230px) while auto is calculated based on the content. i've consulted with @gvnmagni and we'll follow up on this one

It would be nice to limit the values to 2 digits by default (see the last row in the picture below)

The formatter is taken from the dimension formatter. After talking to @teresaalvarezsoler, I found out that there was a discussion about it, but the decision is not to do anything about it till we hear some user feedback. The user can easily fix it by changing the dimension formatter.

@gvnmagni

Is it working this way in order to replicate our current behavior and avoid breaking changes or is it something unexpected?

Yes, it's because most of the TSVB charts have last or current value shown by default and we don't want these charts to display the table, but the old view. This is something we decided with @markov00 and yourself on one of the syncs we had.

Here's the default TSVB chart, adding the table here as a default would seem a bit invasive:
Screenshot 2024-06-18 at 13 34 13

dej611
dej611 approved these changes Jun 18, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@dej611 dej611 left a comment

Choose a reason for hiding this comment

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

Tested again and it works great now 🚀

nickofthyme
nickofthyme approved these changes Jun 19, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@nickofthyme nickofthyme left a comment

Choose a reason for hiding this comment

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

These changes LGTM, new options work great. I think we might run into issues with elastic/elastic-charts#1686 now that it is easier to create very wide inside legends but that needs to be addressed in @elastic/charts.

@kibana-ci
Copy link
Copy Markdown

kibana-ci commented Jun 19, 2024

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
lens 1477 1478 +1

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
expressionXY 166 169 +3
lens 567 570 +3
visualizations 833 834 +1
total +7

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
expressionXY 128.7KB 129.1KB +419.0B
lens 1.5MB 1.5MB +7.7KB
observability 306.5KB 306.5KB +20.0B
observabilityAIAssistantApp 152.7KB 152.7KB +12.0B
visTypePie 11.3KB 11.3KB -19.0B
visTypeXy 42.3KB 42.3KB -10.0B
total +8.1KB

Page load bundle

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

id before after diff
expressionXY 41.2KB 41.7KB +505.0B
visTypePie 8.5KB 8.4KB -154.0B
visTypeXy 28.7KB 28.5KB -164.0B
visualizations 62.0KB 61.9KB -116.0B
total +71.0B
Unknown metric groups

API count

id before after diff
expressionXY 177 180 +3
lens 669 672 +3
visualizations 864 865 +1
total +7

History

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

cc @mbondyra

@mbondyra mbondyra merged commit 2f4997c into elastic:main Jun 19, 2024
@kibanamachine kibanamachine added the backport:skip This PR does not require backporting label Jun 19, 2024
@mbondyra mbondyra deleted the lens/toolbar_redesign_legend_stats branch June 19, 2024 19:04
@mbondyra mbondyra mentioned this pull request Jun 26, 2024
Open
@maryam-saeidi maryam-saeidi mentioned this pull request Mar 13, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dej611 dej611 dej611 approved these changes

@nickofthyme nickofthyme nickofthyme approved these changes

Assignees

@mbondyra mbondyra

Labels

apm:review backport:skip This PR does not require backporting Feature:Lens release_note:feature Makes this part of the condensed release notes Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// v8.15.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Telemetry] Track usage of the new Statistics in legend

9 participants

@mbondyra @elasticmachine @gvnmagni @kibana-ci @dej611 @nickofthyme @smith @markov00 @kibanamachine