Improve linting (#527)

* Print linter version only in CI

* Update golangci-lint to v1.59.1

* Also lint code for build-flag 'tools'

* Add linter config .golangci-lint.yml

* Satisfy usestdlibvars linter

* Satisfy testifylint linter

* Satisfy perfsprint linter

* Satisfy misspell linter

* Satisfy whitespace linter

* Satisfy bodyclose linter

* Satisfy revive linter

* Satisfy gosec linter

* Satisfy dupl linter

* Satisfy govet linter

* Satisfy unparam linter

* Satisfy musttag linter

* Satisfy gocritic linter

* Satisfy goimports linter

Author: rockdaboot

.golangci.yml

@@ -0,0 +1,129 @@
+service:
+  golangci-lint-version: 1.59.x
+
+issues:
+  exclude-dirs:
+    - artifacts
+    - build-targets
+    - design
+    - docker-images
+    - docs
+    - etc
+    - experiments
+    - infrastructure
+    - legal
+    - libpf-rs
+    - mocks
+    - pf-code-indexing-service/cibackend/gomock_*
+    - pf-debug-metadata-service/dmsbackend/gomock_*
+    - pf-host-agent/support/ci-kernels
+    - pf-storage-backend/storagebackend/gomock_*
+    - scratch
+    - systemtests/benchmarks/_outdata
+    - target
+    - virt-tests
+    - vm-images
+
+linters:
+  enable-all: true
+  disable:
+    # Disabled because of
+    #   - too many non-sensical warnings
+    #   - not relevant for us
+    #   - false positives
+    # 
+    # "might be worth fixing" means we should investigate/fix in the mid term
+    - canonicalheader
+    - contextcheck # might be worth fixing
+    - cyclop
+    - depguard
+    - dupword
+    - err113
+    - errorlint # might be worth fixing
+    - exhaustive
+    - exhaustruct
+    - forbidigo
+    - forcetypeassert # might be worth fixing
+    - funlen
+    - gci # might be worth fixing
+    - gochecknoglobals
+    - gochecknoinits
+    - gocognit
+    - goconst
+    - gocyclo
+    - godot
+    - godox # complains about TODO etc
+    - gofumpt
+    - gomnd
+    - gomoddirectives
+    - inamedparam
+    - interfacebloat
+    - ireturn
+    - lll
+    - maintidx
+    - makezero
+    - mnd
+    - nestif
+    - nlreturn
+    - noctx # might be worth fixing
+    - nolintlint
+    - nonamedreturns
+    - paralleltest
+    - protogetter
+    - tagalign
+    - tagliatelle
+    - testpackage
+    - thelper
+    - varnamelen
+    - wastedassign
+    - wsl
+    - wrapcheck
+    # the following linters are deprecated
+    - execinquery
+    # we don't want to change code to Go 1.22+ yet
+    - intrange
+    - copyloopvar
+
+linters-settings:
+  goconst:
+    min-len: 2
+    min-occurrences: 2
+  gocritic:
+    enabled-tags:
+      - diagnostic
+      - experimental
+      - opinionated
+      - performance
+      - style
+    disabled-checks:
+      - dupImport # https://github.com/go-critic/go-critic/issues/845
+      - ifElseChain
+      - octalLiteral
+      - whyNoLint
+      - wrapperFunc
+      - sloppyReassign
+      - uncheckedInlineErr # Experimental rule with high false positive rate.
+
+      # Broken with Go 1.18 feature (https://github.com/golangci/golangci-lint/issues/2649):
+      - hugeParam
+      - rangeValCopy
+      - typeDefFirst
+      - paramTypeCombine
+  gocyclo:
+    min-complexity: 15
+  govet:
+    enable-all: true
+    disable:
+      - fieldalignment
+    settings:
+      printf: # analyzer name, run `go tool vet help` to see all analyzers
+        funcs: # run `go tool vet help printf` to see available settings for `printf` analyzer
+          - debug,debugf,debugln
+          - error,errorf,errorln
+          - fatal,fatalf,fataln
+          - info,infof,infoln
+          - log,logf,logln
+          - warn,warnf,warnln
+          - print,printf,println,sprint,sprintf,sprintln,fprint,fprintf,fprintln
+  misspell:
+    locale: US

Makefile

@@ -2,7 +2,7 @@ SHELL = /bin/bash -eo pipefail
 
 GORELEASER_VERSION = "v1.19.2"
 GO_LICENSER_VERSION = "v0.4.0"
-GOLANGCI_LINT_VERSION = "v1.54.2"
+GOLANGCI_LINT_VERSION = "v1.59.1"
 export DOCKER_IMAGE_NAME = observability/apm-lambda-extension
 export DOCKER_REGISTRY = docker.elastic.co
 
@@ -39,8 +39,8 @@ lint-prep:
 
 .PHONY: lint
 lint:
-	@go run github.com/golangci/golangci-lint/cmd/golangci-lint@$(GOLANGCI_LINT_VERSION) version
-	@go run github.com/golangci/golangci-lint/cmd/golangci-lint@$(GOLANGCI_LINT_VERSION) run
+	@if [ "$(CI)" != "" ]; then go run github.com/golangci/golangci-lint/cmd/golangci-lint@$(GOLANGCI_LINT_VERSION) version; fi
+	@go run github.com/golangci/golangci-lint/cmd/golangci-lint@$(GOLANGCI_LINT_VERSION) run --build-tags tools
 
 NOTICE.txt: go.mod
 	@bash ./scripts/notice.sh

accumulator/batch.go

@@ -31,7 +31,7 @@ var (
 	// ErrMetadataUnavailable is returned when a lambda data is added to
 	// the batch without metadata being set.
 	ErrMetadataUnavailable = errors.New("metadata is not yet available")
-	// ErrBatchFull signfies that the batch has reached full capacity
+	// ErrBatchFull signifies that the batch has reached full capacity
 	// and cannot accept more entries.
 	ErrBatchFull = errors.New("batch is full")
 	// ErrInvalidEncoding is returned for any APMData that is encoded
@@ -184,7 +184,7 @@ func (b *Batch) AddAgentData(apmData APMData) error {
 		return ErrBatchFull
 	}
 	if b.currentlyExecutingRequestID == "" {
-		return fmt.Errorf("lifecycle error, currently executing requestID is not set")
+		return errors.New("lifecycle error, currently executing requestID is not set")
 	}
 	inc, ok := b.invocations[b.currentlyExecutingRequestID]
 	if !ok {
@@ -218,10 +218,10 @@ func (b *Batch) AddAgentData(apmData APMData) error {
 // OnLambdaLogRuntimeDone prepares the data for the invocation to be shipped
 // to APM Server. It accepts requestID and status of the invocation both of
 // which can be retrieved after parsing `platform.runtimeDone` event.
-func (b *Batch) OnLambdaLogRuntimeDone(reqID, status string, time time.Time) error {
+func (b *Batch) OnLambdaLogRuntimeDone(reqID, status string, endTime time.Time) error {
 	b.mu.Lock()
 	defer b.mu.Unlock()
-	return b.finalizeInvocation(reqID, status, time)
+	return b.finalizeInvocation(reqID, status, endTime)
 }
 
 func (b *Batch) OnPlatformStart(reqID string) {
@@ -236,6 +236,8 @@ func (b *Batch) PlatformStartReqID() string {
 // platform.report event the batch will cleanup any datastructure for the request
 // ID. It will return some of the function metadata to allow the caller to enrich
 // the report metrics.
+//
+//nolint:gocritic
 func (b *Batch) OnPlatformReport(reqID string) (string, int64, time.Time, error) {
 	b.mu.Lock()
 	defer b.mu.Unlock()
@@ -249,7 +251,7 @@ func (b *Batch) OnPlatformReport(reqID string) (string, int64, time.Time, error)
 
 // OnShutdown flushes the data for shipping to APM Server by finalizing all
 // the invocation in the batch. If we haven't received a platform.runtimeDone
-// event for an invocation so far we won't be able to recieve it in time thus
+// event for an invocation so far we won't be able to receive it in time thus
 // the status needs to be guessed based on the available information.
 func (b *Batch) OnShutdown(status string) error {
 	b.mu.Lock()
@@ -259,8 +261,8 @@ func (b *Batch) OnShutdown(status string) error {
 		// TODO: @lahsivjar Is it possible to tweak the extension lifecycle in
 		// a way that we receive the platform.report metric for a invocation
 		// consistently and enrich the metrics with reported values?
-		time := time.Unix(0, inc.DeadlineMs*int64(time.Millisecond))
-		if err := b.finalizeInvocation(inc.RequestID, status, time); err != nil {
+		endTime := time.Unix(0, inc.DeadlineMs*int64(time.Millisecond))
+		if err := b.finalizeInvocation(inc.RequestID, status, endTime); err != nil {
 			return err
 		}
 		delete(b.invocations, inc.RequestID)
@@ -315,12 +317,12 @@ func (b *Batch) ToAPMData() APMData {
 	}
 }
 
-func (b *Batch) finalizeInvocation(reqID, status string, time time.Time) error {
+func (b *Batch) finalizeInvocation(reqID, status string, endTime time.Time) error {
 	inc, ok := b.invocations[reqID]
 	if !ok {
 		return fmt.Errorf("invocation for requestID %s does not exist", reqID)
 	}
-	proxyTxn, err := inc.MaybeCreateProxyTxn(status, time)
+	proxyTxn, err := inc.MaybeCreateProxyTxn(status, endTime)
 	if err != nil {
 		return err
 	}

accumulator/batch_test.go

@@ -32,7 +32,7 @@ const metadata = `{"metadata":{"service":{"agent":{"name":"apm-lambda-extension"
 func TestAdd(t *testing.T) {
 	t.Run("empty-without-metadata", func(t *testing.T) {
 		b := NewBatch(1, time.Hour)
-		assert.Error(t, b.AddLambdaData([]byte(`{"log":{}}`)), ErrMetadataUnavailable)
+		assert.ErrorIs(t, b.AddLambdaData([]byte(`{"log":{}}`)), ErrMetadataUnavailable)
 	})
 	t.Run("empty-with-metadata", func(t *testing.T) {
 		b := NewBatch(1, time.Hour)
@@ -96,7 +96,7 @@ func TestLifecycle(t *testing.T) {
 	reqID := "test-req-id"
 	fnARN := "test-fn-arn"
 	lambdaData := `{"log":{"message":"this is log"}}`
-	txnData := fmt.Sprintf(`{"transaction":{"id":"%s"}}`, "023d90ff77f13b9f")
+	txnData := `{"transaction":{"id":"023d90ff77f13b9f"}}`
 	ts := time.Date(2022, time.October, 1, 1, 1, 1, 0, time.UTC)
 	txnDur := time.Second
 
@@ -276,12 +276,12 @@ func TestFindEventType(t *testing.T) {
 func generateCompleteTxn(t *testing.T, src, result, outcome string, d time.Duration) string {
 	t.Helper()
 	tmp, err := sjson.SetBytes([]byte(src), "transaction.result", result)
-	assert.NoError(t, err)
+	require.NoError(t, err)
 	tmp, err = sjson.SetBytes(tmp, "transaction.duration", d.Milliseconds())
-	assert.NoError(t, err)
+	require.NoError(t, err)
 	if outcome != "" {
 		tmp, err = sjson.SetBytes(tmp, "transaction.outcome", outcome)
-		assert.NoError(t, err)
+		require.NoError(t, err)
 	}
 	return string(tmp)
 }

accumulator/invocation.go

@@ -63,7 +63,7 @@ func (inc *Invocation) NeedProxyTransaction() bool {
 // has not sent the corresponding transaction to the extension. The
 // proxy transaction will not be created if the invocation has
 // already been finalized or the agent has reported the transaction.
-func (inc *Invocation) MaybeCreateProxyTxn(status string, time time.Time) ([]byte, error) {
+func (inc *Invocation) MaybeCreateProxyTxn(status string, endTime time.Time) ([]byte, error) {
 	if !inc.NeedProxyTransaction() {
 		return nil, nil
 	}
@@ -74,7 +74,7 @@ func (inc *Invocation) MaybeCreateProxyTxn(status string, time time.Time) ([]byt
 	// Transaction duration cannot be known in partial transaction payload. Estimate
 	// the duration based on the time provided. Time can be based on the runtimeDone
 	// log record or function deadline.
-	duration := time.Sub(inc.Timestamp)
+	duration := endTime.Sub(inc.Timestamp)
 	txn, err = sjson.SetBytes(txn, "transaction.duration", duration.Milliseconds())
 	if err != nil {
 		return nil, err

accumulator/invocation_test.go

@@ -23,6 +23,7 @@ import (
 	"time"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestCreateProxyTransaction(t *testing.T) {
@@ -90,8 +91,8 @@ func TestCreateProxyTransaction(t *testing.T) {
 				TransactionObserved: tc.txnObserved,
 			}
 			result, err := inc.MaybeCreateProxyTxn(tc.runtimeDoneStatus, ts.Add(txnDur))
-			assert.Nil(t, err)
-			if len(tc.output) > 0 {
+			require.NoError(t, err)
+			if tc.output != "" {
 				assert.JSONEq(t, tc.output, string(result))
 			} else {
 				assert.Nil(t, result)

apmproxy/apmserver.go

@@ -56,7 +56,7 @@ func (c *Client) ForwardApmData(ctx context.Context) error {
 	for {
 		select {
 		case <-ctx.Done():
-			c.logger.Debug("Invocation context cancelled, not processing any more agent data")
+			c.logger.Debug("Invocation context canceled, not processing any more agent data")
 			return nil
 		case data := <-c.AgentDataChannel:
 			if err := c.forwardAgentData(ctx, data); err != nil {
@@ -312,11 +312,11 @@ func (c *Client) ComputeGracePeriod() time.Duration {
 	// The grace period for the first reconnection count was 0 but that
 	// leads to collisions with multiple environments.
 	if c.ReconnectionCount == 0 {
-		gracePeriod := rand.Float64() * 5
+		gracePeriod := rand.Float64() * 5 //nolint:gosec
 		return time.Duration(gracePeriod * float64(time.Second))
 	}
 	gracePeriodWithoutJitter := math.Pow(math.Min(float64(c.ReconnectionCount), 6), 2)
-	jitter := rand.Float64()/5 - 0.1
+	jitter := rand.Float64()/5 - 0.1 //nolint:gosec
 	return time.Duration((gracePeriodWithoutJitter + jitter*gracePeriodWithoutJitter) * float64(time.Second))
 }
 
@@ -334,7 +334,7 @@ func (c *Client) ResetFlush() {
 	c.flushCh = make(chan struct{})
 }
 
-// WaitForFlush returns a channel that is closed when the agent has signalled that
+// WaitForFlush returns a channel that is closed when the agent has signaled that
 // the Lambda invocation has completed, and there is no more APM data coming.
 func (c *Client) WaitForFlush() <-chan struct{} {
 	c.flushMutex.Lock()