FrontendService: Add tracing and logging middleware (#107956)

* FrontendService: Add tracing and logging middleware

* tests!

* middleware tests

* context middleware test

* revert http_server back to previous version

* fix lint

* fix test

* use http.NotFound instead of custom http handler

* use existing tracer for package

* use otel/trace.Tracer in request_tracing middleware

* tidy up tracing in contextMiddleware

* fix 404 test

* remove spans from contextMiddleware

* comment

pkg/api/http_server.go

@@ -596,7 +596,7 @@ func (hs *HTTPServer) addMiddlewaresAndStaticRoutes() {
 	m := hs.web
 
 	m.Use(requestmeta.SetupRequestMetadata())
-	m.Use(middleware.RequestTracing(hs.tracer))
+	m.Use(middleware.RequestTracing(hs.tracer, middleware.SkipTracingPaths))
 	m.Use(middleware.RequestMetrics(hs.Features, hs.Cfg, hs.promRegister))
 
 	m.UseMiddleware(hs.LoggerMiddleware.Middleware())

pkg/middleware/request_tracing.go

@@ -71,14 +71,23 @@ func RouteOperationName(req *http.Request) (string, bool) {
 	return "", false
 }
 
-func RequestTracing(tracer tracing.Tracer) web.Middleware {
+// Paths that don't need tracing spans applied to them because of the
+// little value that would provide us
+func SkipTracingPaths(req *http.Request) bool {
+	return strings.HasPrefix(req.URL.Path, "/public/") ||
+		req.URL.Path == "/robots.txt" ||
+		req.URL.Path == "/favicon.ico" ||
+		req.URL.Path == "/api/health"
+}
+
+func TraceAllPaths(req *http.Request) bool {
+	return true
+}
+
+func RequestTracing(tracer trace.Tracer, shouldTrace func(*http.Request) bool) web.Middleware {
 	return func(next http.Handler) http.Handler {
 		return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
-			// skip tracing for a few endpoints
-			if strings.HasPrefix(req.URL.Path, "/public/") ||
-				req.URL.Path == "/robots.txt" ||
-				req.URL.Path == "/favicon.ico" ||
-				req.URL.Path == "/api/health" {
+			if !shouldTrace(req) {
 				next.ServeHTTP(w, req)
 				return
 			}

pkg/server/module_server.go

@@ -193,7 +193,7 @@ func (s *ModuleServer) Run() error {
 	})
 
 	m.RegisterModule(modules.FrontendServer, func() (services.Service, error) {
-		return frontend.ProvideFrontendService(s.cfg, s.promGatherer, s.license)
+		return frontend.ProvideFrontendService(s.cfg, s.features, s.promGatherer, s.registerer, s.license)
 	})
 
 	m.RegisterModule(modules.All, nil)

pkg/services/frontend/context_middleware.go

@@ -0,0 +1,41 @@
+package frontend
+
+import (
+	"context"
+	"net/http"
+
+	"github.com/grafana/grafana/pkg/infra/log"
+	"github.com/grafana/grafana/pkg/infra/tracing"
+	"github.com/grafana/grafana/pkg/services/contexthandler/ctxkey"
+	contextmodel "github.com/grafana/grafana/pkg/services/contexthandler/model"
+	"github.com/grafana/grafana/pkg/web"
+)
+
+// Minimal copy of contextHandler.Middleware for frontend-service
+// frontend-service doesn't handle authentication or know what signed in users are
+func (s *frontendService) contextMiddleware() web.Middleware {
+	return func(next http.Handler) http.Handler {
+		return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			ctx := r.Context()
+
+			reqContext := &contextmodel.ReqContext{
+				Context: web.FromContext(ctx),
+				Logger:  log.New("context"),
+			}
+
+			// inject ReqContext in the context
+			ctx = context.WithValue(ctx, ctxkey.Key{}, reqContext)
+
+			// Set the context for the http.Request.Context
+			// This modifies both r and reqContext.Req since they point to the same value
+			*reqContext.Req = *reqContext.Req.WithContext(ctx)
+
+			traceID := tracing.TraceIDFromContext(ctx, false)
+			if traceID != "" {
+				reqContext.Logger = reqContext.Logger.New("traceID", traceID)
+			}
+
+			next.ServeHTTP(w, r.WithContext(ctx))
+		})
+	}
+}

pkg/services/frontend/frontend_service.go

@@ -9,11 +9,17 @@ import (
 	"github.com/prometheus/client_golang/prometheus"
 	"github.com/prometheus/client_golang/prometheus/promhttp"
 	"go.opentelemetry.io/otel"
+	"go.opentelemetry.io/otel/trace"
 
 	"github.com/grafana/dskit/services"
 	"github.com/grafana/grafana/pkg/infra/log"
+	"github.com/grafana/grafana/pkg/middleware"
+	"github.com/grafana/grafana/pkg/middleware/loggermw"
+	"github.com/grafana/grafana/pkg/middleware/requestmeta"
+	"github.com/grafana/grafana/pkg/services/featuremgmt"
 	"github.com/grafana/grafana/pkg/services/licensing"
 	"github.com/grafana/grafana/pkg/setting"
+	"github.com/grafana/grafana/pkg/web"
 )
 
 var tracer = otel.Tracer("github.com/grafana/grafana/pkg/services/frontend")
@@ -22,14 +28,18 @@ type frontendService struct {
 	*services.BasicService
 	cfg          *setting.Cfg
 	httpServ     *http.Server
+	features     featuremgmt.FeatureToggles
 	log          log.Logger
 	errChan      chan error
 	promGatherer prometheus.Gatherer
+	promRegister prometheus.Registerer
+	tracer       trace.Tracer
+	license      licensing.Licensing
 
 	index *IndexProvider
 }
 
-func ProvideFrontendService(cfg *setting.Cfg, promGatherer prometheus.Gatherer, license licensing.Licensing) (*frontendService, error) {
+func ProvideFrontendService(cfg *setting.Cfg, features featuremgmt.FeatureToggles, promGatherer prometheus.Gatherer, promRegister prometheus.Registerer, license licensing.Licensing) (*frontendService, error) {
 	index, err := NewIndexProvider(cfg, license)
 	if err != nil {
 		return nil, err
@@ -37,8 +47,12 @@ func ProvideFrontendService(cfg *setting.Cfg, promGatherer prometheus.Gatherer,
 
 	s := &frontendService{
 		cfg:          cfg,
+		features:     features,
 		log:          log.New("frontend-server"),
 		promGatherer: promGatherer,
+		promRegister: promRegister,
+		tracer:       tracer,
+		license:      license,
 		index:        index,
 	}
 	s.BasicService = services.NewBasicService(s.start, s.running, s.stop)
@@ -65,6 +79,7 @@ func (s *frontendService) running(ctx context.Context) error {
 
 func (s *frontendService) stop(failureReason error) error {
 	s.log.Info("stopping frontend server", "reason", failureReason)
+
 	if err := s.httpServ.Shutdown(context.Background()); err != nil {
 		s.log.Error("failed to shutdown frontend server", "error", err)
 		return err
@@ -75,17 +90,48 @@ func (s *frontendService) stop(failureReason error) error {
 func (s *frontendService) newFrontendServer(ctx context.Context) *http.Server {
 	s.log.Info("starting frontend server", "addr", ":"+s.cfg.HTTPPort)
 
-	router := http.NewServeMux()
-	router.Handle("/metrics", promhttp.HandlerFor(s.promGatherer, promhttp.HandlerOpts{EnableOpenMetrics: true}))
-	router.HandleFunc("/", s.index.HandleRequest)
+	// Use the same web.Mux as the main grafana server for consistency + middleware reuse
+	handler := web.New()
+	s.addMiddlewares(handler)
+	s.registerRoutes(handler)
 
 	server := &http.Server{
 		// 5s timeout for header reads to avoid Slowloris attacks (https://thetooth.io/blog/slowloris-attack/)
 		ReadHeaderTimeout: 5 * time.Second,
 		Addr:              ":" + s.cfg.HTTPPort,
-		Handler:           router,
+		Handler:           handler,
 		BaseContext:       func(_ net.Listener) context.Context { return ctx },
 	}
 
 	return server
 }
+
+func (s *frontendService) routeGet(m *web.Mux, pattern string, h ...web.Handler) {
+	handlers := append([]web.Handler{middleware.ProvideRouteOperationName(pattern)}, h...)
+	m.Get(pattern, handlers...)
+}
+
+// Apply the same middleware patterns as the main HTTP server
+func (s *frontendService) addMiddlewares(m *web.Mux) {
+	loggermiddleware := loggermw.Provide(s.cfg, s.features)
+
+	m.Use(requestmeta.SetupRequestMetadata())
+	m.UseMiddleware(s.contextMiddleware())
+
+	m.Use(middleware.RequestTracing(s.tracer, middleware.TraceAllPaths))
+	m.Use(middleware.RequestMetrics(s.features, s.cfg, s.promRegister))
+	m.UseMiddleware(loggermiddleware.Middleware())
+
+	m.UseMiddleware(middleware.Recovery(s.cfg, s.license))
+}
+
+func (s *frontendService) registerRoutes(m *web.Mux) {
+	s.routeGet(m, "/metrics", promhttp.HandlerFor(s.promGatherer, promhttp.HandlerOpts{EnableOpenMetrics: true}))
+
+	// Frontend service doesn't (yet?) serve any assets, so explicitly 404
+	// them so we can get logs for them
+	s.routeGet(m, "/public/*", http.NotFound)
+
+	// All other requests return index.html
+	s.routeGet(m, "/*", s.index.HandleRequest)
+}

pkg/services/frontend/frontend_service_test.go

@@ -0,0 +1,170 @@
+package frontend
+
+import (
+	"context"
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"testing"
+
+	"github.com/prometheus/client_golang/prometheus"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"github.com/grafana/grafana/pkg/services/contexthandler"
+	"github.com/grafana/grafana/pkg/services/featuremgmt"
+	"github.com/grafana/grafana/pkg/services/licensing"
+	"github.com/grafana/grafana/pkg/setting"
+	"github.com/grafana/grafana/pkg/web"
+)
+
+// Helper function to create a test service with minimal configuration
+func createTestService(t *testing.T, cfg *setting.Cfg) *frontendService {
+	t.Helper()
+
+	features := featuremgmt.WithFeatures()
+	license := &licensing.OSSLicensingService{}
+
+	var promRegister prometheus.Registerer = prometheus.NewRegistry()
+	promGatherer := promRegister.(*prometheus.Registry)
+
+	service, err := ProvideFrontendService(cfg, features, promGatherer, promRegister, license)
+	require.NoError(t, err)
+
+	return service
+}
+
+func TestFrontendService_ServerCreation(t *testing.T) {
+	t.Run("should create HTTP server with correct configuration", func(t *testing.T) {
+		publicDir := setupTestWebAssets(t)
+		cfg := &setting.Cfg{
+			HTTPPort:       "1234",
+			StaticRootPath: publicDir,
+		}
+		service := createTestService(t, cfg)
+
+		ctx := context.Background()
+		server := service.newFrontendServer(ctx)
+
+		assert.NotNil(t, server)
+		assert.Equal(t, ":1234", server.Addr)
+		assert.NotNil(t, server.Handler)
+		assert.NotNil(t, server.BaseContext)
+	})
+}
+
+func TestFrontendService_Routes(t *testing.T) {
+	publicDir := setupTestWebAssets(t)
+	cfg := &setting.Cfg{
+		HTTPPort:       "3000",
+		StaticRootPath: publicDir,
+	}
+	service := createTestService(t, cfg)
+
+	// Create a test mux to verify route registration
+	mux := web.New()
+	service.addMiddlewares(mux)
+	service.registerRoutes(mux)
+
+	t.Run("should handle frontend wildcard routes", func(t *testing.T) {
+		// Test that routes are registered by making requests
+		testCases := []struct {
+			path        string
+			description string
+		}{
+			// Metrics isn't registered in the test service?
+			{"/", "index route should return HTML"},
+			{"/dashboards", "browse dashboards route should return HTML"},
+			{"/d/de773f33s8qgwf/fep-homepage", "dashboard route should return HTML"},
+		}
+
+		for _, tc := range testCases {
+			t.Run(tc.description, func(t *testing.T) {
+				req := httptest.NewRequest("GET", tc.path, nil)
+				recorder := httptest.NewRecorder()
+
+				mux.ServeHTTP(recorder, req)
+
+				assert.Equal(t, 200, recorder.Code)
+				assert.Contains(t, recorder.Body.String(), "<div id=\"reactRoot\"></div>")
+			})
+		}
+	})
+
+	t.Run("should handle assets 404 correctly", func(t *testing.T) {
+		req := httptest.NewRequest("GET", "/public/build/app.js", nil)
+		recorder := httptest.NewRecorder()
+
+		mux.ServeHTTP(recorder, req)
+
+		assert.Equal(t, 404, recorder.Code)
+		assert.Equal(t, "404 page not found", strings.TrimSpace(recorder.Body.String()))
+	})
+
+	t.Run("should return prometheus metrics", func(t *testing.T) {
+		testCounter := prometheus.NewCounter(prometheus.CounterOpts{
+			Name: "shrimp_count",
+		})
+		err := service.promRegister.Register(testCounter)
+		require.NoError(t, err)
+		testCounter.Inc()
+
+		req := httptest.NewRequest("GET", "/metrics", nil)
+		recorder := httptest.NewRecorder()
+
+		mux.ServeHTTP(recorder, req)
+
+		assert.Contains(t, recorder.Body.String(), "\nshrimp_count 1\n")
+	})
+}
+
+func TestFrontendService_Middleware(t *testing.T) {
+	publicDir := setupTestWebAssets(t)
+	cfg := &setting.Cfg{
+		HTTPPort:       "3000",
+		StaticRootPath: publicDir,
+	}
+
+	t.Run("should register route prom metrics", func(t *testing.T) {
+		service := createTestService(t, cfg)
+		mux := web.New()
+		service.addMiddlewares(mux)
+		service.registerRoutes(mux)
+
+		req := httptest.NewRequest("GET", "/dashboards", nil)
+		recorder := httptest.NewRecorder()
+		mux.ServeHTTP(recorder, req)
+
+		req = httptest.NewRequest("GET", "/public/build/app.js", nil)
+		mux.ServeHTTP(recorder, req)
+
+		req = httptest.NewRequest("GET", "/metrics", nil)
+		mux.ServeHTTP(recorder, req)
+
+		metricsBody := recorder.Body.String()
+		assert.Contains(t, metricsBody, "# TYPE grafana_http_request_duration_seconds histogram")
+		assert.Contains(t, metricsBody, "grafana_http_request_duration_seconds_bucket{handler=\"public-assets\"") // assets 404
+		assert.Contains(t, metricsBody, "grafana_http_request_duration_seconds_bucket{handler=\"/*\"")            // index route
+	})
+
+	t.Run("should add context middleware", func(t *testing.T) {
+		service := createTestService(t, cfg)
+		mux := web.New()
+		service.addMiddlewares(mux)
+
+		mux.Get("/test-route", func(w http.ResponseWriter, r *http.Request) {
+			ctx := contexthandler.FromContext(r.Context())
+			assert.NotNil(t, ctx)
+			assert.NotNil(t, ctx.Context)
+			assert.NotNil(t, ctx.Logger)
+
+			w.WriteHeader(200)
+			_, err := w.Write([]byte("ok"))
+			require.NoError(t, err)
+		})
+
+		req := httptest.NewRequest("GET", "/test-route", nil)
+		recorder := httptest.NewRecorder()
+		mux.ServeHTTP(recorder, req)
+	})
+}

pkg/services/frontend/index.html

@@ -99,12 +99,7 @@
     </script>
 
     [[range $asset := .Assets.JSFiles]]
-      <script
-      nonce="[[$.Nonce]]"
-      src="[[$asset.FilePath]]"
-      type="text/javascript"
-      defer
-      ></script>
+      <script nonce="[[$.Nonce]]" src="[[$asset.FilePath]]" type="text/javascript" defer></script>
     [[end]]
   </body>
 </html>

pkg/services/frontend/index_test.go

@@ -0,0 +1,123 @@
+package frontend
+
+import (
+	"net/http/httptest"
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"github.com/grafana/grafana/pkg/setting"
+	"github.com/grafana/grafana/pkg/web"
+)
+
+// setupTestWebAssets creates a temporary directory with test assets manifest
+func setupTestWebAssets(tb testing.TB) string {
+	tb.Helper()
+
+	publicDir := tb.TempDir()
+	tb.Cleanup(func() { _ = os.RemoveAll(publicDir) })
+
+	// Create build directory
+	buildDir := filepath.Join(publicDir, "build")
+	err := os.MkdirAll(buildDir, 0750)
+	require.NoError(tb, err)
+
+	// Create test assets manifest
+	manifest := `{
+		"entrypoints": {
+			"app": {
+				"assets": {
+					"js": [
+						"public/build/runtime.js",
+						"public/build/app.js"
+					],
+					"css": ["public/build/grafana.app.css"]
+				}
+			},
+			"swagger": {
+				"assets": {
+					"js": ["public/build/runtime.js", "public/build/swagger.js"],
+					"css": ["public/build/grafana.swagger.css"]
+				}
+			},
+			"dark": {
+				"assets": {
+					"css": ["public/build/grafana.dark.css"]
+				}
+			},
+			"light": {
+				"assets": {
+					"css": ["public/build/grafana.light.css"]
+				}
+			}
+		},
+		"runtime.js": {
+			"src": "public/build/runtime.js",
+			"integrity": "sha256-test123"
+		},
+		"app.js": {
+			"src": "public/build/app.js",
+			"integrity": "sha256-test456"
+		}
+	}`
+
+	err = os.WriteFile(filepath.Join(buildDir, "assets-manifest.json"), []byte(manifest), 0644)
+	require.NoError(tb, err)
+
+	return publicDir
+}
+
+func TestFrontendService_WebAssets(t *testing.T) {
+	t.Run("should serve index with proper assets", func(t *testing.T) {
+		publicDir := setupTestWebAssets(t)
+		cfg := &setting.Cfg{
+			HTTPPort:       "3000",
+			StaticRootPath: publicDir,
+			Env:            setting.Dev, // needs to be dev to bypass the cache
+		}
+		service := createTestService(t, cfg)
+
+		mux := web.New()
+		service.addMiddlewares(mux)
+		service.registerRoutes(mux)
+
+		// Test index route which should load web assets
+		req := httptest.NewRequest("GET", "/", nil)
+		recorder := httptest.NewRecorder()
+
+		mux.ServeHTTP(recorder, req)
+
+		assert.Equal(t, 200, recorder.Code)
+		assert.Contains(t, recorder.Header().Get("Content-Type"), "text/html")
+
+		// The response should contain references to the assets
+		body := recorder.Body.String()
+		assert.Contains(t, body, "src=\"public/build/runtime.js\" type=\"text/javascript\"")
+		assert.Contains(t, body, "src=\"public/build/app.js\" type=\"text/javascript\"")
+	})
+
+	t.Run("should handle missing assets manifest gracefully", func(t *testing.T) {
+		cfg := &setting.Cfg{
+			HTTPPort:       "3000",
+			StaticRootPath: "/dev/null", // No build directory or manifest
+			Env:            setting.Dev, // needs to be dev to bypass the cache
+		}
+		service := createTestService(t, cfg)
+
+		mux := web.New()
+		service.addMiddlewares(mux)
+		service.registerRoutes(mux)
+
+		// Test index route which should fail to load web assets
+		req := httptest.NewRequest("GET", "/", nil)
+		recorder := httptest.NewRecorder()
+
+		mux.ServeHTTP(recorder, req)
+
+		// Should return 500 due to missing assets manifest
+		assert.Equal(t, 500, recorder.Code)
+	})
+}