Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

upcoming: [DI-22558] - Added Trigger condition and Dimension Filter components #11445

Conversation

santoshp210-akamai
Copy link
Contributor

@santoshp210-akamai santoshp210-akamai commented Dec 19, 2024

Description 📝

  • Added Dimension Filters feature to the Metric Criteria component
  • Added Trigger condition component to the Create Alert Form under Criteria section

Changes 🔄

  • Added DimensionFilter component which has a FieldArray to add the DimensionFilterField components dynamically
  • Added the Autocomplete components for Data Field, Operator and Value in DimensionFilterField
  • Added the Autocomplete and TextField components for Evaluation Period, Polling Interval and Trigger Occurrences in DimensionFilterField
  • Added and changed some types of CreateAlertDefinitionPayload, Alert interfaces
  • Changed some types for MetricCriteriaForm,CreateAlertPayloadForm and added DimensionFiltersForm, TriggerConditionForm
  • Changed the filterFormValues utility methods for MetricCriteriaForm interfaces and added DimensionFilterForm and TriggerConditionForm 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
image image

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

  • 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

Verification steps

(How to verify changes)

  • The DimensionFilters component should be visible when clicked on the AddDimensionFilter Button
  • Once a Metric Data Field is selected, Dimension Filter Data Field can be selected
  • For the Value field in DimensionFilter component, Dimension Filter Data Field is required
  • For Trigger Conditions the Polling Interval and Evaluation Period components the options are dependent on the Service selected. Should be able to select and choose a value
  • The displayed options in Polling Interval and Evaluation Period components must be greater than or equal to the scrape interval of the selected Metric Data Field
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

@santoshp210-akamai santoshp210-akamai self-assigned this Dec 19, 2024
@santoshp210-akamai santoshp210-akamai marked this pull request as ready for review December 20, 2024 15:01
@santoshp210-akamai santoshp210-akamai requested a review from a team as a code owner December 20, 2024 15:01
@santoshp210-akamai santoshp210-akamai requested review from hkhalil-akamai and pmakode-akamai and removed request for a team December 20, 2024 15:01
Copy link
Contributor

@jaalah-akamai jaalah-akamai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall works and looks good - just some UX issues and perf questions I have:

UX Issues
Screenshot 2024-12-23 at 3 48 16 PM Screenshot 2024-12-23 at 3 48 26 PM
Screenshot 2024-12-23 at 4 25 51 PM Screenshot 2024-12-23 at 4 26 02 PM
Validation Error Never Resolves Re-renderings for Data Field - other fields good
Screenshot 2024-12-23 at 4 32 11 PM
re-renders.mov

@santoshp210-akamai
Copy link
Contributor Author

santoshp210-akamai commented Dec 24, 2024

#11445 (review), @jaalah-akamai , from what I remember for the Metric discussions, I had informed about these issues for the screens like Ipad or tablet like screens when they are in potrait mode. I will see to the UX changes.

@santoshp210-akamai
Copy link
Contributor Author

@jaalah-akamai , For the re-renderings issue we also not sure what's causing this behaviour. From our observations it is re-rendering entire page when we clear an option or we choose CPU utilization. Not sure what's causing this behavior

@bnussman-akamai
Copy link
Member

I believe that re-render is happening because of the maxScrapeInterval state. When a "Data Field" is selected, it causes maxScrapeInterval to be set, which will re-render the entire form because the maxScrapeInterval state is at the top-level component of the form.

@santoshp210-akamai
Copy link
Contributor Author

@bnussman-akamai, #11445 (comment) as per this, in this particular situation is it okay for the form to re-render just for that one field ? As we do want to pass it through the top-level component.
cc: @jaalah-akamai

@jaalah-akamai
Copy link
Contributor

jaalah-akamai commented Dec 26, 2024

@bnussman-akamai, #11445 (comment) as per this, in this particular situation is it okay for the form to re-render just for that one field ? As we do want to pass it through the top-level component. cc: @jaalah-akamai

FormContext may be an option here and we could just remove that field from the payload, but this can be an optimization we make maybe with your UX changes.

@santoshp210-akamai
Copy link
Contributor Author

@jaalah-akamai , The UX changes have been pushed. The units are still overloading in some situations, but there is plan to use them as adornments with using a shortened form for the units. That will be pursued as an enhancement once all the features are done. Regarding the error validation resolution, I'm not able to pinpoint why the issue is happening and requesting if there was a similar behaviour before with setValue usage in the code. Any reasoning or documentation for this specific behaviour would be helpful. Thank you

