Merge branch 'main' into traces-sampler-env-var

Signed-off-by: Shazi <42436533+shayyxi@users.noreply.github.com>

CHANGELOG.md

@@ -30,6 +30,7 @@ We use *breaking :warning:* to mark changes that are not backward compatible (re
 - [#6218](https://github.com/thanos-io/thanos/pull/6218) mixin(Store): handle ResourceExhausted as a non-server error. As a consequence, this error won't contribute to Store's grpc errors alerts.
 - [#6271](https://github.com/thanos-io/thanos/pull/6271) Receive: Fix segfault in `LabelValues` during head compaction.
 - [#6306](https://github.com/thanos-io/thanos/pull/6306) Tracing: tracing in OTLP utilize the OTEL_TRACES_SAMPLER env variable
+- [#6330](https://github.com/thanos-io/thanos/pull/6330) Store: Fix inconsistent error for series limits.
 
 ### Changed
 - [#6168](https://github.com/thanos-io/thanos/pull/6168) Receiver: Make ketama hashring fail early when configured with number of nodes lower than the replication factor.
@@ -42,6 +43,8 @@ We use *breaking :warning:* to mark changes that are not backward compatible (re
 - [#6303](https://github.com/thanos-io/thanos/pull/6303) Store: added and start using streamed snappy encoding for postings list instead of block based one. This leads to constant memory usage during decompression. This approximately halves memory usage when decompressing a postings list in index cache.
 - [#6071](https://github.com/thanos-io/thanos/pull/6071) Query Frontend: *breaking :warning:* Add experimental native histogram support for which we updated and aligned with the [Prometheus common](https://github.com/prometheus/common) model, which is used for caching so a cache reset required.
 - [#6163](https://github.com/thanos-io/thanos/pull/6163) Receiver: changed max backoff from 30s to 5s for forwarding requests. Can be configured with `--receive-forward-max-backoff`.
+- [#6327](https://github.com/thanos-io/thanos/pull/6327) *: *breaking :warning:* Use histograms instead of summaries for instrumented handlers.
+- [#6322](https://github.com/thanos-io/thanos/pull/6322) Logging: Avoid expensive log.Valuer evaluation for disallowed levels.
 
 ### Removed
 

Makefile

@@ -395,6 +395,7 @@ github.com/prometheus/client_golang/prometheus.{DefaultGatherer,DefBuckets,NewUn
 github.com/prometheus/client_golang/prometheus.{NewCounter,NewCounterVec,NewCounterVec,NewGauge,NewGaugeVec,NewGaugeFunc,\
 NewHistorgram,NewHistogramVec,NewSummary,NewSummaryVec}=github.com/prometheus/client_golang/prometheus/promauto.{NewCounter,\
 NewCounterVec,NewCounterVec,NewGauge,NewGaugeVec,NewGaugeFunc,NewHistorgram,NewHistogramVec,NewSummary,NewSummaryVec},\
+github.com/NYTimes/gziphandler.{GzipHandler}=github.com/klauspost/compress/gzhttp.{GzipHandler},\
 sync/atomic=go.uber.org/atomic,github.com/cortexproject/cortex=github.com/thanos-io/thanos/internal/cortex,\
 io/ioutil.{Discard,NopCloser,ReadAll,ReadDir,ReadFile,TempDir,TempFile,Writefile}" $(shell go list ./... | grep -v "internal/cortex")
 	@$(FAILLINT) -paths "fmt.{Print,Println,Sprint}" -ignore-tests ./...

cmd/thanos/query.go

@@ -713,7 +713,7 @@ func runQuery(
 		api := apiv1.NewQueryAPI(
 			logger,
 			endpoints.GetEndpointStatus,
-			*engineFactory,
+			engineFactory,
 			apiv1.PromqlEngineType(defaultEngine),
 			lookbackDeltaCreator,
 			queryableCreator,
@@ -800,7 +800,7 @@ func runQuery(
 		)
 
 		defaultEngineType := querypb.EngineType(querypb.EngineType_value[defaultEngine])
-		grpcAPI := apiv1.NewGRPCAPI(time.Now, queryReplicaLabels, queryableCreator, *engineFactory, defaultEngineType, lookbackDeltaCreator, instantDefaultMaxSourceResolution)
+		grpcAPI := apiv1.NewGRPCAPI(time.Now, queryReplicaLabels, queryableCreator, engineFactory, defaultEngineType, lookbackDeltaCreator, instantDefaultMaxSourceResolution)
 		storeServer := store.NewLimitedStoreServer(store.NewInstrumentedStoreServer(reg, proxy), reg, storeRateLimits)
 		s := grpcserver.New(logger, reg, tracer, grpcLogOpts, tagOpts, comp, grpcProbe,
 			grpcserver.WithServer(apiv1.RegisterQueryServer(grpcAPI)),

cmd/thanos/query_frontend.go

@@ -8,10 +8,10 @@ import (
 	"net/http"
 	"time"
 
-	"github.com/NYTimes/gziphandler"
 	extflag "github.com/efficientgo/tools/extkingpin"
 	"github.com/go-kit/log"
 	"github.com/go-kit/log/level"
+	"github.com/klauspost/compress/gzhttp"
 	"github.com/oklog/run"
 	"github.com/opentracing/opentracing-go"
 	"github.com/pkg/errors"
@@ -277,7 +277,7 @@ func runQueryFrontend(
 	// Create the query frontend transport.
 	handler := transport.NewHandler(*cfg.CortexHandlerConfig, roundTripper, logger, nil)
 	if cfg.CompressResponses {
-		handler = gziphandler.GzipHandler(handler)
+		handler = gzhttp.GzipHandler(handler)
 	}
 
 	httpProbe := prober.NewHTTP()
@@ -311,7 +311,7 @@ func runQueryFrontend(
 					logger,
 					ins.NewHandler(
 						name,
-						gziphandler.GzipHandler(
+						gzhttp.GzipHandler(
 							middleware.RequestID(
 								logMiddleware.HTTPMiddleware(name, f),
 							),

go.mod

@@ -6,7 +6,6 @@ require (
 	cloud.google.com/go/storage v1.28.1 // indirect
 	cloud.google.com/go/trace v1.8.0
 	github.com/GoogleCloudPlatform/opentelemetry-operations-go/exporter/trace v1.8.3
-	github.com/NYTimes/gziphandler v1.1.1
 	github.com/alecthomas/units v0.0.0-20211218093645-b94a6e3cc137
 	github.com/alicebob/miniredis/v2 v2.22.0
 	github.com/blang/semver/v4 v4.0.0

go.sum

@@ -95,8 +95,6 @@ github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/resourcemapp
 github.com/HdrHistogram/hdrhistogram-go v1.1.2 h1:5IcZpTvzydCQeHzK4Ef/D5rrSqwxob0t8PQPMybUNFM=
 github.com/Knetic/govaluate v3.0.1-0.20171022003610-9aa49832a739+incompatible/go.mod h1:r7JcOSlj0wfOMncg0iLm8Leh48TZaKVeNIfJntJ2wa0=
 github.com/Microsoft/go-winio v0.6.0 h1:slsWYD/zyx7lCXoZVlvQrj0hPTM1HI4+v1sIda2yDvg=
-github.com/NYTimes/gziphandler v1.1.1 h1:ZUDjpQae29j0ryrS0u/B8HZfJBtBQHjqw2rQ2cqUQ3I=
-github.com/NYTimes/gziphandler v1.1.1/go.mod h1:n/CVRwUEOgIxrgPvAQhUUr9oeUtvrhMomdKFjzJNB0c=
 github.com/OneOfOne/xxhash v1.2.2/go.mod h1:HSdplMjZKSmBqAxg5vPj2TmRDmfkzw+cTzAElWljhcU=
 github.com/OneOfOne/xxhash v1.2.6 h1:U68crOE3y3MPttCMQGywZOLrTeF5HHJ3/vDBCJn9/bA=
 github.com/OneOfOne/xxhash v1.2.6/go.mod h1:eZbhyaAYD41SGSSsnmcpxVoRiQ/MPUTjUdIIOT9Um7Q=

pkg/api/api.go

@@ -27,9 +27,9 @@ import (
 	"runtime"
 	"time"
 
-	"github.com/NYTimes/gziphandler"
 	"github.com/go-kit/log"
 	"github.com/go-kit/log/level"
+	"github.com/klauspost/compress/gzhttp"
 	"github.com/opentracing/opentracing-go"
 	"github.com/prometheus/common/route"
 	"github.com/prometheus/common/version"
@@ -222,7 +222,7 @@ func GetInstr(
 
 		return tracing.HTTPMiddleware(tracer, name, logger,
 			ins.NewHandler(name,
-				gziphandler.GzipHandler(
+				gzhttp.GzipHandler(
 					middleware.RequestID(
 						logMiddleware.HTTPMiddleware(name, hf),
 					),

pkg/api/query/grpc.go

@@ -23,7 +23,7 @@ type GRPCAPI struct {
 	now                         func() time.Time
 	replicaLabels               []string
 	queryableCreate             query.QueryableCreator
-	engineFactory               QueryEngineFactory
+	engineFactory               *QueryEngineFactory
 	defaultEngine               querypb.EngineType
 	lookbackDeltaCreate         func(int64) time.Duration
 	defaultMaxResolutionSeconds time.Duration
@@ -33,7 +33,7 @@ func NewGRPCAPI(
 	now func() time.Time,
 	replicaLabels []string,
 	creator query.QueryableCreator,
-	engineFactory QueryEngineFactory,
+	engineFactory *QueryEngineFactory,
 	defaultEngine querypb.EngineType,
 	lookbackDeltaCreate func(int64) time.Duration,
 	defaultMaxResolutionSeconds time.Duration,

pkg/api/query/grpc_test.go

@@ -47,7 +47,7 @@ func TestGRPCQueryAPIErrorHandling(t *testing.T) {
 	}
 
 	for _, test := range tests {
-		engineFactory := QueryEngineFactory{
+		engineFactory := &QueryEngineFactory{
 			prometheusEngine: test.engine,
 		}
 		api := NewGRPCAPI(time.Now, nil, queryableCreator, engineFactory, querypb.EngineType_prometheus, lookbackDeltaFunc, 0)

pkg/api/query/v1.go

@@ -133,7 +133,7 @@ type QueryAPI struct {
 	gate            gate.Gate
 	queryableCreate query.QueryableCreator
 	// queryEngine returns appropriate promql.Engine for a query with a given step.
-	engineFactory       QueryEngineFactory
+	engineFactory       *QueryEngineFactory
 	defaultEngine       PromqlEngineType
 	lookbackDeltaCreate func(int64) time.Duration
 	ruleGroups          rules.UnaryClient
@@ -171,7 +171,7 @@ type seriesQueryPerformanceMetricsAggregator interface {
 func NewQueryAPI(
 	logger log.Logger,
 	endpointStatus func() []query.EndpointStatus,
-	engineFactory QueryEngineFactory,
+	engineFactory *QueryEngineFactory,
 	defaultEngine PromqlEngineType,
 	lookbackDeltaCreate func(int64) time.Duration,
 	c query.QueryableCreator,

pkg/api/query/v1_test.go

@@ -183,14 +183,12 @@ func TestQueryEndpoints(t *testing.T) {
 
 	now := time.Now()
 	timeout := 100 * time.Second
-	ef := QueryEngineFactory{
-		engineOpts: promql.EngineOpts{
-			Logger:     nil,
-			Reg:        nil,
-			MaxSamples: 10000,
-			Timeout:    timeout,
-		},
-	}
+	ef := NewQueryEngineFactory(promql.EngineOpts{
+		Logger:     nil,
+		Reg:        nil,
+		MaxSamples: 10000,
+		Timeout:    timeout,
+	}, nil)
 	api := &QueryAPI{
 		baseAPI: &baseAPI.BaseAPI{
 			Now: func() time.Time { return now },
@@ -727,14 +725,12 @@ func TestMetadataEndpoints(t *testing.T) {
 
 	now := time.Now()
 	timeout := 100 * time.Second
-	ef := QueryEngineFactory{
-		engineOpts: promql.EngineOpts{
-			Logger:     nil,
-			Reg:        nil,
-			MaxSamples: 10000,
-			Timeout:    timeout,
-		},
-	}
+	ef := NewQueryEngineFactory(promql.EngineOpts{
+		Logger:     nil,
+		Reg:        nil,
+		MaxSamples: 10000,
+		Timeout:    timeout,
+	}, nil)
 	api := &QueryAPI{
 		baseAPI: &baseAPI.BaseAPI{
 			Now: func() time.Time { return now },

pkg/extprom/http/metrics.go

@@ -10,31 +10,40 @@ import (
 
 type defaultMetrics struct {
 	requestDuration      *prometheus.HistogramVec
-	requestSize          *prometheus.SummaryVec
+	requestSize          *prometheus.HistogramVec
 	requestsTotal        *prometheus.CounterVec
-	responseSize         *prometheus.SummaryVec
+	responseSize         *prometheus.HistogramVec
 	inflightHTTPRequests *prometheus.GaugeVec
 }
 
-func newDefaultMetrics(reg prometheus.Registerer, buckets []float64, extraLabels []string) *defaultMetrics {
-	if buckets == nil {
-		buckets = []float64{0.001, 0.01, 0.1, 0.3, 0.6, 1, 3, 6, 9, 20, 30, 60, 90, 120, 240, 360, 720}
+func newDefaultMetrics(reg prometheus.Registerer, durationBuckets []float64, extraLabels []string) *defaultMetrics {
+	if durationBuckets == nil {
+		durationBuckets = []float64{0.001, 0.01, 0.1, 0.3, 0.6, 1, 3, 6, 9, 20, 30, 60, 90, 120, 240, 360, 720}
 	}
 
+	bytesBuckets := prometheus.ExponentialBuckets(64, 2, 10)
+	bucketFactor := 1.1
+	maxBuckets := uint32(100)
+
 	return &defaultMetrics{
 		requestDuration: promauto.With(reg).NewHistogramVec(
 			prometheus.HistogramOpts{
-				Name:    "http_request_duration_seconds",
-				Help:    "Tracks the latencies for HTTP requests.",
-				Buckets: buckets,
+				Name:                           "http_request_duration_seconds",
+				Help:                           "Tracks the latencies for HTTP requests.",
+				Buckets:                        durationBuckets,
+				NativeHistogramBucketFactor:    bucketFactor,
+				NativeHistogramMaxBucketNumber: maxBuckets,
 			},
 			append([]string{"code", "handler", "method"}, extraLabels...),
 		),
 
-		requestSize: promauto.With(reg).NewSummaryVec(
-			prometheus.SummaryOpts{
-				Name: "http_request_size_bytes",
-				Help: "Tracks the size of HTTP requests.",
+		requestSize: promauto.With(reg).NewHistogramVec(
+			prometheus.HistogramOpts{
+				Name:                           "http_request_size_bytes",
+				Help:                           "Tracks the size of HTTP requests.",
+				Buckets:                        bytesBuckets,
+				NativeHistogramBucketFactor:    bucketFactor,
+				NativeHistogramMaxBucketNumber: maxBuckets,
 			},
 			append([]string{"code", "handler", "method"}, extraLabels...),
 		),
@@ -47,10 +56,13 @@ func newDefaultMetrics(reg prometheus.Registerer, buckets []float64, extraLabels
 			append([]string{"code", "handler", "method"}, extraLabels...),
 		),
 
-		responseSize: promauto.With(reg).NewSummaryVec(
-			prometheus.SummaryOpts{
-				Name: "http_response_size_bytes",
-				Help: "Tracks the size of HTTP responses.",
+		responseSize: promauto.With(reg).NewHistogramVec(
+			prometheus.HistogramOpts{
+				Name:                           "http_response_size_bytes",
+				Help:                           "Tracks the size of HTTP responses.",
+				Buckets:                        bytesBuckets,
+				NativeHistogramBucketFactor:    bucketFactor,
+				NativeHistogramMaxBucketNumber: maxBuckets,
 			},
 			append([]string{"code", "handler", "method"}, extraLabels...),
 		),

pkg/logging/logger.go

@@ -46,11 +46,14 @@ func NewLogger(logLevel, logFormat, debugName string) log.Logger {
 		logger = log.NewJSONLogger(log.NewSyncWriter(os.Stderr))
 	}
 
+	// Sort the logger chain to avoid expensive log.Valuer evaluation for disallowed level.
+	// Ref: https://github.com/go-kit/log/issues/14#issuecomment-945038252
+	logger = log.With(logger, "ts", log.DefaultTimestampUTC, "caller", log.Caller(5))
 	logger = level.NewFilter(logger, lvl)
 
 	if debugName != "" {
 		logger = log.With(logger, "name", debugName)
 	}
 
-	return log.With(logger, "ts", log.DefaultTimestampUTC, "caller", log.DefaultCaller)
+	return logger
 }

pkg/logging/logger_test.go

@@ -0,0 +1,19 @@
+// Copyright (c) The Thanos Authors.
+// Licensed under the Apache License 2.0.
+
+package logging
+
+import (
+	"testing"
+
+	"github.com/go-kit/log/level"
+)
+
+func BenchmarkDisallowedLogLevels(b *testing.B) {
+	logger := NewLogger("warn", "logfmt", "benchmark")
+
+	for i := 0; i < b.N; i++ {
+		level.Info(logger).Log("hello", "world", "number", i)
+		level.Debug(logger).Log("hello", "world", "number", i)
+	}
+}

pkg/store/bucket.go

@@ -1574,7 +1574,11 @@ func (s *BucketStore) LabelNames(ctx context.Context, req *storepb.LabelNamesReq
 	s.mtx.RUnlock()
 
 	if err := g.Wait(); err != nil {
-		return nil, status.Error(codes.Internal, err.Error())
+		code := codes.Internal
+		if s, ok := status.FromError(errors.Cause(err)); ok {
+			code = s.Code()
+		}
+		return nil, status.Error(code, err.Error())
 	}
 
 	anyHints, err := types.MarshalAny(resHints)
@@ -1762,7 +1766,11 @@ func (s *BucketStore) LabelValues(ctx context.Context, req *storepb.LabelValuesR
 	s.mtx.RUnlock()
 
 	if err := g.Wait(); err != nil {
-		return nil, status.Error(codes.Aborted, err.Error())
+		code := codes.Internal
+		if s, ok := status.FromError(errors.Cause(err)); ok {
+			code = s.Code()
+		}
+		return nil, status.Error(code, err.Error())
 	}
 
 	anyHints, err := types.MarshalAny(resHints)