diff --git a/bundles/prometheus/bundle.go b/bundles/prometheus/bundle.go index 65ebe24..0900e56 100644 --- a/bundles/prometheus/bundle.go +++ b/bundles/prometheus/bundle.go @@ -91,6 +91,8 @@ import ( "github.com/prometheus/client_golang/prometheus/collectors" "github.com/prometheus/client_golang/prometheus/promhttp" "github.com/rs/zerolog" + "google.golang.org/grpc" + "google.golang.org/grpc/status" "github.com/datariot/forge/errors" "github.com/datariot/forge/framework" @@ -265,9 +267,97 @@ func (b *Bundle) Initialize(app *framework.App) error { b.registry.MustRegister(collectors.NewProcessCollector(collectors.ProcessCollectorOpts{})) } + // Wire the bundle's own interceptor/middleware into the framework so + // RecordHTTPRequest/RecordGRPCRequest are called automatically for every + // request, without requiring per-handler instrumentation. Manual calls to + // RecordHTTPRequest/RecordGRPCRequest remain supported alongside this. + if b.config.EnableGRPCMetrics { + app.AddUnaryInterceptor(b.UnaryServerInterceptor()) + app.AddStreamInterceptor(b.StreamServerInterceptor()) + } + if b.config.EnableHTTPMetrics { + app.AddHTTPMiddleware(b.HTTPMiddleware()) + } + return nil } +// UnaryServerInterceptor returns a gRPC unary server interceptor that +// automatically records RecordGRPCRequest metrics for every unary call. The +// status label is derived from the returned error via gRPC status codes +// (e.g. "OK", "NotFound", "Internal"), so a nil error always records "OK". +func (b *Bundle) UnaryServerInterceptor() grpc.UnaryServerInterceptor { + return func(ctx context.Context, req interface{}, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (interface{}, error) { + start := time.Now() + resp, err := handler(ctx, req) + b.RecordGRPCRequest(info.FullMethod, status.Code(err).String(), time.Since(start)) + return resp, err + } +} + +// StreamServerInterceptor returns a gRPC stream server interceptor that +// automatically records RecordGRPCRequest metrics for every streaming call. +// See UnaryServerInterceptor for how the status label is derived. +func (b *Bundle) StreamServerInterceptor() grpc.StreamServerInterceptor { + return func(srv interface{}, ss grpc.ServerStream, info *grpc.StreamServerInfo, handler grpc.StreamHandler) error { + start := time.Now() + err := handler(srv, ss) + b.RecordGRPCRequest(info.FullMethod, status.Code(err).String(), time.Since(start)) + return err + } +} + +// HTTPMiddleware returns HTTP middleware that automatically records +// RecordHTTPRequest metrics for every request. +// +// The endpoint label prefers r.Pattern, the matched net/http.ServeMux +// pattern (e.g. "GET /users/{id}"), which the framework's mux populates +// before invoking the handler. That keeps the label a bounded route +// template rather than raw concrete paths. It falls back to r.URL.Path only +// for requests that matched no route (e.g. 404s), where cardinality is +// naturally limited by what clients actually request. +func (b *Bundle) HTTPMiddleware() func(http.Handler) http.Handler { + return func(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + start := time.Now() + wrapper := &statusCapturingResponseWriter{ResponseWriter: w, statusCode: http.StatusOK} + + next.ServeHTTP(wrapper, r) + + endpoint := r.Pattern + if endpoint == "" { + endpoint = r.URL.Path + } + b.RecordHTTPRequest(r.Method, endpoint, wrapper.statusCode, time.Since(start)) + }) + } +} + +// statusCapturingResponseWriter wraps http.ResponseWriter to capture the +// final status code for metrics. framework/http.go has an equivalent +// responseWriter for its request-logging middleware, but it is unexported +// and package-private, so this is a small local copy rather than a shared +// dependency across module boundaries. +type statusCapturingResponseWriter struct { + http.ResponseWriter + statusCode int +} + +// WriteHeader captures the status code before delegating. +func (w *statusCapturingResponseWriter) WriteHeader(code int) { + w.statusCode = code + w.ResponseWriter.WriteHeader(code) +} + +// Write ensures a status code is recorded even if WriteHeader was never +// called explicitly (the standard library implicitly writes 200 OK). +func (w *statusCapturingResponseWriter) Write(data []byte) (int, error) { + if w.statusCode == 0 { + w.statusCode = http.StatusOK + } + return w.ResponseWriter.Write(data) +} + // Registry returns the Prometheus registry for custom metric registration. func (b *Bundle) Registry() prometheus.Registerer { return b.registry diff --git a/bundles/prometheus/bundle_middleware_test.go b/bundles/prometheus/bundle_middleware_test.go new file mode 100644 index 0000000..95fb48d --- /dev/null +++ b/bundles/prometheus/bundle_middleware_test.go @@ -0,0 +1,269 @@ +package prometheus + +import ( + "context" + "net/http" + "net/http/httptest" + "testing" + + "github.com/prometheus/client_golang/prometheus/testutil" + "google.golang.org/grpc" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" + + "github.com/datariot/forge/config" + "github.com/datariot/forge/framework" +) + +// newTestApp creates a minimal, un-started framework.App suitable for +// exercising Bundle.Initialize and the framework's bundle-seam methods +// (AddUnaryInterceptor/AddStreamInterceptor/AddHTTPMiddleware) without +// needing to bind real network listeners. +func newTestApp(t *testing.T) *framework.App { + t.Helper() + cfg := config.DefaultBaseConfig() + cfg.ServiceName = "prometheus-bundle-test" + cfg.GRPCAddr = ":0" + cfg.HTTPAddr = ":0" + + app, err := framework.New(framework.WithConfig(&cfg)) + if err != nil { + t.Fatalf("failed to create app: %v", err) + } + return app +} + +// --- Initialize wiring --- + +func TestBundle_Initialize_WiresAutomaticHTTPMetrics(t *testing.T) { + reg := newTestRegistry() + cfg := DefaultConfig() + cfg.Namespace = "test" + cfg.Registry = reg + cfg.EnableGRPCMetrics = false + b := NewBundle(cfg) + app := newTestApp(t) + + if err := b.Initialize(app); err != nil { + t.Fatalf("Initialize failed: %v", err) + } + + // Build the framework's real HTTP handler the way startHTTPServer does, + // and drive a request through it end-to-end: this is what proves the + // metrics are actually automatic rather than requiring a manual + // RecordHTTPRequest call in a handler. + builder := framework.NewHTTPServerBuilder(app, framework.DefaultHTTPServerConfig()) + httpServer, err := builder.Build() + if err != nil { + t.Fatalf("failed to build HTTP server: %v", err) + } + + req := httptest.NewRequest(http.MethodGet, "/health/live", nil) + w := httptest.NewRecorder() + httpServer.Handler.ServeHTTP(w, req) + + if count := testutil.CollectAndCount(reg, "test_http_requests_total"); count == 0 { + t.Error("expected http_requests_total to have recorded a series after a request through the wrapped handler") + } +} + +func TestBundle_Initialize_WiresAutomaticGRPCMetrics(t *testing.T) { + reg := newTestRegistry() + cfg := DefaultConfig() + cfg.Namespace = "test" + cfg.Registry = reg + cfg.EnableHTTPMetrics = false + b := NewBundle(cfg) + app := newTestApp(t) + + if err := b.Initialize(app); err != nil { + t.Fatalf("Initialize failed: %v", err) + } + + // Invoke the interceptor directly with a fake handler + UnaryServerInfo, + // mirroring how grpc.ChainUnaryInterceptor would call it for a real RPC. + interceptor := b.UnaryServerInterceptor() + fakeHandler := func(ctx context.Context, req interface{}) (interface{}, error) { + return "ok", nil + } + info := &grpc.UnaryServerInfo{FullMethod: "/test.Service/Method"} + + if _, err := interceptor(context.Background(), nil, info, fakeHandler); err != nil { + t.Fatalf("interceptor returned unexpected error: %v", err) + } + + if count := testutil.CollectAndCount(reg, "test_grpc_requests_total"); count == 0 { + t.Error("expected grpc_requests_total to have recorded a series after invoking the interceptor") + } +} + +// --- UnaryServerInterceptor --- + +func TestBundle_UnaryServerInterceptor_RecordsOKStatus(t *testing.T) { + cfg := Config{ + Namespace: "test", + EnableGRPCMetrics: true, + HistogramBuckets: []float64{0.01, 0.1, 1.0}, + ServiceLabels: map[string]string{}, + } + b := newInitializedBundle(t, cfg) + + interceptor := b.UnaryServerInterceptor() + info := &grpc.UnaryServerInfo{FullMethod: "/test.Service/Get"} + + _, err := interceptor(context.Background(), nil, info, + func(ctx context.Context, req interface{}) (interface{}, error) { + return nil, nil + }) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if got := testutil.ToFloat64(b.grpcRequestsTotal.WithLabelValues("/test.Service/Get", codes.OK.String())); got != 1 { + t.Errorf("expected 1 recorded OK request, got %v", got) + } +} + +func TestBundle_UnaryServerInterceptor_RecordsErrorStatus(t *testing.T) { + cfg := Config{ + Namespace: "test", + EnableGRPCMetrics: true, + HistogramBuckets: []float64{0.01, 0.1, 1.0}, + ServiceLabels: map[string]string{}, + } + b := newInitializedBundle(t, cfg) + + interceptor := b.UnaryServerInterceptor() + info := &grpc.UnaryServerInfo{FullMethod: "/test.Service/Get"} + + _, err := interceptor(context.Background(), nil, info, + func(ctx context.Context, req interface{}) (interface{}, error) { + return nil, status.Error(codes.NotFound, "missing") + }) + if err == nil { + t.Fatal("expected handler error to propagate") + } + + if got := testutil.ToFloat64(b.grpcRequestsTotal.WithLabelValues("/test.Service/Get", codes.NotFound.String())); got != 1 { + t.Errorf("expected 1 recorded NotFound request, got %v", got) + } +} + +// --- StreamServerInterceptor --- + +func TestBundle_StreamServerInterceptor_RecordsMetrics(t *testing.T) { + cfg := Config{ + Namespace: "test", + EnableGRPCMetrics: true, + HistogramBuckets: []float64{0.01, 0.1, 1.0}, + ServiceLabels: map[string]string{}, + } + b := newInitializedBundle(t, cfg) + + interceptor := b.StreamServerInterceptor() + info := &grpc.StreamServerInfo{FullMethod: "/test.Service/Stream"} + + err := interceptor(nil, nil, info, func(srv interface{}, stream grpc.ServerStream) error { + return nil + }) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if got := testutil.ToFloat64(b.grpcRequestsTotal.WithLabelValues("/test.Service/Stream", codes.OK.String())); got != 1 { + t.Errorf("expected 1 recorded stream request, got %v", got) + } +} + +// --- HTTPMiddleware --- + +func TestBundle_HTTPMiddleware_UsesRoutePatternWhenAvailable(t *testing.T) { + cfg := Config{ + Namespace: "test", + EnableHTTPMetrics: true, + HistogramBuckets: []float64{0.01, 0.1, 1.0}, + ServiceLabels: map[string]string{}, + } + b := newInitializedBundle(t, cfg) + + mux := http.NewServeMux() + mux.HandleFunc("GET /users/{id}", func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + }) + handler := b.HTTPMiddleware()(mux) + + req := httptest.NewRequest(http.MethodGet, "/users/42", nil) + w := httptest.NewRecorder() + handler.ServeHTTP(w, req) + + if got := testutil.ToFloat64(b.httpRequestsTotal.WithLabelValues("GET", "GET /users/{id}", "200")); got != 1 { + t.Errorf("expected the low-cardinality route pattern to be used as the endpoint label, got %v", got) + } +} + +func TestBundle_HTTPMiddleware_FallsBackToPathWhenUnmatched(t *testing.T) { + cfg := Config{ + Namespace: "test", + EnableHTTPMetrics: true, + HistogramBuckets: []float64{0.01, 0.1, 1.0}, + ServiceLabels: map[string]string{}, + } + b := newInitializedBundle(t, cfg) + + mux := http.NewServeMux() // no routes registered, so every request 404s with no pattern + handler := b.HTTPMiddleware()(mux) + + req := httptest.NewRequest(http.MethodGet, "/nope", nil) + w := httptest.NewRecorder() + handler.ServeHTTP(w, req) + + if got := testutil.ToFloat64(b.httpRequestsTotal.WithLabelValues("GET", "/nope", "404")); got != 1 { + t.Errorf("expected raw path fallback when no route pattern matched, got %v", got) + } +} + +func TestBundle_HTTPMiddleware_MetricsDisabled(t *testing.T) { + cfg := Config{ + Namespace: "test", + EnableHTTPMetrics: false, + HistogramBuckets: []float64{0.01, 0.1, 1.0}, + ServiceLabels: map[string]string{}, + } + b := newInitializedBundle(t, cfg) + + handler := b.HTTPMiddleware()(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + })) + + req := httptest.NewRequest(http.MethodGet, "/anything", nil) + w := httptest.NewRecorder() + + // Should not panic even though httpRequestsTotal/httpRequestDuration are nil. + handler.ServeHTTP(w, req) + if w.Code != http.StatusOK { + t.Errorf("expected 200, got %d", w.Code) + } +} + +func TestBundle_HTTPMiddleware_ImplicitWriteRecordsOK(t *testing.T) { + cfg := Config{ + Namespace: "test", + EnableHTTPMetrics: true, + HistogramBuckets: []float64{0.01, 0.1, 1.0}, + ServiceLabels: map[string]string{}, + } + b := newInitializedBundle(t, cfg) + + // Handler writes a body without ever calling WriteHeader explicitly. + handler := b.HTTPMiddleware()(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte("ok")) + })) + + req := httptest.NewRequest(http.MethodGet, "/implicit", nil) + w := httptest.NewRecorder() + handler.ServeHTTP(w, req) + + if got := testutil.ToFloat64(b.httpRequestsTotal.WithLabelValues("GET", "/implicit", "200")); got != 1 { + t.Errorf("expected implicit 200 to be recorded, got %v", got) + } +} diff --git a/framework/app.go b/framework/app.go index c7581cd..8546d3b 100644 --- a/framework/app.go +++ b/framework/app.go @@ -248,6 +248,10 @@ type App struct { unaryInterceptors []grpc.UnaryServerInterceptor streamInterceptors []grpc.StreamServerInterceptor + // HTTP middleware, applied outermost-first around the built mux (health, + // metrics, pprof, and user HTTPRegistrar routes). + httpMiddlewares []func(http.Handler) http.Handler + // Hooks startupHooks []StartupHook shutdownHooks []ShutdownHook @@ -273,6 +277,7 @@ func New(options ...AppOption) (*App, error) { bundles: make([]Bundle, 0), unaryInterceptors: make([]grpc.UnaryServerInterceptor, 0), streamInterceptors: make([]grpc.StreamServerInterceptor, 0), + httpMiddlewares: make([]func(http.Handler) http.Handler, 0), startupHooks: make([]StartupHook, 0), shutdownHooks: make([]ShutdownHook, 0), } @@ -485,6 +490,78 @@ func WithStreamInterceptor(interceptor grpc.StreamServerInterceptor) AppOption { } } +// AddUnaryInterceptor registers a gRPC unary server interceptor from a +// bundle's Initialize method (or any other code that runs before the gRPC +// server is built). Interceptors are applied in registration order — +// combined with any interceptors passed to New via WithUnaryInterceptor, +// which run first — so a bundle wanting to be outermost (e.g. to time the +// whole call for metrics) should be registered as one of the first bundles. +// +// The gRPC server is built partway through Start, in startGRPCServer, which +// runs after all bundles' Initialize methods. Since Initialize always runs +// before that point, calling AddUnaryInterceptor from Initialize is safe. +// Calling it after the server has already been built is a no-op: interceptors +// are baked into grpc.NewServer at construction time and cannot be added +// afterward. +func (a *App) AddUnaryInterceptor(interceptor grpc.UnaryServerInterceptor) { + if interceptor == nil { + return + } + a.mu.Lock() + defer a.mu.Unlock() + if a.grpcServer != nil { + return + } + a.unaryInterceptors = append(a.unaryInterceptors, interceptor) +} + +// AddStreamInterceptor registers a gRPC stream server interceptor. See +// AddUnaryInterceptor for the ordering and timing guarantees. +func (a *App) AddStreamInterceptor(interceptor grpc.StreamServerInterceptor) { + if interceptor == nil { + return + } + a.mu.Lock() + defer a.mu.Unlock() + if a.grpcServer != nil { + return + } + a.streamInterceptors = append(a.streamInterceptors, interceptor) +} + +// AddHTTPMiddleware registers HTTP middleware from a bundle's Initialize +// method (or any other code that runs before the HTTP server is built). +// Middleware is applied outermost-first in registration order — the first +// middleware added sees the request first and the response last — and wraps +// the complete handler built by startHTTPServer, including the health, +// metrics, pprof, and any user HTTPRegistrar routes. +// +// The HTTP server is built partway through Start, in startHTTPServer, which +// runs after all bundles' Initialize methods, so calling AddHTTPMiddleware +// from Initialize is safe. Calling it after the server has already been +// built is a no-op. +func (a *App) AddHTTPMiddleware(middleware func(http.Handler) http.Handler) { + if middleware == nil { + return + } + a.mu.Lock() + defer a.mu.Unlock() + if a.httpServer != nil { + return + } + a.httpMiddlewares = append(a.httpMiddlewares, middleware) +} + +// httpMiddlewareSnapshot returns a copy of the currently registered HTTP +// middleware chain, in registration order. +func (a *App) httpMiddlewareSnapshot() []func(http.Handler) http.Handler { + a.mu.RLock() + defer a.mu.RUnlock() + snapshot := make([]func(http.Handler) http.Handler, len(a.httpMiddlewares)) + copy(snapshot, a.httpMiddlewares) + return snapshot +} + // Config returns the application configuration. func (a *App) Config() *config.BaseConfig { return a.config diff --git a/framework/app_middleware_test.go b/framework/app_middleware_test.go new file mode 100644 index 0000000..618b9c8 --- /dev/null +++ b/framework/app_middleware_test.go @@ -0,0 +1,209 @@ +package framework + +import ( + "context" + "net/http" + "net/http/httptest" + "testing" + "time" + + "google.golang.org/grpc" + "google.golang.org/grpc/credentials/insecure" + "google.golang.org/grpc/health/grpc_health_v1" + + "github.com/datariot/forge/config" +) + +// --- AddUnaryInterceptor / AddStreamInterceptor --- + +func TestApp_AddUnaryInterceptor_Nil(t *testing.T) { + cfg := config.DefaultBaseConfig() + cfg.ServiceName = "add-unary-interceptor-nil-test" + + app, err := New(WithConfig(&cfg)) + if err != nil { + t.Fatalf("failed to create app: %v", err) + } + + app.AddUnaryInterceptor(nil) + if len(app.unaryInterceptors) != 0 { + t.Errorf("expected nil interceptor to be ignored, got %d registered", len(app.unaryInterceptors)) + } +} + +func TestApp_AddStreamInterceptor_Nil(t *testing.T) { + cfg := config.DefaultBaseConfig() + cfg.ServiceName = "add-stream-interceptor-nil-test" + + app, err := New(WithConfig(&cfg)) + if err != nil { + t.Fatalf("failed to create app: %v", err) + } + + app.AddStreamInterceptor(nil) + if len(app.streamInterceptors) != 0 { + t.Errorf("expected nil interceptor to be ignored, got %d registered", len(app.streamInterceptors)) + } +} + +// TestApp_AddUnaryInterceptor_RunsForRealCall verifies that a unary +// interceptor registered via the bundle seam (AddUnaryInterceptor, called +// the way a bundle would from Initialize) actually runs for a real gRPC +// call, using the always-registered health service as the callee. +func TestApp_AddUnaryInterceptor_RunsForRealCall(t *testing.T) { + cfg := config.DefaultBaseConfig() + cfg.ServiceName = "add-unary-interceptor-test" + cfg.GRPCAddr = ":0" + cfg.HTTPAddr = ":0" + + app, err := New(WithConfig(&cfg), WithGRPCRegistrar(&testRegistrar{})) + if err != nil { + t.Fatalf("failed to create app: %v", err) + } + + called := false + app.AddUnaryInterceptor(func(ctx context.Context, req interface{}, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (interface{}, error) { + called = true + return handler(ctx, req) + }) + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + if err := app.Start(ctx); err != nil { + t.Fatalf("failed to start app: %v", err) + } + defer func() { _ = app.Stop(context.Background()) }() + + conn, err := grpc.NewClient(app.grpcListener.Addr().String(), grpc.WithTransportCredentials(insecure.NewCredentials())) + if err != nil { + t.Fatalf("failed to dial grpc server: %v", err) + } + defer func() { _ = conn.Close() }() + + client := grpc_health_v1.NewHealthClient(conn) + if _, err := client.Check(ctx, &grpc_health_v1.HealthCheckRequest{}); err != nil { + t.Fatalf("health check RPC failed: %v", err) + } + + if !called { + t.Error("expected unary interceptor registered via AddUnaryInterceptor to run") + } +} + +// TestApp_AddUnaryInterceptor_NoopAfterServerBuilt verifies that adding an +// interceptor after the gRPC server has already been constructed is a +// documented no-op rather than a panic or a silently-ignored-but-misleading +// success. +func TestApp_AddUnaryInterceptor_NoopAfterServerBuilt(t *testing.T) { + cfg := config.DefaultBaseConfig() + cfg.ServiceName = "add-unary-interceptor-noop-test" + cfg.GRPCAddr = ":0" + cfg.HTTPAddr = ":0" + + app, err := New(WithConfig(&cfg), WithGRPCRegistrar(&testRegistrar{})) + if err != nil { + t.Fatalf("failed to create app: %v", err) + } + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + if err := app.Start(ctx); err != nil { + t.Fatalf("failed to start app: %v", err) + } + defer func() { _ = app.Stop(context.Background()) }() + + before := len(app.unaryInterceptors) + app.AddUnaryInterceptor(func(ctx context.Context, req interface{}, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (interface{}, error) { + return handler(ctx, req) + }) + if len(app.unaryInterceptors) != before { + t.Error("expected AddUnaryInterceptor to no-op once the gRPC server is built") + } +} + +// --- AddHTTPMiddleware --- + +func TestApp_AddHTTPMiddleware_Nil(t *testing.T) { + cfg := config.DefaultBaseConfig() + cfg.ServiceName = "add-http-middleware-nil-test" + + app, err := New(WithConfig(&cfg)) + if err != nil { + t.Fatalf("failed to create app: %v", err) + } + + app.AddHTTPMiddleware(nil) + if len(app.httpMiddlewares) != 0 { + t.Errorf("expected nil middleware to be ignored, got %d registered", len(app.httpMiddlewares)) + } +} + +// TestApp_AddHTTPMiddleware_RunsForHealthEndpoint verifies that HTTP +// middleware registered via the bundle seam (AddHTTPMiddleware, called the +// way a bundle would from Initialize) actually wraps the handler the +// framework builds, including the built-in /health route. +func TestApp_AddHTTPMiddleware_RunsForHealthEndpoint(t *testing.T) { + cfg := config.DefaultBaseConfig() + cfg.ServiceName = "add-http-middleware-test" + cfg.GRPCAddr = ":0" + cfg.HTTPAddr = ":0" + + app, err := New(WithConfig(&cfg)) + if err != nil { + t.Fatalf("failed to create app: %v", err) + } + + called := false + app.AddHTTPMiddleware(func(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + called = true + next.ServeHTTP(w, r) + }) + }) + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + if err := app.Start(ctx); err != nil { + t.Fatalf("failed to start app: %v", err) + } + defer func() { _ = app.Stop(context.Background()) }() + + req := httptest.NewRequest(http.MethodGet, "/health", nil) + w := httptest.NewRecorder() + app.httpServer.Handler.ServeHTTP(w, req) + + if !called { + t.Error("expected HTTP middleware registered via AddHTTPMiddleware to run for /health") + } + if w.Code != http.StatusOK && w.Code != http.StatusServiceUnavailable { + t.Errorf("expected /health to still respond normally through the wrapped handler, got %d", w.Code) + } +} + +// TestApp_AddHTTPMiddleware_NoopAfterServerBuilt verifies that adding +// middleware after the HTTP server has already been built is a documented +// no-op. +func TestApp_AddHTTPMiddleware_NoopAfterServerBuilt(t *testing.T) { + cfg := config.DefaultBaseConfig() + cfg.ServiceName = "add-http-middleware-noop-test" + cfg.GRPCAddr = ":0" + cfg.HTTPAddr = ":0" + + app, err := New(WithConfig(&cfg)) + if err != nil { + t.Fatalf("failed to create app: %v", err) + } + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + if err := app.Start(ctx); err != nil { + t.Fatalf("failed to start app: %v", err) + } + defer func() { _ = app.Stop(context.Background()) }() + + before := len(app.httpMiddlewares) + app.AddHTTPMiddleware(func(next http.Handler) http.Handler { return next }) + if len(app.httpMiddlewares) != before { + t.Error("expected AddHTTPMiddleware to no-op once the HTTP server is built") + } +} diff --git a/framework/http.go b/framework/http.go index 7334b15..acf3c6f 100644 --- a/framework/http.go +++ b/framework/http.go @@ -179,6 +179,15 @@ func (b *HTTPServerBuilder) buildHandler() http.Handler { handler = b.corsMiddleware(handler) } + // Bundle-registered middleware (e.g. the prometheus bundle's automatic + // request metrics), applied outermost so it covers every route above + // including health and metrics endpoints. Applied in reverse so the + // first-registered middleware ends up outermost. + middlewares := b.app.httpMiddlewareSnapshot() + for i := len(middlewares) - 1; i >= 0; i-- { + handler = middlewares[i](handler) + } + return handler } diff --git a/go.mod b/go.mod index 577ba5b..da1d465 100644 --- a/go.mod +++ b/go.mod @@ -36,6 +36,7 @@ require ( github.com/jackc/pgpassfile v1.0.0 // indirect github.com/jackc/pgservicefile v0.0.0-20240606120523-5a60cdf6a761 // indirect github.com/jackc/puddle/v2 v2.2.2 // indirect + github.com/kylelemons/godebug v1.1.0 // indirect github.com/mattn/go-colorable v0.1.13 // indirect github.com/mattn/go-isatty v0.0.19 // indirect github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect