From 6f0d735d293cc2445346e31e4230bb6213d8707c Mon Sep 17 00:00:00 2001 From: Florian Bacher Date: Wed, 22 May 2024 07:29:23 +0200 Subject: [PATCH 1/5] chore: adapt telemetry setup error handling Signed-off-by: Florian Bacher --- core/pkg/telemetry/metrics.go | 34 +++++++++++++++++++ flagd/pkg/runtime/from_config.go | 9 +++-- .../flag-evaluation/connect_service.go | 20 ++++++----- .../service/flag-evaluation/flag_evaluator.go | 22 ++++++++---- .../flag-evaluation/flag_evaluator_v2.go | 14 +++++--- 5 files changed, 77 insertions(+), 22 deletions(-) diff --git a/core/pkg/telemetry/metrics.go b/core/pkg/telemetry/metrics.go index 76d077a66..dcb3c7ea8 100644 --- a/core/pkg/telemetry/metrics.go +++ b/core/pkg/telemetry/metrics.go @@ -26,6 +26,40 @@ const ( reasonMetric = "feature_flag." + ProviderName + ".evaluation.reason" ) +type IMetricsRecorder interface { + HTTPAttributes(svcName, url, method, code string) []attribute.KeyValue + HTTPRequestDuration(ctx context.Context, duration time.Duration, attrs []attribute.KeyValue) + HTTPResponseSize(ctx context.Context, sizeBytes int64, attrs []attribute.KeyValue) + InFlightRequestStart(ctx context.Context, attrs []attribute.KeyValue) + InFlightRequestEnd(ctx context.Context, attrs []attribute.KeyValue) + RecordEvaluation(ctx context.Context, err error, reason, variant, key string) + Impressions(ctx context.Context, reason, variant, key string) +} + +type NoopMetricsRecorder struct{} + +func (NoopMetricsRecorder) HTTPAttributes(_, _, _, _ string) []attribute.KeyValue { + return []attribute.KeyValue{} +} + +func (NoopMetricsRecorder) HTTPRequestDuration(_ context.Context, _ time.Duration, _ []attribute.KeyValue) { +} + +func (NoopMetricsRecorder) HTTPResponseSize(_ context.Context, _ int64, _ []attribute.KeyValue) { +} + +func (NoopMetricsRecorder) InFlightRequestStart(_ context.Context, _ []attribute.KeyValue) { +} + +func (NoopMetricsRecorder) InFlightRequestEnd(_ context.Context, _ []attribute.KeyValue) { +} + +func (NoopMetricsRecorder) RecordEvaluation(_ context.Context, _ error, _, _, _ string) { +} + +func (NoopMetricsRecorder) Impressions(_ context.Context, _, _, _ string) { +} + type MetricsRecorder struct { httpRequestDurHistogram metric.Float64Histogram httpResponseSizeHistogram metric.Float64Histogram diff --git a/flagd/pkg/runtime/from_config.go b/flagd/pkg/runtime/from_config.go index 28fe43075..0ec0cfd1a 100644 --- a/flagd/pkg/runtime/from_config.go +++ b/flagd/pkg/runtime/from_config.go @@ -51,13 +51,15 @@ func FromConfig(logger *logger.Logger, version string, config Config) (*Runtime, // register trace provider for the runtime err := telemetry.BuildTraceProvider(context.Background(), logger, svcName, version, telCfg) if err != nil { - return nil, fmt.Errorf("error building trace provider: %w", err) + // log the error but continue + logger.Error(fmt.Sprintf("error building trace provider: %v", err)) } // build metrics recorder with startup configurations recorder, err := telemetry.BuildMetricsRecorder(context.Background(), svcName, version, telCfg) if err != nil { - return nil, fmt.Errorf("error building metrics recorder: %w", err) + // log the error but continue + logger.Error(fmt.Sprintf("error building metrics recorder: %v", err)) } // build flag store, collect flag sources & fill sources details @@ -113,7 +115,8 @@ func FromConfig(logger *logger.Logger, version string, config Config) (*Runtime, options, err := telemetry.BuildConnectOptions(telCfg) if err != nil { - return nil, fmt.Errorf("failed to build connect options, %w", err) + // log the error but continue + logger.Error(fmt.Sprintf("failed to build connect options, %v", err)) } return &Runtime{ diff --git a/flagd/pkg/service/flag-evaluation/connect_service.go b/flagd/pkg/service/flag-evaluation/connect_service.go index d66c9c7f5..beb5760d9 100644 --- a/flagd/pkg/service/flag-evaluation/connect_service.go +++ b/flagd/pkg/service/flag-evaluation/connect_service.go @@ -5,6 +5,7 @@ import ( "context" "errors" "fmt" + metricsmw "github.com/open-feature/flagd/flagd/pkg/service/middleware/metrics" "net" "net/http" "strings" @@ -20,7 +21,6 @@ import ( "github.com/open-feature/flagd/flagd/pkg/service/middleware" corsmw "github.com/open-feature/flagd/flagd/pkg/service/middleware/cors" h2cmw "github.com/open-feature/flagd/flagd/pkg/service/middleware/h2c" - metricsmw "github.com/open-feature/flagd/flagd/pkg/service/middleware/metrics" "github.com/prometheus/client_golang/prometheus/promhttp" "go.uber.org/zap" "golang.org/x/net/http2" @@ -183,14 +183,16 @@ func (s *ConnectService) setupServer(svcConf service.Configuration) (net.Listene s.serverMtx.Unlock() // Add middlewares - metricsMiddleware := metricsmw.NewHTTPMetric(metricsmw.Config{ - Service: svcConf.ServiceName, - MetricRecorder: s.metrics, - Logger: s.logger, - HandlerID: "", - }) - - s.AddMiddleware(metricsMiddleware) + if s.metrics != nil { + metricsMiddleware := metricsmw.NewHTTPMetric(metricsmw.Config{ + Service: svcConf.ServiceName, + MetricRecorder: s.metrics, + Logger: s.logger, + HandlerID: "", + }) + + s.AddMiddleware(metricsMiddleware) + } corsMiddleware := corsmw.New(svcConf.CORS) s.AddMiddleware(corsMiddleware) diff --git a/flagd/pkg/service/flag-evaluation/flag_evaluator.go b/flagd/pkg/service/flag-evaluation/flag_evaluator.go index 96a0a60b8..41b83ed42 100644 --- a/flagd/pkg/service/flag-evaluation/flag_evaluator.go +++ b/flagd/pkg/service/flag-evaluation/flag_evaluator.go @@ -29,7 +29,7 @@ type resolverSignature[T constraints] func(context context.Context, reqID, flagK type OldFlagEvaluationService struct { logger *logger.Logger eval evaluator.IEvaluator - metrics *telemetry.MetricsRecorder + metrics telemetry.IMetricsRecorder eventingConfiguration IEvents flagEvalTracer trace.Tracer } @@ -38,13 +38,19 @@ type OldFlagEvaluationService struct { func NewOldFlagEvaluationService(log *logger.Logger, eval evaluator.IEvaluator, eventingCfg IEvents, metricsRecorder *telemetry.MetricsRecorder, ) *OldFlagEvaluationService { - return &OldFlagEvaluationService{ + svc := &OldFlagEvaluationService{ logger: log, eval: eval, - metrics: metricsRecorder, + metrics: &telemetry.NoopMetricsRecorder{}, eventingConfiguration: eventingCfg, flagEvalTracer: otel.Tracer("flagEvaluationService"), } + + if metricsRecorder != nil { + svc.metrics = metricsRecorder + } + + return svc } // nolint:dupl @@ -67,7 +73,9 @@ func (s *OldFlagEvaluationService) ResolveAll( span.SetAttributes(attribute.Int("feature_flag.count", len(values))) for _, value := range values { // register the impression and reason for each flag evaluated - s.metrics.RecordEvaluation(sCtx, value.Error, value.Reason, value.Variant, value.FlagKey) + if s.metrics != nil { + s.metrics.RecordEvaluation(sCtx, value.Error, value.Reason, value.Variant, value.FlagKey) + } switch v := value.Value.(type) { case bool: res.Flags[value.FlagKey] = &schemaV1.AnyFlag{ @@ -276,7 +284,7 @@ func (s *OldFlagEvaluationService) ResolveObject( // resolve is a generic flag resolver func resolve[T constraints](ctx context.Context, logger *logger.Logger, resolver resolverSignature[T], flagKey string, - evaluationContext *structpb.Struct, resp response[T], metrics *telemetry.MetricsRecorder, + evaluationContext *structpb.Struct, resp response[T], metrics telemetry.IMetricsRecorder, ) error { reqID := xid.New().String() defer logger.ClearFields(reqID) @@ -295,7 +303,9 @@ func resolve[T constraints](ctx context.Context, logger *logger.Logger, resolver evalErrFormatted = errFormat(evalErr) } - metrics.RecordEvaluation(ctx, evalErr, reason, variant, flagKey) + if metrics != nil { + metrics.RecordEvaluation(ctx, evalErr, reason, variant, flagKey) + } spanFromContext := trace.SpanFromContext(ctx) spanFromContext.SetAttributes(telemetry.SemConvFeatureFlagAttributes(flagKey, variant)...) diff --git a/flagd/pkg/service/flag-evaluation/flag_evaluator_v2.go b/flagd/pkg/service/flag-evaluation/flag_evaluator_v2.go index 1e78734f2..875a506c9 100644 --- a/flagd/pkg/service/flag-evaluation/flag_evaluator_v2.go +++ b/flagd/pkg/service/flag-evaluation/flag_evaluator_v2.go @@ -22,7 +22,7 @@ import ( type FlagEvaluationService struct { logger *logger.Logger eval evaluator.IEvaluator - metrics *telemetry.MetricsRecorder + metrics telemetry.IMetricsRecorder eventingConfiguration IEvents flagEvalTracer trace.Tracer } @@ -31,15 +31,21 @@ type FlagEvaluationService struct { func NewFlagEvaluationService(log *logger.Logger, eval evaluator.IEvaluator, eventingCfg IEvents, - metricsRecorder *telemetry.MetricsRecorder, + metricsRecorder telemetry.IMetricsRecorder, ) *FlagEvaluationService { - return &FlagEvaluationService{ + svc := &FlagEvaluationService{ logger: log, eval: eval, - metrics: metricsRecorder, + metrics: &telemetry.NoopMetricsRecorder{}, eventingConfiguration: eventingCfg, flagEvalTracer: otel.Tracer("flagd.evaluation.v1"), } + + if metricsRecorder != nil { + svc.metrics = metricsRecorder + } + + return svc } // nolint:dupl,funlen From 4973bd8bcebe7cff83ca14efcd78c03e31d774e3 Mon Sep 17 00:00:00 2001 From: Florian Bacher Date: Wed, 22 May 2024 07:45:15 +0200 Subject: [PATCH 2/5] fix linting Signed-off-by: Florian Bacher --- flagd/pkg/service/flag-evaluation/connect_service.go | 8 ++++---- flagd/pkg/service/flag-evaluation/flag_evaluator.go | 3 ++- flagd/pkg/service/flag-evaluation/flag_evaluator_v2.go | 1 + 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/flagd/pkg/service/flag-evaluation/connect_service.go b/flagd/pkg/service/flag-evaluation/connect_service.go index beb5760d9..2785a6fee 100644 --- a/flagd/pkg/service/flag-evaluation/connect_service.go +++ b/flagd/pkg/service/flag-evaluation/connect_service.go @@ -5,7 +5,6 @@ import ( "context" "errors" "fmt" - metricsmw "github.com/open-feature/flagd/flagd/pkg/service/middleware/metrics" "net" "net/http" "strings" @@ -21,6 +20,7 @@ import ( "github.com/open-feature/flagd/flagd/pkg/service/middleware" corsmw "github.com/open-feature/flagd/flagd/pkg/service/middleware/cors" h2cmw "github.com/open-feature/flagd/flagd/pkg/service/middleware/h2c" + metricsmw "github.com/open-feature/flagd/flagd/pkg/service/middleware/metrics" "github.com/prometheus/client_golang/prometheus/promhttp" "go.uber.org/zap" "golang.org/x/net/http2" @@ -244,8 +244,8 @@ func (s *ConnectService) startServer(svcConf service.Configuration) error { func (s *ConnectService) startMetricsServer(svcConf service.Configuration) error { s.logger.Info(fmt.Sprintf("metrics and probes listening at %d", svcConf.ManagementPort)) - grpc := grpc.NewServer() - grpc_health_v1.RegisterHealthServer(grpc, health.NewServer()) + srv := grpc.NewServer() + grpc_health_v1.RegisterHealthServer(srv, health.NewServer()) mux := http.NewServeMux() mux.Handle("/healthz", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -263,7 +263,7 @@ func (s *ConnectService) startMetricsServer(svcConf service.Configuration) error handler := http.HandlerFunc(func(writer http.ResponseWriter, request *http.Request) { // if this is 'application/grpc' and HTTP2, handle with gRPC, otherwise HTTP. if request.ProtoMajor == 2 && strings.HasPrefix(request.Header.Get("Content-Type"), "application/grpc") { - grpc.ServeHTTP(writer, request) + srv.ServeHTTP(writer, request) } else { mux.ServeHTTP(writer, request) return diff --git a/flagd/pkg/service/flag-evaluation/flag_evaluator.go b/flagd/pkg/service/flag-evaluation/flag_evaluator.go index 41b83ed42..fd2c95e86 100644 --- a/flagd/pkg/service/flag-evaluation/flag_evaluator.go +++ b/flagd/pkg/service/flag-evaluation/flag_evaluator.go @@ -53,7 +53,7 @@ func NewOldFlagEvaluationService(log *logger.Logger, return svc } -// nolint:dupl +// nolint:dupl,funlen func (s *OldFlagEvaluationService) ResolveAll( ctx context.Context, req *connect.Request[schemaV1.ResolveAllRequest], @@ -119,6 +119,7 @@ func (s *OldFlagEvaluationService) ResolveAll( return connect.NewResponse(res), nil } +// nolint:dupl func (s *OldFlagEvaluationService) EventStream( ctx context.Context, req *connect.Request[schemaV1.EventStreamRequest], diff --git a/flagd/pkg/service/flag-evaluation/flag_evaluator_v2.go b/flagd/pkg/service/flag-evaluation/flag_evaluator_v2.go index 875a506c9..b6eea6324 100644 --- a/flagd/pkg/service/flag-evaluation/flag_evaluator_v2.go +++ b/flagd/pkg/service/flag-evaluation/flag_evaluator_v2.go @@ -116,6 +116,7 @@ func (s *FlagEvaluationService) ResolveAll( return connect.NewResponse(res), nil } +// nolint: dupl func (s *FlagEvaluationService) EventStream( ctx context.Context, req *connect.Request[evalV1.EventStreamRequest], From 77f787cf81d7a1f2d617efd7d50ae0763743b1c4 Mon Sep 17 00:00:00 2001 From: Florian Bacher Date: Wed, 22 May 2024 10:12:19 +0200 Subject: [PATCH 3/5] add simple tests Signed-off-by: Florian Bacher --- core/pkg/telemetry/metrics_test.go | 32 ++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/core/pkg/telemetry/metrics_test.go b/core/pkg/telemetry/metrics_test.go index 36ba79dca..6ebafd5c1 100644 --- a/core/pkg/telemetry/metrics_test.go +++ b/core/pkg/telemetry/metrics_test.go @@ -204,3 +204,35 @@ func TestMetrics(t *testing.T) { }) } } + +// some really simple tests just to make sure all methods are actually implemented and nothing panics +func TestNoopMetricsRecorder_HTTPAttributes(t *testing.T) { + no := NoopMetricsRecorder{} + got := no.HTTPAttributes("", "", "", "") + require.Empty(t, got) +} + +func TestNoopMetricsRecorder_HTTPRequestDuration(t *testing.T) { + no := NoopMetricsRecorder{} + no.HTTPRequestDuration(context.TODO(), 0, nil) +} + +func TestNoopMetricsRecorder_InFlightRequestStart(t *testing.T) { + no := NoopMetricsRecorder{} + no.InFlightRequestStart(context.TODO(), nil) +} + +func TestNoopMetricsRecorder_InFlightRequestEnd(t *testing.T) { + no := NoopMetricsRecorder{} + no.InFlightRequestEnd(context.TODO(), nil) +} + +func TestNoopMetricsRecorder_RecordEvaluation(t *testing.T) { + no := NoopMetricsRecorder{} + no.RecordEvaluation(context.TODO(), nil, "", "", "") +} + +func TestNoopMetricsRecorder_Impressions(t *testing.T) { + no := NoopMetricsRecorder{} + no.Impressions(context.TODO(), "", "", "") +} From aef56724cef24a3aa62cb368b48ef51423a8dbfc Mon Sep 17 00:00:00 2001 From: Florian Bacher Date: Wed, 22 May 2024 10:33:00 +0200 Subject: [PATCH 4/5] fix linting Signed-off-by: Florian Bacher --- core/pkg/telemetry/metrics_test.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/core/pkg/telemetry/metrics_test.go b/core/pkg/telemetry/metrics_test.go index 6ebafd5c1..20d556996 100644 --- a/core/pkg/telemetry/metrics_test.go +++ b/core/pkg/telemetry/metrics_test.go @@ -212,27 +212,27 @@ func TestNoopMetricsRecorder_HTTPAttributes(t *testing.T) { require.Empty(t, got) } -func TestNoopMetricsRecorder_HTTPRequestDuration(t *testing.T) { +func TestNoopMetricsRecorder_HTTPRequestDuration(_ *testing.T) { no := NoopMetricsRecorder{} no.HTTPRequestDuration(context.TODO(), 0, nil) } -func TestNoopMetricsRecorder_InFlightRequestStart(t *testing.T) { +func TestNoopMetricsRecorder_InFlightRequestStart(_ *testing.T) { no := NoopMetricsRecorder{} no.InFlightRequestStart(context.TODO(), nil) } -func TestNoopMetricsRecorder_InFlightRequestEnd(t *testing.T) { +func TestNoopMetricsRecorder_InFlightRequestEnd(_ *testing.T) { no := NoopMetricsRecorder{} no.InFlightRequestEnd(context.TODO(), nil) } -func TestNoopMetricsRecorder_RecordEvaluation(t *testing.T) { +func TestNoopMetricsRecorder_RecordEvaluation(_ *testing.T) { no := NoopMetricsRecorder{} no.RecordEvaluation(context.TODO(), nil, "", "", "") } -func TestNoopMetricsRecorder_Impressions(t *testing.T) { +func TestNoopMetricsRecorder_Impressions(_ *testing.T) { no := NoopMetricsRecorder{} no.Impressions(context.TODO(), "", "", "") } From 155bf59f0cf0ab44f2f5998bc25522e4dbd40f1a Mon Sep 17 00:00:00 2001 From: Florian Bacher Date: Fri, 24 May 2024 08:47:26 +0200 Subject: [PATCH 5/5] use IMetricsRecorder consistently and fall back to Noop Signed-off-by: Florian Bacher --- core/pkg/telemetry/builder.go | 2 +- .../flag-evaluation/connect_service.go | 30 ++++++++++--------- .../service/flag-evaluation/flag_evaluator.go | 7 ++--- .../middleware/metrics/http_metrics.go | 4 +-- 4 files changed, 22 insertions(+), 21 deletions(-) diff --git a/core/pkg/telemetry/builder.go b/core/pkg/telemetry/builder.go index 951cbd41a..2883e180e 100644 --- a/core/pkg/telemetry/builder.go +++ b/core/pkg/telemetry/builder.go @@ -43,7 +43,7 @@ func RegisterErrorHandling(log *logger.Logger) { // BuildMetricsRecorder is a helper to build telemetry.MetricsRecorder based on configurations func BuildMetricsRecorder( ctx context.Context, svcName string, svcVersion string, config Config, -) (*MetricsRecorder, error) { +) (IMetricsRecorder, error) { // Build metric reader based on configurations mReader, err := buildMetricReader(ctx, config) if err != nil { diff --git a/flagd/pkg/service/flag-evaluation/connect_service.go b/flagd/pkg/service/flag-evaluation/connect_service.go index 2785a6fee..799d2546c 100644 --- a/flagd/pkg/service/flag-evaluation/connect_service.go +++ b/flagd/pkg/service/flag-evaluation/connect_service.go @@ -57,7 +57,7 @@ func (b bufSwitchHandler) ServeHTTP(writer http.ResponseWriter, request *http.Re type ConnectService struct { logger *logger.Logger eval evaluator.IEvaluator - metrics *telemetry.MetricsRecorder + metrics telemetry.IMetricsRecorder eventingConfiguration IEvents server *http.Server @@ -71,17 +71,21 @@ type ConnectService struct { // NewConnectService creates a ConnectService with provided parameters func NewConnectService( - logger *logger.Logger, evaluator evaluator.IEvaluator, mRecorder *telemetry.MetricsRecorder, + logger *logger.Logger, evaluator evaluator.IEvaluator, mRecorder telemetry.IMetricsRecorder, ) *ConnectService { - return &ConnectService{ + cs := &ConnectService{ logger: logger, eval: evaluator, - metrics: mRecorder, + metrics: &telemetry.NoopMetricsRecorder{}, eventingConfiguration: &eventingConfiguration{ subs: make(map[interface{}]chan service.Notification), mu: &sync.RWMutex{}, }, } + if mRecorder != nil { + cs.metrics = mRecorder + } + return cs } // Serve serves services with provided configuration options @@ -183,16 +187,14 @@ func (s *ConnectService) setupServer(svcConf service.Configuration) (net.Listene s.serverMtx.Unlock() // Add middlewares - if s.metrics != nil { - metricsMiddleware := metricsmw.NewHTTPMetric(metricsmw.Config{ - Service: svcConf.ServiceName, - MetricRecorder: s.metrics, - Logger: s.logger, - HandlerID: "", - }) - - s.AddMiddleware(metricsMiddleware) - } + metricsMiddleware := metricsmw.NewHTTPMetric(metricsmw.Config{ + Service: svcConf.ServiceName, + MetricRecorder: s.metrics, + Logger: s.logger, + HandlerID: "", + }) + + s.AddMiddleware(metricsMiddleware) corsMiddleware := corsmw.New(svcConf.CORS) s.AddMiddleware(corsMiddleware) diff --git a/flagd/pkg/service/flag-evaluation/flag_evaluator.go b/flagd/pkg/service/flag-evaluation/flag_evaluator.go index fd2c95e86..9e553b53a 100644 --- a/flagd/pkg/service/flag-evaluation/flag_evaluator.go +++ b/flagd/pkg/service/flag-evaluation/flag_evaluator.go @@ -36,7 +36,7 @@ type OldFlagEvaluationService struct { // NewOldFlagEvaluationService creates a OldFlagEvaluationService with provided parameters func NewOldFlagEvaluationService(log *logger.Logger, - eval evaluator.IEvaluator, eventingCfg IEvents, metricsRecorder *telemetry.MetricsRecorder, + eval evaluator.IEvaluator, eventingCfg IEvents, metricsRecorder telemetry.IMetricsRecorder, ) *OldFlagEvaluationService { svc := &OldFlagEvaluationService{ logger: log, @@ -73,9 +73,8 @@ func (s *OldFlagEvaluationService) ResolveAll( span.SetAttributes(attribute.Int("feature_flag.count", len(values))) for _, value := range values { // register the impression and reason for each flag evaluated - if s.metrics != nil { - s.metrics.RecordEvaluation(sCtx, value.Error, value.Reason, value.Variant, value.FlagKey) - } + s.metrics.RecordEvaluation(sCtx, value.Error, value.Reason, value.Variant, value.FlagKey) + switch v := value.Value.(type) { case bool: res.Flags[value.FlagKey] = &schemaV1.AnyFlag{ diff --git a/flagd/pkg/service/middleware/metrics/http_metrics.go b/flagd/pkg/service/middleware/metrics/http_metrics.go index 0a8034852..661b9d965 100644 --- a/flagd/pkg/service/middleware/metrics/http_metrics.go +++ b/flagd/pkg/service/middleware/metrics/http_metrics.go @@ -16,7 +16,7 @@ import ( ) type Config struct { - MetricRecorder *telemetry.MetricsRecorder + MetricRecorder telemetry.IMetricsRecorder Logger *logger.Logger Service string GroupedStatus bool @@ -41,7 +41,7 @@ func (cfg *Config) defaults() { log.Fatal("missing logger") } if cfg.MetricRecorder == nil { - cfg.Logger.Fatal("missing OpenTelemetry metric recorder") + cfg.MetricRecorder = &telemetry.NoopMetricsRecorder{} } }