From 7c1225d7807c8b1b4bf80d0c8a409b29c9986a03 Mon Sep 17 00:00:00 2001 From: Bryan Aguilar Date: Thu, 11 Jan 2024 16:57:12 -0800 Subject: [PATCH 1/4] Log on consume failure even if no retry set --- exporter/exporterhelper/common.go | 7 +++-- exporter/exporterhelper/common_test.go | 41 +++++++++++++++++++------- 2 files changed, 34 insertions(+), 14 deletions(-) diff --git a/exporter/exporterhelper/common.go b/exporter/exporterhelper/common.go index 2c5a4e96692..12ecc00b5e3 100644 --- a/exporter/exporterhelper/common.go +++ b/exporter/exporterhelper/common.go @@ -163,9 +163,10 @@ func newBaseExporter(set exporter.CreateSettings, signal component.DataType, req unmarshaler: unmarshaler, signal: signal, - queueSender: &baseRequestSender{}, - obsrepSender: osf(obsReport), - retrySender: &baseRequestSender{}, + queueSender: &baseRequestSender{}, + obsrepSender: osf(obsReport), + // this is required so that exporters that don't use WithRetry() still get error logs on export failure + retrySender: &errorLoggingRequestSender{logger: set.Logger}, timeoutSender: &timeoutSender{cfg: NewDefaultTimeoutSettings()}, set: set, diff --git a/exporter/exporterhelper/common_test.go b/exporter/exporterhelper/common_test.go index 13568ffb49f..4061510482e 100644 --- a/exporter/exporterhelper/common_test.go +++ b/exporter/exporterhelper/common_test.go @@ -84,16 +84,35 @@ func TestQueueRetryOptionsWithRequestExporter(t *testing.T) { } func TestBaseExporterLogging(t *testing.T) { - set := exportertest.NewNopCreateSettings() - logger, observed := observer.New(zap.DebugLevel) - set.Logger = zap.New(logger) - rCfg := configretry.NewDefaultBackOffConfig() - rCfg.Enabled = false - bs, err := newBaseExporter(set, "", true, nil, nil, newNoopObsrepSender, WithRetry(rCfg)) - require.Nil(t, err) - require.True(t, bs.requestExporter) - sendErr := bs.send(context.Background(), newErrorRequest()) - require.Error(t, sendErr) + tests := []struct { + name string + exporterOptions func() []Option + }{ + { + "no options", + func() []Option { return nil }, + }, + { + "with retry", + func() []Option { + rCfg := configretry.NewDefaultBackOffConfig() + rCfg.Enabled = false + return []Option{WithRetry(rCfg)} + }, + }, + } + for _, test := range tests { + set := exportertest.NewNopCreateSettings() + logger, observed := observer.New(zap.DebugLevel) + set.Logger = zap.New(logger) + + bs, err := newBaseExporter(set, "", true, nil, nil, newNoopObsrepSender, test.exporterOptions()...) + require.Nil(t, err) + require.True(t, bs.requestExporter) + sendErr := bs.send(context.Background(), newErrorRequest()) + require.Error(t, sendErr) + + require.Len(t, observed.FilterLevelExact(zap.ErrorLevel).All(), 1) + } - require.Len(t, observed.FilterLevelExact(zap.ErrorLevel).All(), 1) } From 10bd41151a1a1e24fc429b0a5d7978795809039a Mon Sep 17 00:00:00 2001 From: Bryan Aguilar Date: Thu, 11 Jan 2024 16:58:14 -0800 Subject: [PATCH 2/4] Add chlog --- .chloggen/eh_logerrors.yaml | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) create mode 100755 .chloggen/eh_logerrors.yaml diff --git a/.chloggen/eh_logerrors.yaml b/.chloggen/eh_logerrors.yaml new file mode 100755 index 00000000000..306c8c0cbcc --- /dev/null +++ b/.chloggen/eh_logerrors.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: exporterhelper + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: log errors even if component does not use exporter helper retry + +# One or more tracking issues or pull requests related to the change +issues: [9219] + +# (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: [user] \ No newline at end of file From 16217c9ccbe280c57391ac58b65a285be3158f5c Mon Sep 17 00:00:00 2001 From: Bryan Aguilar Date: Thu, 11 Jan 2024 17:12:28 -0800 Subject: [PATCH 3/4] Add log message --- exporter/exporterhelper/common.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/exporter/exporterhelper/common.go b/exporter/exporterhelper/common.go index 12ecc00b5e3..5c3f2025676 100644 --- a/exporter/exporterhelper/common.go +++ b/exporter/exporterhelper/common.go @@ -166,7 +166,7 @@ func newBaseExporter(set exporter.CreateSettings, signal component.DataType, req queueSender: &baseRequestSender{}, obsrepSender: osf(obsReport), // this is required so that exporters that don't use WithRetry() still get error logs on export failure - retrySender: &errorLoggingRequestSender{logger: set.Logger}, + retrySender: &errorLoggingRequestSender{logger: set.Logger, message: "Exporting failed"}, timeoutSender: &timeoutSender{cfg: NewDefaultTimeoutSettings()}, set: set, From 9ea3a12d25cb7429d3d28438f978f6f716246c1f Mon Sep 17 00:00:00 2001 From: Bryan Aguilar Date: Thu, 11 Jan 2024 17:21:19 -0800 Subject: [PATCH 4/4] Change test table struc --- exporter/exporterhelper/common_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/exporter/exporterhelper/common_test.go b/exporter/exporterhelper/common_test.go index 4061510482e..648e3a9f044 100644 --- a/exporter/exporterhelper/common_test.go +++ b/exporter/exporterhelper/common_test.go @@ -86,11 +86,11 @@ func TestQueueRetryOptionsWithRequestExporter(t *testing.T) { func TestBaseExporterLogging(t *testing.T) { tests := []struct { name string - exporterOptions func() []Option + exporterOptions []Option }{ { "no options", - func() []Option { return nil }, + nil, }, { "with retry", @@ -98,7 +98,7 @@ func TestBaseExporterLogging(t *testing.T) { rCfg := configretry.NewDefaultBackOffConfig() rCfg.Enabled = false return []Option{WithRetry(rCfg)} - }, + }(), }, } for _, test := range tests { @@ -106,7 +106,7 @@ func TestBaseExporterLogging(t *testing.T) { logger, observed := observer.New(zap.DebugLevel) set.Logger = zap.New(logger) - bs, err := newBaseExporter(set, "", true, nil, nil, newNoopObsrepSender, test.exporterOptions()...) + bs, err := newBaseExporter(set, "", true, nil, nil, newNoopObsrepSender, test.exporterOptions...) require.Nil(t, err) require.True(t, bs.requestExporter) sendErr := bs.send(context.Background(), newErrorRequest())