chore: adapt telemetry setup error handling (#1315)

Closes #1194 This PR adapts the service setup to not fail when encountering an error in the telemetry setup. --------- Signed-off-by: Florian Bacher <[email protected]>
open-feature · May 27, 2024 · 20bcb78 · 20bcb78
1 parent 9b8fa74
commit 20bcb78
Show file tree

Hide file tree

Showing 8 changed files with 114 additions and 24 deletions.
diff --git 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/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/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(_ *testing.T) {
+	no := NoopMetricsRecorder{}
+	no.HTTPRequestDuration(context.TODO(), 0, nil)
+}
+
+func TestNoopMetricsRecorder_InFlightRequestStart(_ *testing.T) {
+	no := NoopMetricsRecorder{}
+	no.InFlightRequestStart(context.TODO(), nil)
+}
+
+func TestNoopMetricsRecorder_InFlightRequestEnd(_ *testing.T) {
+	no := NoopMetricsRecorder{}
+	no.InFlightRequestEnd(context.TODO(), nil)
+}
+
+func TestNoopMetricsRecorder_RecordEvaluation(_ *testing.T) {
+	no := NoopMetricsRecorder{}
+	no.RecordEvaluation(context.TODO(), nil, "", "", "")
+}
+
+func TestNoopMetricsRecorder_Impressions(_ *testing.T) {
+	no := NoopMetricsRecorder{}
+	no.Impressions(context.TODO(), "", "", "")
+}
diff --git 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
@@ -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
@@ -242,8 +246,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) {
@@ -261,7 +265,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
@@ -29,25 +29,31 @@ 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
 }
 
 // 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 {
-	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
+// nolint:dupl,funlen
 func (s *OldFlagEvaluationService) ResolveAll(
 	ctx context.Context,
 	req *connect.Request[schemaV1.ResolveAllRequest],
@@ -68,6 +74,7 @@ func (s *OldFlagEvaluationService) ResolveAll(
 	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)
+
 		switch v := value.Value.(type) {
 		case bool:
 			res.Flags[value.FlagKey] = &schemaV1.AnyFlag{
@@ -111,6 +118,7 @@ func (s *OldFlagEvaluationService) ResolveAll(
 	return connect.NewResponse(res), nil
 }
 
+// nolint:dupl
 func (s *OldFlagEvaluationService) EventStream(
 	ctx context.Context,
 	req *connect.Request[schemaV1.EventStreamRequest],
@@ -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
@@ -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
@@ -110,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],

diff --git 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{}
 	}
 }