Skip to content

[Lens] Combined histogram/range aggregation for numbers#76121

Merged
dej611 merged 73 commits intoelastic:masterfrom
dej611:feature/lens/histograms
Sep 23, 2020
Merged

[Lens] Combined histogram/range aggregation for numbers#76121
dej611 merged 73 commits intoelastic:masterfrom
dej611:feature/lens/histograms

Conversation

@dej611
Copy link
Copy Markdown
Contributor

@dej611 dej611 commented Aug 27, 2020 •
edited
Loading

Summary

This PR introduces the combined histogram/range aggregation in Lens as a single operation.

  • When opening the panel the Granularity range input set to the middle of the range

Screenshot 2020-09-18 at 12 10 33

  • It is possible to manually change the Granularity level (now there are 7 "ticks" available)

Screenshot 2020-09-18 at 12 09 07

auto-ranges-tick

  • Or go fully manual and explicitly set each interval/range

Screenshot 2020-09-18 at 12 11 38

  • New range intervals will pick the previous value as new from value:

Screenshot 2020-09-18 at 12 11 59

  • In case the last range is "open" (the to value is +Infinity) then it's still ok:

Screenshot 2020-09-18 at 12 14 05

  • It is possible to drag and drop range intervals
    auto-ranges-dnd

  • An invalid range interval happens when from > to

Screenshot 2020-09-18 at 12 16 57

This PR is still a WIP, the full list of tasks to complete is the following:

  • Fixes
    • Make the range results render
      • Fix the x interval label formatting + tooltips
      • Fix the filtering selection issue
    • Make the Max bars work correctly => Now unified range input
    • Workout the correct step values for the Granularity UI => compute it based on STEPS and MAX_HISTOGRAM_BARS configured
    • Add range/intervals validation logic
    • Add Tooltips to the range forms panel
    • Revisit the Granularity UI
    • Revisit the Auto interval UI
    • Fix the Drag and Drop re-order issue in Advanced Editor
  • Missing features

Related issue #59424

Checklist

Delete any items that are not applicable to this PR.

@dej611 dej611 added release_note:enhancement Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// v8.0.0 Feature:Lens v7.10.0 labels Aug 27, 2020
@dej611 dej611 mentioned this pull request Sep 3, 2020
Closed
@cchaos cchaos self-requested a review September 4, 2020 19:11
@dej611
Copy link
Copy Markdown
Contributor Author

dej611 commented Sep 7, 2020

@cchaos @AlonaNadler The PR is in good shape to be reviewed now.

The panel does not show the Max Intervals (bars) anymore now, as after a discussion with @flash1293, I decided to stop at the development for a good MVP first, instead of an all-in-one solution.

I think that probably the Max Intervals (bars) feature may require some additional discussion about what the user may expect from it, and how to retrieve all the data in order to make that happen, which should be held in a future enhancement step. I preferred to give priority to the base functionality as for now in this PR: let me know what you think.

@flash1293 flash1293 mentioned this pull request Sep 8, 2020
Closed
3 tasks
wylieconlon
wylieconlon suggested changes Sep 8, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@wylieconlon wylieconlon left a comment

Choose a reason for hiding this comment

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

Most of this review is based on the idea that there should be consistency between how we expose these options in Visualize and Lens, not because we can't do something different, but because I don't think these differences are intentional.

@dej611 dej611 mentioned this pull request Sep 18, 2020
Closed
@dej611
Copy link
Copy Markdown
Contributor Author

dej611 commented Sep 18, 2020

@wylieconlon @AlonaNadler @cchaos the PR is up-to-date with the latest design anche changes requested.
Please let me know your feedback about this state.

@dej611
Copy link
Copy Markdown
Contributor Author

dej611 commented Sep 21, 2020

@elasticmachine merge upstream

flash1293
flash1293 reviewed Sep 21, 2020
View reviewed changes
Copy link
Copy Markdown
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.

I remember we talked about fixing the "Over time" case to always put the date on the x axis if it's present, but this doesn't seem to be in the latest version:
Screenshot 2020-09-21 at 12 05 24

