Improve formatting of timestamp too new/old error (#3967)

* Improved error formatting for timestamp too new/old

Signed-off-by: Marco Pracucci <[email protected]>

* Directly return the TenantID() error instead of generating a new one

Signed-off-by: Marco Pracucci <[email protected]>

* Reuse a shared error for 'ingester stopping'

Signed-off-by: Marco Pracucci <[email protected]>

Author: pracucci

pkg/distributor/distributor_test.go

@@ -1724,7 +1724,7 @@ func TestDistributorValidation(t *testing.T) {
 				TimestampMs: int64(past),
 				Value:       2,
 			}},
-			err: httpgrpc.Errorf(http.StatusBadRequest, "sample for 'testmetric' has timestamp too old: %d", past),
+			err: httpgrpc.Errorf(http.StatusBadRequest, `timestamp too old: %d metric: "testmetric"`, past),
 		},
 
 		// Test validation fails for samples from the future.
@@ -1734,7 +1734,7 @@ func TestDistributorValidation(t *testing.T) {
 				TimestampMs: int64(future),
 				Value:       4,
 			}},
-			err: httpgrpc.Errorf(http.StatusBadRequest, "sample for 'testmetric' has timestamp too new: %d", future),
+			err: httpgrpc.Errorf(http.StatusBadRequest, `timestamp too new: %d metric: "testmetric"`, future),
 		},
 
 		// Test maximum labels names per series.

pkg/ingester/ingester.go

@@ -49,6 +49,8 @@ const (
 var (
 	// This is initialised if the WAL is enabled and the records are fetched from this pool.
 	recordPool sync.Pool
+
+	errIngesterStopping = errors.New("ingester stopping")
 )
 
 // Config for an Ingester.
@@ -452,7 +454,7 @@ func (i *Ingester) Push(ctx context.Context, req *cortexpb.WriteRequest) (*corte
 
 	userID, err := tenant.TenantID(ctx)
 	if err != nil {
-		return nil, fmt.Errorf("no user id")
+		return nil, err
 	}
 
 	// Given metadata is a best-effort approach, and we don't halt on errors
@@ -535,7 +537,7 @@ func (i *Ingester) append(ctx context.Context, userID string, labels labelPairs,
 		}
 	}()
 	if i.stopped {
-		return fmt.Errorf("ingester stopping")
+		return errIngesterStopping
 	}
 
 	// getOrCreateSeries copies the memory for `labels`, except on the error path.
@@ -624,7 +626,7 @@ func (i *Ingester) appendMetadata(userID string, m *cortexpb.MetricMetadata) err
 	i.userStatesMtx.RLock()
 	if i.stopped {
 		i.userStatesMtx.RUnlock()
-		return fmt.Errorf("ingester stopping")
+		return errIngesterStopping
 	}
 	i.userStatesMtx.RUnlock()
 
@@ -955,7 +957,7 @@ func (i *Ingester) MetricsMetadata(ctx context.Context, req *client.MetricsMetad
 
 	userID, err := tenant.TenantID(ctx)
 	if err != nil {
-		return nil, fmt.Errorf("no user id")
+		return nil, err
 	}
 
 	userMetadata := i.getUserMetadata(userID)

pkg/ingester/ingester_v2.go

@@ -693,7 +693,7 @@ func (i *Ingester) v2Push(ctx context.Context, req *cortexpb.WriteRequest) (*cor
 
 	userID, err := tenant.TenantID(ctx)
 	if err != nil {
-		return nil, fmt.Errorf("no user id")
+		return nil, err
 	}
 
 	db, err := i.getOrCreateTSDB(userID, false)
@@ -705,7 +705,7 @@ func (i *Ingester) v2Push(ctx context.Context, req *cortexpb.WriteRequest) (*cor
 	i.userStatesMtx.RLock()
 	if i.stopped {
 		i.userStatesMtx.RUnlock()
-		return nil, fmt.Errorf("ingester stopping")
+		return nil, errIngesterStopping
 	}
 	i.userStatesMtx.RUnlock()
 

pkg/ingester/user_state.go

@@ -2,7 +2,6 @@ package ingester
 
 import (
 	"context"
-	"fmt"
 	"net/http"
 	"sync"
 	"time"
@@ -181,7 +180,7 @@ func (us *userStates) teardown() {
 func (us *userStates) getViaContext(ctx context.Context) (*userState, bool, error) {
 	userID, err := tenant.TenantID(ctx)
 	if err != nil {
-		return nil, false, fmt.Errorf("no user id")
+		return nil, false, err
 	}
 	state, ok := us.get(userID)
 	return state, ok, nil

pkg/util/validation/validate.go

@@ -37,8 +37,8 @@ const (
 	errLabelNameTooLong   = "label name too long: %.200q metric %.200q"
 	errLabelValueTooLong  = "label value too long: %.200q metric %.200q"
 	errTooManyLabels      = "series has too many labels (actual: %d, limit: %d) series: '%s'"
-	errTooOld             = "sample for '%s' has timestamp too old: %d"
-	errTooNew             = "sample for '%s' has timestamp too new: %d"
+	errTooOld             = "timestamp too old: %d metric: %.200q"
+	errTooNew             = "timestamp too new: %d metric: %.200q"
 	errDuplicateLabelName = "duplicate label name: %.200q metric %.200q"
 	errLabelsNotSorted    = "labels not sorted: %.200q metric %.200q"
 
@@ -98,12 +98,12 @@ type SampleValidationConfig interface {
 func ValidateSample(cfg SampleValidationConfig, userID string, metricName string, s cortexpb.Sample) error {
 	if cfg.RejectOldSamples(userID) && model.Time(s.TimestampMs) < model.Now().Add(-cfg.RejectOldSamplesMaxAge(userID)) {
 		DiscardedSamples.WithLabelValues(greaterThanMaxSampleAge, userID).Inc()
-		return httpgrpc.Errorf(http.StatusBadRequest, errTooOld, metricName, model.Time(s.TimestampMs))
+		return httpgrpc.Errorf(http.StatusBadRequest, errTooOld, model.Time(s.TimestampMs), metricName)
 	}
 
 	if model.Time(s.TimestampMs) > model.Now().Add(cfg.CreationGracePeriod(userID)) {
 		DiscardedSamples.WithLabelValues(tooFarInFuture, userID).Inc()
-		return httpgrpc.Errorf(http.StatusBadRequest, errTooNew, metricName, model.Time(s.TimestampMs))
+		return httpgrpc.Errorf(http.StatusBadRequest, errTooNew, model.Time(s.TimestampMs), metricName)
 	}
 
 	return nil