add reason label to the numTotalFailedNotifications metric #3094

qinxx108 · 2022-10-07T17:31:36Z

This is to follow up the discussion on issue: #2927

Instead of creating a new metric, we will add the status code label to the numTotalFailedNotifications metric. The default status code is going to be "5xx" and if the integration receivers defined the NewErrorWithStatusCode, we will translate the returned status code and put that in the label override

rohang98 · 2022-10-12T16:12:21Z

notify/notify.go

 	}
+	r.metrics.numTotalFailedNotifications.WithLabelValues(r.integration.Name(), statusCodeCategory).Inc()


Shouldn't this increment still be inside the if statement?

qinxx108 · 2022-10-13T15:57:37Z

@gotjosh @roidelapluie
Hi Josh and Julien. This is to follow up with #2927
To address the comments to put the status category into the label

Could you please take a look to see if this look good to you?

bwplotka

Some ideas to improve this code, but generally looks amazing. Thanks for coming to Prometheus ContribFest and this contribution!

bwplotka · 2022-10-27T19:12:33Z

notify/notify.go

@@ -262,7 +265,7 @@ func NewMetrics(r prometheus.Registerer) *Metrics {
 			Namespace: "alertmanager",
 			Name:      "notifications_failed_total",
 			Help:      "The total number of failed notifications.",
-		}, []string{"integration"}),
+		}, []string{"integration", "statusCode"}),


Let's be concise. Perhaps code makes more sense?

bwplotka · 2022-10-27T19:14:40Z

notify/util.go

+		return "5xx", nil
+	}
+
+	return "", fmt.Errorf("unexpected status code %v", statusCode)


Do we really want to fail some application logic if status code is unknown? I would just return unknown string here.

In the Notify function which call this i did not failed the logic, instead use the default if the status code is unexpected. I agreed empty string is less explained itself. changed to unknown

bwplotka · 2022-10-27T19:15:13Z

notify/notify.go

