Enable errorlint and check for plain error comparisons (grafana#2907)

* Use errorlint to check for plain error comparisons and type assertions * Explicitly load golangci-lint config file * Fix linter warnings about plain error comparison * Fix linter warnings about plain error type assertions * Implement tooManySpansError.Error() with pointer receiver * Implement traceTooLargeError.Error() with pointer receiver * Implement unsupportedError.Error() with pointer receiver * Implement ParseError.Error() with pointer receiver * Improve comment for errorlint in .golangci.yml * Implement validationError.Error() with pointer receiver * Check for ParseError using errors.As() * Remove custom multi error implementation * More readable condition for errors.Is(err, context.Canceled) * Implement validationError.Unwrap()
dimitris4000 · Sep 21, 2023 · 98760ac · 98760ac
1 parent ff5520f
commit 98760ac
Show file tree

Hide file tree

Showing 101 changed files with 347 additions and 304 deletions.
diff --git a/.golangci.yml b/.golangci.yml
@@ -43,6 +43,7 @@ output:
 linters:
   enable:
     - errcheck
+    - errorlint
     - goconst
     - gofmt
     - goimports
@@ -60,6 +61,8 @@ linters:
 linters-settings:
   errcheck:
     exclude: ./.errcheck-exclude.txt
+  errorlint:
+    errorf: false # don't check whether fmt.Errorf uses %w to wrap errors
 
 issues:
   exclude:

diff --git a/Makefile b/Makefile
@@ -154,7 +154,7 @@ check-jsonnetfmt: jsonnetfmt
 
 .PHONY: lint
 lint:
-	$(LINT) run
+	$(LINT) run --config .golangci.yml
 
 ### Docker Images
 

diff --git a/cmd/tempo-cli/cmd-convert-parquet-2to3.go b/cmd/tempo-cli/cmd-convert-parquet-2to3.go
@@ -15,6 +15,7 @@ import (
 	"github.com/grafana/tempo/tempodb/encoding/vparquet2"
 	"github.com/grafana/tempo/tempodb/encoding/vparquet3"
 	"github.com/parquet-go/parquet-go"
+	"github.com/pkg/errors"
 )
 
 type convertParquet2to3 struct {
@@ -117,7 +118,7 @@ func (i *parquetIterator) Next(_ context.Context) (common.ID, *tempopb.Trace, er
 	}
 
 	_, err := i.r.Read(traces)
-	if err == io.EOF {
+	if errors.Is(err, io.EOF) {
 		return nil, nil, io.EOF
 	}
 	if err != nil {

diff --git a/cmd/tempo-cli/cmd-gen-bloom.go b/cmd/tempo-cli/cmd-gen-bloom.go
@@ -8,6 +8,8 @@ import (
 	"os"
 	"strconv"
 
+	"github.com/pkg/errors"
+
 	"github.com/google/uuid"
 	willf_bloom "github.com/willf/bloom"
 
@@ -43,7 +45,7 @@ func ReplayBlockAndDoForEachRecord(meta *backend.BlockMeta, filepath string, for
 	objectRW := v2.NewObjectReaderWriter()
 	for {
 		buffer, _, err := dataReader.NextPage(buffer)
-		if err == io.EOF {
+		if errors.Is(err, io.EOF) {
 			break
 		}
 		if err != nil {
@@ -64,7 +66,7 @@ func ReplayBlockAndDoForEachRecord(meta *backend.BlockMeta, filepath string, for
 			}
 		}
 
-		if iterErr != io.EOF {
+		if !errors.Is(iterErr, io.EOF) {
 			return iterErr
 		}
 	}

diff --git a/cmd/tempo-cli/cmd-gen-index.go b/cmd/tempo-cli/cmd-gen-index.go
@@ -8,6 +8,7 @@ import (
 	"os"
 
 	"github.com/google/uuid"
+	"github.com/pkg/errors"
 
 	"github.com/grafana/tempo/tempodb/backend"
 	"github.com/grafana/tempo/tempodb/encoding/common"
@@ -40,7 +41,7 @@ func ReplayBlockAndGetRecords(meta *backend.BlockMeta, filepath string) ([]v2.Re
 	currentOffset := uint64(0)
 	for {
 		buffer, pageLen, err := dataReader.NextPage(buffer)
-		if err == io.EOF {
+		if errors.Is(err, io.EOF) {
 			break
 		}
 		if err != nil {
@@ -60,7 +61,7 @@ func ReplayBlockAndGetRecords(meta *backend.BlockMeta, filepath string) ([]v2.Re
 			lastID = id
 		}
 
-		if iterErr != io.EOF {
+		if !errors.Is(iterErr, io.EOF) {
 			replayError = iterErr
 			break
 		}

diff --git a/cmd/tempo-cli/cmd-list-block.go b/cmd/tempo-cli/cmd-list-block.go
@@ -13,6 +13,8 @@ import (
 
 	"github.com/dustin/go-humanize"
 	"github.com/google/uuid"
+	"github.com/pkg/errors"
+
 	"github.com/grafana/tempo/pkg/model"
 	"github.com/grafana/tempo/pkg/tempopb"
 	"github.com/grafana/tempo/pkg/util"
@@ -52,12 +54,12 @@ func dumpBlock(r tempodb_backend.Reader, c tempodb_backend.Compactor, tenantID s
 	id := uuid.MustParse(blockID)
 
 	meta, err := r.BlockMeta(context.TODO(), id, tenantID)
-	if err != nil && err != tempodb_backend.ErrDoesNotExist {
+	if err != nil && !errors.Is(err, tempodb_backend.ErrDoesNotExist) {
 		return err
 	}
 
 	compactedMeta, err := c.CompactedBlockMeta(id, tenantID)
-	if err != nil && err != tempodb_backend.ErrDoesNotExist {
+	if err != nil && !errors.Is(err, tempodb_backend.ErrDoesNotExist) {
 		return err
 	}
 
@@ -132,7 +134,7 @@ func dumpBlock(r tempodb_backend.Reader, c tempodb_backend.Compactor, tenantID s
 		prevID := make([]byte, 16)
 		for {
 			objID, obj, err := iter.NextBytes(ctx)
-			if err == io.EOF {
+			if errors.Is(err, io.EOF) {
 				break
 			} else if err != nil {
 				return err
@@ -242,7 +244,7 @@ func addKey(kvp kvPairs, key string, count int) {
 	kvp[key] = v
 }
 
-func addVal(kvp kvPairs, key string, val string, count int) {
+func addVal(kvp kvPairs, key, val string, count int) {
 	v := kvp[key]
 	stats, ok := v.all[val]
 	if !ok {

diff --git a/cmd/tempo-cli/cmd-list-column.go b/cmd/tempo-cli/cmd-list-column.go
@@ -7,6 +7,7 @@ import (
 
 	"github.com/google/uuid"
 	"github.com/parquet-go/parquet-go"
+	"github.com/pkg/errors"
 
 	pq "github.com/grafana/tempo/pkg/parquetquery"
 	"github.com/grafana/tempo/tempodb/encoding/vparquet"
@@ -54,7 +55,7 @@ func (cmd *listColumnCmd) Run(ctx *globalOptions) error {
 		buffer := make([]parquet.Value, 10000)
 		for {
 			pg, err := pages.ReadPage()
-			if pg == nil || err == io.EOF {
+			if pg == nil || errors.Is(err, io.EOF) {
 				break
 			}
 
@@ -66,7 +67,7 @@ func (cmd *listColumnCmd) Run(ctx *globalOptions) error {
 				}
 
 				// check for EOF after processing any returned data
-				if err == io.EOF {
+				if errors.Is(err, io.EOF) {
 					break
 				}
 				// todo: better error handling

diff --git a/cmd/tempo-cli/cmd-query-blocks.go b/cmd/tempo-cli/cmd-query-blocks.go
@@ -8,6 +8,8 @@ import (
 
 	"github.com/gogo/protobuf/jsonpb"
 	"github.com/google/uuid"
+	"github.com/pkg/errors"
+
 	"github.com/grafana/tempo/pkg/boundedwaitgroup"
 	"github.com/grafana/tempo/pkg/model/trace"
 	"github.com/grafana/tempo/pkg/tempopb"
@@ -127,13 +129,13 @@ func queryBlock(ctx context.Context, r backend.Reader, c backend.Compactor, bloc
 	}
 
 	meta, err := r.BlockMeta(context.Background(), id, tenantID)
-	if err != nil && err != backend.ErrDoesNotExist {
+	if err != nil && !errors.Is(err, backend.ErrDoesNotExist) {
 		return nil, err
 	}
 
-	if err == backend.ErrDoesNotExist {
+	if errors.Is(err, backend.ErrDoesNotExist) {
 		compactedMeta, err := c.CompactedBlockMeta(id, tenantID)
-		if err != nil && err != backend.ErrDoesNotExist {
+		if err != nil && !errors.Is(err, backend.ErrDoesNotExist) {
 			return nil, err
 		}
 

diff --git a/cmd/tempo-cli/cmd-query-search.go b/cmd/tempo-cli/cmd-query-search.go
@@ -5,10 +5,12 @@ import (
 	"io"
 	"time"
 
-	"github.com/grafana/dskit/user"
-	"github.com/grafana/tempo/pkg/tempopb"
+	"github.com/pkg/errors"
 	"google.golang.org/grpc"
 	"google.golang.org/grpc/credentials/insecure"
+
+	"github.com/grafana/dskit/user"
+	"github.com/grafana/tempo/pkg/tempopb"
 )
 
 type querySearchCmd struct {
@@ -62,7 +64,7 @@ func (cmd *querySearchCmd) Run(_ *globalOptions) error {
 				return err
 			}
 		}
-		if err == io.EOF {
+		if errors.Is(err, io.EOF) {
 			return nil
 		}
 		if err != nil {

diff --git a/cmd/tempo-cli/cmd-search.go b/cmd/tempo-cli/cmd-search.go
@@ -6,6 +6,8 @@ import (
 	"time"
 
 	"github.com/google/uuid"
+	"github.com/pkg/errors"
+
 	"github.com/grafana/tempo/pkg/boundedwaitgroup"
 	"github.com/grafana/tempo/pkg/tempopb"
 	"github.com/grafana/tempo/tempodb"
@@ -69,7 +71,7 @@ func (cmd *searchBlocksCmd) Run(opts *globalOptions) error {
 
 			// search here
 			meta, err := r.BlockMeta(ctx, id2, cmd.TenantID)
-			if err == backend.ErrDoesNotExist {
+			if errors.Is(err, backend.ErrDoesNotExist) {
 				return
 			}
 			if err != nil {

diff --git a/cmd/tempo-cli/shared.go b/cmd/tempo-cli/shared.go
@@ -9,6 +9,8 @@ import (
 	"time"
 
 	"github.com/google/uuid"
+	"github.com/pkg/errors"
+
 	"github.com/grafana/tempo/pkg/boundedwaitgroup"
 	"github.com/grafana/tempo/tempodb/backend"
 )
@@ -105,14 +107,14 @@ func loadBlock(r backend.Reader, c backend.Compactor, tenantID string, id uuid.U
 	}
 
 	meta, err := r.BlockMeta(context.Background(), id, tenantID)
-	if err == backend.ErrDoesNotExist && !includeCompacted {
+	if errors.Is(err, backend.ErrDoesNotExist) && !includeCompacted {
 		return nil, nil
-	} else if err != nil && err != backend.ErrDoesNotExist {
+	} else if err != nil && !errors.Is(err, backend.ErrDoesNotExist) {
 		return nil, err
 	}
 
 	compactedMeta, err := c.CompactedBlockMeta(id, tenantID)
-	if err != nil && err != backend.ErrDoesNotExist {
+	if err != nil && !errors.Is(err, backend.ErrDoesNotExist) {
 		return nil, err
 	}
 

diff --git a/cmd/tempo-vulture/main.go b/cmd/tempo-vulture/main.go
@@ -16,6 +16,7 @@ import (
 	"github.com/go-test/deep"
 	jaeger_grpc "github.com/jaegertracing/jaeger/cmd/agent/app/reporter/grpc"
 	zaplogfmt "github.com/jsternberg/zap-logfmt"
+	"github.com/pkg/errors"
 	"github.com/prometheus/client_golang/prometheus/promhttp"
 	"go.uber.org/zap"
 	"go.uber.org/zap/zapcore"
@@ -467,7 +468,7 @@ func queryTrace(client *httpclient.Client, info *util.TraceInfo) (traceMetrics,
 
 	trace, err := client.QueryTrace(hexID)
 	if err != nil {
-		if err == util.ErrTraceNotFound {
+		if errors.Is(err, util.ErrTraceNotFound) {
 			tm.notFoundByID++
 		} else {
 			tm.requestFailed++

diff --git a/cmd/tempo/app/app.go b/cmd/tempo/app/app.go
@@ -207,14 +207,14 @@ func (t *App) Run() error {
 		// let's find out which module failed
 		for m, s := range serviceMap {
 			if s == service {
-				switch service.FailureCase() {
-				case modules.ErrStopProcess:
-					level.Info(log.Logger).Log("msg", "received stop signal via return error", "module", m, "err", service.FailureCase())
-				case context.Canceled:
-				default:
-					level.Error(log.Logger).Log("msg", "module failed", "module", m, "err", service.FailureCase())
+				err = service.FailureCase()
+				if errors.Is(err, modules.ErrStopProcess) {
+					level.Info(log.Logger).Log("msg", "received stop signal via return error", "module", m, "err", err)
+				} else if errors.Is(err, context.Canceled) {
+					return
+				} else if err != nil {
+					level.Error(log.Logger).Log("msg", "module failed", "module", m, "err", err)
 				}
-
 				return
 			}
 		}

diff --git a/cmd/tempo/app/modules.go b/cmd/tempo/app/modules.go
@@ -263,7 +263,7 @@ func (t *App) initIngester() (services.Service, error) {
 func (t *App) initGenerator() (services.Service, error) {
 	t.cfg.Generator.Ring.ListenPort = t.cfg.Server.GRPCListenPort
 	genSvc, err := generator.New(&t.cfg.Generator, t.Overrides, prometheus.DefaultRegisterer, log.Logger)
-	if err == generator.ErrUnconfigured && t.cfg.Target != MetricsGenerator { // just warn if we're not running the metrics-generator
+	if errors.Is(err, generator.ErrUnconfigured) && t.cfg.Target != MetricsGenerator { // just warn if we're not running the metrics-generator
 		level.Warn(log.Logger).Log("msg", "metrics-generator is not configured.", "err", err)
 		return services.NewIdleService(nil, nil), nil
 	}

diff --git a/integration/util.go b/integration/util.go
@@ -347,7 +347,7 @@ func SearchStreamAndAssertTrace(t *testing.T, client tempopb.StreamingQuerierCli
 				break
 			}
 		}
-		if err == io.EOF {
+		if errors.Is(err, io.EOF) {
 			break
 		}
 		require.NoError(t, err)

diff --git a/modules/frontend/handler.go b/modules/frontend/handler.go
@@ -156,15 +156,12 @@ func copyHeader(dst, src http.Header) {
 // httpgrpc errors can bubble up to here and should be translated to http errors. It returns
 // httpgrpc error.
 func writeError(w http.ResponseWriter, err error) error {
-	switch err {
-	case context.Canceled:
+	if errors.Is(err, context.Canceled) {
 		err = errCanceled
-	case context.DeadlineExceeded:
+	} else if errors.Is(err, context.DeadlineExceeded) {
 		err = errDeadlineExceeded
-	default:
-		if isRequestBodyTooLarge(err) {
-			err = errRequestEntityTooLarge
-		}
+	} else if isRequestBodyTooLarge(err) {
+		err = errRequestEntityTooLarge
 	}
 	server.WriteError(w, err)
 	return err

diff --git a/modules/frontend/v1/frontend.go b/modules/frontend/v1/frontend.go
@@ -377,7 +377,7 @@ func (f *Frontend) queueRequest(ctx context.Context, req *request) error {
 	f.activeUsers.UpdateUserTimestamp(joinedTenantID, now)
 
 	err = f.requestQueue.EnqueueRequest(joinedTenantID, req, maxQueriers)
-	if err == queue.ErrTooManyRequests {
+	if errors.Is(err, queue.ErrTooManyRequests) {
 		return errTooManyRequest
 	}
 	return err

diff --git a/modules/generator/processor/localblocks/processor.go b/modules/generator/processor/localblocks/processor.go
@@ -91,8 +91,8 @@ func (p *Processor) PushSpans(_ context.Context, req *tempopb.PushSpansRequest)
 
 	for _, batch := range req.Batches {
 		if batch = filterBatch(batch); batch != nil {
-			switch err := p.liveTraces.Push(batch, p.Cfg.MaxLiveTraces); err {
-			case errMaxExceeded:
+			err := p.liveTraces.Push(batch, p.Cfg.MaxLiveTraces)
+			if errors.Is(err, errMaxExceeded) {
 				metricDroppedTraces.WithLabelValues(p.tenant, reasonLiveTracesExceeded).Inc()
 			}
 		}
@@ -562,7 +562,7 @@ func (p *Processor) reloadBlocks() error {
 	for _, id := range ids {
 		meta, err := r.BlockMeta(ctx, id, t)
 
-		if err == backend.ErrDoesNotExist {
+		if errors.Is(err, backend.ErrDoesNotExist) {
 			// Partially written block, delete and continue
 			err = l.ClearBlock(id, t)
 			if err != nil {
-Original file line number
+Diff line change
@@ Expand Up @@
     				break
     			}
     		}
-    		if err == io.EOF {
+    		if errors.Is(err, io.EOF) {
     			break
     		}
     		require.NoError(t, err)
@@ Expand Down @@