From f522611f21400cd0ad2fc7608ccd2cf2e70fd2a1 Mon Sep 17 00:00:00 2001 From: Victor Lyuboslavsky <2685025+getvictor@users.noreply.github.com> Date: Tue, 16 Sep 2025 11:10:33 -0500 Subject: [PATCH] Added missing OpenTelemetry instrumentation to several API endpoints. (#32960) Fixes #32331 Manually tested all paths. `/test` path removed in https://github.com/fleetdm/fleet/pull/32962 Also added support for sending errors to OpenTelemetry, like we do for APM/Sentry. # Checklist for submitter - [x] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. See [Changes files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/guides/committing-changes.md#changes-files) for more information. ## Testing - [x] QA'd all new/changed functionality manually ## Summary by CodeRabbit * **New Features** * Added OpenTelemetry tracing across core HTTP endpoints (health, version, assets, metrics, enroll/root, debug, Apple MDM, SCEP, SCIM) with dynamic per-request route instrumentation. * Enhanced error reporting to include OpenTelemetry spans/events with contextual user/host attributes. * **Tests** * Added unit tests validating SCIM and error-handling telemetry, span naming, and sensitive-data redaction. --- changes/32331-otel-instrumentation | 1 + cmd/fleet/serve.go | 24 +-- ee/server/scim/scim.go | 100 +++++++++- ee/server/scim/scim_otel_test.go | 211 +++++++++++++++++++++ ee/server/service/hostidentity/scep.go | 10 +- server/contexts/ctxerr/ctxerr.go | 56 +++++- server/contexts/ctxerr/ctxerr_otel_test.go | 129 +++++++++++++ server/service/handler.go | 24 ++- server/service/middleware/otel/otel.go | 45 +++++ server/service/testing_utils.go | 6 +- 10 files changed, 572 insertions(+), 34 deletions(-) create mode 100644 changes/32331-otel-instrumentation create mode 100644 ee/server/scim/scim_otel_test.go create mode 100644 server/contexts/ctxerr/ctxerr_otel_test.go create mode 100644 server/service/middleware/otel/otel.go diff --git a/changes/32331-otel-instrumentation b/changes/32331-otel-instrumentation new file mode 100644 index 0000000000..f4ccbdfdff --- /dev/null +++ b/changes/32331-otel-instrumentation @@ -0,0 +1 @@ +* Added missing OpenTelemetry instrumentation to several API endpoints. diff --git a/cmd/fleet/serve.go b/cmd/fleet/serve.go index a598206df8..b1039813cc 100644 --- a/cmd/fleet/serve.go +++ b/cmd/fleet/serve.go @@ -61,6 +61,7 @@ import ( "github.com/fleetdm/fleet/v4/server/service/async" "github.com/fleetdm/fleet/v4/server/service/conditional_access_microsoft_proxy" "github.com/fleetdm/fleet/v4/server/service/middleware/endpoint_utils" + otelmw "github.com/fleetdm/fleet/v4/server/service/middleware/otel" "github.com/fleetdm/fleet/v4/server/service/redis_key_value" "github.com/fleetdm/fleet/v4/server/service/redis_lock" "github.com/fleetdm/fleet/v4/server/service/redis_policy_set" @@ -1234,9 +1235,9 @@ the way that the Fleet server works. launcher := launcher.New(svc, logger, grpc.NewServer(), healthCheckers) rootMux := http.NewServeMux() - rootMux.Handle("/healthz", service.PrometheusMetricsHandler("healthz", health.Handler(httpLogger, healthCheckers))) - rootMux.Handle("/version", service.PrometheusMetricsHandler("version", version.Handler())) - rootMux.Handle("/assets/", service.PrometheusMetricsHandler("static_assets", service.ServeStaticAssets("/assets/"))) + rootMux.Handle("/healthz", service.PrometheusMetricsHandler("healthz", otelmw.WrapHandler(health.Handler(httpLogger, healthCheckers), "/healthz", config))) + rootMux.Handle("/version", service.PrometheusMetricsHandler("version", otelmw.WrapHandler(version.Handler(), "/version", config))) + rootMux.Handle("/assets/", service.PrometheusMetricsHandler("static_assets", otelmw.WrapHandlerDynamic(service.ServeStaticAssets("/assets/"), config))) if len(config.Server.PrivateKey) > 0 { commander := apple_mdm.NewMDMAppleCommander(mdmStorage, mdmPushService) @@ -1281,6 +1282,7 @@ the way that the Fleet server works. ddmService, commander, appCfg.ServerSettings.ServerURL, + config, ); err != nil { initFatal(err, "setup mdm apple services") } @@ -1288,10 +1290,10 @@ the way that the Fleet server works. if license.IsPremium() { // SCEP proxy (for NDES, etc.) - if err = service.RegisterSCEPProxy(rootMux, ds, logger, nil); err != nil { + if err = service.RegisterSCEPProxy(rootMux, ds, logger, nil, &config); err != nil { initFatal(err, "setup SCEP proxy") } - if err = scim.RegisterSCIM(rootMux, ds, svc, logger); err != nil { + if err = scim.RegisterSCIM(rootMux, ds, svc, logger, &config); err != nil { initFatal(err, "setup SCIM") } // Host identify SCEP feature only works if a private key has been set up @@ -1300,7 +1302,7 @@ the way that the Fleet server works. if err != nil { initFatal(err, "setup host identity SCEP depot") } - if err = hostidentity.RegisterSCEP(rootMux, hostIdentitySCEPDepot, ds, logger); err != nil { + if err = hostidentity.RegisterSCEP(rootMux, hostIdentitySCEPDepot, ds, logger, &config); err != nil { initFatal(err, "setup host identity SCEP") } } else { @@ -1312,12 +1314,12 @@ the way that the Fleet server works. rootMux.Handle("/metrics", basicAuthHandler( config.Prometheus.BasicAuth.Username, config.Prometheus.BasicAuth.Password, - service.PrometheusMetricsHandler("metrics", promhttp.Handler()), + service.PrometheusMetricsHandler("metrics", otelmw.WrapHandler(promhttp.Handler(), "/metrics", config)), )) } else { if config.Prometheus.BasicAuth.Disable { level.Info(logger).Log("msg", "metrics endpoint enabled with http basic auth disabled") - rootMux.Handle("/metrics", service.PrometheusMetricsHandler("metrics", promhttp.Handler())) + rootMux.Handle("/metrics", service.PrometheusMetricsHandler("metrics", otelmw.WrapHandler(promhttp.Handler(), "/metrics", config))) } else { level.Info(logger).Log("msg", "metrics endpoint disabled (http basic auth credentials not set)") } @@ -1435,13 +1437,13 @@ the way that the Fleet server works. rootMux.Handle("/api/v1/fleet/scim/details", apiHandler) rootMux.Handle("/api/latest/fleet/scim/details", apiHandler) - rootMux.Handle("/enroll", endUserEnrollOTAHandler) - rootMux.Handle("/", frontendHandler) + rootMux.Handle("/enroll", otelmw.WrapHandler(endUserEnrollOTAHandler, "/enroll", config)) + rootMux.Handle("/", otelmw.WrapHandler(frontendHandler, "/", config)) debugHandler := &debugMux{ fleetAuthenticatedHandler: service.MakeDebugHandler(svc, config, logger, eh, ds), } - rootMux.Handle("/debug/", debugHandler) + rootMux.Handle("/debug/", otelmw.WrapHandlerDynamic(debugHandler, config)) if debug { // Add debug endpoints with a random diff --git a/ee/server/scim/scim.go b/ee/server/scim/scim.go index 5cb3198b52..e9d0d4fad1 100644 --- a/ee/server/scim/scim.go +++ b/ee/server/scim/scim.go @@ -3,21 +3,24 @@ package scim import ( "bytes" "encoding/json" + "errors" "fmt" "io" "net/http" "strings" "github.com/elimity-com/scim" - "github.com/elimity-com/scim/errors" + scimerrors "github.com/elimity-com/scim/errors" "github.com/elimity-com/scim/optional" "github.com/elimity-com/scim/schema" "github.com/fleetdm/fleet/v4/server/authz" + "github.com/fleetdm/fleet/v4/server/config" "github.com/fleetdm/fleet/v4/server/fleet" "github.com/fleetdm/fleet/v4/server/service/middleware/auth" "github.com/fleetdm/fleet/v4/server/service/middleware/log" kitlog "github.com/go-kit/log" "github.com/go-kit/log/level" + "go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp" ) const ( @@ -29,7 +32,11 @@ func RegisterSCIM( ds fleet.Datastore, svc fleet.Service, logger kitlog.Logger, + fleetConfig *config.FleetConfig, ) error { + if fleetConfig == nil { + return errors.New("fleet config is nil") + } config := scim.ServiceProviderConfig{ DocumentationURI: optional.NewString("https://fleetdm.com/docs/get-started/why-fleet"), MaxResults: maxResults, @@ -209,7 +216,7 @@ func RegisterSCIM( return err } - // TODO: Add APM/OpenTelemetry tracing and Prometheus middleware + // Apply middleware including OTEL instrumentation applyMiddleware := func(prefix string, server http.Handler) http.Handler { handler := http.StripPrefix(prefix, server) handler = AuthorizationMiddleware(authorizer, scimLogger, handler) @@ -222,11 +229,90 @@ func RegisterSCIM( // We cannot use Go URL path pattern like {version} because the http.StripPrefix method // that gets us to the root SCIM path does not support wildcards: https://github.com/golang/go/issues/64909 - mux.Handle("/api/v1/fleet/scim/", applyMiddleware("/api/v1/fleet/scim", server)) - mux.Handle("/api/latest/fleet/scim/", applyMiddleware("/api/latest/fleet/scim", server)) + // Apply OTEL instrumentation at the mux level (outermost) + mux.Handle("/api/v1/fleet/scim/", scimOTELMiddleware(applyMiddleware("/api/v1/fleet/scim", server), "/api/v1/fleet/scim", *fleetConfig)) + mux.Handle("/api/latest/fleet/scim/", scimOTELMiddleware(applyMiddleware("/api/latest/fleet/scim", server), "/api/latest/fleet/scim", *fleetConfig)) return nil } +// scimOTELMiddleware provides OpenTelemetry instrumentation for SCIM endpoints +// It creates proper span names without exposing sensitive IDs +func scimOTELMiddleware(next http.Handler, prefix string, cfg config.FleetConfig) http.Handler { + if !cfg.Logging.TracingEnabled || cfg.Logging.TracingType != "opentelemetry" { + return next + } + + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // Determine the SCIM route pattern based on the path + // OTEL is the outermost middleware, so we see the full path including prefix + fullPath := r.URL.Path + + // Remove the prefix to get the SCIM-specific path + scimPath := strings.TrimPrefix(fullPath, prefix) + // Handle both "/Schemas" and "Schemas" by trimming the leading slash + scimPath = strings.TrimPrefix(scimPath, "/") + + var route string + + // Debug: Log the actual path we're processing + // fmt.Printf("DEBUG: SCIM OTEL fullPath=%q, scimPath=%q\n", fullPath, scimPath) + + // Normalize the path to create a route pattern without exposing IDs + switch { + case strings.HasPrefix(scimPath, "Users"): + segments := strings.Split(scimPath, "/") + if len(segments) == 1 || (len(segments) == 2 && segments[1] == "") { + route = prefix + "/Users" + } else { + // Individual user operations - don't expose the user ID + route = prefix + "/Users/{id}" + } + case strings.HasPrefix(scimPath, "Groups"): + segments := strings.Split(scimPath, "/") + if len(segments) == 1 || (len(segments) == 2 && segments[1] == "") { + route = prefix + "/Groups" + } else { + // Individual group operations - don't expose the group ID + route = prefix + "/Groups/{id}" + } + case strings.HasPrefix(scimPath, "Schemas"): + segments := strings.Split(scimPath, "/") + if len(segments) == 1 || (len(segments) == 2 && segments[1] == "") { + route = prefix + "/Schemas" + } else { + route = prefix + "/Schemas/{id}" + } + case scimPath == "ServiceProviderConfig" || scimPath == "ServiceProviderConfig/": + route = prefix + "/ServiceProviderConfig" + case scimPath == "ResourceTypes" || scimPath == "ResourceTypes/": + route = prefix + "/ResourceTypes" + default: + // For any other path, use the full path but check for potential IDs + // If the path looks like it might contain an ID (has multiple segments), + // we should sanitize it + segments := strings.Split(strings.Trim(scimPath, "/"), "/") + if len(segments) > 1 { + // Might be something like CustomResource/123 + // Replace the last segment with {id} if it looks like an ID + route = prefix + "/" + segments[0] + "/{id}" + } else { + // Single segment path, use as is + route = prefix + "/" + scimPath + } + } + + // Create the instrumented handler with the proper route + instrumentedHandler := otelhttp.NewHandler( + otelhttp.WithRouteTag(route, next), + "", // Empty operation name - will be set by span name formatter + otelhttp.WithSpanNameFormatter(func(operation string, req *http.Request) string { + return req.Method + " " + route + }), + ) + instrumentedHandler.ServeHTTP(w, r) + }) +} + // LastRequestMiddleware saves the details of the last request to SCIM endpoints in the datastore. // These details can be used as a debug tool by the Fleet admin to see if SCIM integration is working. func LastRequestMiddleware(ds fleet.Datastore, logger kitlog.Logger, next http.Handler) http.Handler { @@ -253,13 +339,13 @@ func LastRequestMiddleware(ds fleet.Datastore, logger kitlog.Logger, next http.H case multi.statusCode >= 400: status = "error" // Attempt to parse the response body as a SCIM error. - var parsedScimError errors.ScimError + var parsedScimError scimerrors.ScimError if err := json.Unmarshal(multi.body.Bytes(), &parsedScimError); err == nil { details = parsedScimError.Detail } else { details = multi.body.String() } - if multi.statusCode == errors.ScimErrorInvalidValue.Status && details == errors.ScimErrorInvalidValue.Detail && + if multi.statusCode == scimerrors.ScimErrorInvalidValue.Status && details == scimerrors.ScimErrorInvalidValue.Detail && strings.Contains(r.URL.Path, "/Users") { // We customize the error message here since we can't do it inside the 3rd party SCIM library. details = `Missing required attributes. "userName", "givenName", and "familyName" are required. Please configure your identity provider to send required attributes to Fleet.` @@ -294,7 +380,7 @@ func AuthorizationMiddleware(authorizer *authz.Authorizer, logger kitlog.Logger, } func errorHandler(w http.ResponseWriter, logger kitlog.Logger, detail string, status int) { - scimErr := errors.ScimError{ + scimErr := scimerrors.ScimError{ Status: status, Detail: detail, } diff --git a/ee/server/scim/scim_otel_test.go b/ee/server/scim/scim_otel_test.go new file mode 100644 index 0000000000..a34db18b42 --- /dev/null +++ b/ee/server/scim/scim_otel_test.go @@ -0,0 +1,211 @@ +package scim + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/fleetdm/fleet/v4/server/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.opentelemetry.io/otel/sdk/trace" + "go.opentelemetry.io/otel/sdk/trace/tracetest" +) + +func TestSCIMOTELMiddleware(t *testing.T) { + t.Parallel() + testCases := []struct { + name string + path string + method string + expectedSpan string + }{ + { + name: "Users list", + path: "Users", + method: "GET", + expectedSpan: "GET /api/v1/fleet/scim/Users", + }, + { + name: "Users list with trailing slash", + path: "Users/", + method: "GET", + expectedSpan: "GET /api/v1/fleet/scim/Users", + }, + { + name: "Individual user - hides ID", + path: "Users/12345", + method: "GET", + expectedSpan: "GET /api/v1/fleet/scim/Users/{id}", + }, + { + name: "Update user - hides ID", + path: "Users/67890", + method: "PATCH", + expectedSpan: "PATCH /api/v1/fleet/scim/Users/{id}", + }, + { + name: "Groups list", + path: "Groups", + method: "GET", + expectedSpan: "GET /api/v1/fleet/scim/Groups", + }, + { + name: "Individual group - hides ID", + path: "Groups/abc-def-123", + method: "PUT", + expectedSpan: "PUT /api/v1/fleet/scim/Groups/{id}", + }, + { + name: "Schemas", + path: "Schemas", + method: "GET", + expectedSpan: "GET /api/v1/fleet/scim/Schemas", + }, + { + name: "Individual schema", + path: "Schemas/urn:ietf:params:scim:schemas:core:2.0:User", + method: "GET", + expectedSpan: "GET /api/v1/fleet/scim/Schemas/{id}", + }, + { + name: "Service provider config", + path: "ServiceProviderConfig", + method: "GET", + expectedSpan: "GET /api/v1/fleet/scim/ServiceProviderConfig", + }, + { + name: "Resource types", + path: "ResourceTypes", + method: "GET", + expectedSpan: "GET /api/v1/fleet/scim/ResourceTypes", + }, + { + name: "Unknown path - uses full path", + path: "SomethingElse", + method: "GET", + expectedSpan: "GET /api/v1/fleet/scim/SomethingElse", + }, + { + name: "Bulk operations endpoint", + path: "Bulk", + method: "POST", + expectedSpan: "POST /api/v1/fleet/scim/Bulk", + }, + { + name: "Search endpoint", + path: ".search", + method: "POST", + expectedSpan: "POST /api/v1/fleet/scim/.search", + }, + { + name: "Unknown resource with ID - hides ID", + path: "CustomResource/abc123", + method: "GET", + expectedSpan: "GET /api/v1/fleet/scim/CustomResource/{id}", + }, + { + name: "Unknown nested path with ID - hides ID", + path: "Custom/Resource/123", + method: "DELETE", + expectedSpan: "DELETE /api/v1/fleet/scim/Custom/{id}", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // Create a test span recorder + sr := tracetest.NewSpanRecorder() + tp := trace.NewTracerProvider(trace.WithSpanProcessor(sr)) + + // Create test configuration with OTEL enabled + cfg := config.FleetConfig{ + Logging: config.LoggingConfig{ + TracingEnabled: true, + TracingType: "opentelemetry", + }, + } + + // Create a test handler that just returns 200 OK + testHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + }) + + // Wrap with SCIM OTEL middleware + tracer := tp.Tracer("test") + wrappedHandler := scimOTELMiddleware(testHandler, "/api/v1/fleet/scim", cfg) + + // Create request - now OTEL runs before StripPrefix so it sees the full path + req := httptest.NewRequest(tc.method, "/api/v1/fleet/scim/"+tc.path, nil) + + // Add span to context + ctx, span := tracer.Start(req.Context(), "test-parent-span") + defer span.End() + req = req.WithContext(ctx) + + // Execute request + w := httptest.NewRecorder() + wrappedHandler.ServeHTTP(w, req) + + // Force span to end + span.End() + + // Check spans + spans := sr.Ended() + require.GreaterOrEqual(t, len(spans), 2, "Should have at least parent and child spans") + + // Find the SCIM span (should be the second one, after the parent) + var scimSpan trace.ReadOnlySpan + for _, s := range spans { + if s.Name() == tc.expectedSpan { + scimSpan = s + break + } + } + + require.NotNil(t, scimSpan, "Should find SCIM span with name: %s", tc.expectedSpan) + assert.Equal(t, tc.expectedSpan, scimSpan.Name()) + + // Check that the route tag is set correctly (without exposing IDs) + attrs := scimSpan.Attributes() + for _, attr := range attrs { + if string(attr.Key) == "http.route" { + // The route should match the pattern, not the actual path + assert.NotContains(t, attr.Value.AsString(), "123", "Should not expose user ID") + assert.NotContains(t, attr.Value.AsString(), "67890", "Should not expose user ID") + } + } + }) + } +} + +func TestSCIMOTELMiddleware_Disabled(t *testing.T) { + t.Parallel() + // Create test configuration with OTEL disabled + cfg := config.FleetConfig{ + Logging: config.LoggingConfig{ + TracingEnabled: false, + }, + } + + // Create a test handler + called := false + testHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + called = true + w.WriteHeader(http.StatusOK) + }) + + // Wrap with SCIM OTEL middleware + wrappedHandler := scimOTELMiddleware(testHandler, "/api/v1/fleet/scim", cfg) + + // Create request - OTEL sees the full path now + req := httptest.NewRequest("GET", "/api/v1/fleet/scim/Users", nil) + w := httptest.NewRecorder() + + // Execute request + wrappedHandler.ServeHTTP(w, req) + + // Should have called the handler without any OTEL instrumentation + assert.True(t, called, "Handler should have been called") + assert.Equal(t, http.StatusOK, w.Code) +} diff --git a/ee/server/service/hostidentity/scep.go b/ee/server/service/hostidentity/scep.go index ac9419e9c1..dc28672893 100644 --- a/ee/server/service/hostidentity/scep.go +++ b/ee/server/service/hostidentity/scep.go @@ -19,11 +19,13 @@ import ( "github.com/cenkalti/backoff/v4" "github.com/fleetdm/fleet/v4/ee/server/service/hostidentity/types" + "github.com/fleetdm/fleet/v4/server/config" "github.com/fleetdm/fleet/v4/server/contexts/ctxerr" "github.com/fleetdm/fleet/v4/server/fleet" "github.com/fleetdm/fleet/v4/server/mdm/assets" scepdepot "github.com/fleetdm/fleet/v4/server/mdm/scep/depot" scepserver "github.com/fleetdm/fleet/v4/server/mdm/scep/server" + "github.com/fleetdm/fleet/v4/server/service/middleware/otel" "github.com/go-kit/kit/log" kitlog "github.com/go-kit/log" "github.com/go-kit/log/level" @@ -66,7 +68,11 @@ func RegisterSCEP( scepStorage scepdepot.Depot, ds fleet.Datastore, logger kitlog.Logger, + fleetConfig *config.FleetConfig, ) error { + if fleetConfig == nil { + return errors.New("fleet config is nil") + } err := initAssets(ds) if err != nil { return fmt.Errorf("initializing host identity assets: %w", err) @@ -89,12 +95,12 @@ func RegisterSCEP( e.GetEndpoint = scepserver.EndpointLoggingMiddleware(scepLogger)(e.GetEndpoint) e.PostEndpoint = scepserver.EndpointLoggingMiddleware(scepLogger)(e.PostEndpoint) - // Note: Monitoring (APM/OpenTel) is missing for this SCEP server. - // In addition, the scepserver error handler does not send errors to APM/Sentry/Redis. + // The scepserver error handler does not send errors to APM/Sentry/Redis. // It should be enhanced to do so if/when we start monitoring error traces. // This note also applies to the other SCEP servers we use. // That is why we're not using ctxerr wrappers here. scepHandler := scepserver.MakeHTTPHandler(e, scepService, scepLogger) + scepHandler = otel.WrapHandler(scepHandler, scepPath, *fleetConfig) mux.Handle(scepPath, scepHandler) return nil } diff --git a/server/contexts/ctxerr/ctxerr.go b/server/contexts/ctxerr/ctxerr.go index a7ba824796..d4587304f6 100644 --- a/server/contexts/ctxerr/ctxerr.go +++ b/server/contexts/ctxerr/ctxerr.go @@ -25,6 +25,9 @@ import ( "github.com/fleetdm/fleet/v4/server/fleet" "github.com/getsentry/sentry-go" "go.elastic.co/apm/v2" + "go.opentelemetry.io/otel/attribute" + "go.opentelemetry.io/otel/codes" + "go.opentelemetry.io/otel/trace" ) type key int @@ -291,20 +294,61 @@ func FromContext(ctx context.Context) Handler { // Handle handles err by passing it to the registered error handler, // deduplicating it and storing it for a configured duration. It also takes -// care of sending it to the configured APM, if any. +// care of sending it to the configured OpenTelemetry/APM/Sentry, if any. func Handle(ctx context.Context, err error) { + if err == nil { + return + } + // as a last resource, wrap the error if there isn't // a FleetError in the chain var ferr *FleetError if !errors.As(err, &ferr) { - err = Wrap(ctx, err, "missing FleetError in chain") + wrapped := wrapError(ctx, "missing FleetError in chain", err, nil) + if wrapped == nil { + return // shouldn't happen since err is not nil, but be safe + } + // wrapError returns an error interface, but we know it's a *FleetError + ferr = wrapped.(*FleetError) } - cause := err - if ferr := FleetCause(err); ferr != nil { + cause := ferr + if rootCause := FleetCause(ferr); rootCause != nil { // use the FleetCause error so we send the most relevant stacktrace to APM // (the one from the initial New/Wrap call). - cause = ferr + cause = rootCause + } + + // send to OpenTelemetry if there's an active span + if span := trace.SpanFromContext(ctx); span != nil && span.IsRecording() { + // Mark the current span as failed by setting the error status. + // This status can be overridden if we recovered from the error. + span.SetStatus(codes.Error, cause.Error()) + + // Build attributes for the exception event + attrs := []attribute.KeyValue{ + attribute.String("exception.type", fmt.Sprintf("%T", cause)), + attribute.String("exception.message", cause.Error()), + attribute.String("exception.stacktrace", strings.Join(cause.Stack(), "\n")), + } + + // Add contextual information if available (same as Sentry) + v, _ := viewer.FromContext(ctx) + h, _ := host.FromContext(ctx) + + if v.User != nil { + attrs = append(attrs, + // Not sending the email here as it may contain sensitive information (PII). + attribute.Int64("user.id", int64(v.User.ID)), //nolint:gosec + ) + } else if h != nil { + attrs = append(attrs, + attribute.String("host.hostname", h.Hostname), + attribute.Int64("host.id", int64(h.ID)), //nolint:gosec + ) + } + + span.AddEvent("exception", trace.WithAttributes(attrs...)) } // send to elastic APM @@ -338,7 +382,7 @@ func Handle(ctx context.Context, err error) { } if eh := FromContext(ctx); eh != nil { - eh.Store(err) + eh.Store(ferr) } } diff --git a/server/contexts/ctxerr/ctxerr_otel_test.go b/server/contexts/ctxerr/ctxerr_otel_test.go new file mode 100644 index 0000000000..85d7c803c6 --- /dev/null +++ b/server/contexts/ctxerr/ctxerr_otel_test.go @@ -0,0 +1,129 @@ +package ctxerr + +import ( + "context" + "strings" + "testing" + + "github.com/fleetdm/fleet/v4/server/contexts/host" + "github.com/fleetdm/fleet/v4/server/contexts/viewer" + "github.com/fleetdm/fleet/v4/server/fleet" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.opentelemetry.io/otel/sdk/trace" + "go.opentelemetry.io/otel/sdk/trace/tracetest" +) + +func TestHandleSendsContextToOTEL(t *testing.T) { + t.Parallel() + testCases := []struct { + name string + setupContext func(context.Context) context.Context + errorMessage string + expectedAttrs map[string]any // expected attributes in the exception event + }{ + { + name: "with user context", + setupContext: func(ctx context.Context) context.Context { + testUser := &fleet.User{ + ID: 123, + Email: "test@example.com", + } + return viewer.NewContext(ctx, viewer.Viewer{User: testUser}) + }, + errorMessage: "test error with user context", + expectedAttrs: map[string]any{ + "user.id": int64(123), + }, + }, + { + name: "with host context", + setupContext: func(ctx context.Context) context.Context { + testHost := &fleet.Host{ + ID: 456, + Hostname: "test-host.example.com", + } + return host.NewContext(ctx, testHost) + }, + errorMessage: "test error with host context", + expectedAttrs: map[string]any{ + "host.hostname": "test-host.example.com", + "host.id": int64(456), + }, + }, + { + name: "without additional context", + setupContext: func(ctx context.Context) context.Context { + return ctx // no additional context + }, + errorMessage: "test error without context", + expectedAttrs: map[string]any{}, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // Create a test span recorder and tracer provider + sr := tracetest.NewSpanRecorder() + tp := trace.NewTracerProvider(trace.WithSpanProcessor(sr)) + + // Create a context with an active span using the test tracer + tracer := tp.Tracer("test") + ctx, span := tracer.Start(t.Context(), "test-span") + defer span.End() + + // Setup context with test-specific data + ctx = tc.setupContext(ctx) + + // Create and handle an error + err := New(ctx, tc.errorMessage) + Handle(ctx, err) + + // Force span to end so we can check recorded data + span.End() + + // Check that the exception event was created + spans := sr.Ended() + require.Len(t, spans, 1) + + // Find the exception event + events := spans[0].Events() + var exceptionEvent *trace.Event + for i := range events { + if events[i].Name == "exception" { + exceptionEvent = &events[i] + break + } + } + require.NotNil(t, exceptionEvent, "Expected to find an exception event") + + // Check all expected attributes are present + attributes := make(map[string]any) + for _, attr := range exceptionEvent.Attributes { + switch attr.Key { + case "user.id", "host.id": + attributes[string(attr.Key)] = attr.Value.AsInt64() + default: + attributes[string(attr.Key)] = attr.Value.AsString() + } + } + + // Always check for stack trace + stackTrace, ok := attributes["exception.stacktrace"].(string) + assert.True(t, ok, "Expected exception.stacktrace attribute") + assert.Contains(t, stackTrace, "TestHandleSendsContextToOTEL", "Stack trace should contain test function name") + assert.True(t, strings.Contains(stackTrace, "\n"), "Stack trace should be formatted with newlines") + + // Check for exception message and type + assert.Equal(t, tc.errorMessage, attributes["exception.message"]) + assert.Equal(t, "*ctxerr.FleetError", attributes["exception.type"]) + + // Check test-specific expected attributes + for expectedKey, expectedValue := range tc.expectedAttrs { + actualValue, found := attributes[expectedKey] + assert.True(t, found, "Expected to find attribute %s", expectedKey) + assert.Equal(t, expectedValue, actualValue, "Attribute %s should match", expectedKey) + } + }) + } +} diff --git a/server/service/handler.go b/server/service/handler.go index adf6de7925..669e0ff315 100644 --- a/server/service/handler.go +++ b/server/service/handler.go @@ -3,6 +3,7 @@ package service import ( "context" "encoding/json" + "errors" "fmt" "net/http" "os" @@ -29,6 +30,7 @@ import ( "github.com/fleetdm/fleet/v4/server/service/middleware/endpoint_utils" "github.com/fleetdm/fleet/v4/server/service/middleware/log" "github.com/fleetdm/fleet/v4/server/service/middleware/mdmconfigured" + "github.com/fleetdm/fleet/v4/server/service/middleware/otel" "github.com/fleetdm/fleet/v4/server/service/middleware/ratelimit" kithttp "github.com/go-kit/kit/transport/http" kitlog "github.com/go-kit/log" @@ -1174,14 +1176,15 @@ func RegisterAppleMDMProtocolServices( ddmService nanomdm_service.DeclarativeManagement, profileService nanomdm_service.ProfileService, serverURLPrefix string, + fleetConfig config.FleetConfig, ) error { - if err := registerSCEP(mux, scepConfig, scepStorage, mdmStorage, logger); err != nil { + if err := registerSCEP(mux, scepConfig, scepStorage, mdmStorage, logger, fleetConfig); err != nil { return fmt.Errorf("scep: %w", err) } - if err := registerMDM(mux, mdmStorage, checkinAndCommandService, ddmService, profileService, logger); err != nil { + if err := registerMDM(mux, mdmStorage, checkinAndCommandService, ddmService, profileService, logger, fleetConfig); err != nil { return fmt.Errorf("mdm: %w", err) } - if err := registerMDMServiceDiscovery(mux, logger, serverURLPrefix); err != nil { + if err := registerMDMServiceDiscovery(mux, logger, serverURLPrefix, fleetConfig); err != nil { return fmt.Errorf("service discovery: %w", err) } return nil @@ -1191,6 +1194,7 @@ func registerMDMServiceDiscovery( mux *http.ServeMux, logger kitlog.Logger, serverURLPrefix string, + fleetConfig config.FleetConfig, ) error { serviceDiscoveryLogger := kitlog.With(logger, "component", "mdm-apple-service-discovery") fullMDMEnrollmentURL := fmt.Sprintf("%s%s", serverURLPrefix, apple_mdm.AccountDrivenEnrollPath) @@ -1204,7 +1208,7 @@ func registerMDMServiceDiscovery( http.Error(w, "Internal Server Error", http.StatusInternalServerError) } }) - mux.Handle(apple_mdm.ServiceDiscoveryPath, serviceDiscoveryHandler) + mux.Handle(apple_mdm.ServiceDiscoveryPath, otel.WrapHandler(serviceDiscoveryHandler, apple_mdm.ServiceDiscoveryPath, fleetConfig)) return nil } @@ -1216,6 +1220,7 @@ func registerSCEP( scepStorage scep_depot.Depot, mdmStorage fleet.MDMAppleStore, logger kitlog.Logger, + fleetConfig config.FleetConfig, ) error { var signer scepserver.CSRSignerContext = scepserver.SignCSRAdapter(scep_depot.NewSigner( scepStorage, @@ -1240,7 +1245,7 @@ func registerSCEP( e.GetEndpoint = scepserver.EndpointLoggingMiddleware(scepLogger)(e.GetEndpoint) e.PostEndpoint = scepserver.EndpointLoggingMiddleware(scepLogger)(e.PostEndpoint) scepHandler := scepserver.MakeHTTPHandler(e, scepService, scepLogger) - mux.Handle(apple_mdm.SCEPPath, scepHandler) + mux.Handle(apple_mdm.SCEPPath, otel.WrapHandler(scepHandler, apple_mdm.SCEPPath, fleetConfig)) return nil } @@ -1249,7 +1254,11 @@ func RegisterSCEPProxy( ds fleet.Datastore, logger kitlog.Logger, timeout *time.Duration, + fleetConfig *config.FleetConfig, ) error { + if fleetConfig == nil { + return errors.New("fleet config is nil") + } scepService := eeservice.NewSCEPProxyService( ds, kitlog.With(logger, "component", "scep-proxy-service"), @@ -1260,6 +1269,8 @@ func RegisterSCEPProxy( e.GetEndpoint = scepserver.EndpointLoggingMiddleware(scepLogger)(e.GetEndpoint) e.PostEndpoint = scepserver.EndpointLoggingMiddleware(scepLogger)(e.PostEndpoint) scepHandler := scepserver.MakeHTTPHandlerWithIdentifier(e, apple_mdm.SCEPProxyPath, scepLogger) + // Not using OTEL dynamic wrapper so as not to expose {identifier} in the span name + scepHandler = otel.WrapHandler(scepHandler, apple_mdm.SCEPProxyPath, *fleetConfig) rootMux.Handle(apple_mdm.SCEPProxyPath, scepHandler) return nil } @@ -1298,6 +1309,7 @@ func registerMDM( ddmService nanomdm_service.DeclarativeManagement, profileService nanomdm_service.ProfileService, logger kitlog.Logger, + fleetConfig config.FleetConfig, ) error { certVerifier := mdmcrypto.NewSCEPVerifier(mdmStorage) mdmLogger := NewNanoMDMLogger(kitlog.With(logger, "component", "http-mdm-apple-mdm")) @@ -1329,7 +1341,7 @@ func registerMDM( } mdmHandler = httpmdm.CertExtractMdmSignatureMiddleware(mdmHandler, httpmdm.MdmSignatureVerifierFunc(cryptoutil.VerifyMdmSignature), httpmdm.SigLogWithLogger(mdmLogger.With("handler", "cert-extract"))) - mux.Handle(apple_mdm.MDMPath, mdmHandler) + mux.Handle(apple_mdm.MDMPath, otel.WrapHandler(mdmHandler, apple_mdm.MDMPath, fleetConfig)) return nil } diff --git a/server/service/middleware/otel/otel.go b/server/service/middleware/otel/otel.go new file mode 100644 index 0000000000..a546119b92 --- /dev/null +++ b/server/service/middleware/otel/otel.go @@ -0,0 +1,45 @@ +package otel + +import ( + "net/http" + + "github.com/fleetdm/fleet/v4/server/config" + "go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp" +) + +// WrapHandler wraps an HTTP handler with OpenTelemetry instrumentation for a fixed route. +// It creates spans named as "{method} {route}" (e.g., "GET /healthz"). +func WrapHandler(handler http.Handler, route string, config config.FleetConfig) http.Handler { + if config.Logging.TracingEnabled && config.Logging.TracingType == "opentelemetry" { + // Wrap with OTEL handler to create properly named spans: "{method} {route}" + return otelhttp.NewHandler( + otelhttp.WithRouteTag(route, handler), + "", // Empty operation name - will be set by span name formatter + otelhttp.WithSpanNameFormatter(func(operation string, r *http.Request) string { + return r.Method + " " + route + }), + ) + } + return handler +} + +// WrapHandlerDynamic wraps an HTTP handler with OpenTelemetry instrumentation using dynamic routes. +// It creates spans based on the actual request path (e.g., "GET /assets/app.js"). +func WrapHandlerDynamic(handler http.Handler, config config.FleetConfig) http.Handler { + if config.Logging.TracingEnabled && config.Logging.TracingType == "opentelemetry" { + // Create a wrapper that instruments each request with its actual path + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // Use the actual request path as the route + route := r.URL.Path + instrumentedHandler := otelhttp.NewHandler( + otelhttp.WithRouteTag(route, handler), + "", // Empty operation name - will be set by span name formatter + otelhttp.WithSpanNameFormatter(func(operation string, req *http.Request) string { + return req.Method + " " + route + }), + ) + instrumentedHandler.ServeHTTP(w, r) + }) + } + return handler +} diff --git a/server/service/testing_utils.go b/server/service/testing_utils.go index cf79b912f0..e8efc9eb4b 100644 --- a/server/service/testing_utils.go +++ b/server/service/testing_utils.go @@ -441,6 +441,7 @@ func RunServerForTestsWithServiceWithDS(t *testing.T, ctx context.Context, ds fl }, commander, "https://test-url.com", + cfg, ) require.NoError(t, err) } @@ -458,6 +459,7 @@ func RunServerForTestsWithServiceWithDS(t *testing.T, ctx context.Context, ds fl ds, logger, timeout, + &cfg, ) require.NoError(t, err) } @@ -479,7 +481,7 @@ func RunServerForTestsWithServiceWithDS(t *testing.T, ctx context.Context, ds fl extra = append(extra, WithLoginRateLimit(throttled.PerMin(1000))) if len(opts) > 0 && opts[0].HostIdentity != nil { - require.NoError(t, hostidentity.RegisterSCEP(rootMux, opts[0].HostIdentity.SCEPStorage, ds, logger)) + require.NoError(t, hostidentity.RegisterSCEP(rootMux, opts[0].HostIdentity.SCEPStorage, ds, logger, &cfg)) var httpSigVerifier func(http.Handler) http.Handler httpSigVerifier, err := httpsig.Middleware(ds, opts[0].HostIdentity.RequireHTTPMessageSignature, kitlog.With(logger, "component", "http-sig-verifier")) require.NoError(t, err) @@ -497,7 +499,7 @@ func RunServerForTestsWithServiceWithDS(t *testing.T, ctx context.Context, ds fl rootMux.Handle("/enroll", ServeEndUserEnrollOTA(svc, "", ds, logger)) if len(opts) > 0 && opts[0].EnableSCIM { - require.NoError(t, scim.RegisterSCIM(rootMux, ds, svc, logger)) + require.NoError(t, scim.RegisterSCIM(rootMux, ds, svc, logger, &cfg)) rootMux.Handle("/api/v1/fleet/scim/details", apiHandler) rootMux.Handle("/api/latest/fleet/scim/details", apiHandler) }