chore: change linting rules and refactor (#40)

* chore: change linting rules and refactor

* Fix the linting rules which are worth fixing and enable the
corresponding linters
* Disable the lll linter which we find ourselves often fighting
against

* containermetadata: fix race condition in tests

stop relying on the cgroup global variable in tests, which is
racy when tests run in parallel.

instead, replace with a field in the container metadata
provider

.golangci.yml

@@ -9,21 +9,15 @@ linters:
     #   - not relevant for us
     #   - false positives
     #
-    # "might be worth fixing" means we should investigate/fix in the mid term
-    - containedctx # might be worth fixing
-    - contextcheck # might be worth fixing
     - cyclop
     - depguard
     - dupword
-    - durationcheck # might be worth fixing
     - err113
-    - errorlint # might be worth fixing
     - exhaustive
     - exhaustruct
     - forbidigo
-    - forcetypeassert # might be worth fixing
     - funlen
-    - gci # might be worth fixing
+    - gci
     - gochecknoglobals
     - gochecknoinits
     - gocognit
@@ -37,23 +31,19 @@ linters:
     - inamedparam
     - interfacebloat
     - ireturn
+    - lll
     - maintidx
     - makezero
     - mnd
     - nestif
-    - nilerr # might be worth fixing
     - nilnil
     - nlreturn
-    - noctx # might be worth fixing
     - nonamedreturns
     - paralleltest
     - protogetter
-    - sqlclosecheck # might be worth fixing
     - tagalign
     - tagliatelle
-    - testableexamples # might be worth fixing
     - testpackage
-    - tparallel # might be worth fixing
     - thelper
     - varnamelen
     - wastedassign
@@ -64,6 +54,8 @@ linters:
     - exportloopref
 
 linters-settings:
+  goimports:
+    local-prefixes: github.com/DataDog/dd-otel-host-profiler
   goconst:
     min-len: 2
     min-occurrences: 2
@@ -92,8 +84,6 @@ linters-settings:
           - log,logf,logln
           - warn,warnf,warnln
           - print,printf,println,sprint,sprintf,sprintln,fprint,fprintf,fprintln
-  lll:
-    tab-width: 4
   misspell:
     locale: US
     ignore-words:

Makefile

@@ -36,4 +36,4 @@ docker-image:
 
 profiler-in-docker: docker-image
 	docker run -v "$$PWD":/app -it --rm --user $(shell id -u):$(shell id -g) dd-otel-host-profiler \
-	   bash -c "cd /app && make VERSION=$(VERSION)"
+	   bash -c "cd /app && make VERSION=$(VERSION)"

containermetadata/containermetadata.go

@@ -33,6 +33,10 @@ import (
 	"github.com/docker/docker/api/types/container"
 	"github.com/docker/docker/client"
 	lru "github.com/elastic/go-freelru"
+	"github.com/open-telemetry/opentelemetry-ebpf-profiler/libpf"
+	"github.com/open-telemetry/opentelemetry-ebpf-profiler/metrics"
+	"github.com/open-telemetry/opentelemetry-ebpf-profiler/periodiccaller"
+	"github.com/open-telemetry/opentelemetry-ebpf-profiler/stringutil"
 	log "github.com/sirupsen/logrus"
 	"github.com/zeebo/xxh3"
 	corev1 "k8s.io/api/core/v1"
@@ -42,11 +46,6 @@ import (
 	"k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/rest"
 	"k8s.io/client-go/tools/cache"
-
-	"github.com/open-telemetry/opentelemetry-ebpf-profiler/libpf"
-	"github.com/open-telemetry/opentelemetry-ebpf-profiler/metrics"
-	"github.com/open-telemetry/opentelemetry-ebpf-profiler/periodiccaller"
-	"github.com/open-telemetry/opentelemetry-ebpf-profiler/stringutil"
 )
 
 const (
@@ -93,8 +92,6 @@ var (
 
 	containerIDPattern = regexp.MustCompile(`.+://([0-9a-f]{64})`)
 
-	cgroup = "/proc/%d/cgroup"
-
 	ErrDeferred = errors.New("lookup deferred due to previous failure")
 )
 
@@ -126,6 +123,10 @@ type containerMetadataProvider struct {
 
 	// deferredPID prevents busy loops for PIDs where the cgroup extraction fails.
 	deferredPID *lru.SyncedLRU[libpf.PID, libpf.Void]
+
+	// file pattern to extract container ID from cgroup file
+	// only used for testing
+	cgroupPattern string
 }
 
 // ContainerMetadata contains the container and/or pod metadata.
@@ -172,7 +173,7 @@ func NewContainerMetadataProvider(ctx context.Context, nodeName string, monitorI
 	containerIDCache, err := lru.NewSynced[libpf.PID, containerIDEntry](
 		containerIDCacheSize, libpf.PID.Hash32)
 	if err != nil {
-		return nil, fmt.Errorf("unable to create container id cache: %v", err)
+		return nil, fmt.Errorf("unable to create container id cache: %w", err)
 	}
 	containerIDCache.SetLifetime(containerIDCacheTimeout)
 
@@ -181,6 +182,7 @@ func NewContainerMetadataProvider(ctx context.Context, nodeName string, monitorI
 		dockerClient:     getDockerClient(),
 		containerdClient: getContainerdClient(),
 		nodeName:         nodeName,
+		cgroupPattern:    "proc/%d/cgroup",
 	}
 
 	p.deferredPID, err = lru.NewSynced[libpf.PID, libpf.Void](deferredLRUSize,
@@ -193,14 +195,14 @@ func NewContainerMetadataProvider(ctx context.Context, nodeName string, monitorI
 	if os.Getenv(kubernetesServiceHost) != "" {
 		err = createKubernetesClient(ctx, p)
 		if err != nil {
-			return nil, fmt.Errorf("failed to create kubernetes client %v", err)
+			return nil, fmt.Errorf("failed to create kubernetes client %w", err)
 		}
 	} else {
 		log.Infof("Environment variable %s not set", kubernetesServiceHost)
 		p.containerMetadataCache, err = lru.NewSynced[string, ContainerMetadata](
 			containerMetadataCacheSize, hashString)
 		if err != nil {
-			return nil, fmt.Errorf("unable to create container metadata cache: %v", err)
+			return nil, fmt.Errorf("unable to create container metadata cache: %w", err)
 		}
 	}
 
@@ -238,7 +240,7 @@ func getPodsPerNode(ctx context.Context, h *containerMetadataProvider) (int, err
 		FieldSelector: "spec.nodeName=" + h.nodeName,
 	})
 	if err != nil {
-		return 0, fmt.Errorf("failed to get kubernetes nodes for '%s': %v",
+		return 0, fmt.Errorf("failed to get kubernetes nodes for '%s': %w",
 			h.nodeName, err)
 	}
 
@@ -277,28 +279,28 @@ func createKubernetesClient(ctx context.Context, p *containerMetadataProvider) e
 
 	config, err := rest.InClusterConfig()
 	if err != nil {
-		return fmt.Errorf("failed to create in cluster configuration for Kubernetes: %v", err)
+		return fmt.Errorf("failed to create in cluster configuration for Kubernetes: %w", err)
 	}
 	p.kubeClientSet, err = kubernetes.NewForConfig(config)
 	if err != nil {
-		return fmt.Errorf("failed to create Kubernetes client: %v", err)
+		return fmt.Errorf("failed to create Kubernetes client: %w", err)
 	}
 
 	k, ok := p.kubeClientSet.(*kubernetes.Clientset)
 	if !ok {
-		return fmt.Errorf("failed to create Kubernetes client: %v", err)
+		return fmt.Errorf("failed to create Kubernetes client: %w", err)
 	}
 
 	if p.nodeName == "" {
 		p.nodeName, err = getNodeName()
 		if err != nil {
-			return fmt.Errorf("failed to get kubernetes node name; %v", err)
+			return fmt.Errorf("failed to get kubernetes node name; %w", err)
 		}
 	}
 
 	p.containerMetadataCache, err = getContainerMetadataCache(ctx, p)
 	if err != nil {
-		return fmt.Errorf("failed to create container metadata cache: %v", err)
+		return fmt.Errorf("failed to create container metadata cache: %w", err)
 	}
 
 	// Create the shared informer factory and use the client to connect to
@@ -332,7 +334,7 @@ func createKubernetesClient(ctx context.Context, p *containerMetadataProvider) e
 		},
 	})
 	if err != nil {
-		return fmt.Errorf("failed to attach event handler: %v", err)
+		return fmt.Errorf("failed to attach event handler: %w", err)
 	}
 
 	// Shutdown the informer when the context attached to this handler expires
@@ -546,7 +548,7 @@ func (p *containerMetadataProvider) getKubernetesPodMetadata(pidContainerID stri
 		FieldSelector: "spec.nodeName=" + p.nodeName,
 	})
 	if err != nil {
-		return ContainerMetadata{}, fmt.Errorf("failed to retrieve kubernetes pods, %v", err)
+		return ContainerMetadata{}, fmt.Errorf("failed to retrieve kubernetes pods, %w", err)
 	}
 
 	for j := range pods.Items {
@@ -607,7 +609,7 @@ func (p *containerMetadataProvider) getDockerContainerMetadata(pidContainerID st
 	containers, err := p.dockerClient.ContainerList(context.Background(),
 		container.ListOptions{})
 	if err != nil {
-		return ContainerMetadata{}, fmt.Errorf("failed to list docker containers, %v", err)
+		return ContainerMetadata{}, fmt.Errorf("failed to list docker containers, %w", err)
 	}
 
 	for i := range containers {
@@ -646,7 +648,7 @@ func (p *containerMetadataProvider) getContainerdContainerMetadata(pidContainerI
 	containers, err := p.containerdClient.Containers(ctx)
 	if err != nil {
 		return ContainerMetadata{},
-			fmt.Errorf("failed to get containerd containers in namespace '%s': %v",
+			fmt.Errorf("failed to get containerd containers in namespace '%s': %w",
 				fields[1], err)
 	}
 
@@ -681,7 +683,7 @@ func (p *containerMetadataProvider) lookupContainerID(pid libpf.PID) (containerI
 		return "", envUndefined, ErrDeferred
 	}
 
-	containerID, env, err = p.extractContainerIDFromFile(fmt.Sprintf(cgroup, pid))
+	containerID, env, err = p.extractContainerIDFromFile(fmt.Sprintf(p.cgroupPattern, pid))
 	if err != nil {
 		p.deferredPID.Add(pid, libpf.Void{})
 		return "", envUndefined, err
@@ -705,7 +707,7 @@ func (p *containerMetadataProvider) extractContainerIDFromFile(cgroupFilePath st
 				"Failed to get container id", cgroupFilePath)
 			return "", envUndefined, nil
 		}
-		return "", envUndefined, fmt.Errorf("failed to get container id from %s: %v",
+		return "", envUndefined, fmt.Errorf("failed to get container id from %s: %w",
 			cgroupFilePath, err)
 	}
 	defer f.Close()

containermetadata/containermetadata_test.go

@@ -20,15 +20,13 @@ import (
 	"github.com/containerd/containerd"
 	"github.com/docker/docker/client"
 	lru "github.com/elastic/go-freelru"
+	"github.com/open-telemetry/opentelemetry-ebpf-profiler/libpf"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 	corev1 "k8s.io/api/core/v1"
 	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/kubernetes/fake"
-
-	"github.com/open-telemetry/opentelemetry-ebpf-profiler/libpf"
-
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
 )
 
 func TestExtractContainerIDFromFile(t *testing.T) {
@@ -276,6 +274,8 @@ func TestGetKubernetesPodMetadata(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
+			t.Parallel()
+
 			containerMetadataCache, err := lru.NewSynced[string, ContainerMetadata](
 				containerMetadataCacheSize, hashString)
 			require.NoError(t, err)
@@ -289,12 +289,12 @@ func TestGetKubernetesPodMetadata(t *testing.T) {
 				kubeClientSet:          test.clientset,
 				dockerClient:           nil,
 				containerIDCache:       containerIDCache,
+				cgroupPattern:          "testdata/cgroupv%dkubernetes",
 			}
 			instance.deferredPID, err = lru.NewSynced[libpf.PID, libpf.Void](1024,
 				func(u libpf.PID) uint32 { return uint32(u) })
 			require.NoError(t, err)
 
-			cgroup = "testdata/cgroupv%dkubernetes"
 			meta, err := instance.GetContainerMetadata(test.pid)
 			if test.err != nil {
 				require.Error(t, err)
@@ -333,6 +333,7 @@ func BenchmarkGetKubernetesPodMetadata(b *testing.B) {
 			kubeClientSet:          clientset,
 			dockerClient:           nil,
 			containerIDCache:       containerIDCache,
+			cgroupPattern:          "/tmp/test_containermetadata_cgroup%d",
 		}
 		instance.deferredPID, err = lru.NewSynced[libpf.PID, libpf.Void](1024,
 			func(u libpf.PID) uint32 { return uint32(u) })
@@ -374,7 +375,6 @@ func BenchmarkGetKubernetesPodMetadata(b *testing.B) {
 					"%dd89697807a981b82f6245ac3a13be232c1e13435d52bc3f53060d61babe19", j)
 			require.NoError(b, err)
 
-			cgroup = "/tmp/test_containermetadata_cgroup%d"
 			opts := v1.CreateOptions{}
 			clientsetPod, err := clientset.CoreV1().Pods("default").Create(
 				context.Background(), pod, opts)

helpers.go

@@ -22,12 +22,12 @@ import (
 	"syscall"
 	"unicode"
 
-	"github.com/DataDog/dd-otel-host-profiler/reporter"
+	"github.com/jsimonetti/rtnetlink"
 	"github.com/open-telemetry/opentelemetry-ebpf-profiler/tracer"
 	log "github.com/sirupsen/logrus"
-
-	"github.com/jsimonetti/rtnetlink"
 	"golang.org/x/sys/unix"
+
+	"github.com/DataDog/dd-otel-host-profiler/reporter"
 )
 
 var ValidTagKeyRegex = regexp.MustCompile(`^[a-zA-Z][a-zA-Z0-9-._/]+$`)
@@ -77,7 +77,7 @@ func getSourceIPAddress(domain string) (net.IP, error) {
 
 	dstIPs, err := resolveDestination(domain)
 	if err != nil {
-		return nil, fmt.Errorf("unable to resolve %s: %v", domain, err)
+		return nil, fmt.Errorf("unable to resolve %s: %w", domain, err)
 	}
 	if len(dstIPs) == 0 {
 		return nil, fmt.Errorf("unable to resolve %s: no IP address", domain)
@@ -92,7 +92,7 @@ func getSourceIPAddress(domain string) (net.IP, error) {
 	for _, ip := range dstIPs {
 		addressFamily, err := addressFamily(ip)
 		if err != nil {
-			return nil, fmt.Errorf("unable to get address family for %s: %v", ip.String(), err)
+			return nil, fmt.Errorf("unable to get address family for %s: %w", ip.String(), err)
 		}
 
 		req := &rtnetlink.RouteMessage{
@@ -105,7 +105,7 @@ func getSourceIPAddress(domain string) (net.IP, error) {
 
 		routes, err := conn.Route.Get(req)
 		if err != nil {
-			lastError = fmt.Errorf("unable to get route to %s (%s): %v", domain, ip.String(), err)
+			lastError = fmt.Errorf("unable to get route to %s (%s): %w", domain, ip.String(), err)
 			continue
 		}
 
@@ -135,7 +135,7 @@ func getSourceIPAddress(domain string) (net.IP, error) {
 	}
 
 	if !found {
-		return nil, fmt.Errorf("no route found to %s: %v", domain, lastError)
+		return nil, fmt.Errorf("no route found to %s: %w", domain, lastError)
 	}
 
 	log.Debugf("Traffic to %v is routed from %v", domain, srcIP.String())
@@ -151,14 +151,14 @@ func getHostnameAndSourceIP(domain string) (hostname, sourceIP string, err error
 		if name, hostnameErr := os.Hostname(); hostnameErr == nil {
 			hostname = name
 		} else {
-			joinedErr = fmt.Errorf("failed to get hostname: %v", hostnameErr)
+			joinedErr = fmt.Errorf("failed to get hostname: %w", hostnameErr)
 		}
 
 		if srcIP, ipErr := getSourceIPAddress(domain); ipErr == nil {
 			sourceIP = srcIP.String()
 		} else {
 			joinedErr = errors.Join(joinedErr,
-				fmt.Errorf("failed to get source IP: %v", ipErr))
+				fmt.Errorf("failed to get source IP: %w", ipErr))
 		}
 
 		return joinedErr

main.go

@@ -26,7 +26,6 @@ import (
 	"github.com/open-telemetry/opentelemetry-ebpf-profiler/tracer"
 	tracertypes "github.com/open-telemetry/opentelemetry-ebpf-profiler/tracer/types"
 	"github.com/open-telemetry/opentelemetry-ebpf-profiler/util"
-
 	log "github.com/sirupsen/logrus"
 	"github.com/tklauser/numcpus"
 	"golang.org/x/sys/unix"
@@ -71,7 +70,7 @@ func startTraceHandling(ctx context.Context, rep otelreporter.TraceReporter,
 	traceCh := make(chan *host.Trace)
 
 	if err := trc.StartMapMonitors(ctx, traceCh); err != nil {
-		return fmt.Errorf("failed to start map monitors: %v", err)
+		return fmt.Errorf("failed to start map monitors: %w", err)
 	}
 
 	_, err := tracehandler.Start(ctx, rep, trc.TraceProcessor(),