…ic Data Field and Dimension Filter Data Field
@santoshp210-akamai santoshp210-akamai requested a review from a team as a code owner December 30, 2024 06:34
@santoshp210-akamai santoshp210-akamai requested review from AzureLatte and removed request for a team December 30, 2024 06:34
@santoshp210-akamai santoshp210-akamai force-pushed the feature/create-alert-form-dimension-filters-and-trigger-conditions branch from 34d575b to e364d1d Compare December 30, 2024 07:27
Copy link

github-actions bot commented Dec 30, 2024

Coverage Report:
Base Coverage: 86.97%
Current Coverage: 86.94%

@santoshp210-akamai santoshp210-akamai force-pushed the feature/create-alert-form-dimension-filters-and-trigger-conditions branch from fb29615 to f20078a Compare December 31, 2024 05:30
@jaalah-akamai jaalah-akamai added Approved Multiple approvals and ready to merge! Cloud Pulse Cloud Pulse - Alerting labels Jan 3, 2025
@linode-gh-bot
Copy link
Collaborator

Cloud Manager UI test results

🎉 471 passing tests on test run #18 ↗︎

❌ Failing✅ Passing↪️ Skipped🕐 Duration
0 Failing471 Passing2 Skipped91m 6s

@jaalah-akamai jaalah-akamai merged commit b47661f into linode:develop Jan 7, 2025
22 of 23 checks passed
Copy link

cypress bot commented Jan 7, 2025

Cloud Manager E2E    Run #7052

Run Properties:  status check failed Failed #7052  •  git commit b47661f73b: upcoming: [DI-22558] - Added Trigger condition and Dimension Filter components (...
Project Cloud Manager E2E
Branch Review develop
Run status status check failed Failed #7052
Run duration 23m 32s
Commit git commit b47661f73b: upcoming: [DI-22558] - Added Trigger condition and Dimension Filter components (...
Committer santoshp210-akamai
View all properties for this run ↗︎

Test results
Tests that failed  Failures 23
Tests that were flaky  Flaky 1
Tests that did not run due to a developer annotating a test with .skip  Pending 2
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 455
View all changes introduced in this branch ↗︎

Tests for review

Failed  linodes/clone-linode.spec.ts • 1 failed test

View Output Video

Test Artifacts
clone linode > can clone a Linode from Linode details page Screenshots Video
Failed  stackscripts/create-stackscripts.spec.ts • 1 failed test

View Output Video

Test Artifacts
Create stackscripts > creates a StackScript with Any/All target image Screenshots Video
Failed  linodes/rebuild-linode.spec.ts • 2 failed tests

View Output Video

Test Artifacts
rebuild linode > rebuilds a linode from Image Screenshots Video
rebuild linode > rebuilds a linode from Community StackScript Screenshots Video
Failed  firewalls/update-firewall.spec.ts • 3 failed tests

View Output Video

Test Artifacts
update firewall > updates a firewall's linodes and rules Screenshots Video
update firewall > updates a firewall's status Screenshots Video
update firewall > updates a firewall's label Screenshots Video
Failed  firewalls/create-firewall.spec.ts • 1 failed test

View Output Video

Test Artifacts
create firewall > creates a firewall assigned to a linode Screenshots Video

The first 5 failed specs are shown, see all 13 specs in Cypress Cloud.

Flakiness  cypress/e2e/core/linodes/linode-config.spec.ts • 1 flaky test

View Output Video

Test Artifacts
Linode Config management > End-to-End > Clones a config Screenshots Video

dmcintyr-akamai pushed a commit to dmcintyr-akamai/manager that referenced this pull request Jan 9, 2025
…omponents (linode#11445)

* upcoming: [DI-22558] - Added Trigger condition and Dimension Filter changes

* removed unnecessary console log statement

* upcoming: [DI-22558] - Importing right interface for Trigger condition for error validation and minor changes

* upcoming: [DI-22588] - Added the Unit Tests for the components

* upcoming: [DI-22588] - Added changesets and few changes to the messages in validation schema

* upcoming: [DI-22588] - Review comments and made some minor UX changes to the component

* Updated stylings

* upcoming: [DI-22588] - UX changes

* upcoming: [DI-22588] - Fixed the validation error resolution for Metric Data Field and Dimension Filter Data Field

* upcoming: [DI-22588] - Review changes

* upcoming: [DI-22588] - Moved mockData to individual test files instead of exporting to fix the failing tests

* upcoming: [DI-22558] - Typescript issue fixes

* Update constants.ts

---------

Co-authored-by: nikhagra-akamai <nagrawal@akamai.com>
Co-authored-by: vmangalr <vmangalr@akamai.com>
Co-authored-by: venkatmano-akamai <chk-Venkatesh@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Multiple approvals and ready to merge! Cloud Pulse - Alerting Cloud Pulse
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants