From 8659fa65ecde8bb54eae78ac30212ce6fb7fa9da Mon Sep 17 00:00:00 2001 From: James McDermott Date: Thu, 28 May 2026 18:03:28 +0100 Subject: [PATCH 01/21] Feat: Add OpenTelemetry metrics for package resources size - plus enough Prometheus monitoring stack to make it manually testable - picked from changes in WIP https://github.com/kptdev/porch/pull/561 - new histogram- and gauge-type metrics - available in e.g. Prometheus as: - porch_package_size_bytes_bucket - porch_package_size_bytes_count - porch_package_size_bytes_sum - porch_package_size_bytes_total - recorded in Porch flows that update package revision resources: - create package revision - delete package revision - discover/sync package revisions from a registered repository - delete package revisions on unregistering a repository - direct update of PackageRevisionResources in rpkg push Signed-off-by: James McDermott --- cmd/porch/main.go | 11 +- controllers/main.go | 23 +- .../metrics-resources/prometheus-config.yaml | 34 +++ deployments/metrics/Kptfile | 17 ++ deployments/metrics/grafana-deployment.yaml | 162 +++++++++++ .../metrics/prometheus-deployment.yaml | 120 ++++++++ deployments/porch/2-function-runner.yaml | 5 + deployments/porch/22-function-templates.yaml | 10 + deployments/porch/3-porch-server.yaml | 8 + deployments/porch/9-controllers.yaml | 16 +- func/server/server.go | 9 +- func/wrapper-server/main.go | 10 +- go.mod | 3 +- internal/otel/otel.go | 94 ------ internal/telemetry/metrics.go | 83 ++++++ internal/telemetry/metrics_test.go | 85 ++++++ internal/telemetry/otel.go | 221 ++++++++++++++ internal/{otel => telemetry}/otel_test.go | 132 ++++++--- pkg/cache/dbcache/dbpackage.go | 2 + pkg/cache/dbcache/dbrepository.go | 5 + pkg/cache/dbcache/dbreposync.go | 5 + scripts/deploy-monitoring.sh | 270 ++++++++++++++++++ test/e2e/api/metrics_test.go | 61 ++-- 23 files changed, 1214 insertions(+), 172 deletions(-) create mode 100644 deployments/metrics-resources/prometheus-config.yaml create mode 100644 deployments/metrics/Kptfile create mode 100644 deployments/metrics/grafana-deployment.yaml create mode 100644 deployments/metrics/prometheus-deployment.yaml delete mode 100644 internal/otel/otel.go create mode 100644 internal/telemetry/metrics.go create mode 100644 internal/telemetry/metrics_test.go create mode 100644 internal/telemetry/otel.go rename internal/{otel => telemetry}/otel_test.go (58%) create mode 100755 scripts/deploy-monitoring.sh diff --git a/cmd/porch/main.go b/cmd/porch/main.go index 9addef8f5..664902ad3 100644 --- a/cmd/porch/main.go +++ b/cmd/porch/main.go @@ -16,8 +16,9 @@ package main import ( "os" + "time" - porchotel "github.com/kptdev/porch/internal/otel" + "github.com/kptdev/porch/internal/telemetry" "github.com/kptdev/porch/pkg/cmd/server" genericapiserver "k8s.io/apiserver/pkg/server" "k8s.io/component-base/cli" @@ -34,12 +35,18 @@ func main() { func run() int { log.SetLogger(zap.New(zap.UseDevMode(true))) ctx := genericapiserver.SetupSignalContext() - err := porchotel.SetupOpenTelemetry(ctx) + otelResources, err := telemetry.SetupOpenTelemetry(ctx) if err != nil { genericapiserver.RequestShutdown() klog.Errorf("%v\n", err) return 1 } + defer func() { + if err := otelResources.ShutdownWithTimeout(10 * time.Second); err != nil { + klog.Warningf("failed to gracefully shutdown OpenTelemetry: %v", err) + } + }() + options := server.NewPorchServerOptions(os.Stdout, os.Stderr) cmd := server.NewCommandStartPorchServer(ctx, options) code := cli.Run(cmd) diff --git a/controllers/main.go b/controllers/main.go index c8f956f26..ed1364fa9 100644 --- a/controllers/main.go +++ b/controllers/main.go @@ -25,6 +25,7 @@ import ( "net/http" "os" "strings" + "time" // Import all Kubernetes client auth plugins (e.g. Azure, GCP, OIDC, etc.) // to ensure that exec-entrypoint and run can make use of them. @@ -44,7 +45,6 @@ import ( "github.com/kptdev/porch/controllers/packagevariants/pkg/controllers/packagevariant" "github.com/kptdev/porch/controllers/packagevariantsets/pkg/controllers/packagevariantset" "github.com/kptdev/porch/controllers/repositories/pkg/controllers/repository" - porchotel "github.com/kptdev/porch/internal/otel" "github.com/kptdev/porch/pkg/cache/contentcache" "github.com/kptdev/porch/pkg/controllerrestmapper" "k8s.io/apimachinery/pkg/runtime" @@ -59,6 +59,7 @@ import ( porchv1alpha2 "github.com/kptdev/porch/api/porch/v1alpha2" configapi "github.com/kptdev/porch/api/porchconfig/v1alpha1" porchinternal "github.com/kptdev/porch/internal/api/porchinternal/v1alpha1" + "github.com/kptdev/porch/internal/telemetry" //+kubebuilder:scaffold:imports ) @@ -123,7 +124,18 @@ func run(ctx context.Context) error { return err } - mgr, err := newManager(ctx, scheme) + otel.SetLogger(klog.NewKlogr()) + otelResources, err := telemetry.SetupOpenTelemetry(ctx) + if err != nil { + return fmt.Errorf("error setting up OpenTelemetry: %w", err) + } + defer func() { + if shutdownErr := otelResources.ShutdownWithTimeout(10 * time.Second); shutdownErr != nil { + klog.Warningf("failed to gracefully shutdown OpenTelemetry: %v", shutdownErr) + } + }() + + mgr, err := newManager(scheme) if err != nil { return err } @@ -189,7 +201,7 @@ func initScheme() (*runtime.Scheme, error) { // --- Manager --- -func newManager(ctx context.Context, scheme *runtime.Scheme) (ctrl.Manager, error) { +func newManager(scheme *runtime.Scheme) (ctrl.Manager, error) { config := textlogger.NewConfig( textlogger.Verbosity(4), textlogger.Output(os.Stdout), @@ -202,11 +214,6 @@ func newManager(ctx context.Context, scheme *runtime.Scheme) (ctrl.Manager, erro return otelhttp.NewTransport(rt) } - otel.SetLogger(klog.NewKlogr()) - if err := porchotel.SetupOpenTelemetry(ctx); err != nil { - return nil, fmt.Errorf("error setting up OpenTelemetry: %w", err) - } - mgr, err := ctrl.NewManager(cfg, ctrl.Options{ Scheme: scheme, Metrics: metricsserver.Options{ diff --git a/deployments/metrics-resources/prometheus-config.yaml b/deployments/metrics-resources/prometheus-config.yaml new file mode 100644 index 000000000..f2d59d96c --- /dev/null +++ b/deployments/metrics-resources/prometheus-config.yaml @@ -0,0 +1,34 @@ +global: + scrape_native_histograms: true + scrape_interval: 10s + evaluation_interval: 10s + external_labels: + cluster: 'porch' + +scrape_configs: + - job_name: 'porch-server' + scrape_native_histograms: true + scrape_interval: 10s + scrape_timeout: 10s + static_configs: + - targets: ['api.porch-system.svc.cluster.local:9464'] + labels: + service: 'porch' + + - job_name: 'porch-controllers' + scrape_native_histograms: true + scrape_interval: 10s + scrape_timeout: 10s + static_configs: + - targets: ['porch-controllers.porch-system.svc.cluster.local:9464'] + labels: + service: 'porch' + + - job_name: 'function-runner' + scrape_interval: 10s + scrape_timeout: 10s + static_configs: + - targets: ['function-runner.porch-system.svc.cluster.local:9464'] + labels: + service: 'function-runner' + component: 'grpc-server' \ No newline at end of file diff --git a/deployments/metrics/Kptfile b/deployments/metrics/Kptfile new file mode 100644 index 000000000..9abda44ca --- /dev/null +++ b/deployments/metrics/Kptfile @@ -0,0 +1,17 @@ +apiVersion: kpt.dev/v1 +kind: Kptfile +metadata: + name: porch-monitoring + annotations: + config.kubernetes.io/local-config: "true" +info: + description: Prometheus monitoring stack for Porch +pipeline: + mutators: + - image: apply-setters:v0.2.0 + configMap: + prometheus-image: "docker.io/prom/prometheus:latest" + prometheus-nodeport: "30091" + - image: set-namespace:v0.4.1 + configMap: + namespace: porch-monitoring diff --git a/deployments/metrics/grafana-deployment.yaml b/deployments/metrics/grafana-deployment.yaml new file mode 100644 index 000000000..d944a172f --- /dev/null +++ b/deployments/metrics/grafana-deployment.yaml @@ -0,0 +1,162 @@ +# Copyright 2026 The Nephio Authors +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +apiVersion: apps/v1 +kind: Deployment +metadata: + name: grafana + namespace: porch-monitoring # kpt-set: ${namespace} + labels: + app: grafana +spec: + replicas: 1 + selector: + matchLabels: + app: grafana + template: + metadata: + labels: + app: grafana + spec: + containers: + - name: grafana + image: docker.io/grafana/grafana:latest # kpt-set: ${grafana-image} + ports: + - containerPort: 3000 + name: http + env: + - name: GF_AUTH_ANONYMOUS_ENABLED + value: "true" + - name: GF_AUTH_ANONYMOUS_ORG_ROLE + value: "Admin" + - name: GF_AUTH_DISABLE_LOGIN_FORM + value: "true" + - name: GF_USERS_ALLOW_SIGN_UP + value: "false" + - name: GF_DASHBOARDS_DEFAULT_HOME_DASHBOARD_PATH + value: "/var/lib/grafana/dashboards/grafana-porch-server-dashboard.json" + - name: GF_DASHBOARDS_MIN_REFRESH_INTERVAL + value: "1s" + - name: GF_SERVER_ENABLE_GZIP + value: "true" + - name: GF_DATABASE_WAL + value: "true" + livenessProbe: + httpGet: + path: /api/health + port: 3000 + initialDelaySeconds: 30 + periodSeconds: 10 + timeoutSeconds: 5 + failureThreshold: 6 + readinessProbe: + httpGet: + path: /api/health + port: 3000 + initialDelaySeconds: 10 + periodSeconds: 10 + timeoutSeconds: 5 + failureThreshold: 3 + volumeMounts: + - name: grafana-storage + mountPath: /var/lib/grafana + - name: grafana-datasources + mountPath: /etc/grafana/provisioning/datasources + - name: grafana-dashboards-provider + mountPath: /etc/grafana/provisioning/dashboards + - name: grafana-dashboards + mountPath: /var/lib/grafana/dashboards + resources: + requests: + memory: "256Mi" + cpu: "250m" + limits: + memory: "1Gi" + cpu: "1" + volumes: + - name: grafana-storage + emptyDir: {} + - name: grafana-datasources + configMap: + name: grafana-datasources + - name: grafana-dashboards-provider + configMap: + name: grafana-dashboards-provider + - name: grafana-dashboards + configMap: + name: grafana-dashboards +--- +apiVersion: v1 +kind: ConfigMap +metadata: + name: grafana-dashboards-provider + namespace: porch-monitoring # kpt-set: ${namespace} +data: + dashboards.yaml: | + apiVersion: 1 + providers: + - name: 'default' + orgId: 1 + folder: '' + type: file + disableDeletion: false + updateIntervalSeconds: 10 + allowUiUpdates: true + options: + path: /var/lib/grafana/dashboards +--- +apiVersion: v1 +kind: ConfigMap +metadata: + name: grafana-datasources + namespace: porch-monitoring # kpt-set: ${namespace} +data: + prometheus.yaml: | + apiVersion: 1 + datasources: + - name: Prometheus + type: prometheus + access: proxy + uid: prometheus + url: http://prometheus:9090 + isDefault: true + editable: true + pyroscope.yaml: | + apiVersion: 1 + datasources: + - name: Pyroscope + type: phlare + access: proxy + uid: pyroscope + url: http://pyroscope:4040 + editable: true + jsonData: + backendType: pyroscope +--- +apiVersion: v1 +kind: Service +metadata: + name: grafana + namespace: porch-monitoring # kpt-set: ${namespace} + labels: + app: grafana +spec: + type: NodePort + ports: + - port: 3000 + targetPort: 3000 + nodePort: 30301 # kpt-set: ${grafana-nodeport} + name: http + selector: + app: grafana diff --git a/deployments/metrics/prometheus-deployment.yaml b/deployments/metrics/prometheus-deployment.yaml new file mode 100644 index 000000000..d9c318348 --- /dev/null +++ b/deployments/metrics/prometheus-deployment.yaml @@ -0,0 +1,120 @@ +# Copyright 2026 The Nephio Authors +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +apiVersion: apps/v1 +kind: Deployment +metadata: + name: prometheus + namespace: porch-monitoring # kpt-set: ${namespace} + labels: + app: prometheus +spec: + replicas: 1 + selector: + matchLabels: + app: prometheus + template: + metadata: + labels: + app: prometheus + spec: + hostNetwork: true + dnsPolicy: ClusterFirstWithHostNet + serviceAccountName: prometheus + containers: + - name: prometheus + image: docker.io/prom/prometheus:latest # kpt-set: ${prometheus-image} + args: + - '--config.file=/etc/prometheus/prometheus-config.yaml' + - '--storage.tsdb.path=/prometheus' + - '--web.console.libraries=/usr/share/prometheus/console_libraries' + - '--web.console.templates=/usr/share/prometheus/consoles' + - '--storage.tsdb.retention.time=6h' + - '--query.timeout=1m' + ports: + - containerPort: 9090 + name: http + volumeMounts: + - name: prometheus-config + mountPath: /etc/prometheus + - name: prometheus-storage + mountPath: /prometheus + resources: + requests: + memory: "512Mi" + cpu: "500m" + limits: + memory: "2Gi" + cpu: "2000m" + volumes: + - name: prometheus-config + configMap: + name: prometheus-config + - name: prometheus-storage + emptyDir: {} +--- +apiVersion: v1 +kind: ServiceAccount +metadata: + name: prometheus + namespace: porch-monitoring # kpt-set: ${namespace} +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRole +metadata: + name: prometheus +rules: + - apiGroups: [""] + resources: + - nodes + - nodes/proxy + - services + - endpoints + - pods + verbs: ["get", "list", "watch"] + - apiGroups: + - extensions + resources: + - ingresses + verbs: ["get", "list", "watch"] +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRoleBinding +metadata: + name: prometheus +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: ClusterRole + name: prometheus +subjects: + - kind: ServiceAccount + name: prometheus + namespace: porch-monitoring # kpt-set: ${namespace} +--- +apiVersion: v1 +kind: Service +metadata: + name: prometheus + namespace: porch-monitoring # kpt-set: ${namespace} + labels: + app: prometheus +spec: + type: NodePort + ports: + - port: 9090 + targetPort: 9090 + nodePort: 30091 # kpt-set: ${prometheus-nodeport} + name: http + selector: + app: prometheus diff --git a/deployments/porch/2-function-runner.yaml b/deployments/porch/2-function-runner.yaml index 08f34f5b7..8a0210766 100644 --- a/deployments/porch/2-function-runner.yaml +++ b/deployments/porch/2-function-runner.yaml @@ -113,3 +113,8 @@ spec: - port: 9445 protocol: TCP targetPort: 9445 + name: grpc + - port: 9464 + protocol: TCP + targetPort: 9464 + name: metrics diff --git a/deployments/porch/22-function-templates.yaml b/deployments/porch/22-function-templates.yaml index 79692a9f8..7798baafa 100644 --- a/deployments/porch/22-function-templates.yaml +++ b/deployments/porch/22-function-templates.yaml @@ -46,6 +46,11 @@ template: value: none - name: OTEL_EXPORTER_PROMETHEUS_HOST value: 0.0.0.0 + - name: OTEL_EXPORTER_PROMETHEUS_PORT + value: "9464" # Default value, showing for visibility + ports: + - containerPort: 9464 + name: metrics readinessProbe: exec: command: [ "/wrapper-server-tools/grpc-health-probe", "-addr", "localhost:9446" ] @@ -71,6 +76,11 @@ template: - port: 9446 protocol: TCP targetPort: 9446 + name: server + - port: 9464 + protocol: TCP + targetPort: 9464 + name: metrics selector: fn.kpt.dev/image: to-be-replaced type: ClusterIP diff --git a/deployments/porch/3-porch-server.yaml b/deployments/porch/3-porch-server.yaml index 5afffe094..87c975b73 100644 --- a/deployments/porch/3-porch-server.yaml +++ b/deployments/porch/3-porch-server.yaml @@ -95,6 +95,10 @@ spec: - --max-request-body-size=6291456 # Keep this in sync with function-runner's corresponding argument - --cache-type=db - --disable-admission-plugins=MutatingAdmissionPolicy # This can be enabled once kindest/node 1.36.1 is released + ports: + - containerPort: 9464 + name: metrics + protocol: TCP startupProbe: httpGet: path: /healthz @@ -138,5 +142,9 @@ spec: protocol: TCP targetPort: 8443 name: webhooks + - port: 9464 + protocol: TCP + targetPort: 9464 + name: metrics selector: app: porch-server diff --git a/deployments/porch/9-controllers.yaml b/deployments/porch/9-controllers.yaml index 1dc93b308..c6dfb1662 100644 --- a/deployments/porch/9-controllers.yaml +++ b/deployments/porch/9-controllers.yaml @@ -112,4 +112,18 @@ spec: scheme: HTTP periodSeconds: 10 successThreshold: 1 - timeoutSeconds: 5 + timeoutSeconds: 5 +--- +apiVersion: v1 +kind: Service +metadata: + name: porch-controllers + namespace: porch-system +spec: + ports: + - port: 9464 + protocol: TCP + targetPort: 9464 + name: metrics + selector: + k8s-app: porch-controllers diff --git a/func/server/server.go b/func/server/server.go index 416dd1a05..294c61df2 100644 --- a/func/server/server.go +++ b/func/server/server.go @@ -30,7 +30,7 @@ import ( pb "github.com/kptdev/porch/func/evaluator" "github.com/kptdev/porch/func/healthchecker" "github.com/kptdev/porch/func/internal" - porchotel "github.com/kptdev/porch/internal/otel" + "github.com/kptdev/porch/internal/telemetry" "go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc" "google.golang.org/grpc" "google.golang.org/grpc/health/grpc_health_v1" @@ -120,12 +120,17 @@ func run(o *options) error { lis.Close() }() - err = porchotel.SetupOpenTelemetry(ctx) + otelResources, err := telemetry.SetupOpenTelemetry(ctx) if err != nil { contextsignal.RequestShutdown() klog.Errorf("%v\n", err) return err } + defer func() { + if err := otelResources.ShutdownWithTimeout(10 * time.Second); err != nil { + klog.Warningf("failed to gracefully shutdown OpenTelemetry: %v", err) + } + }() availableRuntimes := map[string]struct{}{ execRuntime: {}, diff --git a/func/wrapper-server/main.go b/func/wrapper-server/main.go index 0030af3c2..c5ed5fca2 100644 --- a/func/wrapper-server/main.go +++ b/func/wrapper-server/main.go @@ -25,11 +25,12 @@ import ( "os" "os/exec" "strconv" + "time" "github.com/kptdev/krm-functions-sdk/go/fn" pb "github.com/kptdev/porch/func/evaluator" "github.com/kptdev/porch/func/healthchecker" - porchotel "github.com/kptdev/porch/internal/otel" + "github.com/kptdev/porch/internal/telemetry" "github.com/spf13/cobra" "go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc" "go.opentelemetry.io/otel" @@ -79,12 +80,17 @@ type options struct { func (o *options) run() error { ctx := contextsignal.SetupSignalContext() - err := porchotel.SetupOpenTelemetry(ctx) + otelResources, err := telemetry.SetupOpenTelemetry(ctx) if err != nil { contextsignal.RequestShutdown() klog.Errorf("%v\n", err) return err } + defer func() { + if err := otelResources.ShutdownWithTimeout(10 * time.Second); err != nil { + klog.Warningf("failed to gracefully shutdown OpenTelemetry: %v", err) + } + }() klog.Info("OpenTelemetry initialized") address := fmt.Sprintf(":%d", o.port) lis, err := net.Listen("tcp", address) diff --git a/go.mod b/go.mod index ce3205600..57e2b3fe6 100644 --- a/go.mod +++ b/go.mod @@ -37,6 +37,8 @@ require ( go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp v0.65.0 go.opentelemetry.io/contrib/propagators/autoprop v0.63.0 go.opentelemetry.io/otel v1.43.0 + go.opentelemetry.io/otel/exporters/prometheus v0.65.0 + go.opentelemetry.io/otel/metric v1.43.0 go.opentelemetry.io/otel/sdk v1.43.0 go.opentelemetry.io/otel/sdk/metric v1.43.0 go.opentelemetry.io/otel/trace v1.43.0 @@ -109,7 +111,6 @@ require ( go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetrichttp v1.43.0 // indirect go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc v1.43.0 // indirect go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp v1.43.0 // indirect - go.opentelemetry.io/otel/exporters/prometheus v0.65.0 // indirect go.opentelemetry.io/otel/exporters/stdout/stdoutlog v0.19.0 // indirect go.opentelemetry.io/otel/exporters/stdout/stdoutmetric v1.43.0 // indirect go.opentelemetry.io/otel/exporters/stdout/stdouttrace v1.43.0 // indirect diff --git a/internal/otel/otel.go b/internal/otel/otel.go deleted file mode 100644 index 7ef667e10..000000000 --- a/internal/otel/otel.go +++ /dev/null @@ -1,94 +0,0 @@ -// Copyright 2026 The kpt Authors -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package porch - -import ( - "context" - "fmt" - "net/http" - "time" - - "go.opentelemetry.io/contrib/bridges/prometheus" - "go.opentelemetry.io/contrib/exporters/autoexport" - "go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp" - "go.opentelemetry.io/contrib/propagators/autoprop" - "go.opentelemetry.io/otel" - "go.opentelemetry.io/otel/sdk/metric" - "go.opentelemetry.io/otel/sdk/trace" - "k8s.io/klog/v2" - controllerruntimemetrics "sigs.k8s.io/controller-runtime/pkg/metrics" -) - -// Sets up OpenTelemetry with parameters -// from environment variables based on the -// opentelemetry.io/contrib/exporters/autoexport" -func SetupOpenTelemetry(ctx context.Context) error { - setupTiming := time.Now() - err := setupTracing(ctx) - if err != nil { - return err - } - err = setupMetrics(ctx) - if err != nil { - return err - } - http.DefaultTransport = otelhttp.NewTransport(http.DefaultTransport) - http.DefaultClient.Transport = http.DefaultTransport - klog.Infof("OpenTelemetry initialized in %s", time.Since(setupTiming)) - return nil - -} - -func setupTracing(ctx context.Context) error { - exp, err := autoexport.NewSpanExporter(ctx) - if err != nil { - return fmt.Errorf("failed to create span exporter: %w", err) - } - tp := trace.NewTracerProvider(trace.WithBatcher(exp)) - go func() { - <-ctx.Done() - if err := tp.Shutdown(context.Background()); err != nil { - panic(err) - } - }() - otel.SetTracerProvider(tp) - otel.SetTextMapPropagator(autoprop.NewTextMapPropagator()) - - return nil -} - -func setupMetrics(ctx context.Context) error { - autoexport.WithFallbackMetricProducer(func(ctx context.Context) (metric.Producer, error) { - return prometheus.NewMetricProducer( - prometheus.WithGatherer(controllerruntimemetrics.Registry), - ), nil - }) - - mr, err := autoexport.NewMetricReader(ctx) - if err != nil { - return fmt.Errorf("failed to create metric reader: %w", err) - } - go func() { - <-ctx.Done() - if err := mr.Shutdown(context.Background()); err != nil { - panic(err) - } - }() - - mp := metric.NewMeterProvider(metric.WithReader(mr)) - otel.SetMeterProvider(mp) - - return nil -} diff --git a/internal/telemetry/metrics.go b/internal/telemetry/metrics.go new file mode 100644 index 000000000..745e4716e --- /dev/null +++ b/internal/telemetry/metrics.go @@ -0,0 +1,83 @@ +// Copyright 2026 The kpt and Nephio Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package telemetry + +import ( + "context" + "fmt" + + "github.com/kptdev/porch/pkg/repository" + "go.opentelemetry.io/otel" + "go.opentelemetry.io/otel/attribute" + "go.opentelemetry.io/otel/metric" + "k8s.io/klog/v2" +) + +const meterName = "github.com/nephio-project/porch" + +var ( + prrResourceSizeHistogram metric.Float64Histogram + prrResourceSizeGauge metric.Int64Gauge +) + +func InitMetrics() { + m := otel.Meter(meterName) + var err error + + prrResourceSizeHistogram, err = m.Float64Histogram( + "porch_package_size_bytes", + metric.WithDescription("File size, in bytes, of a package revision's resources"), + metric.WithUnit("By"), + metric.WithExplicitBucketBoundaries(0, 1024, 2048, 4096, 8192, 16384, 32768, 65536, 131072, 262144, 524288, 1048576, 2097152, 4194304, 8388608, 16777216, 33554432, 67108864, 134217728, 268435456, 536870912, 1073741824), + ) + if err != nil { + panic(fmt.Sprintf("failed to create porch_package_size_bytes histogram: %v", err)) + } + + prrResourceSizeGauge, err = m.Int64Gauge( + "porch_package_size_bytes_total", + metric.WithDescription("Total file size, in bytes, of a package revision's resources"), + ) + if err != nil { + panic(fmt.Sprintf("failed to create porch_package_size_bytes gauge: %v", err)) + } + +} + +// Porch server and function runner metric recording functions +func RecordPackageSizeUpdate(pkgRev repository.PackageRevision, newResourcesSize int64) { + if prrResourceSizeHistogram == nil { + klog.Warning("prrResourceSizeHistogram is nil") + return + } + klog.Infof("Recording package resources size %dB for package %q", newResourcesSize, pkgRev.Key().RKey().Name+"/"+pkgRev.Key().PKey().Path+"/"+pkgRev.Key().PKey().Package) + prrResourceSizeHistogram.Record(context.Background(), float64(newResourcesSize), + metric.WithAttributes( + attribute.String("namespace", pkgRev.KubeObjectNamespace()), + attribute.String("repository", pkgRev.Key().RKey().Name), + attribute.String("package", pkgRev.Key().PKey().Path+"/"+pkgRev.Key().PKey().Package), + ), + ) + + if prrResourceSizeGauge == nil { + klog.Warning("prrResourceSizeGauge is nil") + return + } + prrResourceSizeGauge.Record(context.Background(), newResourcesSize, + metric.WithAttributes( + attribute.String("namespace", pkgRev.KubeObjectNamespace()), + attribute.String("repository", pkgRev.Key().RKey().Name), + attribute.String("package", pkgRev.Key().PKey().Path+"/"+pkgRev.Key().PKey().Package))) +} diff --git a/internal/telemetry/metrics_test.go b/internal/telemetry/metrics_test.go new file mode 100644 index 000000000..6476837a0 --- /dev/null +++ b/internal/telemetry/metrics_test.go @@ -0,0 +1,85 @@ +package telemetry + +import ( + "context" + "testing" + + "github.com/kptdev/porch/pkg/repository" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.opentelemetry.io/otel" + sdkmetric "go.opentelemetry.io/otel/sdk/metric" + "go.opentelemetry.io/otel/sdk/metric/metricdata" +) + +type fakePackageRevision struct { + repository.PackageRevision + key repository.PackageRevisionKey + namespace string +} + +func (f *fakePackageRevision) KubeObjectNamespace() string { return f.namespace } +func (f *fakePackageRevision) Key() repository.PackageRevisionKey { return f.key } + +// Remaining interface methods are not called by RecordPackageSizeUpdate, +// so they can panic if invoked unexpectedly. + +func TestRecordPackageSizeUpdate_NilInstruments(t *testing.T) { + InitMetrics() + histogramBefore := prrResourceSizeHistogram + prrResourceSizeHistogram = nil + defer func() { prrResourceSizeHistogram = histogramBefore }() + + fake := &fakePackageRevision{ + namespace: "ns", + key: repository.PackageRevisionKey{ + WorkspaceName: "ws", + Revision: 1, + }, + } + // Should return early without panic + assert.NotPanics(t, func() { RecordPackageSizeUpdate(fake, 1024) }) + + prrResourceSizeHistogram = histogramBefore + gaugeBefore := prrResourceSizeGauge + prrResourceSizeGauge = nil + defer func() { prrResourceSizeGauge = gaugeBefore }() + // Should return early without panic + assert.NotPanics(t, func() { RecordPackageSizeUpdate(fake, 1024) }) +} + +func TestRecordPackageSizeUpdate_RecordsMetrics(t *testing.T) { + reader := sdkmetric.NewManualReader() + mp := sdkmetric.NewMeterProvider(sdkmetric.WithReader(reader)) + otel.SetMeterProvider(mp) + defer mp.Shutdown(context.Background()) + + InitMetrics() + + fake := &fakePackageRevision{ + namespace: "test-ns", + key: repository.PackageRevisionKey{ + WorkspaceName: "ws", + Revision: 1, + }, + } + + RecordPackageSizeUpdate(fake, 4096) + + var rm metricdata.ResourceMetrics + require.NoError(t, reader.Collect(context.Background(), &rm)) + + var foundHistogram, foundGauge bool + for _, sm := range rm.ScopeMetrics { + for _, m := range sm.Metrics { + if m.Name == "porch_package_size_bytes" { + foundHistogram = true + } + if m.Name == "porch_package_size_bytes_total" { + foundGauge = true + } + } + } + assert.True(t, foundHistogram, "expected porch_package_size_bytes histogram to be recorded") + assert.True(t, foundGauge, "expected porch_package_size_bytes_total gauge to be recorded") +} diff --git a/internal/telemetry/otel.go b/internal/telemetry/otel.go new file mode 100644 index 000000000..6c2b7df3d --- /dev/null +++ b/internal/telemetry/otel.go @@ -0,0 +1,221 @@ +// Copyright 2026 The kpt Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package telemetry + +import ( + "context" + "fmt" + "net/http" + "os" + "strconv" + "time" + + prombridge "go.opentelemetry.io/contrib/bridges/prometheus" + "go.opentelemetry.io/contrib/exporters/autoexport" + "go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp" + "go.opentelemetry.io/contrib/propagators/autoprop" + "go.opentelemetry.io/otel" + otelprometheus "go.opentelemetry.io/otel/exporters/prometheus" + sdkmetric "go.opentelemetry.io/otel/sdk/metric" + "go.opentelemetry.io/otel/sdk/trace" + + "github.com/prometheus/client_golang/prometheus" + "github.com/prometheus/client_golang/prometheus/promhttp" + "k8s.io/klog/v2" + controllerruntimemetrics "sigs.k8s.io/controller-runtime/pkg/metrics" +) + +const ( + otelPortEnv = "OTEL_EXPORTER_PROMETHEUS_PORT" +) + +// OTelResources holds all OpenTelemetry resources that need lifecycle management. +// Use Shutdown() to cleanly release all resources. +type OTelResources struct { + metricsServer *http.Server + metricsPort int + meterProvider *sdkmetric.MeterProvider + tracerProvider *trace.TracerProvider + metricReader sdkmetric.Reader +} + +// Shutdown gracefully shuts down all OpenTelemetry resources. +func (r *OTelResources) Shutdown(ctx context.Context) error { + shutdownTiming := time.Now() + var errs []error + if r.metricsServer != nil { + if err := r.metricsServer.Shutdown(ctx); err != nil { + errs = append(errs, fmt.Errorf("metrics server shutdown: %w", err)) + } + } + if r.metricReader != nil { + if err := r.metricReader.Shutdown(ctx); err != nil { + errs = append(errs, fmt.Errorf("metric reader shutdown: %w", err)) + } + } + if r.meterProvider != nil { + if err := r.meterProvider.Shutdown(ctx); err != nil { + errs = append(errs, fmt.Errorf("meter provider shutdown: %w", err)) + } + } + if r.tracerProvider != nil { + if err := r.tracerProvider.Shutdown(ctx); err != nil { + errs = append(errs, fmt.Errorf("tracer provider shutdown: %w", err)) + } + } + if len(errs) > 0 { + return fmt.Errorf("otel shutdown errors: %v", errs) + } + klog.Infof("OpenTelemetry shut down in %s", time.Since(shutdownTiming)) + return nil +} + +// ShutdownWithTimeout is a convenience wrapper around Shutdown with a timeout. +func (r *OTelResources) ShutdownWithTimeout(timeout time.Duration) error { + ctx, cancel := context.WithTimeout(context.Background(), timeout) + defer cancel() + return r.Shutdown(ctx) +} + +// Flush forces a flush of the meter provider, useful in tests. +func (r *OTelResources) Flush() error { + if r.meterProvider != nil { + return r.meterProvider.ForceFlush(context.Background()) + } + return nil +} + +// SetupOpenTelemetry is the single entry point for all OpenTelemetry setup. +// It configures tracing, metrics (including the Prometheus HTTP server if +// OTEL_EXPORTER_PROMETHEUS_PORT is set), and initializes all Porch metric +// instruments. Returns OTelResources for lifecycle management. +func SetupOpenTelemetry(ctx context.Context) (*OTelResources, error) { + setupTiming := time.Now() + res := &OTelResources{} + + // Setup tracing + if err := setupTracing(ctx, res); err != nil { + return nil, err + } + + // Setup metrics provider + if err := setupMetrics(ctx, res); err != nil { + return nil, err + } + + // Initialize all Porch metric instruments + InitMetrics() + + // Start the Prometheus metrics HTTP server if port is configured + if err := startMetricsServerIfConfigured(res); err != nil { + return nil, err + } + + http.DefaultTransport = otelhttp.NewTransport(http.DefaultTransport) + http.DefaultClient.Transport = http.DefaultTransport + klog.Infof("OpenTelemetry initialized in %s", time.Since(setupTiming)) + return res, nil +} + +func setupTracing(ctx context.Context, res *OTelResources) error { + exp, err := autoexport.NewSpanExporter(ctx) + if err != nil { + return fmt.Errorf("failed to create span exporter: %w", err) + } + tp := trace.NewTracerProvider(trace.WithBatcher(exp)) + res.tracerProvider = tp + otel.SetTracerProvider(tp) + otel.SetTextMapPropagator(autoprop.NewTextMapPropagator()) + return nil +} + +func setupMetrics(ctx context.Context, res *OTelResources) error { + exporter := os.Getenv("OTEL_METRICS_EXPORTER") + + autoexport.WithFallbackMetricProducer(func(ctx context.Context) (sdkmetric.Producer, error) { + return prombridge.NewMetricProducer( + prombridge.WithGatherer(prometheus.Gatherers{ + prometheus.DefaultGatherer, + controllerruntimemetrics.Registry, + }), + ), nil + }) + + promExp, err := otelprometheus.New( + otelprometheus.WithRegisterer(prometheus.DefaultRegisterer), + ) + if err != nil { + return fmt.Errorf("failed to create prometheus exporter: %w", err) + } + + readers := []sdkmetric.Option{sdkmetric.WithReader(promExp)} + + if exporter != "prometheus" { + autoMr, err := autoexport.NewMetricReader(ctx) + if err != nil { + return fmt.Errorf("failed to create metric reader: %w", err) + } + res.metricReader = autoMr + readers = append(readers, sdkmetric.WithReader(autoMr)) + } + + mp := sdkmetric.NewMeterProvider(readers...) + res.meterProvider = mp + otel.SetMeterProvider(mp) + + return nil +} + +func startMetricsServerIfConfigured(res *OTelResources) error { + portStr := os.Getenv(otelPortEnv) + if portStr == "" { + return nil + } + port, err := strconv.Atoi(portStr) + if err != nil { + return fmt.Errorf("invalid %s value %q: %w", otelPortEnv, portStr, err) + } + if port <= 0 { + return nil + } + + gatherers := prometheus.Gatherers{ + prometheus.DefaultGatherer, + controllerruntimemetrics.Registry, + } + handler := promhttp.HandlerFor(gatherers, promhttp.HandlerOpts{ + ErrorHandling: promhttp.ContinueOnError, + }) + + mux := http.NewServeMux() + mux.Handle("/metrics", handler) + + srv := &http.Server{ + Addr: fmt.Sprintf(":%d", port), + Handler: mux, + ReadHeaderTimeout: 10 * time.Second, + } + res.metricsServer = srv + res.metricsPort = port + + go func() { + if err := srv.ListenAndServe(); err != nil && err != http.ErrServerClosed { + klog.Errorf("OTel metrics server error: %v", err) + } + }() + klog.Infof("OTel metrics server started on port %d", port) + + return nil +} diff --git a/internal/otel/otel_test.go b/internal/telemetry/otel_test.go similarity index 58% rename from internal/otel/otel_test.go rename to internal/telemetry/otel_test.go index 37dbad60e..4ec4cdc80 100644 --- a/internal/otel/otel_test.go +++ b/internal/telemetry/otel_test.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -package porch +package telemetry import ( "context" @@ -22,6 +22,7 @@ import ( "net/http" "net/http/httptest" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -33,24 +34,77 @@ import ( otlptraces "go.opentelemetry.io/proto/otlp/collector/trace/v1" ) +const ( + ENV_OTEL_METRICS_EXPORTER = "OTEL_METRICS_EXPORTER" + METRICS_EXPORTER_PROMETHEUS = "prometheus" + METRICS_EXPORTER_OTLP = "otlp" + + ENV_OTEL_TRACES_EXPORTER = "OTEL_TRACES_EXPORTER" + DEFAULT_OTEL_TRACES_EXPORTER = "none" + + ENV_OTEL_EXPORTER_PROMETHEUS_PORT = "OTEL_EXPORTER_PROMETHEUS_PORT" + ENV_OTEL_EXPORTER_OTLP_ENDPOINT = "OTEL_EXPORTER_OTLP_ENDPOINT" + ENV_OTEL_EXPORTER_OTLP_PROTOCOL = "OTEL_EXPORTER_OTLP_PROTOCOL" +) + +func TestPrometheusHTTPServer(t *testing.T) { + // Find a free port + lis, err := net.Listen("tcp", ":0") + require.NoError(t, err) + port := lis.Addr().(*net.TCPAddr).Port + lis.Close() + + t.Setenv(ENV_OTEL_METRICS_EXPORTER, METRICS_EXPORTER_PROMETHEUS) + t.Setenv(ENV_OTEL_TRACES_EXPORTER, DEFAULT_OTEL_TRACES_EXPORTER) + t.Setenv(ENV_OTEL_EXPORTER_PROMETHEUS_PORT, fmt.Sprintf("%d", port)) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + res, err := SetupOpenTelemetry(ctx) + require.NoError(t, err) + defer res.ShutdownWithTimeout(5 * time.Second) + + // Verify the HTTP server is serving metrics + resp, err := http.Get(fmt.Sprintf("http://localhost:%d/metrics", port)) + require.NoError(t, err) + defer resp.Body.Close() + + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + assert.Contains(t, string(body), "target_info") +} + +func TestPrometheusHTTPServerInvalidPort(t *testing.T) { + t.Setenv(ENV_OTEL_METRICS_EXPORTER, METRICS_EXPORTER_PROMETHEUS) + t.Setenv(ENV_OTEL_TRACES_EXPORTER, DEFAULT_OTEL_TRACES_EXPORTER) + t.Setenv(ENV_OTEL_EXPORTER_PROMETHEUS_PORT, "not-a-number") + + ctx := context.Background() + _, err := SetupOpenTelemetry(ctx) + require.Error(t, err) + assert.Contains(t, err.Error(), "invalid") +} + func TestOtelMetricsPushHTTP(t *testing.T) { requestWaitChannel := make(chan struct{}) ts := httptest.NewServer(&mockHTTPMetricsServer{t: t, ch: requestWaitChannel}) defer ts.Close() - t.Setenv("OTEL_METRICS_EXPORTER", "otlp") - t.Setenv("OTEL_TRACES_EXPORTER", "none") - t.Setenv("OTEL_EXPORTER_OTLP_ENDPOINT", ts.URL) - t.Setenv("OTEL_EXPORTER_OTLP_PROTOCOL", "http/protobuf") + t.Setenv(ENV_OTEL_METRICS_EXPORTER, METRICS_EXPORTER_OTLP) + t.Setenv(ENV_OTEL_TRACES_EXPORTER, DEFAULT_OTEL_TRACES_EXPORTER) + t.Setenv(ENV_OTEL_EXPORTER_OTLP_ENDPOINT, ts.URL) + t.Setenv(ENV_OTEL_EXPORTER_OTLP_PROTOCOL, "http/protobuf") ctx, cancel := context.WithCancel(context.Background()) defer cancel() - err := SetupOpenTelemetry(ctx) + res, err := SetupOpenTelemetry(ctx) require.NoError(t, err) - cancel() + // Shutdown flushes the periodic reader, which triggers the export + res.ShutdownWithTimeout(5 * time.Second) <-requestWaitChannel } @@ -60,15 +114,15 @@ func TestOtelTracesPushHTTP(t *testing.T) { ts := httptest.NewServer(&mockHTTPTraceServer{t: t, ch: requestWaitChannel}) defer ts.Close() - t.Setenv("OTEL_TRACES_EXPORTER", "otlp") - t.Setenv("OTEL_METRICS_EXPORTER", "none") - t.Setenv("OTEL_EXPORTER_OTLP_ENDPOINT", ts.URL) - t.Setenv("OTEL_EXPORTER_OTLP_PROTOCOL", "http/protobuf") + t.Setenv(ENV_OTEL_TRACES_EXPORTER, METRICS_EXPORTER_OTLP) + t.Setenv(ENV_OTEL_METRICS_EXPORTER, DEFAULT_OTEL_TRACES_EXPORTER) + t.Setenv(ENV_OTEL_EXPORTER_OTLP_ENDPOINT, ts.URL) + t.Setenv(ENV_OTEL_EXPORTER_OTLP_PROTOCOL, "http/protobuf") ctx, cancel := context.WithCancel(context.Background()) defer cancel() - err := SetupOpenTelemetry(ctx) + res, err := SetupOpenTelemetry(ctx) require.NoError(t, err) // Create a span to trigger trace export @@ -76,36 +130,26 @@ func TestOtelTracesPushHTTP(t *testing.T) { _, span := tracer.Start(ctx, "test-span") span.End() + // Shutdown flushes the batch span processor + res.ShutdownWithTimeout(5 * time.Second) <-requestWaitChannel } func TestSetupOpenTelemetryPrometheusEndpoint(t *testing.T) { - // Find available port - listener, err := net.Listen("tcp", ":0") - require.NoError(t, err) - port := listener.Addr().(*net.TCPAddr).Port - listener.Close() - - t.Setenv("OTEL_METRICS_EXPORTER", "prometheus") - t.Setenv("OTEL_EXPORTER_PROMETHEUS_HOST", "localhost") - t.Setenv("OTEL_EXPORTER_PROMETHEUS_PORT", fmt.Sprintf("%d", port)) + t.Setenv(ENV_OTEL_METRICS_EXPORTER, METRICS_EXPORTER_PROMETHEUS) + t.Setenv(ENV_OTEL_TRACES_EXPORTER, DEFAULT_OTEL_TRACES_EXPORTER) ctx, cancel := context.WithCancel(context.Background()) defer cancel() - err = SetupOpenTelemetry(ctx) - require.NoError(t, err) - - // Make request to the Prometheus metrics endpoint - resp, err := http.Get(fmt.Sprintf("http://localhost:%d/metrics", port)) + res, err := SetupOpenTelemetry(ctx) require.NoError(t, err) - defer resp.Body.Close() + defer res.ShutdownWithTimeout(5 * time.Second) - body, err := io.ReadAll(resp.Body) + // Verify that metrics are accessible via the OTel meter provider + meter := otel.Meter("test") + counter, err := meter.Float64Counter("test_counter") require.NoError(t, err) - - metricsText := string(body) - // Verify at least one metric line exists (non-comment, non-empty) - assert.Regexp(t, `(?m)^([a-zA-Z_][a-zA-Z0-9_]*)`, metricsText) + counter.Add(ctx, 1) } func TestOtelMetricsPushGRPC(t *testing.T) { @@ -125,18 +169,18 @@ func TestOtelMetricsPushGRPC(t *testing.T) { }() defer s.Stop() - t.Setenv("OTEL_METRICS_EXPORTER", "otlp") - t.Setenv("OTEL_TRACES_EXPORTER", "none") - t.Setenv("OTEL_EXPORTER_OTLP_ENDPOINT", fmt.Sprintf("http://localhost:%d", lis.Addr().(*net.TCPAddr).Port)) - t.Setenv("OTEL_EXPORTER_OTLP_PROTOCOL", "grpc") + t.Setenv(ENV_OTEL_METRICS_EXPORTER, METRICS_EXPORTER_OTLP) + t.Setenv(ENV_OTEL_TRACES_EXPORTER, DEFAULT_OTEL_TRACES_EXPORTER) + t.Setenv(ENV_OTEL_EXPORTER_OTLP_ENDPOINT, fmt.Sprintf("http://localhost:%d", lis.Addr().(*net.TCPAddr).Port)) + t.Setenv(ENV_OTEL_EXPORTER_OTLP_PROTOCOL, "grpc") ctx, cancel := context.WithCancel(context.Background()) defer cancel() - err = SetupOpenTelemetry(ctx) + res, err := SetupOpenTelemetry(ctx) require.NoError(t, err) - cancel() + res.ShutdownWithTimeout(5 * time.Second) <-requestWaitChannel } @@ -157,15 +201,15 @@ func TestOtelTracesPushGRPC(t *testing.T) { }() defer s.Stop() - t.Setenv("OTEL_TRACES_EXPORTER", "otlp") - t.Setenv("OTEL_METRICS_EXPORTER", "none") - t.Setenv("OTEL_EXPORTER_OTLP_ENDPOINT", fmt.Sprintf("http://localhost:%d", lis.Addr().(*net.TCPAddr).Port)) - t.Setenv("OTEL_EXPORTER_OTLP_PROTOCOL", "grpc") + t.Setenv(ENV_OTEL_TRACES_EXPORTER, METRICS_EXPORTER_OTLP) + t.Setenv(ENV_OTEL_METRICS_EXPORTER, "none") + t.Setenv(ENV_OTEL_EXPORTER_OTLP_ENDPOINT, fmt.Sprintf("http://localhost:%d", lis.Addr().(*net.TCPAddr).Port)) + t.Setenv(ENV_OTEL_EXPORTER_OTLP_PROTOCOL, "grpc") ctx, cancel := context.WithCancel(context.Background()) defer cancel() - err = SetupOpenTelemetry(ctx) + res, err := SetupOpenTelemetry(ctx) require.NoError(t, err) // Create a span to trigger trace export @@ -173,7 +217,7 @@ func TestOtelTracesPushGRPC(t *testing.T) { _, span := tracer.Start(ctx, "test-span") span.End() - cancel() + res.ShutdownWithTimeout(5 * time.Second) <-requestWaitChannel } diff --git a/pkg/cache/dbcache/dbpackage.go b/pkg/cache/dbcache/dbpackage.go index 28afa20e1..33d368e79 100644 --- a/pkg/cache/dbcache/dbpackage.go +++ b/pkg/cache/dbcache/dbpackage.go @@ -20,6 +20,7 @@ import ( "time" porchapi "github.com/kptdev/porch/api/porch/v1alpha1" + "github.com/kptdev/porch/internal/telemetry" "github.com/kptdev/porch/pkg/repository" "github.com/kptdev/porch/pkg/util" "go.opentelemetry.io/otel/trace" @@ -134,6 +135,7 @@ func (p *dbPackage) DeletePackageRevision(ctx context.Context, old repository.Pa if len(prSlice) == 0 { return pkgDeleteFromDB(ctx, p.Key()) } + telemetry.RecordPackageSizeUpdate(dbPR, 0) if dbPR.IsLatestRevision() { klog.Infof("dbPackage %+v: latest PackageRevision deleted. Sending notification.", p.Key()) diff --git a/pkg/cache/dbcache/dbrepository.go b/pkg/cache/dbcache/dbrepository.go index d4fb35514..927088178 100644 --- a/pkg/cache/dbcache/dbrepository.go +++ b/pkg/cache/dbcache/dbrepository.go @@ -25,6 +25,7 @@ import ( kptfilev1 "github.com/kptdev/kpt/pkg/api/kptfile/v1" porchapi "github.com/kptdev/porch/api/porch/v1alpha1" configapi "github.com/kptdev/porch/api/porchconfig/v1alpha1" + "github.com/kptdev/porch/internal/telemetry" cachetypes "github.com/kptdev/porch/pkg/cache/types" "github.com/kptdev/porch/pkg/engine" "github.com/kptdev/porch/pkg/externalrepo" @@ -345,6 +346,8 @@ func (r *dbRepository) DeletePackageRevision(ctx context.Context, pr2Delete repo r.deleteCachedGitPR(pr2Delete.Key().PkgKey, pr2Delete.Key().WorkspaceName) } + telemetry.RecordPackageSizeUpdate(pr2Delete, 0) + foundPRs, err := pkgRevReadPRsFromDB(ctx, foundPkg.Key()) if err != nil { return err @@ -486,6 +489,8 @@ func (r *dbRepository) ClosePackageRevisionDraft(ctx context.Context, prd reposi return nil, err } + telemetry.RecordPackageSizeUpdate(pr, pr.resourcesSizeBytes) + if r.pushDraftsToGit && pr.gitPRDraft != nil && r.externalRepo != nil { gitPR, err := r.externalRepo.ClosePackageRevisionDraft(ctx, pr.gitPRDraft, 0) if err != nil { diff --git a/pkg/cache/dbcache/dbreposync.go b/pkg/cache/dbcache/dbreposync.go index cd768b45c..332e75414 100644 --- a/pkg/cache/dbcache/dbreposync.go +++ b/pkg/cache/dbcache/dbreposync.go @@ -23,6 +23,7 @@ import ( "unicode/utf8" porchapi "github.com/kptdev/porch/api/porch/v1alpha1" + "github.com/kptdev/porch/internal/telemetry" cachetypes "github.com/kptdev/porch/pkg/cache/types" "github.com/kptdev/porch/pkg/repository" pkgerrors "github.com/pkg/errors" @@ -242,6 +243,8 @@ func (s *repositorySync) cacheExternalPRs(ctx context.Context, externalPrMap map klog.Errorf("repositorySync %+v: failed to save external package revision %+v to database", s.repo.Key(), extPRKey) return err } + + telemetry.RecordPackageSizeUpdate(&dbPR, dbPR.resourcesSizeBytes) } return nil @@ -267,6 +270,8 @@ func (s *repositorySync) deletePRsOnlyInCache(ctx context.Context, cachedPrMap m klog.Errorf("repositorySync %+v: failed to delete cached PR %+v not in external repo", s.repo.Key(), dbPRKey) return err } + + telemetry.RecordPackageSizeUpdate(dbPR, 0) } return nil } diff --git a/scripts/deploy-monitoring.sh b/scripts/deploy-monitoring.sh new file mode 100755 index 000000000..5e7a72f16 --- /dev/null +++ b/scripts/deploy-monitoring.sh @@ -0,0 +1,270 @@ +#!/usr/bin/env bash +# Copyright 2024-2025 The Nephio Authors +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +set -e +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +METRICS_DIR="${SCRIPT_DIR}/../deployments/metrics" +DOT_ENV_PATH="${SCRIPT_DIR}/../.env" + +if [ -f "$DOT_ENV_PATH" ]; then + export $(grep -v '^#' "$DOT_ENV_PATH" | xargs) +fi + +# Configuration +NAMESPACE="${NAMESPACE:-porch-monitoring}" +PROMETHEUS_NODEPORT="${PROMETHEUS_NODEPORT:-30091}" +GRAFANA_NODEPORT="${GRAFANA_NODEPORT:-30301}" + +DOCKERHUB_MIRROR="${DOCKERHUB_MIRROR:-docker.io}" +KRM_FN_REGISTRY_URL="${KRM_FN_REGISTRY_URL:-gcr.io/kptdev/krm-functions-catalog}" +PROMETHEUS_IMAGE="${DOCKERHUB_MIRROR}/prom/prometheus:latest" +GRAFANA_IMAGE="${DOCKERHUB_MIRROR}/grafana/grafana:latest" + +RED='\033[0;31m' +GREEN='\033[0;32m' +YELLOW='\033[1;33m' +NC='\033[0m' + +log_info() { + echo -e "${GREEN}[INFO]${NC} $1" +} +log_warn() { + echo -e "${YELLOW}[WARN]${NC} $1" +} +log_error() { + echo -e "${RED}[ERROR]${NC} $1" +} + +if ! command -v kubectl &> /dev/null; then + log_error "kubectl not found. Please install kubectl first." + exit 1 +fi + +check_kpt() { + if ! command -v kpt &> /dev/null; then + log_error "kpt not found. Please install kpt from: https://kpt.dev/installation/" + exit 1 + fi +} + +prepare_manifests() { + local temp_dir=$(mktemp -d) + cp -r "${METRICS_DIR}"/* "$temp_dir/" + + cat > "$temp_dir/Kptfile" < /dev/null 2>&1 + + echo "$temp_dir" +} + +apply_manifests() { + local manifests_dir=$1 + + log_info "Applying manifests using kpt live apply..." + if [ ! -f "$manifests_dir/resourcegroup.yaml" ]; then + log_info "Initializing kpt inventory..." + kpt live init "$manifests_dir" --namespace "$NAMESPACE" --name porch-monitoring + fi + + log_info "Running kpt live apply..." + kpt live apply "$manifests_dir" --reconcile-timeout=2m --output=events || { + log_warn "kpt live apply reconcile timeout - resources are deployed but may still be starting up" + } +} + +create_namespace() { + if kubectl get namespace "$NAMESPACE" &> /dev/null; then + log_info "Namespace $NAMESPACE already exists" + else + log_info "Creating namespace $NAMESPACE" + kubectl create namespace "$NAMESPACE" + fi +} + +deploy_monitoring() { + log_info "Deploying monitoring stack..." + log_info "Rendering manifests with kpt..." + + local manifests_dir + manifests_dir=$(prepare_manifests) + + kubectl create configmap prometheus-config \ + --from-file="${SCRIPT_DIR}/../deployments/metrics-resources/prometheus-config.yaml" \ + -n "$NAMESPACE" \ + --dry-run=client -o yaml | kubectl apply -f - + + + declare -a grafana_dashboards + while read -r dashboard_file; do + grafana_dashboards+=("--from-file=$(basename "$dashboard_file")=$dashboard_file") + done < <(find "${SCRIPT_DIR}/../deployments/metrics-resources" -name "grafana*dashboard.json" -type f) + + kubectl create configmap grafana-dashboards \ + "${grafana_dashboards[@]}" \ + -n "$NAMESPACE" \ + --dry-run=client -o yaml | kubectl apply -f - + + apply_manifests "$manifests_dir" + + rm -rf "$manifests_dir" + + log_info "Monitoring stack deployed successfully" +} + +wait_for_deployment() { + local deployment=$1 + log_info "Waiting for $deployment to be ready..." + kubectl wait --for=condition=available --timeout=300s deployment/$deployment -n "$NAMESPACE" +} + +get_service_urls() { + log_info "Getting service URLs..." + log_info "Setting up port forwarding..." + + pkill -f "port-forward.*prometheus" 2>/dev/null || true + pkill -f "port-forward.*grafana" 2>/dev/null || true + sleep 2 + + kubectl port-forward -n "${NAMESPACE}" svc/prometheus 9092:9090 > /dev/null 2>&1 & + PROMETHEUS_PF_PID=$! + kubectl port-forward -n "${NAMESPACE}" svc/grafana 3001:3000 > /dev/null 2>&1 & + GRAFANA_PF_PID=$! + + sleep 2 + + echo "${PROMETHEUS_PF_PID}" > /tmp/porch-prometheus-pf.pid + echo "${GRAFANA_PF_PID}" > /tmp/porch-grafana-pf.pid + + PROMETHEUS_URL="http://localhost:9092" + GRAFANA_URL="http://localhost:3001" + PROMETHEUS_NODEPORT_URL="http://localhost:${PROMETHEUS_NODEPORT}" + GRAFANA_NODEPORT_URL="http://localhost:${GRAFANA_NODEPORT}" + + echo "" + log_info "==========================================" + log_info "Services deployed successfully!" + log_info "==========================================" + echo "" + log_info "Access via port-forward (recommended):" + log_info " Prometheus: ${PROMETHEUS_URL}" + log_info " Grafana: ${GRAFANA_URL}" + log_info " Username: admin" + log_info " Password: admin" + echo "" + log_info "Or access via NodePort:" + log_info " Prometheus: ${PROMETHEUS_NODEPORT_URL}" + log_info " Grafana: ${GRAFANA_NODEPORT_URL}" + echo "" + log_info " - Prometheus UI on http://localhost:${PROMETHEUS_PORT}" + log_info " - porch-server metrics on port 9093" + log_info " - function-runner metrics on port 9094" + log_info "" + log_info "Note: Performance tests expose metrics on port 9095" + log_info " (typically at 172.17.0.1:9095 for scraping from within cluster)" + log_info " For kind clusters, tests scrape from host.docker.internal:9095" + log_info " (resolves to host machine from container)" + echo "" + log_info "To stop port forwarding:" + log_info ' kill $(cat /tmp/porch-prometheus-pf.pid /tmp/porch-grafana-pf.pid 2>/dev/null)' + echo "" +} + +cleanup() { + log_warn "Cleaning up existing deployment..." + + if [ -f /tmp/porch-prometheus-pf.pid ] || [ -f /tmp/porch-grafana-pf.pid ] ; then + log_info "Stopping port forwarding..." + kill $(cat /tmp/porch-prometheus-pf.pid /tmp/porch-grafana-pf.pid 2>/dev/null) 2>/dev/null || true + rm -f /tmp/porch-prometheus-pf.pid /tmp/porch-grafana-pf.pid + fi + + if kubectl get namespace "$NAMESPACE" &> /dev/null; then + log_info "Deleting resources in namespace $NAMESPACE..." + kubectl delete deployment prometheus grafana -n "$NAMESPACE" --ignore-not-found=true + kubectl delete service prometheus grafana -n "$NAMESPACE" --ignore-not-found=true + kubectl delete configmap prometheus-config grafana-dashboards grafana-dashboards-provider grafana-datasources -n "$NAMESPACE" --ignore-not-found=true + kubectl delete serviceaccount prometheus -n "$NAMESPACE" --ignore-not-found=true + kubectl delete clusterrole prometheus -n "$NAMESPACE" --ignore-not-found=true + kubectl delete clusterrolebinding prometheus -n "$NAMESPACE" --ignore-not-found=true + + log_info "Deleting namespace $NAMESPACE..." + kubectl delete namespace "$NAMESPACE" --ignore-not-found=true + else + log_info "Namespace $NAMESPACE does not exist, nothing to clean up" + fi + + log_info "Cleanup completed" +} + +main() { + local action="${1:-deploy}" + case "$action" in + deploy) + log_info "Starting deployment of Prometheus and Grafana..." + check_kpt + create_namespace + deploy_monitoring + wait_for_deployment prometheus + wait_for_deployment grafana + get_service_urls + ;; + cleanup) + check_kpt + cleanup + ;; + restart) + check_kpt + cleanup + sleep 2 + main deploy + ;; + *) + log_error "Unknown action: $action" + echo "Usage: $0 {deploy|cleanup|restart}" + echo "" + echo "Environment variables:" + echo " NAMESPACE - Kubernetes namespace (default: porch-monitoring)" + echo " PROMETHEUS_NODEPORT - Prometheus NodePort (default: 30091)" + echo " GRAFANA_NODEPORT - Grafana NodePort (default: 30301)" + echo "" + echo "Requirements:" + echo " - kpt CLI (install from: https://kpt.dev/installation/)" + echo " - kubectl configured with cluster access" + exit 1 + ;; + esac +} +main "$@" diff --git a/test/e2e/api/metrics_test.go b/test/e2e/api/metrics_test.go index d28009aa3..bf5db72f0 100644 --- a/test/e2e/api/metrics_test.go +++ b/test/e2e/api/metrics_test.go @@ -16,34 +16,33 @@ package api import ( suiteutils "github.com/kptdev/porch/test/e2e/suiteutils" - "github.com/stretchr/testify/assert" ) func (t *PorchSuite) TestMetricsEndpoint() { porchServerShouldHaveRegexList := []string{ - "go_*", - "http_server_*", - "http_client_*", + "go_.*", + "http_server_.*", + "http_client_.*", "errors_total*", "target_info*", - "promhttp_metric_handler_*", + "promhttp_metric_handler_.*", } porchControllerShouldHaveRegexList := []string{ - "controller_*", - "go_*", + "controller_.*", + "go_.*", } porchFunctionRunnerShouldHaveRegexList := []string{ - "go_*", - "rpc_server_*", - // "rpc_client_*", //There is no way to force both function runners to have at least one connection, so no metrics + "go_.*", + "rpc_server_.*", + // "rpc_client_.*", //There is no way to force both function runners to have at least one connection, so no metrics } porchWrapperServerShouldHaveRegexList := []string{ - "go_*", - "rpc_server_*", + "go_.*", + "rpc_server_.*", } - //This is needed to ensure that there is at least one wrapper-server instance. - + // Create a package revision and update it with a mutator. + // This is needed to trigger a render and ensure that there is at least one wrapper-server instance. resources := t.setupFunctionTestPackage("git-fn-distroless", "test-fn-redis-bucket", "test-description", TestPackageSetupOptions{ UpstreamRef: "redis-bucket/v1", UpstreamDir: "redis-bucket", @@ -67,17 +66,43 @@ data: } for _, regex := range porchServerShouldHaveRegexList { - t.Regexp(regex, collectionResults.PorchServerMetrics, "porch server metrics should contain %q", regex) + t.Assert().Regexp(regex, collectionResults.PorchServerMetrics, "porch server metrics should contain %q", regex) } for _, regex := range porchControllerShouldHaveRegexList { - assert.Regexp(t.T(), regex, collectionResults.PorchControllerMetrics, "porch controller metrics should contain %q", regex) + t.Assert().Regexp(regex, collectionResults.PorchControllerMetrics, "porch controller metrics should contain %q", regex) } for _, regex := range porchFunctionRunnerShouldHaveRegexList { - assert.Regexp(t.T(), regex, collectionResults.PorchFunctionRunnerMetrics, "porch function runner metrics should contain %q", regex) + t.Assert().Regexp(regex, collectionResults.PorchFunctionRunnerMetrics, "porch function runner metrics should contain %q", regex) } for _, regex := range porchWrapperServerShouldHaveRegexList { - assert.Regexp(t.T(), regex, collectionResults.PorchWrapperServerMetrics, "porch wrapper server metrics should contain %q", regex) + t.Assert().Regexp(regex, collectionResults.PorchWrapperServerMetrics, "porch wrapper server metrics should contain %q", regex) + } +} + +func (t *PorchSuite) TestPackageSizeMetric() { + expectedMetrics := []string{ + `porch_package_size_bytes_bucket`, + `porch_package_size_bytes_count`, + `porch_package_size_bytes_sum`, + `porch_package_size_bytes_total`, + } + + // Create a new package revision to ensure metric creation in porch-server + t.setupFunctionTestPackage("git-fn-distroless", "test-fn-redis-bucket", "test-description", TestPackageSetupOptions{ + UpstreamRef: "redis-bucket/v1", + UpstreamDir: "redis-bucket", + }) + + // Sync some package revisions to ensure metric creation in porch-controllers + t.RegisterGitRepositoryF(t.GetTestBlueprintsRepoURL(), suiteutils.TestBlueprintsRepoName, "", suiteutils.GiteaUser, suiteutils.GiteaPassword) + + collectionResults, err := t.CollectMetricsFromPods() + t.Require().NoError(err, "failed to collect metrics from pods:") + + for _, metricName := range expectedMetrics { + t.Assert().Regexp(metricName, collectionResults.PorchServerMetrics, "porch server metrics should contain %q", metricName) + t.Assert().Regexp(metricName, collectionResults.PorchControllerMetrics, "porch controller metrics should contain %q", metricName) } } From ffd34db948f27e78319b92ea29f20d44a08fe140 Mon Sep 17 00:00:00 2001 From: James McDermott Date: Tue, 2 Jun 2026 13:21:11 +0100 Subject: [PATCH 02/21] Update docs to mention new package resources size metrics Signed-off-by: James McDermott --- .../configurations/opentelemetry.md | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/docs/content/en/docs/6_configuration_and_deployments/configurations/opentelemetry.md b/docs/content/en/docs/6_configuration_and_deployments/configurations/opentelemetry.md index 8b10fe3d6..d98b49fff 100644 --- a/docs/content/en/docs/6_configuration_and_deployments/configurations/opentelemetry.md +++ b/docs/content/en/docs/6_configuration_and_deployments/configurations/opentelemetry.md @@ -410,6 +410,42 @@ env: This allows routing different telemetry signals to specialized backends. + +## Available Metrics + +Porch records the following metrics via OpenTelemetry: + +### Package Size Metrics + +| Metric Name | Type | Unit | Description | +|----------------------------------|-----------|-------|-------------| +| `porch_package_size_bytes` | Histogram | Bytes | File size of a package's resources expressed as a histogram | +| `porch_package_size_bytes_total` | Gauge | Bytes | Total file size of a package's resources | + +Package size metrics are recorded with the following attributes from the relevant package: + +| Attribute | Description | +|--------------|-------------| +| `namespace` | Kubernetes namespace of the package revision | +| `repository` | Name of the repository containing the package | +| `package` | Path and name of the package | + +These metrics are recorded as part of every flow that updates package revision resources: +- Create package revision +- Delete package revision +- Discover/sync package revisions from a registered repository +- Delete package revisions on unregistering a repository +- Direct update of PackageRevisionResources (e.g. `rpkg push`) + +**Prometheus metric names:** + +When using the Prometheus exporter, these are made available under the metric names: +- `porch_package_size_bytes_bucket` +- `porch_package_size_bytes_count` +- `porch_package_size_bytes_sum` +- `porch_package_size_bytes_total` + + ## Troubleshooting ### Verify Metrics Endpoint From 1f53d13dc01cc94897236171eddded666927f994 Mon Sep 17 00:00:00 2001 From: James McDermott Date: Fri, 5 Jun 2026 10:36:28 +0100 Subject: [PATCH 03/21] Address Copilot review comments Signed-off-by: James McDermott --- cmd/porch/main.go | 2 +- .../metrics-resources/prometheus-config.yaml | 14 ++ deployments/metrics/Kptfile | 17 --- deployments/metrics/grafana-deployment.yaml | 9 +- .../metrics/prometheus-deployment.yaml | 9 +- func/server/server.go | 2 +- func/wrapper-server/main.go | 2 +- go.mod | 3 +- internal/telemetry/metrics.go | 71 ++++++---- internal/telemetry/metrics_test.go | 52 ++++--- internal/telemetry/otel.go | 4 +- pkg/cache/dbcache/dbpackage.go | 2 - .../dbcache/dbpackagerevisionresourcessql.go | 5 +- pkg/cache/dbcache/dbrepository.go | 4 +- pkg/cache/dbcache/dbreposync.go | 6 +- scripts/deploy-monitoring.sh | 34 ++--- test/e2e/api/metrics_test.go | 129 +++++++++++++++++- test/e2e/suiteutils/suite.go | 5 +- test/e2e/suiteutils/suite_utils.go | 74 +++++++++- 19 files changed, 328 insertions(+), 116 deletions(-) delete mode 100644 deployments/metrics/Kptfile diff --git a/cmd/porch/main.go b/cmd/porch/main.go index 664902ad3..f1705a38e 100644 --- a/cmd/porch/main.go +++ b/cmd/porch/main.go @@ -1,4 +1,4 @@ -// Copyright 2022 The kpt Authors +// Copyright 2022, 2026 The kpt Authors // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. diff --git a/deployments/metrics-resources/prometheus-config.yaml b/deployments/metrics-resources/prometheus-config.yaml index f2d59d96c..0ef2d3f9e 100644 --- a/deployments/metrics-resources/prometheus-config.yaml +++ b/deployments/metrics-resources/prometheus-config.yaml @@ -1,3 +1,17 @@ +# Copyright 2026 The kpt Authors +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + global: scrape_native_histograms: true scrape_interval: 10s diff --git a/deployments/metrics/Kptfile b/deployments/metrics/Kptfile deleted file mode 100644 index 9abda44ca..000000000 --- a/deployments/metrics/Kptfile +++ /dev/null @@ -1,17 +0,0 @@ -apiVersion: kpt.dev/v1 -kind: Kptfile -metadata: - name: porch-monitoring - annotations: - config.kubernetes.io/local-config: "true" -info: - description: Prometheus monitoring stack for Porch -pipeline: - mutators: - - image: apply-setters:v0.2.0 - configMap: - prometheus-image: "docker.io/prom/prometheus:latest" - prometheus-nodeport: "30091" - - image: set-namespace:v0.4.1 - configMap: - namespace: porch-monitoring diff --git a/deployments/metrics/grafana-deployment.yaml b/deployments/metrics/grafana-deployment.yaml index d944a172f..814e23bed 100644 --- a/deployments/metrics/grafana-deployment.yaml +++ b/deployments/metrics/grafana-deployment.yaml @@ -1,4 +1,4 @@ -# Copyright 2026 The Nephio Authors +# Copyright 2026 The kpt Authors # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. @@ -11,7 +11,6 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. - apiVersion: apps/v1 kind: Deployment metadata: @@ -33,7 +32,7 @@ spec: - name: grafana image: docker.io/grafana/grafana:latest # kpt-set: ${grafana-image} ports: - - containerPort: 3000 + - containerPort: 3000 # kpt-set: ${grafana-container-port} name: http env: - name: GF_AUTH_ANONYMOUS_ENABLED @@ -154,8 +153,8 @@ metadata: spec: type: NodePort ports: - - port: 3000 - targetPort: 3000 + - port: 3000 # kpt-set: ${grafana-container-port} + targetPort: 3000 # kpt-set: ${grafana-container-port} nodePort: 30301 # kpt-set: ${grafana-nodeport} name: http selector: diff --git a/deployments/metrics/prometheus-deployment.yaml b/deployments/metrics/prometheus-deployment.yaml index d9c318348..fe89648b6 100644 --- a/deployments/metrics/prometheus-deployment.yaml +++ b/deployments/metrics/prometheus-deployment.yaml @@ -1,4 +1,4 @@ -# Copyright 2026 The Nephio Authors +# Copyright 2026 The kpt Authors # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. @@ -11,7 +11,6 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. - apiVersion: apps/v1 kind: Deployment metadata: @@ -43,7 +42,7 @@ spec: - '--storage.tsdb.retention.time=6h' - '--query.timeout=1m' ports: - - containerPort: 9090 + - containerPort: 9090 # kpt-set: ${prometheus-container-port} name: http volumeMounts: - name: prometheus-config @@ -112,8 +111,8 @@ metadata: spec: type: NodePort ports: - - port: 9090 - targetPort: 9090 + - port: 9090 # kpt-set: ${prometheus-container-port} + targetPort: 9090 # kpt-set: ${prometheus-container-port} nodePort: 30091 # kpt-set: ${prometheus-nodeport} name: http selector: diff --git a/func/server/server.go b/func/server/server.go index 294c61df2..d4fb60a2c 100644 --- a/func/server/server.go +++ b/func/server/server.go @@ -1,4 +1,4 @@ -// Copyright 2022-2025 The kpt Authors +// Copyright 2022-2026 The kpt Authors // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. diff --git a/func/wrapper-server/main.go b/func/wrapper-server/main.go index c5ed5fca2..a2cb2b093 100644 --- a/func/wrapper-server/main.go +++ b/func/wrapper-server/main.go @@ -1,4 +1,4 @@ -// Copyright 2022 The kpt Authors +// Copyright 2022, 2026 The kpt Authors // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. diff --git a/go.mod b/go.mod index 57e2b3fe6..827cef32e 100644 --- a/go.mod +++ b/go.mod @@ -66,7 +66,6 @@ require ( sigs.k8s.io/cli-utils v0.37.2 sigs.k8s.io/controller-runtime v0.24.1 sigs.k8s.io/kustomize/kyaml v0.21.1 - sigs.k8s.io/structured-merge-diff/v6 v6.3.2 sigs.k8s.io/yaml v1.6.0 ) @@ -225,7 +224,6 @@ require ( go.mongodb.org/mongo-driver v1.17.6 // indirect go.opentelemetry.io/auto/sdk v1.2.1 // indirect go.opentelemetry.io/otel/exporters/otlp/otlptrace v1.43.0 // indirect - go.opentelemetry.io/otel/metric v1.43.0 // indirect go.uber.org/zap v1.27.1 // indirect go.yaml.in/yaml/v2 v2.4.4 // indirect go.yaml.in/yaml/v3 v3.0.4 // indirect @@ -249,4 +247,5 @@ require ( sigs.k8s.io/apiserver-network-proxy/konnectivity-client v0.34.0 // indirect sigs.k8s.io/json v0.0.0-20250730193827-2d320260d730 // indirect sigs.k8s.io/randfill v1.0.0 // indirect + sigs.k8s.io/structured-merge-diff/v6 v6.3.2 // indirect ) diff --git a/internal/telemetry/metrics.go b/internal/telemetry/metrics.go index 745e4716e..44a83a898 100644 --- a/internal/telemetry/metrics.go +++ b/internal/telemetry/metrics.go @@ -1,4 +1,4 @@ -// Copyright 2026 The kpt and Nephio Authors +// Copyright 2026 The kpt Authors // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -16,7 +16,6 @@ package telemetry import ( "context" - "fmt" "github.com/kptdev/porch/pkg/repository" "go.opentelemetry.io/otel" @@ -25,59 +24,71 @@ import ( "k8s.io/klog/v2" ) -const meterName = "github.com/nephio-project/porch" +const meterName = "github.com/kptdev/porch" var ( - prrResourceSizeHistogram metric.Float64Histogram - prrResourceSizeGauge metric.Int64Gauge + prResourceSizeHistogram metric.Float64Histogram + prResourceSizeGauge metric.Int64Gauge ) -func InitMetrics() { +func InitMetrics() (err error) { m := otel.Meter(meterName) - var err error - prrResourceSizeHistogram, err = m.Float64Histogram( + prResourceSizeHistogram, err = m.Float64Histogram( "porch_package_size_bytes", - metric.WithDescription("File size, in bytes, of a package revision's resources"), metric.WithUnit("By"), + metric.WithDescription("Distribution of package revision resources' file size, in bytes"), metric.WithExplicitBucketBoundaries(0, 1024, 2048, 4096, 8192, 16384, 32768, 65536, 131072, 262144, 524288, 1048576, 2097152, 4194304, 8388608, 16777216, 33554432, 67108864, 134217728, 268435456, 536870912, 1073741824), ) if err != nil { - panic(fmt.Sprintf("failed to create porch_package_size_bytes histogram: %v", err)) + klog.Errorf("failed to create porch_package_size_bytes histogram: %v", err) + return } - prrResourceSizeGauge, err = m.Int64Gauge( + prResourceSizeGauge, err = m.Int64Gauge( "porch_package_size_bytes_total", + metric.WithUnit("By"), metric.WithDescription("Total file size, in bytes, of a package revision's resources"), ) if err != nil { - panic(fmt.Sprintf("failed to create porch_package_size_bytes gauge: %v", err)) + klog.Errorf("failed to create porch_package_size_bytes gauge: %v", err) + return } + return nil } // Porch server and function runner metric recording functions -func RecordPackageSizeUpdate(pkgRev repository.PackageRevision, newResourcesSize int64) { - if prrResourceSizeHistogram == nil { - klog.Warning("prrResourceSizeHistogram is nil") +func RecordPackageRevisionResourcesSize(prKey repository.PackageRevisionKey, resourcesSize int64) { + prPath := func() string { + if prKey.PKey().Path != "" { + return prKey.PKey().Path + "/" + } + return "" + }() + attributes := attribute.NewSet( + attribute.String("namespace", prKey.RKey().Namespace), + attribute.String("repository", prKey.RKey().Name), + attribute.String("package", prPath+prKey.PKey().Package), + attribute.String("workspaceName", prKey.WorkspaceName), + ) + + if prResourceSizeHistogram == nil { + klog.Warning("prResourceSizeHistogram is nil - was InitMetrics() called?") return } - klog.Infof("Recording package resources size %dB for package %q", newResourcesSize, pkgRev.Key().RKey().Name+"/"+pkgRev.Key().PKey().Path+"/"+pkgRev.Key().PKey().Package) - prrResourceSizeHistogram.Record(context.Background(), float64(newResourcesSize), - metric.WithAttributes( - attribute.String("namespace", pkgRev.KubeObjectNamespace()), - attribute.String("repository", pkgRev.Key().RKey().Name), - attribute.String("package", pkgRev.Key().PKey().Path+"/"+pkgRev.Key().PKey().Package), - ), - ) - if prrResourceSizeGauge == nil { - klog.Warning("prrResourceSizeGauge is nil") + if klog.V(3).Enabled() { + klog.Infof( + "Recording package resources size %dB for package revision with attributes %v", + resourcesSize, attributes.MarshalLog()) + } + + prResourceSizeHistogram.Record(context.Background(), float64(resourcesSize), metric.WithAttributeSet(attributes)) + + if prResourceSizeGauge == nil { + klog.Warning("prResourceSizeGauge is nil - was InitMetrics() called?") return } - prrResourceSizeGauge.Record(context.Background(), newResourcesSize, - metric.WithAttributes( - attribute.String("namespace", pkgRev.KubeObjectNamespace()), - attribute.String("repository", pkgRev.Key().RKey().Name), - attribute.String("package", pkgRev.Key().PKey().Path+"/"+pkgRev.Key().PKey().Package))) + prResourceSizeGauge.Record(context.Background(), resourcesSize, metric.WithAttributeSet(attributes)) } diff --git a/internal/telemetry/metrics_test.go b/internal/telemetry/metrics_test.go index 6476837a0..457516ae0 100644 --- a/internal/telemetry/metrics_test.go +++ b/internal/telemetry/metrics_test.go @@ -1,3 +1,17 @@ +// Copyright 2026 The kpt Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + package telemetry import ( @@ -26,26 +40,25 @@ func (f *fakePackageRevision) Key() repository.PackageRevisionKey { return f.key func TestRecordPackageSizeUpdate_NilInstruments(t *testing.T) { InitMetrics() - histogramBefore := prrResourceSizeHistogram - prrResourceSizeHistogram = nil - defer func() { prrResourceSizeHistogram = histogramBefore }() + histogramBefore := prResourceSizeHistogram + prResourceSizeHistogram = nil + defer func() { prResourceSizeHistogram = histogramBefore }() - fake := &fakePackageRevision{ - namespace: "ns", - key: repository.PackageRevisionKey{ + fake := + repository.PackageRevisionKey{ + PkgKey: repository.PackageKey{RepoKey: repository.RepositoryKey{Namespace: "ns"}}, WorkspaceName: "ws", Revision: 1, - }, - } + } // Should return early without panic - assert.NotPanics(t, func() { RecordPackageSizeUpdate(fake, 1024) }) + assert.NotPanics(t, func() { RecordPackageRevisionResourcesSize(fake, 1024) }) - prrResourceSizeHistogram = histogramBefore - gaugeBefore := prrResourceSizeGauge - prrResourceSizeGauge = nil - defer func() { prrResourceSizeGauge = gaugeBefore }() + prResourceSizeHistogram = histogramBefore + gaugeBefore := prResourceSizeGauge + prResourceSizeGauge = nil + defer func() { prResourceSizeGauge = gaugeBefore }() // Should return early without panic - assert.NotPanics(t, func() { RecordPackageSizeUpdate(fake, 1024) }) + assert.NotPanics(t, func() { RecordPackageRevisionResourcesSize(fake, 1024) }) } func TestRecordPackageSizeUpdate_RecordsMetrics(t *testing.T) { @@ -56,15 +69,14 @@ func TestRecordPackageSizeUpdate_RecordsMetrics(t *testing.T) { InitMetrics() - fake := &fakePackageRevision{ - namespace: "test-ns", - key: repository.PackageRevisionKey{ + fake := + repository.PackageRevisionKey{ + PkgKey: repository.PackageKey{RepoKey: repository.RepositoryKey{Namespace: "test-ns"}}, WorkspaceName: "ws", Revision: 1, - }, - } + } - RecordPackageSizeUpdate(fake, 4096) + RecordPackageRevisionResourcesSize(fake, 4096) var rm metricdata.ResourceMetrics require.NoError(t, reader.Collect(context.Background(), &rm)) diff --git a/internal/telemetry/otel.go b/internal/telemetry/otel.go index 6c2b7df3d..e679f9fa4 100644 --- a/internal/telemetry/otel.go +++ b/internal/telemetry/otel.go @@ -116,7 +116,9 @@ func SetupOpenTelemetry(ctx context.Context) (*OTelResources, error) { } // Initialize all Porch metric instruments - InitMetrics() + if err := InitMetrics(); err != nil { + return nil, fmt.Errorf("failed to initialize Porch metrics: %w", err) + } // Start the Prometheus metrics HTTP server if port is configured if err := startMetricsServerIfConfigured(res); err != nil { diff --git a/pkg/cache/dbcache/dbpackage.go b/pkg/cache/dbcache/dbpackage.go index 33d368e79..28afa20e1 100644 --- a/pkg/cache/dbcache/dbpackage.go +++ b/pkg/cache/dbcache/dbpackage.go @@ -20,7 +20,6 @@ import ( "time" porchapi "github.com/kptdev/porch/api/porch/v1alpha1" - "github.com/kptdev/porch/internal/telemetry" "github.com/kptdev/porch/pkg/repository" "github.com/kptdev/porch/pkg/util" "go.opentelemetry.io/otel/trace" @@ -135,7 +134,6 @@ func (p *dbPackage) DeletePackageRevision(ctx context.Context, old repository.Pa if len(prSlice) == 0 { return pkgDeleteFromDB(ctx, p.Key()) } - telemetry.RecordPackageSizeUpdate(dbPR, 0) if dbPR.IsLatestRevision() { klog.Infof("dbPackage %+v: latest PackageRevision deleted. Sending notification.", p.Key()) diff --git a/pkg/cache/dbcache/dbpackagerevisionresourcessql.go b/pkg/cache/dbcache/dbpackagerevisionresourcessql.go index f9b409d2c..958149689 100644 --- a/pkg/cache/dbcache/dbpackagerevisionresourcessql.go +++ b/pkg/cache/dbcache/dbpackagerevisionresourcessql.go @@ -1,4 +1,4 @@ -// Copyright 2024-2025 The kpt Authors +// Copyright 2024-2026 The kpt Authors // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -19,6 +19,7 @@ import ( "database/sql" "fmt" + "github.com/kptdev/porch/internal/telemetry" "github.com/kptdev/porch/pkg/repository" "go.opentelemetry.io/otel/trace" "k8s.io/klog/v2" @@ -163,6 +164,8 @@ func pkgRevResourcesDeleteFromDB(ctx context.Context, prk repository.PackageRevi klog.Warningf("pkgRevResourcesDeleteFromDB: deletion of package revision resources for %+v failed: %q", prk, err) } + telemetry.RecordPackageRevisionResourcesSize(prk, 0) + return err } diff --git a/pkg/cache/dbcache/dbrepository.go b/pkg/cache/dbcache/dbrepository.go index 927088178..32fbb2c98 100644 --- a/pkg/cache/dbcache/dbrepository.go +++ b/pkg/cache/dbcache/dbrepository.go @@ -346,8 +346,6 @@ func (r *dbRepository) DeletePackageRevision(ctx context.Context, pr2Delete repo r.deleteCachedGitPR(pr2Delete.Key().PkgKey, pr2Delete.Key().WorkspaceName) } - telemetry.RecordPackageSizeUpdate(pr2Delete, 0) - foundPRs, err := pkgRevReadPRsFromDB(ctx, foundPkg.Key()) if err != nil { return err @@ -489,7 +487,7 @@ func (r *dbRepository) ClosePackageRevisionDraft(ctx context.Context, prd reposi return nil, err } - telemetry.RecordPackageSizeUpdate(pr, pr.resourcesSizeBytes) + telemetry.RecordPackageRevisionResourcesSize(pr.Key(), pr.resourcesSizeBytes) if r.pushDraftsToGit && pr.gitPRDraft != nil && r.externalRepo != nil { gitPR, err := r.externalRepo.ClosePackageRevisionDraft(ctx, pr.gitPRDraft, 0) diff --git a/pkg/cache/dbcache/dbreposync.go b/pkg/cache/dbcache/dbreposync.go index 332e75414..f514ae806 100644 --- a/pkg/cache/dbcache/dbreposync.go +++ b/pkg/cache/dbcache/dbreposync.go @@ -1,4 +1,4 @@ -// Copyright 2025 The kpt Authors +// Copyright 2025-2026 The kpt Authors // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -244,7 +244,7 @@ func (s *repositorySync) cacheExternalPRs(ctx context.Context, externalPrMap map return err } - telemetry.RecordPackageSizeUpdate(&dbPR, dbPR.resourcesSizeBytes) + telemetry.RecordPackageRevisionResourcesSize(dbPR.Key(), dbPR.resourcesSizeBytes) } return nil @@ -270,8 +270,6 @@ func (s *repositorySync) deletePRsOnlyInCache(ctx context.Context, cachedPrMap m klog.Errorf("repositorySync %+v: failed to delete cached PR %+v not in external repo", s.repo.Key(), dbPRKey) return err } - - telemetry.RecordPackageSizeUpdate(dbPR, 0) } return nil } diff --git a/scripts/deploy-monitoring.sh b/scripts/deploy-monitoring.sh index 5e7a72f16..c6276ba65 100755 --- a/scripts/deploy-monitoring.sh +++ b/scripts/deploy-monitoring.sh @@ -1,5 +1,5 @@ #!/usr/bin/env bash -# Copyright 2024-2025 The Nephio Authors +# Copyright 2026 The kpt Authors # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. @@ -18,17 +18,21 @@ SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" METRICS_DIR="${SCRIPT_DIR}/../deployments/metrics" DOT_ENV_PATH="${SCRIPT_DIR}/../.env" -if [ -f "$DOT_ENV_PATH" ]; then - export $(grep -v '^#' "$DOT_ENV_PATH" | xargs) +if [[ -f "$DOT_ENV_PATH" ]]; then + source "$DOT_ENV_PATH" fi # Configuration NAMESPACE="${NAMESPACE:-porch-monitoring}" +PROMETHEUS_LOCAL_PORT="${PROMETHEUS_LOCAL_PORT:-9092}" +PROMETHEUS_CONTAINER_PORT="${PROMETHEUS_CONTAINER_PORT:-9090}" PROMETHEUS_NODEPORT="${PROMETHEUS_NODEPORT:-30091}" +GRAFANA_LOCAL_PORT="${GRAFANA_LOCAL_PORT:-3001}" +GRAFANA_CONTAINER_PORT="${GRAFANA_CONTAINER_PORT:-3000}" GRAFANA_NODEPORT="${GRAFANA_NODEPORT:-30301}" DOCKERHUB_MIRROR="${DOCKERHUB_MIRROR:-docker.io}" -KRM_FN_REGISTRY_URL="${KRM_FN_REGISTRY_URL:-gcr.io/kptdev/krm-functions-catalog}" +KRM_FN_REGISTRY_URL="${KRM_FN_REGISTRY_URL:-ghcr.io/kptdev/krm-functions-catalog}" PROMETHEUS_IMAGE="${DOCKERHUB_MIRROR}/prom/prometheus:latest" GRAFANA_IMAGE="${DOCKERHUB_MIRROR}/grafana/grafana:latest" @@ -78,6 +82,8 @@ pipeline: configMap: prometheus-image: "${PROMETHEUS_IMAGE}" grafana-image: "${GRAFANA_IMAGE}" + prometheus-container-port: "${PROMETHEUS_CONTAINER_PORT}" + grafana-container-port: "${GRAFANA_CONTAINER_PORT}" prometheus-nodeport: "${PROMETHEUS_NODEPORT}" grafana-nodeport: "${GRAFANA_NODEPORT}" - image: ${KRM_FN_REGISTRY_URL}/set-namespace:v0.4.1 @@ -85,7 +91,7 @@ pipeline: namespace: ${NAMESPACE} EOF - kpt fn render "$temp_dir" > /dev/null 2>&1 + kpt fn render "$temp_dir" #> /dev/null 2>&1 echo "$temp_dir" } @@ -158,9 +164,9 @@ get_service_urls() { pkill -f "port-forward.*grafana" 2>/dev/null || true sleep 2 - kubectl port-forward -n "${NAMESPACE}" svc/prometheus 9092:9090 > /dev/null 2>&1 & + kubectl port-forward -n "${NAMESPACE}" svc/prometheus "$PROMETHEUS_LOCAL_PORT":"$PROMETHEUS_CONTAINER_PORT" > /dev/null 2>&1 & PROMETHEUS_PF_PID=$! - kubectl port-forward -n "${NAMESPACE}" svc/grafana 3001:3000 > /dev/null 2>&1 & + kubectl port-forward -n "${NAMESPACE}" svc/grafana "$GRAFANA_LOCAL_PORT":"$GRAFANA_CONTAINER_PORT" > /dev/null 2>&1 & GRAFANA_PF_PID=$! sleep 2 @@ -168,8 +174,8 @@ get_service_urls() { echo "${PROMETHEUS_PF_PID}" > /tmp/porch-prometheus-pf.pid echo "${GRAFANA_PF_PID}" > /tmp/porch-grafana-pf.pid - PROMETHEUS_URL="http://localhost:9092" - GRAFANA_URL="http://localhost:3001" + PROMETHEUS_URL="http://localhost:$PROMETHEUS_LOCAL_PORT" + GRAFANA_URL="http://localhost:$GRAFANA_LOCAL_PORT" PROMETHEUS_NODEPORT_URL="http://localhost:${PROMETHEUS_NODEPORT}" GRAFANA_NODEPORT_URL="http://localhost:${GRAFANA_NODEPORT}" @@ -188,14 +194,10 @@ get_service_urls() { log_info " Prometheus: ${PROMETHEUS_NODEPORT_URL}" log_info " Grafana: ${GRAFANA_NODEPORT_URL}" echo "" - log_info " - Prometheus UI on http://localhost:${PROMETHEUS_PORT}" - log_info " - porch-server metrics on port 9093" - log_info " - function-runner metrics on port 9094" + log_info " - Prometheus scraping metrics from porch-server port 9464" + log_info " - Prometheus scraping metrics from porch-controller port 9464" + log_info " - Prometheus scraping metrics from function-runner port 9464" log_info "" - log_info "Note: Performance tests expose metrics on port 9095" - log_info " (typically at 172.17.0.1:9095 for scraping from within cluster)" - log_info " For kind clusters, tests scrape from host.docker.internal:9095" - log_info " (resolves to host machine from container)" echo "" log_info "To stop port forwarding:" log_info ' kill $(cat /tmp/porch-prometheus-pf.pid /tmp/porch-grafana-pf.pid 2>/dev/null)' diff --git a/test/e2e/api/metrics_test.go b/test/e2e/api/metrics_test.go index bf5db72f0..6294a36c3 100644 --- a/test/e2e/api/metrics_test.go +++ b/test/e2e/api/metrics_test.go @@ -15,7 +15,13 @@ package api import ( + "slices" + "strconv" + + porchapi "github.com/kptdev/porch/api/porch/v1alpha1" + configapi "github.com/kptdev/porch/api/porchconfig/v1alpha1" suiteutils "github.com/kptdev/porch/test/e2e/suiteutils" + "sigs.k8s.io/controller-runtime/pkg/client" ) func (t *PorchSuite) TestMetricsEndpoint() { @@ -23,8 +29,8 @@ func (t *PorchSuite) TestMetricsEndpoint() { "go_.*", "http_server_.*", "http_client_.*", - "errors_total*", - "target_info*", + "errors_total.*", + "target_info.*", "promhttp_metric_handler_.*", } porchControllerShouldHaveRegexList := []string{ @@ -106,3 +112,122 @@ func (t *PorchSuite) TestPackageSizeMetric() { t.Assert().Regexp(metricName, collectionResults.PorchControllerMetrics, "porch controller metrics should contain %q", metricName) } } + +func (t *PorchSuite) TestPackageSizeMetricValues() { + // Create a new package via init, no task specified + const ( + repository = "metrics-values" + packageName = "metrics-package" + workspace = "metrics-workspace" + description = "empty-package description" + + expectedMetric = "porch_package_size_bytes_total" + ) + + // initialize a package + resources := t.setupFunctionTestPackage(repository, packageName, workspace, TestPackageSetupOptions{ + UpstreamRef: "redis-bucket/v1", + UpstreamDir: "redis-bucket", + }) + resources.Spec.Resources["configmap.yaml"] = ` +apiVersion: v1 +kind: ConfigMap +metadata: + name: kptfile.kpt.dev +data: + name: bucket-namespace +` + + // push a resource change + t.AddMutator(resources, t.KrmFunctionsRegistry+"/"+setNamespaceImage, suiteutils.WithConfigPath("configmap.yaml")) + t.UpdateF(resources) + + pr := &porchapi.PackageRevision{} + t.GetF(client.ObjectKey{Namespace: t.Namespace, Name: resources.Name}, pr) + + t.validatePorchServerSizeMetric(pr, expectedMetric) + + // propose and approve + pr.Spec.Lifecycle = porchapi.PackageRevisionLifecycleProposed + t.UpdateF(pr) + pr.Spec.Lifecycle = porchapi.PackageRevisionLifecyclePublished + pr = t.UpdateApprovalF(pr) + + t.validatePorchServerSizeMetric(pr, expectedMetric) + + // propose-delete and delete + pr.Spec.Lifecycle = porchapi.PackageRevisionLifecycleDeletionProposed + t.UpdateApprovalF(pr) + t.DeleteE(pr) + pr.Status.ResourcesSizeBytes = 0 + t.validatePorchServerSizeMetric(pr, expectedMetric) + + // register a repo to sync some package revisions and test metric creation in porch-controllers + t.RegisterGitRepositoryF(t.GetTestBlueprintsRepoURL(), suiteutils.TestBlueprintsRepoName, "", suiteutils.GiteaUser, suiteutils.GiteaPassword) + + upstreamPr := &porchapi.PackageRevision{} + t.GetF(client.ObjectKey{Namespace: t.Namespace, Name: "test-blueprints.basens.v4"}, upstreamPr) + t.validatePorchControllerSizeMetric(upstreamPr, expectedMetric) + + // delete the repo and wait for packages to be deleted to verify the metric is updated + var repo configapi.Repository + t.GetF(client.ObjectKey{Namespace: t.Namespace, Name: suiteutils.TestBlueprintsRepoName}, &repo) + t.DeleteE(&repo) + t.WaitUntilRepositoryDeleted(suiteutils.TestBlueprintsRepoName, t.Namespace) + t.WaitUntilAllPackagesDeleted(suiteutils.TestBlueprintsRepoName, t.Namespace) + + upstreamPr.Status.ResourcesSizeBytes = 0 + t.validatePorchControllerSizeMetric(upstreamPr, expectedMetric) +} + +func (t *PorchSuite) validatePorchServerSizeMetric(pr *porchapi.PackageRevision, metricName string) { + t.T().Helper() + if t.UsingDBCache { + collectionResults, err := t.CollectMetricsFromPods() + t.Require().NoError(err, "failed to collect metrics from pods:") + parsedResults, err := collectionResults.Parse() + t.Require().NoError(err, "failed to parse collected metrics:") + + t.Assert().Contains(parsedResults.PorchServerMetrics, metricName) + + metric := parsedResults.PorchServerMetrics[metricName] + metric = slices.DeleteFunc(metric, func(aMetric suiteutils.MetricResult) bool { + return !(aMetric.Attributes["namespace"] == t.Namespace && + aMetric.Attributes["package"] == pr.Spec.PackageName && + aMetric.Attributes["repository"] == pr.Spec.RepositoryName && + aMetric.Attributes["workspaceName"] == pr.Spec.WorkspaceName) + }) + t.Require().Len(metric, 1) + value, err := strconv.Atoi(metric[0].Value.(string)) + t.Require().NoError(err, "non-integer metric value:") + t.Assert().EqualValues(pr.Status.ResourcesSizeBytes, value) + } else { + t.Assert().EqualValues(0, pr.Status.ResourcesSizeBytes, "PackageRevision resources size should not be available in non-DB cache deployment") + } +} + +func (t *PorchSuite) validatePorchControllerSizeMetric(pr *porchapi.PackageRevision, metricName string) { + t.T().Helper() + if t.UsingDBCache { + collectionResults, err := t.CollectMetricsFromPods() + t.Require().NoError(err, "failed to collect metrics from pods:") + parsedResults, err := collectionResults.Parse() + t.Require().NoError(err, "failed to parse collected metrics:") + + t.Assert().Contains(parsedResults.PorchControllerMetrics, metricName) + + metric := parsedResults.PorchControllerMetrics[metricName] + metric = slices.DeleteFunc(metric, func(aMetric suiteutils.MetricResult) bool { + return !(aMetric.Attributes["namespace"] == t.Namespace && + aMetric.Attributes["package"] == pr.Spec.PackageName && + aMetric.Attributes["repository"] == pr.Spec.RepositoryName && + aMetric.Attributes["workspaceName"] == pr.Spec.WorkspaceName) + }) + t.Require().Len(metric, 1) + value, err := strconv.Atoi(metric[0].Value.(string)) + t.Require().NoError(err, "non-integer metric value:") + t.Assert().EqualValues(pr.Status.ResourcesSizeBytes, value) + } else { + t.Assert().EqualValues(0, pr.Status.ResourcesSizeBytes, "PackageRevision resources size should not be available in non-DB cache deployment") + } +} diff --git a/test/e2e/suiteutils/suite.go b/test/e2e/suiteutils/suite.go index 490f000ce..9b51533ad 100644 --- a/test/e2e/suiteutils/suite.go +++ b/test/e2e/suiteutils/suite.go @@ -160,10 +160,7 @@ func (t *TestSuite) Initialize() { } func (t *TestSuite) checkIfUsingDBCache() { - t.UsingDBCache = func() bool { - _, envVarSet := os.LookupEnv("DB_CACHE") - return envVarSet - }() + _, t.UsingDBCache = os.LookupEnv("DB_CACHE") } func (t *TestSuite) PorchServerServiceKey() client.ObjectKey { diff --git a/test/e2e/suiteutils/suite_utils.go b/test/e2e/suiteutils/suite_utils.go index 64201bdd6..23bc910bf 100644 --- a/test/e2e/suiteutils/suite_utils.go +++ b/test/e2e/suiteutils/suite_utils.go @@ -16,11 +16,14 @@ package suiteutils import ( "context" + "encoding/json" "errors" "fmt" "os" "reflect" + "regexp" "slices" + "strconv" "strings" "sync" @@ -64,6 +67,75 @@ type MetricsCollectionResults struct { PorchWrapperServerMetrics string } +type ParsedMetricsResults struct { + PorchServerMetrics map[string][]MetricResult + PorchControllerMetrics map[string][]MetricResult + PorchFunctionRunnerMetrics map[string][]MetricResult + PorchWrapperServerMetrics map[string][]MetricResult +} + +type MetricResult struct { + Value any + Attributes map[string]string +} + +func (r *MetricsCollectionResults) Parse() (parsed *ParsedMetricsResults, err error) { + jsonQuotingRegex, err := regexp.Compile("((^|,)([^\"]*?)(:))") + parsed = &ParsedMetricsResults{ + PorchServerMetrics: make(map[string][]MetricResult), + PorchControllerMetrics: make(map[string][]MetricResult), + PorchFunctionRunnerMetrics: make(map[string][]MetricResult), + PorchWrapperServerMetrics: make(map[string][]MetricResult), + } + + for _, pair := range []struct { + raw string + parsedResult map[string][]MetricResult + }{ + {r.PorchServerMetrics, parsed.PorchServerMetrics}, + {r.PorchControllerMetrics, parsed.PorchControllerMetrics}, + {r.PorchFunctionRunnerMetrics, parsed.PorchFunctionRunnerMetrics}, + {r.PorchWrapperServerMetrics, parsed.PorchWrapperServerMetrics}, + } { + + rawMetrics := strings.Split(pair.raw, "\n") + for _, metricLine := range rawMetrics { + if metricLine == "" { + continue + } + parts := strings.Split(metricLine, " ") + if len(parts) < 2 { + continue + } + metricNameAndAttributes := parts[0] + metricValue := parts[1] + + nonValueParts := strings.Split(metricNameAndAttributes, "{") + if len(nonValueParts) < 2 { + continue + } + + metricName := nonValueParts[0] + attributes := strings.ReplaceAll(nonValueParts[1], "=", ":") + attributes = strings.ReplaceAll(attributes, "\\\"", "\"") + attributes = jsonQuotingRegex.ReplaceAllString(attributes, `$2"$3"$4`) + + attributeMap := make(map[string]string) + err := json.Unmarshal([]byte("{"+attributes), &attributeMap) + if err != nil { + return nil, err + } + + pair.parsedResult[metricName] = append(pair.parsedResult[metricName], MetricResult{ + Value: metricValue, + Attributes: attributeMap, + }) + } + } + + return parsed, nil +} + type TestSuiteWithGit struct { TestSuite gitConfig GitConfig @@ -249,7 +321,7 @@ func (t *TestSuite) registerGitRepositoryFromConfigF(name string, config GitConf t.CreateF(repo) t.Cleanup(func() { - t.DeleteE(repo) + t.DeleteL(repo) t.WaitUntilRepositoryDeleted(name, repo.Namespace) t.WaitUntilAllPackagesDeleted(name, repo.Namespace) if IsPorchTestRepo(config.Repo) { From 65c4694a30114b9dd927aeb6122871bc969c88ff Mon Sep 17 00:00:00 2001 From: James McDermott Date: Mon, 8 Jun 2026 12:13:57 +0100 Subject: [PATCH 04/21] Address Copilot review comments part 2 Signed-off-by: James McDermott --- deployments/metrics/prometheus-deployment.yaml | 1 - internal/telemetry/metrics_test.go | 4 ++-- scripts/deploy-monitoring.sh | 2 +- test/e2e/suiteutils/suite_utils.go | 4 ++-- 4 files changed, 5 insertions(+), 6 deletions(-) diff --git a/deployments/metrics/prometheus-deployment.yaml b/deployments/metrics/prometheus-deployment.yaml index fe89648b6..29d059550 100644 --- a/deployments/metrics/prometheus-deployment.yaml +++ b/deployments/metrics/prometheus-deployment.yaml @@ -28,7 +28,6 @@ spec: labels: app: prometheus spec: - hostNetwork: true dnsPolicy: ClusterFirstWithHostNet serviceAccountName: prometheus containers: diff --git a/internal/telemetry/metrics_test.go b/internal/telemetry/metrics_test.go index 457516ae0..179e2dff0 100644 --- a/internal/telemetry/metrics_test.go +++ b/internal/telemetry/metrics_test.go @@ -38,7 +38,7 @@ func (f *fakePackageRevision) Key() repository.PackageRevisionKey { return f.key // Remaining interface methods are not called by RecordPackageSizeUpdate, // so they can panic if invoked unexpectedly. -func TestRecordPackageSizeUpdate_NilInstruments(t *testing.T) { +func TestRecordPackageRevisionResourcesSize_NilInstruments(t *testing.T) { InitMetrics() histogramBefore := prResourceSizeHistogram prResourceSizeHistogram = nil @@ -61,7 +61,7 @@ func TestRecordPackageSizeUpdate_NilInstruments(t *testing.T) { assert.NotPanics(t, func() { RecordPackageRevisionResourcesSize(fake, 1024) }) } -func TestRecordPackageSizeUpdate_RecordsMetrics(t *testing.T) { +func TestRecordPackageRevisionResourcesSize_RecordsMetrics(t *testing.T) { reader := sdkmetric.NewManualReader() mp := sdkmetric.NewMeterProvider(sdkmetric.WithReader(reader)) otel.SetMeterProvider(mp) diff --git a/scripts/deploy-monitoring.sh b/scripts/deploy-monitoring.sh index c6276ba65..b07557f85 100755 --- a/scripts/deploy-monitoring.sh +++ b/scripts/deploy-monitoring.sh @@ -13,7 +13,7 @@ # See the License for the specific language governing permissions and # limitations under the License. -set -e +set -euo pipefail SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" METRICS_DIR="${SCRIPT_DIR}/../deployments/metrics" DOT_ENV_PATH="${SCRIPT_DIR}/../.env" diff --git a/test/e2e/suiteutils/suite_utils.go b/test/e2e/suiteutils/suite_utils.go index 23bc910bf..ffe8e6d20 100644 --- a/test/e2e/suiteutils/suite_utils.go +++ b/test/e2e/suiteutils/suite_utils.go @@ -80,7 +80,7 @@ type MetricResult struct { } func (r *MetricsCollectionResults) Parse() (parsed *ParsedMetricsResults, err error) { - jsonQuotingRegex, err := regexp.Compile("((^|,)([^\"]*?)(:))") + jsonQuotingRegex := regexp.MustCompile("((^|,)([^\"]*?)(:))") parsed = &ParsedMetricsResults{ PorchServerMetrics: make(map[string][]MetricResult), PorchControllerMetrics: make(map[string][]MetricResult), @@ -103,7 +103,7 @@ func (r *MetricsCollectionResults) Parse() (parsed *ParsedMetricsResults, err er if metricLine == "" { continue } - parts := strings.Split(metricLine, " ") + parts := strings.Fields(metricLine) if len(parts) < 2 { continue } From 5257450e5e8d89a57ea31c1275cd5dee43eae277 Mon Sep 17 00:00:00 2001 From: James McDermott Date: Mon, 8 Jun 2026 15:56:48 +0100 Subject: [PATCH 05/21] Address Copilot review comments part 3 Signed-off-by: James McDermott --- internal/telemetry/metrics.go | 6 +++--- internal/telemetry/metrics_test.go | 6 +++--- scripts/deploy-monitoring.sh | 25 ++++++++++++++----------- test/e2e/api/metrics_test.go | 2 +- 4 files changed, 21 insertions(+), 18 deletions(-) diff --git a/internal/telemetry/metrics.go b/internal/telemetry/metrics.go index 44a83a898..e8d9829e3 100644 --- a/internal/telemetry/metrics.go +++ b/internal/telemetry/metrics.go @@ -27,14 +27,14 @@ import ( const meterName = "github.com/kptdev/porch" var ( - prResourceSizeHistogram metric.Float64Histogram + prResourceSizeHistogram metric.Int64Histogram prResourceSizeGauge metric.Int64Gauge ) func InitMetrics() (err error) { m := otel.Meter(meterName) - prResourceSizeHistogram, err = m.Float64Histogram( + prResourceSizeHistogram, err = m.Int64Histogram( "porch_package_size_bytes", metric.WithUnit("By"), metric.WithDescription("Distribution of package revision resources' file size, in bytes"), @@ -84,7 +84,7 @@ func RecordPackageRevisionResourcesSize(prKey repository.PackageRevisionKey, res resourcesSize, attributes.MarshalLog()) } - prResourceSizeHistogram.Record(context.Background(), float64(resourcesSize), metric.WithAttributeSet(attributes)) + prResourceSizeHistogram.Record(context.Background(), resourcesSize, metric.WithAttributeSet(attributes)) if prResourceSizeGauge == nil { klog.Warning("prResourceSizeGauge is nil - was InitMetrics() called?") diff --git a/internal/telemetry/metrics_test.go b/internal/telemetry/metrics_test.go index 179e2dff0..2dfa43865 100644 --- a/internal/telemetry/metrics_test.go +++ b/internal/telemetry/metrics_test.go @@ -35,11 +35,11 @@ type fakePackageRevision struct { func (f *fakePackageRevision) KubeObjectNamespace() string { return f.namespace } func (f *fakePackageRevision) Key() repository.PackageRevisionKey { return f.key } -// Remaining interface methods are not called by RecordPackageSizeUpdate, +// Remaining interface methods are not called by RecordPackageRevisionResourcesSize, // so they can panic if invoked unexpectedly. func TestRecordPackageRevisionResourcesSize_NilInstruments(t *testing.T) { - InitMetrics() + require.NoError(t, InitMetrics()) histogramBefore := prResourceSizeHistogram prResourceSizeHistogram = nil defer func() { prResourceSizeHistogram = histogramBefore }() @@ -67,7 +67,7 @@ func TestRecordPackageRevisionResourcesSize_RecordsMetrics(t *testing.T) { otel.SetMeterProvider(mp) defer mp.Shutdown(context.Background()) - InitMetrics() + require.NoError(t, InitMetrics()) fake := repository.PackageRevisionKey{ diff --git a/scripts/deploy-monitoring.sh b/scripts/deploy-monitoring.sh index b07557f85..0382f7bf0 100755 --- a/scripts/deploy-monitoring.sh +++ b/scripts/deploy-monitoring.sh @@ -17,6 +17,7 @@ set -euo pipefail SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" METRICS_DIR="${SCRIPT_DIR}/../deployments/metrics" DOT_ENV_PATH="${SCRIPT_DIR}/../.env" +PORT_FORWARD_DIR="$(mktemp --directory --suffix "_porch-monitoring-pf.pid.d")" if [[ -f "$DOT_ENV_PATH" ]]; then source "$DOT_ENV_PATH" @@ -156,12 +157,17 @@ wait_for_deployment() { kubectl wait --for=condition=available --timeout=300s deployment/$deployment -n "$NAMESPACE" } +stop_port_forwards() { + find /tmp/tmp*_porch-monitoring-pf.pid.d/ -name '*.pid' -exec pkill --pidfile '{}' \; 2>/dev/null || true + find /tmp/tmp*_porch-monitoring-pf.pid.d/ -name '*.pid' ! -wholename "${PORT_FORWARD_DIR}*" -exec rm '{}' \; 2>/dev/null || true + find /tmp/tmp*_porch-monitoring-pf.pid.d/ -type d ! -wholename "${PORT_FORWARD_DIR}*" -exec rmdir '{}' \; 2>/dev/null || true +} + get_service_urls() { log_info "Getting service URLs..." - log_info "Setting up port forwarding..." - pkill -f "port-forward.*prometheus" 2>/dev/null || true - pkill -f "port-forward.*grafana" 2>/dev/null || true + log_info "Setting up port forwarding..." + stop_port_forwards sleep 2 kubectl port-forward -n "${NAMESPACE}" svc/prometheus "$PROMETHEUS_LOCAL_PORT":"$PROMETHEUS_CONTAINER_PORT" > /dev/null 2>&1 & @@ -171,8 +177,8 @@ get_service_urls() { sleep 2 - echo "${PROMETHEUS_PF_PID}" > /tmp/porch-prometheus-pf.pid - echo "${GRAFANA_PF_PID}" > /tmp/porch-grafana-pf.pid + echo "${PROMETHEUS_PF_PID}" > "$PORT_FORWARD_DIR"/porch-prometheus-pf.pid + echo "${GRAFANA_PF_PID}" > "$PORT_FORWARD_DIR"/porch-grafana-pf.pid PROMETHEUS_URL="http://localhost:$PROMETHEUS_LOCAL_PORT" GRAFANA_URL="http://localhost:$GRAFANA_LOCAL_PORT" @@ -200,18 +206,15 @@ get_service_urls() { log_info "" echo "" log_info "To stop port forwarding:" - log_info ' kill $(cat /tmp/porch-prometheus-pf.pid /tmp/porch-grafana-pf.pid 2>/dev/null)' + log_info ' find /tmp/tmp*_porch-monitoring-pf.pid.d/ -name '*.pid' -exec pkill --pidfile '{}' \;' echo "" } cleanup() { log_warn "Cleaning up existing deployment..." - if [ -f /tmp/porch-prometheus-pf.pid ] || [ -f /tmp/porch-grafana-pf.pid ] ; then - log_info "Stopping port forwarding..." - kill $(cat /tmp/porch-prometheus-pf.pid /tmp/porch-grafana-pf.pid 2>/dev/null) 2>/dev/null || true - rm -f /tmp/porch-prometheus-pf.pid /tmp/porch-grafana-pf.pid - fi + log_info "Stopping port forwarding..." + stop_port_forwards if kubectl get namespace "$NAMESPACE" &> /dev/null; then log_info "Deleting resources in namespace $NAMESPACE..." diff --git a/test/e2e/api/metrics_test.go b/test/e2e/api/metrics_test.go index 6294a36c3..d34f6a824 100644 --- a/test/e2e/api/metrics_test.go +++ b/test/e2e/api/metrics_test.go @@ -198,7 +198,7 @@ func (t *PorchSuite) validatePorchServerSizeMetric(pr *porchapi.PackageRevision, aMetric.Attributes["workspaceName"] == pr.Spec.WorkspaceName) }) t.Require().Len(metric, 1) - value, err := strconv.Atoi(metric[0].Value.(string)) + value, err := strconv.ParseInt(metric[0].Value.(string), 0, 64) t.Require().NoError(err, "non-integer metric value:") t.Assert().EqualValues(pr.Status.ResourcesSizeBytes, value) } else { From ecf5fd27f4590424e4bcb57277d43d3c4534c55b Mon Sep 17 00:00:00 2001 From: James McDermott Date: Mon, 8 Jun 2026 16:52:08 +0100 Subject: [PATCH 06/21] comment nitpick to retrigger CI Signed-off-by: James McDermott --- pkg/cli/commands/rpkg/approve/command_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cli/commands/rpkg/approve/command_test.go b/pkg/cli/commands/rpkg/approve/command_test.go index ac31db480..df494ea41 100644 --- a/pkg/cli/commands/rpkg/approve/command_test.go +++ b/pkg/cli/commands/rpkg/approve/command_test.go @@ -128,7 +128,7 @@ func TestCmd(t *testing.T) { "Approve deletion-proposed package": { output: pkgRevName + " approved\n", fakeclient: fake.NewClientBuilder().WithInterceptorFuncs(interceptor.Funcs{ - //fake subresourceupdate + // fake SubResourceUpdate SubResourceUpdate: func(ctx context.Context, client client.Client, subResourceName string, obj client.Object, opts ...client.SubResourceUpdateOption) error { return nil }, From 000bc7554f334063d03cab46595b726f6f3b7f9b Mon Sep 17 00:00:00 2001 From: James McDermott Date: Mon, 8 Jun 2026 17:48:01 +0100 Subject: [PATCH 07/21] Address Copilot review comments part 4 Signed-off-by: James McDermott --- .../configurations/opentelemetry.md | 11 ++++---- internal/telemetry/otel.go | 7 +++++- .../dbcache/dbpackagerevisionresourcessql.go | 4 +-- scripts/deploy-monitoring.sh | 16 ++++++------ test/e2e/suiteutils/suite_utils.go | 25 +++++++++++-------- 5 files changed, 36 insertions(+), 27 deletions(-) diff --git a/docs/content/en/docs/6_configuration_and_deployments/configurations/opentelemetry.md b/docs/content/en/docs/6_configuration_and_deployments/configurations/opentelemetry.md index d98b49fff..5e59da418 100644 --- a/docs/content/en/docs/6_configuration_and_deployments/configurations/opentelemetry.md +++ b/docs/content/en/docs/6_configuration_and_deployments/configurations/opentelemetry.md @@ -424,11 +424,12 @@ Porch records the following metrics via OpenTelemetry: Package size metrics are recorded with the following attributes from the relevant package: -| Attribute | Description | -|--------------|-------------| -| `namespace` | Kubernetes namespace of the package revision | -| `repository` | Name of the repository containing the package | -| `package` | Path and name of the package | +| Attribute | Description | +|-----------------|-------------| +| `namespace` | Kubernetes namespace of the package revision | +| `repository` | Name of the repository containing the package | +| `package` | Path and name of the package | +| `workspaceName` | A short, unique description of the changes contained in the package revision | These metrics are recorded as part of every flow that updates package revision resources: - Create package revision diff --git a/internal/telemetry/otel.go b/internal/telemetry/otel.go index e679f9fa4..67c0d84a1 100644 --- a/internal/telemetry/otel.go +++ b/internal/telemetry/otel.go @@ -38,6 +38,7 @@ import ( ) const ( + otelHostEnv = "OTEL_EXPORTER_PROMETHEUS_HOST" otelPortEnv = "OTEL_EXPORTER_PROMETHEUS_PORT" ) @@ -181,6 +182,10 @@ func setupMetrics(ctx context.Context, res *OTelResources) error { } func startMetricsServerIfConfigured(res *OTelResources) error { + hostStr := os.Getenv(otelHostEnv) + if hostStr == "" { + return nil + } portStr := os.Getenv(otelPortEnv) if portStr == "" { return nil @@ -205,7 +210,7 @@ func startMetricsServerIfConfigured(res *OTelResources) error { mux.Handle("/metrics", handler) srv := &http.Server{ - Addr: fmt.Sprintf(":%d", port), + Addr: fmt.Sprintf("%s:%d", hostStr, port), Handler: mux, ReadHeaderTimeout: 10 * time.Second, } diff --git a/pkg/cache/dbcache/dbpackagerevisionresourcessql.go b/pkg/cache/dbcache/dbpackagerevisionresourcessql.go index 958149689..f034e5c11 100644 --- a/pkg/cache/dbcache/dbpackagerevisionresourcessql.go +++ b/pkg/cache/dbcache/dbpackagerevisionresourcessql.go @@ -160,12 +160,10 @@ func pkgRevResourcesDeleteFromDB(ctx context.Context, prk repository.PackageRevi if err == nil { klog.V(5).Infof("pkgRevResourcesDeleteFromDB: deleted package revision resources for %+v", prk) + telemetry.RecordPackageRevisionResourcesSize(prk, 0) } else { klog.Warningf("pkgRevResourcesDeleteFromDB: deletion of package revision resources for %+v failed: %q", prk, err) } - - telemetry.RecordPackageRevisionResourcesSize(prk, 0) - return err } diff --git a/scripts/deploy-monitoring.sh b/scripts/deploy-monitoring.sh index 0382f7bf0..057aa7a35 100755 --- a/scripts/deploy-monitoring.sh +++ b/scripts/deploy-monitoring.sh @@ -170,18 +170,18 @@ get_service_urls() { stop_port_forwards sleep 2 - kubectl port-forward -n "${NAMESPACE}" svc/prometheus "$PROMETHEUS_LOCAL_PORT":"$PROMETHEUS_CONTAINER_PORT" > /dev/null 2>&1 & + kubectl port-forward -n "${NAMESPACE}" svc/prometheus "${PROMETHEUS_LOCAL_PORT}":"${PROMETHEUS_CONTAINER_PORT}" > /dev/null 2>&1 & PROMETHEUS_PF_PID=$! - kubectl port-forward -n "${NAMESPACE}" svc/grafana "$GRAFANA_LOCAL_PORT":"$GRAFANA_CONTAINER_PORT" > /dev/null 2>&1 & + kubectl port-forward -n "${NAMESPACE}" svc/grafana "${GRAFANA_LOCAL_PORT}":"${GRAFANA_CONTAINER_PORT}" > /dev/null 2>&1 & GRAFANA_PF_PID=$! sleep 2 - echo "${PROMETHEUS_PF_PID}" > "$PORT_FORWARD_DIR"/porch-prometheus-pf.pid - echo "${GRAFANA_PF_PID}" > "$PORT_FORWARD_DIR"/porch-grafana-pf.pid + echo "${PROMETHEUS_PF_PID}" > "${PORT_FORWARD_DIR}"/porch-prometheus-pf.pid + echo "${GRAFANA_PF_PID}" > "${PORT_FORWARD_DIR}"/porch-grafana-pf.pid - PROMETHEUS_URL="http://localhost:$PROMETHEUS_LOCAL_PORT" - GRAFANA_URL="http://localhost:$GRAFANA_LOCAL_PORT" + PROMETHEUS_URL="http://localhost:${PROMETHEUS_LOCAL_PORT}" + GRAFANA_URL="http://localhost:${GRAFANA_LOCAL_PORT}" PROMETHEUS_NODEPORT_URL="http://localhost:${PROMETHEUS_NODEPORT}" GRAFANA_NODEPORT_URL="http://localhost:${GRAFANA_NODEPORT}" @@ -205,8 +205,8 @@ get_service_urls() { log_info " - Prometheus scraping metrics from function-runner port 9464" log_info "" echo "" - log_info "To stop port forwarding:" - log_info ' find /tmp/tmp*_porch-monitoring-pf.pid.d/ -name '*.pid' -exec pkill --pidfile '{}' \;' + log_info "To stop port forwarding, run:" + log_info " find /tmp/tmp*_porch-monitoring-pf.pid.d/ -name '*.pid' -exec pkill --pidfile '{}' \;" echo "" } diff --git a/test/e2e/suiteutils/suite_utils.go b/test/e2e/suiteutils/suite_utils.go index ffe8e6d20..912b6f4b2 100644 --- a/test/e2e/suiteutils/suite_utils.go +++ b/test/e2e/suiteutils/suite_utils.go @@ -79,8 +79,9 @@ type MetricResult struct { Attributes map[string]string } +var jsonQuotingRegex = regexp.MustCompile("((^|,)([^\"]*?)(:))") + func (r *MetricsCollectionResults) Parse() (parsed *ParsedMetricsResults, err error) { - jsonQuotingRegex := regexp.MustCompile("((^|,)([^\"]*?)(:))") parsed = &ParsedMetricsResults{ PorchServerMetrics: make(map[string][]MetricResult), PorchControllerMetrics: make(map[string][]MetricResult), @@ -111,19 +112,23 @@ func (r *MetricsCollectionResults) Parse() (parsed *ParsedMetricsResults, err er metricValue := parts[1] nonValueParts := strings.Split(metricNameAndAttributes, "{") - if len(nonValueParts) < 2 { + + if len(nonValueParts) > 2 || len(nonValueParts) == 0 { + // Malformed metric - skip it. continue } - metricName := nonValueParts[0] - attributes := strings.ReplaceAll(nonValueParts[1], "=", ":") - attributes = strings.ReplaceAll(attributes, "\\\"", "\"") - attributes = jsonQuotingRegex.ReplaceAllString(attributes, `$2"$3"$4`) - attributeMap := make(map[string]string) - err := json.Unmarshal([]byte("{"+attributes), &attributeMap) - if err != nil { - return nil, err + if len(nonValueParts) == 2 { + attributes := strings.ReplaceAll(nonValueParts[1], "=", ":") + attributes = strings.ReplaceAll(attributes, "\\\"", "\"") + attributes = jsonQuotingRegex.ReplaceAllString(attributes, `$2"$3"$4`) + attributes = "{" + attributes + + err := json.Unmarshal([]byte(attributes), &attributeMap) + if err != nil { + return nil, err + } } pair.parsedResult[metricName] = append(pair.parsedResult[metricName], MetricResult{ From 024a2210a4eaff219393697a58779b5e118f34fe Mon Sep 17 00:00:00 2001 From: James McDermott Date: Mon, 8 Jun 2026 18:02:53 +0100 Subject: [PATCH 08/21] Fix failing unit test Signed-off-by: James McDermott --- internal/telemetry/otel_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/internal/telemetry/otel_test.go b/internal/telemetry/otel_test.go index 4ec4cdc80..be94cb206 100644 --- a/internal/telemetry/otel_test.go +++ b/internal/telemetry/otel_test.go @@ -42,6 +42,7 @@ const ( ENV_OTEL_TRACES_EXPORTER = "OTEL_TRACES_EXPORTER" DEFAULT_OTEL_TRACES_EXPORTER = "none" + ENV_OTEL_EXPORTER_PROMETHEUS_HOST = "OTEL_EXPORTER_PROMETHEUS_HOST" ENV_OTEL_EXPORTER_PROMETHEUS_PORT = "OTEL_EXPORTER_PROMETHEUS_PORT" ENV_OTEL_EXPORTER_OTLP_ENDPOINT = "OTEL_EXPORTER_OTLP_ENDPOINT" ENV_OTEL_EXPORTER_OTLP_PROTOCOL = "OTEL_EXPORTER_OTLP_PROTOCOL" @@ -56,6 +57,7 @@ func TestPrometheusHTTPServer(t *testing.T) { t.Setenv(ENV_OTEL_METRICS_EXPORTER, METRICS_EXPORTER_PROMETHEUS) t.Setenv(ENV_OTEL_TRACES_EXPORTER, DEFAULT_OTEL_TRACES_EXPORTER) + t.Setenv(ENV_OTEL_EXPORTER_PROMETHEUS_HOST, "0.0.0.0") t.Setenv(ENV_OTEL_EXPORTER_PROMETHEUS_PORT, fmt.Sprintf("%d", port)) ctx, cancel := context.WithCancel(context.Background()) @@ -78,6 +80,7 @@ func TestPrometheusHTTPServer(t *testing.T) { func TestPrometheusHTTPServerInvalidPort(t *testing.T) { t.Setenv(ENV_OTEL_METRICS_EXPORTER, METRICS_EXPORTER_PROMETHEUS) t.Setenv(ENV_OTEL_TRACES_EXPORTER, DEFAULT_OTEL_TRACES_EXPORTER) + t.Setenv(ENV_OTEL_EXPORTER_PROMETHEUS_HOST, "0.0.0.0") t.Setenv(ENV_OTEL_EXPORTER_PROMETHEUS_PORT, "not-a-number") ctx := context.Background() From d1990e48f2f41b9779d961a23caa1f2f76df3e97 Mon Sep 17 00:00:00 2001 From: James McDermott Date: Tue, 9 Jun 2026 16:32:56 +0100 Subject: [PATCH 09/21] Address Copilot review comments part 5 Signed-off-by: James McDermott --- internal/telemetry/otel.go | 27 ++++++------- test/e2e/api/metrics_test.go | 53 ++++++++++---------------- test/e2e/suiteutils/suite.go | 2 +- test/e2e/suiteutils/suite_utils.go | 61 +++++++++++------------------- 4 files changed, 58 insertions(+), 85 deletions(-) diff --git a/internal/telemetry/otel.go b/internal/telemetry/otel.go index 67c0d84a1..9b9267651 100644 --- a/internal/telemetry/otel.go +++ b/internal/telemetry/otel.go @@ -149,23 +149,24 @@ func setupMetrics(ctx context.Context, res *OTelResources) error { autoexport.WithFallbackMetricProducer(func(ctx context.Context) (sdkmetric.Producer, error) { return prombridge.NewMetricProducer( - prombridge.WithGatherer(prometheus.Gatherers{ - prometheus.DefaultGatherer, - controllerruntimemetrics.Registry, - }), + prombridge.WithGatherer(controllerruntimemetrics.Registry), ), nil }) - promExp, err := otelprometheus.New( - otelprometheus.WithRegisterer(prometheus.DefaultRegisterer), - ) - if err != nil { - return fmt.Errorf("failed to create prometheus exporter: %w", err) - } - - readers := []sdkmetric.Option{sdkmetric.WithReader(promExp)} + readers := []sdkmetric.Option{} + if exporter == "prometheus" || os.Getenv(otelPortEnv) != "" { - if exporter != "prometheus" { + // Only create the Prometheus exporter when we intend to expose a scrape + // endpoint, to avoid writing OTel metrics into the default Prometheus + // registry when pushing via OTLP. + promExp, err := otelprometheus.New( + otelprometheus.WithRegisterer(prometheus.DefaultRegisterer), + ) + if err != nil { + return fmt.Errorf("failed to create prometheus exporter: %w", err) + } + readers = append(readers, sdkmetric.WithReader(promExp)) + } else { autoMr, err := autoexport.NewMetricReader(ctx) if err != nil { return fmt.Errorf("failed to create metric reader: %w", err) diff --git a/test/e2e/api/metrics_test.go b/test/e2e/api/metrics_test.go index d34f6a824..0838d5195 100644 --- a/test/e2e/api/metrics_test.go +++ b/test/e2e/api/metrics_test.go @@ -16,11 +16,11 @@ package api import ( "slices" - "strconv" porchapi "github.com/kptdev/porch/api/porch/v1alpha1" configapi "github.com/kptdev/porch/api/porchconfig/v1alpha1" suiteutils "github.com/kptdev/porch/test/e2e/suiteutils" + "github.com/prometheus/common/model" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -182,31 +182,19 @@ data: func (t *PorchSuite) validatePorchServerSizeMetric(pr *porchapi.PackageRevision, metricName string) { t.T().Helper() - if t.UsingDBCache { - collectionResults, err := t.CollectMetricsFromPods() - t.Require().NoError(err, "failed to collect metrics from pods:") - parsedResults, err := collectionResults.Parse() - t.Require().NoError(err, "failed to parse collected metrics:") - - t.Assert().Contains(parsedResults.PorchServerMetrics, metricName) - - metric := parsedResults.PorchServerMetrics[metricName] - metric = slices.DeleteFunc(metric, func(aMetric suiteutils.MetricResult) bool { - return !(aMetric.Attributes["namespace"] == t.Namespace && - aMetric.Attributes["package"] == pr.Spec.PackageName && - aMetric.Attributes["repository"] == pr.Spec.RepositoryName && - aMetric.Attributes["workspaceName"] == pr.Spec.WorkspaceName) - }) - t.Require().Len(metric, 1) - value, err := strconv.ParseInt(metric[0].Value.(string), 0, 64) - t.Require().NoError(err, "non-integer metric value:") - t.Assert().EqualValues(pr.Status.ResourcesSizeBytes, value) - } else { - t.Assert().EqualValues(0, pr.Status.ResourcesSizeBytes, "PackageRevision resources size should not be available in non-DB cache deployment") - } + t.validateSizeMetric(pr, metricName, func(parsedResults *suiteutils.ParsedMetricsResults) map[string][]suiteutils.MetricResult { + return parsedResults.PorchServerMetrics + }) } func (t *PorchSuite) validatePorchControllerSizeMetric(pr *porchapi.PackageRevision, metricName string) { + t.T().Helper() + t.validateSizeMetric(pr, metricName, func(parsedResults *suiteutils.ParsedMetricsResults) map[string][]suiteutils.MetricResult { + return parsedResults.PorchControllerMetrics + }) +} + +func (t *PorchSuite) validateSizeMetric(pr *porchapi.PackageRevision, metricName string, selectPodMetrics func(*suiteutils.ParsedMetricsResults) map[string][]suiteutils.MetricResult) { t.T().Helper() if t.UsingDBCache { collectionResults, err := t.CollectMetricsFromPods() @@ -214,20 +202,21 @@ func (t *PorchSuite) validatePorchControllerSizeMetric(pr *porchapi.PackageRevis parsedResults, err := collectionResults.Parse() t.Require().NoError(err, "failed to parse collected metrics:") - t.Assert().Contains(parsedResults.PorchControllerMetrics, metricName) + podParsedResults := selectPodMetrics(parsedResults) - metric := parsedResults.PorchControllerMetrics[metricName] + t.Assert().Contains(podParsedResults, metricName) + + metric := podParsedResults[metricName] metric = slices.DeleteFunc(metric, func(aMetric suiteutils.MetricResult) bool { - return !(aMetric.Attributes["namespace"] == t.Namespace && - aMetric.Attributes["package"] == pr.Spec.PackageName && - aMetric.Attributes["repository"] == pr.Spec.RepositoryName && - aMetric.Attributes["workspaceName"] == pr.Spec.WorkspaceName) + return !(aMetric.Attributes["namespace"] == model.LabelValue(t.Namespace) && + aMetric.Attributes["package"] == model.LabelValue(pr.Spec.PackageName) && + aMetric.Attributes["repository"] == model.LabelValue(pr.Spec.RepositoryName) && + aMetric.Attributes["workspaceName"] == model.LabelValue(pr.Spec.WorkspaceName)) }) t.Require().Len(metric, 1) - value, err := strconv.Atoi(metric[0].Value.(string)) - t.Require().NoError(err, "non-integer metric value:") - t.Assert().EqualValues(pr.Status.ResourcesSizeBytes, value) + t.Assert().EqualValues(model.SampleValue(pr.Status.ResourcesSizeBytes), metric[0].Value) } else { t.Assert().EqualValues(0, pr.Status.ResourcesSizeBytes, "PackageRevision resources size should not be available in non-DB cache deployment") } + } diff --git a/test/e2e/suiteutils/suite.go b/test/e2e/suiteutils/suite.go index 9b51533ad..72a75fbaf 100644 --- a/test/e2e/suiteutils/suite.go +++ b/test/e2e/suiteutils/suite.go @@ -378,7 +378,7 @@ func (t *TestSuite) delete(obj client.Object, opts []client.DeleteOption, eh Err t.T().Helper() t.Logf("deleting object %v", DebugFormat(obj)) - if err := t.Client.Delete(t.GetContext(), obj, opts...); err != nil { + if err := client.IgnoreNotFound(t.Client.Delete(t.GetContext(), obj, opts...)); err != nil { eh("failed to delete resource %s: %v", DebugFormat(obj), err) } } diff --git a/test/e2e/suiteutils/suite_utils.go b/test/e2e/suiteutils/suite_utils.go index 912b6f4b2..87aa157c0 100644 --- a/test/e2e/suiteutils/suite_utils.go +++ b/test/e2e/suiteutils/suite_utils.go @@ -16,12 +16,10 @@ package suiteutils import ( "context" - "encoding/json" "errors" "fmt" "os" "reflect" - "regexp" "slices" "strconv" @@ -37,6 +35,8 @@ import ( configapi "github.com/kptdev/porch/api/porchconfig/v1alpha1" pvapi "github.com/kptdev/porch/controllers/packagevariants/api/v1alpha1" internalapi "github.com/kptdev/porch/internal/api/porchinternal/v1alpha1" + "github.com/prometheus/common/expfmt" + "github.com/prometheus/common/model" coreapi "k8s.io/api/core/v1" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -58,6 +58,7 @@ const ( var ( PackageRevisionGVK = porchapi.SchemeGroupVersion.WithKind("PackageRevision") + metricsParser = expfmt.NewTextParser(model.LegacyValidation) ) type MetricsCollectionResults struct { @@ -75,12 +76,10 @@ type ParsedMetricsResults struct { } type MetricResult struct { - Value any - Attributes map[string]string + Value model.SampleValue + Attributes model.LabelSet } -var jsonQuotingRegex = regexp.MustCompile("((^|,)([^\"]*?)(:))") - func (r *MetricsCollectionResults) Parse() (parsed *ParsedMetricsResults, err error) { parsed = &ParsedMetricsResults{ PorchServerMetrics: make(map[string][]MetricResult), @@ -98,43 +97,27 @@ func (r *MetricsCollectionResults) Parse() (parsed *ParsedMetricsResults, err er {r.PorchFunctionRunnerMetrics, parsed.PorchFunctionRunnerMetrics}, {r.PorchWrapperServerMetrics, parsed.PorchWrapperServerMetrics}, } { + if pair.raw == "" { + continue + } - rawMetrics := strings.Split(pair.raw, "\n") - for _, metricLine := range rawMetrics { - if metricLine == "" { - continue - } - parts := strings.Fields(metricLine) - if len(parts) < 2 { - continue - } - metricNameAndAttributes := parts[0] - metricValue := parts[1] - - nonValueParts := strings.Split(metricNameAndAttributes, "{") + metricFamilies, err := metricsParser.TextToMetricFamilies(strings.NewReader(pair.raw)) + if err != nil { + return nil, err + } - if len(nonValueParts) > 2 || len(nonValueParts) == 0 { - // Malformed metric - skip it. + for metricName, family := range metricFamilies { + samples, err := expfmt.ExtractSamples(&expfmt.DecodeOptions{}, family) + if err != nil { continue } - metricName := nonValueParts[0] - attributeMap := make(map[string]string) - if len(nonValueParts) == 2 { - attributes := strings.ReplaceAll(nonValueParts[1], "=", ":") - attributes = strings.ReplaceAll(attributes, "\\\"", "\"") - attributes = jsonQuotingRegex.ReplaceAllString(attributes, `$2"$3"$4`) - attributes = "{" + attributes - - err := json.Unmarshal([]byte(attributes), &attributeMap) - if err != nil { - return nil, err - } - } + for _, sample := range samples { + pair.parsedResult[metricName] = append(pair.parsedResult[metricName], MetricResult{ + Value: sample.Value, + Attributes: model.LabelSet(sample.Metric), + }) - pair.parsedResult[metricName] = append(pair.parsedResult[metricName], MetricResult{ - Value: metricValue, - Attributes: attributeMap, - }) + } } } @@ -326,7 +309,7 @@ func (t *TestSuite) registerGitRepositoryFromConfigF(name string, config GitConf t.CreateF(repo) t.Cleanup(func() { - t.DeleteL(repo) + t.DeleteE(repo) t.WaitUntilRepositoryDeleted(name, repo.Namespace) t.WaitUntilAllPackagesDeleted(name, repo.Namespace) if IsPorchTestRepo(config.Repo) { From 9f465a1a2f15d386c5ff0922363dc0a8096164f4 Mon Sep 17 00:00:00 2001 From: James McDermott Date: Tue, 9 Jun 2026 16:57:29 +0100 Subject: [PATCH 10/21] Nitpick to retrigger CI Signed-off-by: James McDermott --- pkg/cli/commands/rpkg/docs/docs.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/cli/commands/rpkg/docs/docs.go b/pkg/cli/commands/rpkg/docs/docs.go index 95b8e3fbd..bcfbf241a 100644 --- a/pkg/cli/commands/rpkg/docs/docs.go +++ b/pkg/cli/commands/rpkg/docs/docs.go @@ -237,7 +237,7 @@ var PullLong = ` Args: K8S_PACKAGE_REV_NAME: - The kubernetes name of a an existing package revision in a repository. + The kubernetes name of an existing package revision in a repository. DIR: A local directory where the package manifests will be written. @@ -255,7 +255,7 @@ var PushLong = ` Args: K8S_PACKAGE_REV_NAME: - The kubernetes name of a an existing package revision in a repository. + The kubernetes name of an existing package revision in a repository. DIR: A local directory with the new manifest. If the manifests have be read from stdin, use '-' in place of DIR. From 8a260594ebcbbc2d523a2f5c9a3ffd31434cd57a Mon Sep 17 00:00:00 2001 From: James McDermott Date: Tue, 9 Jun 2026 19:41:06 +0100 Subject: [PATCH 11/21] Address Copilot review comments part 6 Signed-off-by: James McDermott --- internal/telemetry/otel.go | 5 +++-- test/e2e/suiteutils/suite_utils.go | 6 +++--- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/internal/telemetry/otel.go b/internal/telemetry/otel.go index 9b9267651..cc1ba9b10 100644 --- a/internal/telemetry/otel.go +++ b/internal/telemetry/otel.go @@ -100,8 +100,9 @@ func (r *OTelResources) Flush() error { // SetupOpenTelemetry is the single entry point for all OpenTelemetry setup. // It configures tracing, metrics (including the Prometheus HTTP server if -// OTEL_EXPORTER_PROMETHEUS_PORT is set), and initializes all Porch metric -// instruments. Returns OTelResources for lifecycle management. +// OTEL_EXPORTER_PROMETHEUS_HOST and OTEL_EXPORTER_PROMETHEUS_PORT are set), +// and initializes all Porch metric instruments. Returns OTelResources +// for lifecycle management. func SetupOpenTelemetry(ctx context.Context) (*OTelResources, error) { setupTiming := time.Now() res := &OTelResources{} diff --git a/test/e2e/suiteutils/suite_utils.go b/test/e2e/suiteutils/suite_utils.go index 87aa157c0..26c0e2950 100644 --- a/test/e2e/suiteutils/suite_utils.go +++ b/test/e2e/suiteutils/suite_utils.go @@ -21,7 +21,6 @@ import ( "os" "reflect" "slices" - "strconv" "strings" "sync" @@ -46,6 +45,7 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/wait" + "k8s.io/klog/v2" "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/yaml" @@ -58,7 +58,6 @@ const ( var ( PackageRevisionGVK = porchapi.SchemeGroupVersion.WithKind("PackageRevision") - metricsParser = expfmt.NewTextParser(model.LegacyValidation) ) type MetricsCollectionResults struct { @@ -87,6 +86,7 @@ func (r *MetricsCollectionResults) Parse() (parsed *ParsedMetricsResults, err er PorchFunctionRunnerMetrics: make(map[string][]MetricResult), PorchWrapperServerMetrics: make(map[string][]MetricResult), } + metricsParser := expfmt.NewTextParser(model.LegacyValidation) for _, pair := range []struct { raw string @@ -109,6 +109,7 @@ func (r *MetricsCollectionResults) Parse() (parsed *ParsedMetricsResults, err er for metricName, family := range metricFamilies { samples, err := expfmt.ExtractSamples(&expfmt.DecodeOptions{}, family) if err != nil { + klog.Errorf("error extracting metric sample for %q: %v", metricName, err) continue } for _, sample := range samples { @@ -116,7 +117,6 @@ func (r *MetricsCollectionResults) Parse() (parsed *ParsedMetricsResults, err er Value: sample.Value, Attributes: model.LabelSet(sample.Metric), }) - } } } From 42ec7875b408b4a29e0ba1ebd31c31e9a409137d Mon Sep 17 00:00:00 2001 From: James McDermott Date: Tue, 9 Jun 2026 21:02:17 +0100 Subject: [PATCH 12/21] Introduce retrigger.txt for easier CI retriggering Signed-off-by: James McDermott --- .github/retrigger.txt | 1 + 1 file changed, 1 insertion(+) create mode 100644 .github/retrigger.txt diff --git a/.github/retrigger.txt b/.github/retrigger.txt new file mode 100644 index 000000000..3e269cbda --- /dev/null +++ b/.github/retrigger.txt @@ -0,0 +1 @@ +Push a change to this file to retrigger GitHub CI \ No newline at end of file From 30cf8e5e34249f58d2869bdd375ed9aa7937a7c2 Mon Sep 17 00:00:00 2001 From: James McDermott Date: Wed, 10 Jun 2026 14:12:05 +0100 Subject: [PATCH 13/21] Address copilot review comments part 7 Signed-off-by: James McDermott --- .../grafana-package-sizes-dashboard.json | 49 +++++++++++++++++++ deployments/metrics/grafana-deployment.yaml | 26 +++++++++- internal/telemetry/metrics_test.go | 6 ++- internal/telemetry/otel.go | 2 +- scripts/deploy-monitoring.sh | 19 ++++--- 5 files changed, 92 insertions(+), 10 deletions(-) create mode 100644 deployments/metrics-resources/grafana-package-sizes-dashboard.json diff --git a/deployments/metrics-resources/grafana-package-sizes-dashboard.json b/deployments/metrics-resources/grafana-package-sizes-dashboard.json new file mode 100644 index 000000000..f4d84f6f0 --- /dev/null +++ b/deployments/metrics-resources/grafana-package-sizes-dashboard.json @@ -0,0 +1,49 @@ +{ + "annotations": { "list": [] }, + "editable": true, + "fiscalYearStartMonth": 0, + "graphTooltip": 1, + "id": null, + "links": [], + "liveNow": false, + "panels": [ + { + "datasource": { "type": "prometheus", "uid": "prometheus" }, + "description": "Total file size, in bytes, of package revisions' resources", + "fieldConfig": { + "defaults": { + "color": { "mode": "palette-classic" }, + "custom": { "axisCenteredZero": false, "axisLabel": "", "axisPlacement": "auto", "drawStyle": "line", "fillOpacity": 0, "gradientMode": "none", "lineInterpolation": "linear", "lineWidth": 1, "pointSize": 5, "scaleDistribution": { "type": "linear" }, "showPoints": "auto", "spanNulls": false, "stacking": { "group": "A", "mode": "none" }, "thresholdsStyle": { "mode": "off" } }, + "unit": "decbytes" + }, + "overrides": [] + }, + "gridPos": { "h": 100, "w": 24, "x": 0, "y": 0 }, + "id": 101, + "options": { "legend": { "displayMode": "list", "placement": "bottom", "showLegend": true }, "tooltip": { "mode": "multi", "sort": "none" } }, + "targets": [ + { + "datasource": { "type": "prometheus", "uid": "prometheus" }, + "expr": "porch_package_size_bytes_total", + "legendFormat": "{{namespace}}/{{package}}/{{workspaceName}}", + "range": true, + "refId": "total-size-gauge" + } + ], + "title": "PR Resource Sizes", + "type": "timeseries" + } + ], + "refresh": "10s", + "schemaVersion": 38, + "style": "dark", + "tags": ["porch", "resources"], + "templating": { "list": [] }, + "time": { "from": "now-1h", "to": "now" }, + "timepicker": {}, + "timezone": "", + "title": "Porch File-system Resources", + "uid": "porch-package-resources", + "version": 1, + "weekStart": "" +} \ No newline at end of file diff --git a/deployments/metrics/grafana-deployment.yaml b/deployments/metrics/grafana-deployment.yaml index 814e23bed..bc452e5e3 100644 --- a/deployments/metrics/grafana-deployment.yaml +++ b/deployments/metrics/grafana-deployment.yaml @@ -38,9 +38,19 @@ spec: - name: GF_AUTH_ANONYMOUS_ENABLED value: "true" - name: GF_AUTH_ANONYMOUS_ORG_ROLE - value: "Admin" + value: "Viewer" - name: GF_AUTH_DISABLE_LOGIN_FORM - value: "true" + value: "false" + - name: GF_SECURITY_ADMIN_USER + valueFrom: + secretKeyRef: + name: grafana-admin-creds + key: GF_SECURITY_ADMIN_USER + - name: GF_SECURITY_ADMIN_PASSWORD + valueFrom: + secretKeyRef: + name: grafana-admin-creds + key: GF_SECURITY_ADMIN_PASSWORD - name: GF_USERS_ALLOW_SIGN_UP value: "false" - name: GF_DASHBOARDS_DEFAULT_HOME_DASHBOARD_PATH @@ -144,6 +154,18 @@ data: backendType: pyroscope --- apiVersion: v1 +data: + GF_SECURITY_ADMIN_USER: cG9yY2g= # kpt-set: ${grafana-user} + GF_SECURITY_ADMIN_PASSWORD: cG9yY2g= # kpt-set: ${grafana-pw} +kind: Secret +metadata: + name: grafana-admin-creds + namespace: porch-monitoring # kpt-set: ${namespace} + labels: + app: grafana +type: Opaque +--- +apiVersion: v1 kind: Service metadata: name: grafana diff --git a/internal/telemetry/metrics_test.go b/internal/telemetry/metrics_test.go index 2dfa43865..2e34aef73 100644 --- a/internal/telemetry/metrics_test.go +++ b/internal/telemetry/metrics_test.go @@ -62,10 +62,14 @@ func TestRecordPackageRevisionResourcesSize_NilInstruments(t *testing.T) { } func TestRecordPackageRevisionResourcesSize_RecordsMetrics(t *testing.T) { + previousMp := otel.GetMeterProvider() reader := sdkmetric.NewManualReader() mp := sdkmetric.NewMeterProvider(sdkmetric.WithReader(reader)) otel.SetMeterProvider(mp) - defer mp.Shutdown(context.Background()) + defer func() { + otel.SetMeterProvider(previousMp) + mp.Shutdown(context.Background()) + }() require.NoError(t, InitMetrics()) diff --git a/internal/telemetry/otel.go b/internal/telemetry/otel.go index cc1ba9b10..713e0443c 100644 --- a/internal/telemetry/otel.go +++ b/internal/telemetry/otel.go @@ -155,7 +155,7 @@ func setupMetrics(ctx context.Context, res *OTelResources) error { }) readers := []sdkmetric.Option{} - if exporter == "prometheus" || os.Getenv(otelPortEnv) != "" { + if exporter == "prometheus" || (os.Getenv(otelHostEnv) != "" && os.Getenv(otelPortEnv) != "") { // Only create the Prometheus exporter when we intend to expose a scrape // endpoint, to avoid writing OTel metrics into the default Prometheus diff --git a/scripts/deploy-monitoring.sh b/scripts/deploy-monitoring.sh index 057aa7a35..d02baa01c 100755 --- a/scripts/deploy-monitoring.sh +++ b/scripts/deploy-monitoring.sh @@ -31,11 +31,15 @@ PROMETHEUS_NODEPORT="${PROMETHEUS_NODEPORT:-30091}" GRAFANA_LOCAL_PORT="${GRAFANA_LOCAL_PORT:-3001}" GRAFANA_CONTAINER_PORT="${GRAFANA_CONTAINER_PORT:-3000}" GRAFANA_NODEPORT="${GRAFANA_NODEPORT:-30301}" +GRAFANA_ADMIN_USER="${GRAFANA_ADMIN_USER:-porch}" +GRAFANA_ADMIN_PW="${GRAFANA_ADMIN_PW:-porch}" DOCKERHUB_MIRROR="${DOCKERHUB_MIRROR:-docker.io}" KRM_FN_REGISTRY_URL="${KRM_FN_REGISTRY_URL:-ghcr.io/kptdev/krm-functions-catalog}" -PROMETHEUS_IMAGE="${DOCKERHUB_MIRROR}/prom/prometheus:latest" -GRAFANA_IMAGE="${DOCKERHUB_MIRROR}/grafana/grafana:latest" +PROMETHEUS_VERSION="${PROMETHEUS_VERSION:-latest}" +PROMETHEUS_IMAGE="${DOCKERHUB_MIRROR}/prom/prometheus:${PROMETHEUS_VERSION}" +GRAFANA_VERSION="${GRAFANA_VERSION:-latest}" +GRAFANA_IMAGE="${DOCKERHUB_MIRROR}/grafana/grafana:${GRAFANA_VERSION}" RED='\033[0;31m' GREEN='\033[0;32m' @@ -87,6 +91,8 @@ pipeline: grafana-container-port: "${GRAFANA_CONTAINER_PORT}" prometheus-nodeport: "${PROMETHEUS_NODEPORT}" grafana-nodeport: "${GRAFANA_NODEPORT}" + grafana-user: "$(echo -n "$GRAFANA_ADMIN_USER" | base64)" + grafana-pw: "$(echo -n "$GRAFANA_ADMIN_PW" | base64)" - image: ${KRM_FN_REGISTRY_URL}/set-namespace:v0.4.1 configMap: namespace: ${NAMESPACE} @@ -193,8 +199,8 @@ get_service_urls() { log_info "Access via port-forward (recommended):" log_info " Prometheus: ${PROMETHEUS_URL}" log_info " Grafana: ${GRAFANA_URL}" - log_info " Username: admin" - log_info " Password: admin" + log_info " Username: ${GRAFANA_ADMIN_USER}" + log_info " Password: ${GRAFANA_ADMIN_PW}" echo "" log_info "Or access via NodePort:" log_info " Prometheus: ${PROMETHEUS_NODEPORT_URL}" @@ -221,9 +227,10 @@ cleanup() { kubectl delete deployment prometheus grafana -n "$NAMESPACE" --ignore-not-found=true kubectl delete service prometheus grafana -n "$NAMESPACE" --ignore-not-found=true kubectl delete configmap prometheus-config grafana-dashboards grafana-dashboards-provider grafana-datasources -n "$NAMESPACE" --ignore-not-found=true + kubectl delete secret grafana-admin-creds -n "$NAMESPACE" --ignore-not-found=true kubectl delete serviceaccount prometheus -n "$NAMESPACE" --ignore-not-found=true - kubectl delete clusterrole prometheus -n "$NAMESPACE" --ignore-not-found=true - kubectl delete clusterrolebinding prometheus -n "$NAMESPACE" --ignore-not-found=true + kubectl delete clusterrole prometheus --ignore-not-found=true + kubectl delete clusterrolebinding prometheus --ignore-not-found=true log_info "Deleting namespace $NAMESPACE..." kubectl delete namespace "$NAMESPACE" --ignore-not-found=true From b4a2bdfc96726598484834d2db3602eb11121abe Mon Sep 17 00:00:00 2001 From: ezmcdja Date: Thu, 11 Jun 2026 16:55:30 +0100 Subject: [PATCH 14/21] Address Copilot review comments part 8 Signed-off-by: ezmcdja --- deployments/metrics/grafana-deployment.yaml | 34 ------------------- .../metrics/prometheus-deployment.yaml | 3 +- internal/telemetry/otel.go | 7 ---- internal/telemetry/otel_test.go | 8 ++--- scripts/deploy-monitoring.sh | 17 ++++------ test/e2e/suiteutils/suite_utils.go | 8 ++--- 6 files changed, 14 insertions(+), 63 deletions(-) diff --git a/deployments/metrics/grafana-deployment.yaml b/deployments/metrics/grafana-deployment.yaml index bc452e5e3..86e0a85a8 100644 --- a/deployments/metrics/grafana-deployment.yaml +++ b/deployments/metrics/grafana-deployment.yaml @@ -35,12 +35,6 @@ spec: - containerPort: 3000 # kpt-set: ${grafana-container-port} name: http env: - - name: GF_AUTH_ANONYMOUS_ENABLED - value: "true" - - name: GF_AUTH_ANONYMOUS_ORG_ROLE - value: "Viewer" - - name: GF_AUTH_DISABLE_LOGIN_FORM - value: "false" - name: GF_SECURITY_ADMIN_USER valueFrom: secretKeyRef: @@ -141,17 +135,6 @@ data: url: http://prometheus:9090 isDefault: true editable: true - pyroscope.yaml: | - apiVersion: 1 - datasources: - - name: Pyroscope - type: phlare - access: proxy - uid: pyroscope - url: http://pyroscope:4040 - editable: true - jsonData: - backendType: pyroscope --- apiVersion: v1 data: @@ -164,20 +147,3 @@ metadata: labels: app: grafana type: Opaque ---- -apiVersion: v1 -kind: Service -metadata: - name: grafana - namespace: porch-monitoring # kpt-set: ${namespace} - labels: - app: grafana -spec: - type: NodePort - ports: - - port: 3000 # kpt-set: ${grafana-container-port} - targetPort: 3000 # kpt-set: ${grafana-container-port} - nodePort: 30301 # kpt-set: ${grafana-nodeport} - name: http - selector: - app: grafana diff --git a/deployments/metrics/prometheus-deployment.yaml b/deployments/metrics/prometheus-deployment.yaml index 29d059550..187540380 100644 --- a/deployments/metrics/prometheus-deployment.yaml +++ b/deployments/metrics/prometheus-deployment.yaml @@ -108,11 +108,10 @@ metadata: labels: app: prometheus spec: - type: NodePort + type: ClusterIP ports: - port: 9090 # kpt-set: ${prometheus-container-port} targetPort: 9090 # kpt-set: ${prometheus-container-port} - nodePort: 30091 # kpt-set: ${prometheus-nodeport} name: http selector: app: prometheus diff --git a/internal/telemetry/otel.go b/internal/telemetry/otel.go index 713e0443c..fc81de53d 100644 --- a/internal/telemetry/otel.go +++ b/internal/telemetry/otel.go @@ -49,7 +49,6 @@ type OTelResources struct { metricsPort int meterProvider *sdkmetric.MeterProvider tracerProvider *trace.TracerProvider - metricReader sdkmetric.Reader } // Shutdown gracefully shuts down all OpenTelemetry resources. @@ -61,11 +60,6 @@ func (r *OTelResources) Shutdown(ctx context.Context) error { errs = append(errs, fmt.Errorf("metrics server shutdown: %w", err)) } } - if r.metricReader != nil { - if err := r.metricReader.Shutdown(ctx); err != nil { - errs = append(errs, fmt.Errorf("metric reader shutdown: %w", err)) - } - } if r.meterProvider != nil { if err := r.meterProvider.Shutdown(ctx); err != nil { errs = append(errs, fmt.Errorf("meter provider shutdown: %w", err)) @@ -172,7 +166,6 @@ func setupMetrics(ctx context.Context, res *OTelResources) error { if err != nil { return fmt.Errorf("failed to create metric reader: %w", err) } - res.metricReader = autoMr readers = append(readers, sdkmetric.WithReader(autoMr)) } diff --git a/internal/telemetry/otel_test.go b/internal/telemetry/otel_test.go index be94cb206..1e47a9972 100644 --- a/internal/telemetry/otel_test.go +++ b/internal/telemetry/otel_test.go @@ -107,7 +107,7 @@ func TestOtelMetricsPushHTTP(t *testing.T) { require.NoError(t, err) // Shutdown flushes the periodic reader, which triggers the export - res.ShutdownWithTimeout(5 * time.Second) + require.NoError(t, res.ShutdownWithTimeout(5*time.Second)) <-requestWaitChannel } @@ -134,7 +134,7 @@ func TestOtelTracesPushHTTP(t *testing.T) { span.End() // Shutdown flushes the batch span processor - res.ShutdownWithTimeout(5 * time.Second) + require.NoError(t, res.ShutdownWithTimeout(5*time.Second)) <-requestWaitChannel } func TestSetupOpenTelemetryPrometheusEndpoint(t *testing.T) { @@ -183,7 +183,7 @@ func TestOtelMetricsPushGRPC(t *testing.T) { res, err := SetupOpenTelemetry(ctx) require.NoError(t, err) - res.ShutdownWithTimeout(5 * time.Second) + require.NoError(t, res.ShutdownWithTimeout(5*time.Second)) <-requestWaitChannel } @@ -220,7 +220,7 @@ func TestOtelTracesPushGRPC(t *testing.T) { _, span := tracer.Start(ctx, "test-span") span.End() - res.ShutdownWithTimeout(5 * time.Second) + require.NoError(t, res.ShutdownWithTimeout(5*time.Second)) <-requestWaitChannel } diff --git a/scripts/deploy-monitoring.sh b/scripts/deploy-monitoring.sh index d02baa01c..2da415f57 100755 --- a/scripts/deploy-monitoring.sh +++ b/scripts/deploy-monitoring.sh @@ -176,9 +176,9 @@ get_service_urls() { stop_port_forwards sleep 2 - kubectl port-forward -n "${NAMESPACE}" svc/prometheus "${PROMETHEUS_LOCAL_PORT}":"${PROMETHEUS_CONTAINER_PORT}" > /dev/null 2>&1 & + kubectl port-forward -n "${NAMESPACE}" deployment/prometheus "${PROMETHEUS_LOCAL_PORT}":"${PROMETHEUS_CONTAINER_PORT}" > /dev/null 2>&1 & PROMETHEUS_PF_PID=$! - kubectl port-forward -n "${NAMESPACE}" svc/grafana "${GRAFANA_LOCAL_PORT}":"${GRAFANA_CONTAINER_PORT}" > /dev/null 2>&1 & + kubectl port-forward -n "${NAMESPACE}" deployment/grafana "${GRAFANA_LOCAL_PORT}":"${GRAFANA_CONTAINER_PORT}" > /dev/null 2>&1 & GRAFANA_PF_PID=$! sleep 2 @@ -188,8 +188,6 @@ get_service_urls() { PROMETHEUS_URL="http://localhost:${PROMETHEUS_LOCAL_PORT}" GRAFANA_URL="http://localhost:${GRAFANA_LOCAL_PORT}" - PROMETHEUS_NODEPORT_URL="http://localhost:${PROMETHEUS_NODEPORT}" - GRAFANA_NODEPORT_URL="http://localhost:${GRAFANA_NODEPORT}" echo "" log_info "==========================================" @@ -202,13 +200,10 @@ get_service_urls() { log_info " Username: ${GRAFANA_ADMIN_USER}" log_info " Password: ${GRAFANA_ADMIN_PW}" echo "" - log_info "Or access via NodePort:" - log_info " Prometheus: ${PROMETHEUS_NODEPORT_URL}" - log_info " Grafana: ${GRAFANA_NODEPORT_URL}" - echo "" - log_info " - Prometheus scraping metrics from porch-server port 9464" - log_info " - Prometheus scraping metrics from porch-controller port 9464" - log_info " - Prometheus scraping metrics from function-runner port 9464" + log_info " - Prometheus is scraping metrics from:" + log_info " - porch-server port 9464" + log_info " - porch-controller port 9464" + log_info " - function-runner port 9464" log_info "" echo "" log_info "To stop port forwarding, run:" diff --git a/test/e2e/suiteutils/suite_utils.go b/test/e2e/suiteutils/suite_utils.go index 26c0e2950..c5295961f 100644 --- a/test/e2e/suiteutils/suite_utils.go +++ b/test/e2e/suiteutils/suite_utils.go @@ -45,7 +45,6 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/wait" - "k8s.io/klog/v2" "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/yaml" @@ -79,8 +78,8 @@ type MetricResult struct { Attributes model.LabelSet } -func (r *MetricsCollectionResults) Parse() (parsed *ParsedMetricsResults, err error) { - parsed = &ParsedMetricsResults{ +func (r *MetricsCollectionResults) Parse() (*ParsedMetricsResults, error) { + parsed := &ParsedMetricsResults{ PorchServerMetrics: make(map[string][]MetricResult), PorchControllerMetrics: make(map[string][]MetricResult), PorchFunctionRunnerMetrics: make(map[string][]MetricResult), @@ -109,8 +108,7 @@ func (r *MetricsCollectionResults) Parse() (parsed *ParsedMetricsResults, err er for metricName, family := range metricFamilies { samples, err := expfmt.ExtractSamples(&expfmt.DecodeOptions{}, family) if err != nil { - klog.Errorf("error extracting metric sample for %q: %v", metricName, err) - continue + return nil, fmt.Errorf("error extracting metric sample for %q: %v", metricName, err) } for _, sample := range samples { pair.parsedResult[metricName] = append(pair.parsedResult[metricName], MetricResult{ From 5439633aa5c22845ac504dc3762716f59c47bd5d Mon Sep 17 00:00:00 2001 From: ezmcdja Date: Thu, 11 Jun 2026 19:11:51 +0100 Subject: [PATCH 15/21] Address Copilot review comments part 9 Signed-off-by: ezmcdja --- .../configurations/opentelemetry.md | 12 ++++++------ internal/telemetry/metrics.go | 2 +- test/e2e/api/metrics_test.go | 6 +++--- test/e2e/suiteutils/suite_utils.go | 13 +++++++------ 4 files changed, 17 insertions(+), 16 deletions(-) diff --git a/docs/content/en/docs/6_configuration_and_deployments/configurations/opentelemetry.md b/docs/content/en/docs/6_configuration_and_deployments/configurations/opentelemetry.md index 5e59da418..d9efd651b 100644 --- a/docs/content/en/docs/6_configuration_and_deployments/configurations/opentelemetry.md +++ b/docs/content/en/docs/6_configuration_and_deployments/configurations/opentelemetry.md @@ -424,12 +424,12 @@ Porch records the following metrics via OpenTelemetry: Package size metrics are recorded with the following attributes from the relevant package: -| Attribute | Description | -|-----------------|-------------| -| `namespace` | Kubernetes namespace of the package revision | -| `repository` | Name of the repository containing the package | -| `package` | Path and name of the package | -| `workspaceName` | A short, unique description of the changes contained in the package revision | +| Attribute | Description | +|------------------|-------------| +| `namespace` | Kubernetes namespace of the package revision | +| `repository` | Name of the repository containing the package | +| `package` | Path and name of the package | +| `workspace_name` | WorkspaceName of the package revision - short, unique description of the changes | These metrics are recorded as part of every flow that updates package revision resources: - Create package revision diff --git a/internal/telemetry/metrics.go b/internal/telemetry/metrics.go index e8d9829e3..383a7d9de 100644 --- a/internal/telemetry/metrics.go +++ b/internal/telemetry/metrics.go @@ -70,7 +70,7 @@ func RecordPackageRevisionResourcesSize(prKey repository.PackageRevisionKey, res attribute.String("namespace", prKey.RKey().Namespace), attribute.String("repository", prKey.RKey().Name), attribute.String("package", prPath+prKey.PKey().Package), - attribute.String("workspaceName", prKey.WorkspaceName), + attribute.String("workspace_name", prKey.WorkspaceName), ) if prResourceSizeHistogram == nil { diff --git a/test/e2e/api/metrics_test.go b/test/e2e/api/metrics_test.go index 0838d5195..636cf589d 100644 --- a/test/e2e/api/metrics_test.go +++ b/test/e2e/api/metrics_test.go @@ -209,11 +209,11 @@ func (t *PorchSuite) validateSizeMetric(pr *porchapi.PackageRevision, metricName metric := podParsedResults[metricName] metric = slices.DeleteFunc(metric, func(aMetric suiteutils.MetricResult) bool { return !(aMetric.Attributes["namespace"] == model.LabelValue(t.Namespace) && - aMetric.Attributes["package"] == model.LabelValue(pr.Spec.PackageName) && aMetric.Attributes["repository"] == model.LabelValue(pr.Spec.RepositoryName) && - aMetric.Attributes["workspaceName"] == model.LabelValue(pr.Spec.WorkspaceName)) + aMetric.Attributes["package"] == model.LabelValue(pr.Spec.PackageName) && + aMetric.Attributes["workspace_name"] == model.LabelValue(pr.Spec.WorkspaceName)) }) - t.Require().Len(metric, 1) + t.Require().Len(metric, 1, "Expected metrics to include %q entry with {namespace=%q, package=%q, repository=%q, workspace_name=%q}, but did not", metricName, t.Namespace, pr.Spec.RepositoryName, pr.Spec.PackageName, pr.Spec.WorkspaceName) t.Assert().EqualValues(model.SampleValue(pr.Status.ResourcesSizeBytes), metric[0].Value) } else { t.Assert().EqualValues(0, pr.Status.ResourcesSizeBytes, "PackageRevision resources size should not be available in non-DB cache deployment") diff --git a/test/e2e/suiteutils/suite_utils.go b/test/e2e/suiteutils/suite_utils.go index c5295961f..098829050 100644 --- a/test/e2e/suiteutils/suite_utils.go +++ b/test/e2e/suiteutils/suite_utils.go @@ -90,11 +90,12 @@ func (r *MetricsCollectionResults) Parse() (*ParsedMetricsResults, error) { for _, pair := range []struct { raw string parsedResult map[string][]MetricResult + nameForError string }{ - {r.PorchServerMetrics, parsed.PorchServerMetrics}, - {r.PorchControllerMetrics, parsed.PorchControllerMetrics}, - {r.PorchFunctionRunnerMetrics, parsed.PorchFunctionRunnerMetrics}, - {r.PorchWrapperServerMetrics, parsed.PorchWrapperServerMetrics}, + {r.PorchServerMetrics, parsed.PorchServerMetrics, "PorchServerMetrics"}, + {r.PorchControllerMetrics, parsed.PorchControllerMetrics, "PorchControllerMetrics"}, + {r.PorchFunctionRunnerMetrics, parsed.PorchFunctionRunnerMetrics, "PorchFunctionRunnerMetrics"}, + {r.PorchWrapperServerMetrics, parsed.PorchWrapperServerMetrics, "PorchWrapperServerMetrics"}, } { if pair.raw == "" { continue @@ -102,13 +103,13 @@ func (r *MetricsCollectionResults) Parse() (*ParsedMetricsResults, error) { metricFamilies, err := metricsParser.TextToMetricFamilies(strings.NewReader(pair.raw)) if err != nil { - return nil, err + return nil, fmt.Errorf("error extracting metrics from %s text: %w", pair.nameForError, err) } for metricName, family := range metricFamilies { samples, err := expfmt.ExtractSamples(&expfmt.DecodeOptions{}, family) if err != nil { - return nil, fmt.Errorf("error extracting metric sample for %q: %v", metricName, err) + return nil, fmt.Errorf("error extracting %s metric sample for %q: %w", pair.nameForError, metricName, err) } for _, sample := range samples { pair.parsedResult[metricName] = append(pair.parsedResult[metricName], MetricResult{ From aa5cceae2145f1988288dfa5883d0d30f305f89c Mon Sep 17 00:00:00 2001 From: ezmcdja Date: Thu, 11 Jun 2026 21:29:55 +0100 Subject: [PATCH 16/21] retrigger Signed-off-by: ezmcdja --- .github/retrigger.txt | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.github/retrigger.txt b/.github/retrigger.txt index 3e269cbda..0bc7c4689 100644 --- a/.github/retrigger.txt +++ b/.github/retrigger.txt @@ -1 +1,3 @@ -Push a change to this file to retrigger GitHub CI \ No newline at end of file +Push a change to this file to retrigger GitHub CI + +retrigger \ No newline at end of file From 9463c6d2e9f98c9d239eb239f42fd08947c6e69b Mon Sep 17 00:00:00 2001 From: ezmcdja Date: Thu, 11 Jun 2026 21:50:14 +0100 Subject: [PATCH 17/21] Address Copilot review comments part 10 Signed-off-by: ezmcdja --- .../metrics-resources/grafana-package-sizes-dashboard.json | 2 +- scripts/deploy-monitoring.sh | 5 ++++- test/e2e/api/metrics_test.go | 2 +- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/deployments/metrics-resources/grafana-package-sizes-dashboard.json b/deployments/metrics-resources/grafana-package-sizes-dashboard.json index f4d84f6f0..68aebf826 100644 --- a/deployments/metrics-resources/grafana-package-sizes-dashboard.json +++ b/deployments/metrics-resources/grafana-package-sizes-dashboard.json @@ -25,7 +25,7 @@ { "datasource": { "type": "prometheus", "uid": "prometheus" }, "expr": "porch_package_size_bytes_total", - "legendFormat": "{{namespace}}/{{package}}/{{workspaceName}}", + "legendFormat": "{{namespace}}/{{package}}/{{workspace_name}}", "range": true, "refId": "total-size-gauge" } diff --git a/scripts/deploy-monitoring.sh b/scripts/deploy-monitoring.sh index 2da415f57..ac35b9a4f 100755 --- a/scripts/deploy-monitoring.sh +++ b/scripts/deploy-monitoring.sh @@ -31,8 +31,10 @@ PROMETHEUS_NODEPORT="${PROMETHEUS_NODEPORT:-30091}" GRAFANA_LOCAL_PORT="${GRAFANA_LOCAL_PORT:-3001}" GRAFANA_CONTAINER_PORT="${GRAFANA_CONTAINER_PORT:-3000}" GRAFANA_NODEPORT="${GRAFANA_NODEPORT:-30301}" + GRAFANA_ADMIN_USER="${GRAFANA_ADMIN_USER:-porch}" -GRAFANA_ADMIN_PW="${GRAFANA_ADMIN_PW:-porch}" +GRAFANA_ADMIN_PW="${GRAFANA_ADMIN_PW:-}" +[[ -z $GRAFANA_ADMIN_PW ]] && GRAFANA_ADMIN_PW="$(date +%s | sha256sum | base64 | head -c 15)" DOCKERHUB_MIRROR="${DOCKERHUB_MIRROR:-docker.io}" KRM_FN_REGISTRY_URL="${KRM_FN_REGISTRY_URL:-ghcr.io/kptdev/krm-functions-catalog}" @@ -199,6 +201,7 @@ get_service_urls() { log_info " Grafana: ${GRAFANA_URL}" log_info " Username: ${GRAFANA_ADMIN_USER}" log_info " Password: ${GRAFANA_ADMIN_PW}" + log_info " stored in: kubectl -n porch-monitoring get secrets --selector app=grafana -o yaml" echo "" log_info " - Prometheus is scraping metrics from:" log_info " - porch-server port 9464" diff --git a/test/e2e/api/metrics_test.go b/test/e2e/api/metrics_test.go index 636cf589d..0e7b3e237 100644 --- a/test/e2e/api/metrics_test.go +++ b/test/e2e/api/metrics_test.go @@ -213,7 +213,7 @@ func (t *PorchSuite) validateSizeMetric(pr *porchapi.PackageRevision, metricName aMetric.Attributes["package"] == model.LabelValue(pr.Spec.PackageName) && aMetric.Attributes["workspace_name"] == model.LabelValue(pr.Spec.WorkspaceName)) }) - t.Require().Len(metric, 1, "Expected metrics to include %q entry with {namespace=%q, package=%q, repository=%q, workspace_name=%q}, but did not", metricName, t.Namespace, pr.Spec.RepositoryName, pr.Spec.PackageName, pr.Spec.WorkspaceName) + t.Require().GreaterOrEqualf(metric, 1, "Expected metrics to include %q entry with {namespace=%q, package=%q, repository=%q, workspace_name=%q}, but did not", metricName, t.Namespace, pr.Spec.RepositoryName, pr.Spec.PackageName, pr.Spec.WorkspaceName) t.Assert().EqualValues(model.SampleValue(pr.Status.ResourcesSizeBytes), metric[0].Value) } else { t.Assert().EqualValues(0, pr.Status.ResourcesSizeBytes, "PackageRevision resources size should not be available in non-DB cache deployment") From 2830a6a644d5810a742f4b128a4335ffe1797b1b Mon Sep 17 00:00:00 2001 From: ezmcdja Date: Thu, 11 Jun 2026 21:56:33 +0100 Subject: [PATCH 18/21] retrigger Signed-off-by: ezmcdja --- .github/retrigger.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/retrigger.txt b/.github/retrigger.txt index 0bc7c4689..173b9d724 100644 --- a/.github/retrigger.txt +++ b/.github/retrigger.txt @@ -1,3 +1,4 @@ Push a change to this file to retrigger GitHub CI +retrigger retrigger \ No newline at end of file From 82694c6d31b58c356fd0404498797703486b8c88 Mon Sep 17 00:00:00 2001 From: ezmcdja Date: Fri, 12 Jun 2026 10:02:49 +0100 Subject: [PATCH 19/21] Address Copilot review comments part 11 Signed-off-by: ezmcdja --- scripts/deploy-monitoring.sh | 11 ++++++----- test/e2e/api/metrics_test.go | 2 +- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/scripts/deploy-monitoring.sh b/scripts/deploy-monitoring.sh index ac35b9a4f..7c57da9b5 100755 --- a/scripts/deploy-monitoring.sh +++ b/scripts/deploy-monitoring.sh @@ -34,7 +34,7 @@ GRAFANA_NODEPORT="${GRAFANA_NODEPORT:-30301}" GRAFANA_ADMIN_USER="${GRAFANA_ADMIN_USER:-porch}" GRAFANA_ADMIN_PW="${GRAFANA_ADMIN_PW:-}" -[[ -z $GRAFANA_ADMIN_PW ]] && GRAFANA_ADMIN_PW="$(date +%s | sha256sum | base64 | head -c 15)" +[[ -z $GRAFANA_ADMIN_PW ]] && GRAFANA_ADMIN_PW="$(date +%s | shasum -a 256 | base64 | head -c 15)" DOCKERHUB_MIRROR="${DOCKERHUB_MIRROR:-docker.io}" KRM_FN_REGISTRY_URL="${KRM_FN_REGISTRY_URL:-ghcr.io/kptdev/krm-functions-catalog}" @@ -162,13 +162,14 @@ deploy_monitoring() { wait_for_deployment() { local deployment=$1 log_info "Waiting for $deployment to be ready..." - kubectl wait --for=condition=available --timeout=300s deployment/$deployment -n "$NAMESPACE" + kubectl wait --for=condition=available --timeout=300s deployment/"$deployment" -n "$NAMESPACE" } stop_port_forwards() { - find /tmp/tmp*_porch-monitoring-pf.pid.d/ -name '*.pid' -exec pkill --pidfile '{}' \; 2>/dev/null || true - find /tmp/tmp*_porch-monitoring-pf.pid.d/ -name '*.pid' ! -wholename "${PORT_FORWARD_DIR}*" -exec rm '{}' \; 2>/dev/null || true - find /tmp/tmp*_porch-monitoring-pf.pid.d/ -type d ! -wholename "${PORT_FORWARD_DIR}*" -exec rmdir '{}' \; 2>/dev/null || true + if find /tmp/tmp*_porch-monitoring-pf.pid.d/ -name '*.pid' -exec pkill -F '{}' \; 2>/dev/null; then + find /tmp/tmp*_porch-monitoring-pf.pid.d/ -name '*.pid' ! -wholename "${PORT_FORWARD_DIR}*" -exec rm '{}' \; 2>/dev/null || true + find /tmp/tmp*_porch-monitoring-pf.pid.d/ -type d ! -wholename "${PORT_FORWARD_DIR}*" -exec rmdir '{}' \; 2>/dev/null || true + fi } get_service_urls() { diff --git a/test/e2e/api/metrics_test.go b/test/e2e/api/metrics_test.go index 0e7b3e237..52d0095d2 100644 --- a/test/e2e/api/metrics_test.go +++ b/test/e2e/api/metrics_test.go @@ -213,7 +213,7 @@ func (t *PorchSuite) validateSizeMetric(pr *porchapi.PackageRevision, metricName aMetric.Attributes["package"] == model.LabelValue(pr.Spec.PackageName) && aMetric.Attributes["workspace_name"] == model.LabelValue(pr.Spec.WorkspaceName)) }) - t.Require().GreaterOrEqualf(metric, 1, "Expected metrics to include %q entry with {namespace=%q, package=%q, repository=%q, workspace_name=%q}, but did not", metricName, t.Namespace, pr.Spec.RepositoryName, pr.Spec.PackageName, pr.Spec.WorkspaceName) + t.Require().GreaterOrEqualf(len(metric), 1, "Expected metrics to include %q entry with {namespace=%q, package=%q, repository=%q, workspace_name=%q}, but did not", metricName, t.Namespace, pr.Spec.RepositoryName, pr.Spec.PackageName, pr.Spec.WorkspaceName) t.Assert().EqualValues(model.SampleValue(pr.Status.ResourcesSizeBytes), metric[0].Value) } else { t.Assert().EqualValues(0, pr.Status.ResourcesSizeBytes, "PackageRevision resources size should not be available in non-DB cache deployment") From b2535aeebf765b58499510fdd6bf08bd6f652ae5 Mon Sep 17 00:00:00 2001 From: James McDermott Date: Mon, 15 Jun 2026 09:01:34 +0100 Subject: [PATCH 20/21] Address review comment Signed-off-by: James McDermott --- .github/retrigger.txt | 4 ---- 1 file changed, 4 deletions(-) delete mode 100644 .github/retrigger.txt diff --git a/.github/retrigger.txt b/.github/retrigger.txt deleted file mode 100644 index 173b9d724..000000000 --- a/.github/retrigger.txt +++ /dev/null @@ -1,4 +0,0 @@ -Push a change to this file to retrigger GitHub CI - -retrigger -retrigger \ No newline at end of file From cf910a7debcb32a5faff11f72a8f838745a34bd9 Mon Sep 17 00:00:00 2001 From: James McDermott Date: Mon, 15 Jun 2026 10:01:19 +0100 Subject: [PATCH 21/21] Address Copilot review comments part 12 Signed-off-by: James McDermott --- internal/telemetry/metrics.go | 6 +++--- internal/telemetry/metrics_test.go | 6 +++--- internal/telemetry/otel.go | 18 +++++++++++++++--- .../dbcache/dbpackagerevisionresourcessql.go | 2 +- pkg/cache/dbcache/dbrepository.go | 2 +- pkg/cache/dbcache/dbreposync.go | 2 +- scripts/deploy-monitoring.sh | 2 +- test/e2e/api/metrics_test.go | 2 +- 8 files changed, 26 insertions(+), 14 deletions(-) diff --git a/internal/telemetry/metrics.go b/internal/telemetry/metrics.go index 383a7d9de..91ac2c2d3 100644 --- a/internal/telemetry/metrics.go +++ b/internal/telemetry/metrics.go @@ -59,7 +59,7 @@ func InitMetrics() (err error) { } // Porch server and function runner metric recording functions -func RecordPackageRevisionResourcesSize(prKey repository.PackageRevisionKey, resourcesSize int64) { +func RecordPackageRevisionResourcesSize(ctx context.Context, prKey repository.PackageRevisionKey, resourcesSize int64) { prPath := func() string { if prKey.PKey().Path != "" { return prKey.PKey().Path + "/" @@ -84,11 +84,11 @@ func RecordPackageRevisionResourcesSize(prKey repository.PackageRevisionKey, res resourcesSize, attributes.MarshalLog()) } - prResourceSizeHistogram.Record(context.Background(), resourcesSize, metric.WithAttributeSet(attributes)) + prResourceSizeHistogram.Record(ctx, resourcesSize, metric.WithAttributeSet(attributes)) if prResourceSizeGauge == nil { klog.Warning("prResourceSizeGauge is nil - was InitMetrics() called?") return } - prResourceSizeGauge.Record(context.Background(), resourcesSize, metric.WithAttributeSet(attributes)) + prResourceSizeGauge.Record(ctx, resourcesSize, metric.WithAttributeSet(attributes)) } diff --git a/internal/telemetry/metrics_test.go b/internal/telemetry/metrics_test.go index 2e34aef73..923fee63e 100644 --- a/internal/telemetry/metrics_test.go +++ b/internal/telemetry/metrics_test.go @@ -51,14 +51,14 @@ func TestRecordPackageRevisionResourcesSize_NilInstruments(t *testing.T) { Revision: 1, } // Should return early without panic - assert.NotPanics(t, func() { RecordPackageRevisionResourcesSize(fake, 1024) }) + assert.NotPanics(t, func() { RecordPackageRevisionResourcesSize(context.Background(), fake, 1024) }) prResourceSizeHistogram = histogramBefore gaugeBefore := prResourceSizeGauge prResourceSizeGauge = nil defer func() { prResourceSizeGauge = gaugeBefore }() // Should return early without panic - assert.NotPanics(t, func() { RecordPackageRevisionResourcesSize(fake, 1024) }) + assert.NotPanics(t, func() { RecordPackageRevisionResourcesSize(context.Background(), fake, 1024) }) } func TestRecordPackageRevisionResourcesSize_RecordsMetrics(t *testing.T) { @@ -80,7 +80,7 @@ func TestRecordPackageRevisionResourcesSize_RecordsMetrics(t *testing.T) { Revision: 1, } - RecordPackageRevisionResourcesSize(fake, 4096) + RecordPackageRevisionResourcesSize(context.Background(), fake, 4096) var rm metricdata.ResourceMetrics require.NoError(t, reader.Collect(context.Background(), &rm)) diff --git a/internal/telemetry/otel.go b/internal/telemetry/otel.go index fc81de53d..c13bfac8e 100644 --- a/internal/telemetry/otel.go +++ b/internal/telemetry/otel.go @@ -38,8 +38,10 @@ import ( ) const ( - otelHostEnv = "OTEL_EXPORTER_PROMETHEUS_HOST" - otelPortEnv = "OTEL_EXPORTER_PROMETHEUS_PORT" + otelHostEnv = "OTEL_EXPORTER_PROMETHEUS_HOST" + otelHostDefault = "0.0.0.0" + otelPortEnv = "OTEL_EXPORTER_PROMETHEUS_PORT" + otelPortDefault = "9464" ) // OTelResources holds all OpenTelemetry resources that need lifecycle management. @@ -149,7 +151,17 @@ func setupMetrics(ctx context.Context, res *OTelResources) error { }) readers := []sdkmetric.Option{} - if exporter == "prometheus" || (os.Getenv(otelHostEnv) != "" && os.Getenv(otelPortEnv) != "") { + if exporter == "prometheus" { + if os.Getenv(otelHostEnv) == "" { + if err := os.Setenv(otelHostEnv, otelHostDefault); err != nil { + return err + } + } + if os.Getenv(otelPortEnv) == "" { + if err := os.Setenv(otelPortEnv, otelPortDefault); err != nil { + return err + } + } // Only create the Prometheus exporter when we intend to expose a scrape // endpoint, to avoid writing OTel metrics into the default Prometheus diff --git a/pkg/cache/dbcache/dbpackagerevisionresourcessql.go b/pkg/cache/dbcache/dbpackagerevisionresourcessql.go index f034e5c11..229d10abb 100644 --- a/pkg/cache/dbcache/dbpackagerevisionresourcessql.go +++ b/pkg/cache/dbcache/dbpackagerevisionresourcessql.go @@ -160,7 +160,7 @@ func pkgRevResourcesDeleteFromDB(ctx context.Context, prk repository.PackageRevi if err == nil { klog.V(5).Infof("pkgRevResourcesDeleteFromDB: deleted package revision resources for %+v", prk) - telemetry.RecordPackageRevisionResourcesSize(prk, 0) + telemetry.RecordPackageRevisionResourcesSize(ctx, prk, 0) } else { klog.Warningf("pkgRevResourcesDeleteFromDB: deletion of package revision resources for %+v failed: %q", prk, err) } diff --git a/pkg/cache/dbcache/dbrepository.go b/pkg/cache/dbcache/dbrepository.go index 32fbb2c98..48735a285 100644 --- a/pkg/cache/dbcache/dbrepository.go +++ b/pkg/cache/dbcache/dbrepository.go @@ -487,7 +487,7 @@ func (r *dbRepository) ClosePackageRevisionDraft(ctx context.Context, prd reposi return nil, err } - telemetry.RecordPackageRevisionResourcesSize(pr.Key(), pr.resourcesSizeBytes) + telemetry.RecordPackageRevisionResourcesSize(ctx, pr.Key(), pr.resourcesSizeBytes) if r.pushDraftsToGit && pr.gitPRDraft != nil && r.externalRepo != nil { gitPR, err := r.externalRepo.ClosePackageRevisionDraft(ctx, pr.gitPRDraft, 0) diff --git a/pkg/cache/dbcache/dbreposync.go b/pkg/cache/dbcache/dbreposync.go index f514ae806..ec8ee1827 100644 --- a/pkg/cache/dbcache/dbreposync.go +++ b/pkg/cache/dbcache/dbreposync.go @@ -244,7 +244,7 @@ func (s *repositorySync) cacheExternalPRs(ctx context.Context, externalPrMap map return err } - telemetry.RecordPackageRevisionResourcesSize(dbPR.Key(), dbPR.resourcesSizeBytes) + telemetry.RecordPackageRevisionResourcesSize(ctx, dbPR.Key(), dbPR.resourcesSizeBytes) } return nil diff --git a/scripts/deploy-monitoring.sh b/scripts/deploy-monitoring.sh index 7c57da9b5..83287754a 100755 --- a/scripts/deploy-monitoring.sh +++ b/scripts/deploy-monitoring.sh @@ -211,7 +211,7 @@ get_service_urls() { log_info "" echo "" log_info "To stop port forwarding, run:" - log_info " find /tmp/tmp*_porch-monitoring-pf.pid.d/ -name '*.pid' -exec pkill --pidfile '{}' \;" + log_info " find /tmp/tmp*_porch-monitoring-pf.pid.d/ -name '*.pid' -exec pkill -F '{}' \;" echo "" } diff --git a/test/e2e/api/metrics_test.go b/test/e2e/api/metrics_test.go index 52d0095d2..01ace799e 100644 --- a/test/e2e/api/metrics_test.go +++ b/test/e2e/api/metrics_test.go @@ -213,7 +213,7 @@ func (t *PorchSuite) validateSizeMetric(pr *porchapi.PackageRevision, metricName aMetric.Attributes["package"] == model.LabelValue(pr.Spec.PackageName) && aMetric.Attributes["workspace_name"] == model.LabelValue(pr.Spec.WorkspaceName)) }) - t.Require().GreaterOrEqualf(len(metric), 1, "Expected metrics to include %q entry with {namespace=%q, package=%q, repository=%q, workspace_name=%q}, but did not", metricName, t.Namespace, pr.Spec.RepositoryName, pr.Spec.PackageName, pr.Spec.WorkspaceName) + t.Require().Lenf(metric, 1, "Expected metrics to include exactly 1 %q entry with {namespace=%q, repository=%q, package=%q, workspace_name=%q}, but did not", metricName, t.Namespace, pr.Spec.RepositoryName, pr.Spec.PackageName, pr.Spec.WorkspaceName) t.Assert().EqualValues(model.SampleValue(pr.Status.ResourcesSizeBytes), metric[0].Value) } else { t.Assert().EqualValues(0, pr.Status.ResourcesSizeBytes, "PackageRevision resources size should not be available in non-DB cache deployment")