search

open-telemetry / opentelemetry-collector-contrib

Contrib repository for the OpenTelemetry Collector
https://opentelemetry.io
Apache License 2.0
2.76k stars 2.19k forks source link

Enable and enforce goleak in all packages #30438

Open crobert-1 opened 6 months ago

crobert-1 commented 6 months ago

Component(s)

No response

Is your feature request related to a problem? Please describe.

There's currently no widespread testing in this repository for detecting leaking goroutines. Leaked goroutines are a result of improperly handling concurrency in golang, and a common cause of memory leaks.

Describe the solution you'd like

goleak is a testing tool used to detect leaking goroutines. This is currently being done in core (#9165) and would be useful in contrib as well.

Describe alternatives you've considered

No response

Additional context

No response

crobert-1 commented 6 months ago

A couple notes from my initial efforts:

  1. The tailsampling processor is the only existing package that already uses goleak.
  2. For goleak to work properly it's required to run from the TestMain function of testing. However, the following packages already use TestMain. I'll have to investigate to determine how to use TestMain to accomplish existing functionality as well as goleak for each of these packages. 
    ./testbed/tests/trace_test.go
./testbed/stabilitytests/trace_test.go
./testbed/correctnesstests/traces/correctness_test.go
./exporter/splunkhecexporter/integration_test.go
./exporter/kineticaexporter/exporter_metric_test.go
./exporter/datadogexporter/traces_exporter_test.go
crobert-1 commented 6 months ago

I decided to upgrade my bash script for adding goleak to make it do everything in one go, without manual intervention. This used some commands from contrib's makefile to get all go packages, and then filter down to only packages containing tests. The script also now cleans up failing tests to clearly delineate which tests are successul, which are failing.

ALL_PKG_DIRS=$(go list -f '{{ .Dir }}' ./... | sort)
ALL_SRC=$(find $ALL_PKG_DIRS -name '*.go' -not -path '*/third_party/*' -not -path '*/local/*' -type f | sort)
ALL_PKGS_WITH_TESTS=$(dirname $(find $ALL_PKG_DIRS -name '*.go' -not -path '*/third_party/*' -not -path '*/local/*' -type f | grep test.go) | sort --unique)

FAILED_PACKAGES_LIST_FILE="/Users/crobert/dev/opentelemetry-collector-contrib/failed_packages.txt"

for pkg_dir in $ALL_PKGS_WITH_TESTS;
do
  if [[ "$pkg_dir" == '/Users/crobert/dev/opentelemetry-collector-contrib' ]]; then
    echo "Skipping repo head dir"
    continue
  fi

  echo "Attempting package: $pkg_dir"
  cp ./package_test.go $pkg_dir

  PACKAGE_NAME=$(basename $pkg_dir)

  sed -i '' -e "s|package component|package $PACKAGE_NAME|g" $pkg_dir/package_test.go

  pushd . > /dev/null
  cd $pkg_dir
  echo "Running go mod tidy"
  go mod tidy
  echo "Running go test"
  go test -v . > /dev/null
  if [ $? -eq 0 ]; then
    echo "Package was successful"
  else
    echo "Package was not successful"
    echo "$pkg_dir" >> $FAILED_PACKAGES_LIST_FILE
    rm $pkg_dir/package_test.go
    go mod tidy
  fi
  popd > /dev/null
done

Sample output:

~/dev/opentelemetry-collector-contrib $ ./add_leak_test.sh 
Skipping repo head dir
Attempting package: /Users/crobert/dev/opentelemetry-collector-contrib/cmd/configschema
Running go mod tidy
Running go test
Package was not successful
...
Attempting package: /Users/crobert/dev/opentelemetry-collector-contrib/cmd/mdatagen/internal/metadata
Running go mod tidy
Running go test
Package was successful
crobert-1 commented 6 months ago

Here's the initial list of failing packages:

./cmd/configschema
./cmd/configschema/cfgmetadatagen/cfgmetadatagen
./cmd/configschema/docsgen/docsgen
./cmd/mdatagen
./cmd/mdatagen/internal/metadata
./cmd/opampsupervisor
./cmd/otelcontribcol
./cmd/oteltestbedcol
./cmd/telemetrygen
./cmd/telemetrygen/internal/e2etest
./connector/countconnector
./connector/datadogconnector
./connector/exceptionsconnector
./connector/failoverconnector
./connector/routingconnector
./connector/servicegraphconnector
./connector/spanmetricsconnector
./exporter/alertmanagerexporter
./exporter/alibabacloudlogserviceexporter
./exporter/awscloudwatchlogsexporter
./exporter/awsemfexporter
./exporter/awskinesisexporter
./exporter/awss3exporter
./exporter/awsxrayexporter
./exporter/azuredataexplorerexporter
./exporter/azuremonitorexporter
./exporter/carbonexporter
./exporter/cassandraexporter
./exporter/clickhouseexporter
./exporter/coralogixexporter
./exporter/datadogexporter
./exporter/datadogexporter/integrationtest
./exporter/datadogexporter/internal/clientutil
./exporter/datadogexporter/internal/hostmetadata
./exporter/datadogexporter/internal/hostmetadata/internal/gohai
./exporter/datadogexporter/internal/hostmetadata/provider
./exporter/datadogexporter/internal/logs
./exporter/datadogexporter/internal/metrics
./exporter/datadogexporter/internal/testutil
./exporter/datasetexporter
./exporter/dynatraceexporter
./exporter/dynatraceexporter/config
./exporter/elasticsearchexporter
./exporter/f5cloudexporter
./exporter/fileexporter
./exporter/googlecloudexporter
./exporter/googlecloudpubsubexporter
./exporter/googlemanagedprometheusexporter
./exporter/honeycombmarkerexporter
./exporter/influxdbexporter
./exporter/instanaexporter
./exporter/kafkaexporter
./exporter/kineticaexporter
./exporter/loadbalancingexporter
./exporter/logicmonitorexporter
./exporter/logicmonitorexporter/internal/logs
./exporter/logicmonitorexporter/internal/traces
./exporter/logzioexporter
./exporter/lokiexporter
./exporter/mezmoexporter
./exporter/opencensusexporter
./exporter/opensearchexporter
./exporter/prometheusexporter
./exporter/prometheusremotewriteexporter
./exporter/pulsarexporter
./exporter/sapmexporter
./exporter/sentryexporter
./exporter/signalfxexporter
./exporter/signalfxexporter/internal/correlation
./exporter/signalfxexporter/internal/dimensions
./exporter/signalfxexporter/internal/hostmetadata
./exporter/signalfxexporter/internal/translation
./exporter/skywalkingexporter
./exporter/splunkhecexporter
./exporter/sumologicexporter
./exporter/syslogexporter
./exporter/tencentcloudlogserviceexporter
./exporter/zipkinexporter
./extension/asapauthextension
./extension/awsproxy
./extension/basicauthextension
./extension/bearertokenauthextension
./extension/encoding/jaegerencodingextension
./extension/encoding/jsonlogencodingextension
./extension/encoding/otlpencodingextension
./extension/encoding/textencodingextension
./extension/encoding/zipkinencodingextension
./extension/headerssetterextension
./extension/healthcheckextension
./extension/httpforwarder
./extension/jaegerremotesampling
./extension/jaegerremotesampling/internal
./extension/oauth2clientauthextension
./extension/observer/dockerobserver
./extension/observer/ecsobserver
./extension/observer/ecstaskobserver
./extension/observer/hostobserver
./extension/observer/k8sobserver
./extension/oidcauthextension
./extension/opampextension
./extension/pprofextension
./extension/remotetapextension
./extension/sigv4authextension
./extension/storage/dbstorage
./extension/storage/filestorage
./extension/storage/storagetest
./internal/aws/ecsutil
./internal/aws/k8s/k8sclient
./internal/aws/proxy
./internal/aws/xray
./internal/aws/xray/telemetry
./internal/common/ttlmap
./internal/datadog
./internal/docker
./internal/filter/filterottl
./internal/metadataproviders/aws/ec2
./internal/metadataproviders/azure
./internal/sharedcomponent
./internal/splunk
./pkg/ottl
./pkg/ottl/e2e
./pkg/ottl/ottlfuncs
./pkg/stanza/adapter
./pkg/stanza/operator/helper
./pkg/stanza/operator/parser/regex
./pkg/stanza/operator/transformer/recombine
./processor/attributesprocessor
./processor/cumulativetodeltaprocessor
./processor/datadogprocessor
./processor/deltatorateprocessor
./processor/filterprocessor
./processor/groupbyattrsprocessor
./processor/groupbytraceprocessor
./processor/k8sattributesprocessor
./processor/k8sattributesprocessor/internal/kube
./processor/k8sattributesprocessor/internal/observability
./processor/logstransformprocessor
./processor/metricsgenerationprocessor
./processor/metricstransformprocessor
./processor/probabilisticsamplerprocessor
./processor/redactionprocessor
./processor/remotetapprocessor
./processor/resourcedetectionprocessor
./processor/resourcedetectionprocessor/internal
./processor/resourcedetectionprocessor/internal/aws/ec2
./processor/resourcedetectionprocessor/internal/aws/ecs
./processor/resourcedetectionprocessor/internal/aws/eks
./processor/resourcedetectionprocessor/internal/aws/elasticbeanstalk
./processor/resourcedetectionprocessor/internal/aws/lambda
./processor/resourcedetectionprocessor/internal/azure
./processor/resourcedetectionprocessor/internal/azure/aks
./processor/resourcedetectionprocessor/internal/docker
./processor/resourcedetectionprocessor/internal/env
./processor/resourcedetectionprocessor/internal/heroku
./processor/resourcedetectionprocessor/internal/k8snode
./processor/resourcedetectionprocessor/internal/system
./processor/resourceprocessor
./processor/routingprocessor
./processor/schemaprocessor
./processor/servicegraphprocessor
./processor/spanmetricsprocessor
./processor/spanprocessor
./processor/sumologicprocessor
./processor/tailsamplingprocessor
./processor/tailsamplingprocessor/internal/sampling
./processor/transformprocessor
./processor/transformprocessor/internal/logs
./processor/transformprocessor/internal/metrics
./processor/transformprocessor/internal/traces
./receiver/activedirectorydsreceiver
./receiver/activedirectorydsreceiver/internal/metadata
./receiver/aerospikereceiver
./receiver/aerospikereceiver/internal/metadata
./receiver/apachereceiver
./receiver/apachereceiver/internal/metadata
./receiver/apachesparkreceiver
./receiver/apachesparkreceiver/internal/metadata
./receiver/awscloudwatchmetricsreceiver
./receiver/awscloudwatchreceiver
./receiver/awscontainerinsightreceiver
./receiver/awscontainerinsightreceiver/internal/cadvisor
./receiver/awscontainerinsightreceiver/internal/ecsInfo
./receiver/awscontainerinsightreceiver/internal/host
./receiver/awscontainerinsightreceiver/internal/k8sapiserver
./receiver/awscontainerinsightreceiver/internal/stores
./receiver/awsecscontainermetricsreceiver
./receiver/awsfirehosereceiver
./receiver/awsxrayreceiver
./receiver/awsxrayreceiver/internal/udppoller
./receiver/azureblobreceiver
./receiver/azureeventhubreceiver
./receiver/azuremonitorreceiver
./receiver/azuremonitorreceiver/internal/metadata
./receiver/bigipreceiver
./receiver/bigipreceiver/internal/metadata
./receiver/carbonreceiver
./receiver/chronyreceiver
./receiver/chronyreceiver/internal/metadata
./receiver/cloudflarereceiver
./receiver/cloudfoundryreceiver
./receiver/collectdreceiver
./receiver/couchdbreceiver
./receiver/couchdbreceiver/internal/metadata
./receiver/datadogreceiver
./receiver/dockerstatsreceiver
./receiver/dockerstatsreceiver/internal/metadata
./receiver/elasticsearchreceiver
./receiver/elasticsearchreceiver/internal/metadata
./receiver/expvarreceiver
./receiver/expvarreceiver/internal/metadata
./receiver/filelogreceiver
./receiver/filereceiver
./receiver/filestatsreceiver
./receiver/filestatsreceiver/internal/metadata
./receiver/flinkmetricsreceiver
./receiver/flinkmetricsreceiver/internal/metadata
./receiver/fluentforwardreceiver
./receiver/fluentforwardreceiver/observ
./receiver/gitproviderreceiver
./receiver/gitproviderreceiver/internal/metadata
./receiver/gitproviderreceiver/internal/scraper/githubscraper
./receiver/googlecloudpubsubreceiver
./receiver/googlecloudpubsubreceiver/internal
./receiver/googlecloudspannerreceiver
./receiver/googlecloudspannerreceiver/internal/datasource
./receiver/googlecloudspannerreceiver/internal/filter
./receiver/googlecloudspannerreceiver/internal/filterfactory
./receiver/googlecloudspannerreceiver/internal/metadata
./receiver/googlecloudspannerreceiver/internal/metadataconfig
./receiver/googlecloudspannerreceiver/internal/metadataparser
./receiver/googlecloudspannerreceiver/internal/statsreader
./receiver/haproxyreceiver
./receiver/haproxyreceiver/internal/metadata
./receiver/hostmetricsreceiver
./receiver/hostmetricsreceiver/internal/scraper/cpuscraper
./receiver/hostmetricsreceiver/internal/scraper/cpuscraper/internal/metadata
./receiver/hostmetricsreceiver/internal/scraper/diskscraper
./receiver/hostmetricsreceiver/internal/scraper/diskscraper/internal/metadata
./receiver/hostmetricsreceiver/internal/scraper/filesystemscraper
./receiver/hostmetricsreceiver/internal/scraper/filesystemscraper/internal/metadata
./receiver/hostmetricsreceiver/internal/scraper/loadscraper
./receiver/hostmetricsreceiver/internal/scraper/loadscraper/internal/metadata
./receiver/hostmetricsreceiver/internal/scraper/memoryscraper
./receiver/hostmetricsreceiver/internal/scraper/memoryscraper/internal/metadata
./receiver/hostmetricsreceiver/internal/scraper/networkscraper
./receiver/hostmetricsreceiver/internal/scraper/networkscraper/internal/metadata
./receiver/hostmetricsreceiver/internal/scraper/pagingscraper
./receiver/hostmetricsreceiver/internal/scraper/pagingscraper/internal/metadata
./receiver/hostmetricsreceiver/internal/scraper/processesscraper
./receiver/hostmetricsreceiver/internal/scraper/processesscraper/internal/metadata
./receiver/hostmetricsreceiver/internal/scraper/processscraper
./receiver/hostmetricsreceiver/internal/scraper/processscraper/internal/metadata
./receiver/httpcheckreceiver
./receiver/httpcheckreceiver/internal/metadata
./receiver/iisreceiver
./receiver/iisreceiver/internal/metadata
./receiver/influxdbreceiver
./receiver/jaegerreceiver
./receiver/jmxreceiver
./receiver/jmxreceiver/internal/subprocess
./receiver/journaldreceiver
./receiver/k8sclusterreceiver
./receiver/k8sclusterreceiver/internal/clusterresourcequota
./receiver/k8sclusterreceiver/internal/collection
./receiver/k8sclusterreceiver/internal/cronjob
./receiver/k8sclusterreceiver/internal/demonset
./receiver/k8sclusterreceiver/internal/deployment
./receiver/k8sclusterreceiver/internal/hpa
./receiver/k8sclusterreceiver/internal/jobs
./receiver/k8sclusterreceiver/internal/metadata
./receiver/k8sclusterreceiver/internal/namespace
./receiver/k8sclusterreceiver/internal/node
./receiver/k8sclusterreceiver/internal/pod
./receiver/k8sclusterreceiver/internal/replicaset
./receiver/k8sclusterreceiver/internal/replicationcontroller
./receiver/k8sclusterreceiver/internal/resourcequota
./receiver/k8sclusterreceiver/internal/statefulset
./receiver/k8seventsreceiver
./receiver/k8sobjectsreceiver
./receiver/kafkametricsreceiver
./receiver/kafkametricsreceiver/internal/metadata
./receiver/kafkareceiver
./receiver/kubeletstatsreceiver
./receiver/kubeletstatsreceiver/internal/kubelet
./receiver/kubeletstatsreceiver/internal/metadata
./receiver/lokireceiver
./receiver/memcachedreceiver
./receiver/memcachedreceiver/internal/metadata
./receiver/mongodbatlasreceiver
./receiver/mongodbatlasreceiver/internal/metadata
./receiver/mongodbreceiver
./receiver/mongodbreceiver/internal/metadata
./receiver/mysqlreceiver
./receiver/mysqlreceiver/internal/metadata
./receiver/namedpipereceiver
./receiver/nginxreceiver
./receiver/nginxreceiver/internal/metadata
./receiver/nsxtreceiver
./receiver/nsxtreceiver/internal/metadata
./receiver/opencensusreceiver
./receiver/opencensusreceiver/internal/ocmetrics
./receiver/opencensusreceiver/internal/octrace
./receiver/oracledbreceiver
./receiver/oracledbreceiver/internal/metadata
./receiver/osqueryreceiver
./receiver/otlpjsonfilereceiver
./receiver/podmanreceiver
./receiver/postgresqlreceiver
./receiver/postgresqlreceiver/internal/metadata
./receiver/prometheusreceiver
./receiver/prometheusreceiver/internal
./receiver/pulsarreceiver
./receiver/purefareceiver
./receiver/purefareceiver/internal
./receiver/purefbreceiver
./receiver/purefbreceiver/internal
./receiver/rabbitmqreceiver
./receiver/rabbitmqreceiver/internal/metadata
./receiver/receivercreator
./receiver/redisreceiver
./receiver/redisreceiver/internal/metadata
./receiver/riakreceiver
./receiver/riakreceiver/internal/metadata
./receiver/saphanareceiver
./receiver/saphanareceiver/internal/metadata
./receiver/sapmreceiver
./receiver/signalfxreceiver
./receiver/simpleprometheusreceiver
./receiver/skywalkingreceiver
./receiver/snmpreceiver
./receiver/snowflakereceiver
./receiver/snowflakereceiver/internal/metadata
./receiver/solacereceiver
./receiver/splunkenterprisereceiver
./receiver/splunkenterprisereceiver/internal/metadata
./receiver/splunkhecreceiver
./receiver/sqlqueryreceiver
./receiver/sqlserverreceiver
./receiver/sqlserverreceiver/internal/metadata
./receiver/sshcheckreceiver
./receiver/sshcheckreceiver/internal/configssh
./receiver/sshcheckreceiver/internal/metadata
./receiver/statsdreceiver
./receiver/syslogreceiver
./receiver/tcplogreceiver
./receiver/udplogreceiver
./receiver/vcenterreceiver
./receiver/vcenterreceiver/internal/metadata
./receiver/wavefrontreceiver
./receiver/webhookeventreceiver
./receiver/windowseventlogreceiver
./receiver/windowsperfcountersreceiver
./receiver/zipkinreceiver
./receiver/zookeeperreceiver
./receiver/zookeeperreceiver/internal/metadata
./testbed/correctnesstests/metrics
./testbed/correctnesstests/traces
./testbed/stabilitytests
./testbed/testbed
./testbed/tests
crobert-1 commented 6 months ago

After ignoring opencensus-go issues, here's the remaining package list that needs to be enabled:

iblancasa commented 4 months ago

It seems the testing for k8seventsreceiver was added in #31011

crobert-1 commented 4 months ago

It seems the testing for k8seventsreceiver was added in #31011

I'm not seeing a goleak check in k8seventreceiver, do you see it somewhere?

iblancasa commented 4 months ago

It seems the testing for k8seventsreceiver was added in #31011

I'm not seeing a goleak check in k8seventreceiver, do you see it somewhere?

Sorry. It seems I checked it wrongly

crobert-1 commented 3 months ago

Note, after thinking some more about the component's Shutdown definition, I think a couple of my recent goleak additions were incorrect.

Doc link

Relevant part:

    // If there are any background operations running by the component they must be aborted before
    // this function returns.

Some of my recent fixes have a context cancel being called in the test itself that would properly shutdown the component's background process. This is not the correct way to handle this. The component's Shutdown itself needs to cancel a context when necesssary, to make sure no background operations are running. This will be more obviously required if in the future the collector is able to hot reload components without restarting itself entirely.

To make sure this is not the case in any components, a full search through the repo of context.WithCancel and context.WithTimeout may be necessary in tests to ensure goleak is properly catching leaks, and testing isn't just working around it.