Adding feature direction and moving suppression rules to each feature #960

amitgalitz · 2024-12-19T19:49:21Z

Description

Added feature direction capability to each feature so users can select if anomalies should only be considered if they actual values goes either above or below the expected value.
Moved suppressionRules into the feature creation and redesigned it,
Fixed a bug when we had multiple rules with negative values where the toast message was blank.
Additional slight wording changes on different form fields

Check List

New functionality includes testing.
- All tests pass
[] New functionality has been documented.
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Amit Galitzky <[email protected]>

kaituo · 2024-12-20T06:30:40Z

I manually tested your code and found two issues.

First, I tested bwc by creating a detector using cli. By default, every feature has two rules like

"rules": [
            {
                "action": "IGNORE_ANOMALY",
                "conditions": [
                    {
                        "feature_name": "deny max",
                        "threshold_type": "ACTUAL_OVER_EXPECTED_RATIO",
                        "operator": "LTE",
                        "value": 0.2
                    },
                    {
                        "feature_name": "deny max",
                        "threshold_type": "EXPECTED_OVER_ACTUAL_RATIO",
                        "operator": "LTE",
                        "value": 0.2
                    }
                ]
            }
        ]

But when I open the detector from UI, I can see only one rule:

Second, I am expecting that I cannot define a suppression rule from the opposite direction a user has selected. For example, when I select "Rise above expected value", I should not be able to see the "below the expected value" from the rule section. But I do see it:

amitgalitz · 2024-12-20T19:04:25Z

I will address these issue, in addition, I missed on the part with the default rules, I will make sure we show them correctly in UI and also now for every detector with a feature direction we will have just that one default rule show along with the change on the UI so selection for only one shows.

Signed-off-by: Amit Galitzky <[email protected]>

kaituo · 2024-12-26T18:00:56Z

A few thoughts.

First, for every detector with a feature direction we will have just that two identical default rules show along with the change.

Second, current UX will overwrite my rules when I am playing around directions. For example, my double direction rules will not come back once I re-select "Deviation in any direction". Before cx click save, you have the original rules in record. You can restore them back, right?

Third, is it a good experience to change customers' rules when I change direction? For example, original rules are "above".

When I change to "below", both rules changed to "below":

Should we just hide the "above" rules?

Signed-off-by: Amit Galitzky <[email protected]>

kaituo · 2025-01-24T01:31:28Z

public/pages/ConfigureModel/components/SuppressionRules/SuppressionRules.tsx

+  feature: any;
+  featureIndex: any;


Can you add types instead of any?

added types

kaituo · 2025-01-24T02:15:45Z

public/pages/ConfigureModel/components/SuppressionRules/SuppressionRules.tsx

+                                          const cleanedSuppressionRules =
+                                            form.values.suppressionRules.filter(
+                                              (_, i) => i === props.featureIndex
+                                            );
+                                          form.setFieldValue(
+                                            `suppressionRules.`,
+                                            cleanedSuppressionRules
+                                          );


Why do we need these lines? Is arrayHelpers.remove enough? Also, should we use the following instead since you want to filter out any rule with props.featureIndex?

const cleanedSuppressionRules = form.values.suppressionRules.filter( (_, i) => i !== props.featureIndex ); form.setFieldValue('suppressionRules', cleanedSuppressionRules);

I previously had this to fix a bug where if there was only 1 suppression rule it wouldn't let me delete the last rule and fully clean up the array. I can remove this now though

public/pages/ConfigureModel/components/SuppressionRules/SuppressionRules.tsx

kaituo · 2025-01-24T03:24:16Z

public/pages/ConfigureModel/components/SuppressionRules/SuppressionRules.tsx

@@ -101,7 +83,7 @@ export function SuppressionRules(props: SuppressionRulesProps) {
                        ]}
                        hintLink={`${BASE_DOCS_LINK}/ad`}
                        isInvalid={isInvalid(field.name, form)}
-                        error={extractArrayError(field.name, form)}
+                        //error={extractArrayError(field.name, form)}


do you really want to hide error here?

I was going to remove this completely as now I have errors seen before each field individually

ok, can you add comment about this?

added comment about new extractError

kaituo · 2025-01-24T16:05:35Z

public/pages/ConfigureModel/components/SuppressionRules/SuppressionRules.tsx

+                                        isInvalid={
+                                          !!extractError(
+                                            `suppressionRules.${
+                                              props.featureIndex
+                                            }[${index}].${
+                                              isPercentage === false
+                                                ? 'absoluteThreshold'
+                                                : 'relativeThreshold'
+                                            }`,
+                                            form
+                                          )
+                                        }
+                                        error={extractError(
+                                          `suppressionRules.${
+                                            props.featureIndex
+                                          }[${index}].${
+                                            isPercentage === false
+                                              ? 'absoluteThreshold'
+                                              : 'relativeThreshold'
+                                          }`,
+                                          form
+                                        )}


Is it better to call the function extractError only once like

const thresholdFieldName = `suppressionRules.${props.featureIndex}[${index}].${ isPercentage === false ? 'absoluteThreshold' : 'relativeThreshold' }`; const thresholdError = extractError(thresholdFieldName, form); <FormattedFormRow isInvalid={!!thresholdError} error={thresholdError} fullWidth > {/* … */} </FormattedFormRow>

Yes I just have to put inside an inline expression and do something like this:

