From c8b2d20aa352434dc6f3cf9c63f86b769d9279f4 Mon Sep 17 00:00:00 2001
From: Christian Simon <simon@swine.de>
Date: Thu, 27 Jul 2023 10:11:58 +0100
Subject: [PATCH] Fix overlapping metric registrations

This fixes a problem when multiple metrics bucket are regsitered with
different names. Also adds a test so this won't be happening in the
future.

Signed-off-by: Christian Simon <simon@swine.de>
---
 objstore.go      | 14 +++++++-------
 objstore_test.go |  8 ++++++++
 2 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/objstore.go b/objstore.go
index 2a0fe4dc..61e3dfb4 100644
--- a/objstore.go
+++ b/objstore.go
@@ -438,10 +438,11 @@ func WrapWithMetrics(b Bucket, reg prometheus.Registerer, name string) *metricBu
 			Buckets:     []float64{0.001, 0.01, 0.1, 0.3, 0.6, 1, 3, 6, 9, 20, 30, 60, 90, 120},
 		}, []string{"operation"}),
 
-		lastSuccessfulUploadTime: promauto.With(reg).NewGaugeVec(prometheus.GaugeOpts{
-			Name: "objstore_bucket_last_successful_upload_time",
-			Help: "Second timestamp of the last successful upload to the bucket.",
-		}, []string{"bucket"}),
+		lastSuccessfulUploadTime: promauto.With(reg).NewGauge(prometheus.GaugeOpts{
+			Name:        "objstore_bucket_last_successful_upload_time",
+			Help:        "Second timestamp of the last successful upload to the bucket.",
+			ConstLabels: prometheus.Labels{"bucket": name},
+		}),
 	}
 	for _, op := range []string{
 		OpIter,
@@ -465,7 +466,6 @@ func WrapWithMetrics(b Bucket, reg prometheus.Registerer, name string) *metricBu
 	} {
 		bkt.opsTransferredBytes.WithLabelValues(op)
 	}
-	bkt.lastSuccessfulUploadTime.WithLabelValues(b.Name())
 	return bkt
 }
 
@@ -479,7 +479,7 @@ type metricBucket struct {
 	opsFetchedBytes          *prometheus.CounterVec
 	opsTransferredBytes      *prometheus.HistogramVec
 	opsDuration              *prometheus.HistogramVec
-	lastSuccessfulUploadTime *prometheus.GaugeVec
+	lastSuccessfulUploadTime prometheus.Gauge
 }
 
 func (b *metricBucket) WithExpectedErrs(fn IsOpFailureExpectedFunc) Bucket {
@@ -599,7 +599,7 @@ func (b *metricBucket) Upload(ctx context.Context, name string, r io.Reader) err
 		}
 		return err
 	}
-	b.lastSuccessfulUploadTime.WithLabelValues(b.bkt.Name()).SetToCurrentTime()
+	b.lastSuccessfulUploadTime.SetToCurrentTime()
 	b.opsDuration.WithLabelValues(op).Observe(time.Since(start).Seconds())
 	return nil
 }
diff --git a/objstore_test.go b/objstore_test.go
index ab5c5e5b..4b105281 100644
--- a/objstore_test.go
+++ b/objstore_test.go
@@ -72,6 +72,14 @@ func TestMetricBucket_Close(t *testing.T) {
 	testutil.Assert(t, promtest.ToFloat64(bkt.lastSuccessfulUploadTime) > lastUpload)
 }
 
+// TestMetricBucket_MultipleClients tests that the metrics from two different buckets clients are not conflicting with each other.
+func TestMetricBucket_Multiple_Clients(t *testing.T) {
+	reg := prometheus.NewPedanticRegistry()
+
+	WrapWithMetrics(NewInMemBucket(), reg, "abc")
+	WrapWithMetrics(NewInMemBucket(), reg, "def")
+}
+
 func TestDownloadUploadDirConcurrency(t *testing.T) {
 	r := prometheus.NewRegistry()
 	m := WrapWithMetrics(NewInMemBucket(), r, "")