From 115fe04580203b102ff58e80bf8baadaf594ef04 Mon Sep 17 00:00:00 2001 From: Curtis Robert Date: Mon, 12 Feb 2024 02:00:29 -0800 Subject: [PATCH] [exporter/logicmonitorexporter] Fix leaking goroutines on shutdown (#31150) **Description:** The [logicmonitor dependency](https://github.com/logicmonitor/lm-data-sdk-go) that handles exporting data relies on the context being cancelled to shutdown its running goroutines ([traces reference](https://github.com/logicmonitor/lm-data-sdk-go/blob/93e0505a0dce33bfd874c67bdf85e3845126cc3d/pkg/ratelimiter/traces.go#L113), [logs reference](https://github.com/logicmonitor/lm-data-sdk-go/blob/93e0505a0dce33bfd874c67bdf85e3845126cc3d/pkg/ratelimiter/logs.go#L70)). The exporter has now been updated to properly cancel the passed context to fix leaking goroutines on shutdown. This change also enables `goleak` to check for leaking goroutines in the logicmonitor exporter package. **Link to tracking Issue:** #30438 **Testing:** All existing tests are passing, as well as added `goleak` check. --- .chloggen/goleak_logicmonitorexp.yaml | 27 +++++++++++++++++++ exporter/logicmonitorexporter/factory.go | 5 +++- .../logicmonitorexporter/logs_exporter.go | 10 +++++++ .../logs_exporter_test.go | 1 + exporter/logicmonitorexporter/package_test.go | 14 ++++++++++ .../logicmonitorexporter/traces_exporter.go | 11 ++++++++ .../traces_exporter_test.go | 1 + 7 files changed, 68 insertions(+), 1 deletion(-) create mode 100755 .chloggen/goleak_logicmonitorexp.yaml create mode 100644 exporter/logicmonitorexporter/package_test.go diff --git a/.chloggen/goleak_logicmonitorexp.yaml b/.chloggen/goleak_logicmonitorexp.yaml new file mode 100755 index 000000000000..4f450b9688b5 --- /dev/null +++ b/.chloggen/goleak_logicmonitorexp.yaml @@ -0,0 +1,27 @@ +# Use this changelog template to create an entry for release notes. + +# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' +change_type: bug_fix + +# The name of the component, or a single word describing the area of concern, (e.g. filelogreceiver) +component: logicmonitorexporter + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: Fix memory leak on shutdown + +# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists. +issues: [31150] + +# (Optional) One or more lines of additional information to render under the primary note. +# These lines will be padded with 2 spaces and then inserted directly into the document. +# Use pipe (|) for multiline entries. +subtext: + +# If your change doesn't affect end users or the exported elements of any package, +# you should instead start your pull request title with [chore] or use the "Skip Changelog" label. +# Optional: The change log or logs in which this entry should be included. +# e.g. '[user]' or '[user, api]' +# Include 'user' if the change is relevant to end users. +# Include 'api' if there is a change to a library API. +# Default: '[user]' +change_logs: [] diff --git a/exporter/logicmonitorexporter/factory.go b/exporter/logicmonitorexporter/factory.go index 66b58834297b..e5c6e9c3f2db 100644 --- a/exporter/logicmonitorexporter/factory.go +++ b/exporter/logicmonitorexporter/factory.go @@ -46,6 +46,7 @@ func createLogsExporter(ctx context.Context, set exporter.CreateSettings, cfg co exporterhelper.WithStart(lmLogExp.start), exporterhelper.WithQueue(c.QueueSettings), exporterhelper.WithRetry(c.BackOffConfig), + exporterhelper.WithShutdown(lmLogExp.shutdown), ) } @@ -61,5 +62,7 @@ func createTracesExporter(ctx context.Context, set exporter.CreateSettings, cfg exporterhelper.WithCapabilities(consumer.Capabilities{MutatesData: false}), exporterhelper.WithStart(lmTraceExp.start), exporterhelper.WithRetry(c.BackOffConfig), - exporterhelper.WithQueue(c.QueueSettings)) + exporterhelper.WithQueue(c.QueueSettings), + exporterhelper.WithShutdown(lmTraceExp.shutdown), + ) } diff --git a/exporter/logicmonitorexporter/logs_exporter.go b/exporter/logicmonitorexporter/logs_exporter.go index 44ba752f4360..725555c94368 100644 --- a/exporter/logicmonitorexporter/logs_exporter.go +++ b/exporter/logicmonitorexporter/logs_exporter.go @@ -32,6 +32,7 @@ type logExporter struct { config *Config sender *logs.Sender settings component.TelemetrySettings + cancel context.CancelFunc } // Create new logicmonitor logs exporter @@ -52,6 +53,7 @@ func (e *logExporter) start(ctx context.Context, host component.Host) error { opts := buildLogIngestOpts(e.config, client) + ctx, e.cancel = context.WithCancel(ctx) e.sender, err = logs.NewSender(ctx, e.settings.Logger, opts...) if err != nil { return err @@ -95,6 +97,14 @@ func (e *logExporter) PushLogData(ctx context.Context, lg plog.Logs) error { return e.sender.SendLogs(ctx, payload) } +func (e *logExporter) shutdown(_ context.Context) error { + if e.cancel != nil { + e.cancel() + } + + return nil +} + func buildLogIngestOpts(config *Config, client *http.Client) []lmsdklogs.Option { authParams := utils.AuthParams{ AccessID: config.APIToken.AccessID, diff --git a/exporter/logicmonitorexporter/logs_exporter_test.go b/exporter/logicmonitorexporter/logs_exporter_test.go index 9531bb9afac3..cd7839246473 100644 --- a/exporter/logicmonitorexporter/logs_exporter_test.go +++ b/exporter/logicmonitorexporter/logs_exporter_test.go @@ -110,6 +110,7 @@ func TestPushLogData(t *testing.T) { exp := newLogsExporter(test.args.ctx, test.fields.config, set) require.NoError(t, exp.start(test.args.ctx, componenttest.NewNopHost())) + defer func() { require.NoError(t, exp.shutdown(test.args.ctx)) }() err := exp.PushLogData(test.args.ctx, test.args.lg) assert.NoError(t, err) }) diff --git a/exporter/logicmonitorexporter/package_test.go b/exporter/logicmonitorexporter/package_test.go new file mode 100644 index 000000000000..babe01d0f205 --- /dev/null +++ b/exporter/logicmonitorexporter/package_test.go @@ -0,0 +1,14 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +package logicmonitorexporter + +import ( + "testing" + + "go.uber.org/goleak" +) + +func TestMain(m *testing.M) { + goleak.VerifyTestMain(m) +} diff --git a/exporter/logicmonitorexporter/traces_exporter.go b/exporter/logicmonitorexporter/traces_exporter.go index 4f2866da4d77..ca3a5e221f31 100644 --- a/exporter/logicmonitorexporter/traces_exporter.go +++ b/exporter/logicmonitorexporter/traces_exporter.go @@ -19,6 +19,7 @@ type tracesExporter struct { config *Config sender *traces.Sender settings component.TelemetrySettings + cancel context.CancelFunc } // newTracesExporter creates new Logicmonitor Traces Exporter. @@ -43,6 +44,8 @@ func (e *tracesExporter) start(ctx context.Context, host component.Host) error { AccessKey: string(e.config.APIToken.AccessKey), BearerToken: string(e.config.Headers["Authorization"]), } + + ctx, e.cancel = context.WithCancel(ctx) e.sender, err = traces.NewSender(ctx, e.config.Endpoint, client, authParams, e.settings.Logger) if err != nil { return err @@ -53,3 +56,11 @@ func (e *tracesExporter) start(ctx context.Context, host component.Host) error { func (e *tracesExporter) PushTraceData(ctx context.Context, td ptrace.Traces) error { return e.sender.SendTraces(ctx, td) } + +func (e *tracesExporter) shutdown(_ context.Context) error { + if e.cancel != nil { + e.cancel() + } + + return nil +} diff --git a/exporter/logicmonitorexporter/traces_exporter_test.go b/exporter/logicmonitorexporter/traces_exporter_test.go index 34fbd651ffd9..6a606e4c7c8c 100644 --- a/exporter/logicmonitorexporter/traces_exporter_test.go +++ b/exporter/logicmonitorexporter/traces_exporter_test.go @@ -54,6 +54,7 @@ func TestPushTraceData(t *testing.T) { exp, err := f.CreateTracesExporter(ctx, params, config) assert.NoError(t, err) assert.NoError(t, exp.Start(ctx, componenttest.NewNopHost())) + defer func() { assert.NoError(t, exp.Shutdown(ctx)) }() testTraces := ptrace.NewTraces() generateTraces().CopyTo(testTraces)