Skip to content

Fix telemetry metrics configuration - #76

Open
behinddwalls wants to merge 2 commits into
tobi:mainfrom
behinddwalls:preetam/issue-75-telemetry-metrics
Open

behinddwalls wants to merge 2 commits into
tobi:mainfrom
behinddwalls:preetam/issue-75-telemetry-metrics

Conversation

@behinddwalls

Copy link
Copy Markdown

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

## 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 tobi#75
@behinddwalls
behinddwalls marked this pull request as ready for review September 30, 2026 17:48
@0bserver07

Copy link
Copy Markdown
Contributor

Checked this against main and you're right, the recorder gets installed and /metrics is served no matter what telemetry.metrics says. CI hasn't run on this yet, so I ran everything locally with your branch on top of main (build, clippy, unit, e2e and the sims), all green. Small thing: the None check in the handler can't be hit anymore since the route only exists when metrics are on, so one of the two checks could go.

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.
@behinddwalls

Copy link
Copy Markdown
Author

Removed the redundant disabled-state check in 094964f. Route construction now uses the optional Prometheus handle as the source of truth and injects the enabled handle into the handler, so the handler no longer has an impossible None branch. The disabled route still reserves /metrics with a 404.

[addressed by agent]

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

telemetry.metrics=false does not disable Prometheus metrics

2 participants