{(() => { const thresholdFieldName = `suppressionRules.${props.featureIndex}[${index}].${ isPercentage === false ? 'absoluteThreshold' : 'relativeThreshold' }`; const thresholdError = extractError(thresholdFieldName, form); return ( <FormattedFormRow isInvalid={!!thresholdError} error={thresholdError} fullWidth >

but need to make sure I have correct access to all the constants

made the change

Signed-off-by: Amit Galitzky <[email protected]>

kaituo · 2025-01-27T22:12:53Z

public/pages/ConfigureModel/components/Features/Features.tsx

@@ -63,6 +63,8 @@ export function Features(props: FeaturesProps) {
      <EuiFlexGroup direction="column" style={{ margin: '0px' }}>
        <FieldArray name="featureList" validateOnChange={true}>
          {({ push, remove, form: { values } }: FieldArrayRenderProps) => {
+                              console.log("values.featureList: " + JSON.stringify(values.featureList))


Do you want to keep this log line? Looks like debug message.

removed, added it when I was checking the types I added

Signed-off-by: Amit Galitzky <[email protected]>

…#960) * adding feature direction and moving suppression rules to each feature Signed-off-by: Amit Galitzky <[email protected]> * fixing bug where two conditions didn't display Signed-off-by: Amit Galitzky <[email protected]> * improve error handling Signed-off-by: Amit Galitzky <[email protected]> * style changes and removing extra errors Signed-off-by: Amit Galitzky <[email protected]> * remove console log Signed-off-by: Amit Galitzky <[email protected]> --------- Signed-off-by: Amit Galitzky <[email protected]> (cherry picked from commit e60e4e9)

…opensearch-project#960) * adding feature direction and moving suppression rules to each feature Signed-off-by: Amit Galitzky <[email protected]> * fixing bug where two conditions didn't display Signed-off-by: Amit Galitzky <[email protected]> * improve error handling Signed-off-by: Amit Galitzky <[email protected]> * style changes and removing extra errors Signed-off-by: Amit Galitzky <[email protected]> * remove console log Signed-off-by: Amit Galitzky <[email protected]> --------- Signed-off-by: Amit Galitzky <[email protected]>

…#960) (#966) * adding feature direction and moving suppression rules to each feature * fixing bug where two conditions didn't display * improve error handling * style changes and removing extra errors * remove console log --------- Signed-off-by: Amit Galitzky <[email protected]>

amitgalitz requested review from ohltyler, kaituo, jackiehanyang, sean-zheng-amazon, dbwiddis, owaiskazi19 and joshpalis as code owners December 19, 2024 19:49

opensearch-trigger-bot bot added the backport 2.x label Dec 19, 2024

amitgalitz force-pushed the direction branch from 1e72613 to 104aa8a Compare December 19, 2024 20:52

adding feature direction and moving suppression rules to each feature

553c521

Signed-off-by: Amit Galitzky <[email protected]>

amitgalitz force-pushed the direction branch from 104aa8a to 553c521 Compare December 19, 2024 22:01

fixing bug where two conditions didn't display

6e7187f

Signed-off-by: Amit Galitzky <[email protected]>

improve error handling

9dcec7e

Signed-off-by: Amit Galitzky <[email protected]>

kaituo reviewed Jan 24, 2025

View reviewed changes

style changes and removing extra errors

138a20f

Signed-off-by: Amit Galitzky <[email protected]>

kaituo reviewed Jan 27, 2025

View reviewed changes

remove console log

d1d4e96

Signed-off-by: Amit Galitzky <[email protected]>

kaituo approved these changes Jan 27, 2025

View reviewed changes

kaituo merged commit e60e4e9 into opensearch-project:main Jan 27, 2025
9 checks passed

opensearch-trigger-bot bot mentioned this pull request Jan 27, 2025

[Backport 2.x] Adding feature direction and moving suppression rules to each feature #964

Closed

amitgalitz mentioned this pull request Jan 28, 2025

[Backport 2.x] Adding feature direction and moving suppression rules to each feature #966

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding feature direction and moving suppression rules to each feature #960

Adding feature direction and moving suppression rules to each feature #960

amitgalitz commented Dec 19, 2024 •

edited

Loading

kaituo commented Dec 20, 2024

amitgalitz commented Dec 20, 2024

kaituo commented Dec 26, 2024

kaituo Jan 24, 2025

amitgalitz Jan 27, 2025

kaituo Jan 24, 2025

amitgalitz Jan 27, 2025

kaituo Jan 24, 2025

amitgalitz Jan 27, 2025

kaituo Jan 27, 2025

amitgalitz Jan 27, 2025

kaituo Jan 24, 2025

amitgalitz Jan 27, 2025 •

edited

Loading

amitgalitz Jan 27, 2025

kaituo Jan 27, 2025

amitgalitz Jan 27, 2025

Adding feature direction and moving suppression rules to each feature #960

Adding feature direction and moving suppression rules to each feature #960

Conversation

amitgalitz commented Dec 19, 2024 • edited Loading

Description

Check List

kaituo commented Dec 20, 2024

amitgalitz commented Dec 20, 2024

kaituo commented Dec 26, 2024

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

amitgalitz Jan 27, 2025 • edited Loading

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

amitgalitz commented Dec 19, 2024 •

edited

Loading

amitgalitz Jan 27, 2025 •

edited

Loading