[Lens] Thresholds added by mbondyra · Pull Request #108342 · elastic/kibana

mbondyra · 2021-08-12T10:12:48Z

Summary

Built from @dej611 PR

Fixes #87551

Threshold as new layer type

Thresholds can be line or area

Tasks:

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.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Unit or functional tests were updated or added to match the most common scenarios

…uge files

Threshold tests

dej611 · 2021-09-02T12:25:08Z

@elasticmachine merge upstream

mbondyra · 2021-09-06T08:09:46Z

@elasticmachine merge upstream

dej611 · 2021-09-16T12:31:12Z

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.

MichaelMarcialis

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

dej611 · 2021-09-20T08:27:44Z

@elasticmachine merge upstream

kibanamachine · 2021-09-20T08:27:45Z

merge conflict between base and head

dej611 · 2021-09-20T12:50:11Z

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

dej611 · 2021-09-20T12:51:16Z

jenkins, test this

flash1293 · 2021-09-21T07:59:22Z

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?

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.

flash1293 · 2021-09-21T08:00:52Z

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 · 2021-09-21T12:00:44Z

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 · 2021-09-21T14:22:01Z

💚 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

💛 Build #154230 was flaky 81fa3bd
💔 Build #154196 failed 81fa3bd
💔 Build #154191 failed ec4dfef
💛 Build #153798 was flaky a430f71
💔 Build #153593 failed 55a2555

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

flash1293

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

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 · 2021-09-22T09:17:51Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

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 · 2021-09-23T05:43:22Z

@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

dej611 · 2021-09-23T05:47:40Z

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

monfera

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 force-pushed the lens/thresholds branch 3 times, most recently from f39d3a8 to fa576d4 Compare August 13, 2021 08:23

dej611 and others added 9 commits August 23, 2021 08:33

✨ Add threshold code

f63b5d8

fix dropping the field

300ab45

fix adding bottom axis

c22b14d

fix do not show horizontal axis for terms and filters

5c85dcf

extract StaticHeader component

bc1cf5d

convert section to header

32c1a57

initialize percentage with percentage value

2423cdc

fix debouncing for threshold value and thickness line range, divide h…

e3a469c

…uge files

fix two layer threshold to pie (tbc)

5c5ecb5

mbondyra force-pushed the lens/thresholds branch from 7aabc47 to 5c5ecb5 Compare August 23, 2021 07:26

dej611 added 5 commits August 25, 2021 14:51

✅ Add first batch of tests

f919159

✅ Added basic tests for layer header

2d0f356

✅ Add editor tabs tests

a930b43

✅ Add more tests

f272020

✅ Add indexpattern tests

d95d441

dej611 mentioned this pull request Aug 25, 2021

Threshold tests mbondyra/kibana#5

Merged

17 tasks

dej611 and others added 6 commits August 26, 2021 18:08

✅ Add operation tests

c3d00a0

✅ Add xy visualization test for thresholds

53d85f2

✅ Add functional tests for thresholds

ae4201d

✅ Add one more tests for terms op

ead4e50

Merge pull request #5 from dej611/lens/thresholds-tests

40069fe

Threshold tests

Merge branch 'master' into lens/thresholds

e49a928

kibanamachine and others added 3 commits September 2, 2021 08:25

Merge branch 'master' into lens/thresholds

d3e0c0f

🐛 Fix broken test

1c95f6c

✨ Pickup static value when transitioning to formula

869889d

MichaelMarcialis approved these changes Sep 16, 2021

View reviewed changes

dej611 added 2 commits September 20, 2021 11:32

Merge remote-tracking branch 'upstream/master' into lens/thresholds

ec4dfef

🚨 fix linting issue

81fa3bd

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

dej611 requested a review from stratoula September 20, 2021 12:55

🐛 Fix debouncing sync issue in panel

69768de

flash1293 approved these changes Sep 22, 2021

View reviewed changes

dej611 merged commit 0cbdf3f into elastic:master Sep 22, 2021

kibanamachine mentioned this pull request Sep 22, 2021

[7.x] [Lens] Thresholds added (#108342) #112789

Merged

This was referenced Sep 22, 2021

[Lens] Thresholds: Change default color palette for Thresholds #112796

Closed

[Lens] Thresholds: consider to change default operation to 95% Percentile rather than Median on field drop #112801

Closed

This was referenced Sep 23, 2021

[Lens] Support multiple thresholds on bar/line/area charts #55265

Closed

[Lens] Support dynamic thresholds based on data from the chart #87548

Closed

monfera reviewed Sep 24, 2021

View reviewed changes

mbondyra deleted the lens/thresholds branch September 27, 2021 09:34

Conversation

mbondyra commented Aug 12, 2021 • edited by dej611 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Notes

Checklist

Uh oh!

dej611 commented Sep 2, 2021

Uh oh!

mbondyra commented Sep 6, 2021

Uh oh!

dej611 commented Sep 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MichaelMarcialis left a comment

Choose a reason for hiding this comment

Uh oh!

dej611 commented Sep 20, 2021

Uh oh!

kibanamachine commented Sep 20, 2021

Uh oh!

dej611 commented Sep 20, 2021

Uh oh!

dej611 commented Sep 20, 2021

Uh oh!

flash1293 commented Sep 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flash1293 commented Sep 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dej611 commented Sep 21, 2021

Uh oh!

kibanamachine commented Sep 21, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Public APIs missing exports

Page load bundle

API count

History

Uh oh!

flash1293 left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Sep 22, 2021

💚 Backport successful

Uh oh!

shahzad31 commented Sep 23, 2021

Uh oh!

dej611 commented Sep 23, 2021

Uh oh!

monfera left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

mbondyra commented Aug 12, 2021 •

edited by dej611

Loading

dej611 commented Sep 16, 2021 •

edited

Loading

flash1293 commented Sep 21, 2021 •

edited

Loading

flash1293 commented Sep 21, 2021 •

edited

Loading