From 60d44f5e45643e1603427a94a4b69918b82e3b07 Mon Sep 17 00:00:00 2001 From: Curtis Robert Date: Thu, 4 Jan 2024 16:00:48 -0800 Subject: [PATCH 1/7] [chore][receiver/otlpreceiver] Enable goleak check --- receiver/otlpreceiver/factory_test.go | 1 + receiver/otlpreceiver/package_test.go | 17 +++++++++++++++++ 2 files changed, 18 insertions(+) create mode 100644 receiver/otlpreceiver/package_test.go diff --git a/receiver/otlpreceiver/factory_test.go b/receiver/otlpreceiver/factory_test.go index eddf51f196e..fbe4613ab1e 100644 --- a/receiver/otlpreceiver/factory_test.go +++ b/receiver/otlpreceiver/factory_test.go @@ -261,6 +261,7 @@ func TestCreateMetricReceiver(t *testing.T) { assert.NoError(t, err) if tt.wantStartErr { assert.Error(t, mr.Start(context.Background(), componenttest.NewNopHost())) + assert.NoError(t, mr.Shutdown(context.Background())) } else { require.NoError(t, mr.Start(context.Background(), componenttest.NewNopHost())) assert.NoError(t, mr.Shutdown(context.Background())) diff --git a/receiver/otlpreceiver/package_test.go b/receiver/otlpreceiver/package_test.go new file mode 100644 index 00000000000..9d49b5c8f7b --- /dev/null +++ b/receiver/otlpreceiver/package_test.go @@ -0,0 +1,17 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +package otlpreceiver + +import ( + "testing" + + "go.uber.org/goleak" +) + +// The IgnoreTopFunction call prevents catching the leak generated by opencensus +// defaultWorker.Start which at this time is part of the package's init call. +// See https://github.com/open-telemetry/opentelemetry-collector/issues/9165#issuecomment-1874836336 for more context. +func TestMain(m *testing.M) { + goleak.VerifyTestMain(m, goleak.IgnoreTopFunction("go.opencensus.io/stats/view.(*worker).start")) +} From 31feb8e3485a85ad732fdebeb51e270d04d9ae7c Mon Sep 17 00:00:00 2001 From: Curtis Robert Date: Fri, 5 Jan 2024 10:41:30 -0800 Subject: [PATCH 2/7] Shutdown everytime http server starts to fail to clean up GRPC server --- receiver/otlpreceiver/factory_test.go | 3 --- receiver/otlpreceiver/otlp.go | 7 ++++++- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/receiver/otlpreceiver/factory_test.go b/receiver/otlpreceiver/factory_test.go index fbe4613ab1e..7ebb5ce51cc 100644 --- a/receiver/otlpreceiver/factory_test.go +++ b/receiver/otlpreceiver/factory_test.go @@ -151,7 +151,6 @@ func TestCreateTracesReceiver(t *testing.T) { assert.NoError(t, err) if tt.wantStartErr { assert.Error(t, tr.Start(context.Background(), componenttest.NewNopHost())) - assert.NoError(t, tr.Shutdown(context.Background())) } else { assert.NoError(t, tr.Start(context.Background(), componenttest.NewNopHost())) assert.NoError(t, tr.Shutdown(context.Background())) @@ -261,7 +260,6 @@ func TestCreateMetricReceiver(t *testing.T) { assert.NoError(t, err) if tt.wantStartErr { assert.Error(t, mr.Start(context.Background(), componenttest.NewNopHost())) - assert.NoError(t, mr.Shutdown(context.Background())) } else { require.NoError(t, mr.Start(context.Background(), componenttest.NewNopHost())) assert.NoError(t, mr.Shutdown(context.Background())) @@ -371,7 +369,6 @@ func TestCreateLogReceiver(t *testing.T) { assert.NoError(t, err) if tt.wantStartErr { assert.Error(t, mr.Start(context.Background(), componenttest.NewNopHost())) - assert.NoError(t, mr.Shutdown(context.Background())) } else { require.NoError(t, mr.Start(context.Background(), componenttest.NewNopHost())) assert.NoError(t, mr.Shutdown(context.Background())) diff --git a/receiver/otlpreceiver/otlp.go b/receiver/otlpreceiver/otlp.go index 22932661685..6f6ad6c4afb 100644 --- a/receiver/otlpreceiver/otlp.go +++ b/receiver/otlpreceiver/otlp.go @@ -10,6 +10,7 @@ import ( "net/http" "sync" + "go.uber.org/multierr" "go.uber.org/zap" "google.golang.org/grpc" @@ -168,11 +169,15 @@ func (r *otlpReceiver) startHTTPServer(host component.Host) error { // Start runs the trace receiver on the gRPC server. Currently // it also enables the metrics receiver too. -func (r *otlpReceiver) Start(_ context.Context, host component.Host) error { +func (r *otlpReceiver) Start(ctx context.Context, host component.Host) error { if err := r.startGRPCServer(host); err != nil { return err } if err := r.startHTTPServer(host); err != nil { + // It's possible that a valid GRPC server configuration was specified, + // but an invalid HTTP configuration. If that's the case, the successfully + // started GRPC server must be shutdown to ensure no goroutines are leaked. + err = multierr.Append(err, r.Shutdown(ctx)) return err } From e8260984168a87f6681dbf3adc81c8bb08ff64db Mon Sep 17 00:00:00 2001 From: Curtis Robert Date: Fri, 5 Jan 2024 10:44:06 -0800 Subject: [PATCH 3/7] Add changelog --- .chloggen/goleak_otlpreceiver.yaml | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) create mode 100755 .chloggen/goleak_otlpreceiver.yaml diff --git a/.chloggen/goleak_otlpreceiver.yaml b/.chloggen/goleak_otlpreceiver.yaml new file mode 100755 index 00000000000..7837fcdaa99 --- /dev/null +++ b/.chloggen/goleak_otlpreceiver.yaml @@ -0,0 +1,25 @@ +# 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. otlpreceiver) +component: otlpreceiver + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: Fix goroutine leak when GRPC server is started but HTTP server is unsuccessful + +# One or more tracking issues or pull requests related to the change +issues: [9165] + +# (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: + +# 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: [] \ No newline at end of file From 9e86afe4a153e60f701a5af757b68b0234108729 Mon Sep 17 00:00:00 2001 From: Curtis Robert Date: Mon, 8 Jan 2024 16:34:12 -0800 Subject: [PATCH 4/7] make gotidy --- receiver/otlpreceiver/go.mod | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/receiver/otlpreceiver/go.mod b/receiver/otlpreceiver/go.mod index b3f6f47cad3..8c05f53c274 100644 --- a/receiver/otlpreceiver/go.mod +++ b/receiver/otlpreceiver/go.mod @@ -19,6 +19,7 @@ require ( go.opentelemetry.io/otel/metric v1.21.0 go.opentelemetry.io/otel/trace v1.21.0 go.uber.org/goleak v1.3.0 + go.uber.org/multierr v1.11.0 go.uber.org/zap v1.26.0 google.golang.org/genproto/googleapis/rpc v0.0.0-20231127180814-3a041ad873d4 google.golang.org/grpc v1.60.1 @@ -75,7 +76,6 @@ require ( go.opentelemetry.io/otel/exporters/prometheus v0.44.1-0.20231201153405-6027c1ae76f2 // indirect go.opentelemetry.io/otel/sdk v1.21.0 // indirect go.opentelemetry.io/otel/sdk/metric v1.21.0 // indirect - go.uber.org/multierr v1.11.0 // indirect golang.org/x/net v0.19.0 // indirect golang.org/x/sys v0.15.0 // indirect golang.org/x/text v0.14.0 // indirect From e98792c6339fabf44c11730f7be02aef8bb827f0 Mon Sep 17 00:00:00 2001 From: Curtis Robert Date: Tue, 23 Jan 2024 08:20:53 -0800 Subject: [PATCH 5/7] make gotidy --- receiver/otlpreceiver/go.mod | 1 - 1 file changed, 1 deletion(-) diff --git a/receiver/otlpreceiver/go.mod b/receiver/otlpreceiver/go.mod index 6bc7e612f02..b6afac671f0 100644 --- a/receiver/otlpreceiver/go.mod +++ b/receiver/otlpreceiver/go.mod @@ -82,7 +82,6 @@ require ( go.opentelemetry.io/otel/sdk v1.22.0 // indirect go.opentelemetry.io/otel/sdk/metric v1.22.0 // indirect go.opentelemetry.io/proto/otlp v1.0.0 // indirect - go.uber.org/multierr v1.11.0 // indirect golang.org/x/net v0.20.0 // indirect golang.org/x/sys v0.16.0 // indirect golang.org/x/text v0.14.0 // indirect From b6283bfdd318bb9fe026b1d2c0d407d4b38ae51d Mon Sep 17 00:00:00 2001 From: Curtis Robert Date: Wed, 24 Jan 2024 08:07:57 -0800 Subject: [PATCH 6/7] Changes requested in PR - Simplify return --- receiver/otlpreceiver/otlp.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/receiver/otlpreceiver/otlp.go b/receiver/otlpreceiver/otlp.go index 6f6ad6c4afb..a8f2b12e3ee 100644 --- a/receiver/otlpreceiver/otlp.go +++ b/receiver/otlpreceiver/otlp.go @@ -177,8 +177,7 @@ func (r *otlpReceiver) Start(ctx context.Context, host component.Host) error { // It's possible that a valid GRPC server configuration was specified, // but an invalid HTTP configuration. If that's the case, the successfully // started GRPC server must be shutdown to ensure no goroutines are leaked. - err = multierr.Append(err, r.Shutdown(ctx)) - return err + return multierr.Append(err, r.Shutdown(ctx)) } return nil From cda6406b3de69a454f48ca2310917a8867af71d3 Mon Sep 17 00:00:00 2001 From: Curtis Robert Date: Wed, 24 Jan 2024 13:14:45 -0800 Subject: [PATCH 7/7] Change requested in PR: Move to errors.join --- receiver/otlpreceiver/go.mod | 2 +- receiver/otlpreceiver/otlp.go | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/receiver/otlpreceiver/go.mod b/receiver/otlpreceiver/go.mod index b6afac671f0..0d6e111ef9c 100644 --- a/receiver/otlpreceiver/go.mod +++ b/receiver/otlpreceiver/go.mod @@ -19,7 +19,6 @@ require ( go.opentelemetry.io/otel/metric v1.22.0 go.opentelemetry.io/otel/trace v1.22.0 go.uber.org/goleak v1.3.0 - go.uber.org/multierr v1.11.0 go.uber.org/zap v1.26.0 google.golang.org/genproto/googleapis/rpc v0.0.0-20231127180814-3a041ad873d4 google.golang.org/grpc v1.60.1 @@ -82,6 +81,7 @@ require ( go.opentelemetry.io/otel/sdk v1.22.0 // indirect go.opentelemetry.io/otel/sdk/metric v1.22.0 // indirect go.opentelemetry.io/proto/otlp v1.0.0 // indirect + go.uber.org/multierr v1.11.0 // indirect golang.org/x/net v0.20.0 // indirect golang.org/x/sys v0.16.0 // indirect golang.org/x/text v0.14.0 // indirect diff --git a/receiver/otlpreceiver/otlp.go b/receiver/otlpreceiver/otlp.go index a8f2b12e3ee..b302ae56387 100644 --- a/receiver/otlpreceiver/otlp.go +++ b/receiver/otlpreceiver/otlp.go @@ -10,7 +10,6 @@ import ( "net/http" "sync" - "go.uber.org/multierr" "go.uber.org/zap" "google.golang.org/grpc" @@ -177,7 +176,7 @@ func (r *otlpReceiver) Start(ctx context.Context, host component.Host) error { // It's possible that a valid GRPC server configuration was specified, // but an invalid HTTP configuration. If that's the case, the successfully // started GRPC server must be shutdown to ensure no goroutines are leaked. - return multierr.Append(err, r.Shutdown(ctx)) + return errors.Join(err, r.Shutdown(ctx)) } return nil