From 3ab1bc86b6d4e46934658757fbb0cafd462d9366 Mon Sep 17 00:00:00 2001 From: Nayef Ghattas Date: Thu, 14 Nov 2024 10:29:17 +0100 Subject: [PATCH] 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 | 18 ++------ Makefile | 2 +- containermetadata/containermetadata.go | 46 +++++++++++---------- containermetadata/containermetadata_test.go | 14 +++---- helpers.go | 18 ++++---- main.go | 3 +- reporter/datadog_reporter.go | 8 ++-- reporter/symbol_uploader.go | 10 ++--- tools/merge-licenses-copyrights.go | 4 +- 9 files changed, 56 insertions(+), 67 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 9388cbe..acf8111 100644 --- a/.golangci.yml +++ b/.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: diff --git a/Makefile b/Makefile index d67217d..95f4f7b 100644 --- a/Makefile +++ b/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)" \ No newline at end of file + bash -c "cd /app && make VERSION=$(VERSION)" diff --git a/containermetadata/containermetadata.go b/containermetadata/containermetadata.go index 40779b9..486955c 100644 --- a/containermetadata/containermetadata.go +++ b/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() diff --git a/containermetadata/containermetadata_test.go b/containermetadata/containermetadata_test.go index 5f01207..7568677 100644 --- a/containermetadata/containermetadata_test.go +++ b/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) diff --git a/helpers.go b/helpers.go index fdc3b54..457f01d 100644 --- a/helpers.go +++ b/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 diff --git a/main.go b/main.go index c9ff9ad..7e017b2 100644 --- a/main.go +++ b/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(), diff --git a/reporter/datadog_reporter.go b/reporter/datadog_reporter.go index e53141c..5aadf59 100644 --- a/reporter/datadog_reporter.go +++ b/reporter/datadog_reporter.go @@ -16,17 +16,17 @@ import ( "strconv" "time" - "github.com/DataDog/dd-otel-host-profiler/containermetadata" "github.com/DataDog/zstd" lru "github.com/elastic/go-freelru" pprofile "github.com/google/pprof/profile" - log "github.com/sirupsen/logrus" - "github.com/zeebo/xxh3" - "github.com/open-telemetry/opentelemetry-ebpf-profiler/libpf" "github.com/open-telemetry/opentelemetry-ebpf-profiler/libpf/xsync" "github.com/open-telemetry/opentelemetry-ebpf-profiler/process" "github.com/open-telemetry/opentelemetry-ebpf-profiler/reporter" + log "github.com/sirupsen/logrus" + "github.com/zeebo/xxh3" + + "github.com/DataDog/dd-otel-host-profiler/containermetadata" ) // Assert that we implement the full Reporter interface. diff --git a/reporter/symbol_uploader.go b/reporter/symbol_uploader.go index 912df1b..ecb9d2b 100644 --- a/reporter/symbol_uploader.go +++ b/reporter/symbol_uploader.go @@ -28,12 +28,11 @@ import ( "github.com/DataDog/zstd" lru "github.com/elastic/go-freelru" - log "github.com/sirupsen/logrus" - "github.com/open-telemetry/opentelemetry-ebpf-profiler/libpf" "github.com/open-telemetry/opentelemetry-ebpf-profiler/libpf/pfelf" "github.com/open-telemetry/opentelemetry-ebpf-profiler/libpf/readatbuf" "github.com/open-telemetry/opentelemetry-ebpf-profiler/process" + log "github.com/sirupsen/logrus" ) const uploadCacheSize = 16384 @@ -356,12 +355,11 @@ func (d *DatadogSymbolUploader) copySymbols(ctx context.Context, inputPath, outp func (d *DatadogSymbolUploader) uploadSymbols(ctx context.Context, symbolFile *os.File, e *executableMetadata) error { - req, err := d.buildSymbolUploadRequest(symbolFile, e) + req, err := d.buildSymbolUploadRequest(ctx, symbolFile, e) if err != nil { return fmt.Errorf("failed to build symbol upload request: %w", err) } - req = req.WithContext(ctx) resp, err := d.client.Do(req) if err != nil { return err @@ -378,7 +376,7 @@ func (d *DatadogSymbolUploader) uploadSymbols(ctx context.Context, symbolFile *o return nil } -func (d *DatadogSymbolUploader) buildSymbolUploadRequest(symbolFile *os.File, +func (d *DatadogSymbolUploader) buildSymbolUploadRequest(ctx context.Context, symbolFile *os.File, e *executableMetadata) (*http.Request, error) { b := new(bytes.Buffer) @@ -422,7 +420,7 @@ func (d *DatadogSymbolUploader) buildSymbolUploadRequest(symbolFile *os.File, return nil, fmt.Errorf("failed to close zstd writer: %w", err) } - r, err := http.NewRequest(http.MethodPost, d.intakeURL, b) + r, err := http.NewRequestWithContext(ctx, http.MethodPost, d.intakeURL, b) if err != nil { return nil, fmt.Errorf("failed to create request: %w", err) } diff --git a/tools/merge-licenses-copyrights.go b/tools/merge-licenses-copyrights.go index 1453966..3d31453 100644 --- a/tools/merge-licenses-copyrights.go +++ b/tools/merge-licenses-copyrights.go @@ -247,7 +247,7 @@ func scanPkg(pkg string) (string, error) { p := filepath.Join(pkg, entry.Name()) c, err := scanFile(p) if err != nil { - return "unknown", fmt.Errorf("error scanning %s: %s", p, err) + return "unknown", fmt.Errorf("error scanning %s: %w", p, err) } for _, c := range c { if _, dup := dedup[c]; dup { @@ -272,7 +272,7 @@ func isCopyright(line string) (string, bool) { func scanFile(fname string) ([]string, error) { f, err := os.Open(fname) if err != nil { - return nil, fmt.Errorf("cannot open %s: %s", fname, err) + return nil, fmt.Errorf("cannot open %s: %w", fname, err) } defer f.Close() var copyrights []string