From 505ed389b70be83d959e90b53e3ff2dcf94f27d0 Mon Sep 17 00:00:00 2001 From: Christian Richter Date: Tue, 16 Jul 2024 14:46:49 +0200 Subject: [PATCH] Fix nil-pointer issue with misconfigured tracing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Jörn Friedrich Dreyer Signed-off-by: Christian Richter --- ocis-pkg/tracing/tracing.go | 2 ++ services/ocs/pkg/command/server.go | 12 +++++------ services/ocs/pkg/config/tracing.go | 12 +++++++++++ services/ocs/pkg/middleware/logtrace.go | 28 ------------------------- services/ocs/pkg/server/http/option.go | 21 +++++++++++++------ services/ocs/pkg/server/http/server.go | 13 ++++-------- services/ocs/pkg/tracing/tracing.go | 23 -------------------- 7 files changed, 39 insertions(+), 72 deletions(-) delete mode 100644 services/ocs/pkg/middleware/logtrace.go delete mode 100644 services/ocs/pkg/tracing/tracing.go diff --git a/ocis-pkg/tracing/tracing.go b/ocis-pkg/tracing/tracing.go index fb86e48ea..30718d086 100644 --- a/ocis-pkg/tracing/tracing.go +++ b/ocis-pkg/tracing/tracing.go @@ -86,6 +86,8 @@ func GetTraceProvider(endpoint, collector, serviceName, traceType string) (*sdkt jaeger.WithEndpoint(collector), ), ) + } else { + return sdktrace.NewTracerProvider(sdktrace.WithSampler(sdktrace.NeverSample())), nil } if err != nil { return nil, err diff --git a/services/ocs/pkg/command/server.go b/services/ocs/pkg/command/server.go index c252872ec..cdb231163 100644 --- a/services/ocs/pkg/command/server.go +++ b/services/ocs/pkg/command/server.go @@ -5,15 +5,14 @@ import ( "fmt" "os" + "github.com/oklog/run" "github.com/owncloud/ocis/v2/ocis-pkg/config/configlog" + ogrpc "github.com/owncloud/ocis/v2/ocis-pkg/service/grpc" + "github.com/owncloud/ocis/v2/ocis-pkg/tracing" "github.com/owncloud/ocis/v2/ocis-pkg/version" + "github.com/owncloud/ocis/v2/services/ocs/pkg/config" "github.com/owncloud/ocis/v2/services/ocs/pkg/config/parser" "github.com/owncloud/ocis/v2/services/ocs/pkg/logging" - "github.com/owncloud/ocis/v2/services/ocs/pkg/tracing" - - "github.com/oklog/run" - ogrpc "github.com/owncloud/ocis/v2/ocis-pkg/service/grpc" - "github.com/owncloud/ocis/v2/services/ocs/pkg/config" "github.com/owncloud/ocis/v2/services/ocs/pkg/metrics" "github.com/owncloud/ocis/v2/services/ocs/pkg/server/debug" "github.com/owncloud/ocis/v2/services/ocs/pkg/server/http" @@ -31,7 +30,7 @@ func Server(cfg *config.Config) *cli.Command { }, Action: func(c *cli.Context) error { logger := logging.Configure(cfg.Service.Name, cfg.Log) - err := tracing.Configure(cfg) + traceProvider, err := tracing.GetServiceTraceProvider(cfg.Tracing, cfg.Service.Name) if err != nil { return err } @@ -62,6 +61,7 @@ func Server(cfg *config.Config) *cli.Command { http.Context(ctx), http.Config(cfg), http.Metrics(metrics), + http.TraceProvider(traceProvider), ) if err != nil { diff --git a/services/ocs/pkg/config/tracing.go b/services/ocs/pkg/config/tracing.go index 5fed7129f..f6f23b2ba 100644 --- a/services/ocs/pkg/config/tracing.go +++ b/services/ocs/pkg/config/tracing.go @@ -1,5 +1,7 @@ package config +import "github.com/owncloud/ocis/v2/ocis-pkg/tracing" + // Tracing defines the available tracing configuration. type Tracing struct { Enabled bool `yaml:"enabled" env:"OCIS_TRACING_ENABLED;OCS_TRACING_ENABLED" desc:"Activates tracing." introductionVersion:"pre5.0"` @@ -7,3 +9,13 @@ type Tracing struct { Endpoint string `yaml:"endpoint" env:"OCIS_TRACING_ENDPOINT;OCS_TRACING_ENDPOINT" desc:"The endpoint of the tracing agent." introductionVersion:"pre5.0"` Collector string `yaml:"collector" env:"OCIS_TRACING_COLLECTOR;OCS_TRACING_COLLECTOR" desc:"The HTTP endpoint for sending spans directly to a collector, i.e. http://jaeger-collector:14268/api/traces. Only used if the tracing endpoint is unset." introductionVersion:"pre5.0"` } + +// Convert Tracing to the tracing package's Config struct. +func (t Tracing) Convert() tracing.Config { + return tracing.Config{ + Enabled: t.Enabled, + Type: t.Type, + Endpoint: t.Endpoint, + Collector: t.Collector, + } +} diff --git a/services/ocs/pkg/middleware/logtrace.go b/services/ocs/pkg/middleware/logtrace.go deleted file mode 100644 index 202de69e0..000000000 --- a/services/ocs/pkg/middleware/logtrace.go +++ /dev/null @@ -1,28 +0,0 @@ -package middleware - -import ( - "net/http" - - ocstracing "github.com/owncloud/ocis/v2/services/ocs/pkg/tracing" - "go.opentelemetry.io/otel/propagation" - "go.opentelemetry.io/otel/trace" -) - -var propagator = propagation.NewCompositeTextMapPropagator( - propagation.Baggage{}, - propagation.TraceContext{}, -) - -// LogTrace Sets the initial trace in the ocs service. -func LogTrace(next http.Handler) http.Handler { - return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - spanOpts := []trace.SpanStartOption{ - trace.WithSpanKind(trace.SpanKindServer), - } - ctx, span := ocstracing.TraceProvider.Tracer("ocs").Start(r.Context(), r.URL.Path, spanOpts...) - defer span.End() - - propagator.Inject(ctx, propagation.HeaderCarrier(r.Header)) - next.ServeHTTP(w, r.WithContext(ctx)) - }) -} diff --git a/services/ocs/pkg/server/http/option.go b/services/ocs/pkg/server/http/option.go index dd10e91e2..d259c1be7 100644 --- a/services/ocs/pkg/server/http/option.go +++ b/services/ocs/pkg/server/http/option.go @@ -7,6 +7,7 @@ import ( "github.com/owncloud/ocis/v2/services/ocs/pkg/config" "github.com/owncloud/ocis/v2/services/ocs/pkg/metrics" "github.com/urfave/cli/v2" + "go.opentelemetry.io/otel/trace" ) // Option defines a single option function. @@ -14,12 +15,13 @@ type Option func(o *Options) // Options defines the available options for this package. type Options struct { - Namespace string - Logger log.Logger - Context context.Context - Config *config.Config - Metrics *metrics.Metrics - Flags []cli.Flag + Namespace string + Logger log.Logger + Context context.Context + Config *config.Config + Metrics *metrics.Metrics + Flags []cli.Flag + TraceProvider trace.TracerProvider } // newOptions initializes the available default options. @@ -74,3 +76,10 @@ func Namespace(val string) Option { o.Namespace = val } } + +// TraceProvider provides a function to set the TraceProvider option. +func TraceProvider(tp trace.TracerProvider) Option { + return func(o *Options) { + o.TraceProvider = tp + } +} diff --git a/services/ocs/pkg/server/http/server.go b/services/ocs/pkg/server/http/server.go index fe4e1da59..d8efb98a8 100644 --- a/services/ocs/pkg/server/http/server.go +++ b/services/ocs/pkg/server/http/server.go @@ -2,18 +2,17 @@ package http import ( "fmt" - "github.com/cs3org/reva/v2/pkg/store" chimiddleware "github.com/go-chi/chi/v5/middleware" "github.com/owncloud/ocis/v2/ocis-pkg/cors" "github.com/owncloud/ocis/v2/ocis-pkg/middleware" "github.com/owncloud/ocis/v2/ocis-pkg/service/http" "github.com/owncloud/ocis/v2/ocis-pkg/version" - ocsmw "github.com/owncloud/ocis/v2/services/ocs/pkg/middleware" svc "github.com/owncloud/ocis/v2/services/ocs/pkg/service/v0" ocisstore "github.com/owncloud/ocis/v2/services/store/pkg/store" "go-micro.dev/v4" microstore "go-micro.dev/v4/store" + "go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp" ) // Server initializes the http service and server. @@ -29,6 +28,7 @@ func Server(opts ...Option) (http.Service, error) { http.Address(options.Config.HTTP.Addr), http.Context(options.Context), http.Flags(options.Flags...), + http.TraceProvider(options.TraceProvider), ) if err != nil { options.Logger.Error(). @@ -74,17 +74,12 @@ func Server(opts ...Option) (http.Service, error) { version.GetString(), ), middleware.Logger(options.Logger), - ocsmw.LogTrace, + middleware.TraceContext, + otelhttp.NewMiddleware(options.Config.Service.Name, otelhttp.WithTracerProvider(options.TraceProvider)), ), svc.Store(signingKeyStore), ) - { - handle = svc.NewInstrument(handle, options.Metrics) - handle = svc.NewLogging(handle, options.Logger) - handle = svc.NewTracing(handle) - } - if err := micro.RegisterHandler(service.Server(), handle); err != nil { return http.Service{}, err } diff --git a/services/ocs/pkg/tracing/tracing.go b/services/ocs/pkg/tracing/tracing.go deleted file mode 100644 index 6119fac10..000000000 --- a/services/ocs/pkg/tracing/tracing.go +++ /dev/null @@ -1,23 +0,0 @@ -package tracing - -import ( - pkgtrace "github.com/owncloud/ocis/v2/ocis-pkg/tracing" - "github.com/owncloud/ocis/v2/services/ocs/pkg/config" - "go.opentelemetry.io/otel/trace" -) - -var ( - // TraceProvider is the global trace provider for the ocs service. - TraceProvider = trace.NewNoopTracerProvider() -) - -func Configure(cfg *config.Config) error { - var err error - if cfg.Tracing.Enabled { - if TraceProvider, err = pkgtrace.GetTraceProvider(cfg.Tracing.Endpoint, cfg.Tracing.Collector, cfg.Service.Name, cfg.Tracing.Type); err != nil { - return err - } - } - - return nil -}