@@ -293,7 +296,7 @@ func NewMetrics(r prometheus.Registerer) *Metrics {
 		"telegram",
 	} {
 		m.numNotifications.WithLabelValues(integration)
-		m.numTotalFailedNotifications.WithLabelValues(integration)
+		m.numTotalFailedNotifications.WithLabelValues(integration, "")


This does not make a lot of sense... I think we want to populate here counters with each possible 3 status codes: 4xx, 5xx and unknown.

bwplotka · 2022-11-03T21:42:36Z

notify/util.go

+// getFailureStatusCodeCategory return the status code category for failure request
+// the status starts with 4 will return 4xx and starts with 5 will return 5xx
+// other than 4xx and 5xx input status will return an error.
+func getFailureStatusCodeCategory(statusCode int) (string, error) {


Based on how we use this, I feel we should not return error here, but just return default value, WDYT? Would make more clean and simple.

I kind of wanting to return an error so that the user of the function has knowledge about if the status code has been successfully translated. Otherwise they will have to rely on comparing if the result = failureUnknownCategoryCode if failureUnknownCategoryCode is not the ideal result for them in error case. WDYT?

Hm, well there are no users of this function yet, and as YAGNI goes, we should assume there will be none. I would focus on our usage:

result, interErr := getFailureStatusCodeCategory(e.StatusCode) if interErr == nil { statusCodeCategory = result }

We don't want to know IF the status was translated correctly, but rather we want defaultStatusCodeCategory in this case. So I would just return defaultStatusCodeCategory

Also, now when we talk about it, util is not a great place for it - it's pretty generic - we want it in util if we have more than one usage of it. Given the function is very small and used only in one place, why not:

Moving this function next to notify.go file?
OR

Copy the content of this function to caller place? It's might be too shallow for a function, but up to you (:

I agree with Bartek, the function should return 4xx if [400,550), 5xx if [500,599) or fallback toother.

bwplotka

LGTM just one nit (:

bwplotka

Last nits, thanks!

bwplotka · 2022-11-14T14:58:04Z

notify/util.go

 // UserAgentHeader is the default User-Agent for notification requests
 var UserAgentHeader = fmt.Sprintf("Alertmanager/%s", version.Version)

+// PossibleFailureStatusCategory is a list of possible failure status code category
+var PossibleFailureStatusCategory = []string{failure4xxCategoryCode, failure5xxCategoryCode, failureUnknownCategoryCode}


Why it's exported? Can we make it private and move it next to notify.go?

moved to notify.go

bwplotka · 2022-11-14T14:59:22Z

notify/util.go

+// getFailureStatusCodeCategory return the status code category for failure request
+// the status starts with 4 will return 4xx and starts with 5 will return 5xx
+// other than 4xx and 5xx input status will return an error.
+func getFailureStatusCodeCategory(statusCode int) (string, error) {


Hm, well there are no users of this function yet, and as YAGNI goes, we should assume there will be none. I would focus on our usage:

result, interErr := getFailureStatusCodeCategory(e.StatusCode) if interErr == nil { statusCodeCategory = result }

We don't want to know IF the status was translated correctly, but rather we want defaultStatusCodeCategory in this case. So I would just return defaultStatusCodeCategory

Also, now when we talk about it, util is not a great place for it - it's pretty generic - we want it in util if we have more than one usage of it. Given the function is very small and used only in one place, why not:

Moving this function next to notify.go file?
OR

Copy the content of this function to caller place? It's might be too shallow for a function, but up to you (:

simonpasquier · 2022-11-15T14:50:36Z

notify/notify.go

 	if err != nil {
-		r.metrics.numTotalFailedNotifications.WithLabelValues(r.integration.Name()).Inc()
+		if e, ok := errors.Cause(err).(*ErrorWithStatusCode); ok {


not sure why we need to call Cause() here

When the error returned from

r.exec(ctx, l, alerts...)

The error already wrapped once with errors.Wrapf

simonpasquier · 2022-11-15T14:56:10Z

notify/util.go

+// getFailureStatusCodeCategory return the status code category for failure request
+// the status starts with 4 will return 4xx and starts with 5 will return 5xx
+// other than 4xx and 5xx input status will return an error.
+func getFailureStatusCodeCategory(statusCode int) (string, error) {


I agree with Bartek, the function should return 4xx if [400,550), 5xx if [500,599) or fallback toother.

simonpasquier · 2022-11-15T15:04:14Z

notify/notify.go

@@ -662,8 +668,16 @@ func NewRetryStage(i Integration, groupName string, metrics *Metrics) *RetryStag
 func (r RetryStage) Exec(ctx context.Context, l log.Logger, alerts ...*types.Alert) (context.Context, []*types.Alert, error) {
 	r.metrics.numNotifications.WithLabelValues(r.integration.Name()).Inc()
 	ctx, alerts, err := r.exec(ctx, l, alerts...)
+
+	statusCodeCategory := defaultStatusCodeCategory


For integrations that don't implement ErrorWithStatusCode, this could be inaccurate (e.g. the receiving side might have returned a 400 status code). It's also not correct if Alertmanager couldn't even send the notification (template error for instance). And the email notifier is another case that doesn't fit here.

I wonder if we shouldn't abstract away from the HTTP situation and consider a more generic label like reason with a handful number of possible values:

server error (e.g. 5xx)

client error (e.g. 4xx)

authentication error

template

other

How about

DestinationServerError

DestinationClientError

Fatal (all code error)

Template

Other

authentication error will belong to DestinationClientError

And in case the integration doesn't implement the ErrorWithStatusCode, the category will belong to Other

WDYT?

@simonpasquier would this work?

I think that we can start with:

ClientError (typically templating errors, 4xx responses)

ServerError (typically 5xx responses from the server)

Other (e.g. receivers for which we haven't implemented ClientError/ServerError logic yet)

And we can always add more in the future where we see fit (e.g. maybe we'll want to distinguish authn failures, serialization errors, templating errors, ...)

gotjosh

LGTM!

All my comments are nits - but please let's address them, and we're ready to merge. Assuming that at this point, both @simonpasquier and @bwplotka's comments are resolved and they have nothing else to add.

notify/util.go

gotjosh · 2023-01-27T21:38:37Z

notify/notify.go

@@ -662,8 +665,13 @@ func NewRetryStage(i Integration, groupName string, metrics *Metrics) *RetryStag
 func (r RetryStage) Exec(ctx context.Context, l log.Logger, alerts ...*types.Alert) (context.Context, []*types.Alert, error) {
 	r.metrics.numNotifications.WithLabelValues(r.integration.Name()).Inc()
 	ctx, alerts, err := r.exec(ctx, l, alerts...)
+
+	failureReason := DefaultReason.String()


I've filed #3231 so that we can do the same for all other notifiers.

Signed-off-by: Yijie Qin <[email protected]>

… when unknown status code Signed-off-by: Yijie Qin <[email protected]>

Signed-off-by: Yijie Qin <[email protected]>

gotjosh · 2023-02-03T12:08:00Z

Build is broken right now but tests pass, I'll assume everything is proper order here until we get that fixed that merge this.

gotjosh · 2023-02-03T12:09:41Z

Thanks very much for your contribution!

…etheus#3094) * add reason label to the numTotalFailedNotifications metric Signed-off-by: Yijie Qin <[email protected]>

qinxx108 force-pushed the 4xx-error-for-notification-failure branch 2 times, most recently from 106cf64 to b99dad3 Compare October 12, 2022 02:39

krishnateja325 approved these changes Oct 12, 2022

View reviewed changes

rohang98 reviewed Oct 12, 2022

View reviewed changes

qinxx108 force-pushed the 4xx-error-for-notification-failure branch from b99dad3 to eb74eb5 Compare October 12, 2022 19:34

rohang98 approved these changes Oct 12, 2022

View reviewed changes

bwplotka approved these changes Oct 27, 2022

View reviewed changes

qinxx108 force-pushed the 4xx-error-for-notification-failure branch 2 times, most recently from edf6a31 to 6ce9839 Compare November 3, 2022 17:20

bwplotka reviewed Nov 3, 2022

View reviewed changes

bwplotka reviewed Nov 14, 2022

View reviewed changes

simonpasquier reviewed Nov 15, 2022

View reviewed changes

qinxx108 force-pushed the 4xx-error-for-notification-failure branch 2 times, most recently from 8b09c90 to 73c8b96 Compare November 16, 2022 19:07

qinxx108 force-pushed the 4xx-error-for-notification-failure branch 6 times, most recently from 0aac23f to 8940bb2 Compare January 12, 2023 17:37

qinxx108 force-pushed the 4xx-error-for-notification-failure branch from 8940bb2 to 205b802 Compare January 20, 2023 05:54

gotjosh approved these changes Jan 27, 2023

View reviewed changes

gotjosh mentioned this pull request Jan 27, 2023

Include status code label to the numTotalFailedNotifications for all notifiers #3231

Open

12 tasks

qinxx108 force-pushed the 4xx-error-for-notification-failure branch from 00b8741 to 25b9227 Compare February 2, 2023 18:39

qinxx108 added 4 commits February 2, 2023 13:41

add status code label to the numTotalFailedNotifications metric

d01896a

Signed-off-by: Yijie Qin <[email protected]>

getFailureStatusCodeCategory to return error rather than empty string…

1fdaa83

… when unknown status code Signed-off-by: Yijie Qin <[email protected]>

move the metric inc

0e86d0f

Signed-off-by: Yijie Qin <[email protected]>

fix comments

10e4d1c

Signed-off-by: Yijie Qin <[email protected]>

qinxx108 added 5 commits February 2, 2023 13:41

remove unnessary line

1ef398f

Signed-off-by: Yijie Qin <[email protected]>

fix comments

00c6676

Signed-off-by: Yijie Qin <[email protected]>

use status reason rather than code

b364f98

Signed-off-by: Yijie Qin <[email protected]>

use iota for reason type

b28f771

Signed-off-by: Yijie Qin <[email protected]>

address comment

3f5d3a2

Signed-off-by: Yijie Qin <[email protected]>

qinxx108 force-pushed the 4xx-error-for-notification-failure branch from 25b9227 to 3f5d3a2 Compare February 2, 2023 18:41

Merge branch 'prometheus:main' into 4xx-error-for-notification-failure

eaf1864

gotjosh enabled auto-merge (squash) February 3, 2023 12:08

gotjosh disabled auto-merge February 3, 2023 12:09

gotjosh merged commit 7923bc5 into prometheus:main Feb 3, 2023

gotjosh changed the title ~~add status code label to the numTotalFailedNotifications metric~~ add reason label to the numTotalFailedNotifications metric Feb 3, 2023

gotjosh mentioned this pull request Feb 24, 2023

Creating new metric for filtering 4xx errors #2927

Closed

krishnateja325 mentioned this pull request Apr 19, 2023

Update prometheus/alertmanager to version v0.25.1-0.20230505130626-263ca5c9438e cortexproject/cortex#5276

Merged

2 tasks

This was referenced Jun 13, 2023

grab reason label from cortex_alertmanager_notifications_failed_total metric cortexproject/cortex#5403

Closed

AlertManager: include reason label in cortex_alertmanager_notifications_failed_total cortexproject/cortex#5409

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add reason label to the numTotalFailedNotifications metric #3094

add reason label to the numTotalFailedNotifications metric #3094

qinxx108 commented Oct 7, 2022

rohang98 Oct 12, 2022

qinxx108 Oct 12, 2022

qinxx108 commented Oct 13, 2022 •

edited

Loading

bwplotka left a comment •

edited

Loading

bwplotka Oct 27, 2022

qinxx108 Oct 31, 2022

bwplotka Oct 27, 2022

qinxx108 Nov 1, 2022

bwplotka Oct 27, 2022

qinxx108 Nov 1, 2022

bwplotka Nov 3, 2022

qinxx108 Nov 9, 2022

bwplotka Nov 14, 2022

simonpasquier Nov 15, 2022

qinxx108 Nov 16, 2022

bwplotka left a comment

bwplotka left a comment

bwplotka Nov 14, 2022

qinxx108 Nov 16, 2022

bwplotka Nov 14, 2022

simonpasquier Nov 15, 2022

qinxx108 Nov 16, 2022

simonpasquier Nov 15, 2022

simonpasquier Nov 15, 2022

qinxx108 Nov 16, 2022

jeromeinsf Dec 29, 2022

simonpasquier Jan 6, 2023

qinxx108 Jan 10, 2023

gotjosh left a comment •

edited

Loading

gotjosh Jan 27, 2023

gotjosh commented Feb 3, 2023

gotjosh commented Feb 3, 2023

		}
		r.metrics.numTotalFailedNotifications.WithLabelValues(r.integration.Name(), statusCodeCategory).Inc()

add reason label to the numTotalFailedNotifications metric #3094

add reason label to the numTotalFailedNotifications metric #3094

Conversation

qinxx108 commented Oct 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qinxx108 commented Oct 13, 2022 • edited Loading

bwplotka left a comment • edited Loading

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

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

bwplotka left a comment

Choose a reason for hiding this comment

bwplotka 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

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 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gotjosh commented Feb 3, 2023

gotjosh commented Feb 3, 2023

qinxx108 commented Oct 13, 2022 •

edited

Loading

bwplotka left a comment •

edited

Loading

gotjosh left a comment •

edited

Loading