Revisit custom threshold types in public folder #170306

maryam-saeidi · 2023-11-01T11:20:12Z

Summary

This PR:

Removes preFill logic
In this PR, I removed the logic about prefilling custom threshold rule params as it was originally for other rule types (not custom equation) and to be used in the Metric threshold rule and the code related to this logic was super confusing, and I wasn't even sure if it works as expected since we haven't used this logic anywhere. I created a ticket to bring back this feature properly later, specifically for the custom equation, and integrate it in one of the apps, such as Infra. We also need to be able to preFill data view information (both adHoc and persisted data view)
Renames types and file names
- From metricThreshold to customThreshold
- From metricExplorer to expression
Removes unused types
Remove logic related to aggregations other than the custom equation at the top level

Also, the fields that end with pct now have the % after the related value: (The reason message was fixed in another PR)

🧪 How to test

Nothing has changed related to functionality, so please make sure the custom threshold rule is working as before for
- Creating a new rule with multiple conditions
- Adding groups
- Editing a rule and checking the charts are shown as before
- Test both adHoc and persisted data view

…ult level.

apmmachine · 2023-11-01T11:20:28Z

🤖 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!)

…fix'

x-pack/plugins/observability/server/lib/rules/custom_threshold/custom_threshold_executor.ts

fkanout

@maryam-saeidi, when visiting the rule details it shows an error about not enabling to get the execution history.

fkanout

On serverless, it fires alerts without the error of execution history we saw in stateful

fkanout · 2023-11-09T15:47:37Z

x-pack/plugins/observability/common/custom_threshold_rule/constants.ts

@@ -5,16 +5,13 @@
 * 2.0.
 */

