upcoming: [DI-22184] - Added Metric Criteria, Metric components #11392

santoshp210-akamai · 2024-12-10T17:57:32Z

Description 📝

Added the MetricCriteria and Metric components
Added new Unit Tests for the added components and added more tests and checks for existing components to reflect the latest changes
Changed the types and validations to reflect the latest API spec

Changes 🔄

Added the Metric Criteria component for the Criteria Section using FieldArray to add the Metric components dynamically
Added Autocomplete and TextField inputs for Data Field, Aggregation Type, Operator and Threshold in Metric
Added the relevant test cases for Unit Tests
Added and changed some types of CreateAlertDefinitionPayload, Alert interfaces

Target release date 🗓️

Please specify a release date (and environment, if applicable) to guarantee timely review of this PR. If exact date is not known, please approximate and update it as needed.

Preview 📷

Before	After

How to test 🧪

Prerequisites

(How to setup test environment)

The tabs are controlled by a featureFlag called aclpAlerting, the flag has been currently disabled. For testing enable the definitions part of the aclpAlerting flag to be true.
The API endpoints are not live yet, so use Legacy MSW Handlers for the mock responses

Verification steps

In Monitor (once the prerequisites are followed) , Alerts tab should be visible next to Dashboards.
Under that, Definition tab and a Create button should be visible.
On clicking Create, the Form Page should be visible
To choose from the Data Field drop down, selecting a Service is required
To choose from the Aggregation Field drop down, selecting a Data Field is required.
The Metric component can be added and removed dynamically with the Add Metric and Delete Icon buttons
The Delete Icon button is only visible when there is more than one Metric component

Author Checklists

As an Author, to speed up the review process, I considered 🤔

👀 Doing a self review
❔ Our contribution guidelines
🤏 Splitting feature into small PRs
➕ Adding a changeset
🧪 Providing/improving test coverage
🔐 Removing all sensitive information from the code and PR description
🚩 Using a feature flag to protect the release
👣 Providing comprehensive reproduction steps
📑 Providing or updating our documentation
🕛 Scheduling a pair reviewing session
📱 Providing mobile support
♿ Providing accessibility support

I have read and considered all applicable items listed above.

As an Author, before moving this PR from Draft to Open, I confirmed ✅

All unit tests are passing
TypeScript compilation succeeded without errors
Code passes all linting rules

…nit Tests and minor changes to align with changes in API spec

…nit tests, added a type for the Form, renamed prop

…testing

…Unit Test

santoshp210-akamai · 2024-12-10T18:01:21Z

There are currently some discussions with respect to limiting the number of Metric Components and the way to limit them in UI.
As discussed with @jaalah-akamai , the Unit being part of the adornment of TextField and the Threshold input being float are also under discussion. These changes are minimal and will be resolved soon. It will be either updated in this PR or will be followed by a minimal PR.

santoshp210-akamai · 2024-12-10T18:08:24Z

The typecheck manager test is failing as there is a state variable that's not used as it will be used in components that will be added later, but we are setting the state in this component

github-actions · 2024-12-10T18:31:07Z

Coverage Report: ✅
Base Coverage: 86.89%
Current Coverage: 86.89%

linode-gh-bot · 2024-12-10T18:51:03Z

Cloud Manager UI test results

🎉 467 passing tests on test run #2 ↗︎

❌ Failing	✅ Passing	↪️ Skipped	🕐 Duration
`0 Failing`	`467 Passing`	`2 Skipped`	`106m 36s`

hkhalil-akamai · 2024-12-10T22:21:42Z

packages/manager/src/features/CloudPulse/Alerts/CreateAlert/utilities.ts

+      metric: rule.metric!,
+      operator: rule.operator!,


Is there a way to avoid this non-null assertion?

The metric value is null in the first place just to handle clear option in the Autocomplete, and we have validation to make sure it's a required field and the submit won't happen successfully unless and until the field has the proper value . So the non-null assertion is almost never going to be checked.

We actually did the same thing for Severity.

manager/packages/manager/src/features/CloudPulse/Alerts/CreateAlert/utilities.ts

Line 20 in 681acc6

const severity = formValues.severity!;

It's important to note this isn't a null check, it's just a null assertion which will assume the value is non-null even if it isn't. I prefer to avoid it if at all possible unless the team believes it's harmless. For example, we can use a default instead:

Suggested change

metric: rule.metric!,

operator: rule.operator!,

metric: rule.metric ?? '',

operator: rule.operator ?? 'eq',

I guess to fix this we'd have to update the form types to not allow things like metric and operator to be nullable. This might require us to make those fields not clearable to satisfy the types

The fields not being clearable should be fine, but for components to be controlled we have initialized with null. I think the solution suggested by @hkhalil-akamai might be more suited for our use-case. I will try that change and if it's not affecting anything I'll make that change and push the changes.

