Skip to content
New issue

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

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

Already on GitHub? # to your account

fix: metric maintenace disappearance #1145

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

AleksandrMatsko
Copy link
Member

@AleksandrMatsko AleksandrMatsko commented Feb 19, 2025

PR Summary

Fix the bug, appearing in the following case:

  • have metric, that comes rarely (more than 10 min between dots)
  • create trigger with graphite source based on such metric with function filterSeries
  • set maintenance for the resulted metric
  • update trigger, for example change it's description
  • no maintenance on metric anymore

It happens because last check for the trigger is modified according to time series fetched from now-10min to now.

This PR try to avoid unnecessary modifying last check, by checking the difference between existed and new versions of trigger (more precisely it is checked if the metrics evaluation and alerting rules have changed).

@AleksandrMatsko
Copy link
Member Author

/build

Copy link

Build and push Docker images with tag: fix-metric-maintenace-disappearance.2025-02-20.af5e136

@AleksandrMatsko AleksandrMatsko marked this pull request as ready for review February 21, 2025 10:45
@AleksandrMatsko AleksandrMatsko requested a review from a team as a code owner February 21, 2025 10:45
for metric := range lastCheck.Metrics {
if _, ok := timeSeriesNames[metric]; !ok {
lastCheck.RemoveMetricState(metric)
modifyLastCheck := true
Copy link
Member

Choose a reason for hiding this comment

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

At here you can initialize variable to needed value (like at 43 line):

modifyLastCheck := len(timeSeriesNames) != 0 || metricEvaluationRulesChanged(existedTrigger, newTrigger)

It would be more readable I think

Copy link
Member Author

Choose a reason for hiding this comment

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

well that's reasonable, so refactored condition a bit and add comment

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants