From c31746bddb43c951aef5e1b67a090b645bc11334 Mon Sep 17 00:00:00 2001 From: Ian Adams Date: Mon, 11 Nov 2024 15:11:52 -0500 Subject: [PATCH 1/3] [receiver/azureeventhub] Close storage extension client during component shutdown --- .chloggen/aeh-storageclient-shutdown.yaml | 27 +++++++++++++++++++ .../azureeventhubreceiver/eventhubhandler.go | 20 ++++++++++---- 2 files changed, 42 insertions(+), 5 deletions(-) create mode 100644 .chloggen/aeh-storageclient-shutdown.yaml diff --git a/.chloggen/aeh-storageclient-shutdown.yaml b/.chloggen/aeh-storageclient-shutdown.yaml new file mode 100644 index 0000000000000..d5b09a5e52ac7 --- /dev/null +++ b/.chloggen/aeh-storageclient-shutdown.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: receiver/azureeventhub + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: When using a storage extension, the component will call Close on the client during component shutdown. This fixes a bug that resulted in a file potentially remaining locked after component shutdown. + +# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists. +issues: [36238] + +# (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: [user] \ No newline at end of file diff --git a/receiver/azureeventhubreceiver/eventhubhandler.go b/receiver/azureeventhubreceiver/eventhubhandler.go index bf48cb1047f90..e45bb8ce7063f 100644 --- a/receiver/azureeventhubreceiver/eventhubhandler.go +++ b/receiver/azureeventhubreceiver/eventhubhandler.go @@ -8,6 +8,7 @@ import ( eventhub "github.com/Azure/azure-event-hubs-go/v3" "go.opentelemetry.io/collector/component" + "go.opentelemetry.io/collector/extension/experimental/storage" "go.opentelemetry.io/collector/receiver" "go.uber.org/zap" @@ -43,11 +44,12 @@ type listerHandleWrapper interface { } type eventhubHandler struct { - hub hubWrapper - dataConsumer dataConsumer - config *Config - settings receiver.Settings - cancel context.CancelFunc + hub hubWrapper + dataConsumer dataConsumer + config *Config + settings receiver.Settings + cancel context.CancelFunc + storageClient storage.Client } func (h *eventhubHandler) run(ctx context.Context, host component.Host) error { @@ -58,6 +60,7 @@ func (h *eventhubHandler) run(ctx context.Context, host component.Host) error { h.settings.Logger.Debug("Error connecting to Storage", zap.Error(err)) return err } + h.storageClient = storageClient if h.hub == nil { // set manually for testing. hub, newHubErr := eventhub.NewHubFromConnectionString(h.config.Connection, eventhub.HubWithOffsetPersistence(&storageCheckpointPersister{storageClient: storageClient})) @@ -160,6 +163,13 @@ func (h *eventhubHandler) newMessageHandler(ctx context.Context, event *eventhub } func (h *eventhubHandler) close(ctx context.Context) error { + if h.storageClient != nil { + if err := h.storageClient.Close(ctx); err != nil { + return err + } + h.storageClient = nil + } + if h.hub != nil { err := h.hub.Close(ctx) if err != nil { From 40b77bbde4a544a761c9eab425358507e0c6971f Mon Sep 17 00:00:00 2001 From: Ian Adams Date: Mon, 11 Nov 2024 15:16:54 -0500 Subject: [PATCH 2/3] chloggen newline --- .chloggen/aeh-storageclient-shutdown.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.chloggen/aeh-storageclient-shutdown.yaml b/.chloggen/aeh-storageclient-shutdown.yaml index d5b09a5e52ac7..62ba267a8bd5d 100644 --- a/.chloggen/aeh-storageclient-shutdown.yaml +++ b/.chloggen/aeh-storageclient-shutdown.yaml @@ -24,4 +24,4 @@ subtext: # 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: [user] \ No newline at end of file +change_logs: [user] From 19198cec8d9fca0b411b61243e5c989a0ca33515 Mon Sep 17 00:00:00 2001 From: Ian Adams Date: Wed, 13 Nov 2024 14:26:08 -0500 Subject: [PATCH 3/3] Add storage client closure test case + utilize errors.Join --- .../azureeventhubreceiver/eventhubhandler.go | 27 ++++++++++--------- .../eventhubhandler_test.go | 21 +++++++++++++++ 2 files changed, 36 insertions(+), 12 deletions(-) diff --git a/receiver/azureeventhubreceiver/eventhubhandler.go b/receiver/azureeventhubreceiver/eventhubhandler.go index e45bb8ce7063f..94ad5915f2334 100644 --- a/receiver/azureeventhubreceiver/eventhubhandler.go +++ b/receiver/azureeventhubreceiver/eventhubhandler.go @@ -5,6 +5,7 @@ package azureeventhubreceiver // import "github.com/open-telemetry/opentelemetry import ( "context" + "errors" eventhub "github.com/Azure/azure-event-hubs-go/v3" "go.opentelemetry.io/collector/component" @@ -55,15 +56,17 @@ type eventhubHandler struct { func (h *eventhubHandler) run(ctx context.Context, host component.Host) error { ctx, h.cancel = context.WithCancel(ctx) - storageClient, err := adapter.GetStorageClient(ctx, host, h.config.StorageID, h.settings.ID) - if err != nil { - h.settings.Logger.Debug("Error connecting to Storage", zap.Error(err)) - return err + if h.storageClient == nil { // set manually for testing. + storageClient, err := adapter.GetStorageClient(ctx, host, h.config.StorageID, h.settings.ID) + if err != nil { + h.settings.Logger.Debug("Error connecting to Storage", zap.Error(err)) + return err + } + h.storageClient = storageClient } - h.storageClient = storageClient if h.hub == nil { // set manually for testing. - hub, newHubErr := eventhub.NewHubFromConnectionString(h.config.Connection, eventhub.HubWithOffsetPersistence(&storageCheckpointPersister{storageClient: storageClient})) + hub, newHubErr := eventhub.NewHubFromConnectionString(h.config.Connection, eventhub.HubWithOffsetPersistence(&storageCheckpointPersister{storageClient: h.storageClient})) if newHubErr != nil { h.settings.Logger.Debug("Error connecting to Event Hub", zap.Error(newHubErr)) return newHubErr @@ -75,8 +78,7 @@ func (h *eventhubHandler) run(ctx context.Context, host component.Host) error { if h.config.Partition == "" { // listen to each partition of the Event Hub - var runtimeInfo *eventhub.HubRuntimeInformation - runtimeInfo, err = h.hub.GetRuntimeInformation(ctx) + runtimeInfo, err := h.hub.GetRuntimeInformation(ctx) if err != nil { h.settings.Logger.Debug("Error getting Runtime Information", zap.Error(err)) return err @@ -90,7 +92,7 @@ func (h *eventhubHandler) run(ctx context.Context, host component.Host) error { } } } else { - err = h.setUpOnePartition(ctx, h.config.Partition, true) + err := h.setUpOnePartition(ctx, h.config.Partition, true) if err != nil { h.settings.Logger.Debug("Error setting up partition", zap.Error(err)) return err @@ -163,9 +165,10 @@ func (h *eventhubHandler) newMessageHandler(ctx context.Context, event *eventhub } func (h *eventhubHandler) close(ctx context.Context) error { + var errs error if h.storageClient != nil { if err := h.storageClient.Close(ctx); err != nil { - return err + errs = errors.Join(errs, err) } h.storageClient = nil } @@ -173,7 +176,7 @@ func (h *eventhubHandler) close(ctx context.Context) error { if h.hub != nil { err := h.hub.Close(ctx) if err != nil { - return err + errs = errors.Join(errs, err) } h.hub = nil } @@ -181,7 +184,7 @@ func (h *eventhubHandler) close(ctx context.Context) error { h.cancel() } - return nil + return errs } func (h *eventhubHandler) setDataConsumer(dataConsumer dataConsumer) { diff --git a/receiver/azureeventhubreceiver/eventhubhandler_test.go b/receiver/azureeventhubreceiver/eventhubhandler_test.go index 438f71bddc5d8..629dc43071f20 100644 --- a/receiver/azureeventhubreceiver/eventhubhandler_test.go +++ b/receiver/azureeventhubreceiver/eventhubhandler_test.go @@ -154,3 +154,24 @@ func TestEventhubHandler_newMessageHandler(t *testing.T) { assert.Equal(t, "bar", read.AsString()) assert.NoError(t, ehHandler.close(context.Background())) } + +func TestEventhubHandler_closeWithStorageClient(t *testing.T) { + config := createDefaultConfig() + config.(*Config).Connection = "Endpoint=sb://namespace.servicebus.windows.net/;SharedAccessKeyName=RootManageSharedAccessKey;SharedAccessKey=superSecret1234=;EntityPath=hubName" + + ehHandler := &eventhubHandler{ + settings: receivertest.NewNopSettings(), + dataConsumer: &mockDataConsumer{}, + config: config.(*Config), + } + ehHandler.hub = &mockHubWrapper{} + mockClient := newMockClient() + ehHandler.storageClient = mockClient + + assert.NoError(t, ehHandler.run(context.Background(), componenttest.NewNopHost())) + require.NotNil(t, ehHandler.storageClient) + require.NotNil(t, mockClient.cache) + assert.NoError(t, ehHandler.close(context.Background())) + require.Nil(t, ehHandler.storageClient) + require.Nil(t, mockClient.cache) +}