-export const SNAPSHOT_CUSTOM_AGGREGATIONS = ['avg', 'max', 'min', 'rate'] as const;
 export const METRIC_EXPLORER_AGGREGATIONS = [


To be deleted it's not used anymore.

Actually, this is still used in x-pack/plugins/observability/server/lib/rules/custom_threshold/register_custom_threshold_rule_type.ts

fkanout · 2023-11-09T15:59:56Z

...ackages/observability/alerting_test_data/src/scenarios/custom_threshold_log_count_groupby.ts

@@ -22,7 +23,7 @@ export const scenario2 = {
    params: {
      criteria: [
        {
-          aggType: Aggregators.CUSTOM,
+          aggType: CUSTOM_AGGREGATOR,


Same: I think we should keep referring to the custom from Aggregators to keep everything on track when an updates occurs. Can you explain the benefits of not using the value fromAggregators?
I will not repeate the same comment for the other files :D

I changed it to CUSTOM_AGGREGATOR because it is the only acceptable value at this level and does not belong to Aggregators anymore. There were a lot of type duplications to handle this scenario, and by splitting them, we don't need to exclude custom from Aggregators to be used in the rule.

I am considering creating a ticket to remove CUSTOM_AGGREGATOR everywhere if it does not have a backward compatibility issue.

fkanout · 2023-11-09T16:00:29Z

x-pack/plugins/observability/common/custom_threshold_rule/formatters/index.ts

@@ -14,7 +14,7 @@ import { InfraWaffleMapDataFormat } from './types';

 export const FORMATTERS = {
  number: formatNumber,
-  // Because the implimentation for formatting large numbers is the same as formatting
+  // Because the implementation for formatting large numbers is the same as formatting


fkanout · 2023-11-09T16:01:25Z

x-pack/plugins/observability/common/custom_threshold_rule/metrics_explorer.ts

- * 2.0.
- */
-
-import * as rt from 'io-ts';


👏🏻 DELETEING

fkanout · 2023-11-09T16:27:32Z

x-pack/plugins/observability/common/custom_threshold_rule/types.ts

+ *
+ * This utility function can be used to turn a TypeScript enum into a io-ts codec.
+ */
+export function fromEnum<EnumType extends string>(


What does this function do?

This is for turning a Typescript enum into an io-ts validation. I used it for the Aggregators enum.

there is already some nice utils exposed via perhaps worth reusing that or move that into packages/kbn-io-ts-utils
import { enumeration } from '@kbn/securitysolution-io-ts-types';

fkanout · 2023-11-09T16:38:18Z

x-pack/plugins/observability/public/components/custom_threshold/components/alert_flyout.tsx

-  const { groupBy, filterQuery, metrics } = metricThresholdPrefill;
-
-  return <AlertFlyout options={{ groupBy, filterQuery, metrics }} visible setVisible={onClose} />;
+  return <AlertFlyout visible setVisible={onClose} />;


We don't need to send options={{ groupBy, filterQuery, metrics }} ?

This is related to preFill logic; I removed it because it did not match the new custom threshold structure and created a ticket to add this logic correctly (#170295)

maryam-saeidi · 2023-11-10T07:41:53Z

@fkanout I tried the Stateful rule details page and didn't face the issue you shared. Can you please check which API fails and if there are any error logs in your server? Is it related to changes in this PR?

shahzad31

LGTM !!

Tested creating threshold rule, editing, charts, group by and rule details page. !!

benakansara · 2023-11-13T17:26:28Z

@maryam-saeidi I think using a different unit for percentage metrics is confusing for user and we need to handle different scenarios when switching between percentage and non-percentage metrics. Also, how do we handle a scenario when all metrics in a condition are percentage.

Switching from `average` to `count`

Screen.Recording.2023-11-13.at.22.44.22.mov

Using both percentage metrics in a condition

Screen.Recording.2023-11-13.at.22.50.33.mov

maryam-saeidi · 2023-11-13T17:42:35Z

@maryam-saeidi I think using a different unit for percentage metrics is confusing for user and we need to handle different scenarios when switching between percentage and non-percentage metrics. Also, how do we handle a scenario when all metrics in a condition are percentage.

Switching from average to count

Screen.Recording.2023-11-13.at.22.44.22.mov

Using both percentage metrics in a condition

Screen.Recording.2023-11-13.at.22.50.33.mov

@benakansara For now, I only handled the scenario that there is only one percentage metric, which is similar to what we had in the Metric threshold. It should be easy to do the same when all the metrics are percentages if we decide to do so. I'll create an issue to get @maciejforcone 's opinion on this case as well. This improvement helps to see the value of percentage fields accordingly instead of setting 0.1 as the threshold for percentage fields, but I agree that the current solution is not perfect either.

Regarding the first issue, I didn't find an ideal solution; one possible option is not to have a default value for the threshold similar to the Metric threshold. Let's discuss this with @maciejforcone as well.

Issue: #171121

kibana-ci · 2023-11-14T08:38:39Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`observability`	491	480	-11

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.1MB	1.1MB	-6.9KB
`triggersActionsUi`	1.4MB	1.4MB	+12.0B
total			-6.9KB

Page load bundle

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

id	before	after	diff
`observability`	104.1KB	100.4KB	-3.7KB

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`observability`	37	38	+1

Total ESLint disabled count

id	before	after	diff
`observability`	42	43	+1

History

💔 Build #175428 failed e02debb
💚 Build #175308 succeeded a24ebd6
💛 Build #175159 was flaky 7bbca06
💛 Build #174640 was flaky 1e7ea5d
💛 Build #174163 was flaky d630f20

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

cc @maryam-saeidi

maryam-saeidi added 8 commits October 26, 2023 13:27

Rename metricThreshold to customThreshold in public

f0b93a7

Merge branch 'main' into 159340-revisit-custom-threshold-public-types

000696c

Rename metricThreshold to customThreshold in public folder

babe850

Rename metricThreshold and remove unused types

7385077

Remove extra logic related to aggregations other than custom for defa…

b23afdf

…ult level.

Remove metric_expression_params functions

4b6b1fa

Remove extra validation for aggType custom

cf8e0f6

Remove preFill logic and unnecessary types

84ded2f

maryam-saeidi added the release_note:skip Skip the PR/issue when compiling release notes label Nov 1, 2023

maryam-saeidi self-assigned this Nov 1, 2023

Merge branch 'main' into 159340-revisit-custom-threshold-public-types

f79a012

kibanamachine and others added 12 commits November 1, 2023 11:26

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

881a6c1

…fix'

Fix pct field format and rearrange types

886a4ed

Merge branch 'main' into 159340-revisit-custom-threshold-public-types

9b3f22e

Fix no data reason and refactor threshold component

c69f7b0

Merge branch 'main' into 159340-revisit-custom-threshold-public-types

4d6e9f0

Fix unit props

c4e25e8

Merge branch 'main' into 159340-revisit-custom-threshold-public-types

cd73bae

Remove aggregators that are not supported in the custom threshold

d2417fc

Merge branch 'main' into 159340-revisit-custom-threshold-public-types

922c069

Remove more types

a298924

Remove usage of Aggregators.CUSTOM

5ef6a4c

Merge branch 'main' into 159340-revisit-custom-threshold-public-types

de870f6

maryam-saeidi marked this pull request as ready for review November 3, 2023 09:02

maryam-saeidi requested review from a team as code owners November 3, 2023 09:02

cnasikas approved these changes Nov 6, 2023

View reviewed changes

fkanout self-requested a review November 8, 2023 07:37

Merge branch 'main' into 159340-revisit-custom-threshold-public-types

2bcb226

maryam-saeidi mentioned this pull request Nov 8, 2023

[AO] Revisit the Threshold rule's files organization, used types, and variable names #159340

Closed

6 tasks

Remove comment

6debb6f

maryam-saeidi commented Nov 8, 2023

View reviewed changes

x-pack/plugins/observability/server/lib/rules/custom_threshold/custom_threshold_executor.ts Show resolved Hide resolved

maryam-saeidi and others added 2 commits November 8, 2023 12:41

Fix translation

d630f20

Merge branch 'main' into 159340-revisit-custom-threshold-public-types

1e7ea5d

fkanout requested a review from a team as a code owner November 9, 2023 14:21

fkanout reviewed Nov 9, 2023

View reviewed changes

maryam-saeidi added 3 commits November 11, 2023 01:19

Merge branch 'main' into 159340-revisit-custom-threshold-public-types

7bbca06

Fix bug in changing aggs

261685c

Merge branch 'main' into 159340-revisit-custom-threshold-public-types

a24ebd6

shahzad31 approved these changes Nov 13, 2023

View reviewed changes

maryam-saeidi mentioned this pull request Nov 13, 2023

[Custom threshold] How to handle percentage field for multiple aggregations #171121

Closed

Merge branch 'main' into 159340-revisit-custom-threshold-public-types

e02debb

maryam-saeidi enabled auto-merge (squash) November 13, 2023 17:56

Merge branch 'main' into 159340-revisit-custom-threshold-public-types

a91dc5e

benakansara approved these changes Nov 14, 2023

View reviewed changes

maryam-saeidi merged commit 6f2ad26 into elastic:main Nov 14, 2023

kibanamachine added v8.12.0 backport:skip This commit does not require backporting labels Nov 14, 2023

maryam-saeidi deleted the 159340-revisit-custom-threshold-public-types branch November 14, 2023 09:04

logeekal mentioned this pull request Nov 16, 2023

DO NOT MERGE - Timeline New UI + Modal + Unified components #171385

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revisit custom threshold types in public folder #170306

Revisit custom threshold types in public folder #170306

maryam-saeidi commented Nov 1, 2023 •

edited

Loading

apmmachine commented Nov 1, 2023

fkanout left a comment •

edited

Loading

fkanout left a comment

fkanout Nov 9, 2023

maryam-saeidi Nov 10, 2023

fkanout Nov 9, 2023

maryam-saeidi Nov 10, 2023

fkanout Nov 9, 2023

fkanout Nov 9, 2023

fkanout Nov 9, 2023

maryam-saeidi Nov 10, 2023

shahzad31 Nov 13, 2023

fkanout Nov 9, 2023

maryam-saeidi Nov 10, 2023

maryam-saeidi commented Nov 10, 2023

shahzad31 left a comment

benakansara commented Nov 13, 2023

maryam-saeidi commented Nov 13, 2023 •

edited

Loading

Switching from `average` to `count`

Using both percentage metrics in a condition

kibana-ci commented Nov 14, 2023

ESLint disabled line counts

Total ESLint disabled count

Revisit custom threshold types in public folder #170306

Revisit custom threshold types in public folder #170306

Conversation

maryam-saeidi commented Nov 1, 2023 • edited Loading

Summary

🧪 How to test

apmmachine commented Nov 1, 2023

🤖 GitHub comments

fkanout left a comment • edited Loading

Choose a reason for hiding this comment

fkanout left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maryam-saeidi commented Nov 10, 2023

shahzad31 left a comment

Choose a reason for hiding this comment

benakansara commented Nov 13, 2023

Switching from average to count

Using both percentage metrics in a condition

maryam-saeidi commented Nov 13, 2023 • edited Loading

Switching from average to count

Using both percentage metrics in a condition

kibana-ci commented Nov 14, 2023

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Page load bundle

ESLint disabled line counts

Total ESLint disabled count

History

maryam-saeidi commented Nov 1, 2023 •

edited

Loading

fkanout left a comment •

edited

Loading

Switching from `average` to `count`

maryam-saeidi commented Nov 13, 2023 •

edited

Loading

Switching from `average` to `count`