Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update mimir-prometheus at grafana/mimir-prometheus@53cf3fb #9148

Merged
merged 9 commits into from
Sep 6, 2024

Conversation

krajorama
Copy link
Contributor

@krajorama krajorama commented Aug 30, 2024

What this PR does

Update mimir-prometheus at grafana/mimir-prometheus@53cf3fb

Includes fixes we depend on:
prometheus/prometheus#14771
prometheus/prometheus#14831

Pin Google GRPC to 1.65, ref grafana/dskit#581
Keep pinning client_golang to 1.19.1 due to data races in histogram exemplars. prometheus/client_golang#1623 , prometheus/client_golang#1608

The remote read client interface has changed in prometheus/prometheus#11379
I've updated the users and also added native histogram support to backfill.

The client golang update contains prometheus/client_golang#1424 which invalidates some
tests and also exposes some test errors. From now on GatherAndCompare checks if all supplied metrics are actually
present and errors otherwise, this it cannot be used to assert on missing metrics as before.

@krajorama krajorama requested review from stevesg, grafanabot and a team as code owners August 30, 2024 15:02
@krajorama krajorama changed the title update mimir-prometheus at update mimir-prometheus at grafana/mimir-prometheus@fdf902d Aug 30, 2024
@@ -429,7 +429,7 @@ func otelMetricsToTimeseries(ctx context.Context, tenantID string, addSuffixes b
// Old, less efficient, version of otelMetricsToTimeseries.
func otelMetricsToTimeseriesOld(ctx context.Context, tenantID string, addSuffixes bool, discardedDueToOtelParseError *prometheus.CounterVec, logger log.Logger, md pmetric.Metrics) ([]mimirpb.PreallocTimeseries, error) {
converter := prometheusremotewrite.NewPrometheusConverter()
errs := converter.FromMetrics(ctx, md, prometheusremotewrite.Settings{
_, errs := converter.FromMetrics(ctx, md, prometheusremotewrite.Settings{
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note to reviewers: I'm not sure what we'd do with annotations here @aknuds1

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can do this:

	annots, errs := converter.FromMetrics(ctx, md, prometheusremotewrite.Settings{
		AddMetricSuffixes:                   addSuffixes,
		EnableCreatedTimestampZeroIngestion: enableCTZeroIngestion,
	})
	promTS := converter.TimeSeries()
	if errs != nil {
                ...
		level.Warn(logger).Log("msg", "OTLP parse error", "err", parseErrs)
	}
	ws, _ := annots.AsStrings("", 0, 0)
	if len(ws) > 0 {
		level.Warn(logger).Log("msg", "Warnings translating OTLP metrics to Prometheus write request", "warnings", ws)
	}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

followup PR? there's already a lot going on in this PR

pkg/ruler/remotequerier.go Outdated Show resolved Hide resolved
@krajorama
Copy link
Contributor Author

@aknuds1 I'm ignoring annotations coming from OTLP metrics conversion for now , you might want to change that later.

@krajorama krajorama changed the title update mimir-prometheus at grafana/mimir-prometheus@fdf902d update mimir-prometheus at grafana/mimir-prometheus@53cf3fb Sep 6, 2024
Followup to
grafana/mimir-prometheus#689

Signed-off-by: György Krajcsovits <[email protected]>
Possible performance regression:
Ref: grafana/dskit#581

Also has breaking API changes for the experimental buffers.

Signed-off-by: György Krajcsovits <[email protected]>
Signed-off-by: György Krajcsovits <[email protected]>
Have a function that can emulate the old GatherAndCompare

Follow up to prometheus/client_golang#1424

Signed-off-by: György Krajcsovits <[email protected]>
Follow up to:
prometheus/prometheus#14772

Signed-off-by: György Krajcsovits <[email protected]>
Due to data races in histogram exemplars.

Signed-off-by: György Krajcsovits <[email protected]>
This reverts commit 44c5c37.
@krajorama krajorama marked this pull request as ready for review September 6, 2024 08:53
@@ -1,6 +1,6 @@
module github.com/grafana/mimir

go 1.22
go 1.22.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what effect does this have?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From reading https://go.dev/doc/toolchain, I don't think there's any difference between the two.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure, go mod tidy sets it

Copy link
Member

@jesusvazquez jesusvazquez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Also checked that this brings grafana/mimir-prometheus#689

@krajorama krajorama merged commit e6ec9dd into main Sep 6, 2024
29 checks passed
@krajorama krajorama deleted the krajo/update-prometheus branch September 6, 2024 10:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants