PMM-15228 Speedup AuthServer#5658
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5658 +/- ##
==========================================
- Coverage 43.59% 42.60% -1.00%
==========================================
Files 415 431 +16
Lines 43134 34938 -8196
Branches 0 585 +585
==========================================
- Hits 18804 14884 -3920
+ Misses 22454 18563 -3891
+ Partials 1876 1491 -385
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR adds Prometheus observability to the Grafana AuthServer in pmm-managed, exposing request/cache/latency metrics and wiring the server into the Prometheus registry, plus updating the PMM Health Grafana dashboard to visualize the new signals.
Changes:
- Implemented a custom Prometheus collector in
AuthServerwith counters/gauges/histograms for auth requests, Grafana calls, cache behavior, in-flight requests, and latencies. - Registered the
AuthServercollector during pmm-managed startup so metrics are exposed automatically. - Updated the PMM Health dashboard to include panels for the new auth metrics and additional runtime/health visualizations.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| managed/services/grafana/auth_server.go | Adds Prometheus metric descriptors/state, implements prometheus.Collector, and instruments key auth/cache/DB/Grafana code paths. |
| managed/cmd/pmm-managed/main.go | Registers the AuthServer as a Prometheus collector at startup. |
| dashboards/dashboards/PMM Health/PMM_Health.json | Adds/adjusts dashboard panels and queries to surface new auth metrics and runtime health info. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (2)
managed/services/grafana/auth_server.go:406
- The metrics label uses the raw
X-Original-Uriheader whenextractOriginalRequestfails. That header can include query strings and high-cardinality / potentially sensitive values, which is risky to expose in Prometheus labels. Use a stable, safe route label in this error path (e.g., the currentreq.URL.Pathwhich will be/auth_request).
s.incAuthRequests(req.Method, req.Header.Get("X-Original-Uri"), http.StatusBadRequest)
managed/services/grafana/auth_server.go:451
routeis recorded as the full cleaned request path (e.g./graph/api/datasources/proxy/8/in tests). That can create unbounded label cardinality and an ever-growingsync.Map(memory leak over time) when paths contain IDs or other variable segments. Consider using the matched rule prefix fromresolveRule(or another normalized route name) as theroutelabel instead of the raw path.
s.incAuthRequests(req.Method, req.URL.Path, http.StatusOK)
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
managed/services/grafana/auth_server.go:258
- mAuthRequests uses the raw cleaned request path as a label and as part of the sync.Map key. For paths with variable segments (e.g. Grafana proxy routes like /graph/api/datasources/proxy//), this can create unbounded time series and unbounded in-process memory growth because entries are never evicted from the sync.Map. Consider normalizing the label (e.g., use the matched rule prefix from resolveRule / nextPrefix chain, or otherwise bucket variable segments) to keep cardinality bounded.
mAuthRequestsDesc: prom.NewDesc(
prom.BuildFQName(prometheusNamespace, prometheusSubsystem, "requests_total"),
"Total number of authentication requests.",
[]string{"method", "route", "status_code"},
nil,
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
managed/services/grafana/auth_server.go:444
routelabel is currently set to the full request path (req.URL.Path/X-Original-Uri). That can create unbounded label cardinality (IDs, arbitrary paths) and also growss.mAuthRequestswithout bound (onesync.Mapentry per unique path/method/status), which can become a memory/DoS risk over time. Consider using the matched rule prefix (fromrules/methodRules) or another bounded route identifier for the metric label instead of the raw path.
status := httpStatusForAuthError(authErr.code)
s.incAuthRequests(req.Method, req.URL.Path, status)
s.returnError(rw, status, m, l)
| // Check for URL encoding (%), dot traversal (.), double slashes (//), or CR/LF. | ||
| if c == '%' || c == '.' || c == '\n' || c == '\r' || (c == '/' && i > 0 && uri[i-1] == '/') { | ||
| needsWork = true | ||
| break | ||
| } |
Ticket number: PMM-15228
Percona-Lab/pmm-submodules#4481
This pull request adds detailed Prometheus metrics to the
AuthServerfor improved observability and performance analysis, and introduces a highly optimized, allocation-free path cleaning function. It also includes several bug fixes and refactorings around request path handling and authentication logic.Observability and Metrics:
AuthServerfor tracking authentication requests, cache hits/misses, cache size, and operation latencies. The server now implements theprom.Collectorinterface and registers itself for metrics collection. (auth_server.go,main.go) [1] [2] [3] F749b569L830R830, [4]Performance Improvements:
cleanPathlogic with a new zero-allocation fast path cleaning function that strips query parameters, normalizes, and unescapes paths only when necessary. This significantly reduces allocations for common cases and is covered by new unit and benchmark tests. (auth_server.go,auth_server_test.go) [1] [2]Authentication and Path Handling Fixes:
req.URL.Path) after extraction, preventing inconsistencies and potential security issues. (auth_server.go)Cache and Latency Metrics:
auth_server.go) (managed/services/grafana/auth_server.goR166-R167Testing and Benchmarking:
auth_server_test.go)These changes improve the reliability, observability, and efficiency of the authentication subsystem.