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] Thresholds added #108342

Merged
merged 63 commits into from
Sep 22, 2021
Merged

[Lens] Thresholds added #108342

merged 63 commits into from
Sep 22, 2021

Conversation

mbondyra
Copy link
Contributor

@mbondyra mbondyra commented Aug 12, 2021

Summary

Built from @dej611 PR

Fixes #87551

Screenshot 2021-08-12 at 12 09 36

Threshold as new layer type
Screenshot 2021-08-12 at 12 11 50

Thresholds can be line or area

Screenshot 2021-08-12 at 12 12 25 Screenshot 2021-08-04 at 16 43 42

Tasks:

  • New layer type: Threshold
    • Enable only for numeric values
      • check tooltip content for calculation operations
    • Icon for threshold layer
    • Handle the logic for Delete/Reset layer based only on Data Layers
    • Add layer button refactoring
      • Keep current logic if there's only 1 layer type
      • Show type popup if more layers are available
        • Add validation logic to the popup (date histogram, available data dimensions, ...)
    • Show only available axis as dimensions (left, right, horizontal)
      • Show/hide dimensions based on data layers
      • On creation pick the first available dimension and create a static dimension there (75% of max data)
      • When removing data dimensions keep the threshold dimension marking it as invalid
        • Keep it also if it's the only available dimension.
      • If the user removes all possible threshold axis, delete the threshold layer
    • Drag and drop field should default to "Median of X"
    • Dragging a text field to a vis with a threshold sometimes adds the field as "break down by" to the threshold panel instead of the chart panel
    • Duplicate dimension on threshold layer does not work properly
    • Swap threshold dimensions does not work properly
  • Static value operation
  • Filter threshold layer on suggestions
    • Restore threshold layer on XY suggestions
    • Make sure the threshold layer is wiped when switching to a chart that does not support thresholds
  • Threshold dimension editor
    • Add static value tab
      • Only available on threshold layers
    • Validate empty threshold value
    • => Formula transition
      • Carry the static value in Formula
      • When going back to "Static value" show a callout about losing Formula's state.
        • The Threshold value should contain the Formula's output as static value while showing the callout. Editing the input should wipe the Formula's state.
    • => Quick functions
      • Do not go back to static value when selecting a function
      • When going back to "Static value" restore the computed static value rather than the fallback value
    • Color default should be assigned
    • Support threshold area
      • Make threshold area stop after next threshold with the same direction. Different directions overlap is ok.
      • Area fill should have opacity < 1. Area border should have 100% opacity.
    • bugs:
    • switching to pie when threshold layer is present doesn't work correctly
    • horizontal axis for categorical data shouldn't appear
    • when dropping field to workspace, it should not be dropped into threshold
    • settings should be debounced
    • xy chart with threshold defined -> convert to datatable -> convert back and add threshold -> threshold column is lost
      https://user-images.githubusercontent.com/4283304/132191929-8bca0ba2-5b19-4642-8d73-be209c8b685c.mp4
    • Drop an existing threshold dimension into an empty dimension field creates a new threshold on top of the group list (it should be bottom)

Notes

  • On tab switching from Formula to Static value a temporary state is now created. Whne editing the static value the temporary state is discarded and a new static value column is created as final one.
    • In order to implement this I had to add a hack to force a rerender (forceRender) on top of another hack we use to delay the rendering in some circumstances. I tried to investigated various routes, but they all leads to some UI flickering due to the updateDatasourceAsync function: the new flag forces a sync update for the redux store rather than async.

Checklist

Delete any items that are not applicable to this PR.

@mbondyra mbondyra force-pushed the lens/thresholds branch 3 times, most recently from f39d3a8 to fa576d4 Compare August 13, 2021 08:23
@dej611 dej611 mentioned this pull request Aug 25, 2021
17 tasks
@dej611
Copy link
Contributor

dej611 commented Sep 2, 2021

@elasticmachine merge upstream

@mbondyra
Copy link
Contributor Author

mbondyra commented Sep 6, 2021

@elasticmachine merge upstream

@dej611
Copy link
Contributor

dej611 commented Sep 16, 2021

