AM Metric: Add tenant label to valid/invalid configs #2960

gotjosh · 2020-07-30T16:15:35Z

Gives us a way to know whenever a tenant has an invalid configuration in
place.

Signed-off-by: gotjosh [email protected]

Checklist

Tests updated
Documentation added
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

pkg/alertmanager/multitenant.go

jtlisi

When we delete a config: https://github.com/cortexproject/cortex/blob/9e8374f8dfd5eaac8084d50fc9d5936b3657b431/pkg/alertmanager/multitenant.go#L335

I think it would make sense to unregister the metrics for that user? WDYT?

pkg/alertmanager/multitenant.go

gotjosh · 2020-07-30T16:35:20Z

I think it would make sense to unregister the metrics for that user? WDYT?

Originally, I thought that it might look strange that all your other metrics are registered except this one. But maybe it makes sense?

I have added the commit if we decide to not do it I can simply remove it.

Gives us a way to know whenever a tenant has an invalid configuration in place. Signed-off-by: gotjosh <[email protected]>

Signed-off-by: gotjosh <[email protected]>

pracucci

Given the value the per-tenant metric can assume would be only 1 or 0, I'm wondering if we could keep the metric as is (no change) and add a new metric (with a better naming) to cover the new use case.

gotjosh · 2020-08-03T14:18:20Z

Given the value the per-tenant metric can assume would be only 1 or 0

There's an extra scenario that this serves as a precedent. If you provide an invalid config whilst you have a valid config, Alertmanager will revert back to the last working configuration.

cortex/pkg/alertmanager/multitenant.go

