*: Enable revive and gofumpt linters (#251)

* Upgrade linter

Fix linter pointed issues

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

* Fix additional errors

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

* Add commands to Makefile

Update docs

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

* Fix goimprots issue

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

* Install gofumpt

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

.github/workflows/build.yml

@@ -63,7 +63,10 @@ jobs:
         run: sudo apt-get install llvm libelf-dev
 
       - name: Install golangci-lint
-        run: curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(go env GOPATH)/bin v1.42.0
+        run: curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(go env GOPATH)/bin v1.44.2
+
+      - name: Install gofumpt
+        run: go install mvdan.cc/gofumpt@latest
 
       - name: Check out code into the Go module directory
         uses: actions/checkout@v2

.golangci.yml

@@ -3,8 +3,42 @@ run:
   skip-dirs:
     - internal/pprof
     - internal/go
+
+linters:
+  enable:
+    - depguard
+    - godot
+    - gofumpt
+    - goimports
+    - revive
+    - whitespace
+
+issues:
+  exclude-rules:
+    - path: _test.go
+      linters:
+        - errcheck
+
 linters-settings:
+  depguard:
+    list-type: blacklist
+    include-go-root: true
+    packages-with-error-message:
+      - sync/atomic: "Use go.uber.org/atomic instead of sync/atomic"
+      - github.com/stretchr/testify/assert: "Use github.com/stretchr/testify/require instead of github.com/stretchr/testify/assert"
+      - github.com/go-kit/kit/log: "Use github.com/go-kit/log instead of github.com/go-kit/kit/log"
+      - github.com/pkg/errors: "Use fmt.Errorf instead"
   errcheck:
     exclude: ./.errcheck_excludes.txt
+  goimports:
+    local-prefixes: github.com/parca-dev/parca-agent
+  gofumpt:
+    extra-rules: true
   misspell:
     locale: US
+  revive:
+    rules:
+      # https://github.com/mgechev/revive/blob/master/RULES_DESCRIPTIONS.md#unexported-return
+      - name: unexported-return
+        severity: warning
+        disabled: true

CONTRIBUTING.md

@@ -122,6 +122,8 @@ This is roughly what the contribution workflow should look like:
 - Create a topic branch from where you want to base your work (usually master).
 - Make commits of logical units.
 - Make sure the tests pass, and add any new tests as appropriate.
+- Make sure the code is properly formatted. (`make format` could be useful here.)
+- Make sure the code is properly linted. (`make lint` could be useful here.)
 - Make sure your commit messages follow the commit guidelines (see below).
 - Push your changes to a topic branch in your fork of the repository.
 - Submit a pull request to the original repository.

Makefile

@@ -227,14 +227,15 @@ README.md: $(CMD_EMBEDMD) $(OUT_DIR)/help.txt deploy/manifests
 	$(CMD_EMBEDMD) -w README.md
 
 .PHONY: format
-format: go-fmt check-license
+format: go/fmt check-license
 
-.PHONY: c-fmt
-c-fmt:
+.PHONY: c/fmt
+c/fmt:
 	clang-format -i --style=GNU $(BPF_SRC)
 
-.PHONY: go-fmt
-go-fmt:
+.PHONY: go/fmt
+go/fmt:
+	gofumpt -l -w $(shell go list ./... | grep -E -v "internal/pprof|internal/go" | sed 's#^github.com/parca-dev/parca-agent/##')
 	go fmt $(shell go list ./... | grep -E -v "internal/pprof|internal/go")
 
 .PHONY: check-license

cmd/debug-info/main.go

@@ -114,7 +114,7 @@ func main() {
 			if err := os.RemoveAll(flags.Extract.OutputDir); err != nil {
 				return fmt.Errorf("failed to clean output dir, %s: %w", flags.Extract.OutputDir, err)
 			}
-			if err := os.MkdirAll(flags.Extract.OutputDir, 0755); err != nil {
+			if err := os.MkdirAll(flags.Extract.OutputDir, 0o755); err != nil {
 				return fmt.Errorf("failed to create output dir, %s: %w", flags.Extract.OutputDir, err)
 			}
 			buildIDFiles := map[string]string{}

cmd/parca-agent/main.go

@@ -204,7 +204,6 @@ func main() {
 						Error:        profiler.LastError(),
 						Link:         fmt.Sprintf("/query?%s", q.Encode()),
 					})
-
 				}
 			}
 

env.sh

@@ -10,3 +10,5 @@ go install github.com/google/go-jsonnet/cmd/jsonnetfmt@latest
 go install github.com/jsonnet-bundler/jsonnet-bundler/cmd/jb@latest
 
 go install github.com/campoy/embedmd@latest
+
+go install mvdan.cc/gofumpt@latest

pkg/agent/profile_listener.go

@@ -25,32 +25,32 @@ import (
 	"google.golang.org/grpc"
 )
 