bnussman-akamai · 2024-12-11T15:53:59Z

packages/manager/src/features/CloudPulse/Alerts/CreateAlert/Criteria/MetricCriteria.tsx

+
+  const { control, watch } = useFormContext<CreateAlertDefinitionForm>();
+
+  const metricCriteriaWatcher = watch(name);


Please see #11357 (comment). watch should be avoided if possible

Okay, going ahead with useWatch()

bnussman-akamai · 2024-12-11T15:58:06Z

packages/manager/src/features/CloudPulse/Alerts/CreateAlert/Criteria/MetricCriteria.tsx

+  React.useEffect(() => {
+    const formMetricValues = new Set(
+      metricCriteriaWatcher.map((item: MetricCriteriaForm) => item.metric)
+    );
+
+    const intervalList =
+      metricDefinitions &&
+      metricDefinitions.data
+        .filter((item) => formMetricValues.has(item.metric))
+        .map((item) => item.scrape_interval);
+    const maxInterval = Math.max(
+      ...convertToSeconds(intervalList ? intervalList : [])
+    );
+    setMaxInterval(maxInterval);
+  }, [setMaxInterval, metricCriteriaWatcher, metricDefinitions]);


Let's try to remove this useEffect. We're really trying to not use useEffect in our codebase anymore 😖. Usually this can be accomplished by moving this logic into an onChange rather than doing it here as a side-effect of something changing.

Any particular reason why we are avoiding useEffect ? Because while implementing I saw multiple components using it, so implemented

We've had a handful of bugs caused by useEffect. The official react docs also advocate to not use useEffect: https://react.dev/learn/you-might-not-need-an-effect

santoshp210-akamai added 7 commits December 9, 2024 12:37

upcoming: [DI-22184] - Added Metric, MetricCriteria components with U…

35aba8c

…nit Tests and minor changes to align with changes in API spec

upcoming: [DI-22184] - Internal Review Changes: Assertion checks in U…

8f27d96

…nit tests, added a type for the Form, renamed prop

upcoming: [DI-22184] - Removing few initial values used for internal …

6fa2545

…testing

upcoming: [DI-22184] - Fixing previoius accidental commit

6431f02

upcoming: [DI-22184] - Made the Clear icon a separate Component with …

0a5731c

…Unit Test

merge commit: Latest commits from linode/develop

179623f

Merge branch 'linode:develop' into feature/create-alert-form-criteria

8b71746

santoshp210-akamai requested a review from a team as a code owner December 10, 2024 17:57

santoshp210-akamai requested review from hana-akamai and hkhalil-akamai and removed request for a team December 10, 2024 17:57

upcoming: [DI-22184] - Added changesets

302095d

santoshp210-akamai self-assigned this Dec 10, 2024

hkhalil-akamai reviewed Dec 10, 2024

View reviewed changes

bnussman-akamai reviewed Dec 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

upcoming: [DI-22184] - Added Metric Criteria, Metric components #11392

upcoming: [DI-22184] - Added Metric Criteria, Metric components #11392

santoshp210-akamai commented Dec 10, 2024

santoshp210-akamai commented Dec 10, 2024

santoshp210-akamai commented Dec 10, 2024

github-actions bot commented Dec 10, 2024 •

edited

Loading

linode-gh-bot commented Dec 10, 2024

hkhalil-akamai Dec 10, 2024

santoshp210-akamai Dec 11, 2024

hkhalil-akamai Dec 11, 2024

bnussman-akamai Dec 12, 2024

santoshp210-akamai Dec 12, 2024

bnussman-akamai Dec 11, 2024

santoshp210-akamai Dec 11, 2024

bnussman-akamai Dec 11, 2024

santoshp210-akamai Dec 11, 2024

bnussman-akamai Dec 11, 2024


		const { control, watch } = useFormContext<CreateAlertDefinitionForm>();

		const metricCriteriaWatcher = watch(name);

upcoming: [DI-22184] - Added Metric Criteria, Metric components #11392

Are you sure you want to change the base?

upcoming: [DI-22184] - Added Metric Criteria, Metric components #11392

Conversation

santoshp210-akamai commented Dec 10, 2024

Description 📝

Changes 🔄

Target release date 🗓️

Preview 📷

How to test 🧪

Prerequisites

Verification steps

As an Author, to speed up the review process, I considered 🤔

As an Author, before moving this PR from Draft to Open, I confirmed ✅

santoshp210-akamai commented Dec 10, 2024

santoshp210-akamai commented Dec 10, 2024

github-actions bot commented Dec 10, 2024 • edited Loading

linode-gh-bot commented Dec 10, 2024

Cloud Manager UI test results

🎉 467 passing tests on test run #2 ↗︎

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

github-actions bot commented Dec 10, 2024 •

edited

Loading