Add metricsAddr flag for separate metrics listener - #93
Conversation
When set (via -metricsAddr or IMAGEPROXY_METRICSADDR), Prometheus metrics are served on a separate HTTP listener and removed from the main address. Default behavior is unchanged.
There was a problem hiding this comment.
Pull request overview
Adds an optional dedicated metrics listener to the cmd/imageproxy binary so Prometheus scraping can be served on a separate address without exposing /metrics on the main proxy listener.
Changes:
- Introduces
-metricsAddr(and correspondingIMAGEPROXY_METRICSADDR) to enable a standalone metrics HTTP server. - When
-metricsAddris set,/metricson the main-addris replaced with a 404 handler and served only on the metrics listener.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Move /metrics routing into cmd/imageproxy so the library only generates metrics and the cmd is the sole place that serves them.
…moval Bind the metrics listener in the main goroutine via net.Listen so a bind failure (e.g. address already in use) fails fast before the main proxy starts and before logging that the metrics server is listening. Also add a test asserting the proxy library no longer special-cases /metrics.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
actions/cache@v2 is hard-deprecated by GitHub and auto-fails the run. golangci-lint v1.31 cannot parse the Go 1.25 stdlib (generics).
setup-go@v1 hangs on modern runners (can't fetch the Go version index).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- /metrics on the main listener now returns 404 (not 400) when
metricsAddr is set, matching the documented behavior.
- Metrics serve errors are logged instead of fatal, so a transient
metrics-listener failure does not bring down the proxy.
- Use a real *http.Server with ReadHeaderTimeout for the metrics
listener, and drop the unused server := &http.Server{...} above.
- data.go: apply Go 1.19+ doc-comment heading format (goimports). - .golangci.yml: exclude pre-existing staticcheck/gosec findings on legacy transport fallback code and the main listener's http.ListenAndServe usage. - cmd/imageproxy-sign: restore os.Stdout after TestMainFunc swaps it, otherwise Go 1.25's runtime hits the closed pipe when writing the coverage profile.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fmt.Fprint(w, "OK") | ||
| return | ||
| } | ||
|
|
||
| if r.URL.Path == "/metrics" { | ||
| var h http.Handler = promhttp.Handler() | ||
| h.ServeHTTP(w, r) | ||
| return | ||
| } | ||
|
|
||
| var h http.Handler = http.HandlerFunc(p.serveImage) |
There was a problem hiding this comment.
Proxy.ServeHTTP no longer special-cases /metrics, which changes the behavior of the imageproxy library for any consumers that mount Proxy directly (previously /metrics returned Prometheus output; now it becomes a 400 via request parsing). If this is intentional, it should be called out in docs/README and/or the PR description as a behavior/API change for library users; otherwise consider keeping /metrics handling in the library and letting the cmd/imageproxy router override it when -metricsAddr is set.
|
|
||
| # The main proxy listener uses http.ListenAndServe; pre-existing. | ||
| - path: cmd/imageproxy/main.go | ||
| linters: gosec | ||
| text: G114 |
There was a problem hiding this comment.
The exclude-rule path: cmd/imageproxy/main.go + text: G114 suppresses all G114 findings in that file. Since this PR adds a second HTTP server to the same file, this rule can accidentally mask future (or new) timeout-related findings beyond the pre-existing http.ListenAndServe call. Consider removing this exclude-rule and using a targeted inline suppression (e.g., //nolint:gosec on the specific ListenAndServe line) so new servers in the file still get checked.
| # The main proxy listener uses http.ListenAndServe; pre-existing. | |
| - path: cmd/imageproxy/main.go | |
| linters: gosec | |
| text: G114 |
Summary
-metricsAddrflag (auto-mapped toIMAGEPROXY_METRICSADDRvia the existing envy setup) that serves Prometheus metrics on a separate HTTP listener./metricsis removed from the main-addrand served only on the metrics address. When unset, behavior is unchanged.Test plan
go build ./cmd/imageproxysucceedsIMAGEPROXY_METRICSADDRset,/metricson main addr returns 404 and/metricson the metrics addr returns Prometheus outputIMAGEPROXY_METRICSADDRunset,/metricson main addr still works (default behavior preserved)imageproxy -helplists the new flag and env var