From 7901da40bb6a45d412460949ce360208129e80f9 Mon Sep 17 00:00:00 2001 From: Vyacheslav Stepanov Date: Mon, 13 Jan 2025 20:48:36 +0200 Subject: [PATCH] [exporter/clickhouse] Fix Nil Pointer Exception on Metrics/Traces export without service.name Resource Attribute (#37034) #### Description Fixing Nil Pointer Exception regression introduced in https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/35725 #### Link to tracking issue Fixes #37030 #### Testing Unit test adjusted to include test Metric without `service.name` Resource Attributes #### Documentation --- .../fix_clickhouseexporter-metrics-npe.yaml | 27 +++++++++++++++++++ exporter/clickhouseexporter/exporter_logs.go | 6 +---- .../exporter_metrics_test.go | 2 +- .../clickhouseexporter/exporter_traces.go | 6 ++--- .../internal/exponential_histogram_metrics.go | 11 ++++---- .../internal/gauge_metrics.go | 11 ++++---- .../internal/histogram_metrics.go | 11 ++++---- .../internal/metrics_model.go | 10 +++++++ .../internal/metrics_model_test.go | 22 +++++++++++++++ .../internal/sum_metrics.go | 11 ++++---- .../internal/summary_metrics.go | 11 ++++---- 11 files changed, 94 insertions(+), 34 deletions(-) create mode 100644 .chloggen/fix_clickhouseexporter-metrics-npe.yaml diff --git a/.chloggen/fix_clickhouseexporter-metrics-npe.yaml b/.chloggen/fix_clickhouseexporter-metrics-npe.yaml new file mode 100644 index 000000000000..3fd8e0cdb56c --- /dev/null +++ b/.chloggen/fix_clickhouseexporter-metrics-npe.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: clickhouseexporter + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: Fix Nil Pointer Exception on Metrics/Traces export without service.name Resource Attribute + +# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists. +issues: [37030] + +# (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] diff --git a/exporter/clickhouseexporter/exporter_logs.go b/exporter/clickhouseexporter/exporter_logs.go index f57fbf97ee42..ba7c8b97c480 100644 --- a/exporter/clickhouseexporter/exporter_logs.go +++ b/exporter/clickhouseexporter/exporter_logs.go @@ -12,7 +12,6 @@ import ( _ "github.com/ClickHouse/clickhouse-go/v2" // For register database driver. "go.opentelemetry.io/collector/component" "go.opentelemetry.io/collector/pdata/plog" - conventions "go.opentelemetry.io/collector/semconv/v1.27.0" "go.uber.org/zap" "github.com/open-telemetry/opentelemetry-collector-contrib/exporter/clickhouseexporter/internal" @@ -77,10 +76,7 @@ func (e *logsExporter) pushLogsData(ctx context.Context, ld plog.Logs) error { res := logs.Resource() resURL := logs.SchemaUrl() resAttr := internal.AttributesToMap(res.Attributes()) - var serviceName string - if v, ok := res.Attributes().Get(conventions.AttributeServiceName); ok { - serviceName = v.Str() - } + serviceName := internal.GetServiceName(res.Attributes()) for j := 0; j < logs.ScopeLogs().Len(); j++ { rs := logs.ScopeLogs().At(j).LogRecords() diff --git a/exporter/clickhouseexporter/exporter_metrics_test.go b/exporter/clickhouseexporter/exporter_metrics_test.go index 2000303cab67..b9b8255bf355 100644 --- a/exporter/clickhouseexporter/exporter_metrics_test.go +++ b/exporter/clickhouseexporter/exporter_metrics_test.go @@ -294,7 +294,7 @@ func simpleMetrics(count int) pmetric.Metrics { } rm = metrics.ResourceMetrics().AppendEmpty() - rm.Resource().Attributes().PutStr("service.name", "demo 2") + // Removed service.name from second metric to test both with/without ServiceName cases rm.Resource().Attributes().PutStr("Resource Attributes 2", "value2") rm.Resource().SetDroppedAttributesCount(20) rm.SetSchemaUrl("Resource SchemaUrl 2") diff --git a/exporter/clickhouseexporter/exporter_traces.go b/exporter/clickhouseexporter/exporter_traces.go index 39a706c60afd..277a60e9b437 100644 --- a/exporter/clickhouseexporter/exporter_traces.go +++ b/exporter/clickhouseexporter/exporter_traces.go @@ -14,7 +14,6 @@ import ( "github.com/ClickHouse/clickhouse-go/v2/lib/column" "go.opentelemetry.io/collector/component" "go.opentelemetry.io/collector/pdata/ptrace" - conventions "go.opentelemetry.io/collector/semconv/v1.27.0" "go.uber.org/zap" "github.com/open-telemetry/opentelemetry-collector-contrib/exporter/clickhouseexporter/internal" @@ -77,7 +76,8 @@ func (e *tracesExporter) pushTraceData(ctx context.Context, td ptrace.Traces) er spans := td.ResourceSpans().At(i) res := spans.Resource() resAttr := internal.AttributesToMap(res.Attributes()) - serviceName, _ := res.Attributes().Get(conventions.AttributeServiceName) + serviceName := internal.GetServiceName(res.Attributes()) + for j := 0; j < spans.ScopeSpans().Len(); j++ { rs := spans.ScopeSpans().At(j).Spans() scopeName := spans.ScopeSpans().At(j).Scope().Name() @@ -96,7 +96,7 @@ func (e *tracesExporter) pushTraceData(ctx context.Context, td ptrace.Traces) er r.TraceState().AsRaw(), r.Name(), r.Kind().String(), - serviceName.AsString(), + serviceName, resAttr, scopeName, scopeVersion, diff --git a/exporter/clickhouseexporter/internal/exponential_histogram_metrics.go b/exporter/clickhouseexporter/internal/exponential_histogram_metrics.go index 064e12a2b234..e74e6456a70a 100644 --- a/exporter/clickhouseexporter/internal/exponential_histogram_metrics.go +++ b/exporter/clickhouseexporter/internal/exponential_histogram_metrics.go @@ -11,7 +11,6 @@ import ( "go.opentelemetry.io/collector/pdata/pcommon" "go.opentelemetry.io/collector/pdata/pmetric" - conventions "go.opentelemetry.io/collector/semconv/v1.27.0" "go.uber.org/zap" ) @@ -130,20 +129,22 @@ func (e *expHistogramMetrics) insert(ctx context.Context, db *sql.DB) error { }() for _, model := range e.expHistogramModels { - serviceName, _ := model.metadata.ResAttr.Get(conventions.AttributeServiceName) + resAttr := AttributesToMap(model.metadata.ResAttr) + scopeAttr := AttributesToMap(model.metadata.ScopeInstr.Attributes()) + serviceName := GetServiceName(model.metadata.ResAttr) for i := 0; i < model.expHistogram.DataPoints().Len(); i++ { dp := model.expHistogram.DataPoints().At(i) attrs, times, values, traceIDs, spanIDs := convertExemplars(dp.Exemplars()) _, err = statement.ExecContext(ctx, - AttributesToMap(model.metadata.ResAttr), + resAttr, model.metadata.ResURL, model.metadata.ScopeInstr.Name(), model.metadata.ScopeInstr.Version(), - AttributesToMap(model.metadata.ScopeInstr.Attributes()), + scopeAttr, model.metadata.ScopeInstr.DroppedAttributesCount(), model.metadata.ScopeURL, - serviceName.AsString(), + serviceName, model.metricName, model.metricDescription, model.metricUnit, diff --git a/exporter/clickhouseexporter/internal/gauge_metrics.go b/exporter/clickhouseexporter/internal/gauge_metrics.go index e2fbfe2dc365..a0fbb4d275f1 100644 --- a/exporter/clickhouseexporter/internal/gauge_metrics.go +++ b/exporter/clickhouseexporter/internal/gauge_metrics.go @@ -11,7 +11,6 @@ import ( "go.opentelemetry.io/collector/pdata/pcommon" "go.opentelemetry.io/collector/pdata/pmetric" - conventions "go.opentelemetry.io/collector/semconv/v1.27.0" "go.uber.org/zap" ) @@ -109,20 +108,22 @@ func (g *gaugeMetrics) insert(ctx context.Context, db *sql.DB) error { }() for _, model := range g.gaugeModels { - serviceName, _ := model.metadata.ResAttr.Get(conventions.AttributeServiceName) + resAttr := AttributesToMap(model.metadata.ResAttr) + scopeAttr := AttributesToMap(model.metadata.ScopeInstr.Attributes()) + serviceName := GetServiceName(model.metadata.ResAttr) for i := 0; i < model.gauge.DataPoints().Len(); i++ { dp := model.gauge.DataPoints().At(i) attrs, times, values, traceIDs, spanIDs := convertExemplars(dp.Exemplars()) _, err = statement.ExecContext(ctx, - AttributesToMap(model.metadata.ResAttr), + resAttr, model.metadata.ResURL, model.metadata.ScopeInstr.Name(), model.metadata.ScopeInstr.Version(), - AttributesToMap(model.metadata.ScopeInstr.Attributes()), + scopeAttr, model.metadata.ScopeInstr.DroppedAttributesCount(), model.metadata.ScopeURL, - serviceName.AsString(), + serviceName, model.metricName, model.metricDescription, model.metricUnit, diff --git a/exporter/clickhouseexporter/internal/histogram_metrics.go b/exporter/clickhouseexporter/internal/histogram_metrics.go index f3374b655ba2..cdd4508722e8 100644 --- a/exporter/clickhouseexporter/internal/histogram_metrics.go +++ b/exporter/clickhouseexporter/internal/histogram_metrics.go @@ -11,7 +11,6 @@ import ( "go.opentelemetry.io/collector/pdata/pcommon" "go.opentelemetry.io/collector/pdata/pmetric" - conventions "go.opentelemetry.io/collector/semconv/v1.27.0" "go.uber.org/zap" ) @@ -121,20 +120,22 @@ func (h *histogramMetrics) insert(ctx context.Context, db *sql.DB) error { }() for _, model := range h.histogramModel { - serviceName, _ := model.metadata.ResAttr.Get(conventions.AttributeServiceName) + resAttr := AttributesToMap(model.metadata.ResAttr) + scopeAttr := AttributesToMap(model.metadata.ScopeInstr.Attributes()) + serviceName := GetServiceName(model.metadata.ResAttr) for i := 0; i < model.histogram.DataPoints().Len(); i++ { dp := model.histogram.DataPoints().At(i) attrs, times, values, traceIDs, spanIDs := convertExemplars(dp.Exemplars()) _, err = statement.ExecContext(ctx, - AttributesToMap(model.metadata.ResAttr), + resAttr, model.metadata.ResURL, model.metadata.ScopeInstr.Name(), model.metadata.ScopeInstr.Version(), - AttributesToMap(model.metadata.ScopeInstr.Attributes()), + scopeAttr, model.metadata.ScopeInstr.DroppedAttributesCount(), model.metadata.ScopeURL, - serviceName.AsString(), + serviceName, model.metricName, model.metricDescription, model.metricUnit, diff --git a/exporter/clickhouseexporter/internal/metrics_model.go b/exporter/clickhouseexporter/internal/metrics_model.go index a412051800c0..7c9377e83fe1 100644 --- a/exporter/clickhouseexporter/internal/metrics_model.go +++ b/exporter/clickhouseexporter/internal/metrics_model.go @@ -17,6 +17,7 @@ import ( "github.com/ClickHouse/clickhouse-go/v2/lib/column/orderedmap" "go.opentelemetry.io/collector/pdata/pcommon" "go.opentelemetry.io/collector/pdata/pmetric" + conventions "go.opentelemetry.io/collector/semconv/v1.27.0" "go.uber.org/zap" ) @@ -175,6 +176,15 @@ func AttributesToMap(attributes pcommon.Map) column.IterableOrderedMap { }, attributes.Len()) } +func GetServiceName(resAttr pcommon.Map) string { + var serviceName string + if v, ok := resAttr.Get(conventions.AttributeServiceName); ok { + serviceName = v.AsString() + } + + return serviceName +} + func convertSliceToArraySet[T any](slice []T) clickhouse.ArraySet { var set clickhouse.ArraySet for _, item := range slice { diff --git a/exporter/clickhouseexporter/internal/metrics_model_test.go b/exporter/clickhouseexporter/internal/metrics_model_test.go index 6b0c53b800c0..8301be087802 100644 --- a/exporter/clickhouseexporter/internal/metrics_model_test.go +++ b/exporter/clickhouseexporter/internal/metrics_model_test.go @@ -12,6 +12,7 @@ import ( "github.com/stretchr/testify/require" "go.opentelemetry.io/collector/pdata/pcommon" "go.opentelemetry.io/collector/pdata/pmetric" + conventions "go.opentelemetry.io/collector/semconv/v1.27.0" "go.uber.org/zap/zaptest" ) @@ -225,3 +226,24 @@ func Test_newPlaceholder(t *testing.T) { expectStr := "(?,?,?,?,?)," require.Equal(t, newPlaceholder(5), &expectStr) } + +func Test_GetServiceName(t *testing.T) { + t.Run("should return empty string on unset service.name", func(t *testing.T) { + require.Equal(t, "", GetServiceName(pcommon.NewMap())) + }) + t.Run("should return correct string from service.name", func(t *testing.T) { + resAttr := pcommon.NewMap() + resAttr.PutStr(conventions.AttributeServiceName, "test-service") + require.Equal(t, "test-service", GetServiceName(resAttr)) + }) + t.Run("should return empty string on empty service.name", func(t *testing.T) { + resAttr := pcommon.NewMap() + resAttr.PutEmpty(conventions.AttributeServiceName) + require.Equal(t, "", GetServiceName(resAttr)) + }) + t.Run("should return string from non-string service.name", func(t *testing.T) { + resAttr := pcommon.NewMap() + resAttr.PutBool(conventions.AttributeServiceName, true) + require.Equal(t, "true", GetServiceName(resAttr)) + }) +} diff --git a/exporter/clickhouseexporter/internal/sum_metrics.go b/exporter/clickhouseexporter/internal/sum_metrics.go index 89455f8e3048..28e5d553a18d 100644 --- a/exporter/clickhouseexporter/internal/sum_metrics.go +++ b/exporter/clickhouseexporter/internal/sum_metrics.go @@ -11,7 +11,6 @@ import ( "go.opentelemetry.io/collector/pdata/pcommon" "go.opentelemetry.io/collector/pdata/pmetric" - conventions "go.opentelemetry.io/collector/semconv/v1.27.0" "go.uber.org/zap" ) @@ -113,20 +112,22 @@ func (s *sumMetrics) insert(ctx context.Context, db *sql.DB) error { }() for _, model := range s.sumModel { - serviceName, _ := model.metadata.ResAttr.Get(conventions.AttributeServiceName) + resAttr := AttributesToMap(model.metadata.ResAttr) + scopeAttr := AttributesToMap(model.metadata.ScopeInstr.Attributes()) + serviceName := GetServiceName(model.metadata.ResAttr) for i := 0; i < model.sum.DataPoints().Len(); i++ { dp := model.sum.DataPoints().At(i) attrs, times, values, traceIDs, spanIDs := convertExemplars(dp.Exemplars()) _, err = statement.ExecContext(ctx, - AttributesToMap(model.metadata.ResAttr), + resAttr, model.metadata.ResURL, model.metadata.ScopeInstr.Name(), model.metadata.ScopeInstr.Version(), - AttributesToMap(model.metadata.ScopeInstr.Attributes()), + scopeAttr, model.metadata.ScopeInstr.DroppedAttributesCount(), model.metadata.ScopeURL, - serviceName.AsString(), + serviceName, model.metricName, model.metricDescription, model.metricUnit, diff --git a/exporter/clickhouseexporter/internal/summary_metrics.go b/exporter/clickhouseexporter/internal/summary_metrics.go index d98197c12b2e..749445ec427c 100644 --- a/exporter/clickhouseexporter/internal/summary_metrics.go +++ b/exporter/clickhouseexporter/internal/summary_metrics.go @@ -11,7 +11,6 @@ import ( "go.opentelemetry.io/collector/pdata/pcommon" "go.opentelemetry.io/collector/pdata/pmetric" - conventions "go.opentelemetry.io/collector/semconv/v1.27.0" "go.uber.org/zap" ) @@ -103,21 +102,23 @@ func (s *summaryMetrics) insert(ctx context.Context, db *sql.DB) error { _ = statement.Close() }() for _, model := range s.summaryModel { - serviceName, _ := model.metadata.ResAttr.Get(conventions.AttributeServiceName) + resAttr := AttributesToMap(model.metadata.ResAttr) + scopeAttr := AttributesToMap(model.metadata.ScopeInstr.Attributes()) + serviceName := GetServiceName(model.metadata.ResAttr) for i := 0; i < model.summary.DataPoints().Len(); i++ { dp := model.summary.DataPoints().At(i) quantiles, values := convertValueAtQuantile(dp.QuantileValues()) _, err = statement.ExecContext(ctx, - AttributesToMap(model.metadata.ResAttr), + resAttr, model.metadata.ResURL, model.metadata.ScopeInstr.Name(), model.metadata.ScopeInstr.Version(), - AttributesToMap(model.metadata.ScopeInstr.Attributes()), + scopeAttr, model.metadata.ScopeInstr.DroppedAttributesCount(), model.metadata.ScopeURL, - serviceName.AsString(), + serviceName, model.metricName, model.metricDescription, model.metricUnit,