Could you check what happened there?

Otherwise this looks good to me

@dej611
Copy link
Copy Markdown
Contributor Author

dej611 commented Sep 21, 2020

@flash1293 I think this reverted it to handle in a different way: 41a937c (#76121)

It broke the original behaviour but perhaps there's a way to have the same result at higher level?

@flash1293
Copy link
Copy Markdown
Contributor

flash1293 commented Sep 21, 2020

@dej611 We can sort that out separately, thanks for linking to the change

wylieconlon
wylieconlon approved these changes Sep 21, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@wylieconlon wylieconlon left a comment

Choose a reason for hiding this comment

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

Very nice work! I think we can merge this and make some behavioral tweaks later.

x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/ranges/constants.ts
export const DEFAULT_INTERVAL = 1000;
export const AUTO_BARS = 'auto';
export const MIN_HISTOGRAM_BARS = 1;
export const SLICES = 6;
Copy link
Copy Markdown
Contributor

@wylieconlon wylieconlon Sep 21, 2020

Choose a reason for hiding this comment

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

My opinion on the granularity settings has changed a bit since we last talked about it. The main reason is that with these settings, there is a big jump from 1 bar to 10-16 bars, which prevents me from getting anything in between. I'm not sure what the right tweak is, but if I could hardcode my "optimal" steps it might be something like [1, 5, 10, 20, 40, 80, 100, 200].

Copy link
Copy Markdown
Contributor Author

@dej611 dej611 Sep 22, 2020

Choose a reason for hiding this comment

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

I have some concerns about an hardcoded set of values:

  1. we might have to think about a strategy in case the user specify a maxHistogramBars value which is not covered in the list
  2. because the range input starts with the middle mark, the list might have an odd number of values

Because of 1) I feel we may fall again under a slices approach - maybe it's a rare scenario ok -, while for 2) it's more a stylistic reason.

@AlonaNadler
Copy link
Copy Markdown

AlonaNadler commented Sep 22, 2020

Great work @dej611 👍 👏
The PR looks great, big improvement.

  • the auto ranges works great. The manual custom intervals work fantastic.
    Few minor things:
  • Sometimes when I click on the + nothing happens. But once I clicked on the + multiple times the granularity changes. Ideally every click on + or - changes the preview. example:

Sep-22-2020 07-00-52

  • We need a text review doc.

@dej611
Copy link
Copy Markdown
Contributor Author

dej611 commented Sep 22, 2020

Sometimes when I click on the + nothing happens. But once I clicked on the + multiple times the granularity changes. Ideally every click on + or - changes the preview. example:

@AlonaNadler I see.
That is relative to the number of slices set right now compared to the dataset whole range. Even if the numbers of slices goes from 7 to 5 it will still happen based on the dataset size for smaller ones.

@flash1293
Copy link
Copy Markdown
Contributor

flash1293 commented Sep 22, 2020

Sometimes when I click on the + nothing happens. But once I clicked on the + multiple times the granularity changes. Ideally every click on + or - changes the preview. example:

This is a limitation we can't work around right now because of the reasons discussed in the last weekly (we don't know the actual data distribution in the config UI). I think the current UI does a good job at working with this limitation but it's not perfect. Once we expose the currently rendered data we can think about how to improve this experience.

@dej611 dej611 requested a review from cchaos September 22, 2020 15:48
cchaos
cchaos approved these changes Sep 22, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

@dej611 I pushed a design cleanup commit essentially adding some tooltips to the buttons and fixing accessibility and layouts. LGTM from my side. It would be nice if we could eventually add a tooltip generally to the "Granularity" label explaining the concept, but I won't hold up this PR anymore.

@dej611 dej611 self-assigned this Sep 23, 2020
@dej611 dej611 merged commit 0f8043c into elastic:master Sep 23, 2020
@dej611 dej611 deleted the feature/lens/histograms branch September 23, 2020 08:23
@dej611 dej611 linked an issue Sep 23, 2020 that may be closed by this pull request
[Lens] Create combined histogram/range aggregation for numbers #59424
Closed
@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented Sep 23, 2020

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