I'll summary all the pending issues in follow ups in this list:

  • The suggestion system should provide the same results, for the same data configuration, with or without a threshold layer.
  • Tweak suggestions threshold styling for thicker lines
  • When transitioning from/to a Percentage chart the threshold static value can be recomputed accordingly?
  • Collision issue between the threshold icons and axis tick value labels (cc datavis team)
  • Consider to change default operation for dropped field to 95th Percentile from Median?
  • Invalid Threshold styling issue
    • underline color does not match text color
    • Remove threshold color for invalid thresholds?
  • Dynamically change the visualization bounds based on the threshold value
  • Change default color palette for Thresholds
  • Compute the default static value from all the dimensions for the first available axis rather than just one dimension
  • Static value transition enhancements: transitioning from Formula to Static value can be smarter and avoid temporary state with callout by just converting the formula to static value
  • When moving a threshold to the other axis, the configuration gets lost - on duplicating a threshold line the same thing happens, but I think it would be OK in this case.

Copy link
Contributor

@MichaelMarcialis MichaelMarcialis left a comment

Choose a reason for hiding this comment

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

Thanks for making those last changes. Along with the list of follow-up issues, this looks good to me.

@dej611
Copy link
Contributor

dej611 commented Sep 20, 2021

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

merge conflict between base and head

@dej611 dej611 added the auto-backport Deprecated - use backport:version if exact versions are needed label Sep 20, 2021
@dej611
Copy link
Contributor

dej611 commented Sep 20, 2021

Failed jest tests look not related to this PR, nor Lens. Will try to rerun the CI.

@dej611
Copy link
Contributor

dej611 commented Sep 20, 2021

jenkins, test this

@flash1293
Copy link
Contributor

flash1293 commented Sep 21, 2021

Playing around with the PR I noticed two UI issues:

It seems like the debouncing can lead to lost updates - see how the "above" settings flickers before getting reset. Do we need to use an updater function somewhere instead of spreading the old state?
lost_update

When moving a threshold to the other axis, the configuration gets lost - on duplicating a threshold line the same thing happens, but I think it would be OK in this case.
moving_lost_settings

@flash1293
Copy link
Contributor

flash1293 commented Sep 21, 2021

I don't think we have to address these on the PR, but we should address them before the release. So if we want to merge without a fix for them, we should create follow-up bug issues to fix in separate PRs.

Otherwise this looks great to me - extremely useful feature!

@dej611
Copy link
Contributor

dej611 commented Sep 21, 2021

It seems like the debouncing can lead to lost updates - see how the "above" settings flickers before getting reset. Do we need to use an updater function somewhere instead of spreading the old state?

Fixed this in the PR as it was very localized and only few lines changed.
The latter would require some more work, I'd prefer to have it separate.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
lens 821 831 +10

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 229 233 +4

Async chunks

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

id before after diff
lens 1021.0KB 1.0MB +20.8KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
lens 23 24 +1

Page load bundle

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

id before after diff
lens 38.3KB 38.6KB +310.0B
Unknown metric groups

API count

id before after diff
lens 247 251 +4

History

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

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.

The fix works fine for me, approving as long as we create follow-up issues for the stuff we know we want to adjust.

@dej611 dej611 merged commit 0cbdf3f into elastic:master Sep 22, 2021
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Sep 22, 2021
Co-authored-by: Marta Bondyra <marta.bondyra@gmail.com>
Co-authored-by: dej611 <dej611@gmail.com>
Co-authored-by: Marco Liberati <dej611@users.noreply.github.com>
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@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 Sep 22, 2021
Co-authored-by: Marta Bondyra <marta.bondyra@gmail.com>
Co-authored-by: dej611 <dej611@gmail.com>
Co-authored-by: Marco Liberati <dej611@users.noreply.github.com>
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Marta Bondyra <marta.bondyra@gmail.com>
Co-authored-by: dej611 <dej611@gmail.com>
Co-authored-by: Marco Liberati <dej611@users.noreply.github.com>
@shahzad31
Copy link
Contributor

@mbondyra this looks and works great, do we have any plans to allow additing text annotation to the thresholds lines?

Specifically i am looking to recreate something like this in lens

image

@dej611
Copy link
Contributor

dej611 commented Sep 23, 2021

@shahzad31 while not originally planned, it makes sense to consider it. Will open a new enhancement issue for that.

Copy link
Contributor

@monfera monfera left a comment

Choose a reason for hiding this comment

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

Please, in a follow-up PR, consider replacing "Threshold" on the UI with a truthful word such as "Reference value" or "Reference line" or [subject to discussion], we shouldn't be so inaccurate with our terms on the UI as it leads to poor user experience, see #87548 (comment) cc @dej611 @flash1293 (The code internals are not my concern, though it's good practice to reflect the proper words of the domain, not random words)

@mbondyra mbondyra deleted the lens/thresholds branch September 27, 2021 09:34
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:feature Makes this part of the condensed release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.16.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Lens] Support dynamic thresholds based on a query
10 participants