enable gocritic linter (jaegertracing#4412)

## Short description of the changes - enable gocritic linter - disabled dupArg, exitAfterDefer and appendAssign rules of gocritic until they are addressed. Signed-off-by: Matthieu MOREL <[email protected]> --------- Signed-off-by: Matthieu MOREL <[email protected]>
Jakob3xD · Apr 27, 2023 · b4d2581 · b4d2581
1 parent 48e1438
commit b4d2581
Show file tree

Hide file tree

Showing 28 changed files with 109 additions and 108 deletions.
diff --git a/.golangci.yml b/.golangci.yml
@@ -1,12 +1,31 @@
-run:
-  go: '1.20'
-  timeout: 10m
-  skip-dirs:
-    - mocks
-    - thrift-0.9.2
-    - .*-gen
-  skip-files:
-    - ".*.pb.go$"
+issues:
+  # Excluding configuration per-path, per-linter, per-text and per-source
+  exclude-rules:
+    # Exclude some linters from running on tests files.
+    - path: _test\.go
+      linters: [gosec]
+    - linters: [gocritic]
+      text: "dupArg"
+    - linters: [gocritic]
+      text: "exitAfterDefer"
+    - linters: [gocritic]
+      text: "appendAssign"
+  max-issues-per-linter: 0
+  max-same-issues: 0
+
+linters:
+  disable:
+    - errcheck
+  enable:
+    - bidichk
+    - contextcheck
+    - depguard
+    - gocritic
+    - gofumpt
+    - goimports
+    - gosec
+    - govet
+    - misspell
 
 linters-settings:
   depguard:
@@ -29,24 +48,12 @@ linters-settings:
   gosimple:
     go: "1.20"
 
-linters:
-  enable:
-    - contextcheck
-    - depguard
-    - gofmt
-    - gofumpt
-    - goimports
-    - gosec
-    - govet
-    - misspell
-    - bidichk
-  disable:
-    - errcheck
-
-issues:
-  # Excluding configuration per-path, per-linter, per-text and per-source
-  exclude-rules:
-    # Exclude some linters from running on tests files.
-    - path: _test\.go
-      linters:
-        - gosec
+run:
+  go: "1.20"
+  timeout: 20m
+  skip-dirs:
+    - mocks
+    - thrift-0.9.2
+    - .*-gen
+  skip-files:
+    - ".*.pb.go$"
diff --git a/cmd/agent/app/reporter/client_metrics_test.go b/cmd/agent/app/reporter/client_metrics_test.go
@@ -42,11 +42,9 @@ func (tr *clientMetricsTest) assertLog(t *testing.T, msg, clientUUID string) {
 	logs := tr.logs.FilterMessageSnippet(msg)
 	if clientUUID == "" {
 		assert.Equal(t, 0, logs.Len(), "not expecting log '%s", msg)
-	} else {
-		if assert.Equal(t, 1, logs.Len(), "expecting one log '%s'", msg) {
-			field := logs.All()[0].ContextMap()["client-uuid"]
-			assert.Equal(t, clientUUID, field, "client-uuid should be logged")
-		}
+	} else if assert.Equal(t, 1, logs.Len(), "expecting one log '%s'", msg) {
+		field := logs.All()[0].ContextMap()["client-uuid"]
+		assert.Equal(t, clientUUID, field, "client-uuid should be logged")
 	}
 }
 

diff --git a/cmd/agent/app/testutils/in_memory_reporter.go b/cmd/agent/app/testutils/in_memory_reporter.go
@@ -58,12 +58,12 @@ func (i *InMemoryReporter) EmitBatch(_ context.Context, batch *jaeger.Batch) (er
 func (i *InMemoryReporter) ZipkinSpans() []*zipkincore.Span {
 	i.mutex.Lock()
 	defer i.mutex.Unlock()
-	return i.zSpans[:]
+	return i.zSpans
 }
 
 // Spans returns accumulated spans as a copied slice
 func (i *InMemoryReporter) Spans() []*jaeger.Span {
 	i.mutex.Lock()
 	defer i.mutex.Unlock()
-	return i.jSpans[:]
+	return i.jSpans
 }
diff --git a/cmd/collector/app/zipkin/zipkindeser/json.go b/cmd/collector/app/zipkin/zipkindeser/json.go
@@ -178,7 +178,7 @@ func eToThrift(ip4 string, ip6 string, p int32, service string) (*zipkincore.End
 func port(p int32) int32 {
 	if p >= (1 << 15) {
 		// Zipkin.thrift defines port as i16, so values between (2^15 and 2^16-1) must be encoded as negative
-		p = p - (1 << 16)
+		p -= (1 << 16)
 	}
 	return p
 }

diff --git a/cmd/env/command.go b/cmd/env/command.go
@@ -101,7 +101,7 @@ func Command() *cobra.Command {
 			strings.Join(metrics.AllStorageTypes, ", "),
 		),
 	)
-	long := fmt.Sprintf(longTemplate, strings.Replace(fs.FlagUsagesWrapped(0), "      --", "\n", -1))
+	long := fmt.Sprintf(longTemplate, strings.ReplaceAll(fs.FlagUsagesWrapped(0), "      --", "\n"))
 	return &cobra.Command{
 		Use:   "env",
 		Short: "Help about environment variables.",

diff --git a/cmd/es-index-cleaner/app/index_filter.go b/cmd/es-index-cleaner/app/index_filter.go
@@ -45,12 +45,13 @@ func (i *IndexFilter) Filter(indices []client.Index) []client.Index {
 
 func (i *IndexFilter) filter(indices []client.Index) []client.Index {
 	var reg *regexp.Regexp
-	if i.Archive {
+	switch {
+	case i.Archive:
 		// archive works only for rollover
 		reg, _ = regexp.Compile(fmt.Sprintf("^%sjaeger-span-archive-\\d{6}", i.IndexPrefix))
-	} else if i.Rollover {
+	case i.Rollover:
 		reg, _ = regexp.Compile(fmt.Sprintf("^%sjaeger-(span|service|dependencies)-\\d{6}", i.IndexPrefix))
-	} else {
+	default:
 		reg, _ = regexp.Compile(fmt.Sprintf("^%sjaeger-(span|service|dependencies)-\\d{4}%s\\d{2}%s\\d{2}", i.IndexPrefix, i.IndexDateSeparator, i.IndexDateSeparator))
 	}
 

diff --git a/cmd/ingester/app/consumer/offset/concurrent_list_test.go b/cmd/ingester/app/consumer/offset/concurrent_list_test.go
@@ -122,13 +122,9 @@ func generatePermutations(arr []int64) [][]int64 {
 			for i := 0; i < n; i++ {
 				helper(arr, n-1)
 				if n%2 == 1 {
-					tmp := arr[i]
-					arr[i] = arr[n-1]
-					arr[n-1] = tmp
+					arr[i], arr[n-1] = arr[n-1], arr[i]
 				} else {
-					tmp := arr[0]
-					arr[0] = arr[n-1]
-					arr[n-1] = tmp
+					arr[0], arr[n-1] = arr[n-1], arr[0]
 				}
 			}
 		}

diff --git a/cmd/ingester/app/flags.go b/cmd/ingester/app/flags.go
@@ -140,5 +140,5 @@ func (o *Options) InitFromViper(v *viper.Viper) {
 
 // stripWhiteSpace removes all whitespace characters from a string
 func stripWhiteSpace(str string) string {
-	return strings.Replace(str, " ", "", -1)
+	return strings.ReplaceAll(str, " ", "")
 }
diff --git a/cmd/query/app/query_parser.go b/cmd/query/app/query_parser.go
@@ -83,9 +83,7 @@ type (
 )
 
 func newDurationStringParser() durationParser {
-	return func(s string) (time.Duration, error) {
-		return time.ParseDuration(s)
-	}
+	return time.ParseDuration
 }
 
 func newDurationUnitsParser(units time.Duration) durationParser {

diff --git a/cmd/query/app/query_parser_test.go b/cmd/query/app/query_parser_test.go
@@ -218,9 +218,7 @@ func TestParseDuration(t *testing.T) {
 	request, err := http.NewRequest(http.MethodGet, "x?service=foo&step=1000", nil)
 	require.NoError(t, err)
 	parser := &queryParser{
-		timeNow: func() time.Time {
-			return time.Now()
-		},
+		timeNow: time.Now,
 	}
 	mqp, err := parser.parseMetricsQueryParams(request)
 	require.NoError(t, err)
@@ -231,9 +229,7 @@ func TestParseRepeatedServices(t *testing.T) {
 	request, err := http.NewRequest(http.MethodGet, "x?service=foo&service=bar", nil)
 	require.NoError(t, err)
 	parser := &queryParser{
-		timeNow: func() time.Time {
-			return time.Now()
-		},
+		timeNow: time.Now,
 	}
 	mqp, err := parser.parseMetricsQueryParams(request)
 	require.NoError(t, err)

diff --git a/internal/metrics/prometheus/factory_test.go b/internal/metrics/prometheus/factory_test.go
@@ -202,11 +202,12 @@ func TestTimer(t *testing.T) {
 	assert.EqualValues(t, 2, m1.GetHistogram().GetSampleCount(), "%+v", m1)
 	assert.EqualValues(t, 3, m1.GetHistogram().GetSampleSum(), "%+v", m1)
 	for _, bucket := range m1.GetHistogram().GetBucket() {
-		if bucket.GetUpperBound() < 1 {
+		switch {
+		case bucket.GetUpperBound() < 1:
 			assert.EqualValues(t, 0, bucket.GetCumulativeCount())
-		} else if bucket.GetUpperBound() < 2 {
+		case bucket.GetUpperBound() < 2:
 			assert.EqualValues(t, 1, bucket.GetCumulativeCount())
-		} else {
+		default:
 			assert.EqualValues(t, 2, bucket.GetCumulativeCount())
 		}
 	}
@@ -215,11 +216,12 @@ func TestTimer(t *testing.T) {
 	assert.EqualValues(t, 2, m2.GetHistogram().GetSampleCount(), "%+v", m2)
 	assert.EqualValues(t, 7, m2.GetHistogram().GetSampleSum(), "%+v", m2)
 	for _, bucket := range m2.GetHistogram().GetBucket() {
-		if bucket.GetUpperBound() < 3 {
+		switch {
+		case bucket.GetUpperBound() < 3:
 			assert.EqualValues(t, 0, bucket.GetCumulativeCount())
-		} else if bucket.GetUpperBound() < 4 {
+		case bucket.GetUpperBound() < 4:
 			assert.EqualValues(t, 1, bucket.GetCumulativeCount())
-		} else {
+		default:
 			assert.EqualValues(t, 2, bucket.GetCumulativeCount())
 		}
 	}
@@ -321,11 +323,12 @@ func TestHistogram(t *testing.T) {
 	assert.EqualValues(t, 2, m1.GetHistogram().GetSampleCount(), "%+v", m1)
 	assert.EqualValues(t, 3, m1.GetHistogram().GetSampleSum(), "%+v", m1)
 	for _, bucket := range m1.GetHistogram().GetBucket() {
-		if bucket.GetUpperBound() < 1 {
+		switch {
+		case bucket.GetUpperBound() < 1:
 			assert.EqualValues(t, 0, bucket.GetCumulativeCount())
-		} else if bucket.GetUpperBound() < 2 {
+		case bucket.GetUpperBound() < 2:
 			assert.EqualValues(t, 1, bucket.GetCumulativeCount())
-		} else {
+		default:
 			assert.EqualValues(t, 2, bucket.GetCumulativeCount())
 		}
 	}
@@ -334,11 +337,12 @@ func TestHistogram(t *testing.T) {
 	assert.EqualValues(t, 2, m2.GetHistogram().GetSampleCount(), "%+v", m2)
 	assert.EqualValues(t, 7, m2.GetHistogram().GetSampleSum(), "%+v", m2)
 	for _, bucket := range m2.GetHistogram().GetBucket() {
-		if bucket.GetUpperBound() < 3 {
+		switch {
+		case bucket.GetUpperBound() < 3:
 			assert.EqualValues(t, 0, bucket.GetCumulativeCount())
-		} else if bucket.GetUpperBound() < 4 {
+		case bucket.GetUpperBound() < 4:
 			assert.EqualValues(t, 1, bucket.GetCumulativeCount())
-		} else {
+		default:
 			assert.EqualValues(t, 2, bucket.GetCumulativeCount())
 		}
 	}

diff --git a/model/span.go b/model/span.go
@@ -182,7 +182,7 @@ func (f *Flags) SetFirehose() {
 }
 
 func (f *Flags) setFlags(bit Flags) {
-	*f = *f | bit
+	*f |= bit
 }
 
 // IsSampled returns true if the Flags denote sampling

diff --git a/pkg/bearertoken/http.go b/pkg/bearertoken/http.go
@@ -35,15 +35,16 @@ func PropagationHandler(logger *zap.Logger, h http.Handler) http.Handler {
 		if authHeaderValue != "" {
 			headerValue := strings.Split(authHeaderValue, " ")
 			token := ""
-			if len(headerValue) == 2 {
+			switch {
+			case len(headerValue) == 2:
 				// Make sure we only capture bearer token , not other types like Basic auth.
 				if headerValue[0] == "Bearer" {
 					token = headerValue[1]
 				}
-			} else if len(headerValue) == 1 {
+			case len(headerValue) == 1:
 				// Treat the entire value as a token.
 				token = authHeaderValue
-			} else {
+			default:
 				logger.Warn("Invalid authorization header value, skipping token propagation")
 			}
 			h.ServeHTTP(w, r.WithContext(ContextWithBearerToken(ctx, token)))

diff --git a/pkg/config/tlscfg/certpool_windows.go b/pkg/config/tlscfg/certpool_windows.go
@@ -32,7 +32,8 @@ const (
 // workaround https://github.com/golang/go/issues/16736
 // fix borrowed from Sensu: https://github.com/sensu/sensu-go/pull/4018
 func appendCerts(rootCAs *x509.CertPool) (*x509.CertPool, error) {
-	storeHandle, err := syscall.CertOpenSystemStore(0, syscall.StringToUTF16Ptr("Root"))
+	name, _ := syscall.UTF16PtrFromString("Root")
+	storeHandle, err := syscall.CertOpenSystemStore(0, name)
 	if err != nil {
 		return nil, err
 	}

diff --git a/pkg/config/tlscfg/flags.go b/pkg/config/tlscfg/flags.go
@@ -113,5 +113,5 @@ func (c ServerFlagsConfig) InitFromViper(v *viper.Viper) (Options, error) {
 
 // stripWhiteSpace removes all whitespace characters from a string
 func stripWhiteSpace(str string) string {
-	return strings.Replace(str, " ", "", -1)
+	return strings.ReplaceAll(str, " ", "")
 }
diff --git a/pkg/metrics/metrics.go b/pkg/metrics/metrics.go
@@ -77,12 +77,13 @@ func Init(m interface{}, factory Factory, globalTags map[string]string) error {
 			}
 		}
 		if bucketString := field.Tag.Get("buckets"); bucketString != "" {
-			if field.Type.AssignableTo(timerPtrType) {
+			switch {
+			case field.Type.AssignableTo(timerPtrType):
 				// TODO: Parse timer duration buckets
 				return fmt.Errorf(
 					"Field [%s]: Buckets are not currently initialized for timer metrics",
 					field.Name)
-			} else if field.Type.AssignableTo(histogramPtrType) {
+			case field.Type.AssignableTo(histogramPtrType):
 				bucketValues := strings.Split(bucketString, ",")
 				for _, bucket := range bucketValues {
 					b, err := strconv.ParseFloat(bucket, 64)
@@ -93,41 +94,42 @@ func Init(m interface{}, factory Factory, globalTags map[string]string) error {
 					}
 					buckets = append(buckets, b)
 				}
-			} else {
+			default:
 				return fmt.Errorf(
 					"Field [%s]: Buckets should only be defined for Timer and Histogram metric types",
 					field.Name)
 			}
 		}
 		help := field.Tag.Get("help")
 		var obj interface{}
-		if field.Type.AssignableTo(counterPtrType) {
+		switch {
+		case field.Type.AssignableTo(counterPtrType):
 			obj = factory.Counter(Options{
 				Name: metric,
 				Tags: tags,
 				Help: help,
 			})
-		} else if field.Type.AssignableTo(gaugePtrType) {
+		case field.Type.AssignableTo(gaugePtrType):
 			obj = factory.Gauge(Options{
 				Name: metric,
 				Tags: tags,
 				Help: help,
 			})
-		} else if field.Type.AssignableTo(timerPtrType) {
+		case field.Type.AssignableTo(timerPtrType):
 			// TODO: Add buckets once parsed (see TODO above)
 			obj = factory.Timer(TimerOptions{
 				Name: metric,
 				Tags: tags,
 				Help: help,
 			})
-		} else if field.Type.AssignableTo(histogramPtrType) {
+		case field.Type.AssignableTo(histogramPtrType):
 			obj = factory.Histogram(HistogramOptions{
 				Name:    metric,
 				Tags:    tags,
 				Help:    help,
 				Buckets: buckets,
 			})
-		} else {
+		default:
 			return fmt.Errorf(
 				"Field %s is not a pointer to timer, gauge, or counter",
 				field.Name)

diff --git a/pkg/queue/bounded_queue_test.go b/pkg/queue/bounded_queue_test.go
@@ -189,13 +189,10 @@ func TestResizeUp(t *testing.T) {
 		if !resized { // we'll have a second consumer once the queue is resized
 			// signal that the worker is processing
 			firstConsumer.Done()
-		} else {
 			// once we release the lock, we might end up with multiple calls to reach this
-			if !released {
-				secondConsumer.Done()
-			}
+		} else if !released {
+			secondConsumer.Done()
 		}
-
 		// wait until we are signaled that we can finish
 		releaseConsumers.Wait()
 	})

diff --git a/plugin/metrics/prometheus/options.go b/plugin/metrics/prometheus/options.go
@@ -91,5 +91,5 @@ func (config *namespaceConfig) getTLSFlagsConfig() tlscfg.ClientFlagsConfig {
 
 // stripWhiteSpace removes all whitespace characters from a string.
 func stripWhiteSpace(str string) string {
-	return strings.Replace(str, " ", "", -1)
+	return strings.ReplaceAll(str, " ", "")
 }