id value diff baseline
lens 461 +14 447

page load bundle size

id value diff baseline
lens 1.1MB +30.2KB 1.1MB

History

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

gmmorris added a commit to gmmorris/kibana that referenced this pull request Sep 23, 2020
@gmmorris
Merge branch 'master' into actions/pagerduty-dedup
097b99d 
* master: (31 commits)
  skip tests for old pacakge (elastic#78194)
  [Ingest Pipelines] Add url generator for ingest pipelines app (elastic#77872)
  [Lens] Rename "telemetry" to "stats" (elastic#78125)
  [CSM] Url search (elastic#77516)
  [Drilldowns] Config to disable URL Drilldown  (elastic#77887)
  [Lens] Combined histogram/range aggregation for numbers (elastic#76121)
  Remove legacy plugins support (elastic#77599)
  'Auto' interval must be correctly calculated for natural numbers (elastic#77995)
  [CSM] fix ingest data retry order messed up (elastic#78163)
  Add response status helpers (elastic#78006)
  Bump react-beautiful-dnd (elastic#78028)
  [Security Solution][Detection Engine] Bubbles up more error messages from ES queries to the UI (elastic#78004)
  Index pattern  - refactor constructor (elastic#77791)
  Add `xpack.security.sameSiteCookies` to docker allow list (elastic#78192)
  Remove [key: string]: any; from IIndexPattern (elastic#77968)
  Remove requirement for manage_index_templates privilege for Index Management (elastic#77377)
  [Ingest Manager] Agent bulk actions UI (elastic#77690)
  [Metrics UI] Add inventory view timeline (elastic#77804)
  Reporting/Docs: Updates for setting to enable CSV Download (elastic#78101)
  Update to latest rum-react (elastic#78193)
  ...
@dej611 dej611 mentioned this pull request Sep 23, 2020
Merged
dej611 added a commit to dej611/kibana that referenced this pull request Sep 23, 2020
@dej611 @cchaos
@wylieconlon @elasticmachine @mbondyra
[Lens] Combined histogram/range aggregation for numbers (elastic#76121)
e64e2eb 
Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
Co-authored-by: Wylie Conlon <wylieconlon@gmail.com>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Co-authored-by: Marta Bondyra <marta.bondyra@elastic.co>
Co-authored-by: cchaos <caroline.horn@elastic.co>
dej611 added a commit that referenced this pull request Sep 23, 2020
@dej611 @cchaos
@wylieconlon @elasticmachine @mbondyra
[7.x] [Lens] Combined histogram/range aggregation for numbers (#76121) (
b71228c 
#78287)

Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
Co-authored-by: Wylie Conlon <wylieconlon@gmail.com>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Co-authored-by: Marta Bondyra <marta.bondyra@elastic.co>
Co-authored-by: cchaos <caroline.horn@elastic.co>

Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
Co-authored-by: Wylie Conlon <wylieconlon@gmail.com>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Co-authored-by: Marta Bondyra <marta.bondyra@elastic.co>
Co-authored-by: cchaos <caroline.horn@elastic.co>
@dej611 dej611 mentioned this pull request Sep 24, 2020
Merged
1 task
@dej611 dej611 mentioned this pull request Oct 22, 2020
Merged
1 task
@timroes timroes mentioned this pull request Jan 13, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@flash1293 flash1293 flash1293 left review comments

@lukeelmers lukeelmers lukeelmers left review comments

@mbondyra mbondyra mbondyra left review comments

+3 more reviewers

@gchaps gchaps gchaps left review comments

@cchaos cchaos cchaos approved these changes

@wylieconlon wylieconlon wylieconlon approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@dej611 dej611

Labels

Feature:Lens release_note:enhancement Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// v7.10.0 v8.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Lens] Create combined histogram/range aggregation for numbers

10 participants

@dej611 @flash1293 @AlonaNadler @wylieconlon @mbondyra @cchaos @elasticmachine @kibanamachine @lukeelmers @gchaps