From 808139efb3755fa4e1cc02623063cff0ffa0dd10 Mon Sep 17 00:00:00 2001 From: Preetam Dwivedi Date: Wed, 30 Sep 2026 10:20:20 -0700 Subject: [PATCH 1/2] Fix telemetry metrics configuration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary ### Why? `telemetry.metrics = false` did not affect server startup or routing, so the recorder was always installed and `/metrics` remained available. ### What? Install the Prometheus recorder and authenticated route only when metrics are enabled. Reserve `/metrics` with an explicit 404 when disabled so the SPA owner route cannot handle it. Add enabled/disabled HTTP coverage to the fast suite. ## Test Plan - ✅ `cargo test -p walgit-server --test metrics --test routing_prefix` - ✅ `cargo clippy -p walgit-server --all-targets -- -D warnings` - ✅ `cargo fmt --all -- --check` ## Issue Closes #75 --- crates/walgit-server/src/lib.rs | 28 +++++++++++++++++++++++---- crates/walgit-server/src/metrics.rs | 11 +++++++---- crates/walgit-server/tests/metrics.rs | 22 +++++++++++++++++++++ justfile | 2 +- 4 files changed, 54 insertions(+), 9 deletions(-) create mode 100644 crates/walgit-server/tests/metrics.rs diff --git a/crates/walgit-server/src/lib.rs b/crates/walgit-server/src/lib.rs index 5f6c6154..8bb2cff7 100644 --- a/crates/walgit-server/src/lib.rs +++ b/crates/walgit-server/src/lib.rs @@ -89,7 +89,8 @@ pub struct AppState { /// HTTP requests in flight (counted until the response body is done); on the watchdog line. pub inflight: Arc, pub caches: cache::ServerCaches, - pub metrics_handle: Arc, + /// Prometheus recorder handle. Absent when `telemetry.metrics` is disabled. + pub metrics_handle: Option>, /// Read-through LFS upstream client (`upstream.lfs`). pub lfs_upstream: lfs_upstream::Upstream, /// Startup prewarm state (gates /readyz when configured). @@ -112,7 +113,11 @@ impl AppState { ) -> anyhow::Result> { let registry = walgit_wal::Registry::new(store.clone(), cfg.clone()); let bridge = bridge::Bridge::new(&cfg, registry.clone()); - let metrics_handle = metrics::install()?; + let metrics_handle = if cfg.telemetry.metrics { + Some(metrics::install()?) + } else { + None + }; let tls = tls::load(&cfg)?; if let Some(t) = &tls { tracing::info!(fingerprint = %t.fingerprint, mode = ?cfg.server.tls.mode, "TLS terminated in-process"); @@ -148,7 +153,7 @@ pub fn router(state: Arc) -> Router { .gzip(true) .quality(tower_http::CompressionLevel::Fastest); // Nothing with content is public: the SPA shell/assets, installer, credential - // helper and metrics all sit behind `web::require_auth` (identity from a + // helper and enabled metrics all sit behind `web::require_auth` (identity from a // bearer token or a session cookie). The JSON API and git // endpoints do their own `require_read`/`require_write`; `/_auth/*` is the // login flow itself; `/services/public/*` is the installer a not-yet-signed-in @@ -162,11 +167,25 @@ pub fn router(state: Arc) -> Router { .with_state(()) .layer(web_compression.clone()), ) - .route("/metrics", get(metrics::metrics_route)) .layer(axum::middleware::from_fn_with_state( state.clone(), web::require_auth, )); + let metrics = if state.cfg.telemetry.metrics { + Router::new() + .route("/metrics", get(metrics::metrics_route)) + .layer(axum::middleware::from_fn_with_state( + state.clone(), + web::require_auth, + )) + } else { + // Reserve the top-level name so the SPA's `/{owner}` route cannot turn a + // disabled scrape endpoint into a successful HTML response. + Router::new().route( + "/metrics", + get(|| async { axum::http::StatusCode::NOT_FOUND }), + ) + }; Router::new() .merge( @@ -180,6 +199,7 @@ pub fn router(state: Arc) -> Router { .layer(web_compression), ) .merge(gated) + .merge(metrics) .route("/healthz", get(health::healthz)) .route("/readyz", get(health::readyz)) // The SDK is a static artefact with no data in it; it must load from a diff --git a/crates/walgit-server/src/metrics.rs b/crates/walgit-server/src/metrics.rs index 264809e6..44e982a4 100644 --- a/crates/walgit-server/src/metrics.rs +++ b/crates/walgit-server/src/metrics.rs @@ -1,5 +1,5 @@ -//! Prometheus metrics exporter. Installs a recorder up front (once per process) -//! and exposes the rendered scrape via `/metrics`. +//! Prometheus metrics exporter. When enabled, installs a recorder once per +//! process and exposes the rendered scrape via `/metrics`. use std::sync::{Arc, OnceLock}; @@ -33,12 +33,15 @@ pub fn install() -> anyhow::Result> { /// `GET /metrics` pub async fn metrics_route(State(st): State>) -> impl IntoResponse { - let body = st.metrics_handle.render(); + let Some(handle) = &st.metrics_handle else { + return axum::http::StatusCode::NOT_FOUND.into_response(); + }; ( [( axum::http::header::CONTENT_TYPE, "text/plain; version=0.0.4", )], - body, + handle.render(), ) + .into_response() } diff --git a/crates/walgit-server/tests/metrics.rs b/crates/walgit-server/tests/metrics.rs new file mode 100644 index 00000000..c07b55d2 --- /dev/null +++ b/crates/walgit-server/tests/metrics.rs @@ -0,0 +1,22 @@ +mod harness; + +use axum::http::StatusCode; + +use harness::Server; + +type TestResult = anyhow::Result<()>; + +#[tokio::test] +async fn metrics_config_controls_scrape_endpoint() -> TestResult { + let disabled = Server::start_with_tweak(|cfg| cfg.telemetry.metrics = false).await?; + assert!(disabled.state.metrics_handle.is_none()); + let response = reqwest::get(format!("{}/metrics", disabled.base_url)).await?; + assert_eq!(response.status(), StatusCode::NOT_FOUND); + + let enabled = Server::start_with_tweak(|cfg| cfg.telemetry.metrics = true).await?; + assert!(enabled.state.metrics_handle.is_some()); + let response = reqwest::get(format!("{}/metrics", enabled.base_url)).await?; + assert_eq!(response.status(), StatusCode::OK); + + Ok(()) +} diff --git a/justfile b/justfile index f0449aba..cdd7ecbe 100644 --- a/justfile +++ b/justfile @@ -94,7 +94,7 @@ dev-store-stop: test: {{t5}} cargo test --workspace --lib --bins {{t10}} cargo test -p walgit-store -p walgit-git -p walgit-wal --tests - {{t10}} cargo test -p walgit-server --test web_api --test web_ui --test api_v1 --test static_http --test packfile_uri --test forward --test maintain --test routing_prefix --test lfs_upstream --test drain --test events --test follow --test policy + {{t10}} cargo test -p walgit-server --test web_api --test web_ui --test api_v1 --test static_http --test packfile_uri --test forward --test maintain --test routing_prefix --test metrics --test lfs_upstream --test drain --test events --test follow --test policy # Smart-HTTP end-to-end against real git (≈ 20 s) — run when touching smart.rs/receive/upload-pack/wal. e2e *ARGS: From 094964f3a5056f6d8d004760ec51fcb849ff9325 Mon Sep 17 00:00:00 2001 From: Preetam Dwivedi Date: Thu, 1 Oct 2026 10:57:54 -0700 Subject: [PATCH 2/2] Remove redundant metrics handler guard Use the optional Prometheus handle when constructing the router and inject the enabled handle into the route. The handler can now render without rechecking an impossible disabled state. --- crates/walgit-server/src/lib.rs | 5 +++-- crates/walgit-server/src/metrics.rs | 11 ++++------- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/crates/walgit-server/src/lib.rs b/crates/walgit-server/src/lib.rs index 8bb2cff7..f8017560 100644 --- a/crates/walgit-server/src/lib.rs +++ b/crates/walgit-server/src/lib.rs @@ -66,12 +66,12 @@ use std::future::Future; use std::net::SocketAddr; use std::sync::Arc; -use axum::Router; use axum::body::{Body, to_bytes}; use axum::extract::ConnectInfo; use axum::http::{Method, Request}; use axum::response::{IntoResponse, Response}; use axum::routing::get; +use axum::{Extension, Router}; use bytes::Bytes; use metrics_exporter_prometheus::PrometheusHandle; use walgit_store::DynStore; @@ -171,9 +171,10 @@ pub fn router(state: Arc) -> Router { state.clone(), web::require_auth, )); - let metrics = if state.cfg.telemetry.metrics { + let metrics = if let Some(metrics_handle) = &state.metrics_handle { Router::new() .route("/metrics", get(metrics::metrics_route)) + .layer(Extension(metrics_handle.clone())) .layer(axum::middleware::from_fn_with_state( state.clone(), web::require_auth, diff --git a/crates/walgit-server/src/metrics.rs b/crates/walgit-server/src/metrics.rs index 44e982a4..5cd09d48 100644 --- a/crates/walgit-server/src/metrics.rs +++ b/crates/walgit-server/src/metrics.rs @@ -3,12 +3,10 @@ use std::sync::{Arc, OnceLock}; -use axum::extract::State; +use axum::Extension; use axum::response::IntoResponse; use metrics_exporter_prometheus::PrometheusHandle; -use crate::AppState; - static HANDLE: OnceLock> = OnceLock::new(); /// Install the Prometheus recorder once per process and return a shared handle. @@ -32,10 +30,9 @@ pub fn install() -> anyhow::Result> { } /// `GET /metrics` -pub async fn metrics_route(State(st): State>) -> impl IntoResponse { - let Some(handle) = &st.metrics_handle else { - return axum::http::StatusCode::NOT_FOUND.into_response(); - }; +pub async fn metrics_route( + Extension(handle): Extension>, +) -> impl IntoResponse { ( [( axum::http::header::CONTENT_TYPE,