From 1a2c645dfa095cd20af2af1a7d40dbd41214c653 Mon Sep 17 00:00:00 2001 From: Julius Volz Date: Thu, 15 Dec 2022 12:11:25 +0100 Subject: [PATCH] Correctly handle error unwrapping in rules and remote write receiver errors.Unwrap() actually dangerously returns nil if the error does not have an Unwrap() method, which is the case in at least one of these places where I noticed that no error was being logged at all when it should have. Signed-off-by: Julius Volz --- rules/manager.go | 9 +++++++++ storage/remote/write_handler.go | 19 ++++++++++++++----- 2 files changed, 23 insertions(+), 5 deletions(-) diff --git a/rules/manager.go b/rules/manager.go index ced61ea4925..42f1b59ce0a 100644 --- a/rules/manager.go +++ b/rules/manager.go @@ -673,6 +673,9 @@ func (g *Group) Eval(ctx context.Context, ts time.Time) { rule.SetLastError(err) sp.SetStatus(codes.Error, err.Error()) unwrappedErr := errors.Unwrap(err) + if unwrappedErr == nil { + unwrappedErr = err + } switch { case errors.Is(unwrappedErr, storage.ErrOutOfOrderSample): numOutOfOrder++ @@ -700,6 +703,9 @@ func (g *Group) Eval(ctx context.Context, ts time.Time) { // Series no longer exposed, mark it stale. _, err = app.Append(0, lset, timestamp.FromTime(ts), math.Float64frombits(value.StaleNaN)) unwrappedErr := errors.Unwrap(err) + if unwrappedErr == nil { + unwrappedErr = err + } switch { case unwrappedErr == nil: case errors.Is(unwrappedErr, storage.ErrOutOfOrderSample), errors.Is(unwrappedErr, storage.ErrDuplicateSampleForTimestamp): @@ -727,6 +733,9 @@ func (g *Group) cleanupStaleSeries(ctx context.Context, ts time.Time) { // Rule that produced series no longer configured, mark it stale. _, err := app.Append(0, s, timestamp.FromTime(ts), math.Float64frombits(value.StaleNaN)) unwrappedErr := errors.Unwrap(err) + if unwrappedErr == nil { + unwrappedErr = err + } switch { case unwrappedErr == nil: case errors.Is(unwrappedErr, storage.ErrOutOfOrderSample), errors.Is(unwrappedErr, storage.ErrDuplicateSampleForTimestamp): diff --git a/storage/remote/write_handler.go b/storage/remote/write_handler.go index ad1f0f3ae1b..7a2a9951cf6 100644 --- a/storage/remote/write_handler.go +++ b/storage/remote/write_handler.go @@ -67,11 +67,14 @@ func (h *writeHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { // checkAppendExemplarError modifies the AppendExamplar's returned error based on the error cause. func (h *writeHandler) checkAppendExemplarError(err error, e exemplar.Exemplar, outOfOrderErrs *int) error { - unwrapedErr := errors.Unwrap(err) + unwrappedErr := errors.Unwrap(err) + if unwrappedErr == nil { + unwrappedErr = err + } switch { - case errors.Is(unwrapedErr, storage.ErrNotFound): + case errors.Is(unwrappedErr, storage.ErrNotFound): return storage.ErrNotFound - case errors.Is(unwrapedErr, storage.ErrOutOfOrderExemplar): + case errors.Is(unwrappedErr, storage.ErrOutOfOrderExemplar): *outOfOrderErrs++ level.Debug(h.logger).Log("msg", "Out of order exemplar", "exemplar", fmt.Sprintf("%+v", e)) return nil @@ -98,8 +101,11 @@ func (h *writeHandler) write(ctx context.Context, req *prompb.WriteRequest) (err for _, s := range ts.Samples { _, err = app.Append(0, labels, s.Timestamp, s.Value) if err != nil { - unwrapedErr := errors.Unwrap(err) - if errors.Is(unwrapedErr, storage.ErrOutOfOrderSample) || errors.Is(unwrapedErr, storage.ErrOutOfBounds) || errors.Is(unwrapedErr, storage.ErrDuplicateSampleForTimestamp) { + unwrappedErr := errors.Unwrap(err) + if unwrappedErr == nil { + unwrappedErr = err + } + if errors.Is(err, storage.ErrOutOfOrderSample) || errors.Is(unwrappedErr, storage.ErrOutOfBounds) || errors.Is(unwrappedErr, storage.ErrDuplicateSampleForTimestamp) { level.Error(h.logger).Log("msg", "Out of order sample from remote write", "err", err.Error(), "series", labels.String(), "timestamp", s.Timestamp) } return err @@ -123,6 +129,9 @@ func (h *writeHandler) write(ctx context.Context, req *prompb.WriteRequest) (err _, err = app.AppendHistogram(0, labels, hp.Timestamp, hs) if err != nil { unwrappedErr := errors.Unwrap(err) + if unwrappedErr == nil { + unwrappedErr = err + } // Althogh AppendHistogram does not currently return ErrDuplicateSampleForTimestamp there is // a note indicating its inclusion in the future. if errors.Is(unwrappedErr, storage.ErrOutOfOrderSample) || errors.Is(unwrappedErr, storage.ErrOutOfBounds) || errors.Is(unwrappedErr, storage.ErrDuplicateSampleForTimestamp) {