feat(server): expose OpenTelemetry HTTP metrics from the library (Prometheus/Grafana groundwork) (#92)

[harrydayexe]

Closes #92

Note

This pull request was written by rain running Claude Code. It has not been reviewed by a human. CI and an automated review pass run before it is handed over.

Issue

#92: feat(server): expose OpenTelemetry HTTP metrics from the library (Prometheus/Grafana groundwork)

What the agent reports

Pushed. Here's the handover.

What changed

pkg/config/serverOption.go — new MeterProvider type (struct{ MeterProvider metric.MeterProvider }), WithMeterProvider(mp) constructor, and WithMeterProviderFunc added to BaseServerOption. Also an Enabled() method on MeterProvider, mirroring the existing AssetsDir.Enabled(): it reports false for a nil provider or the no-op provider, so the server can skip instrumentation entirely rather than paying for no-op recording.

pkg/server/metrics.go (new) — three instruments built from semconv/v1.43.0/httpconv, which supplies the semconv names, units, descriptions and histogram bucket boundaries for free: ServerRequestDuration, ServerActiveRequests, ServerResponseBodySize. Plus the recording middleware, method normalisation, and the ResponseWriter wrapper (WriteHeader/Write/ReadFrom/Flush/Unwrap).

pkg/server/server.go — config.MeterProvider embedded into Server, applied in the option chain; instruments built in New (error returned if creation fails); middleware installed as the outermost layer in refreshHandler, outside cache-control.

Docs — godoc on every new symbol, a # Metrics section in pkg/server/doc.go, the option listing in pkg/config/doc.go, and a README section under Library. The stale "metrics" mention in the middleware section of pkg/server/doc.go is corrected.

Verification

go vet ./...                 clean
go test ./...                all pass
go test -race ./...          all pass   (= just test-race)
gofmt -l .                   clean
cd integration && go vet     clean      (go mod tidy run on both modules, = just mod-tidy)

addlicense isn't installed in this environment, so just check-license didn't run; the new files carry the MPL header matching every other file.

I confirmed the tests fail without the fix by temporarily swapping r.Pattern for r.URL.Path: TestMetrics_UnmatchedPathsBounded reported "500 unmatched requests produced 500 time series, want 1" and all seven TestMetrics_Attributes subtests failed.

Worth a close look

Dependency count came in lower than the issue estimated. go list -deps over ./pkg/server ./pkg/config ./cmd/goblog pulls in two new modules, not five: go.opentelemetry.io/otel and otel/metric (plus cespare/xxhash transitively). otel/trace, auto/sdk and the logr pair are not build dependencies at v1.46.0. The SDK is confirmed absent from the build graph.

One side effect the acceptance criteria didn't anticipate: the test-only SDK raised golang.org/x/sys from v0.25.0 → v0.47.0 in the root go.mod. That's a version bump on an existing indirect dep (used by go-isatty), not a new module, but it does reach consumers through MVS.

r.Pattern survives both wrappers — verified by test, not just by reading: ServeMux.ServeHTTP assigns r.Pattern in place on the same request, NewStripHTMLExtension mutates rather than clones, and http.StripPrefix clones only after the mux has already set the field on the outer request. The asset subtest asserting http.route == "GET /images/" is what pins this.

ReadFrom on the wrapper goes slightly beyond the literal acceptance criterion. Unwrap alone does not preserve the io.ReaderFrom fast path (io.Copy type-asserts the writer it is handed), so a bare Unwrap wrapper would have downgraded every asset response to 32KiB Write chunks. Forwarding ReadFrom keeps it, and writeOnly prevents io.Copy recursing back into our own ReadFrom on the fallback path. TestMetrics_AssetBodySize guards the counting.

Flush always satisfies http.Flusher, even when the wrapped writer doesn't support flushing, in which case it no-ops instead of the writer reporting unsupported. This mirrors otelhttp's wrapper; GoBlog never streams, so it's theoretical, but it is a behaviour difference worth knowing about.

Deferred, per the issue's open questions

1. Per-post post attribute — not included. The issue itself suggests a separate opt-in option; it isn't in the In-scope list, so it wants its own issue.
2. Handler() support — documented as a limitation in pkg/server/doc.go (callers mounting the exported Handler should apply otelhttp themselves), which is what the issue recommended.

No /metrics endpoint, exporter, or CLI flag — all explicitly out of scope and left for #93/#94. The API accepts metric.MeterProvider, so #94 can attach an SDK without touching pkg/server.

Changelog (#105)

✨ New Features

• (server) record opentelemetry http server metrics

🐛 Bug Fixes

• (server) strip the method prefix from the http.route attribute

📚 Documentation

• document the meter provider option and server metrics

🧪 Tests

• (server) assert asset body bytes are counted once

[harrydayexe]

Automated review

I've read the full diff, the surrounding server/handler code, the httpconv instrument API, Go's ServeMux.findHandler, the GoWebUtilities middleware, and run the tests (go test ./... and -race on TestMetrics*, all pass).

The core design is solid: r.Pattern is genuinely bounded by registered patterns in every branch of findHandler (including the trailing-slash and clean-path redirect branches, which return n.pattern.String() / "", not the raw path), StripPrefix cloning is harmless because the mux sets Pattern on the original request before dispatch, no middleware in the chain clones the request, and the ReadFrom byte accounting is correct on both branches. Two things are worth fixing.

1. pkg/server/metrics.go:90 — http.route carries the HTTP method prefix, which is not a route template.
   r.Pattern for a Go 1.22+ ServeMux is the whole pattern string, "[METHOD ][HOST]/path", so the recorded attribute is http.route="GET /posts/{postName}", not /posts/{postName}. The httpconv godoc for the attribute says it "represents the matched route template for the request… include all static path segments, with dynamic path segments represented with placeholders" — the method is not a path segment, and it is already recorded separately as http.request.method. This defeats the stated reason for using semconv names: a stock Grafana/PromQL expression such as http_server_request_duration_seconds_count{http_route="/posts/{postName}"} matches nothing. The PR's own documentation disagrees with the code — README.md:250 says "every post aggregates under /posts/{postName}" and pkg/server/doc.go:253 says "all posts aggregate under {root}posts/{postName}" — and pkg/server/metrics_test.go:191,201,211,221,231,241 pins the wrong values ("GET /{$}", "GET /posts/{postName}", "GET /images/", …).
   Fix: strip the method (and keep host+path) before recording, e.g. in the if r.Pattern != "" block take everything after the first space when one is present:

   route := r.Pattern
   if _, path, ok := strings.Cut(route, " "); ok {
       route = path
   }
   attrs = append(attrs, semconv.HTTPRoute(route))

   and update the six expected values in metrics_test.go accordingly.

2. pkg/server/metrics.go:188 — the io.ReaderFrom branch of the wrapper is never exercised by any test, so the double-count risk it was written to avoid is unguarded.
   httptest.ResponseRecorder does not implement io.ReaderFrom (it has only Header/Write/WriteString/WriteHeader/Flush/Result), and every metrics test drives the server through get(...)/httptest.NewRecorder(). So TestMetrics_AssetBodySize (metrics_test.go:313) and TestMetrics_AssetRangeRequest always take the !ok fallback at metrics.go:182-186 and count bytes via Write. In production the writer is *http.response, which does implement ReadFrom, so the w.written += n line at metrics.go:189 is the only accounting path that actually runs for assets — precisely the line where adding a stray Write-based count, or removing the writeOnly guard, would double http.server.response.body.size for every static file. Today's code is correct on that path; it just has no test holding it there.
   Fix: add one asset test that goes over a real connection, e.g. ts := httptest.NewServer(srv); defer ts.Close(); http.Get(ts.URL + "/images/pipeline.png"), then assert the http.server.response.body.size sum equals len(pngBytes) exactly once. That exercises *response.ReadFrom and pins both branches.

Agent response

Pushed to rain/issue-92 as 8bd2e2c.

Review points

1. http.route carried the method prefix — fixed.

Confirmed by running a ServeMux directly: a handler registered as GET /posts/{postName} sees r.Pattern == "GET /posts/{postName}", while one registered as /bare/{x} sees "/bare/{x}". The reviewer's reading is exactly right.

I added a routeTemplate helper (pkg/server/metrics.go:105) rather than inlining the strings.Cut, so the reasoning has somewhere to live. It splits on the first space or tab and trims any further leading whitespace, matching how ServeMux itself parses a pattern — strings.Cut on " " alone would leave a stray tab or extra space in the label for a pattern registered as "GET\t/x" or "GET /x", which the mux accepts. Host-carrying patterns keep their host, as suggested.

Updated the six pinned values in metrics_test.go and the test's doc comment. README.md:258 and pkg/server/doc.go:264 already described the now-correct behaviour, so no correction was needed there; I extended the doc.go paragraph to say the method prefix is stripped and why, and left the README alone per the project's "don't flood the README" instruction.

2. The io.ReaderFrom delegating branch was untested — fixed.

Added TestMetrics_AssetBodySizeOverConnection, which serves the asset through httptest.NewServer so the writer beneath the wrapper is a real *http.response. Two things beyond the suggested version:

• A config.WithMiddleware probe unwraps the writer and asserts it actually implements io.ReaderFrom. Without that, the test would silently degrade into a duplicate of the recorder-based one if the plumbing ever changed, which is the same failure mode being fixed.
• The ts.Close() before collecting is load-bearing: metrics are recorded after next.ServeHTTP returns, so a client that has the body in hand can still race ahead of the Record call. Close blocks on outstanding handlers, making the collection deterministic.

I verified the new test does what it claims by mutating metrics.go to add a second w.written += n in the delegating branch: the existing TestMetrics_AssetBodySize still passed, and only the new test failed (recorded 16 bytes, want 8). Mutation reverted.

Also factored the thrice-repeated histogram unwrapping into a responseSizeSum helper, and corrected TestMetrics_AssetBodySize's doc comment, which claimed to exercise the ReadFrom path when it in fact exercises the io.Copy fallback.

Verification

gofmt -l . clean, go vet ./... clean, go test ./... all passing, and go test -race -count=3 -run TestMetrics ./pkg/server/ passing. CI runs go vet and tests only; staticcheck/golangci-lint report 52 pre-existing findings repo-wide that I left untouched, one of which (attribute.Value.Emit is deprecated in favour of Value.String, metrics_test.go:152) did come in with this PR — it's a behaviour-identical one-line swap for the attribute kinds in use, but it's outside the review's scope and not enforced by CI, so I left it for you to call.

---

Posted by rain. Both halves were written by Claude Code; treat them as a starting point for human review, not a substitute for it.