-type ProfileListener struct {
+type profileListener struct {
 	next      profilestorepb.ProfileStoreServiceClient
 	observers []*observer
 	omtx      *sync.Mutex
 	logger    log.Logger
 }
 
-func NewProfileListener(logger log.Logger, next profilestorepb.ProfileStoreServiceClient) *ProfileListener {
-	return &ProfileListener{
+func NewProfileListener(logger log.Logger, next profilestorepb.ProfileStoreServiceClient) *profileListener {
+	return &profileListener{
 		next:      next,
 		observers: []*observer{},
 		omtx:      &sync.Mutex{},
 		logger:    logger,
 	}
 }
 
-func (l *ProfileListener) WriteRaw(ctx context.Context, r *profilestorepb.WriteRawRequest, opts ...grpc.CallOption) (*profilestorepb.WriteRawResponse, error) {
-	l.ObserveProfile(r)
+func (l *profileListener) WriteRaw(ctx context.Context, r *profilestorepb.WriteRawRequest, opts ...grpc.CallOption) (*profilestorepb.WriteRawResponse, error) {
+	l.observeProfile(r)
 	return l.next.WriteRaw(ctx, r, opts...)
 }
 
 type observer struct {
 	f func(*profilestorepb.WriteRawRequest)
 }
 
-func (l *ProfileListener) ObserveProfile(r *profilestorepb.WriteRawRequest) {
+func (l *profileListener) observeProfile(r *profilestorepb.WriteRawRequest) {
 	l.omtx.Lock()
 	defer l.omtx.Unlock()
 
@@ -59,7 +59,7 @@ func (l *ProfileListener) ObserveProfile(r *profilestorepb.WriteRawRequest) {
 	}
 }
 
-func (l *ProfileListener) Observe(f func(*profilestorepb.WriteRawRequest)) *observer {
+func (l *profileListener) observe(f func(*profilestorepb.WriteRawRequest)) *observer {
 	l.omtx.Lock()
 	defer l.omtx.Unlock()
 
@@ -70,7 +70,7 @@ func (l *ProfileListener) Observe(f func(*profilestorepb.WriteRawRequest)) *obse
 	return o
 }
 
-func (l *ProfileListener) RemoveObserver(o *observer) {
+func (l *profileListener) removeObserver(o *observer) {
 	l.omtx.Lock()
 	defer l.omtx.Unlock()
 
@@ -87,12 +87,11 @@ func (l *ProfileListener) RemoveObserver(o *observer) {
 	}
 }
 
-func (l *ProfileListener) NextMatchingProfile(ctx context.Context, matchers []*labels.Matcher) (*profile.Profile, error) {
+func (l *profileListener) NextMatchingProfile(ctx context.Context, matchers []*labels.Matcher) (*profile.Profile, error) {
 	pCh := make(chan []byte)
 	defer close(pCh)
 
-	o := l.Observe(func(r *profilestorepb.WriteRawRequest) {
-
+	o := l.observe(func(r *profilestorepb.WriteRawRequest) {
 		var searchedSeries *profilestorepb.RawProfileSeries
 
 		for _, series := range r.Series {
@@ -116,7 +115,7 @@ func (l *ProfileListener) NextMatchingProfile(ctx context.Context, matchers []*l
 			pCh <- searchedSeries.Samples[len(searchedSeries.Samples)-1].RawProfile
 		}
 	})
-	defer l.RemoveObserver(o)
+	defer l.removeObserver(o)
 
 	select {
 	case p := <-pCh:

pkg/agent/write_client.go

@@ -15,7 +15,6 @@ package agent
 
 import (
 	"context"
-
 	"sync"
 	"time"
 
@@ -90,7 +89,7 @@ func (b *Batcher) batchLoop(ctx context.Context) error {
 	return nil
 }
 
-func isEqualLabel(a *profilestorepb.LabelSet, b *profilestorepb.LabelSet) bool {
+func isEqualLabel(a, b *profilestorepb.LabelSet) bool {
 	if len(a.Labels) != len(b.Labels) {
 		return false
 	}

pkg/agent/write_client_test.go

@@ -23,7 +23,7 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-func isEqualSample(a []*profilestorepb.RawSample, b []*profilestorepb.RawSample) bool {
+func isEqualSample(a, b []*profilestorepb.RawSample) bool {
 	if len(a) != len(b) {
 		return false
 	}
@@ -37,7 +37,7 @@ func isEqualSample(a []*profilestorepb.RawSample, b []*profilestorepb.RawSample)
 	return ret
 }
 
-func compareProfileSeries(a []*profilestorepb.RawProfileSeries, b []*profilestorepb.RawProfileSeries) bool {
+func compareProfileSeries(a, b []*profilestorepb.RawProfileSeries) bool {
 	if len(a) != len(b) {
 		return false
 	}
@@ -70,15 +70,16 @@ func TestWriteClient(t *testing.T) {
 
 	ctx := context.Background()
 
-	samples1 := []*profilestorepb.RawSample{{RawProfile: []byte{11, 04, 96}}}
+	samples1 := []*profilestorepb.RawSample{{RawProfile: []byte{11, 0o4, 96}}}
 	samples2 := []*profilestorepb.RawSample{{RawProfile: []byte{15, 11, 95}}}
 
 	t.Run("insertFirstProfile", func(t *testing.T) {
 		_, err := batcher.WriteRaw(ctx, &profilestorepb.WriteRawRequest{
 			Series: []*profilestorepb.RawProfileSeries{{
 				Labels:  &labelset1,
 				Samples: samples1,
-			}}})
+			}},
+		})
 
 		series := []*profilestorepb.RawProfileSeries{{
 			Labels:  &labelset1,
@@ -94,14 +95,15 @@ func TestWriteClient(t *testing.T) {
 			Series: []*profilestorepb.RawProfileSeries{{
 				Labels:  &labelset2,
 				Samples: samples2,
-			}}})
+			}},
+		})
 
 		series := []*profilestorepb.RawProfileSeries{
-			&profilestorepb.RawProfileSeries{
+			{
 				Labels:  &labelset1,
 				Samples: samples1,
 			},
-			&profilestorepb.RawProfileSeries{
+			{
 				Labels:  &labelset2,
 				Samples: samples2,
 			},
@@ -116,14 +118,15 @@ func TestWriteClient(t *testing.T) {
 			Series: []*profilestorepb.RawProfileSeries{{
 				Labels:  &labelset1,
 				Samples: samples2,
-			}}})
+			}},
+		})
 
 		series := []*profilestorepb.RawProfileSeries{
-			&profilestorepb.RawProfileSeries{
+			{
 				Labels:  &labelset1,
 				Samples: append(samples1, samples2...),
 			},
-			&profilestorepb.RawProfileSeries{
+			{
 				Labels:  &labelset2,
 				Samples: samples2,
 			},

pkg/buildid/buildid.go

@@ -21,8 +21,8 @@ import (
 	"os"
 
 	"github.com/cespare/xxhash/v2"
-	gobuildid "github.com/parca-dev/parca-agent/internal/go/buildid"
 
+	gobuildid "github.com/parca-dev/parca-agent/internal/go/buildid"
 	"github.com/parca-dev/parca-agent/internal/pprof/elfexec"
 )
 

pkg/containerutils/containerd/containerd.go

@@ -27,23 +27,23 @@ import (
 )
 
 const (
-	DEFAULT_SOCKET_PATH     = "/run/containerd/containerd.sock"
-	DEFAULT_K3S_SOCKET_PATH = "/run/k3s/containerd/containerd.sock"
-	DEFAULT_TIMEOUT         = 2 * time.Second
+	DefaultSocketPath    = "/run/containerd/containerd.sock"
+	DefaultK3SSocketPath = "/run/k3s/containerd/containerd.sock"
+	DefaultTimeout       = 2 * time.Second
 )
 
-type ContainerdClient struct {
+type Client struct {
 	conn   *grpc.ClientConn
 	client pb.RuntimeServiceClient
 }
 
-func NewContainerdClient(path string) (*ContainerdClient, error) {
+func NewContainerdClient(path string) (*Client, error) {
 	conn, err := grpc.Dial(
 		path,
 		grpc.WithTransportCredentials(insecure.NewCredentials()),
 		grpc.WithContextDialer(func(ctx context.Context, addr string) (net.Conn, error) {
 			var d net.Dialer
-			ctx, cancel := context.WithTimeout(ctx, DEFAULT_TIMEOUT)
+			ctx, cancel := context.WithTimeout(ctx, DefaultTimeout)
 			defer cancel()
 
 			d.LocalAddr = nil // if you have a local addr, add it here
@@ -56,21 +56,21 @@ func NewContainerdClient(path string) (*ContainerdClient, error) {
 	}
 
 	client := pb.NewRuntimeServiceClient(conn)
-	return &ContainerdClient{
+	return &Client{
 		conn:   conn,
 		client: client,
 	}, nil
 }
 
-func (c *ContainerdClient) Close() error {
+func (c *Client) Close() error {
 	if c.conn != nil {
 		return c.conn.Close()
 	}
 
 	return nil
 }
 
-func (c *ContainerdClient) PidFromContainerId(containerID string) (int, error) {
+func (c *Client) PIDFromContainerID(containerID string) (int, error) {
 	if !strings.HasPrefix(containerID, "containerd://") {
 		return -1, fmt.Errorf("Invalid CRI %s, it should be containerd", containerID)
 	}

pkg/containerutils/containerutils.go

@@ -76,7 +76,7 @@ import "C"
 
 type CRIClient interface {
 	Close() error
-	PidFromContainerId(containerID string) (int, error)
+	PIDFromContainerID(containerID string) (int, error)
 }
 
 func CgroupPathV2AddMountpoint(path string) (string, error) {