chore(rest-api): Revise metrics namespace, inventory metrics, retire PagerDuty

rest-api/api/cmd/api/main.go

@@ -102,7 +102,7 @@ func main() {
 	mconfig := cfg.GetMetricsConfig()
 	if mconfig.Enabled {
 		// Initialize Prometheus Echo instance
-		ep := capis.InitMetricsServer(e, cfg)
+		ep := capis.InitMetricsServer(e, mconfig.Namespace)
 
 		// Start Prometheus server
 		log.Info().Msg("starting Metrics server")

rest-api/api/internal/config/config.go

@@ -105,6 +105,8 @@ const (
 	ConfigMetricsEnabled = "metrics.enabled"
 	// ConfigMetricsPort specifies the port for Prometheus metrics
 	ConfigMetricsPort = "metrics.port"
+	// ConfigMetricsNamespace specifies the prefix for every exposed metric name
+	ConfigMetricsNamespace = "metrics.namespace"
 
 	// ConfigTracingEnabled is a feature flag for tracing
 	ConfigTracingEnabled = "tracing.enabled"
@@ -256,6 +258,7 @@ func NewConfig() *Config {
 
 	c.v.SetDefault(ConfigMetricsEnabled, true)
 	c.v.SetDefault(ConfigMetricsPort, 9360)
+	c.v.SetDefault(ConfigMetricsNamespace, DefaultMetricsNamespace)
 
 	c.v.SetDefault(ConfigTracingEnabled, false)
 
@@ -539,7 +542,7 @@ func (c *Config) GetSiteConfig() *SiteConfig {
 
 // GetMetricsConfig returns the Metrics config
 func (c *Config) GetMetricsConfig() *MetricsConfig {
-	return NewMetricsConfig(c.GetMetricsEnabled(), c.GetMetricsPort())
+	return NewMetricsConfig(c.GetMetricsEnabled(), c.GetMetricsPort(), c.GetMetricsNamespace())
 }
 
 // GetRateLimiterConfig returns the rate limiter config
@@ -968,6 +971,17 @@ func (c *Config) GetMetricsPort() int {
 	return c.v.GetInt(ConfigMetricsPort)
 }
 
+// GetMetricsNamespace gets the prefix applied to every exposed metric name.
+// An explicitly empty value falls back to the default, since echoprometheus
+// substitutes its own "echo" prefix for an empty one.
+func (c *Config) GetMetricsNamespace() string {
+	namespace := c.v.GetString(ConfigMetricsNamespace)
+	if namespace == "" {
+		return DefaultMetricsNamespace
+	}
+	return namespace
+}
+
 // GetTracingEnabled gets the enabled field for tracing
 func (c *Config) GetTracingEnabled() bool {
 	return c.v.GetBool(ConfigTracingEnabled)

rest-api/api/internal/config/metrics.go

@@ -7,10 +7,17 @@ import (
 	"fmt"
 )
 
-// TemporalConfig holds configuration for Temporal communication
+// DefaultMetricsNamespace prefixes every metric this server exposes and matches
+// its nico-rest-api Helm service name. Operators override it with
+// metrics.namespace. Deliberately independent of api.name, which is the URL path
+// segment callers route on and has no bearing on how these series are named.
+const DefaultMetricsNamespace = "nico_rest_api"
+
+// MetricsConfig holds configuration of Metrics
 type MetricsConfig struct {
-	Enabled bool
-	Port    int
+	Enabled   bool
+	Port      int
+	Namespace string
 }
 
 // GetListenAddr returns the local address for listen socket.
@@ -19,9 +26,10 @@ func (mcfg *MetricsConfig) GetListenAddr() string {
 }
 
 // NewMetricsConfig initializes and returns a configuration object for managing Metrics
-func NewMetricsConfig(enabled bool, port int) *MetricsConfig {
+func NewMetricsConfig(enabled bool, port int, namespace string) *MetricsConfig {
 	return &MetricsConfig{
-		Enabled: enabled,
-		Port:    port,
+		Enabled:   enabled,
+		Port:      port,
+		Namespace: namespace,
 	}
 }

rest-api/api/internal/config/metrics_test.go

@@ -5,17 +5,15 @@ package config
 
 import (
 	"testing"
+
+	"github.com/stretchr/testify/assert"
 )
 
 func TestMetricsConfig(t *testing.T) {
 	type args struct {
-		enabled bool
-		port    int
-	}
-
-	mcfg := MetricsConfig{
-		Enabled: true,
-		Port:    6930,
+		enabled   bool
+		port      int
+		namespace string
 	}
 
 	tests := []struct {
@@ -26,23 +24,71 @@ func TestMetricsConfig(t *testing.T) {
 		{
 			name: "initialize Metrics config",
 			args: args{
-				enabled: true,
-				port:    mcfg.Port,
+				enabled:   true,
+				port:      6930,
+				namespace: DefaultMetricsNamespace,
+			},
+			want: &MetricsConfig{
+				Enabled:   true,
+				Port:      6930,
+				Namespace: DefaultMetricsNamespace,
+			},
+		},
+		{
+			name: "initialize Metrics config with an overridden namespace",
+			args: args{
+				enabled:   true,
+				port:      6930,
+				namespace: "acme_api",
+			},
+			want: &MetricsConfig{
+				Enabled:   true,
+				Port:      6930,
+				Namespace: "acme_api",
 			},
-			want: &mcfg,
 		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			got := NewMetricsConfig(tt.args.enabled, tt.args.port)
+			got := NewMetricsConfig(tt.args.enabled, tt.args.port, tt.args.namespace)
+
+			assert.Equal(t, tt.want, got)
+			assert.Equal(t, tt.want.GetListenAddr(), got.GetListenAddr())
+		})
+	}
+}
 
-			if p := got.Port; p != tt.want.Port {
-				t.Errorf("got.Port = %v, want %v", p, tt.want.Port)
-			}
+func TestConfig_GetMetricsNamespace(t *testing.T) {
+	tests := []struct {
+		name      string
+		configure func(c *Config)
+		want      string
+	}{
+		{
+			name:      "unset falls back to the default",
+			configure: func(c *Config) {},
+			want:      DefaultMetricsNamespace,
+		},
+		{
+			// An operator who blanks the key gets the default rather than
+			// echoprometheus substituting its own "echo" prefix.
+			name:      "explicitly empty falls back to the default",
+			configure: func(c *Config) { c.v.Set(ConfigMetricsNamespace, "") },
+			want:      DefaultMetricsNamespace,
+		},
+		{
+			name:      "override is honored",
+			configure: func(c *Config) { c.v.Set(ConfigMetricsNamespace, "acme_api") },
+			want:      "acme_api",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			c := NewConfig()
+			tt.configure(c)
+			defer c.v.Set(ConfigMetricsNamespace, DefaultMetricsNamespace)
 
-			if got := got.GetListenAddr(); got != tt.want.GetListenAddr() {
-				t.Errorf("GetListenAddr() = %v, want %v", got, tt.want.GetListenAddr())
-			}
+			assert.Equal(t, tt.want, c.GetMetricsNamespace())
 		})
 	}
 }

rest-api/api/internal/server/server.go

@@ -300,12 +300,14 @@ func InitAPIServer(cfg *config.Config, dbSession *cdb.Session, tc tsdkClient.Cli
 	return e
 }
 
-func InitMetricsServer(e *echo.Echo, cfg *config.Config) *echo.Echo {
+func InitMetricsServer(e *echo.Echo, namespace string) *echo.Echo {
 	ep := echo.New()
 	ep.HideBanner = true
 
 	conf := echoPrometheus.MiddlewareConfig{
-		Subsystem: fmt.Sprintf("%s_api", cfg.GetAPIName()),
+		// The prefix has to go in Subsystem, since echoprometheus substitutes
+		// its own "echo" for an empty one.
+		Subsystem: namespace,
 		Skipper:   api.MetricsURLSkipper,
 	}
 

rest-api/api/internal/server/server_test.go

@@ -25,6 +25,7 @@ import (
 	cdbm "github.com/NVIDIA/infra-controller/rest-api/db/pkg/db/model"
 	cdbu "github.com/NVIDIA/infra-controller/rest-api/db/pkg/util"
 	echo "github.com/labstack/echo/v4"
+	"github.com/prometheus/client_golang/prometheus"
 	"github.com/stretchr/testify/assert"
 	temporalClient "go.temporal.io/sdk/client"
 	tmocks "go.temporal.io/sdk/mocks"
@@ -124,8 +125,7 @@ func Test_InitTemporalClients(t *testing.T) {
 
 func Test_InitMetricsServer(t *testing.T) {
 	type args struct {
-		e   *echo.Echo
-		cfg *config.Config
+		e *echo.Echo
 	}
 	tests := []struct {
 		name string
@@ -134,14 +134,32 @@ func Test_InitMetricsServer(t *testing.T) {
 		{
 			name: "test initMetricsServer success",
 			args: args{
-				e:   echo.New(),
-				cfg: common.GetTestConfig(),
+				e: echo.New(),
 			},
 		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			InitMetricsServer(tt.args.e, tt.args.cfg)
+			// A tracked route, since MetricsURLSkipper only records /v2/ and /metrics.
+			tt.args.e.GET("/v2/probe", func(c echo.Context) error {
+				return c.NoContent(http.StatusOK)
+			})
+
+			InitMetricsServer(tt.args.e, config.DefaultMetricsNamespace)
+
+			tt.args.e.ServeHTTP(httptest.NewRecorder(), httptest.NewRequest(http.MethodGet, "/v2/probe", nil))
+
+			// The prefix is the published contract. An empty Subsystem would
+			// silently produce echo_requests_total instead.
+			families, err := prometheus.DefaultGatherer.Gather()
+			assert.NoError(t, err)
+
+			names := make([]string, 0, len(families))
+			for _, family := range families {
+				names = append(names, family.GetName())
+			}
+			assert.Contains(t, names, "nico_rest_api_requests_total")
+			assert.Contains(t, names, "nico_rest_api_request_duration_seconds")
 		})
 	}
 }

rest-api/cert-manager/pkg/certs/server.go

@@ -54,7 +54,7 @@
 	appService.AddHealthRoute(ctx)
 	appService.AddVersionRoute(ctx)
 	appService.AddMetricsRoute(ctx)
-	appService.Use(core.NewHTTPMiddleware(ctx, core.WithRequestMetrics("cloud_cert_manager"))...)
+	appService.Use(core.NewHTTPMiddleware(ctx, core.WithRequestMetrics("nico_rest_cert_manager"))...)
 	appService.Path("/v1/pki/ca").Handler(s.PKICACertificateHandler(ctx)).Methods("GET")
 	appService.Path("/v1/pki/ca/pem").Handler(s.PKICACertificateHandler(ctx)).Methods("GET")
 	appService.Path("/v1/pki/cloud-cert").Handler(s.PKICloudCertificateHandler(ctx)).Methods("POST")
@@ -66,7 +66,7 @@
 	s.insecService = insec
 
 	if o.sentryDSN != "" {
 		sentry.Init(sentry.ClientOptions{
 			Dsn:   o.sentryDSN,
 			Debug: true,
 		})

rest-api/cert-manager/pkg/core/httpservice.go

@@ -308,11 +308,21 @@ type httpMiddleware struct {
 // HTTPMiddlewareOption defines a middleware option
 type HTTPMiddlewareOption func(*httpMiddleware)
 
+// MetricsNamespaceEnv overrides the prefix a caller passes to WithRequestMetrics.
+// These services read no config file, so an environment variable is the only
+// override available to an operator.
+const MetricsNamespaceEnv = "METRICS_NAMESPACE"
+
 // WithRequestMetrics enables service specific metrics regarding
 // request duration and count. By default these metrics are disabled.
+// serverName is the prefix applied to the metric names, and METRICS_NAMESPACE
+// takes precedence over it when set.
 func WithRequestMetrics(serverName string) HTTPMiddlewareOption {
 	return func(h *httpMiddleware) {
 		h.latencyMetricsName = serverName
+		if namespace := os.Getenv(MetricsNamespaceEnv); namespace != "" {
+			h.latencyMetricsName = namespace
+		}
 	}
 }
 

rest-api/cert-manager/pkg/core/httpservice_test.go

@@ -88,7 +88,7 @@
 				return
 			}
 			w.WriteHeader(http.StatusOK)
 			w.Write(body)
 		}).Methods("POST")
 
 		body, code := run("POST", "/echo", "123")
@@ -112,7 +112,7 @@
 				case <-time.After(d):
 					log.Printf("SlowAPIcall done after %v", d)
 					w.WriteHeader(http.StatusOK)
 					w.Write([]byte("ok\n"))
 				}
 			}
 		}
@@ -147,7 +147,7 @@
 		s := NewHTTPService("unix://" + socketPath)
 		s.AddHealthRoute(ctx)
 		s.Use(NewHTTPMiddleware(ctx)...)
 		s.Start(ctx)
 
 		c := &http.Client{
 			Timeout: 10 * time.Second,
@@ -184,6 +184,45 @@
 	}
 }
 
+func TestWithRequestMetrics(t *testing.T) {
+	tests := []struct {
+		name       string
+		serverName string
+		envValue   string
+		want       string
+	}{
+		{
+			name:       "uses the supplied prefix when the environment is unset",
+			serverName: "nico_rest_cert_manager",
+			want:       "nico_rest_cert_manager",
+		},
+		{
+			name:       "METRICS_NAMESPACE takes precedence",
+			serverName: "nico_rest_cert_manager",
+			envValue:   "acme_cert_manager",
+			want:       "acme_cert_manager",
+		},
+		{
+			// An empty value is indistinguishable from unset, and an empty prefix
+			// would expose bare names like "http_duration_seconds".
+			name:       "an empty METRICS_NAMESPACE leaves the supplied prefix alone",
+			serverName: "nico_rest_cert_manager",
+			envValue:   "",
+			want:       "nico_rest_cert_manager",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Setenv(MetricsNamespaceEnv, tt.envValue)
+
+			h := &httpMiddleware{}
+			WithRequestMetrics(tt.serverName)(h)
+
+			assert.Equal(t, tt.want, h.latencyMetricsName)
+		})
+	}
+}
+
 func Test_telemetryMiddleware(t *testing.T) {
 	otel.SetTracerProvider(sdktrace.NewTracerProvider())