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.

Sign up for GitHub

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

Jump to bottom

#3513: Mark muted alerts #3793

Merged
merged 2 commits into from May 13, 2024
Merged

#3513: Mark muted alerts #3793

merged 2 commits into from May 13, 2024

Conversation

grobinson-grafana
Copy link
Contributor

@grobinson-grafana grobinson-grafana commented Apr 2, 2024 •
edited

This pull request updates TimeMuteStage and TimeActiveStage to mark groups as muted when its alerts are muted by an active or mute time interval, and remove any existing markers when outside all active and mute time intervals.

It is based on #3792, #3794 and #3795.

@grobinson-grafana grobinson-grafana mentioned this pull request Apr 2, 2024
Open
@grobinson-grafana grobinson-grafana marked this pull request as draft April 2, 2024 18:20
@grobinson-grafana grobinson-grafana marked this pull request as ready for review April 3, 2024 10:26
grobinson-grafana
grobinson-grafana commented Apr 3, 2024
View reviewed changes
dispatch/dispatch.go Outdated
@@ -182,6 +184,7 @@ func (d *Dispatcher) run(it provider.AlertIterator) {
for _, ag := range groups {
if ag.empty() {
ag.stop()
d.marker.DeleteByGroupKey(ag.GroupKey())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to delete the marker when the aggregation group is deleted. There is a similar mechanism in provider/mem/mem.go:

alertmanager/provider/mem/mem.go

Line 108 in 14cbe63

m.Delete(alert.Fingerprint())

@grobinson-grafana grobinson-grafana mentioned this pull request Apr 3, 2024
Open
@grobinson-grafana grobinson-grafana changed the title Mark muted alerts #3513: Mark muted alerts Apr 3, 2024
@grobinson-grafana grobinson-grafana force-pushed the grobinson/mark-muted-alerts-3 branch 2 times, most recently from 2021070 to 9a433cf Compare April 11, 2024 14:32
@grobinson-grafana grobinson-grafana marked this pull request as draft April 11, 2024 15:01
@grobinson-grafana grobinson-grafana force-pushed the grobinson/mark-muted-alerts-3 branch 4 times, most recently from ff2401d to 3cec1e8 Compare April 29, 2024 16:55
@grobinson-grafana grobinson-grafana marked this pull request as ready for review April 30, 2024 13:40
@grobinson-grafana grobinson-grafana mentioned this pull request Apr 30, 2024
Merged
@grobinson-grafana
Mark muted groups
9342bc2 
This commit updates TimeMuteStage and TimeActiveStage to mark groups
as muted when its alerts are muted by an active or mute time interval,
and remove any existing markers when outside all active and mute
time intervals.

Signed-off-by: George Robinson <[email protected]>
grobinson-grafana
grobinson-grafana commented Apr 30, 2024
View reviewed changes
dispatch/dispatch.go
@@ -107,7 +108,7 @@ func NewDispatcher(
ap provider.Alerts,
r *Route,
s notify.Stage,
mk types.AlertMarker,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

mk was never used, here I am replacing it with types.GroupMarker.

dispatch/dispatch.go
@@ -145,8 +147,8 @@ func (d *Dispatcher) Run() {
}

func (d *Dispatcher) run(it provider.AlertIterator) {
cleanup := time.NewTicker(30 * time.Second)
defer cleanup.Stop()
maintenance := time.NewTicker(30 * time.Second)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I renamed this as all other "cleanup" functions in Alertmanager are called maintenance.

dispatch/dispatch.go
@@ -175,28 +177,30 @@ func (d *Dispatcher) run(it provider.AlertIterator) {
}
d.metrics.processingDuration.Observe(time.Since(now).Seconds())

case <-cleanup.C:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved this logic to a method called doMaintenance. The main purpose of this was being able to test it.

dispatch/dispatch.go
for _, ag := range groups {
if ag.empty() {
ag.stop()
d.marker.DeleteByGroupKey(ag.routeID, ag.GroupKey())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We delete the marker for the aggregation group when the aggregation group is deleted itself.

dispatch/dispatch.go
@@ -374,6 +378,7 @@ type aggrGroup struct {
labels model.LabelSet
opts *RouteOpts
logger log.Logger
routeID string
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We needed to add routeID to aggrGroup because it's used in d.marker.DeleteByGroupKey(ag.routeID, ag.GroupKey()).

dispatch/dispatch.go
@@ -447,6 +453,7 @@ func (ag *aggrGroup) run(nf notifyFunc) {
ctx = notify.WithRepeatInterval(ctx, ag.opts.RepeatInterval)
ctx = notify.WithMuteTimeIntervals(ctx, ag.opts.MuteTimeIntervals)
ctx = notify.WithActiveTimeIntervals(ctx, ag.opts.ActiveTimeIntervals)
ctx = notify.WithRouteID(ctx, ag.routeID)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We also need to add it to the context so it can be extracted in TimeMuteStage and TimeActiveStage.

dispatch/dispatch_test.go
@@ -691,3 +691,48 @@ type limits struct {
func (l limits) MaxNumberOfAggregationGroups() int {
return l.groups
}

func TestDispatcher_DoMaintenance(t *testing.T) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here I am testing that the marker is deleted when the aggregation group is garbage collected.

notify/notify_test.go
@@ -827,12 +828,6 @@ func TestTimeMuteStage(t *testing.T) {
}
eveningsAndWeekends := map[string][]timeinterval.TimeInterval{
"evenings": {{
Weekdays: []timeinterval.WeekdayRange{{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed Weekdays so I could test a group being muted by both evenings and weekends at the same time.

notify/notify_test.go

// Get the names of all time intervals for the context.
muteTimeIntervalNames := make([]string, 0, len(test.intervals))
for name := range test.intervals {
muteTimeIntervalNames = append(muteTimeIntervalNames, name)
}
// Sort the names so we can compare mutedBy with test.mutedBy.
sort.Strings(muteTimeIntervalNames)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Needs to be sorted because test.intervals is a map.

grobinson-grafana
grobinson-grafana commented Apr 30, 2024
View reviewed changes
notify/notify_test.go
require.NoError(t, prom_testutil.GatherAndCompare(r, strings.NewReader(`
# HELP alertmanager_marked_alerts How many alerts by state are currently marked in the Alertmanager regardless of their expiry.
# TYPE alertmanager_marked_alerts gauge
alertmanager_marked_alerts{state="active"} 0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do I need to add metrics for marked groups? @gotjosh

Copy link
Member

Choose a reason for hiding this comment

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

I don't think so - I wouldn't really understand their historically use-case.

gotjosh
gotjosh approved these changes May 9, 2024
View reviewed changes
Copy link
Member

@gotjosh gotjosh left a comment

Choose a reason for hiding this comment

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

LGTM

dispatch/dispatch.go Show resolved Hide resolved
notify/notify.go
Comment on lines +236 to +237
// RouteID extracts a RouteID from the context. Iff none exists, the
// // second argument is false.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// RouteID extracts a RouteID from the context. Iff none exists, the
// // second argument is false.
// RouteID extracts a RouteID from the context. If none exists, the
// second argument is false.

The other comments have the same typo of iff

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh iff is not a spelling mistake, it refers to if and only if.

Copy link
Member

Choose a reason for hiding this comment

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

Today I learned 🤯

@grobinson-grafana
Remove unlock to defer
c921eab 
Signed-off-by: George Robinson <[email protected]>
@gotjosh gotjosh merged commit c4a763c into prometheus:main May 13, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gotjosh gotjosh gotjosh approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@grobinson-grafana @gotjosh