From 753479c8c8e770adb88b9637f83a4f27a965508f Mon Sep 17 00:00:00 2001 From: gotjosh Date: Tue, 26 Sep 2023 12:27:30 +0100 Subject: [PATCH 1/3] Metrics: Wire up `objstore_bucket_operation_duration_seconds` in `iter` operations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Trying to get some statistics in Mimir about the time we're spending in our `iter` operations I realised that despite these operations showing up as part of the labels their value was always 0. Soon enough and with the help of Peter, I realised that these were not wired up in `objstore.go`. This PR does exactly that and measures the duration of the whole `iter` operation. There's one big caveat though, `iter` operations take a callback as an argument which is executed per each item returned. The computed duration will include whatever time we spend on each of these callbacks which might an undesirable effect for users of the library. Sadly, I can't get around this without changing the interface for `iter` to (e.g.) return JUST the time the network operation took. In the meantime, I've done the best I've could without incurring in a big change and documented the behaviour as part of the Histogram's HELP. Co-Authored-By: Peter Štibraný <895919+pstibrany@users.noreply.github.com> Signed-off-by: gotjosh --- go.mod | 2 +- go.sum | 1 - objstore.go | 4 +++- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index dee14caa..d66fc2fe 100644 --- a/go.mod +++ b/go.mod @@ -27,6 +27,7 @@ require ( golang.org/x/oauth2 v0.4.0 golang.org/x/sync v0.1.0 google.golang.org/api v0.103.0 + google.golang.org/grpc v1.53.0 gopkg.in/alecthomas/kingpin.v2 v2.2.6 gopkg.in/yaml.v2 v2.4.0 ) @@ -92,7 +93,6 @@ require ( golang.org/x/xerrors v0.0.0-20220907171357-04be3eba64a2 // indirect google.golang.org/appengine v1.6.7 // indirect google.golang.org/genproto v0.0.0-20230110181048-76db0878b65f // indirect - google.golang.org/grpc v1.53.0 // indirect google.golang.org/protobuf v1.28.1 // indirect gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c // indirect gopkg.in/ini.v1 v1.67.0 // indirect diff --git a/go.sum b/go.sum index 860aca72..45756f80 100644 --- a/go.sum +++ b/go.sum @@ -491,7 +491,6 @@ golang.org/x/sys v0.0.0-20210615035016-665e8c7367d1/go.mod h1:oPkhp1MJrh7nUepCBc golang.org/x/sys v0.0.0-20210616045830-e2b7044e8c71/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.0.0-20211216021012-1d35b9e2eb4e/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.0.0-20220114195835-da31bd327af9/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= -golang.org/x/sys v0.0.0-20220209214540-3681064d5158/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.0.0-20220715151400-c0bba94af5f8/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.5.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.6.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= diff --git a/objstore.go b/objstore.go index 20723704..3648b878 100644 --- a/objstore.go +++ b/objstore.go @@ -433,7 +433,7 @@ func WrapWithMetrics(b Bucket, reg prometheus.Registerer, name string) *metricBu opsDuration: promauto.With(reg).NewHistogramVec(prometheus.HistogramOpts{ Name: "objstore_bucket_operation_duration_seconds", - Help: "Duration of successful operations against the bucket", + Help: "Duration of successful operations against the bucket per operation - iter operations include time spent on each function call.", ConstLabels: prometheus.Labels{"bucket": name}, Buckets: []float64{0.001, 0.01, 0.1, 0.3, 0.6, 1, 3, 6, 9, 20, 30, 60, 90, 120}, }, []string{"operation"}), @@ -504,12 +504,14 @@ func (b *metricBucket) Iter(ctx context.Context, dir string, f func(name string) const op = OpIter b.ops.WithLabelValues(op).Inc() + start := time.Now() err := b.bkt.Iter(ctx, dir, f, options...) if err != nil { if !b.isOpFailureExpected(err) && ctx.Err() != context.Canceled { b.opsFailures.WithLabelValues(op).Inc() } } + b.opsDuration.WithLabelValues(op).Observe(time.Since(start).Seconds()) return err } From abdf84053ece3533b033aeaf3bd192a7c487ebb8 Mon Sep 17 00:00:00 2001 From: gotjosh Date: Tue, 26 Sep 2023 12:32:02 +0100 Subject: [PATCH 2/3] better words Signed-off-by: gotjosh --- objstore.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/objstore.go b/objstore.go index 3648b878..219a9572 100644 --- a/objstore.go +++ b/objstore.go @@ -433,7 +433,7 @@ func WrapWithMetrics(b Bucket, reg prometheus.Registerer, name string) *metricBu opsDuration: promauto.With(reg).NewHistogramVec(prometheus.HistogramOpts{ Name: "objstore_bucket_operation_duration_seconds", - Help: "Duration of successful operations against the bucket per operation - iter operations include time spent on each function call.", + Help: "Duration of successful operations against the bucket per operation - iter operations include time spent on each callback.", ConstLabels: prometheus.Labels{"bucket": name}, Buckets: []float64{0.001, 0.01, 0.1, 0.3, 0.6, 1, 3, 6, 9, 20, 30, 60, 90, 120}, }, []string{"operation"}), From 934414af4b0de0ed9ef3bf6aca251c611940d98a Mon Sep 17 00:00:00 2001 From: gotjosh Date: Tue, 26 Sep 2023 12:34:20 +0100 Subject: [PATCH 3/3] Add a changelog entry Signed-off-by: gotjosh --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0dc65033..46b1410e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ We use *breaking :warning:* to mark changes that are not backward compatible (re - [#62](https://github.com/thanos-io/objstore/pull/62) S3: Fix ignored context cancellation in `Iter` method. - [#77](https://github.com/thanos-io/objstore/pull/77) Fix buckets wrapped with metrics from being unable to determine object sizes in `Upload`. - [#78](https://github.com/thanos-io/objstore/pull/78) S3: Fix possible concurrent modification of the PutUserMetadata map. +- [#79](https://github.com/thanos-io/objstore/pull/79) Metrics: Fix `objstore_bucket_operation_duration_seconds` for `iter` operations. ### Added - [#15](https://github.com/thanos-io/objstore/pull/15) Add Oracle Cloud Infrastructure Object Storage Bucket support.