Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: adapt telemetry setup error handling #1315

Merged
merged 5 commits into from
May 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion core/pkg/telemetry/builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
34 changes: 34 additions & 0 deletions core/pkg/telemetry/metrics.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,40 @@
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) {

Check warning on line 48 in core/pkg/telemetry/metrics.go

View check run for this annotation

Codecov / codecov/patch

core/pkg/telemetry/metrics.go#L48

Added line #L48 was not covered by tests
}

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
Expand Down
32 changes: 32 additions & 0 deletions core/pkg/telemetry/metrics_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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(), "", "", "")
}
9 changes: 6 additions & 3 deletions flagd/pkg/runtime/from_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Kavindu-Dodan marked this conversation as resolved.
Show resolved Hide resolved
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
Expand Down Expand Up @@ -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{
Expand Down
18 changes: 11 additions & 7 deletions flagd/pkg/service/flag-evaluation/connect_service.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@
type ConnectService struct {
logger *logger.Logger
eval evaluator.IEvaluator
metrics *telemetry.MetricsRecorder
metrics telemetry.IMetricsRecorder
eventingConfiguration IEvents

server *http.Server
Expand All @@ -71,17 +71,21 @@

// 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
Expand Down Expand Up @@ -242,8 +246,8 @@
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) {
Expand All @@ -261,7 +265,7 @@
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)

Check warning on line 268 in flagd/pkg/service/flag-evaluation/connect_service.go

View check run for this annotation

Codecov / codecov/patch

flagd/pkg/service/flag-evaluation/connect_service.go#L268

Added line #L268 was not covered by tests
} else {
mux.ServeHTTP(writer, request)
return
Expand Down
24 changes: 17 additions & 7 deletions flagd/pkg/service/flag-evaluation/flag_evaluator.go
Original file line number Diff line number Diff line change
Expand Up @@ -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],
Expand All @@ -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{
Expand Down Expand Up @@ -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],
Expand Down Expand Up @@ -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)
Expand All @@ -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)...)
Expand Down
15 changes: 11 additions & 4 deletions flagd/pkg/service/flag-evaluation/flag_evaluator_v2.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand All @@ -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
Expand Down Expand Up @@ -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],
Expand Down
4 changes: 2 additions & 2 deletions flagd/pkg/service/middleware/metrics/http_metrics.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
)

type Config struct {
MetricRecorder *telemetry.MetricsRecorder
MetricRecorder telemetry.IMetricsRecorder
Logger *logger.Logger
Service string
GroupedStatus bool
Expand All @@ -41,7 +41,7 @@
log.Fatal("missing logger")
}
if cfg.MetricRecorder == nil {
cfg.Logger.Fatal("missing OpenTelemetry metric recorder")
cfg.MetricRecorder = &telemetry.NoopMetricsRecorder{}

Check warning on line 44 in flagd/pkg/service/middleware/metrics/http_metrics.go

View check run for this annotation

Codecov / codecov/patch

flagd/pkg/service/middleware/metrics/http_metrics.go#L44

Added line #L44 was not covered by tests
}
}

Expand Down
Loading