Fix two dashboard charts that plotted the wrong data - #416
Conversation
The comparative section of the cluster dashboard rendered a "Connection Count" bar per server, but the series was built as metrics.map(() => 1), so every bar was a hardcoded one and the chart showed no real data at all. The section already fetches the performance summary for its other three bars, and that response had no connection count to offer, so the endpoint now reports an active_connections figure per connection, taken from the sum of numbackends across the databases in the server's most recent pg_stat_database snapshot, and the chart plots that. Both charts on the query drill-down page queried the pg_stat_statements probe without filtering to the query the user had selected, because MetricFilters had no way to express such a filter; the series therefore aggregated across every statement in the database whilst the page presented them as belonging to one query. MetricFilters now carries a QueryID, applied as queryid::text so a 64-bit identifier survives the round trip through JSON, and both the time-series and latest-row paths of GET /api/v1/metrics/query accept a queryid parameter that is validated as a 64-bit integer before it reaches the query layer. The query detail page passes the identifier of the selected query to both charts. Closes #404
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 34 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (16)
Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 175 |
| Duplication | 42 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
|
@coderabbitai review |
|
|
@coderabbitai review |
|
Every function whose rollback the sweep converted now clears the project's 90% line-coverage floor, and every modified line but one is executed by a test. The three performance-summary endpoints and the query executor were the gap: their handlers had no test at all, so the converted rollback lines were never reached. The new handler tests follow the harness pattern established in PR #416, wiring a real handler to the local Postgres over a trimmed metrics schema and driving it through httptest. Distinct helper and schema names keep the two test files independent, and no production line #416 touches is modified here. Coverage of the touched units, before and after: handlePerfSummary 0.0% -> 94.5% handleDatabaseSummaries 0.0% -> 90.0% handleTopQueries 0.0% -> 95.9% executeQuery 25.0% -> 91.7% UnacknowledgeAlert 81.0% -> 90.5% DeleteAutoDetectedCluster 76.0% -> 92.0% RemoveServerFromCluster 0.0% -> 95.2% Migrate (collector) 72.7% -> 90.9% Reaching the last few branches needed realistic failure injection rather than mocks: a query tracer that cancels the request context between statements, schema drift such as a dropped column, a migration that closes its own connection, and a monitored server on a dead port. The one modified line that remains uncovered is the rollback inside StoreMetrics in collector/src/probes/storage.go. Its guard tests an outer err that the INSERT and Commit failure paths shadow with :=, so the guard can never be true and the rollback is unreachable. That is a pre-existing defect worth its own issue; this change deliberately does not alter the behaviour.
1 participant
Summary
Two unrelated chart defects, both of which made a chart show
something other than what its title claimed.
The comparative section of the cluster dashboard rendered a
"Connection Count" bar per server, but the series was built as
metrics.map(() => 1), so every bar was a hardcoded one. Thesection already fetches
/api/v1/metrics/performance-summaryforits other three bars, and that response had no connection count to
offer, so
PerfConnectionResponsenow carries anactive_connectionsfigure, taken from the sum ofnumbackendsacross the databases in the server's most recent
pg_stat_databasesnapshot, and the chart plots that value. No newrequest is issued.
Both charts on the query drill-down page, "Execution Time Over
Time" and "Calls Over Time", queried the
pg_stat_statementsprobewithout filtering to the selected query, because
MetricFiltershad no way to express such a filter; the two series therefore
aggregated across every statement in the database whilst the page
presented them as belonging to one query.
MetricFiltersnowcarries a
QueryID, applied asqueryid::textso that a 64-bitidentifier survives the round trip through JSON without losing
precision, and both the time-series and the latest-row paths of
GET /api/v1/metrics/queryaccept aqueryidparameter that isvalidated as a 64-bit integer before it reaches the query layer.
The query detail page passes the identifier of the selected query
to both charts.
The OpenAPI specification and the generated
docs/admin-guide/api/openapi.jsonare updated for the newqueryidparameter, and forindex_name, which the time-seriesendpoint has accepted for some time but never documented.
Test plan
New
ComparativeChartsSection.test.tsxcovers the connectioncount reaching the chart, the fallback to zero when the field is
absent, and the loading, empty, and error states.
New
QueryDetail.test.tsxasserts that both chart parameter setscarry the selected
queryId, and covers the AI overview panel,the query text toggle, and the error paths.
useMetrics.test.tsgains cases forqueryidappearing in, andbeing omitted from, the request URL.
Go:
metricQueryBase,buildLatestRowsQueryandBuildDerivedMetricsQuerygainqueryidplaceholder andargument-binding tests; the metrics handler gains tests that the
filter reaches the query layer and that a malformed
queryidisrejected with 400 on both request paths.
New
perf_summary_connection_count_test.gocoversqueryConnectionCountagainst a live Postgres (latest snapshotonly, no data, missing table) and drives
handlePerfSummaryend-to-end, asserting
active_connectionsper connectionalongside the metrics it already reported.
Client coverage (
make coverage, 173 files, 3527 tests passing):ComparativeChartsSection.tsx100% lines,QueryDetail.tsx97.93% lines,
useMetrics.ts100% lines.Go coverage for the touched functions:
queryConnectionCount100%,
parseQueryIDFilter100%,metricQueryBase100%,buildLatestRowsQuery100%, andhandlePerfSummarylifted from0% to 87.8%.
Closes #404