From 0d81535b7cb62facb047a94580903f795298131e Mon Sep 17 00:00:00 2001 From: Bogdan Drutu Date: Wed, 26 Feb 2025 19:50:55 -0800 Subject: [PATCH] Fix bug where the error logged when conversion of data fails is always nil (#12510) The problem was that in the logging statement (in all signals) we log `err` instead of `cErr`. The good part is that this is not a UB, because if we get to the point where that function is called, the `err` was nil otherwise and never modified again so not race condition in accessing it. Separate the lambda function in a different function so we don't repeat the same mistake of capturing other params by mistake. Signed-off-by: Bogdan Drutu --- .chloggen/fix-wrong-error-log.yaml | 25 ++++++++++++++++++ exporter/exporterhelper/logs.go | 26 +++++++++++-------- exporter/exporterhelper/metrics.go | 26 +++++++++++-------- exporter/exporterhelper/traces.go | 26 +++++++++++-------- .../xexporterhelper/profiles.go | 26 +++++++++++-------- 5 files changed, 85 insertions(+), 44 deletions(-) create mode 100644 .chloggen/fix-wrong-error-log.yaml diff --git a/.chloggen/fix-wrong-error-log.yaml b/.chloggen/fix-wrong-error-log.yaml new file mode 100644 index 00000000000..144507be08b --- /dev/null +++ b/.chloggen/fix-wrong-error-log.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: Fix bug where the error logged when conversion of data fails is always nil + +# One or more tracking issues or pull requests related to the change +issues: [12510] + +# (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] diff --git a/exporter/exporterhelper/logs.go b/exporter/exporterhelper/logs.go index 30fbb4fa2bb..1fb5244bc15 100644 --- a/exporter/exporterhelper/logs.go +++ b/exporter/exporterhelper/logs.go @@ -139,19 +139,23 @@ func NewLogsRequest( return nil, err } - lc, err := consumer.NewLogs(func(ctx context.Context, ld plog.Logs) error { - req, cErr := converter(ctx, ld) - if cErr != nil { - set.Logger.Error("Failed to convert logs. Dropping data.", + lc, err := consumer.NewLogs(newConsumeLogs(converter, be, set.Logger), be.ConsumerOptions...) + if err != nil { + return nil, err + } + + return &logsExporter{BaseExporter: be, Logs: lc}, nil +} + +func newConsumeLogs(converter RequestFromLogsFunc, be *internal.BaseExporter, logger *zap.Logger) consumer.ConsumeLogsFunc { + return func(ctx context.Context, ld plog.Logs) error { + req, err := converter(ctx, ld) + if err != nil { + logger.Error("Failed to convert metrics. Dropping data.", zap.Int("dropped_log_records", ld.LogRecordCount()), zap.Error(err)) - return consumererror.NewPermanent(cErr) + return consumererror.NewPermanent(err) } return be.Send(ctx, req) - }, be.ConsumerOptions...) - - return &logsExporter{ - BaseExporter: be, - Logs: lc, - }, err + } } diff --git a/exporter/exporterhelper/metrics.go b/exporter/exporterhelper/metrics.go index 0f414706fed..cc4b090e9fe 100644 --- a/exporter/exporterhelper/metrics.go +++ b/exporter/exporterhelper/metrics.go @@ -131,19 +131,23 @@ func NewMetricsRequest( return nil, err } - mc, err := consumer.NewMetrics(func(ctx context.Context, md pmetric.Metrics) error { - req, cErr := converter(ctx, md) - if cErr != nil { - set.Logger.Error("Failed to convert metrics. Dropping data.", + mc, err := consumer.NewMetrics(newConsumeMetrics(converter, be, set.Logger), be.ConsumerOptions...) + if err != nil { + return nil, err + } + + return &metricsExporter{BaseExporter: be, Metrics: mc}, nil +} + +func newConsumeMetrics(converter RequestFromMetricsFunc, be *internal.BaseExporter, logger *zap.Logger) consumer.ConsumeMetricsFunc { + return func(ctx context.Context, md pmetric.Metrics) error { + req, err := converter(ctx, md) + if err != nil { + logger.Error("Failed to convert metrics. Dropping data.", zap.Int("dropped_data_points", md.DataPointCount()), zap.Error(err)) - return consumererror.NewPermanent(cErr) + return consumererror.NewPermanent(err) } return be.Send(ctx, req) - }, be.ConsumerOptions...) - - return &metricsExporter{ - BaseExporter: be, - Metrics: mc, - }, err + } } diff --git a/exporter/exporterhelper/traces.go b/exporter/exporterhelper/traces.go index 03cc3f1566d..602917a7306 100644 --- a/exporter/exporterhelper/traces.go +++ b/exporter/exporterhelper/traces.go @@ -131,19 +131,23 @@ func NewTracesRequest( return nil, err } - tc, err := consumer.NewTraces(func(ctx context.Context, td ptrace.Traces) error { - req, cErr := converter(ctx, td) - if cErr != nil { - set.Logger.Error("Failed to convert traces. Dropping data.", + tc, err := consumer.NewTraces(newConsumeTraces(converter, be, set.Logger), be.ConsumerOptions...) + if err != nil { + return nil, err + } + + return &tracesExporter{BaseExporter: be, Traces: tc}, nil +} + +func newConsumeTraces(converter RequestFromTracesFunc, be *internal.BaseExporter, logger *zap.Logger) consumer.ConsumeTracesFunc { + return func(ctx context.Context, td ptrace.Traces) error { + req, err := converter(ctx, td) + if err != nil { + logger.Error("Failed to convert metrics. Dropping data.", zap.Int("dropped_spans", td.SpanCount()), zap.Error(err)) - return consumererror.NewPermanent(cErr) + return consumererror.NewPermanent(err) } return be.Send(ctx, req) - }, be.ConsumerOptions...) - - return &tracesExporter{ - BaseExporter: be, - Traces: tc, - }, err + } } diff --git a/exporter/exporterhelper/xexporterhelper/profiles.go b/exporter/exporterhelper/xexporterhelper/profiles.go index 0ac80e79f87..f1daf606f43 100644 --- a/exporter/exporterhelper/xexporterhelper/profiles.go +++ b/exporter/exporterhelper/xexporterhelper/profiles.go @@ -134,19 +134,23 @@ func NewProfilesRequestExporter( return nil, err } - tc, err := xconsumer.NewProfiles(func(ctx context.Context, pd pprofile.Profiles) error { - req, cErr := converter(ctx, pd) - if cErr != nil { - set.Logger.Error("Failed to convert profiles. Dropping data.", + tc, err := xconsumer.NewProfiles(newConsumeProfiles(converter, be, set.Logger), be.ConsumerOptions...) + if err != nil { + return nil, err + } + + return &profileExporter{BaseExporter: be, Profiles: tc}, nil +} + +func newConsumeProfiles(converter RequestFromProfilesFunc, be *internal.BaseExporter, logger *zap.Logger) xconsumer.ConsumeProfilesFunc { + return func(ctx context.Context, pd pprofile.Profiles) error { + req, err := converter(ctx, pd) + if err != nil { + logger.Error("Failed to convert metrics. Dropping data.", zap.Int("dropped_samples", pd.SampleCount()), zap.Error(err)) - return consumererror.NewPermanent(cErr) + return consumererror.NewPermanent(err) } return be.Send(ctx, req) - }, be.ConsumerOptions...) - - return &profileExporter{ - BaseExporter: be, - Profiles: tc, - }, err + } }