From c5235f07e09b916e6459c134cf86f72426504777 Mon Sep 17 00:00:00 2001 From: Antoine Toulme Date: Wed, 13 Mar 2024 22:45:31 -0700 Subject: [PATCH 1/3] [extension/awsproxy] add support for shutdown test --- .chloggen/awsproxy_lifecycle.yaml | 27 +++++++++++++++++ extension/awsproxy/extension.go | 18 ++++++----- extension/awsproxy/extension_test.go | 6 +++- .../awsproxy/generated_component_test.go | 30 +++++++++++++++++++ extension/awsproxy/metadata.yaml | 2 -- 5 files changed, 72 insertions(+), 11 deletions(-) create mode 100644 .chloggen/awsproxy_lifecycle.yaml create mode 100644 extension/awsproxy/generated_component_test.go diff --git a/.chloggen/awsproxy_lifecycle.yaml b/.chloggen/awsproxy_lifecycle.yaml new file mode 100644 index 0000000000000..041a540311503 --- /dev/null +++ b/.chloggen/awsproxy_lifecycle.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: enhancement + +# The name of the component, or a single word describing the area of concern, (e.g. filelogreceiver) +component: awsproxyextension + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: Make awsproxy extension support shutdown + +# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists. +issues: [27849] + +# (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/extension/awsproxy/extension.go b/extension/awsproxy/extension.go index c2a3445104fa6..7680fd2e56fcc 100644 --- a/extension/awsproxy/extension.go +++ b/extension/awsproxy/extension.go @@ -25,6 +25,12 @@ type xrayProxy struct { var _ extension.Extension = (*xrayProxy)(nil) func (x xrayProxy) Start(_ context.Context, _ component.Host) error { + srv, err := proxy.NewServer(&x.config.ProxyConfig, x.settings.Logger) + + if err != nil { + return err + } + x.server = srv go func() { if err := x.server.ListenAndServe(); !errors.Is(err, http.ErrServerClosed) && err != nil { x.settings.ReportStatus(component.NewFatalErrorEvent(err)) @@ -35,20 +41,16 @@ func (x xrayProxy) Start(_ context.Context, _ component.Host) error { } func (x xrayProxy) Shutdown(ctx context.Context) error { - return x.server.Shutdown(ctx) + if x.server != nil { + return x.server.Shutdown(ctx) + } + return nil } func newXrayProxy(config *Config, telemetrySettings component.TelemetrySettings) (extension.Extension, error) { - srv, err := proxy.NewServer(&config.ProxyConfig, telemetrySettings.Logger) - - if err != nil { - return nil, err - } - p := &xrayProxy{ config: config, logger: telemetrySettings.Logger, - server: srv, settings: telemetrySettings, } diff --git a/extension/awsproxy/extension_test.go b/extension/awsproxy/extension_test.go index 91719528d8ccd..c3bf316053951 100644 --- a/extension/awsproxy/extension_test.go +++ b/extension/awsproxy/extension_test.go @@ -4,6 +4,7 @@ package awsproxy import ( + "context" "testing" "github.com/stretchr/testify/assert" @@ -14,7 +15,7 @@ import ( ) func TestInvalidEndpoint(t *testing.T) { - _, err := newXrayProxy( + x, err := newXrayProxy( &Config{ ProxyConfig: proxy.Config{ TCPAddrConfig: confignet.TCPAddrConfig{ @@ -24,5 +25,8 @@ func TestInvalidEndpoint(t *testing.T) { }, componenttest.NewNopTelemetrySettings(), ) + assert.NoError(t, err) + err = x.Start(context.Background(), componenttest.NewNopHost()) + defer assert.NoError(t, x.Shutdown(context.Background())) assert.Error(t, err) } diff --git a/extension/awsproxy/generated_component_test.go b/extension/awsproxy/generated_component_test.go new file mode 100644 index 0000000000000..a8cd2782de761 --- /dev/null +++ b/extension/awsproxy/generated_component_test.go @@ -0,0 +1,30 @@ +// Code generated by mdatagen. DO NOT EDIT. + +package awsproxy + +import ( + "context" + "testing" + + "github.com/stretchr/testify/require" + "go.opentelemetry.io/collector/component" + "go.opentelemetry.io/collector/confmap/confmaptest" + "go.opentelemetry.io/collector/extension/extensiontest" +) + +func TestComponentLifecycle(t *testing.T) { + factory := NewFactory() + + cm, err := confmaptest.LoadConf("metadata.yaml") + require.NoError(t, err) + cfg := factory.CreateDefaultConfig() + sub, err := cm.Sub("tests::config") + require.NoError(t, err) + require.NoError(t, component.UnmarshalConfig(sub, cfg)) + t.Run("shutdown", func(t *testing.T) { + e, err := factory.CreateExtension(context.Background(), extensiontest.NewNopCreateSettings(), cfg) + require.NoError(t, err) + err = e.Shutdown(context.Background()) + require.NoError(t, err) + }) +} diff --git a/extension/awsproxy/metadata.yaml b/extension/awsproxy/metadata.yaml index 81955aae5b429..10cd9d3518177 100644 --- a/extension/awsproxy/metadata.yaml +++ b/extension/awsproxy/metadata.yaml @@ -9,7 +9,5 @@ status: codeowners: active: [Aneurysm9, mxiamxia] -# TODO: Update the extension to make the tests pass tests: skip_lifecycle: true - skip_shutdown: true From e8b9ce2def651cb08bd05ffe6a51b507cf59b829 Mon Sep 17 00:00:00 2001 From: Antoine Toulme Date: Thu, 14 Mar 2024 21:51:56 -0700 Subject: [PATCH 2/3] Update .chloggen/awsproxy_lifecycle.yaml --- .chloggen/awsproxy_lifecycle.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.chloggen/awsproxy_lifecycle.yaml b/.chloggen/awsproxy_lifecycle.yaml index 041a540311503..6e1092e500fea 100644 --- a/.chloggen/awsproxy_lifecycle.yaml +++ b/.chloggen/awsproxy_lifecycle.yaml @@ -7,7 +7,7 @@ change_type: enhancement component: awsproxyextension # A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). -note: Make awsproxy extension support shutdown +note: Make awsproxy extension more resilient by initiating the AWS client during Start. # Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists. issues: [27849] From 85b43299c62f343caa7b5d9405f308f85165e194 Mon Sep 17 00:00:00 2001 From: Antoine Toulme Date: Thu, 4 Apr 2024 17:37:25 -0700 Subject: [PATCH 3/3] Use pointer receivers --- extension/awsproxy/extension.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/extension/awsproxy/extension.go b/extension/awsproxy/extension.go index 7680fd2e56fcc..8d8517fa51ba8 100644 --- a/extension/awsproxy/extension.go +++ b/extension/awsproxy/extension.go @@ -24,7 +24,7 @@ type xrayProxy struct { var _ extension.Extension = (*xrayProxy)(nil) -func (x xrayProxy) Start(_ context.Context, _ component.Host) error { +func (x *xrayProxy) Start(_ context.Context, _ component.Host) error { srv, err := proxy.NewServer(&x.config.ProxyConfig, x.settings.Logger) if err != nil { @@ -40,7 +40,7 @@ func (x xrayProxy) Start(_ context.Context, _ component.Host) error { return nil } -func (x xrayProxy) Shutdown(ctx context.Context) error { +func (x *xrayProxy) Shutdown(ctx context.Context) error { if x.server != nil { return x.server.Shutdown(ctx) }