#3513: Add GroupMarker interface #3792

grobinson-grafana · 2024-04-02T09:59:09Z

This commit adds a new GroupMarker interface that marks the status of groups. For example, whether a group is muted because or one or more active or mute time intervals. It renames the existing Marker interface to AlertMarker to avoid confusion.

It is based on #3791.

grobinson-grafana · 2024-04-03T15:01:14Z

types/types.go

+ // Muted returns true if the group is muted, otherwise false. If the group
+ // is muted then it also returns the names of the time intervals that muted
+ // it.
+ Muted(groupKey string) ([]string, bool)


In the original version of GroupMarker I had:

type GroupMarker interface { // Muted returns true if the group is muted, otherwise false. If the group // is muted then it also returns the names of the time intervals that muted // it. Muted(groupKey string, fingerprint model.Fingerprint) ([]string, bool) ... }

but then I realized we didn't need to store the fingerprint at all.

The reason here is that active and mute timings work against routes. That means either all alerts in an aggregation group are suppressed because of time intervals, or none of them are. It is not possible to have two alerts A and B in the same aggregation group, where A is muted from a mute time interval and B is not.

We have found an issue where because groupKey is not guaranteed to be unique, it's possible for two (or more) different groups to have the same groupKey. In such cases, where one group is muted and another group is not, both will be marked as muted.

The following YAML shows a configuration containing two routes with the same groupKey. This happens because the groupKey is calculated using:

The matchers in the route

The group_by labels (default is empty)

And both routes have the same matchers and group_by labels.

receivers: - name: test1 - name: test2 route: receiver: test1 routes: - receiver: test1 matchers: - foo=bar - receiver: test2 matchers: - foo=bar mute_time_intervals: - name: weekends

The reason a user might have such a configuration is to mute notifications on the weekends, but still send webhooks to an issue tracker.

grobinson-grafana · 2024-04-29T16:22:07Z

types/types.go

+// GroupMarker helps to mark groups as active or muted.
+// All methods are goroutine-safe.
+//
+// TODO(grobinson): routeID is used in Muted and SetMuted because groupKey


I added a TODO in the source code to make it clear that this is not how I want the interface to look, but fixing this is blocked due to #3817 that we will fix in another PR.

grobinson-grafana · 2024-04-30T08:51:01Z

types/types.go

 // All methods are goroutine-safe.
-type Marker interface {
+type AlertMarker interface {


I renamed this interface to AlertMarker so it could be better differentiated from GroupMarker.

grobinson-grafana · 2024-04-30T09:28:46Z

types/types.go

+func (m *MemMarker) Muted(routeID, groupKey string) ([]string, bool) {
+ m.mtx.Lock()
+ defer m.mtx.Unlock()
+ status, ok := m.groups[routeID+groupKey]


Since routeID will be removed in the future I have chosen to just concatenate the two strings together.

This commit adds a new GroupMarker interface that marks the status of groups. For example, whether an alert is muted because or one or more active or mute time intervals. It renames the existing Marker interface to AlertMarker to avoid confusion. Signed-off-by: George Robinson <[email protected]>

Signed-off-by: George Robinson <[email protected]>

This commit changes memMarker to MemMarker as it now implements both the AlertMarker and GroupMarker interfaces. We can return *memMarker, but it causes lint to fail. Signed-off-by: George Robinson <[email protected]>

This commit fixes a bug in SetMuted where a marker could not be removed. The method now works as documented in the interface. Signed-off-by: George Robinson <[email protected]>

Signed-off-by: George Robinson <[email protected]>

I realized that since active and mute timings are applied to whole group rather than individual alerts within a group, we can remove fingerprints from the GroupMarker interface. This will make the code much simpler and also reduce the amount of data that needs to be tracked. Signed-off-by: George Robinson <[email protected]>

Signed-off-by: George Robinson <[email protected]>

grobinson-grafana · 2024-04-30T12:39:20Z

types/types.go

+ // Muted returns true if the group is muted, otherwise false. If the group
+ // is muted then it also returns the names of the time intervals that muted
+ // it.
+ Muted(routeID, groupKey string) ([]string, bool)


I added routeID to avoid the issue of non unique group keys from causing groups to be incorrectly marked as muted. See #3817 for more information.

gotjosh

Overall looks good, but please see my comment.

gotjosh · 2024-04-30T13:17:19Z

types/types.go

+// groupStatus stores the state of the group, and, as applicable, the names
+// of all active and mute time intervals that are muting it.


This claims to store the state of the group but its only attribute is mutedBy is this correct?

Yes! I copied it from here:

// AlertStatus stores the state of an alert and, as applicable, the IDs of // silences silencing the alert and of other alerts inhibiting the alert.

gotjosh · 2024-04-30T13:21:23Z

types/types.go

+ status = &groupStatus{}
+ m.groups[routeID+groupKey] = status
+ }
+ status.mutedBy = timeIntervalNames


Don't we need to aggregate all the time-intervals that this has been muted by?

We don't – the reason is that the Mutes method we just merged returns them all:

alertmanager/timeinterval/timeinterval.go

Lines 37 to 53 in dacbf00

func (i *Intervener) Mutes(names []string, now time.Time) (bool, []string, error) {

var in []string

for _, name := range names {

interval, ok := i.intervals[name]

if !ok {

return false, nil, fmt.Errorf("time interval %s doesn't exist in config", name)

}

for _, ti := range interval {

if ti.ContainsTime(now.UTC()) {

in = append(in, name)

}

}

}

return len(in) > 0, in, nil

}

.

gotjosh · 2024-04-30T13:22:12Z

types/types.go

+func NewMarker(r prometheus.Registerer) *MemMarker {
+ m := &MemMarker{
+ alerts: map[model.Fingerprint]*AlertStatus{},
+ groups: map[string]*groupStatus{},


How is this gargage collected? As in, how does the marker now that it no longer needs to store the status of an alert.

See #3793 (comment).

gotjosh

LGTM

Thanks for clarifying my doubts!

* Add GroupMarker interface This commit adds a new GroupMarker interface that marks the status of groups. For example, whether an alert is muted because or one or more active or mute time intervals. It renames the existing Marker interface to AlertMarker to avoid confusion. Signed-off-by: George Robinson <[email protected]> --------- Signed-off-by: George Robinson <[email protected]>

This was referenced Apr 2, 2024

Muted alerts are not suppressed in API #3513

Open

#3513: Mark muted alerts #3793

Merged

grobinson-grafana changed the title ~~Add GroupMarker interface~~ #3513: Add GroupMarker interface Apr 3, 2024

grobinson-grafana commented Apr 3, 2024

View reviewed changes

grobinson-grafana force-pushed the grobinson/mark-muted-alerts-2 branch 2 times, most recently from 7fb35af to f3b9659 Compare April 11, 2024 14:26

grobinson-grafana marked this pull request as draft April 11, 2024 15:01

grobinson-grafana marked this pull request as ready for review April 24, 2024 11:26

grobinson-grafana force-pushed the grobinson/mark-muted-alerts-2 branch from e5b9050 to 4b094e9 Compare April 29, 2024 16:20

grobinson-grafana commented Apr 29, 2024

View reviewed changes

grobinson-grafana force-pushed the grobinson/mark-muted-alerts-2 branch from 4b094e9 to 0d47e10 Compare April 29, 2024 16:48

grobinson-grafana mentioned this pull request Apr 30, 2024

#3513: TimeMuter returns the names of time intervals #3791

Merged

grobinson-grafana commented Apr 30, 2024

View reviewed changes

grobinson-grafana added 11 commits April 30, 2024 13:12

Remove newline

3c8dc62

Signed-off-by: George Robinson <[email protected]>

Add tests

eca889b

Signed-off-by: George Robinson <[email protected]>

Change memMarker to MemMarker

6b056cc

This commit changes memMarker to MemMarker as it now implements both the AlertMarker and GroupMarker interfaces. We can return *memMarker, but it causes lint to fail. Signed-off-by: George Robinson <[email protected]>

Fix marker cannot be removed

64a03b8

This commit fixes a bug in SetMuted where a marker could not be removed. The method now works as documented in the interface. Signed-off-by: George Robinson <[email protected]>

Fix lint

d6055ea

Signed-off-by: George Robinson <[email protected]>

Fix GroupMarker interface

47e952c

Signed-off-by: George Robinson <[email protected]>

Fix comments on GroupMarker interface

3132f61

Signed-off-by: George Robinson <[email protected]>

Fix comment in test

88dfcdb

Signed-off-by: George Robinson <[email protected]>

Add routeID to GroupMarker interface

4a2781c

Signed-off-by: George Robinson <[email protected]>

grobinson-grafana force-pushed the grobinson/mark-muted-alerts-2 branch from 0d47e10 to 4a2781c Compare April 30, 2024 12:12

grobinson-grafana commented Apr 30, 2024

View reviewed changes

gotjosh reviewed Apr 30, 2024

View reviewed changes

gotjosh approved these changes Apr 30, 2024

View reviewed changes

gotjosh merged commit d31a249 into prometheus:main Apr 30, 2024
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#3513: Add GroupMarker interface #3792

#3513: Add GroupMarker interface #3792

grobinson-grafana commented Apr 2, 2024 •

edited

grobinson-grafana Apr 3, 2024

grobinson-grafana Apr 24, 2024

grobinson-grafana Apr 24, 2024

grobinson-grafana Apr 29, 2024

grobinson-grafana Apr 30, 2024

grobinson-grafana Apr 30, 2024

grobinson-grafana Apr 30, 2024

gotjosh left a comment

gotjosh Apr 30, 2024

grobinson-grafana Apr 30, 2024

gotjosh Apr 30, 2024

grobinson-grafana Apr 30, 2024

gotjosh Apr 30, 2024

grobinson-grafana Apr 30, 2024

gotjosh Apr 30, 2024

gotjosh left a comment

		// groupStatus stores the state of the group, and, as applicable, the names
		// of all active and mute time intervals that are muting it.

	func (i *Intervener) Mutes(names []string, now time.Time) (bool, []string, error) {
	var in []string
	for _, name := range names {
	interval, ok := i.intervals[name]
	if !ok {
	return false, nil, fmt.Errorf("time interval %s doesn't exist in config", name)
	}

	for _, ti := range interval {
	if ti.ContainsTime(now.UTC()) {
	in = append(in, name)
	}
	}
	}

	return len(in) > 0, in, nil
	}

#3513: Add GroupMarker interface #3792

#3513: Add GroupMarker interface #3792

Conversation

grobinson-grafana commented Apr 2, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gotjosh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gotjosh left a comment

Choose a reason for hiding this comment

grobinson-grafana commented Apr 2, 2024 •

edited