[exporter/signalfx] Fix goroutine leaks #32781

crobert-1 · 2024-04-30T18:15:10Z

Description:

This change is a refactor to allow us to enable goleak checks in each of the exporter's sub-directories, as well as the signalfx receiver (which uses the signalfxexporter in a test). The main idea is to use start/shutdown for each package's functionality, rather than relying on the top level context cancel of the exporter. The addition of start and shutdowns for each package means we can more closely control the lifecycle of each as needed, and enable goleak for each package.

The memory leaks being fixed are:

Call the TTLMap.Shutdown so the ticker is properly stopped: t.prevPts.Shutdown().
Don't block waiting for a request if the context is cancelled.

Link to tracking issue:
#30438

crobert-1 · 2024-04-30T18:17:27Z

exporter/signalfxexporter/exporter.go

@@ -60,7 +60,6 @@ type signalfxExporter struct {
 	hostMetadataSyncer *hostmetadata.Syncer
 	converter          *translation.MetricsConverter
 	dimClient          *dimensions.DimensionClient
-	cancelFn           func()


The cancelFn was originally responsible for shutting down all sub-packages. I've refactored to enable each package to start and shutdown their own functionality, as requested by the caller. (More explanation in PR description).

crobert-1 · 2024-04-30T18:18:58Z

exporter/signalfxexporter/internal/dimensions/dimclient.go

@@ -32,7 +32,7 @@ import (
 // updates are currently not done by this port.
 type DimensionClient struct {
 	sync.RWMutex
-	ctx           context.Context


Contexts aren't supposed to be stored, per go's documentation, so we should store the cancel instead.

This is unrelated right? Can you please move it to a separate PR?

This is a part of the refactor as a whole, removing the stored context in favor of the cancel allows the dimclient to be able to start and stop by its own calls. If we leave it as is we're relying on the exporter's top level context cancel (which technically works), but then means we can't call goleak in this package and we're relying on another package to manage its lifecycle without being obvious.

I'd prefer to keep it as is, unless you think this is too much change (or too confusing) in one PR.

crobert-1 · 2024-04-30T18:20:04Z

exporter/signalfxexporter/internal/dimensions/dimclient.go

@@ -319,7 +325,7 @@ func (dc *DimensionClient) makePatchRequest(dim *DimensionUpdate) (*http.Request
 	}

 	req, err := http.NewRequestWithContext(
-		context.Background(),
+		ctx,


This allows the dimension client to shut itself down when requested, instead of relying on an outside caller to handle its lifecycle.

crobert-1 · 2024-04-30T18:24:19Z

exporter/signalfxexporter/internal/translation/delta_translator.go

 	return &deltaTranslator{prevPts: m}
 }

+func (t *deltaTranslator) start() {
+	if t.prevPts != nil {
+		t.prevPts.Start()


This should be the equivalent functionality as before, just starting when start is called, instead of when the object is created. This is a better UX, from my understanding.

.chloggen/goleak_signalfxexp.yaml

exporter/signalfxexporter/exporter.go

crobert-1 · 2024-05-03T22:28:51Z

~~Did some more testing, looks like I've broken something during shutdown. I'll investigate and move back to ready to review when I've figured out what's going on.~~

Nevermind, I was running tests using the hostmetrics receiver and didn't realize it blocks shutdown until active scrape operations are complete. I tested with debug exporter, and signalfx exporter before these changes, and everything is working as previously.

github-actions · 2024-05-21T05:20:10Z