Lines 416 to 423 in 6db67a4

    
           userAmConfig, err = amconfig.Load(cfg.RawConfig) 
        
           if err != nil && hasExisting { 
        
           	// XXX: This means that if a user has a working configuration and 
        
           	// they submit a broken one, we'll keep processing the last known 
        
           	// working configuration, and they'll never know. 
        
           	// TODO: Provide a way of communicating this to the user and for removing 
        
           	// Alertmanager instances. 
        
           	return fmt.Errorf("invalid Cortex configuration for %v: %v", cfg.User, err)

By having the extra-label adding a way to signal this becomes trivial and uniform from a UX perspective (anything other than cortex_alertmanager_configs{status="valid"} == 1 is a failure).

With that said, I'm not fuzzed (as the above would just have to be a new metric then?) - Happy to go with what you think would align best with what we do in other parts of Cortex.

gotjosh · 2020-08-03T15:49:13Z

Discussed offline, where Marco suggested that prometheus_config_last_reload_successful looks pretty similar to what we want to do and thus should be a separate metric.

gotjosh · 2020-08-03T15:49:39Z

We've settled on cortex_alertmanager_invalid_config

Signed-off-by: gotjosh <[email protected]>

gotjosh · 2020-08-03T18:46:12Z

integration/alertmanager_test.go

@@ -79,7 +80,8 @@ func TestAlertmanagerStoreAPI(t *testing.T) {
 	err = c.SetAlertmanagerConfig(context.Background(), cortexAlertmanagerUserConfigYaml, map[string]string{})
 	require.NoError(t, err)

-	require.NoError(t, am.WaitSumMetrics(e2e.Equals(1), "cortex_alertmanager_configs"))
+	time.Sleep(2 * time.Second)


I solved the CI failure using a quick hack, but I dislike Sleep is tests as it a general source of flaky tests. The problem here is the metric does not get initially registered (given it is per tenant) until much later.

A solution I can see would be to create a new method (or argument to WaitSumMetrics) that waits for a metric to appear instead of bailing out if we don't see the metric on the initial metrics page load. As I can see this being useful in other areas.

Please let me know and I'll be happy to make the change.

Let's work on it in a separate PR. Could you open an issue, please? I have this pending PR #2522 which could be used as a baseline to add a functional option to enable the logic you suggest.

Thanks marco, filed in #2975

pracucci

LGTM, thanks!

gouthamve

LGTM with a suggestion!

pkg/alertmanager/multitenant.go

Signed-off-by: gotjosh <[email protected]>

jtlisi

LGTM

pull-request-size bot added the size/M label Jul 30, 2020

gotjosh commented Jul 30, 2020

View reviewed changes

pkg/alertmanager/multitenant.go Show resolved Hide resolved

jtlisi reviewed Jul 30, 2020

View reviewed changes

pkg/alertmanager/multitenant.go Show resolved Hide resolved

gotjosh added 3 commits August 3, 2020 09:34

AM Metric: Add tenant label to valid/invalid configs

979f911

Gives us a way to know whenever a tenant has an invalid configuration in place. Signed-off-by: gotjosh <[email protected]>

Remove the metrics when we have no active config

6896639

Signed-off-by: gotjosh <[email protected]>

These metrics no longer get initialised on starting up the component

a72a5e1

Signed-off-by: gotjosh <[email protected]>

gotjosh force-pushed the invalid-configs-metrics branch from f0d9580 to a72a5e1 Compare August 3, 2020 12:26

pracucci reviewed Aug 3, 2020

View reviewed changes

Address review feedback

c5cc415

Signed-off-by: gotjosh <[email protected]>

gotjosh force-pushed the invalid-configs-metrics branch from 4ec50cf to c5cc415 Compare August 3, 2020 18:41

gotjosh commented Aug 3, 2020

View reviewed changes

pracucci approved these changes Aug 4, 2020

View reviewed changes

gouthamve approved these changes Aug 4, 2020

View reviewed changes

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

s/alertmanager_invalid_config/alertmanager_config_invalid

5dd75f7

Signed-off-by: gotjosh <[email protected]>

jtlisi approved these changes Aug 4, 2020

View reviewed changes

gouthamve merged commit b404a95 into cortexproject:master Aug 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

AM Metric: Add tenant label to valid/invalid configs #2960

AM Metric: Add tenant label to valid/invalid configs #2960

Uh oh!

gotjosh commented Jul 30, 2020 •

edited

Loading

Uh oh!

Uh oh!

jtlisi left a comment •

edited

Loading

Uh oh!

Uh oh!

gotjosh commented Jul 30, 2020

Uh oh!

pracucci left a comment

Uh oh!

gotjosh commented Aug 3, 2020

Uh oh!

gotjosh commented Aug 3, 2020

Uh oh!

gotjosh commented Aug 3, 2020

Uh oh!

gotjosh Aug 3, 2020 •

edited

Loading

Uh oh!

pracucci Aug 4, 2020

Uh oh!

gotjosh Aug 4, 2020

Uh oh!

pracucci left a comment

Uh oh!

gouthamve left a comment

Uh oh!

Uh oh!

jtlisi left a comment

Uh oh!

Uh oh!

AM Metric: Add tenant label to valid/invalid configs #2960

AM Metric: Add tenant label to valid/invalid configs #2960

Uh oh!

Conversation

gotjosh commented Jul 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

jtlisi 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!

gotjosh commented Jul 30, 2020

Uh oh!

pracucci left a comment

Choose a reason for hiding this comment

Uh oh!

gotjosh commented Aug 3, 2020

Uh oh!

gotjosh commented Aug 3, 2020

Uh oh!

gotjosh commented Aug 3, 2020

Uh oh!

gotjosh Aug 3, 2020 • 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!

pracucci Aug 4, 2020

Choose a reason for hiding this comment

Uh oh!

gotjosh Aug 4, 2020

Choose a reason for hiding this comment

Uh oh!

pracucci left a comment

Choose a reason for hiding this comment

Uh oh!

gouthamve left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jtlisi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gotjosh commented Jul 30, 2020 •

edited

Loading

jtlisi left a comment •

edited

Loading

gotjosh Aug 3, 2020 •

edited

Loading