feat: allow configuring alert rules - #209
Conversation
There was a problem hiding this comment.
Pull request overview
Adds operator-level support for overriding Prometheus alert rule for: durations by alert name, addressing deployments that need shorter/longer “for” windows without editing shipped rule files.
Changes:
- Introduces
alert_for_durationcharm config (comma-separatedAlertName=durationpairs) and validation/parsing helpers. - Updates charm config-change flow to render alert rule YAML with configured
for:overrides before COS integration reads them. - Adds unit tests for the new config validation and for rendering overrides into rule YAML.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/validate_config.py |
Adds alert-name discovery from shipped rule YAML, validation for alert_for_duration, and parsing into a dict. |
src/charm.py |
Validates the new config option and renders rule YAML with overridden for: durations on config changes. |
charmcraft.yaml |
Exposes alert_for_duration config with documentation and examples. |
tests/unit/test_validate_config.py |
Adds unit tests for validation/parsing of alert_for_duration. |
tests/unit/test_charm.py |
Extends config-change tests and adds a test that rendered rule YAML reflects the override. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
jneo8
left a comment
There was a problem hiding this comment.
Hi @ioanalazea
Thanks for the contribution.
I am thinking instead of alert_for_duration, expose one alert_rules option holding a full rules document that replaces the shipped set may be a better solution here. we do not want to add a new option per field (severity, thresholds, extra alerts).
two points:
- Validation before publishing. Invalid rules aren't caught locally; Prometheus drops the whole relation blob, silently killing alerts for every app on that grafana-agent.
- A warning in the description: the reload cancels currently-firing alerts and reports removed ones to Alertmanager as resolved
77235ab to
3ed9449
Compare
Hello, Thanks for the feedback. I agree that having such a config option would be better, so I have updated my PR with the proposed changes. I have addressed both points. For point 2, I added a message to the description of the config option. For point 1, I wanted to elaborate a little. Initially, I considered adding a validation function to the exporter code to validate the proposed YAML. However, I don't think that would be feasible in the long term. For this reason, my idea is to use Looking forward to any feedback. Thanks a lot! |
3ed9449 to
c882257
Compare
jneo8
left a comment
There was a problem hiding this comment.
Overall is good.
For this change functional test is required.
Let me know if you need any help during the implementation of functional test.
b1812b8 to
f73adb2
Compare
afe09ad to
77ca2ad
Compare
|
Overall, lgtm, but |
e3ebd89 to
11c3c67
Compare
Add an alert_rules config option for overriding the currently shipped Prometheus alerts. Fixes: canonical#208 Signed-off-by: Ioana Lazea <ioana.lazea@canonical.com>
11c3c67 to
717a55e
Compare
Thank you for the feedback. That seemed like a race condition where assert triggered before the file was removed. I added block_until to fix that. |
chanchiwai-ray
left a comment
There was a problem hiding this comment.
Wait for the test to pass
Add an alert_rules config option for overriding Prometheus alerts rules.
Fixes: #208