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

feat: add alert rule options to not send alert on state recovery and to send regardless of state change #5856

Merged
merged 16 commits into from
Mar 15, 2022

Conversation

mmdoogie
Copy link
Contributor

@mmdoogie mmdoogie commented Feb 9, 2022

This was previous requested in #3723 that went stale. I recently had a use case for it and decided to add it.

Added a checkbox to the alert rule builder that's used to determine whether or not to include noRecoveries in the TICKscript.
I also saw that stateChangesOnly was already in the structure so I exposed that in a similar manner.

image

Additionally, the swagger.json didn't look like it matches the current response to the API even before the changes, it was fixed with a new flag added.

  • CHANGELOG.md updated with a link to the PR (not the Issue)
  • Well-formatted commit messages
  • Rebased/mergeable
  • Tests pass
  • swagger.json updated (if modified Go structs or API)
  • Sign CLA (if not already signed)

@mmdoogie mmdoogie marked this pull request as ready for review February 9, 2022 08:49
@sranka
Copy link
Contributor

sranka commented Feb 10, 2022

Thank you @mmdoogie for your contribution. Can you please change your commit messages to semantic format ? For example:

  • chore: update CHANGELOG
  • feat(kapacitor): add option to set noRecoveries on alerts

@mmdoogie
Copy link
Contributor Author

Updated messages as requested. Also synced up the swagger.json with the existing state of alerts/alertNode from the codebase and added the new object parameter.

Copy link
Contributor

@sranka sranka left a comment

Choose a reason for hiding this comment

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

Thank you for updating the swagger file to be closer to reality. It still needs some polishing to be approved.

  • every new alert must be created with a generated ID
  • kapacitor/ast_test.go needs to test the new noRecoveries property
  • UI must be improved

server/kapacitors.go Outdated Show resolved Hide resolved
ui/src/kapacitor/reducers/rules.js Show resolved Hide resolved
ui/src/kapacitor/components/RuleHandlers.tsx Outdated Show resolved Hide resolved
@sranka
Copy link
Contributor

sranka commented Feb 27, 2022

@mmdoogie please let me know if you need help with the changes.

@mmdoogie
Copy link
Contributor Author

Sorry it has taken me a moment to get back to this. UI rearranged and better spaced now, definitely better:
2
also not present unless a handler has been added:
1

Will add a test for the new parameters and fix the ID part soon.

@mmdoogie mmdoogie marked this pull request as draft March 12, 2022 23:48
@mmdoogie
Copy link
Contributor Author

There's an interaction with the pipeline/tick/AST code in kapacitor -- when a new rule is created and POSTed to chronograf, the kapacitor/alerts.go addAlertNodes uses that functionality to build the alert nodes tickscript. It appears to be parsing the flags correctly up to that point but the build/format process only returns the actual alert nodes and not any of the flag parameters.

@sranka
Copy link
Contributor

sranka commented Mar 15, 2022

There's an interaction with the pipeline/tick/AST code in kapacitor -- when a new rule is created and POSTed to chronograf, the kapacitor/alerts.go addAlertNodes uses that functionality to build the alert nodes tickscript. It appears to be parsing the flags correctly up to that point but the build/format process only returns the actual alert nodes and not any of the flag parameters.

This was a UI issue that you already fixed in d806ba8, and I also modified a test to cover it in 592d2d5

@sranka sranka marked this pull request as ready for review March 15, 2022 07:38
@sranka sranka self-requested a review March 15, 2022 07:46
Copy link
Contributor

@sranka sranka left a comment

Choose a reason for hiding this comment

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

Thank you @mmdoogie for addressing my feedback. I rebased your changes on top of the latest master and also tested the new functionality manually to see it in action. It works fine. I also added an extra test to verify that the flags are kept when updating alert nodes.

@sranka sranka merged commit 6412738 into influxdata:master Mar 15, 2022
@ivankudibal ivankudibal added this to the 1.9.4 milestone Mar 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants