Queriers: Stop emitting spans for every objectstorage call (#9857)

**What this PR does / why we need it**:
Stop emitting spans for every call to our object storages (like S3 or
Azure). They are causing every chunk fetching to emit a span. Request metrics will still be reported just fine.

**Which issue(s) this PR fixes**:
N/A

CHANGELOG.md

@@ -66,6 +66,7 @@
 
 ##### Changes
 
+* [9857](https://github.com/grafana/loki/pull/9857) **DylanGuedes**: Stop emitting spans for every `AWS.S3` or `Azure.Blob` call.
 * [9212](https://github.com/grafana/loki/pull/9212) **trevorwhitney**: Rename UsageReport to Analytics. The only external impact of this change is a change in the `-list-targets` output.
 
 #### Promtail

pkg/storage/chunk/client/aws/s3_storage_client.go

@@ -35,6 +35,7 @@ import (
 	"github.com/grafana/loki/pkg/storage/chunk/client/hedging"
 	storageawscommon "github.com/grafana/loki/pkg/storage/common/aws"
 	"github.com/grafana/loki/pkg/util"
+	loki_instrument "github.com/grafana/loki/pkg/util/instrument"
 )
 
 const (
@@ -372,13 +373,15 @@ func (a *S3ObjectClient) GetObject(ctx context.Context, objectKey string) (io.Re
 	// Map the key into a bucket
 	bucket := a.bucketFromKey(objectKey)
 
+	var lastErr error
+
 	retries := backoff.New(ctx, a.cfg.BackoffConfig)
-	err := ctx.Err()
 	for retries.Ongoing() {
 		if ctx.Err() != nil {
 			return nil, 0, errors.Wrap(ctx.Err(), "ctx related error during s3 getObject")
 		}
-		err = instrument.CollectedRequest(ctx, "S3.GetObject", s3RequestDuration, instrument.ErrorCode, func(ctx context.Context) error {
+
+		lastErr = loki_instrument.TimeRequest(ctx, "S3.GetObject", s3RequestDuration, instrument.ErrorCode, func(ctx context.Context) error {
 			var requestErr error
 			resp, requestErr = a.hedgedS3.GetObjectWithContext(ctx, &s3.GetObjectInput{
 				Bucket: aws.String(bucket),
@@ -386,21 +389,23 @@ func (a *S3ObjectClient) GetObject(ctx context.Context, objectKey string) (io.Re
 			})
 			return requestErr
 		})
+
 		var size int64
 		if resp.ContentLength != nil {
 			size = *resp.ContentLength
 		}
-		if err == nil && resp.Body != nil {
+		if lastErr == nil && resp.Body != nil {
 			return resp.Body, size, nil
 		}
 		retries.Wait()
 	}
-	return nil, 0, errors.Wrap(err, "failed to get s3 object")
+
+	return nil, 0, errors.Wrap(lastErr, "failed to get s3 object")
 }
 
 // PutObject into the store
 func (a *S3ObjectClient) PutObject(ctx context.Context, objectKey string, object io.ReadSeeker) error {
-	return instrument.CollectedRequest(ctx, "S3.PutObject", s3RequestDuration, instrument.ErrorCode, func(ctx context.Context) error {
+	return loki_instrument.TimeRequest(ctx, "S3.PutObject", s3RequestDuration, instrument.ErrorCode, func(ctx context.Context) error {
 		putObjectInput := &s3.PutObjectInput{
 			Body:         object,
 			Bucket:       aws.String(a.bucketFromKey(objectKey)),
@@ -425,7 +430,7 @@ func (a *S3ObjectClient) List(ctx context.Context, prefix, delimiter string) ([]
 	var commonPrefixes []client.StorageCommonPrefix
 
 	for i := range a.bucketNames {
-		err := instrument.CollectedRequest(ctx, "S3.List", s3RequestDuration, instrument.ErrorCode, func(ctx context.Context) error {
+		err := loki_instrument.TimeRequest(ctx, "S3.List", s3RequestDuration, instrument.ErrorCode, func(ctx context.Context) error {
 			input := s3.ListObjectsV2Input{
 				Bucket:    aws.String(a.bucketNames[i]),
 				Prefix:    aws.String(prefix),

pkg/storage/chunk/client/azure/blob_storage_client.go

@@ -28,6 +28,7 @@ import (
 	"github.com/grafana/loki/pkg/storage/chunk/client/hedging"
 	client_util "github.com/grafana/loki/pkg/storage/chunk/client/util"
 	"github.com/grafana/loki/pkg/util"
+	loki_instrument "github.com/grafana/loki/pkg/util/instrument"
 	"github.com/grafana/loki/pkg/util/log"
 )
 
@@ -226,7 +227,7 @@ func (b *BlobStorage) GetObject(ctx context.Context, objectKey string) (io.ReadC
 		size int64
 		rc   io.ReadCloser
 	)
-	err := instrument.CollectedRequest(ctx, "azure.GetObject", instrument.NewHistogramCollector(b.metrics.requestDuration), instrument.ErrorCode, func(ctx context.Context) error {
+	err := loki_instrument.TimeRequest(ctx, "azure.GetObject", instrument.NewHistogramCollector(b.metrics.requestDuration), instrument.ErrorCode, func(ctx context.Context) error {
 		var err error
 		rc, size, err = b.getObject(ctx, objectKey)
 		return err
@@ -257,7 +258,7 @@ func (b *BlobStorage) getObject(ctx context.Context, objectKey string) (rc io.Re
 }
 
 func (b *BlobStorage) PutObject(ctx context.Context, objectKey string, object io.ReadSeeker) error {
-	return instrument.CollectedRequest(ctx, "azure.PutObject", instrument.NewHistogramCollector(b.metrics.requestDuration), instrument.ErrorCode, func(ctx context.Context) error {
+	return loki_instrument.TimeRequest(ctx, "azure.PutObject", instrument.NewHistogramCollector(b.metrics.requestDuration), instrument.ErrorCode, func(ctx context.Context) error {
 		blockBlobURL, err := b.getBlobURL(objectKey, false)
 		if err != nil {
 			return err
@@ -471,7 +472,7 @@ func (b *BlobStorage) List(ctx context.Context, prefix, delimiter string) ([]cli
 			return nil, nil, ctx.Err()
 		}
 
-		err := instrument.CollectedRequest(ctx, "azure.List", instrument.NewHistogramCollector(b.metrics.requestDuration), instrument.ErrorCode, func(ctx context.Context) error {
+		err := loki_instrument.TimeRequest(ctx, "azure.List", instrument.NewHistogramCollector(b.metrics.requestDuration), instrument.ErrorCode, func(ctx context.Context) error {
 			listBlob, err := b.containerURL.ListBlobsHierarchySegment(ctx, marker, delimiter, azblob.ListBlobsSegmentOptions{Prefix: prefix})
 			if err != nil {
 				return err
@@ -504,7 +505,7 @@ func (b *BlobStorage) List(ctx context.Context, prefix, delimiter string) ([]cli
 }
 
 func (b *BlobStorage) DeleteObject(ctx context.Context, blobID string) error {
-	return instrument.CollectedRequest(ctx, "azure.DeleteObject", instrument.NewHistogramCollector(b.metrics.requestDuration), instrument.ErrorCode, func(ctx context.Context) error {
+	return loki_instrument.TimeRequest(ctx, "azure.DeleteObject", instrument.NewHistogramCollector(b.metrics.requestDuration), instrument.ErrorCode, func(ctx context.Context) error {
 		blockBlobURL, err := b.getBlobURL(blobID, false)
 		if err != nil {
 			return err

pkg/util/instrument/instrument.go

@@ -0,0 +1,24 @@
+package instrument
+
+import (
+	"context"
+	"time"
+
+	"github.com/weaveworks/common/instrument"
+)
+
+// TimeRequest reports how much time was spent on the given function  `f`.
+//
+// It is a thinner version of weaveworks/common/instrument.CollectedRequest that doesn't emit spans.
+func TimeRequest(ctx context.Context, method string, col instrument.Collector, toStatusCode func(error) string, f func(context.Context) error) error {
+	if toStatusCode == nil {
+		toStatusCode = instrument.ErrorCode
+	}
+
+	start := time.Now()
+	col.Before(ctx, method, start)
+	err := f(ctx)
+	col.After(ctx, method, toStatusCode(err), start)
+
+	return err
+}

pkg/util/instrument/instrument_test.go

@@ -0,0 +1,70 @@
+package instrument
+
+// Source: https://github.com/weaveworks/common/blob/master/instrument/instrument_test.go
+
+import (
+	"context"
+	"errors"
+	"testing"
+	"time"
+
+	"github.com/prometheus/client_golang/prometheus"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"github.com/weaveworks/common/instrument"
+)
+
+func TestNewHistogramCollector(t *testing.T) {
+	m := prometheus.NewHistogramVec(prometheus.HistogramOpts{
+		Namespace: "test",
+		Subsystem: "instrumentation",
+		Name:      "foo",
+		Help:      "",
+		Buckets:   prometheus.DefBuckets,
+	}, instrument.HistogramCollectorBuckets)
+	c := instrument.NewHistogramCollector(m)
+	assert.NotNil(t, c)
+}
+
+type spyCollector struct {
+	before    bool
+	after     bool
+	afterCode string
+}
+
+func (c *spyCollector) Register() {
+}
+
+// Before collects for the upcoming request.
+func (c *spyCollector) Before(_ context.Context, _ string, _ time.Time) {
+	c.before = true
+}
+
+// After collects when the request is done.
+func (c *spyCollector) After(_ context.Context, _, statusCode string, _ time.Time) {
+	c.after = true
+	c.afterCode = statusCode
+}
+
+func TestCollectedRequest(t *testing.T) {
+	c := &spyCollector{}
+	fcalled := false
+	require.NoError(t, instrument.CollectedRequest(context.Background(), "test", c, nil, func(_ context.Context) error {
+		fcalled = true
+		return nil
+	}))
+	assert.True(t, fcalled)
+	assert.True(t, c.before)
+	assert.True(t, c.after)
+	assert.Equal(t, "200", c.afterCode)
+}
+
+func TestCollectedRequest_Error(t *testing.T) {
+	c := &spyCollector{}
+	require.Error(t, instrument.CollectedRequest(context.Background(), "test", c, nil, func(_ context.Context) error {
+		return errors.New("boom")
+	}))
+	assert.True(t, c.before)
+	assert.True(t, c.after)
+	assert.Equal(t, "500", c.afterCode)
+}