add per-tenant alertmanager metrics #2124

jtlisi · 2020-02-12T20:44:38Z

Moved from #2116 because integration tests don't work for Grafana org repo PRs due to NOQUAY being set as an environment variable.

What this PR does:

This PR takes advantage of the util.MetricFamiliesPerUser struct to provide per-tenant Alertmanager metrics.

Which issue(s) this PR fixes:
Fixes #1631

Checklist

Tests updated
CHANGELOG.md updated

jtlisi · 2020-02-13T03:08:21Z

@pracucci I paired down the number of user metrics considerably. The ones that remain convey either essential basic information about the number of alerts/silences or help users avoid silent failures.

cortex_alertmanager_alerts -- Basic information that gives a basic understanding of alertmanager usage on a per-user basis
cortex_alertmanager_silences -- Same as above

cortex_alertmanager_notifications_total & cortex_alertmanager_notifications_failed_total -- These metrics are vital for detecting a silent failure in a users config. A user could have a valid Alertmanager config with a misconfigured integration. This could easily lead to a silent failure where a user is not receiving alerts. Having this metric on a per-user basis makes it easy to configure redundant alerts in either a separate Prometheus/Alertmanager system or using something like Prometheus/(Grafana Alerts).

I also made sure to remove the unused functions I added to metrics_helper.go, as well as add unit tests for the function I did add.

pkg/util/metrics_helper.go

pkg/alertmanager/multitenant.go

pkg/util/metrics_helper_test.go

pstibrany

I have left some comments around metrics code.

pkg/util/metrics_helper.go

pkg/util/metrics_helper_test.go

pkg/alertmanager/multitenant.go

gouthamve

LGTM with nit. I haven't taken a close look at the tests as I think Marco and Peter took a close look there.

@jtlisi Could you also resolve conversations that have been already implemented?

pkg/alertmanager/alertmanager_metrics.go

pracucci

@jtlisi Thanks for addressing my feedback. I'm a bit concerned about the "unpause" logic and wondering if there's any issue there. Please take a look at comments.

pkg/util/metrics_helper.go

pkg/alertmanager/multitenant.go

pkg/alertmanager/multitenant_test.go

pkg/alertmanager/multitenant.go

pkg/alertmanager/alertmanager.go

pkg/alertmanager/multitenant.go

pkg/alertmanager/alertmanager.go

pstibrany

LGTM with minor nits.
(Btw, flaky test is now fixed on master)

pkg/alertmanager/multitenant.go

pkg/alertmanager/alertmanager.go

pracucci

Approving cause my previous concern about the "unpause" is not an issue. However I left few comments which I would be glad if you could address before merging. Thanks @jtlisi !

pkg/alertmanager/alertmanager.go

pracucci · 2020-03-02T08:52:58Z

pkg/alertmanager/alertmanager.go

 	return nil
 }

+func (am *Alertmanager) isActive() bool {


Why isn't exposed, while Pause() is? I think it should be specular to Pause() and being exposed too.

Makes sense

pracucci · 2020-03-02T08:56:39Z

pkg/alertmanager/multitenant.go

-			userAM.Stop()
-			delete(am.alertmanagers, user)
+			// The user alertmanager is only paused in order to retain the prometheus metrics
+			// it has reported to it's registry. If a new config for this user appears, this structure


it's > its

Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>

jtlisi mentioned this pull request Feb 12, 2020

add per-tenant alertmanager metrics #2116

Closed

2 tasks

pracucci reviewed Feb 13, 2020

View reviewed changes

pkg/util/metrics_helper.go Show resolved Hide resolved

pracucci reviewed Feb 13, 2020

View reviewed changes

pkg/alertmanager/multitenant.go Show resolved Hide resolved

pkg/util/metrics_helper_test.go Show resolved Hide resolved

pstibrany reviewed Feb 13, 2020

View reviewed changes

pkg/util/metrics_helper.go Show resolved Hide resolved

pkg/util/metrics_helper_test.go Outdated Show resolved Hide resolved

pkg/util/metrics_helper_test.go Outdated Show resolved Hide resolved

pkg/alertmanager/multitenant.go Show resolved Hide resolved

gouthamve approved these changes Feb 28, 2020

View reviewed changes

pkg/alertmanager/alertmanager_metrics.go Show resolved Hide resolved

pracucci reviewed Feb 28, 2020

View reviewed changes

pstibrany reviewed Feb 28, 2020

View reviewed changes

pkg/alertmanager/multitenant.go Show resolved Hide resolved

pkg/alertmanager/alertmanager.go Outdated Show resolved Hide resolved

pstibrany approved these changes Feb 28, 2020

View reviewed changes

pkg/alertmanager/multitenant.go Outdated Show resolved Hide resolved

pracucci reviewed Feb 28, 2020

View reviewed changes

pkg/alertmanager/alertmanager.go Outdated Show resolved Hide resolved

pracucci approved these changes Feb 28, 2020

View reviewed changes

pkg/alertmanager/alertmanager.go Outdated Show resolved Hide resolved

pracucci reviewed Mar 2, 2020

View reviewed changes

jtlisi added 7 commits March 2, 2020 09:57

add per-tenant alertmanager metrics

380f407

Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>

fix linting error

3bece6c

Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>

update in regards to PR comments

20cfea6

Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>

fix double lock

ef6b4ff

Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>

adjust cortex_alertmanager_alerts_received_total to be per user

87bf3d4

Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>

make isActive public function and fix typo

f50885f

Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>

comment IsActive

cabeb79

Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>

jtlisi merged commit 3c6875d into cortexproject:master Mar 2, 2020

jtlisi deleted the 20200211_pertenant_am_metrics branch March 2, 2020 15:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

add per-tenant alertmanager metrics #2124

add per-tenant alertmanager metrics #2124

Uh oh!

jtlisi commented Feb 12, 2020 •

edited

Loading

jtlisi commented Feb 13, 2020 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

pstibrany left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gouthamve left a comment

Uh oh!

pracucci left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pstibrany left a comment •

edited

Loading

Uh oh!

Uh oh!

pracucci left a comment

Uh oh!

pracucci Mar 2, 2020

jtlisi Mar 2, 2020

pracucci Mar 2, 2020

add per-tenant alertmanager metrics #2124

add per-tenant alertmanager metrics #2124

Uh oh!

Conversation

jtlisi commented Feb 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

jtlisi commented Feb 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pstibrany left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gouthamve left a comment

Choose a reason for hiding this comment

Uh oh!

pracucci left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pstibrany left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pracucci left a comment

Choose a reason for hiding this comment

Uh oh!

pracucci Mar 2, 2020

Choose a reason for hiding this comment

jtlisi Mar 2, 2020

Choose a reason for hiding this comment

pracucci Mar 2, 2020

Choose a reason for hiding this comment

jtlisi commented Feb 12, 2020 •

edited

Loading

jtlisi commented Feb 13, 2020 •

edited

Loading

pstibrany left a comment •

edited

Loading