PMM-15228 Enrich AuthServer with performance metrics - #5670
Conversation
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>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5670 +/- ##
==========================================
- Coverage 43.59% 43.47% -0.12%
==========================================
Files 415 433 +18
Lines 43134 35128 -8006
Branches 0 591 +591
==========================================
- Hits 18804 15272 -3532
+ Misses 22454 18365 -4089
+ 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:
|
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (1)
⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe PR regenerates the PMM Health Grafana dashboard and adds Prometheus instrumentation to Grafana authentication. Request paths are normalized before authorization checks, and authentication, cache, Grafana, and LBAC operations now expose metrics. ChangesPMM Health dashboard
Grafana authentication observability
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant AuthServer
participant LBAC
participant Grafana
participant Prometheus
Client->>AuthServer: send authentication request
AuthServer->>AuthServer: clean and validate request path
AuthServer->>LBAC: resolve roles and filters
LBAC-->>AuthServer: return authorization result
AuthServer->>Grafana: authenticate authorized request
Grafana-->>AuthServer: return response status
AuthServer->>Prometheus: record request, cache, Grafana, and LBAC metrics
AuthServer-->>Client: return authentication response
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
dashboards/dashboards/PMM Health/PMM_Health.json (1)
8089-8100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the hidden target with the invalid subquery.
This target contains two defects:
(irate(...[2m]))[$__interval]is not a valid subquery. A subquery requires a resolution after a colon, for example[$__interval:].$peaksis not declared in thetemplating.listblock at lines 9603-9725.The target sets
"hide": true, so the query never runs and no user sees the failure. The text remains as a broken artifact. Delete the target, or declare$peaksand correct the subquery syntax.♻️ Proposed removal of the hidden target
- { - "datasource": "Metrics", - "editorMode": "code", - "exemplar": false, - "expr": "max_over_time( (irate(ClickHouseProfileEvents_ReadBackoff{instance=~\"$instance\"}[2m]))[$__interval] ) * $peaks", - "hide": true, - "interval": "", - "legendFormat": "peaks - {{instance}}", - "range": true, - "refId": "A" - }, { "datasource": "Metrics", "editorMode": "code", "exemplar": false, "expr": "rate(ClickHouseProfileEvents_ReadBackoff{job=\"clickhouse\"}[$__interval]) ",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dashboards/dashboards/PMM` Health/PMM_Health.json around lines 8089 - 8100, Remove the hidden target identified by refId "A" containing the ClickHouseProfileEvents_ReadBackoff max_over_time expression from the targets array. Do not add a $peaks variable or alter the invalid subquery, since this artifact should be deleted entirely.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dashboards/dashboards/PMM` Health/PMM_Health.json:
- Line 3900: Update the description for the “Incoming Auth Requests Rate” panel
to describe interpreting the pmm_managed_auth_requests_total request-rate
metric, removing all references to type labels, latency spikes, and database or
Grafana duration breakdowns.
- Around line 2670-2690: Update dashboards/dashboards/PMM Health/PMM_Health.json
at lines 2670-2690 by replacing rate() with avg_over_time() for both process
memory gauge queries. At lines 6069-6089, apply the same replacement to both
Grafana memory series and change the virtual-memory range from [5m] to
$__interval. At lines 5578-5589, query the grafana_database_conn_open gauge
directly without rate().
- Around line 9428-9435: Update the “Fetched” target expression to filter on
service_name=~"$service_name" instead of instance=~"$node_name", matching the
sibling targets in the same panel while preserving the existing metric,
aggregation, and interval logic.
- Around line 4666-4676: Update the PromQL expression for the target in the
cache hit-rate panel so the numerator sums
irate(pmm_managed_auth_cache_total[$__rate_interval]) filtered with
status="hit", while retaining the unfiltered total in the denominator. Keep the
existing panel configuration and legend unchanged.
- Around line 6713-6722: Update the disk usage expression in the panel query
identified by refId A so vm_free_disk_space_bytes is aggregated with sum as
well, producing matching label sets for the denominator addition while
preserving the existing job and instance filters.
- Around line 2441-2481: Update the target expression for panel id 1081 to
calculate average failed-batch duration by dividing the rate of
qan_api2_data_ingestion_batch_save_seconds_sum by the corresponding rate of
qan_api2_data_ingestion_batch_save_seconds_count, preserving the existing error
filter and interval. Keep the seconds unit and “Batch save duration with Errors”
title aligned with this duration result.
- Around line 6428-6448: Update the Grafana statistic panels, including the
Organisations Count and Folders Count targets, to use the dashboard’s instance
variable in their Prometheus selectors instead of a fixed instance value, so
both non-HA and active/active HA generated node IDs return data. Preserve the
existing metric names and db_name="grafana" filtering.
In `@managed/services/grafana/auth_server.go`:
- Around line 267-273: Bound authentication metric labels to prevent arbitrary
client-controlled values from creating unbounded Prometheus series. In
managed/services/grafana/auth_server.go:267-273, keep only bounded method and
canonical-route dimensions for mAuthRequests; at 392-403, emit "invalid" instead
of the fallback path when original-request parsing fails; at 422-448, record the
resolved authorization-rule prefix or route template rather than req.URL.Path.
Update managed/services/grafana/auth_server_test.go:402-418 to assert the fixed
invalid-route label.
---
Nitpick comments:
In `@dashboards/dashboards/PMM` Health/PMM_Health.json:
- Around line 8089-8100: Remove the hidden target identified by refId "A"
containing the ClickHouseProfileEvents_ReadBackoff max_over_time expression from
the targets array. Do not add a $peaks variable or alter the invalid subquery,
since this artifact should be deleted entirely.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: aee931c4-6cdb-4bb4-8278-6ad04252c933
📒 Files selected for processing (4)
dashboards/dashboards/PMM Health/PMM_Health.jsonmanaged/cmd/pmm-managed/main.gomanaged/services/grafana/auth_server.gomanaged/services/grafana/auth_server_test.go
Ticket number: PMM-15228
Feature build: Percona-Lab/pmm-submodules#4484
This pull request adds detailed Prometheus metrics to the
AuthServercomponent in order to monitor authentication request flows, cache usage, and request durations. It also ensures that metrics are accurately labeled, especially in error scenarios, and improves code clarity around request path handling.Prometheus Metrics Integration:
authMetricsstruct toAuthServerthat defines and registers Prometheus counters and histograms for tracking authentication requests, cache hits/misses, Grafana backend requests, cache size, and operation durations. (managed/services/grafana/auth_server.go)prom.Collectorinterface forAuthServer, allowing it to be registered with Prometheus and report its metrics. (managed/services/grafana/auth_server.go)AuthServeras a Prometheus collector in the main application entrypoint. (managed/cmd/pmm-managed/main.go)Metrics Collection in Auth Flow:
managed/services/grafana/auth_server.go) [1] [2] [3]managed/services/grafana/auth_server.go)Testing and Path Handling Improvements:
managed/services/grafana/auth_server_test.go)managed/services/grafana/auth_server.go) [1] [2]These changes provide better observability into authentication operations and improve the maintainability and correctness of metrics reporting.
Summary by CodeRabbit
New Features
Monitoring Improvements