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] Value in legend #101353

Merged
merged 5 commits into from
Jun 7, 2021
Merged

Conversation

flash1293
Copy link
Contributor

@flash1293 flash1293 commented Jun 4, 2021

Fixes #95888

Adds a setting for showing value in the legend. It's shown for all xy charts

Screenshot 2021-06-04 at 10 00 30

Screenshot 2021-06-04 at 10 00 16

@flash1293 flash1293 added release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 Feature:Lens v7.14.0 auto-backport Deprecated - use backport:version if exact versions are needed labels Jun 4, 2021
@flash1293 flash1293 marked this pull request as ready for review June 4, 2021 11:12
@flash1293 flash1293 requested a review from a team June 4, 2021 11:12
@elasticmachine
Copy link
Contributor

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

Copy link
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.

Code LGTM. I would consider to add it also to partition charts.

When testing the feature I found some unexpected cases:

  • When there's no date histogram by default there are no values

Screenshot 2021-06-04 at 15 21 11

Screenshot 2021-06-04 at 15 32 23

I understand the problem of "picking" the last entry in this configuration, but as is it's pretty confusing.

  • There's a problem with empty buckets in date histogram:
    • if a category has been hit before, then the empty bucket becomes 0. This becomes even more strange when only some categories are shown as 0 while others don't

lens_legend_values_2

  • if the empty bucket is at the left of the first data point in a date histogram then the "last bucket" value is shown - which is a bit confusing

lens_legend_values

@flash1293
Copy link
Contributor Author

flash1293 commented Jun 4, 2021

@dej611

I would consider to add it also to partition charts

I don't think it adds much value there. This is done mostly because it's a small step towards feature parity with TSVB.

The other things you noted are dictated by elastic-charts, so I think changing them is outside of the scope of this PR - I guess it would make sense to open an issue (or multiple issues) on the charts repo for this.

The question we have to answer is whether these things would stop us from exposing the functionality in Lens.
What do you think @ghudgins ?

@ghudgins
Copy link
Contributor

ghudgins commented Jun 4, 2021

@dej611's points are valid but, having now tried it, I don't think it's too bad. at least personally it's sort of how I expect these sorts of hover value features to work. @MichaelMarcialis might feel differently though.

I think this is worth shipping with those known areas of polish. We should make sure we create those follow-on issues while it's fresh. Regarding partition/proportion visualizations and this feature, to me this feature adds the most value to dense line graphs so it's okay to ship without.

This option decreases the amount of left/right legend label space and increases the need for more legend controls - #100231 & #41418 & elastic/elastic-charts#580
image

...but again, still worth it. It's adding more value than we had yesterday (so to speak)

@dej611
Copy link
Contributor

dej611 commented Jun 4, 2021

@dej611's points are valid but, having now tried it, I don't think it's too bad. at least personally it's sort of how I expect these sorts of hover value features to work. @MichaelMarcialis might feel differently though.

I think the thing which confused me mostly was starting with an ordinal chart, and see nothing when I've enabled the feature - or better: there's a left "jump" of the legend label. So at first I thought it was broken - didn't realise it also worked "on hover" as there's already the popover for that.

Then I moved to date histogram and it was showing something (although some of the bugs above may affect also here), so I tried to hover the chart and saw the interactive value changing.

So, for ordinal charts, I see no particular value added compared to the current popover.

@flash1293
Copy link
Contributor Author

@dej611 What are you suggesting?

@dej611
Copy link
Contributor

dej611 commented Jun 4, 2021

I propose to remove it for ordinal charts, if you agree. Or alternatively to find some "default" value to show in this case. What do you think?

@flash1293
Copy link
Contributor Author

flash1293 commented Jun 4, 2021

@dej611 I wanted to avoid the extra complexity because it's such a minor thing, but I see what you are saying. Check out the latest version - the switch is only shown for non-ordinal charts.

@flash1293
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

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
lens 160 161 +1

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.3MB 1.3MB +1.6KB
Unknown metric groups

API count

id before after diff
lens 171 172 +1

History

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

@flash1293 flash1293 merged commit 3930749 into elastic:master Jun 7, 2021
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Jun 7, 2021
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Jun 7, 2021
@MichaelMarcialis
Copy link
Contributor

I appear to be joining the conversation a few hours late, but I still wanted to chime in. I think the value this PR adds still warrants the merging, but I also share some of the concerns that @dej611 raises above. Out of curiosity, have we considered the following?

When there's no date histogram by default there are no values

Can we show the total count of each legend item across the entire chart prior to hovering? That way, we have something to show for each legend item regardless if the chart is a date histogram and regardless if the user isn't hovering.

if the empty bucket is at the left of the first data point in a date histogram then the "last bucket" value is shown - which is a bit confusing

I personally find that having the default, not-hovering legend item count only include the last interval in the date histogram very confusing. From my perspective, I would imagine that when not hovering the chart, it would show an overall total count for each legend item (as described above). Only when hovering over the chart would it then scope the count to that particular hover area.

I'm not a data scientist however, so if these assumptions are incorrect, please feel free to contradict :)

@flash1293
Copy link
Contributor Author

flash1293 commented Jun 7, 2021

@MichaelMarcialis These things are fully controlled by elastic-charts and IMHO should be raised in a separate issue over there if we feel like we want to change this. I have some thoughts around it and think the current behavior makes sense though. Real quick (but IMHO any further discussion should happen in elastic-charts):

  • Showing the last value is a time series thing - the last bucket is the most relevant for the current state of things
  • I would imagine that when not hovering the chart, it would show an overall total count for each legend item - this might be a good idea for a chart using count of record as metric, but we have many other aggregations (even free style formula soon), and summing up all values of a series often doesn't make any sense. We might be able to only do this in certain cases, but then it's not obvious anymore what that value means just by looking at a chart. I don't think we should not try to be too clever here.

gmmorris added a commit to gmmorris/kibana that referenced this pull request Jun 7, 2021
* master: (90 commits)
  Fix UI breaks on providing long search keyword in 'Search Box' (elastic#101385)
  Adds css class to EuiDescriptionListDescription in order to break word on exception details card (elastic#101481)
  [Lens] Increase timings for drag and drop tests (elastic#101380)
  [Lens] Fix editor react error on configuration panel (elastic#101367)
  [Fleet] Move integrations to a separate app (elastic#99848)
  Fix incorrect message displayed on importing Timeline Templates (elastic#101288)
  [Cases] RBAC (elastic#95058)
  [APM] Visual improvements for new APM layout with left navigation (elastic#101360)
  [master] More precise alerts matching (elastic#99820)
  [Lens] Value in legend (elastic#101353)
  Revert "[Reporting] ILM policy for managing reporting indices (elastic#100130)" (elastic#101358)
  [Discover] Fix header row of data grid in Firefox (elastic#101374)
  Add link to advanced setting in Discover (elastic#101154)
  Url service locators (elastic#101045)
  [Timelion] Update the removal message to mention the exact version (elastic#100994)
  [Security Solution][Detection Engine] Test cases for alias failure test cases where we don't copy aliases correctly (elastic#101437)
  [Event Log] Adding `type_id` to saved object array in event log (elastic#100939)
  [Reporting] Add `location.url` info to console message logs (elastic#101427)
  [Security Solutions][Detection Engine] Fixes timestamp bugs within source indexes when the formats are not ISO8601 format (elastic#101349)
  Improve Task Manager instrumentation (elastic#99160)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed Feature:Lens release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.14.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Lens] Show value in legend
6 participants