Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: change linting rules and refactor #40

Merged
merged 2 commits into from
Nov 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 4 additions & 14 deletions .golangci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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:
Expand Down
2 changes: 1 addition & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -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)"
46 changes: 24 additions & 22 deletions containermetadata/containermetadata.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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 (
Expand Down Expand Up @@ -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")
)

Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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)

Expand All @@ -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,
Expand All @@ -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)
}
}

Expand Down Expand Up @@ -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)
}

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
}

Expand Down Expand Up @@ -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
Expand All @@ -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()
Expand Down
14 changes: 7 additions & 7 deletions containermetadata/containermetadata_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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) })
Expand Down Expand Up @@ -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)
Expand Down
18 changes: 9 additions & 9 deletions helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -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-._/]+$`)
Expand Down Expand Up @@ -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)
Expand All @@ -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{
Expand All @@ -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
}

Expand Down Expand Up @@ -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())
Expand All @@ -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
Expand Down
3 changes: 1 addition & 2 deletions main.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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(),
Expand Down
Loading