Update vendored mimir-prometheus #7057

pracucci · 2024-01-05T16:07:30Z

What this PR does

In this PR I'm upgrading the vendored mimir-prometheus and their deps.

Prometheus changes

The analysis of all Prometheus changes can be found here: grafana/mimir-prometheus#580

In particular, what's affecting Mimir:

MetricType moved from github.com/prometheus/prometheus/model/textparse to github.com/prometheus/common/model
Histogram.ToFloat() now takes in input *FloatHistogram to eventually reuse it (if nil the old behaviour is preserved). In this PR I'm always passing nil to try not burying any logic change. As separate work we can investigate if we would get any perf benefit recycling the FloatHistogram.
discovery.NewManager() now takes in input the metrics Registerer instead of registering metrics on the global one. I've changed Mimir code to pass the per-tenant registerer, so metrics will be renamed and will have the user label attached (see CHANGELOG for the list of renamed metrics). Similar change for refresh.NewDiscovery(). It's important to note that these metrics are only exposed when ruler is configured to discover Alertmanager via service discovery, which is not how we run Mimir in Jsonnet, Helm or at Grafana Labs. None of our dashboards / alerts queries such metrics.
Appender interface added AppendCTZeroSample(). It's still a work in progress in Prometheus. We can defer the actual support for now.
Prometheus moved from github.com/pkg/errors to the standard errors package. This means that github.com/pkg/errors.Cause() will not work anymore on errors wrapped by Prometheus. To avoid subtle bugs, I've removed the usage of Cause() on errors returned by Prometheus TSDB and PromQL. There are few other places where we're still using Cause(), but these are in few selected places where there shouldn't be any Prometheus error. In a separate PR, I will propose to get rid of errors.Cause() completely (but I prefer to keep it separated, to make the review process safer).
There are 3 new PromQL functions. Currently they're all experimental functions and in Prometheus they need to be enabled via a feature flag:
- mad_over_time: the function is shardable, like other _over_time() functions (conceptually it's not different than quantile_over_time()).
- sort_by_label and sort_by_label_desc: like sort() and sort_desc(), these functions are currently not safe to be parallelised by query sharding, so I've added them to the exclusion list.
- I've introduced -querier.promql-experimental-functions-enabled CLI flag (and respective YAML config option) to allow to enable experimental functions (disabled by default).

Other vendored libs changes

-       go.opentelemetry.io/otel v1.19.0
-       go.opentelemetry.io/otel/trace v1.19.0
+       go.opentelemetry.io/otel v1.21.0
+       go.opentelemetry.io/otel/trace v1.21.0

Cheched the release notes. Many changes on tracing, few on metrics. Since many of such changes are related to packages we don't import, I've checked the actual code diff (it's small).

The change in this PR affects us. The PR description explains the rationale, but basically any OTel tracer implementation needs to define a fallback for non implemented functions (I've picked the noop one, we aim to implement all functions anyway).

-       github.com/hashicorp/consul/api v1.25.1 // indirect
+       github.com/hashicorp/consul/api v1.26.1 // indirect

Checked the diff and looks just adding new features:

-       github.com/miekg/dns v1.1.56 // indirect
+       github.com/miekg/dns v1.1.57 // indirect

Checked the diff and looks mostly code cleanup:

Other deps we don't use directly

github.com/felixge/httpsnoop
github.com/fsnotify/fsnotify
github.com/go-logr/logr

Which issue(s) this PR fixes or relates to

N/A

Checklist

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

pracucci · 2024-01-05T17:35:30Z

pkg/querier/engine/config.go

@@ -60,5 +64,5 @@ func NewPromQLEngineOptions(cfg Config, activityTracker *activitytracker.Activit
 		NoStepSubqueryIntervalFn: func(int64) int64 {
 			return cfg.DefaultEvaluationInterval.Milliseconds()
 		},
-	}
+	}, cfg.PromQLExperimentalFunctionsEnabled


Note to reviewers: the reason why I propose to return cfg.PromQLExperimentalFunctionsEnabled from NewPromQLEngineOptions() even if it's based on the input cfg is to have a stronger way to signal the callers to also set the PromQL parser's option to enable the experimental functions.

pkg/ruler/compat.go

aknuds1 · 2024-01-08T17:30:16Z

I've introduced -querier.promql-experimental-functions-enabled CLI flag (and respective YAML config option) to allow to enable experimental functions (disabled by default).

Should this be in the changelog?

aknuds1

LGTM, just wondering about the following:

I've introduced -querier.promql-experimental-functions-enabled CLI flag (and respective YAML config option) to allow to enable experimental functions (disabled by default).

Should this be in the changelog?

pstibrany

Changes lgtm. Agree with Arve that we should include new option in the changelog. I think we should also document which functions it affects.

(Personal note: It's funny to see sort by labels function added, as the PR implementing this function was based on my old PR from 7 years ago. Seeing it now in Mimir, I'm not sure it's a good fit, but here we are... 😂).

pstibrany · 2024-01-09T15:00:08Z

pkg/querier/engine/config.go

@@ -44,10 +46,12 @@ func (cfg *Config) RegisterFlags(f *flag.FlagSet) {
 	f.IntVar(&cfg.MaxSamples, "querier.max-samples", 50e6, sharedWithQueryFrontend("Maximum number of samples a single query can load into memory."))
 	f.DurationVar(&cfg.DefaultEvaluationInterval, "querier.default-evaluation-interval", time.Minute, sharedWithQueryFrontend("The default evaluation interval or step size for subqueries."))
 	f.DurationVar(&cfg.LookbackDelta, "querier.lookback-delta", 5*time.Minute, sharedWithQueryFrontend("Time since the last sample after which a time series is considered stale and ignored by expression evaluations."))
+	f.BoolVar(&cfg.PromQLExperimentalFunctionsEnabled, "querier.promql-experimental-functions-enabled", false, sharedWithQueryFrontend("True to enable support for experimental PromQL functions."))


[nit] It can also be set without passing =true value, simplify wording:

Suggested change

f.BoolVar(&cfg.PromQLExperimentalFunctionsEnabled, "querier.promql-experimental-functions-enabled", false, sharedWithQueryFrontend("True to enable support for experimental PromQL functions."))

f.BoolVar(&cfg.PromQLExperimentalFunctionsEnabled, "querier.promql-experimental-functions-enabled", false, sharedWithQueryFrontend("Enable experimental PromQL functions."))

Shall we include list of experimental functions? How does user know when to use this?

Simplified CLI description in 35db456.

Shall we include list of experimental functions? How does user know when to use this?

Yes, it's in about-versioning.md and I've just added to CHANGELOG too.

dimitarvdimitrov

We should document the behaviour for the CT zero samples (and implement them eventually because they look cool)

I also agree with Arve about the changelog :)

dimitarvdimitrov

LGTM, thank you for carrying this out

(Assuming you address Peter's and Arve's comments)

Added PR number to CHANGELOG Do not use errors.Cause() on errors returned by Prometheus Fixed TestConfig_TranslatesToPrometheusTargetGroup Fixed TestConfig_ConstructsLookupNamesCorrectly Updated query sharding with new PromQL experimental functions Added -querier.promql-experimental-functions-enabled support Fix linter issues Added new CLI flag to list of experimental features Signed-off-by: Marco Pracucci <marco@pracucci.com>

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

pracucci · 2024-01-15T07:46:14Z

I've introduced -querier.promql-experimental-functions-enabled CLI flag (and respective YAML config option) to allow to enable experimental functions (disabled by default).
Should this be in the changelog?

Yes, absolutely. I just forgot about it, and added in 35db456.

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

pracucci force-pushed the upgrade-vendored-mimir-prometheus branch from 45d9b01 to 8b0e00a Compare January 5, 2024 17:27

pracucci marked this pull request as ready for review January 5, 2024 17:27

pracucci requested review from grafanabot and a team as code owners January 5, 2024 17:27

pracucci commented Jan 5, 2024

View reviewed changes

pkg/ruler/compat.go Show resolved Hide resolved

aknuds1 approved these changes Jan 8, 2024

View reviewed changes

pstibrany approved these changes Jan 9, 2024

View reviewed changes

dimitarvdimitrov reviewed Jan 9, 2024

View reviewed changes

dimitarvdimitrov approved these changes Jan 11, 2024

View reviewed changes

pracucci force-pushed the upgrade-vendored-mimir-prometheus branch from 9283af6 to fdfcd20 Compare January 15, 2024 07:41

pracucci added 2 commits January 15, 2024 08:43

Added CHANGELOG entry to mention -querier.promql-experimental-functio…

35db456

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

pracucci force-pushed the upgrade-vendored-mimir-prometheus branch from fdfcd20 to 35db456 Compare January 15, 2024 07:45

Simplify CLI flag description

bc8ae7b

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

pracucci merged commit ee9b483 into main Jan 15, 2024
28 checks passed

gotjosh mentioned this pull request Jan 19, 2024

Alertmanager: Support uploading Grafana Alertmanager Configuration an… #6682

Merged

4 tasks

pracucci mentioned this pull request Jan 25, 2024

Ensure we never use errors.Cause() #7215

Merged

4 tasks

eyenx mentioned this pull request May 28, 2024

chore(mimir): update to 5.3.0 adfinis/helm-charts#1256

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update vendored mimir-prometheus #7057

Update vendored mimir-prometheus #7057

pracucci commented Jan 5, 2024 •

edited

pracucci Jan 5, 2024

aknuds1 commented Jan 8, 2024

aknuds1 left a comment

pstibrany left a comment

pstibrany Jan 9, 2024

pracucci Jan 15, 2024

pracucci Jan 15, 2024

dimitarvdimitrov left a comment

dimitarvdimitrov left a comment •

edited

pracucci commented Jan 15, 2024

	f.BoolVar(&cfg.PromQLExperimentalFunctionsEnabled, "querier.promql-experimental-functions-enabled", false, sharedWithQueryFrontend("True to enable support for experimental PromQL functions."))
	f.BoolVar(&cfg.PromQLExperimentalFunctionsEnabled, "querier.promql-experimental-functions-enabled", false, sharedWithQueryFrontend("Enable experimental PromQL functions."))

Update vendored mimir-prometheus #7057

Update vendored mimir-prometheus #7057

Conversation

pracucci commented Jan 5, 2024 • edited

What this PR does

Prometheus changes

Other vendored libs changes

Other deps we don't use directly

Which issue(s) this PR fixes or relates to

Checklist

pracucci Jan 5, 2024

Choose a reason for hiding this comment

aknuds1 commented Jan 8, 2024

aknuds1 left a comment

Choose a reason for hiding this comment

pstibrany left a comment

Choose a reason for hiding this comment

pstibrany Jan 9, 2024

Choose a reason for hiding this comment

pracucci Jan 15, 2024

Choose a reason for hiding this comment

pracucci Jan 15, 2024

Choose a reason for hiding this comment

dimitarvdimitrov left a comment

Choose a reason for hiding this comment

dimitarvdimitrov left a comment • edited

Choose a reason for hiding this comment

pracucci commented Jan 15, 2024

pracucci commented Jan 5, 2024 •

edited

dimitarvdimitrov left a comment •

edited