[AO] Update the design of Threshold rule creation form #163313

fkanout · 2023-08-07T15:05:02Z

Summary

Fixes #162768
Fixes #162544

After update

apmmachine · 2023-08-07T15:05:22Z

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

/oblt-deploy : Deploy a Kibana instance using the Observability test environments.
/oblt-deploy-serverless : Deploy a serverless Kibana instance using the Observability test environments.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

…shold-rule

fkanout · 2023-08-10T13:01:11Z

...servability/public/components/threshold/components/custom_equation/metric_row_with_count.tsx

The metrics_row_with_agg file handles what this one was doing.

fkanout · 2023-08-10T13:02:07Z

x-pack/plugins/observability/public/components/threshold/components/expression_row.test.tsx

    expect(valueMatch).toBeTruthy();
  });
-
-  it('should render a helpText for the of expression', async () => {


This test is no longer relevant as we removed its related code, the OF expression

elasticmachine · 2023-08-10T13:04:16Z

Pinging @elastic/actionable-observability (Team: Actionable Observability)

maryam-saeidi · 2023-08-10T13:33:47Z

x-pack/plugins/observability/public/components/threshold/i18n_strings.ts

@@ -20,7 +20,7 @@ export const LABEL_LABEL = i18n.translate(
 export const LABEL_HELP_MESSAGE = i18n.translate(
  'xpack.observability.threshold.rule.alertFlyout.customEquationEditor.labelHelpMessage',


~~Make sure to rename the id or remove the related translations manually, as it is not clear when changing the defaultMessage, the related translations will be updated.~~ (link)

Update: I asked for an update in the thread that I shared, and I got an answer, no need to change anything :)

…shold-rule

maryam-saeidi · 2023-08-11T13:42:34Z

I am first checking the UI locally, I will check the code later.
The initial state of the rule at the creation time has a lot of red fields that need to be selected by the user, but it is not clear why there is an issue. What we can do is to have some meaningful default values similar to what we have in elasticsearch query:

Threshold rule	Elasticsearch query rule

We can use Document count as the default aggregation and add a default number for IS ABOVE.

Also, for the Document count aggregation, I don't know how user will be able to figure out they can also add a filter:

Is it possible to make Document count in one line?

@maciejforcone, what do you think?

cc @katrin-freihofner

maryam-saeidi · 2023-08-11T13:56:20Z

When adding the second condition, the position of the delete icon is a bit off.

First condition	Second condition	Design

@maciejforcone In the design, we don't have a delete icon for the first condition, don't we want to give the user the option to delete the first condition instead of the second if they want to?

maryam-saeidi · 2023-08-11T14:32:24Z

@maciejforcone Should we show the default equation in the EQUATION row?

Update
I see we have it in design:

maryam-saeidi · 2023-08-11T15:12:04Z

@fkanout I noticed when the delete button is added, the aggregation width gets smaller, and there is a jump:

Current version

Screen.Recording.2023-08-11.at.17.06.06.mov

Design

Also, I am missing the divider between conditions according to design:

maryam-saeidi

Overall looks good, nice job! 👏🏻

Added some questions and comments :)

maryam-saeidi · 2023-08-11T15:20:03Z

...ervability/public/components/threshold/components/custom_equation/custom_equation_editor.tsx

+                <EuiSpacer size="xs" />
+                <EuiExpression
+                  data-test-subj="customEquation"
+                  description={'Equation'}


Do we need to translate this?

Added the i18n

maryam-saeidi · 2023-08-11T15:23:06Z

...ervability/public/components/threshold/components/custom_equation/custom_equation_editor.tsx

-      />
-    );
-  });
+  const metricRows = customMetrics?.map((row) => (


maryam-saeidi · 2023-08-11T15:30:23Z

x-pack/plugins/observability/public/components/threshold/components/expression_row.tsx

-            )}
-            {criticalThresholdExpression}
-          </StyledExpressionRow>
+          <StyledExpressionRow style={{ gap: aggType !== 'custom' ? 24 : 12 }} />

          {aggType === Aggregators.CUSTOM && (


Is this condition still needed, or can we get rid f it?

fkanout · 2023-08-11T16:21:53Z

@maciejforcone Should we show the default equation in the EQUATION row?
**Update** I see we have it in design:

@maryam-saeidi, It's already there, but maybe you don't have the last version of the PR branch.

simianhacker

LGTM

Addressed your code related comments. UI/UX questions and comments (tweaks) could be done in a follow up PRs.

kibana-ci · 2023-08-11T17:28:05Z

💚 Build Succeeded

Buildkite Build
Commit: 2ab28f0

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`observability`	420	419	-1

Async chunks

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

id	before	after	diff
`observability`	1.0MB	1.0MB	-628.0B

History

💚 Build #149603 succeeded b11fea8
💚 Build #149574 succeeded 5e2187d
💚 Build #149517 succeeded 60cbb9f
💚 Build #149185 succeeded d128231
💔 Build #149168 failed 386bbfc
💔 Build #149129 failed 6361b3e

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

cc @fkanout

Update the design with mainly components

3a2cd32

fkanout and others added 12 commits August 7, 2023 17:06

Merge branch 'main' into 162768-update-rule-creation-form-design-thre…

13b7d44

…shold-rule

Fixes after merging with main

c94c40d

style the ForLastExpression

93cd15a

Add condition title

67df751

update label

a827fbc

add count to the row with agg

26ef61d

manage feedback

1208259

Update tests

31b0169

fix i18n

c8b494c

Merge branch 'main' into 162768-update-rule-creation-form-design-thre…

6361b3e

…shold-rule

fix test

0e9f8fe

Merge branch 'main' into 162768-update-rule-creation-form-design-thre…

386bbfc

…shold-rule

fkanout self-assigned this Aug 10, 2023

fkanout added release_note:skip Skip the PR/issue when compiling release notes Team: Actionable Observability - DEPRECATED For Observability Alerting and SLOs use "Team:obs-ux-management", for AIops "Team:obs-knowledge" v8.10.0 labels Aug 10, 2023

fkanout and others added 2 commits August 10, 2023 14:58

update snapshot

446f35d

Merge branch 'main' into 162768-update-rule-creation-form-design-thre…

d128231

…shold-rule

fkanout commented Aug 10, 2023

View reviewed changes

fkanout marked this pull request as ready for review August 10, 2023 13:04

fkanout requested a review from a team as a code owner August 10, 2023 13:04

maryam-saeidi reviewed Aug 10, 2023

View reviewed changes

fkanout requested a review from maryam-saeidi August 10, 2023 14:19

TEMP: turn on FF for testing

b33bde1

fkanout requested review from a team as code owners August 11, 2023 09:30

Merge branch 'main' into 162768-update-rule-creation-form-design-thre…

60cbb9f

…shold-rule

fkanout added the ci:build-serverless-image label Aug 11, 2023

maryam-saeidi mentioned this pull request Aug 11, 2023

[AO] remove expand/collapse icon button from Threshold rule form #162544

Closed

Merge branch 'main' into 162768-update-rule-creation-form-design-thre…

5e2187d

…shold-rule

fkanout removed the ci:build-serverless-image label Aug 11, 2023

revert: set FF off back again

b11fea8

maryam-saeidi previously requested changes Aug 11, 2023

View reviewed changes

fkanout removed request for a team August 11, 2023 15:58

codereview

2ab28f0

fkanout enabled auto-merge (squash) August 11, 2023 16:33

simianhacker approved these changes Aug 11, 2023

View reviewed changes

fkanout merged commit 2093a1f into elastic:main Aug 11, 2023

kibanamachine added the backport:skip This commit does not require backporting label Aug 11, 2023

elasticmachine unassigned fkanout Oct 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AO] Update the design of Threshold rule creation form #163313

[AO] Update the design of Threshold rule creation form #163313

fkanout commented Aug 7, 2023 •

edited

Loading

apmmachine commented Aug 7, 2023

fkanout Aug 10, 2023

fkanout Aug 10, 2023

elasticmachine commented Aug 10, 2023

maryam-saeidi Aug 10, 2023 •

edited

Loading

maryam-saeidi commented Aug 11, 2023 •

edited

Loading

maryam-saeidi commented Aug 11, 2023

maryam-saeidi commented Aug 11, 2023 •

edited

Loading

maryam-saeidi commented Aug 11, 2023 •

edited

Loading

maryam-saeidi left a comment

maryam-saeidi Aug 11, 2023

fkanout Aug 11, 2023

maryam-saeidi Aug 11, 2023

maryam-saeidi Aug 11, 2023

fkanout Aug 11, 2023

fkanout commented Aug 11, 2023

simianhacker left a comment

kibana-ci commented Aug 11, 2023

		@@ -20,7 +20,7 @@ export const LABEL_LABEL = i18n.translate(
		export const LABEL_HELP_MESSAGE = i18n.translate(
		'xpack.observability.threshold.rule.alertFlyout.customEquationEditor.labelHelpMessage',

[AO] Update the design of Threshold rule creation form #163313

[AO] Update the design of Threshold rule creation form #163313

Conversation

fkanout commented Aug 7, 2023 • edited Loading

Summary

After update

apmmachine commented Aug 7, 2023

🤖 GitHub comments

fkanout Aug 10, 2023

Choose a reason for hiding this comment

fkanout Aug 10, 2023

Choose a reason for hiding this comment

elasticmachine commented Aug 10, 2023

maryam-saeidi Aug 10, 2023 • edited Loading

Choose a reason for hiding this comment

maryam-saeidi commented Aug 11, 2023 • edited Loading

maryam-saeidi commented Aug 11, 2023

maryam-saeidi commented Aug 11, 2023 • edited Loading

maryam-saeidi commented Aug 11, 2023 • edited Loading

maryam-saeidi left a comment

Choose a reason for hiding this comment

maryam-saeidi Aug 11, 2023

Choose a reason for hiding this comment

fkanout Aug 11, 2023

Choose a reason for hiding this comment

maryam-saeidi Aug 11, 2023

Choose a reason for hiding this comment

maryam-saeidi Aug 11, 2023

Choose a reason for hiding this comment

fkanout Aug 11, 2023

Choose a reason for hiding this comment

fkanout commented Aug 11, 2023

@maryam-saeidi, It's already there, but maybe you don't have the last version of the PR branch.

simianhacker left a comment

Choose a reason for hiding this comment

kibana-ci commented Aug 11, 2023

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

History

fkanout commented Aug 7, 2023 •

edited

Loading

maryam-saeidi Aug 10, 2023 •

edited

Loading

maryam-saeidi commented Aug 11, 2023 •

edited

Loading

maryam-saeidi commented Aug 11, 2023 •

edited

Loading

maryam-saeidi commented Aug 11, 2023 •

edited

Loading