Upgrade upstream Prometheus: add keep_firing_for support to alerting rules and track 499 on canceled requests #4099

pracucci · 2023-01-27T13:24:48Z

What this PR does

This PR updates the upstream Prometheus. The user facing changes are:

Add keep_firing_for support to alerting rules
Track 499 in cortex_request_duration_seconds_count for canceled queries

Note to reviewers

As a counterproof of the new tests I've added, I've tried to modify the labels.StableHash() function and all new tests failed.
I manually tested the keep_firing_for and looks working as expected. I've also manually tested the API endpoints /api/v1/rules and /api/v1/alerts: they both looks working as Prometheus ones.
I manually tested the status_code used to track canceled queries and this PR fixes Querier tracks cortex_request_duration_seconds_count tracks status_code=503 on "context canceled" #4071

What has changed in updated vendored libs

Alertmanager changes:

Migrated to github.com/hashicorp/golang-lru/v2
PushoverConfig introduced UserKeyFile and TokenFile
Refactored nflog.Log options and maintenance (commit)

-       github.com/prometheus/alertmanager v0.25.0
+       github.com/prometheus/alertmanager v0.25.1-0.20230119163903-f59460bfd4bf

Prometheus changes:

Added KeepFiringFor to Rule and AlertingRule, and KeepFiringSince to Alert. The 'keep_firing_for' field specifies the minimum amount of time that an alert should remain firing, even if the expression does not return any results.
499 on canceled request
TSDB in our fork now requires ShardFunc
More changes to native histograms (not yet supported by Mimir main, so didn't review changes in depth)

-replace github.com/prometheus/prometheus => github.com/grafana/mimir-prometheus v0.0.0-20230125082610-38af345deab1
+replace github.com/prometheus/prometheus => github.com/grafana/mimir-prometheus v0.0.0-20230127083412-f1986bc584b6

Checked the diff and AWS SDK just added new endpoints:

-       github.com/aws/aws-sdk-go v1.44.159
+       github.com/aws/aws-sdk-go v1.44.187

Which issue(s) this PR fixes or relates to

Fixes #4071

Checklist

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

pracucci · 2023-01-27T13:25:45Z

Makefile

@@ -327,7 +327,14 @@ lint: check-makefiles
 	# Note that we don't automatically suggest replacing sort.Float64s() with slices.Sort() as the documentation for slices.Sort()
 	# at the time of writing warns that slices.Sort() may not correctly handle NaN values.
 	faillint -paths \
-		"sort.{Strings,Ints}=golang.org/x/exp/slices.Sort" \
+		"sort.{Strings,Ints}=golang.org/x/exp/slices.{Sort}" \


Note to reviewers: it wasn't working as expected. Functions must be wrapped by {}.

pracucci · 2023-01-27T13:26:34Z

pkg/alertmanager/alertmanager.go

@@ -188,19 +188,25 @@ func New(cfg *Config, reg *prometheus.Registry) (*Alertmanager, error) {
 	am.state = newReplicatedStates(cfg.UserID, cfg.ReplicationFactor, cfg.Replicator, cfg.Store, am.logger, am.registry)
 	am.persister = newStatePersister(cfg.PersisterConfig, cfg.UserID, am.state, cfg.Store, am.logger, am.registry)

-	am.wg.Add(1)


Note to reviewers: moved below because this was used to track the maintenance goroutine which we're not responsible to start.

pracucci · 2023-01-27T13:27:08Z

pkg/alertmanager/alertmanager.go

-	am.nflog, err = nflog.New(
-		nflog.WithRetention(cfg.Retention),
-		nflog.WithSnapshot(filepath.Join(cfg.TenantDataDir, notificationLogSnapshot)),
-		nflog.WithMaintenance(maintenancePeriod, am.stop, am.wg.Done, nil),


Note to reviewers: this option doesn't exist anymore because now you're expected to start the maintenance goroutine (done below).

pkg/storage/sharding/label_test.go

pracucci · 2023-01-27T13:39:39Z

pkg/ruler/api.go

+	Labels          labels.Labels `json:"labels"`
+	Annotations     labels.Labels `json:"annotations"`
+	State           string        `json:"state"`
+	ActiveAt        *time.Time    `json:"activeAt,omitempty"`


Note to reviewers: added omitempty to keep parity with Prometheus.

krajorama

Approved with comments.
Is there any chance to get the shardfunc onto Prometheus main? I feel we need to review any update to tsdb from now to check if it hardcodes using labels.Hash

krajorama · 2023-01-27T14:08:28Z

pkg/ruler/ruler.proto

@@ -69,4 +69,6 @@ message AlertStateDesc {
      [(gogoproto.nullable) = false, (gogoproto.stdtime) = true];
  google.protobuf.Timestamp valid_until = 9
      [(gogoproto.nullable) = false, (gogoproto.stdtime) = true];
+  google.protobuf.Timestamp keep_firing_since = 10


[nit] not sure if it's a problem but the generated definition does not have the omittempty flag in the json tag, due nullable=false I think. For both this and active_at

We don't use JSON here. The exported JSON is based on the other manually defined struct.

the other manually defined struct

This is what we expose through the API:
https://github.com/grafana/mimir/pull/4099/files#diff-eee58232370c5c7c01c7f2ab7f27cbc4371ca8d99eaf5b180e69d8f1ac57f2bbR53

pracucci · 2023-01-27T14:27:47Z

Is there any chance to get the shardfunc onto Prometheus main? I feel we need to review any update to tsdb from now to check if it hardcodes using labels.Hash

We should talk to @bboreham about it.

pkg/alertmanager/api.go

pkg/storage/sharding/label.go

pkg/storage/sharding/label_test.go

pkg/storage/sharding/label.go

pracucci · 2023-01-27T15:38:18Z

Comment on Slack from @colega:

I think that we need a test in compactor and other features that rely on the stability of the labels hash that checks that given list of series falls into the expected list of shards.

I remember we have for distributors but I don't remember for compactor. I will check it.

gotjosh

LGTM

Thanks for doing the lord's work on implementing keep_firing_for so quickly.

pkg/alertmanager/alertmanager.go

pkg/ruler/api.go

pracucci · 2023-01-30T11:26:29Z

I think that we need a test in compactor and other features that rely on the stability of the labels hash that checks that given list of series falls into the expected list of shards.

I remember we have for distributors but I don't remember for compactor. I will check it.

I confirm we have one for distributors:

mimir/pkg/distributor/distributor_test.go

Line 1306 in a9a30e0

    
           func TestDistributor_Push_ShouldGuaranteeShardingTokenConsistencyOverTheTime(t *testing.T) {

colega

LGTM, thank you for the tests.

Please fix this comment before merging:

mimir/pkg/querier/error_translate_queryable.go

Lines 25 to 26 in 304b184

    
           //	promql.ErrQueryCanceled, mapped to 503 
        
           //	promql.ErrQueryTimeout, mapped to 503

CHANGELOG.md

…rules and track 499 on canceled requests Signed-off-by: Marco Pracucci <marco@pracucci.com>

Signed-off-by: Marco Pracucci <marco@pracucci.com>

pracucci commented Jan 27, 2023

View reviewed changes

pkg/storage/sharding/label_test.go Outdated Show resolved Hide resolved

pracucci commented Jan 27, 2023

View reviewed changes

krajorama approved these changes Jan 27, 2023

View reviewed changes

pracucci force-pushed the update-upstream-prometheus branch from a53745f to c03d3b1 Compare January 27, 2023 14:39

pracucci marked this pull request as ready for review January 27, 2023 14:47

pracucci requested review from a team as code owners January 27, 2023 14:47

colega reviewed Jan 27, 2023

View reviewed changes

gotjosh approved these changes Jan 27, 2023

View reviewed changes

pkg/alertmanager/alertmanager.go Show resolved Hide resolved

pkg/ruler/api.go Outdated Show resolved Hide resolved

pracucci marked this pull request as draft January 30, 2023 11:05

pracucci force-pushed the update-upstream-prometheus branch 2 times, most recently from 38b7586 to 7647c66 Compare January 30, 2023 11:18

pracucci force-pushed the update-upstream-prometheus branch 5 times, most recently from ebfce8b to 304b184 Compare January 30, 2023 15:10

pracucci marked this pull request as ready for review January 30, 2023 15:13

pracucci requested review from colega and krajorama January 30, 2023 15:14

colega approved these changes Jan 31, 2023

View reviewed changes

dimitarvdimitrov reviewed Jan 31, 2023

View reviewed changes

CHANGELOG.md Show resolved Hide resolved

pracucci added 2 commits January 31, 2023 16:13

Upgrade upstream Prometheus: add keep_firing_for support to alerting …

95c93c1

…rules and track 499 on canceled requests Signed-off-by: Marco Pracucci <marco@pracucci.com>

Addressed review comments

f0978fc

Signed-off-by: Marco Pracucci <marco@pracucci.com>

pracucci force-pushed the update-upstream-prometheus branch from 304b184 to f0978fc Compare January 31, 2023 15:23

pracucci enabled auto-merge (squash) January 31, 2023 15:25

Fixed syntax after rebase

9b5f084

Signed-off-by: Marco Pracucci <marco@pracucci.com>

pracucci merged commit 5ac2d26 into main Jan 31, 2023

pracucci deleted the update-upstream-prometheus branch January 31, 2023 15:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade upstream Prometheus: add keep_firing_for support to alerting rules and track 499 on canceled requests #4099

Upgrade upstream Prometheus: add keep_firing_for support to alerting rules and track 499 on canceled requests #4099

pracucci commented Jan 27, 2023 •

edited

Loading

pracucci Jan 27, 2023

pracucci Jan 27, 2023

pracucci Jan 27, 2023 •

edited

Loading

pracucci Jan 27, 2023

krajorama left a comment

krajorama Jan 27, 2023

pracucci Jan 27, 2023

pracucci Jan 27, 2023

pracucci commented Jan 27, 2023

pracucci commented Jan 27, 2023

gotjosh left a comment

pracucci commented Jan 30, 2023

colega left a comment

	// promql.ErrQueryCanceled, mapped to 503
	// promql.ErrQueryTimeout, mapped to 503

Upgrade upstream Prometheus: add keep_firing_for support to alerting rules and track 499 on canceled requests #4099

Upgrade upstream Prometheus: add keep_firing_for support to alerting rules and track 499 on canceled requests #4099

Conversation

pracucci commented Jan 27, 2023 • edited Loading

What this PR does

Note to reviewers

What has changed in updated vendored libs

Which issue(s) this PR fixes or relates to

Checklist

pracucci Jan 27, 2023

Choose a reason for hiding this comment

pracucci Jan 27, 2023

Choose a reason for hiding this comment

pracucci Jan 27, 2023 • edited Loading

Choose a reason for hiding this comment

pracucci Jan 27, 2023

Choose a reason for hiding this comment

krajorama left a comment

Choose a reason for hiding this comment

krajorama Jan 27, 2023

Choose a reason for hiding this comment

pracucci Jan 27, 2023

Choose a reason for hiding this comment

pracucci Jan 27, 2023

Choose a reason for hiding this comment

pracucci commented Jan 27, 2023

pracucci commented Jan 27, 2023

gotjosh left a comment

Choose a reason for hiding this comment

pracucci commented Jan 30, 2023

colega left a comment

Choose a reason for hiding this comment

pracucci commented Jan 27, 2023 •

edited

Loading

pracucci Jan 27, 2023 •

edited

Loading