From 1faf91e7b1e040ef336247e012e685a7e54a595c Mon Sep 17 00:00:00 2001 From: Afzal Ansari Date: Sun, 23 Jul 2023 06:49:47 +0000 Subject: [PATCH 01/13] adds otel tracer to metrics reader component Signed-off-by: Afzal Ansari --- pkg/jtracer/jtracer.go | 2 +- plugin/metrics/prometheus/factory.go | 10 ++++- .../metrics/prometheus/metricsstore/reader.go | 37 +++++++++++-------- .../prometheus/metricsstore/reader_test.go | 21 +++++++---- 4 files changed, 46 insertions(+), 24 deletions(-) diff --git a/pkg/jtracer/jtracer.go b/pkg/jtracer/jtracer.go index 32e9c56c241..80741102946 100644 --- a/pkg/jtracer/jtracer.go +++ b/pkg/jtracer/jtracer.go @@ -61,7 +61,7 @@ func New(serviceName string) (*JTracer, error) { } func NoOp() *JTracer { - return &JTracer{OT: opentracing.NoopTracer{}, OTEL: trace.NewNoopTracerProvider()} + return &JTracer{OT: opentracing.NoopTracer{}, OTEL: trace.NewNoopTracerProvider(), closer: func(ctx context.Context) error { return nil }} } // initOTEL initializes OTEL Tracer diff --git a/plugin/metrics/prometheus/factory.go b/plugin/metrics/prometheus/factory.go index a29416d397a..35488926c25 100644 --- a/plugin/metrics/prometheus/factory.go +++ b/plugin/metrics/prometheus/factory.go @@ -16,10 +16,12 @@ package prometheus import ( "flag" + "log" "github.com/spf13/viper" "go.uber.org/zap" + "github.com/jaegertracing/jaeger/pkg/jtracer" "github.com/jaegertracing/jaeger/plugin" prometheusstore "github.com/jaegertracing/jaeger/plugin/metrics/prometheus/metricsstore" "github.com/jaegertracing/jaeger/storage/metricsstore" @@ -31,12 +33,18 @@ var _ plugin.Configurable = (*Factory)(nil) type Factory struct { options *Options logger *zap.Logger + tracer *jtracer.JTracer } // NewFactory creates a new Factory. func NewFactory() *Factory { + jt, err := jtracer.New("metricsreader") + if err != nil { + log.Fatal("Failed to initialize tracer: %w", err) + } return &Factory{ options: NewOptions("prometheus"), + tracer: jt, } } @@ -60,5 +68,5 @@ func (f *Factory) Initialize(logger *zap.Logger) error { // CreateMetricsReader implements storage.MetricsFactory. func (f *Factory) CreateMetricsReader() (metricsstore.Reader, error) { - return prometheusstore.NewMetricsReader(f.logger, f.options.Primary.Configuration) + return prometheusstore.NewMetricsReader(f.logger, f.options.Primary.Configuration, f.tracer) } diff --git a/plugin/metrics/prometheus/metricsstore/reader.go b/plugin/metrics/prometheus/metricsstore/reader.go index 0cf6852183d..54b760b5fc4 100644 --- a/plugin/metrics/prometheus/metricsstore/reader.go +++ b/plugin/metrics/prometheus/metricsstore/reader.go @@ -26,14 +26,15 @@ import ( "time" "unicode" - "github.com/opentracing/opentracing-go" - ottag "github.com/opentracing/opentracing-go/ext" - otlog "github.com/opentracing/opentracing-go/log" "github.com/prometheus/client_golang/api" promapi "github.com/prometheus/client_golang/api/prometheus/v1" + "go.opentelemetry.io/otel/attribute" + semconv "go.opentelemetry.io/otel/semconv/v1.20.0" + "go.opentelemetry.io/otel/trace" "go.uber.org/zap" "github.com/jaegertracing/jaeger/pkg/bearertoken" + "github.com/jaegertracing/jaeger/pkg/jtracer" "github.com/jaegertracing/jaeger/pkg/prometheus/config" "github.com/jaegertracing/jaeger/plugin/metrics/prometheus/metricsstore/dbmodel" "github.com/jaegertracing/jaeger/proto-gen/api_v2/metrics" @@ -54,6 +55,7 @@ type ( latencyMetricName string callsMetricName string operationLabel string + tracer *jtracer.JTracer } promQueryParams struct { @@ -73,7 +75,7 @@ type ( ) // NewMetricsReader returns a new MetricsReader. -func NewMetricsReader(logger *zap.Logger, cfg config.Configuration) (*MetricsReader, error) { +func NewMetricsReader(logger *zap.Logger, cfg config.Configuration, jt *jtracer.JTracer) (*MetricsReader, error) { logger.Info("Creating metrics reader", zap.Any("configuration", cfg)) roundTripper, err := getHTTPRoundTripper(&cfg, logger) @@ -101,6 +103,7 @@ func NewMetricsReader(logger *zap.Logger, cfg config.Configuration) (*MetricsRea callsMetricName: buildFullCallsMetricName(cfg), latencyMetricName: buildFullLatencyMetricName(cfg), operationLabel: operationLabel, + tracer: jt, } logger.Info("Prometheus reader initialized", zap.String("addr", cfg.ServerURL)) @@ -224,8 +227,11 @@ func (m MetricsReader) executeQuery(ctx context.Context, p metricsQueryParams) ( } promQuery := m.buildPromQuery(p) - span, ctx := startSpanForQuery(ctx, p.metricName, promQuery) - defer span.Finish() + ctx, span := startSpanForQuery(ctx, p.metricName, promQuery, m.tracer.OTEL) + defer span.End() + if err := m.tracer.Close(ctx); err != nil { + m.logger.Error("Error shutting down tracer provider", zap.Error(err)) + } queryRange := promapi.Range{ Start: p.EndTime.Add(-1 * *p.Lookback), @@ -287,17 +293,18 @@ func promqlDurationString(d *time.Duration) string { return string(b) } -func startSpanForQuery(ctx context.Context, metricName, query string) (opentracing.Span, context.Context) { - span, ctx := opentracing.StartSpanFromContext(ctx, metricName) - ottag.DBStatement.Set(span, query) - ottag.DBType.Set(span, "prometheus") - ottag.Component.Set(span, "promql") - return span, ctx +func startSpanForQuery(ctx context.Context, metricName, query string, tp trace.TracerProvider) (context.Context, trace.Span) { + ctx, span := tp.Tracer("prom-metrics-reader").Start(ctx, metricName) + span.SetAttributes( + attribute.Key(semconv.DBStatementKey).String(query), + attribute.Key("db.type").String("prometheus"), + attribute.Key("component").String("promql"), + ) + return ctx, span } -func logErrorToSpan(span opentracing.Span, err error) { - ottag.Error.Set(span, true) - span.LogFields(otlog.Error(err)) +func logErrorToSpan(span trace.Span, err error) { + span.AddEvent(err.Error(), trace.WithAttributes(semconv.OTelStatusCodeError)) } func getHTTPRoundTripper(c *config.Configuration, logger *zap.Logger) (rt http.RoundTripper, err error) { diff --git a/plugin/metrics/prometheus/metricsstore/reader_test.go b/plugin/metrics/prometheus/metricsstore/reader_test.go index cf027f8d6a0..23e996df6e3 100644 --- a/plugin/metrics/prometheus/metricsstore/reader_test.go +++ b/plugin/metrics/prometheus/metricsstore/reader_test.go @@ -33,6 +33,7 @@ import ( "github.com/jaegertracing/jaeger/pkg/bearertoken" "github.com/jaegertracing/jaeger/pkg/config/tlscfg" + "github.com/jaegertracing/jaeger/pkg/jtracer" "github.com/jaegertracing/jaeger/pkg/prometheus/config" "github.com/jaegertracing/jaeger/proto-gen/api_v2/metrics" "github.com/jaegertracing/jaeger/storage/metricsstore" @@ -63,20 +64,22 @@ var defaultConfig = config.Configuration{ func TestNewMetricsReaderValidAddress(t *testing.T) { logger := zap.NewNop() + tracer := jtracer.NoOp() reader, err := NewMetricsReader(logger, config.Configuration{ ServerURL: "http://localhost:1234", ConnectTimeout: defaultTimeout, - }) + }, tracer) require.NoError(t, err) assert.NotNil(t, reader) } func TestNewMetricsReaderInvalidAddress(t *testing.T) { logger := zap.NewNop() + tracer := jtracer.NoOp() reader, err := NewMetricsReader(logger, config.Configuration{ ServerURL: "\n", ConnectTimeout: defaultTimeout, - }) + }, tracer) require.Error(t, err) assert.Contains(t, err.Error(), "failed to initialize prometheus client") assert.Nil(t, reader) @@ -85,6 +88,7 @@ func TestNewMetricsReaderInvalidAddress(t *testing.T) { func TestGetMinStepDuration(t *testing.T) { params := metricsstore.MinStepDurationQueryParameters{} logger := zap.NewNop() + tracer := jtracer.NoOp() listener, err := net.Listen("tcp", "localhost:") require.NoError(t, err) assert.NotNil(t, listener) @@ -92,7 +96,7 @@ func TestGetMinStepDuration(t *testing.T) { reader, err := NewMetricsReader(logger, config.Configuration{ ServerURL: "http://" + listener.Addr().String(), ConnectTimeout: defaultTimeout, - }) + }, tracer) require.NoError(t, err) minStep, err := reader.GetMinStepDuration(context.Background(), ¶ms) @@ -121,11 +125,12 @@ func TestMetricsServerError(t *testing.T) { defer mockPrometheus.Close() logger := zap.NewNop() + tracer := jtracer.NoOp() address := mockPrometheus.Listener.Addr().String() reader, err := NewMetricsReader(logger, config.Configuration{ ServerURL: "http://" + address, ConnectTimeout: defaultTimeout, - }) + }, tracer) require.NoError(t, err) m, err := reader.GetCallRates(context.Background(), ¶ms) @@ -465,7 +470,7 @@ func TestInvalidLatencyUnit(t *testing.T) { NormalizeDuration: true, LatencyUnit: "something invalid", } - _, _ = NewMetricsReader(zap.NewNop(), cfg) + _, _ = NewMetricsReader(zap.NewNop(), cfg, jtracer.NoOp()) } func TestWarningResponse(t *testing.T) { @@ -571,6 +576,7 @@ func TestGetRoundTripperTokenError(t *testing.T) { func TestInvalidCertFile(t *testing.T) { logger := zap.NewNop() + tracer := jtracer.NoOp() reader, err := NewMetricsReader(logger, config.Configuration{ ServerURL: "https://localhost:1234", ConnectTimeout: defaultTimeout, @@ -578,7 +584,7 @@ func TestInvalidCertFile(t *testing.T) { Enabled: true, CAPath: "foo", }, - }) + }, tracer) require.Error(t, err) assert.Nil(t, reader) } @@ -639,12 +645,13 @@ func prepareMetricsReaderAndServer(t *testing.T, config config.Configuration, wa mockPrometheus := startMockPrometheusServer(t, wantPromQlQuery, wantWarnings) logger := zap.NewNop() + tracer := jtracer.NoOp() address := mockPrometheus.Listener.Addr().String() config.ServerURL = "http://" + address config.ConnectTimeout = defaultTimeout - reader, err := NewMetricsReader(logger, config) + reader, err := NewMetricsReader(logger, config, tracer) require.NoError(t, err) return reader, mockPrometheus From faaecd4d53f86eb5e3b3192b8d3636d0aef956e7 Mon Sep 17 00:00:00 2001 From: Afzal Ansari Date: Mon, 24 Jul 2023 19:05:24 +0000 Subject: [PATCH 02/13] updates the tracer to use global tp Signed-off-by: Afzal Ansari --- plugin/metrics/prometheus/factory.go | 10 +-------- .../metrics/prometheus/metricsstore/reader.go | 18 +++++++--------- .../prometheus/metricsstore/reader_test.go | 21 +++++++------------ 3 files changed, 15 insertions(+), 34 deletions(-) diff --git a/plugin/metrics/prometheus/factory.go b/plugin/metrics/prometheus/factory.go index 35488926c25..a29416d397a 100644 --- a/plugin/metrics/prometheus/factory.go +++ b/plugin/metrics/prometheus/factory.go @@ -16,12 +16,10 @@ package prometheus import ( "flag" - "log" "github.com/spf13/viper" "go.uber.org/zap" - "github.com/jaegertracing/jaeger/pkg/jtracer" "github.com/jaegertracing/jaeger/plugin" prometheusstore "github.com/jaegertracing/jaeger/plugin/metrics/prometheus/metricsstore" "github.com/jaegertracing/jaeger/storage/metricsstore" @@ -33,18 +31,12 @@ var _ plugin.Configurable = (*Factory)(nil) type Factory struct { options *Options logger *zap.Logger - tracer *jtracer.JTracer } // NewFactory creates a new Factory. func NewFactory() *Factory { - jt, err := jtracer.New("metricsreader") - if err != nil { - log.Fatal("Failed to initialize tracer: %w", err) - } return &Factory{ options: NewOptions("prometheus"), - tracer: jt, } } @@ -68,5 +60,5 @@ func (f *Factory) Initialize(logger *zap.Logger) error { // CreateMetricsReader implements storage.MetricsFactory. func (f *Factory) CreateMetricsReader() (metricsstore.Reader, error) { - return prometheusstore.NewMetricsReader(f.logger, f.options.Primary.Configuration, f.tracer) + return prometheusstore.NewMetricsReader(f.logger, f.options.Primary.Configuration) } diff --git a/plugin/metrics/prometheus/metricsstore/reader.go b/plugin/metrics/prometheus/metricsstore/reader.go index 54b760b5fc4..9bad3ba3090 100644 --- a/plugin/metrics/prometheus/metricsstore/reader.go +++ b/plugin/metrics/prometheus/metricsstore/reader.go @@ -28,13 +28,13 @@ import ( "github.com/prometheus/client_golang/api" promapi "github.com/prometheus/client_golang/api/prometheus/v1" + "go.opentelemetry.io/otel" "go.opentelemetry.io/otel/attribute" semconv "go.opentelemetry.io/otel/semconv/v1.20.0" "go.opentelemetry.io/otel/trace" "go.uber.org/zap" "github.com/jaegertracing/jaeger/pkg/bearertoken" - "github.com/jaegertracing/jaeger/pkg/jtracer" "github.com/jaegertracing/jaeger/pkg/prometheus/config" "github.com/jaegertracing/jaeger/plugin/metrics/prometheus/metricsstore/dbmodel" "github.com/jaegertracing/jaeger/proto-gen/api_v2/metrics" @@ -55,7 +55,6 @@ type ( latencyMetricName string callsMetricName string operationLabel string - tracer *jtracer.JTracer } promQueryParams struct { @@ -75,7 +74,7 @@ type ( ) // NewMetricsReader returns a new MetricsReader. -func NewMetricsReader(logger *zap.Logger, cfg config.Configuration, jt *jtracer.JTracer) (*MetricsReader, error) { +func NewMetricsReader(logger *zap.Logger, cfg config.Configuration) (*MetricsReader, error) { logger.Info("Creating metrics reader", zap.Any("configuration", cfg)) roundTripper, err := getHTTPRoundTripper(&cfg, logger) @@ -103,7 +102,6 @@ func NewMetricsReader(logger *zap.Logger, cfg config.Configuration, jt *jtracer. callsMetricName: buildFullCallsMetricName(cfg), latencyMetricName: buildFullLatencyMetricName(cfg), operationLabel: operationLabel, - tracer: jt, } logger.Info("Prometheus reader initialized", zap.String("addr", cfg.ServerURL)) @@ -227,11 +225,8 @@ func (m MetricsReader) executeQuery(ctx context.Context, p metricsQueryParams) ( } promQuery := m.buildPromQuery(p) - ctx, span := startSpanForQuery(ctx, p.metricName, promQuery, m.tracer.OTEL) + ctx, span := startSpanForQuery(ctx, p.metricName, promQuery) defer span.End() - if err := m.tracer.Close(ctx); err != nil { - m.logger.Error("Error shutting down tracer provider", zap.Error(err)) - } queryRange := promapi.Range{ Start: p.EndTime.Add(-1 * *p.Lookback), @@ -293,18 +288,19 @@ func promqlDurationString(d *time.Duration) string { return string(b) } -func startSpanForQuery(ctx context.Context, metricName, query string, tp trace.TracerProvider) (context.Context, trace.Span) { +func startSpanForQuery(ctx context.Context, metricName, query string) (context.Context, trace.Span) { + tp := otel.GetTracerProvider() ctx, span := tp.Tracer("prom-metrics-reader").Start(ctx, metricName) span.SetAttributes( attribute.Key(semconv.DBStatementKey).String(query), - attribute.Key("db.type").String("prometheus"), + attribute.Key(semconv.DBSystemKey).String("prometheus"), attribute.Key("component").String("promql"), ) return ctx, span } func logErrorToSpan(span trace.Span, err error) { - span.AddEvent(err.Error(), trace.WithAttributes(semconv.OTelStatusCodeError)) + span.RecordError(err, trace.WithAttributes(semconv.OTelStatusCodeError)) } func getHTTPRoundTripper(c *config.Configuration, logger *zap.Logger) (rt http.RoundTripper, err error) { diff --git a/plugin/metrics/prometheus/metricsstore/reader_test.go b/plugin/metrics/prometheus/metricsstore/reader_test.go index 23e996df6e3..cf027f8d6a0 100644 --- a/plugin/metrics/prometheus/metricsstore/reader_test.go +++ b/plugin/metrics/prometheus/metricsstore/reader_test.go @@ -33,7 +33,6 @@ import ( "github.com/jaegertracing/jaeger/pkg/bearertoken" "github.com/jaegertracing/jaeger/pkg/config/tlscfg" - "github.com/jaegertracing/jaeger/pkg/jtracer" "github.com/jaegertracing/jaeger/pkg/prometheus/config" "github.com/jaegertracing/jaeger/proto-gen/api_v2/metrics" "github.com/jaegertracing/jaeger/storage/metricsstore" @@ -64,22 +63,20 @@ var defaultConfig = config.Configuration{ func TestNewMetricsReaderValidAddress(t *testing.T) { logger := zap.NewNop() - tracer := jtracer.NoOp() reader, err := NewMetricsReader(logger, config.Configuration{ ServerURL: "http://localhost:1234", ConnectTimeout: defaultTimeout, - }, tracer) + }) require.NoError(t, err) assert.NotNil(t, reader) } func TestNewMetricsReaderInvalidAddress(t *testing.T) { logger := zap.NewNop() - tracer := jtracer.NoOp() reader, err := NewMetricsReader(logger, config.Configuration{ ServerURL: "\n", ConnectTimeout: defaultTimeout, - }, tracer) + }) require.Error(t, err) assert.Contains(t, err.Error(), "failed to initialize prometheus client") assert.Nil(t, reader) @@ -88,7 +85,6 @@ func TestNewMetricsReaderInvalidAddress(t *testing.T) { func TestGetMinStepDuration(t *testing.T) { params := metricsstore.MinStepDurationQueryParameters{} logger := zap.NewNop() - tracer := jtracer.NoOp() listener, err := net.Listen("tcp", "localhost:") require.NoError(t, err) assert.NotNil(t, listener) @@ -96,7 +92,7 @@ func TestGetMinStepDuration(t *testing.T) { reader, err := NewMetricsReader(logger, config.Configuration{ ServerURL: "http://" + listener.Addr().String(), ConnectTimeout: defaultTimeout, - }, tracer) + }) require.NoError(t, err) minStep, err := reader.GetMinStepDuration(context.Background(), ¶ms) @@ -125,12 +121,11 @@ func TestMetricsServerError(t *testing.T) { defer mockPrometheus.Close() logger := zap.NewNop() - tracer := jtracer.NoOp() address := mockPrometheus.Listener.Addr().String() reader, err := NewMetricsReader(logger, config.Configuration{ ServerURL: "http://" + address, ConnectTimeout: defaultTimeout, - }, tracer) + }) require.NoError(t, err) m, err := reader.GetCallRates(context.Background(), ¶ms) @@ -470,7 +465,7 @@ func TestInvalidLatencyUnit(t *testing.T) { NormalizeDuration: true, LatencyUnit: "something invalid", } - _, _ = NewMetricsReader(zap.NewNop(), cfg, jtracer.NoOp()) + _, _ = NewMetricsReader(zap.NewNop(), cfg) } func TestWarningResponse(t *testing.T) { @@ -576,7 +571,6 @@ func TestGetRoundTripperTokenError(t *testing.T) { func TestInvalidCertFile(t *testing.T) { logger := zap.NewNop() - tracer := jtracer.NoOp() reader, err := NewMetricsReader(logger, config.Configuration{ ServerURL: "https://localhost:1234", ConnectTimeout: defaultTimeout, @@ -584,7 +578,7 @@ func TestInvalidCertFile(t *testing.T) { Enabled: true, CAPath: "foo", }, - }, tracer) + }) require.Error(t, err) assert.Nil(t, reader) } @@ -645,13 +639,12 @@ func prepareMetricsReaderAndServer(t *testing.T, config config.Configuration, wa mockPrometheus := startMockPrometheusServer(t, wantPromQlQuery, wantWarnings) logger := zap.NewNop() - tracer := jtracer.NoOp() address := mockPrometheus.Listener.Addr().String() config.ServerURL = "http://" + address config.ConnectTimeout = defaultTimeout - reader, err := NewMetricsReader(logger, config, tracer) + reader, err := NewMetricsReader(logger, config) require.NoError(t, err) return reader, mockPrometheus From ec7d45dd6d1f9e3c2073e070631e32e1f77d9c89 Mon Sep 17 00:00:00 2001 From: Afzal Ansari Date: Tue, 25 Jul 2023 11:46:07 +0000 Subject: [PATCH 03/13] adds condition to jt closer Signed-off-by: Afzal Ansari --- pkg/jtracer/jtracer.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/pkg/jtracer/jtracer.go b/pkg/jtracer/jtracer.go index 80741102946..7a95441451c 100644 --- a/pkg/jtracer/jtracer.go +++ b/pkg/jtracer/jtracer.go @@ -61,7 +61,7 @@ func New(serviceName string) (*JTracer, error) { } func NoOp() *JTracer { - return &JTracer{OT: opentracing.NoopTracer{}, OTEL: trace.NewNoopTracerProvider(), closer: func(ctx context.Context) error { return nil }} + return &JTracer{OT: opentracing.NoopTracer{}, OTEL: trace.NewNoopTracerProvider(), closer: nil} } // initOTEL initializes OTEL Tracer @@ -107,5 +107,8 @@ func otelExporter(ctx context.Context) (sdktrace.SpanExporter, error) { // Shutdown the tracerProvider to clean up resources func (jt *JTracer) Close(ctx context.Context) error { - return jt.closer(ctx) + if jt.closer != nil { + return jt.closer(ctx) + } + return nil } From ffcc85e75015f3863c4127f52d60215b9db4cb7d Mon Sep 17 00:00:00 2001 From: Afzal Ansari Date: Tue, 25 Jul 2023 15:22:19 +0000 Subject: [PATCH 04/13] adds tp to main and creates tp in test Signed-off-by: Afzal Ansari --- plugin/metrics/prometheus/factory.go | 6 ++- .../metrics/prometheus/metricsstore/reader.go | 12 ++--- .../prometheus/metricsstore/reader_test.go | 46 ++++++++++++++----- 3 files changed, 45 insertions(+), 19 deletions(-) diff --git a/plugin/metrics/prometheus/factory.go b/plugin/metrics/prometheus/factory.go index a29416d397a..60452061d67 100644 --- a/plugin/metrics/prometheus/factory.go +++ b/plugin/metrics/prometheus/factory.go @@ -18,6 +18,8 @@ import ( "flag" "github.com/spf13/viper" + "go.opentelemetry.io/otel" + "go.opentelemetry.io/otel/trace" "go.uber.org/zap" "github.com/jaegertracing/jaeger/plugin" @@ -31,11 +33,13 @@ var _ plugin.Configurable = (*Factory)(nil) type Factory struct { options *Options logger *zap.Logger + tracer trace.TracerProvider } // NewFactory creates a new Factory. func NewFactory() *Factory { return &Factory{ + tracer: otel.GetTracerProvider(), options: NewOptions("prometheus"), } } @@ -60,5 +64,5 @@ func (f *Factory) Initialize(logger *zap.Logger) error { // CreateMetricsReader implements storage.MetricsFactory. func (f *Factory) CreateMetricsReader() (metricsstore.Reader, error) { - return prometheusstore.NewMetricsReader(f.logger, f.options.Primary.Configuration) + return prometheusstore.NewMetricsReader(f.options.Primary.Configuration, f.logger, f.tracer) } diff --git a/plugin/metrics/prometheus/metricsstore/reader.go b/plugin/metrics/prometheus/metricsstore/reader.go index 9bad3ba3090..0c68687af5e 100644 --- a/plugin/metrics/prometheus/metricsstore/reader.go +++ b/plugin/metrics/prometheus/metricsstore/reader.go @@ -28,7 +28,6 @@ import ( "github.com/prometheus/client_golang/api" promapi "github.com/prometheus/client_golang/api/prometheus/v1" - "go.opentelemetry.io/otel" "go.opentelemetry.io/otel/attribute" semconv "go.opentelemetry.io/otel/semconv/v1.20.0" "go.opentelemetry.io/otel/trace" @@ -50,6 +49,7 @@ type ( MetricsReader struct { client promapi.API logger *zap.Logger + tracer trace.Tracer metricsTranslator dbmodel.Translator latencyMetricName string @@ -74,7 +74,7 @@ type ( ) // NewMetricsReader returns a new MetricsReader. -func NewMetricsReader(logger *zap.Logger, cfg config.Configuration) (*MetricsReader, error) { +func NewMetricsReader(cfg config.Configuration, logger *zap.Logger, tracer trace.TracerProvider) (*MetricsReader, error) { logger.Info("Creating metrics reader", zap.Any("configuration", cfg)) roundTripper, err := getHTTPRoundTripper(&cfg, logger) @@ -97,6 +97,7 @@ func NewMetricsReader(logger *zap.Logger, cfg config.Configuration) (*MetricsRea mr := &MetricsReader{ client: promapi.NewAPI(client), logger: logger, + tracer: tracer.Tracer("prom-metrics-reader"), metricsTranslator: dbmodel.New(operationLabel), callsMetricName: buildFullCallsMetricName(cfg), @@ -225,7 +226,7 @@ func (m MetricsReader) executeQuery(ctx context.Context, p metricsQueryParams) ( } promQuery := m.buildPromQuery(p) - ctx, span := startSpanForQuery(ctx, p.metricName, promQuery) + ctx, span := startSpanForQuery(ctx, p.metricName, promQuery, m.tracer) defer span.End() queryRange := promapi.Range{ @@ -288,9 +289,8 @@ func promqlDurationString(d *time.Duration) string { return string(b) } -func startSpanForQuery(ctx context.Context, metricName, query string) (context.Context, trace.Span) { - tp := otel.GetTracerProvider() - ctx, span := tp.Tracer("prom-metrics-reader").Start(ctx, metricName) +func startSpanForQuery(ctx context.Context, metricName, query string, tp trace.Tracer) (context.Context, trace.Span) { + ctx, span := tp.Start(ctx, metricName) span.SetAttributes( attribute.Key(semconv.DBStatementKey).String(query), attribute.Key(semconv.DBSystemKey).String("prometheus"), diff --git a/plugin/metrics/prometheus/metricsstore/reader_test.go b/plugin/metrics/prometheus/metricsstore/reader_test.go index cf027f8d6a0..45268abca41 100644 --- a/plugin/metrics/prometheus/metricsstore/reader_test.go +++ b/plugin/metrics/prometheus/metricsstore/reader_test.go @@ -29,6 +29,9 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + stdout "go.opentelemetry.io/otel/exporters/stdout/stdouttrace" + sdktrace "go.opentelemetry.io/otel/sdk/trace" + "go.opentelemetry.io/otel/trace" "go.uber.org/zap" "github.com/jaegertracing/jaeger/pkg/bearertoken" @@ -61,22 +64,36 @@ var defaultConfig = config.Configuration{ LatencyUnit: "ms", } +func createTracingProvider() trace.TracerProvider { + exporter, err := stdout.New(stdout.WithPrettyPrint()) + if err != nil { + panic(err) + } + tp := sdktrace.NewTracerProvider( + sdktrace.WithSampler(sdktrace.AlwaysSample()), + sdktrace.WithBatcher(exporter), + ) + return tp +} + func TestNewMetricsReaderValidAddress(t *testing.T) { logger := zap.NewNop() - reader, err := NewMetricsReader(logger, config.Configuration{ + tracer := createTracingProvider() + reader, err := NewMetricsReader(config.Configuration{ ServerURL: "http://localhost:1234", ConnectTimeout: defaultTimeout, - }) + }, logger, tracer) require.NoError(t, err) assert.NotNil(t, reader) } func TestNewMetricsReaderInvalidAddress(t *testing.T) { logger := zap.NewNop() - reader, err := NewMetricsReader(logger, config.Configuration{ + tracer := createTracingProvider() + reader, err := NewMetricsReader(config.Configuration{ ServerURL: "\n", ConnectTimeout: defaultTimeout, - }) + }, logger, tracer) require.Error(t, err) assert.Contains(t, err.Error(), "failed to initialize prometheus client") assert.Nil(t, reader) @@ -85,14 +102,15 @@ func TestNewMetricsReaderInvalidAddress(t *testing.T) { func TestGetMinStepDuration(t *testing.T) { params := metricsstore.MinStepDurationQueryParameters{} logger := zap.NewNop() + tracer := createTracingProvider() listener, err := net.Listen("tcp", "localhost:") require.NoError(t, err) assert.NotNil(t, listener) - reader, err := NewMetricsReader(logger, config.Configuration{ + reader, err := NewMetricsReader(config.Configuration{ ServerURL: "http://" + listener.Addr().String(), ConnectTimeout: defaultTimeout, - }) + }, logger, tracer) require.NoError(t, err) minStep, err := reader.GetMinStepDuration(context.Background(), ¶ms) @@ -121,11 +139,12 @@ func TestMetricsServerError(t *testing.T) { defer mockPrometheus.Close() logger := zap.NewNop() + tracer := createTracingProvider() address := mockPrometheus.Listener.Addr().String() - reader, err := NewMetricsReader(logger, config.Configuration{ + reader, err := NewMetricsReader(config.Configuration{ ServerURL: "http://" + address, ConnectTimeout: defaultTimeout, - }) + }, logger, tracer) require.NoError(t, err) m, err := reader.GetCallRates(context.Background(), ¶ms) @@ -460,12 +479,13 @@ func TestInvalidLatencyUnit(t *testing.T) { t.Errorf("Expected a panic due to invalid latency unit") } }() + tracer := createTracingProvider() cfg := config.Configuration{ SupportSpanmetricsConnector: true, NormalizeDuration: true, LatencyUnit: "something invalid", } - _, _ = NewMetricsReader(zap.NewNop(), cfg) + _, _ = NewMetricsReader(cfg, zap.NewNop(), tracer) } func TestWarningResponse(t *testing.T) { @@ -571,14 +591,15 @@ func TestGetRoundTripperTokenError(t *testing.T) { func TestInvalidCertFile(t *testing.T) { logger := zap.NewNop() - reader, err := NewMetricsReader(logger, config.Configuration{ + tracer := createTracingProvider() + reader, err := NewMetricsReader(config.Configuration{ ServerURL: "https://localhost:1234", ConnectTimeout: defaultTimeout, TLS: tlscfg.Options{ Enabled: true, CAPath: "foo", }, - }) + }, logger, tracer) require.Error(t, err) assert.Nil(t, reader) } @@ -639,12 +660,13 @@ func prepareMetricsReaderAndServer(t *testing.T, config config.Configuration, wa mockPrometheus := startMockPrometheusServer(t, wantPromQlQuery, wantWarnings) logger := zap.NewNop() + tracer := createTracingProvider() address := mockPrometheus.Listener.Addr().String() config.ServerURL = "http://" + address config.ConnectTimeout = defaultTimeout - reader, err := NewMetricsReader(logger, config) + reader, err := NewMetricsReader(config, logger, tracer) require.NoError(t, err) return reader, mockPrometheus From 4852cec4e5829e528aaefc30f5327bad0e6bdc73 Mon Sep 17 00:00:00 2001 From: Afzal Ansari Date: Tue, 25 Jul 2023 17:49:11 +0000 Subject: [PATCH 05/13] adds exporter.GetSpan() to test recorded spans Signed-off-by: Afzal Ansari --- .../prometheus/metricsstore/reader_test.go | 33 +++++++++++-------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/plugin/metrics/prometheus/metricsstore/reader_test.go b/plugin/metrics/prometheus/metricsstore/reader_test.go index 45268abca41..6ce4ce3379b 100644 --- a/plugin/metrics/prometheus/metricsstore/reader_test.go +++ b/plugin/metrics/prometheus/metricsstore/reader_test.go @@ -29,8 +29,8 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - stdout "go.opentelemetry.io/otel/exporters/stdout/stdouttrace" sdktrace "go.opentelemetry.io/otel/sdk/trace" + "go.opentelemetry.io/otel/sdk/trace/tracetest" "go.opentelemetry.io/otel/trace" "go.uber.org/zap" @@ -64,36 +64,36 @@ var defaultConfig = config.Configuration{ LatencyUnit: "ms", } -func createTracingProvider() trace.TracerProvider { - exporter, err := stdout.New(stdout.WithPrettyPrint()) - if err != nil { - panic(err) - } +func tracerProvider(t *testing.T) (trace.TracerProvider, *tracetest.InMemoryExporter) { + exporter := tracetest.NewInMemoryExporter() tp := sdktrace.NewTracerProvider( sdktrace.WithSampler(sdktrace.AlwaysSample()), sdktrace.WithBatcher(exporter), ) - return tp + defer tp.Shutdown(context.Background()) + return tp, exporter } func TestNewMetricsReaderValidAddress(t *testing.T) { logger := zap.NewNop() - tracer := createTracingProvider() + tracer, exp := tracerProvider(t) reader, err := NewMetricsReader(config.Configuration{ ServerURL: "http://localhost:1234", ConnectTimeout: defaultTimeout, }, logger, tracer) + assert.NotEmpty(t, exp.GetSpans(), "No spans recorded during the test.") require.NoError(t, err) assert.NotNil(t, reader) } func TestNewMetricsReaderInvalidAddress(t *testing.T) { logger := zap.NewNop() - tracer := createTracingProvider() + tracer, exp := tracerProvider(t) reader, err := NewMetricsReader(config.Configuration{ ServerURL: "\n", ConnectTimeout: defaultTimeout, }, logger, tracer) + assert.NotEmpty(t, exp.GetSpans(), "No spans recorded during the test.") require.Error(t, err) assert.Contains(t, err.Error(), "failed to initialize prometheus client") assert.Nil(t, reader) @@ -102,7 +102,7 @@ func TestNewMetricsReaderInvalidAddress(t *testing.T) { func TestGetMinStepDuration(t *testing.T) { params := metricsstore.MinStepDurationQueryParameters{} logger := zap.NewNop() - tracer := createTracingProvider() + tracer, exp := tracerProvider(t) listener, err := net.Listen("tcp", "localhost:") require.NoError(t, err) assert.NotNil(t, listener) @@ -111,6 +111,7 @@ func TestGetMinStepDuration(t *testing.T) { ServerURL: "http://" + listener.Addr().String(), ConnectTimeout: defaultTimeout, }, logger, tracer) + assert.NotEmpty(t, exp.GetSpans(), "No spans recorded during the test.") require.NoError(t, err) minStep, err := reader.GetMinStepDuration(context.Background(), ¶ms) @@ -139,12 +140,13 @@ func TestMetricsServerError(t *testing.T) { defer mockPrometheus.Close() logger := zap.NewNop() - tracer := createTracingProvider() + tracer, exp := tracerProvider(t) address := mockPrometheus.Listener.Addr().String() reader, err := NewMetricsReader(config.Configuration{ ServerURL: "http://" + address, ConnectTimeout: defaultTimeout, }, logger, tracer) + assert.NotEmpty(t, exp.GetSpans(), "No spans recorded during the test.") require.NoError(t, err) m, err := reader.GetCallRates(context.Background(), ¶ms) @@ -479,12 +481,13 @@ func TestInvalidLatencyUnit(t *testing.T) { t.Errorf("Expected a panic due to invalid latency unit") } }() - tracer := createTracingProvider() + tracer, exp := tracerProvider(t) cfg := config.Configuration{ SupportSpanmetricsConnector: true, NormalizeDuration: true, LatencyUnit: "something invalid", } + assert.NotEmpty(t, exp.GetSpans(), "No spans recorded during the test.") _, _ = NewMetricsReader(cfg, zap.NewNop(), tracer) } @@ -591,7 +594,7 @@ func TestGetRoundTripperTokenError(t *testing.T) { func TestInvalidCertFile(t *testing.T) { logger := zap.NewNop() - tracer := createTracingProvider() + tracer, exp := tracerProvider(t) reader, err := NewMetricsReader(config.Configuration{ ServerURL: "https://localhost:1234", ConnectTimeout: defaultTimeout, @@ -600,6 +603,7 @@ func TestInvalidCertFile(t *testing.T) { CAPath: "foo", }, }, logger, tracer) + assert.NotEmpty(t, exp.GetSpans(), "No spans recorded during the test.") require.Error(t, err) assert.Nil(t, reader) } @@ -660,12 +664,13 @@ func prepareMetricsReaderAndServer(t *testing.T, config config.Configuration, wa mockPrometheus := startMockPrometheusServer(t, wantPromQlQuery, wantWarnings) logger := zap.NewNop() - tracer := createTracingProvider() + tracer, exp := tracerProvider(t) address := mockPrometheus.Listener.Addr().String() config.ServerURL = "http://" + address config.ConnectTimeout = defaultTimeout + assert.NotEmpty(t, exp.GetSpans(), "No spans recorded during the test.") reader, err := NewMetricsReader(config, logger, tracer) require.NoError(t, err) From a6b2884a4821e90738de317a8412b3e461b2d8a1 Mon Sep 17 00:00:00 2001 From: Afzal Ansari Date: Wed, 26 Jul 2023 09:43:55 +0000 Subject: [PATCH 06/13] fixes the span creation for query Signed-off-by: Afzal Ansari --- .../prometheus/metricsstore/reader_test.go | 39 ++++++++++--------- 1 file changed, 20 insertions(+), 19 deletions(-) diff --git a/plugin/metrics/prometheus/metricsstore/reader_test.go b/plugin/metrics/prometheus/metricsstore/reader_test.go index 6ce4ce3379b..d1f6f3911e9 100644 --- a/plugin/metrics/prometheus/metricsstore/reader_test.go +++ b/plugin/metrics/prometheus/metricsstore/reader_test.go @@ -68,32 +68,29 @@ func tracerProvider(t *testing.T) (trace.TracerProvider, *tracetest.InMemoryExpo exporter := tracetest.NewInMemoryExporter() tp := sdktrace.NewTracerProvider( sdktrace.WithSampler(sdktrace.AlwaysSample()), - sdktrace.WithBatcher(exporter), + sdktrace.WithSyncer(exporter), ) - defer tp.Shutdown(context.Background()) return tp, exporter } func TestNewMetricsReaderValidAddress(t *testing.T) { logger := zap.NewNop() - tracer, exp := tracerProvider(t) + tracer, _ := tracerProvider(t) reader, err := NewMetricsReader(config.Configuration{ ServerURL: "http://localhost:1234", ConnectTimeout: defaultTimeout, }, logger, tracer) - assert.NotEmpty(t, exp.GetSpans(), "No spans recorded during the test.") require.NoError(t, err) assert.NotNil(t, reader) } func TestNewMetricsReaderInvalidAddress(t *testing.T) { logger := zap.NewNop() - tracer, exp := tracerProvider(t) + tracer, _ := tracerProvider(t) reader, err := NewMetricsReader(config.Configuration{ ServerURL: "\n", ConnectTimeout: defaultTimeout, }, logger, tracer) - assert.NotEmpty(t, exp.GetSpans(), "No spans recorded during the test.") require.Error(t, err) assert.Contains(t, err.Error(), "failed to initialize prometheus client") assert.Nil(t, reader) @@ -102,7 +99,7 @@ func TestNewMetricsReaderInvalidAddress(t *testing.T) { func TestGetMinStepDuration(t *testing.T) { params := metricsstore.MinStepDurationQueryParameters{} logger := zap.NewNop() - tracer, exp := tracerProvider(t) + tracer, _ := tracerProvider(t) listener, err := net.Listen("tcp", "localhost:") require.NoError(t, err) assert.NotNil(t, listener) @@ -111,7 +108,6 @@ func TestGetMinStepDuration(t *testing.T) { ServerURL: "http://" + listener.Addr().String(), ConnectTimeout: defaultTimeout, }, logger, tracer) - assert.NotEmpty(t, exp.GetSpans(), "No spans recorded during the test.") require.NoError(t, err) minStep, err := reader.GetMinStepDuration(context.Background(), ¶ms) @@ -146,12 +142,12 @@ func TestMetricsServerError(t *testing.T) { ServerURL: "http://" + address, ConnectTimeout: defaultTimeout, }, logger, tracer) - assert.NotEmpty(t, exp.GetSpans(), "No spans recorded during the test.") require.NoError(t, err) m, err := reader.GetCallRates(context.Background(), ¶ms) assert.NotNil(t, m) - + assert.NotEmpty(t, exp.GetSpans(), "No spans recorded during the test.") + defer require.Error(t, err) assert.Contains(t, err.Error(), "failed executing metrics query") } @@ -242,15 +238,17 @@ func TestGetLatencies(t *testing.T) { BaseQueryParameters: buildTestBaseQueryParametersFrom(tc), Quantile: 0.95, } + tracer, exp := tracerProvider(t) cfg := defaultConfig if tc.updateConfig != nil { cfg = tc.updateConfig(cfg) } - reader, mockPrometheus := prepareMetricsReaderAndServer(t, cfg, tc.wantPromQlQuery, nil) + reader, mockPrometheus := prepareMetricsReaderAndServer(t, cfg, tc.wantPromQlQuery, nil, tracer) defer mockPrometheus.Close() m, err := reader.GetLatencies(context.Background(), ¶ms) require.NoError(t, err) + assert.NotEmpty(t, exp.GetSpans(), "No spans recorded during the test.") assertMetrics(t, m, tc.wantLabels, tc.wantName, tc.wantDescription) }) } @@ -339,15 +337,17 @@ func TestGetCallRates(t *testing.T) { params := metricsstore.CallRateQueryParameters{ BaseQueryParameters: buildTestBaseQueryParametersFrom(tc), } + tracer, exp := tracerProvider(t) cfg := defaultConfig if tc.updateConfig != nil { cfg = tc.updateConfig(cfg) } - reader, mockPrometheus := prepareMetricsReaderAndServer(t, cfg, tc.wantPromQlQuery, nil) + reader, mockPrometheus := prepareMetricsReaderAndServer(t, cfg, tc.wantPromQlQuery, nil, tracer) defer mockPrometheus.Close() m, err := reader.GetCallRates(context.Background(), ¶ms) require.NoError(t, err) + assert.NotEmpty(t, exp.GetSpans(), "No spans recorded during the test.") assertMetrics(t, m, tc.wantLabels, tc.wantName, tc.wantDescription) }) } @@ -461,15 +461,17 @@ func TestGetErrorRates(t *testing.T) { params := metricsstore.ErrorRateQueryParameters{ BaseQueryParameters: buildTestBaseQueryParametersFrom(tc), } + tracer, exp := tracerProvider(t) cfg := defaultConfig if tc.updateConfig != nil { cfg = tc.updateConfig(cfg) } - reader, mockPrometheus := prepareMetricsReaderAndServer(t, cfg, tc.wantPromQlQuery, nil) + reader, mockPrometheus := prepareMetricsReaderAndServer(t, cfg, tc.wantPromQlQuery, nil, tracer) defer mockPrometheus.Close() m, err := reader.GetErrorRates(context.Background(), ¶ms) require.NoError(t, err) + assert.NotEmpty(t, exp.GetSpans(), "No spans recorded during the test.") assertMetrics(t, m, tc.wantLabels, tc.wantName, tc.wantDescription) }) } @@ -495,11 +497,13 @@ func TestWarningResponse(t *testing.T) { params := metricsstore.ErrorRateQueryParameters{ BaseQueryParameters: buildTestBaseQueryParametersFrom(metricsTestCase{serviceNames: []string{"foo"}}), } - reader, mockPrometheus := prepareMetricsReaderAndServer(t, config.Configuration{}, "", []string{"warning0", "warning1"}) + tracer, exp := tracerProvider(t) + reader, mockPrometheus := prepareMetricsReaderAndServer(t, config.Configuration{}, "", []string{"warning0", "warning1"}, tracer) defer mockPrometheus.Close() m, err := reader.GetErrorRates(context.Background(), ¶ms) require.NoError(t, err) + assert.NotEmpty(t, exp.GetSpans(), "No spans recorded during the test.") assert.NotNil(t, m) } @@ -594,7 +598,7 @@ func TestGetRoundTripperTokenError(t *testing.T) { func TestInvalidCertFile(t *testing.T) { logger := zap.NewNop() - tracer, exp := tracerProvider(t) + tracer, _ := tracerProvider(t) reader, err := NewMetricsReader(config.Configuration{ ServerURL: "https://localhost:1234", ConnectTimeout: defaultTimeout, @@ -603,7 +607,6 @@ func TestInvalidCertFile(t *testing.T) { CAPath: "foo", }, }, logger, tracer) - assert.NotEmpty(t, exp.GetSpans(), "No spans recorded during the test.") require.Error(t, err) assert.Nil(t, reader) } @@ -660,17 +663,15 @@ func buildTestBaseQueryParametersFrom(tc metricsTestCase) metricsstore.BaseQuery } } -func prepareMetricsReaderAndServer(t *testing.T, config config.Configuration, wantPromQlQuery string, wantWarnings []string) (metricsstore.Reader, *httptest.Server) { +func prepareMetricsReaderAndServer(t *testing.T, config config.Configuration, wantPromQlQuery string, wantWarnings []string, tracer trace.TracerProvider) (metricsstore.Reader, *httptest.Server) { mockPrometheus := startMockPrometheusServer(t, wantPromQlQuery, wantWarnings) logger := zap.NewNop() - tracer, exp := tracerProvider(t) address := mockPrometheus.Listener.Addr().String() config.ServerURL = "http://" + address config.ConnectTimeout = defaultTimeout - assert.NotEmpty(t, exp.GetSpans(), "No spans recorded during the test.") reader, err := NewMetricsReader(config, logger, tracer) require.NoError(t, err) From 09b3fbf194f8204ee2b780cd2a9be3341786d17c Mon Sep 17 00:00:00 2001 From: Afzal Ansari Date: Wed, 26 Jul 2023 11:25:00 +0000 Subject: [PATCH 07/13] fixes the shutdown issue Signed-off-by: Afzal Ansari --- .../prometheus/metricsstore/reader_test.go | 49 +++++++++++++------ 1 file changed, 35 insertions(+), 14 deletions(-) diff --git a/plugin/metrics/prometheus/metricsstore/reader_test.go b/plugin/metrics/prometheus/metricsstore/reader_test.go index d1f6f3911e9..3b611d0200d 100644 --- a/plugin/metrics/prometheus/metricsstore/reader_test.go +++ b/plugin/metrics/prometheus/metricsstore/reader_test.go @@ -64,29 +64,34 @@ var defaultConfig = config.Configuration{ LatencyUnit: "ms", } -func tracerProvider(t *testing.T) (trace.TracerProvider, *tracetest.InMemoryExporter) { +func tracerProvider(t *testing.T) (trace.TracerProvider, *tracetest.InMemoryExporter, func(ctx context.Context) error) { exporter := tracetest.NewInMemoryExporter() tp := sdktrace.NewTracerProvider( sdktrace.WithSampler(sdktrace.AlwaysSample()), sdktrace.WithSyncer(exporter), ) - return tp, exporter + closer := func(ctx context.Context) error { + return tp.Shutdown(ctx) + } + return tp, exporter, closer } func TestNewMetricsReaderValidAddress(t *testing.T) { logger := zap.NewNop() - tracer, _ := tracerProvider(t) + tracer, _, closer := tracerProvider(t) reader, err := NewMetricsReader(config.Configuration{ ServerURL: "http://localhost:1234", ConnectTimeout: defaultTimeout, }, logger, tracer) require.NoError(t, err) assert.NotNil(t, reader) + err = closer(context.Background()) + require.NoError(t, err) } func TestNewMetricsReaderInvalidAddress(t *testing.T) { logger := zap.NewNop() - tracer, _ := tracerProvider(t) + tracer, _, closer := tracerProvider(t) reader, err := NewMetricsReader(config.Configuration{ ServerURL: "\n", ConnectTimeout: defaultTimeout, @@ -94,12 +99,14 @@ func TestNewMetricsReaderInvalidAddress(t *testing.T) { require.Error(t, err) assert.Contains(t, err.Error(), "failed to initialize prometheus client") assert.Nil(t, reader) + err = closer(context.Background()) + require.NoError(t, err) } func TestGetMinStepDuration(t *testing.T) { params := metricsstore.MinStepDurationQueryParameters{} logger := zap.NewNop() - tracer, _ := tracerProvider(t) + tracer, _, closer := tracerProvider(t) listener, err := net.Listen("tcp", "localhost:") require.NoError(t, err) assert.NotNil(t, listener) @@ -113,6 +120,8 @@ func TestGetMinStepDuration(t *testing.T) { minStep, err := reader.GetMinStepDuration(context.Background(), ¶ms) require.NoError(t, err) assert.Equal(t, time.Millisecond, minStep) + err = closer(context.Background()) + require.NoError(t, err) } func TestMetricsServerError(t *testing.T) { @@ -136,7 +145,7 @@ func TestMetricsServerError(t *testing.T) { defer mockPrometheus.Close() logger := zap.NewNop() - tracer, exp := tracerProvider(t) + tracer, exp, closer := tracerProvider(t) address := mockPrometheus.Listener.Addr().String() reader, err := NewMetricsReader(config.Configuration{ ServerURL: "http://" + address, @@ -147,9 +156,10 @@ func TestMetricsServerError(t *testing.T) { m, err := reader.GetCallRates(context.Background(), ¶ms) assert.NotNil(t, m) assert.NotEmpty(t, exp.GetSpans(), "No spans recorded during the test.") - defer require.Error(t, err) assert.Contains(t, err.Error(), "failed executing metrics query") + err = closer(context.Background()) + require.NoError(t, err) } func TestGetLatencies(t *testing.T) { @@ -238,7 +248,7 @@ func TestGetLatencies(t *testing.T) { BaseQueryParameters: buildTestBaseQueryParametersFrom(tc), Quantile: 0.95, } - tracer, exp := tracerProvider(t) + tracer, exp, closer := tracerProvider(t) cfg := defaultConfig if tc.updateConfig != nil { cfg = tc.updateConfig(cfg) @@ -250,6 +260,8 @@ func TestGetLatencies(t *testing.T) { require.NoError(t, err) assert.NotEmpty(t, exp.GetSpans(), "No spans recorded during the test.") assertMetrics(t, m, tc.wantLabels, tc.wantName, tc.wantDescription) + err = closer(context.Background()) + require.NoError(t, err) }) } } @@ -337,7 +349,7 @@ func TestGetCallRates(t *testing.T) { params := metricsstore.CallRateQueryParameters{ BaseQueryParameters: buildTestBaseQueryParametersFrom(tc), } - tracer, exp := tracerProvider(t) + tracer, exp, closer := tracerProvider(t) cfg := defaultConfig if tc.updateConfig != nil { cfg = tc.updateConfig(cfg) @@ -349,6 +361,8 @@ func TestGetCallRates(t *testing.T) { require.NoError(t, err) assert.NotEmpty(t, exp.GetSpans(), "No spans recorded during the test.") assertMetrics(t, m, tc.wantLabels, tc.wantName, tc.wantDescription) + err = closer(context.Background()) + require.NoError(t, err) }) } } @@ -461,7 +475,7 @@ func TestGetErrorRates(t *testing.T) { params := metricsstore.ErrorRateQueryParameters{ BaseQueryParameters: buildTestBaseQueryParametersFrom(tc), } - tracer, exp := tracerProvider(t) + tracer, exp, closer := tracerProvider(t) cfg := defaultConfig if tc.updateConfig != nil { cfg = tc.updateConfig(cfg) @@ -473,6 +487,8 @@ func TestGetErrorRates(t *testing.T) { require.NoError(t, err) assert.NotEmpty(t, exp.GetSpans(), "No spans recorded during the test.") assertMetrics(t, m, tc.wantLabels, tc.wantName, tc.wantDescription) + err = closer(context.Background()) + require.NoError(t, err) }) } } @@ -483,21 +499,22 @@ func TestInvalidLatencyUnit(t *testing.T) { t.Errorf("Expected a panic due to invalid latency unit") } }() - tracer, exp := tracerProvider(t) + tracer, _, closer := tracerProvider(t) cfg := config.Configuration{ SupportSpanmetricsConnector: true, NormalizeDuration: true, LatencyUnit: "something invalid", } - assert.NotEmpty(t, exp.GetSpans(), "No spans recorded during the test.") _, _ = NewMetricsReader(cfg, zap.NewNop(), tracer) + err := closer(context.Background()) + require.NoError(t, err) } func TestWarningResponse(t *testing.T) { params := metricsstore.ErrorRateQueryParameters{ BaseQueryParameters: buildTestBaseQueryParametersFrom(metricsTestCase{serviceNames: []string{"foo"}}), } - tracer, exp := tracerProvider(t) + tracer, exp, closer := tracerProvider(t) reader, mockPrometheus := prepareMetricsReaderAndServer(t, config.Configuration{}, "", []string{"warning0", "warning1"}, tracer) defer mockPrometheus.Close() @@ -505,6 +522,8 @@ func TestWarningResponse(t *testing.T) { require.NoError(t, err) assert.NotEmpty(t, exp.GetSpans(), "No spans recorded during the test.") assert.NotNil(t, m) + err = closer(context.Background()) + require.NoError(t, err) } func TestGetRoundTripperTLSConfig(t *testing.T) { @@ -598,7 +617,7 @@ func TestGetRoundTripperTokenError(t *testing.T) { func TestInvalidCertFile(t *testing.T) { logger := zap.NewNop() - tracer, _ := tracerProvider(t) + tracer, _, closer := tracerProvider(t) reader, err := NewMetricsReader(config.Configuration{ ServerURL: "https://localhost:1234", ConnectTimeout: defaultTimeout, @@ -609,6 +628,8 @@ func TestInvalidCertFile(t *testing.T) { }, logger, tracer) require.Error(t, err) assert.Nil(t, reader) + err = closer(context.Background()) + require.NoError(t, err) } func startMockPrometheusServer(t *testing.T, wantPromQlQuery string, wantWarnings []string) *httptest.Server { From a0dcb35a1f16f0a7fe038c6215d438f056b48bee Mon Sep 17 00:00:00 2001 From: Afzal Ansari Date: Thu, 27 Jul 2023 05:57:53 +0000 Subject: [PATCH 08/13] validates the no. of spans reported Signed-off-by: Afzal Ansari --- .../prometheus/metricsstore/reader_test.go | 51 +++++++++---------- 1 file changed, 23 insertions(+), 28 deletions(-) diff --git a/plugin/metrics/prometheus/metricsstore/reader_test.go b/plugin/metrics/prometheus/metricsstore/reader_test.go index 3b611d0200d..c6cced1c968 100644 --- a/plugin/metrics/prometheus/metricsstore/reader_test.go +++ b/plugin/metrics/prometheus/metricsstore/reader_test.go @@ -64,14 +64,14 @@ var defaultConfig = config.Configuration{ LatencyUnit: "ms", } -func tracerProvider(t *testing.T) (trace.TracerProvider, *tracetest.InMemoryExporter, func(ctx context.Context) error) { +func tracerProvider(t *testing.T) (trace.TracerProvider, *tracetest.InMemoryExporter, func()) { exporter := tracetest.NewInMemoryExporter() tp := sdktrace.NewTracerProvider( sdktrace.WithSampler(sdktrace.AlwaysSample()), sdktrace.WithSyncer(exporter), ) - closer := func(ctx context.Context) error { - return tp.Shutdown(ctx) + closer := func() { + assert.NoError(t, tp.Shutdown(context.Background())) } return tp, exporter, closer } @@ -85,8 +85,7 @@ func TestNewMetricsReaderValidAddress(t *testing.T) { }, logger, tracer) require.NoError(t, err) assert.NotNil(t, reader) - err = closer(context.Background()) - require.NoError(t, err) + defer closer() } func TestNewMetricsReaderInvalidAddress(t *testing.T) { @@ -99,8 +98,7 @@ func TestNewMetricsReaderInvalidAddress(t *testing.T) { require.Error(t, err) assert.Contains(t, err.Error(), "failed to initialize prometheus client") assert.Nil(t, reader) - err = closer(context.Background()) - require.NoError(t, err) + defer closer() } func TestGetMinStepDuration(t *testing.T) { @@ -120,8 +118,7 @@ func TestGetMinStepDuration(t *testing.T) { minStep, err := reader.GetMinStepDuration(context.Background(), ¶ms) require.NoError(t, err) assert.Equal(t, time.Millisecond, minStep) - err = closer(context.Background()) - require.NoError(t, err) + defer closer() } func TestMetricsServerError(t *testing.T) { @@ -155,11 +152,11 @@ func TestMetricsServerError(t *testing.T) { m, err := reader.GetCallRates(context.Background(), ¶ms) assert.NotNil(t, m) - assert.NotEmpty(t, exp.GetSpans(), "No spans recorded during the test.") require.Error(t, err) assert.Contains(t, err.Error(), "failed executing metrics query") - err = closer(context.Background()) - require.NoError(t, err) + assert.NotEmpty(t, exp.GetSpans(), "Expected spans are recorded") + assert.Len(t, exp.GetSpans(), 1, "HTTP request was traced and span reported") + defer closer() } func TestGetLatencies(t *testing.T) { @@ -258,10 +255,10 @@ func TestGetLatencies(t *testing.T) { m, err := reader.GetLatencies(context.Background(), ¶ms) require.NoError(t, err) - assert.NotEmpty(t, exp.GetSpans(), "No spans recorded during the test.") + assert.NotEmpty(t, exp.GetSpans(), "Spans recorded during the test.") + assert.Len(t, exp.GetSpans(), 1, "HTTP request was traced and span reported") assertMetrics(t, m, tc.wantLabels, tc.wantName, tc.wantDescription) - err = closer(context.Background()) - require.NoError(t, err) + defer closer() }) } } @@ -359,10 +356,10 @@ func TestGetCallRates(t *testing.T) { m, err := reader.GetCallRates(context.Background(), ¶ms) require.NoError(t, err) - assert.NotEmpty(t, exp.GetSpans(), "No spans recorded during the test.") + assert.NotEmpty(t, exp.GetSpans(), "Spans recorded during the test.") + assert.Len(t, exp.GetSpans(), 1, "HTTP request was traced and span reported") assertMetrics(t, m, tc.wantLabels, tc.wantName, tc.wantDescription) - err = closer(context.Background()) - require.NoError(t, err) + defer closer() }) } } @@ -485,10 +482,10 @@ func TestGetErrorRates(t *testing.T) { m, err := reader.GetErrorRates(context.Background(), ¶ms) require.NoError(t, err) - assert.NotEmpty(t, exp.GetSpans(), "No spans recorded during the test.") + assert.NotEmpty(t, exp.GetSpans(), "Spans recorded during the test.") + assert.Len(t, exp.GetSpans(), 1, "HTTP request was traced and span reported") assertMetrics(t, m, tc.wantLabels, tc.wantName, tc.wantDescription) - err = closer(context.Background()) - require.NoError(t, err) + defer closer() }) } } @@ -506,8 +503,7 @@ func TestInvalidLatencyUnit(t *testing.T) { LatencyUnit: "something invalid", } _, _ = NewMetricsReader(cfg, zap.NewNop(), tracer) - err := closer(context.Background()) - require.NoError(t, err) + defer closer() } func TestWarningResponse(t *testing.T) { @@ -520,10 +516,10 @@ func TestWarningResponse(t *testing.T) { m, err := reader.GetErrorRates(context.Background(), ¶ms) require.NoError(t, err) - assert.NotEmpty(t, exp.GetSpans(), "No spans recorded during the test.") + assert.NotEmpty(t, exp.GetSpans(), "Spans recorded during the test.") + assert.Len(t, exp.GetSpans(), 1, "HTTP request was traced and span reported") assert.NotNil(t, m) - err = closer(context.Background()) - require.NoError(t, err) + defer closer() } func TestGetRoundTripperTLSConfig(t *testing.T) { @@ -628,8 +624,7 @@ func TestInvalidCertFile(t *testing.T) { }, logger, tracer) require.Error(t, err) assert.Nil(t, reader) - err = closer(context.Background()) - require.NoError(t, err) + defer closer() } func startMockPrometheusServer(t *testing.T, wantPromQlQuery string, wantWarnings []string) *httptest.Server { From e2799058d7f7e9de20e22b0f8d26429c4732dac8 Mon Sep 17 00:00:00 2001 From: Afzal Ansari Date: Thu, 27 Jul 2023 18:35:54 +0000 Subject: [PATCH 09/13] reorders the defer to execute the close Signed-off-by: Afzal Ansari --- .../prometheus/metricsstore/reader_test.go | 28 +++++++++---------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/plugin/metrics/prometheus/metricsstore/reader_test.go b/plugin/metrics/prometheus/metricsstore/reader_test.go index c6cced1c968..d226f5c1ce7 100644 --- a/plugin/metrics/prometheus/metricsstore/reader_test.go +++ b/plugin/metrics/prometheus/metricsstore/reader_test.go @@ -83,9 +83,9 @@ func TestNewMetricsReaderValidAddress(t *testing.T) { ServerURL: "http://localhost:1234", ConnectTimeout: defaultTimeout, }, logger, tracer) + defer closer() require.NoError(t, err) assert.NotNil(t, reader) - defer closer() } func TestNewMetricsReaderInvalidAddress(t *testing.T) { @@ -95,10 +95,10 @@ func TestNewMetricsReaderInvalidAddress(t *testing.T) { ServerURL: "\n", ConnectTimeout: defaultTimeout, }, logger, tracer) + defer closer() require.Error(t, err) assert.Contains(t, err.Error(), "failed to initialize prometheus client") assert.Nil(t, reader) - defer closer() } func TestGetMinStepDuration(t *testing.T) { @@ -113,12 +113,12 @@ func TestGetMinStepDuration(t *testing.T) { ServerURL: "http://" + listener.Addr().String(), ConnectTimeout: defaultTimeout, }, logger, tracer) + defer closer() require.NoError(t, err) minStep, err := reader.GetMinStepDuration(context.Background(), ¶ms) require.NoError(t, err) assert.Equal(t, time.Millisecond, minStep) - defer closer() } func TestMetricsServerError(t *testing.T) { @@ -148,15 +148,15 @@ func TestMetricsServerError(t *testing.T) { ServerURL: "http://" + address, ConnectTimeout: defaultTimeout, }, logger, tracer) + assert.NotEmpty(t, exp.GetSpans(), "Expected spans are recorded") + assert.Len(t, exp.GetSpans(), 1, "HTTP request was traced and span reported") + defer closer() require.NoError(t, err) m, err := reader.GetCallRates(context.Background(), ¶ms) assert.NotNil(t, m) require.Error(t, err) assert.Contains(t, err.Error(), "failed executing metrics query") - assert.NotEmpty(t, exp.GetSpans(), "Expected spans are recorded") - assert.Len(t, exp.GetSpans(), 1, "HTTP request was traced and span reported") - defer closer() } func TestGetLatencies(t *testing.T) { @@ -254,11 +254,11 @@ func TestGetLatencies(t *testing.T) { defer mockPrometheus.Close() m, err := reader.GetLatencies(context.Background(), ¶ms) - require.NoError(t, err) assert.NotEmpty(t, exp.GetSpans(), "Spans recorded during the test.") assert.Len(t, exp.GetSpans(), 1, "HTTP request was traced and span reported") - assertMetrics(t, m, tc.wantLabels, tc.wantName, tc.wantDescription) defer closer() + require.NoError(t, err) + assertMetrics(t, m, tc.wantLabels, tc.wantName, tc.wantDescription) }) } } @@ -355,11 +355,11 @@ func TestGetCallRates(t *testing.T) { defer mockPrometheus.Close() m, err := reader.GetCallRates(context.Background(), ¶ms) - require.NoError(t, err) assert.NotEmpty(t, exp.GetSpans(), "Spans recorded during the test.") assert.Len(t, exp.GetSpans(), 1, "HTTP request was traced and span reported") - assertMetrics(t, m, tc.wantLabels, tc.wantName, tc.wantDescription) defer closer() + require.NoError(t, err) + assertMetrics(t, m, tc.wantLabels, tc.wantName, tc.wantDescription) }) } } @@ -481,11 +481,11 @@ func TestGetErrorRates(t *testing.T) { defer mockPrometheus.Close() m, err := reader.GetErrorRates(context.Background(), ¶ms) - require.NoError(t, err) assert.NotEmpty(t, exp.GetSpans(), "Spans recorded during the test.") assert.Len(t, exp.GetSpans(), 1, "HTTP request was traced and span reported") - assertMetrics(t, m, tc.wantLabels, tc.wantName, tc.wantDescription) defer closer() + require.NoError(t, err) + assertMetrics(t, m, tc.wantLabels, tc.wantName, tc.wantDescription) }) } } @@ -515,11 +515,11 @@ func TestWarningResponse(t *testing.T) { defer mockPrometheus.Close() m, err := reader.GetErrorRates(context.Background(), ¶ms) + defer closer() require.NoError(t, err) assert.NotEmpty(t, exp.GetSpans(), "Spans recorded during the test.") assert.Len(t, exp.GetSpans(), 1, "HTTP request was traced and span reported") assert.NotNil(t, m) - defer closer() } func TestGetRoundTripperTLSConfig(t *testing.T) { @@ -622,9 +622,9 @@ func TestInvalidCertFile(t *testing.T) { CAPath: "foo", }, }, logger, tracer) + defer closer() require.Error(t, err) assert.Nil(t, reader) - defer closer() } func startMockPrometheusServer(t *testing.T, wantPromQlQuery string, wantWarnings []string) *httptest.Server { From 4319e6212a80ac037bcb3cad749280b6f7fa9c3e Mon Sep 17 00:00:00 2001 From: Afzal Ansari Date: Thu, 27 Jul 2023 20:12:50 +0000 Subject: [PATCH 10/13] fixes spans reported err Signed-off-by: Afzal Ansari --- plugin/metrics/prometheus/metricsstore/reader_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/plugin/metrics/prometheus/metricsstore/reader_test.go b/plugin/metrics/prometheus/metricsstore/reader_test.go index d226f5c1ce7..9ddd5ff92be 100644 --- a/plugin/metrics/prometheus/metricsstore/reader_test.go +++ b/plugin/metrics/prometheus/metricsstore/reader_test.go @@ -148,12 +148,12 @@ func TestMetricsServerError(t *testing.T) { ServerURL: "http://" + address, ConnectTimeout: defaultTimeout, }, logger, tracer) - assert.NotEmpty(t, exp.GetSpans(), "Expected spans are recorded") - assert.Len(t, exp.GetSpans(), 1, "HTTP request was traced and span reported") - defer closer() require.NoError(t, err) m, err := reader.GetCallRates(context.Background(), ¶ms) + assert.NotEmpty(t, exp.GetSpans(), "Expected spans are recorded") + assert.Len(t, exp.GetSpans(), 1, "HTTP request was traced and span reported") + defer closer() assert.NotNil(t, m) require.Error(t, err) assert.Contains(t, err.Error(), "failed executing metrics query") From 32959703572f7e57c8a57a3bc9676773b397cc94 Mon Sep 17 00:00:00 2001 From: Afzal Ansari Date: Thu, 27 Jul 2023 23:58:40 +0000 Subject: [PATCH 11/13] moves defer to its obj creation Signed-off-by: Afzal Ansari --- .../prometheus/metricsstore/reader_test.go | 29 ++++++++----------- 1 file changed, 12 insertions(+), 17 deletions(-) diff --git a/plugin/metrics/prometheus/metricsstore/reader_test.go b/plugin/metrics/prometheus/metricsstore/reader_test.go index 9ddd5ff92be..876f74cab8a 100644 --- a/plugin/metrics/prometheus/metricsstore/reader_test.go +++ b/plugin/metrics/prometheus/metricsstore/reader_test.go @@ -79,11 +79,11 @@ func tracerProvider(t *testing.T) (trace.TracerProvider, *tracetest.InMemoryExpo func TestNewMetricsReaderValidAddress(t *testing.T) { logger := zap.NewNop() tracer, _, closer := tracerProvider(t) + defer closer() reader, err := NewMetricsReader(config.Configuration{ ServerURL: "http://localhost:1234", ConnectTimeout: defaultTimeout, }, logger, tracer) - defer closer() require.NoError(t, err) assert.NotNil(t, reader) } @@ -91,11 +91,11 @@ func TestNewMetricsReaderValidAddress(t *testing.T) { func TestNewMetricsReaderInvalidAddress(t *testing.T) { logger := zap.NewNop() tracer, _, closer := tracerProvider(t) + defer closer() reader, err := NewMetricsReader(config.Configuration{ ServerURL: "\n", ConnectTimeout: defaultTimeout, }, logger, tracer) - defer closer() require.Error(t, err) assert.Contains(t, err.Error(), "failed to initialize prometheus client") assert.Nil(t, reader) @@ -105,6 +105,7 @@ func TestGetMinStepDuration(t *testing.T) { params := metricsstore.MinStepDurationQueryParameters{} logger := zap.NewNop() tracer, _, closer := tracerProvider(t) + defer closer() listener, err := net.Listen("tcp", "localhost:") require.NoError(t, err) assert.NotNil(t, listener) @@ -113,7 +114,6 @@ func TestGetMinStepDuration(t *testing.T) { ServerURL: "http://" + listener.Addr().String(), ConnectTimeout: defaultTimeout, }, logger, tracer) - defer closer() require.NoError(t, err) minStep, err := reader.GetMinStepDuration(context.Background(), ¶ms) @@ -143,17 +143,16 @@ func TestMetricsServerError(t *testing.T) { logger := zap.NewNop() tracer, exp, closer := tracerProvider(t) + defer closer() address := mockPrometheus.Listener.Addr().String() reader, err := NewMetricsReader(config.Configuration{ ServerURL: "http://" + address, ConnectTimeout: defaultTimeout, }, logger, tracer) require.NoError(t, err) - m, err := reader.GetCallRates(context.Background(), ¶ms) - assert.NotEmpty(t, exp.GetSpans(), "Expected spans are recorded") + assert.NotNil(t, exp.GetSpans()[0].Status) assert.Len(t, exp.GetSpans(), 1, "HTTP request was traced and span reported") - defer closer() assert.NotNil(t, m) require.Error(t, err) assert.Contains(t, err.Error(), "failed executing metrics query") @@ -246,6 +245,7 @@ func TestGetLatencies(t *testing.T) { Quantile: 0.95, } tracer, exp, closer := tracerProvider(t) + defer closer() cfg := defaultConfig if tc.updateConfig != nil { cfg = tc.updateConfig(cfg) @@ -254,9 +254,7 @@ func TestGetLatencies(t *testing.T) { defer mockPrometheus.Close() m, err := reader.GetLatencies(context.Background(), ¶ms) - assert.NotEmpty(t, exp.GetSpans(), "Spans recorded during the test.") assert.Len(t, exp.GetSpans(), 1, "HTTP request was traced and span reported") - defer closer() require.NoError(t, err) assertMetrics(t, m, tc.wantLabels, tc.wantName, tc.wantDescription) }) @@ -347,6 +345,7 @@ func TestGetCallRates(t *testing.T) { BaseQueryParameters: buildTestBaseQueryParametersFrom(tc), } tracer, exp, closer := tracerProvider(t) + defer closer() cfg := defaultConfig if tc.updateConfig != nil { cfg = tc.updateConfig(cfg) @@ -355,9 +354,7 @@ func TestGetCallRates(t *testing.T) { defer mockPrometheus.Close() m, err := reader.GetCallRates(context.Background(), ¶ms) - assert.NotEmpty(t, exp.GetSpans(), "Spans recorded during the test.") assert.Len(t, exp.GetSpans(), 1, "HTTP request was traced and span reported") - defer closer() require.NoError(t, err) assertMetrics(t, m, tc.wantLabels, tc.wantName, tc.wantDescription) }) @@ -473,6 +470,7 @@ func TestGetErrorRates(t *testing.T) { BaseQueryParameters: buildTestBaseQueryParametersFrom(tc), } tracer, exp, closer := tracerProvider(t) + defer closer() cfg := defaultConfig if tc.updateConfig != nil { cfg = tc.updateConfig(cfg) @@ -481,9 +479,7 @@ func TestGetErrorRates(t *testing.T) { defer mockPrometheus.Close() m, err := reader.GetErrorRates(context.Background(), ¶ms) - assert.NotEmpty(t, exp.GetSpans(), "Spans recorded during the test.") assert.Len(t, exp.GetSpans(), 1, "HTTP request was traced and span reported") - defer closer() require.NoError(t, err) assertMetrics(t, m, tc.wantLabels, tc.wantName, tc.wantDescription) }) @@ -497,13 +493,13 @@ func TestInvalidLatencyUnit(t *testing.T) { } }() tracer, _, closer := tracerProvider(t) + defer closer() cfg := config.Configuration{ SupportSpanmetricsConnector: true, NormalizeDuration: true, LatencyUnit: "something invalid", } _, _ = NewMetricsReader(cfg, zap.NewNop(), tracer) - defer closer() } func TestWarningResponse(t *testing.T) { @@ -511,14 +507,13 @@ func TestWarningResponse(t *testing.T) { BaseQueryParameters: buildTestBaseQueryParametersFrom(metricsTestCase{serviceNames: []string{"foo"}}), } tracer, exp, closer := tracerProvider(t) + defer closer() reader, mockPrometheus := prepareMetricsReaderAndServer(t, config.Configuration{}, "", []string{"warning0", "warning1"}, tracer) defer mockPrometheus.Close() m, err := reader.GetErrorRates(context.Background(), ¶ms) - defer closer() - require.NoError(t, err) - assert.NotEmpty(t, exp.GetSpans(), "Spans recorded during the test.") assert.Len(t, exp.GetSpans(), 1, "HTTP request was traced and span reported") + require.NoError(t, err) assert.NotNil(t, m) } @@ -614,6 +609,7 @@ func TestGetRoundTripperTokenError(t *testing.T) { func TestInvalidCertFile(t *testing.T) { logger := zap.NewNop() tracer, _, closer := tracerProvider(t) + defer closer() reader, err := NewMetricsReader(config.Configuration{ ServerURL: "https://localhost:1234", ConnectTimeout: defaultTimeout, @@ -622,7 +618,6 @@ func TestInvalidCertFile(t *testing.T) { CAPath: "foo", }, }, logger, tracer) - defer closer() require.Error(t, err) assert.Nil(t, reader) } From abeeeb682d037301333d3c433a97e0509b789a87 Mon Sep 17 00:00:00 2001 From: Afzal Ansari Date: Fri, 28 Jul 2023 15:07:06 +0000 Subject: [PATCH 12/13] reorders the assert in the sequence call Signed-off-by: Afzal Ansari --- plugin/metrics/prometheus/metricsstore/reader.go | 3 ++- .../metrics/prometheus/metricsstore/reader_test.go | 12 ++++++------ 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/plugin/metrics/prometheus/metricsstore/reader.go b/plugin/metrics/prometheus/metricsstore/reader.go index 0c68687af5e..99b968f6ef4 100644 --- a/plugin/metrics/prometheus/metricsstore/reader.go +++ b/plugin/metrics/prometheus/metricsstore/reader.go @@ -237,8 +237,9 @@ func (m MetricsReader) executeQuery(ctx context.Context, p metricsQueryParams) ( mv, warnings, err := m.client.QueryRange(ctx, promQuery, queryRange) if err != nil { + err = fmt.Errorf("failed executing metrics query: %w", err) logErrorToSpan(span, err) - return &metrics.MetricFamily{}, fmt.Errorf("failed executing metrics query: %w", err) + return &metrics.MetricFamily{}, err } if len(warnings) > 0 { m.logger.Warn("Warnings detected on Prometheus query", zap.Any("warnings", warnings), zap.String("query", promQuery), zap.Any("range", queryRange)) diff --git a/plugin/metrics/prometheus/metricsstore/reader_test.go b/plugin/metrics/prometheus/metricsstore/reader_test.go index 876f74cab8a..0caf3c84d0b 100644 --- a/plugin/metrics/prometheus/metricsstore/reader_test.go +++ b/plugin/metrics/prometheus/metricsstore/reader_test.go @@ -151,11 +151,11 @@ func TestMetricsServerError(t *testing.T) { }, logger, tracer) require.NoError(t, err) m, err := reader.GetCallRates(context.Background(), ¶ms) - assert.NotNil(t, exp.GetSpans()[0].Status) - assert.Len(t, exp.GetSpans(), 1, "HTTP request was traced and span reported") assert.NotNil(t, m) require.Error(t, err) assert.Contains(t, err.Error(), "failed executing metrics query") + require.Len(t, exp.GetSpans(), 1, "HTTP request was traced and span reported") + assert.Equal(t, "service_call_rate", exp.GetSpans()[0].Name) } func TestGetLatencies(t *testing.T) { @@ -254,9 +254,9 @@ func TestGetLatencies(t *testing.T) { defer mockPrometheus.Close() m, err := reader.GetLatencies(context.Background(), ¶ms) - assert.Len(t, exp.GetSpans(), 1, "HTTP request was traced and span reported") require.NoError(t, err) assertMetrics(t, m, tc.wantLabels, tc.wantName, tc.wantDescription) + assert.Len(t, exp.GetSpans(), 1, "HTTP request was traced and span reported") }) } } @@ -354,9 +354,9 @@ func TestGetCallRates(t *testing.T) { defer mockPrometheus.Close() m, err := reader.GetCallRates(context.Background(), ¶ms) - assert.Len(t, exp.GetSpans(), 1, "HTTP request was traced and span reported") require.NoError(t, err) assertMetrics(t, m, tc.wantLabels, tc.wantName, tc.wantDescription) + assert.Len(t, exp.GetSpans(), 1, "HTTP request was traced and span reported") }) } } @@ -479,9 +479,9 @@ func TestGetErrorRates(t *testing.T) { defer mockPrometheus.Close() m, err := reader.GetErrorRates(context.Background(), ¶ms) - assert.Len(t, exp.GetSpans(), 1, "HTTP request was traced and span reported") require.NoError(t, err) assertMetrics(t, m, tc.wantLabels, tc.wantName, tc.wantDescription) + assert.Len(t, exp.GetSpans(), 1, "HTTP request was traced and span reported") }) } } @@ -512,9 +512,9 @@ func TestWarningResponse(t *testing.T) { defer mockPrometheus.Close() m, err := reader.GetErrorRates(context.Background(), ¶ms) - assert.Len(t, exp.GetSpans(), 1, "HTTP request was traced and span reported") require.NoError(t, err) assert.NotNil(t, m) + assert.Len(t, exp.GetSpans(), 1, "HTTP request was traced and span reported") } func TestGetRoundTripperTLSConfig(t *testing.T) { From 7d4624bc37cfbe7005e01248be1cdc878d2ab53d Mon Sep 17 00:00:00 2001 From: Afzal Ansari Date: Fri, 28 Jul 2023 19:09:39 +0000 Subject: [PATCH 13/13] sets the span status Signed-off-by: Afzal Ansari --- plugin/metrics/prometheus/metricsstore/reader.go | 4 +++- plugin/metrics/prometheus/metricsstore/reader_test.go | 3 ++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/plugin/metrics/prometheus/metricsstore/reader.go b/plugin/metrics/prometheus/metricsstore/reader.go index 99b968f6ef4..3e3a218d6af 100644 --- a/plugin/metrics/prometheus/metricsstore/reader.go +++ b/plugin/metrics/prometheus/metricsstore/reader.go @@ -29,6 +29,7 @@ import ( "github.com/prometheus/client_golang/api" promapi "github.com/prometheus/client_golang/api/prometheus/v1" "go.opentelemetry.io/otel/attribute" + "go.opentelemetry.io/otel/codes" semconv "go.opentelemetry.io/otel/semconv/v1.20.0" "go.opentelemetry.io/otel/trace" "go.uber.org/zap" @@ -301,7 +302,8 @@ func startSpanForQuery(ctx context.Context, metricName, query string, tp trace.T } func logErrorToSpan(span trace.Span, err error) { - span.RecordError(err, trace.WithAttributes(semconv.OTelStatusCodeError)) + span.RecordError(err) + span.SetStatus(codes.Error, err.Error()) } func getHTTPRoundTripper(c *config.Configuration, logger *zap.Logger) (rt http.RoundTripper, err error) { diff --git a/plugin/metrics/prometheus/metricsstore/reader_test.go b/plugin/metrics/prometheus/metricsstore/reader_test.go index 0caf3c84d0b..09fb85ec3df 100644 --- a/plugin/metrics/prometheus/metricsstore/reader_test.go +++ b/plugin/metrics/prometheus/metricsstore/reader_test.go @@ -29,6 +29,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "go.opentelemetry.io/otel/codes" sdktrace "go.opentelemetry.io/otel/sdk/trace" "go.opentelemetry.io/otel/sdk/trace/tracetest" "go.opentelemetry.io/otel/trace" @@ -155,7 +156,7 @@ func TestMetricsServerError(t *testing.T) { require.Error(t, err) assert.Contains(t, err.Error(), "failed executing metrics query") require.Len(t, exp.GetSpans(), 1, "HTTP request was traced and span reported") - assert.Equal(t, "service_call_rate", exp.GetSpans()[0].Name) + assert.Equal(t, codes.Error, exp.GetSpans()[0].Status.Code) } func TestGetLatencies(t *testing.T) {