Expand Tide Grafana dashboard with operational metrics - #83118
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe Tide Grafana dashboard now uses schema 40 and structured rows. It adds reusable panel builders and query-observability helpers. Panels cover query completeness, durations, errors, partial results, merges, controller health, pool sizes, PR returns, and GitHub API usage. ChangesTide dashboard observability
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@petr-muller: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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
`@clusters/app.ci/openshift-user-workload-monitoring/mixins/_grafana_dashboards/tide.jsonnet`:
- Around line 924-926: Update the query passed to prometheus.target in this
panel to use the existing lazy-series correction via mergesIncreasePinned or
mergesIncreaseTrend, including the corrected expression in both the displayed
daily rate and the max_over_time threshold calculation. Preserve the existing
org/repo/branch grouping, legend format, and >10 filter behavior.
- Around line 768-781: Update the titles passed to queryErrorCountPanel for the
sync and status panels so they describe raw query shard outcome counts rather
than error rates, distinguishing them from the existing “Sync Error Rate (5m)”
panel while preserving the current queries and layout.
- Around line 612-633: Update mergesIncreasePinned, mergesIncreaseTrend, and
mergesIncreasePinnedSameHourYesterday so their new-series correction credits the
first in-window sample via per-series min_over_time rather than the series’
current value. Preserve full per-series matching between the unless and
min_over_time operands, apply the appropriate timestamp/offset behavior for each
helper, and verify the resulting PromQL against live data.
- Around line 487-503: Update the `prometheus.target` expression using `counts`
to assign a stable label to the `vector(0)` fallback series, then change the
`overrides+` `byName` matcher to target that label value instead of an empty
name. Preserve hiding the fallback from the legend, tooltip, and visualization
while leaving the primary series unchanged.
🪄 Autofix
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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 92c6b78a-b575-4884-a82f-41a1b93a16f9
📒 Files selected for processing (2)
clusters/app.ci/openshift-user-workload-monitoring/mixins/_grafana_dashboards/tide.jsonnetclusters/app.ci/openshift-user-workload-monitoring/mixins/grafana_dashboards_out/tide_grafanadashboard.yaml
034c871 to
a0c41c0
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
openshift-merge-bot
Bot
added
the
rehearsals-ack
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
clusters/app.ci/openshift-user-workload-monitoring/mixins/_grafana_dashboards/tide.jsonnet (4)
1143-1146: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale cross-reference: no panel named "Merges per Hour by Repo" exists.
The rewrite removed that panel. Point the comment at the reason instead of at a missing panel.
✏️ Proposed comment fix
- // See "Merges per Hour by Repo": without this the default - // intervalFactor of 2 turns the requested 30m step into 1h blocks. + // Without intervalFactor=1 the default factor of 2 turns the + // requested 30m step into 1h blocks. intervalFactor=1, interval='30m',🤖 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 `@clusters/app.ci/openshift-user-workload-monitoring/mixins/_grafana_dashboards/tide.jsonnet` around lines 1143 - 1146, Update the comment above intervalFactor in the dashboard configuration to explain the 30m step and why intervalFactor=1 is required, removing the stale reference to the deleted “Merges per Hour by Repo” panel.
922-927: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
options:replaces the builder's options object instead of merging into it.
barGaugePanel.newsets anoptionsobject. Using:here discards it. The four bar gauges in this file (Lines 922-927, 1008-1013, 1059-1064, 1111-1116) all re-specifyreduceOptions,orientation,displayMode, andvalueMode, so the important keys survive. Any other key the builder set, such asshowUnfilled, is dropped and falls back to the Grafana default. Useoptions+:so the panels keep the builder's baseline.♻️ Proposed change (apply at all four bar gauges)
- options: { + options+: { reduceOptions: { values: false, calcs: ['lastNotNull'], fields: '' }, orientation: 'horizontal', displayMode: 'gradient', valueMode: 'color', },🤖 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 `@clusters/app.ci/openshift-user-workload-monitoring/mixins/_grafana_dashboards/tide.jsonnet` around lines 922 - 927, Change the options field from replacement to merge semantics in all four bar gauge panel definitions using barGaugePanel.new: replace options: with options+: at the four corresponding gauge blocks so their explicitly configured values remain while preserving baseline options such as showUnfilled from the builder.
490-508: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the same labeled fallback here as in
queryPartialRateHistoryPanel.This panel names the fallback series by the literal
:that results when Grafana substitutes both missing labels with empty strings. The comment at Lines 554-560 inqueryPartialRateHistoryPanelstates that this substitution is not reliable, and that panel therefore attaches an explicitorg_shardlabel so the override has a stable field name. Apply the same idiom here so both panels depend on one documented behavior instead of two.♻️ Proposed change to align the two fallbacks
- // The `or vector(0)` fallback below has no org_shard/error_class - // labels, so its legend substitutes to the literal string ":" — hide - // that pseudo-series entirely. It only exists so the panel always has + // The fallback below carries explicit org_shard/error_class values so + // it has a stable, matchable field name — hide that pseudo-series + // entirely. It only exists so the panel always has // at least one data point (a clean window otherwise has zero matching // time series, which Grafana renders as "No data" instead of "0 // errors"); at value 0 on a bar chart it's invisible anyway. - { matcher: { id: 'byName', options: ':' }, properties: [ + { matcher: { id: 'byName', options: '—:—' }, properties: [ { id: 'custom.hideFrom', value: { legend: true, tooltip: true, viz: true } }, ] }, ], }, options+: { legend+: { sortBy: 'Last', sortDesc: true }, }, }) .addTarget(prometheus.target( - '(%s) > 0 or vector(0)' % counts, + '(%s) > 0 or label_replace(label_replace(vector(0), "org_shard", "—", "", ""), "error_class", "—", "", "")' % counts, legendFormat='{{org_shard}}:{{error_class}}', ));🤖 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 `@clusters/app.ci/openshift-user-workload-monitoring/mixins/_grafana_dashboards/tide.jsonnet` around lines 490 - 508, Update the fallback expression in the panel’s prometheus.target call, alongside the counts query and legendFormat, to attach an explicit org_shard label using the same labeled-fallback idiom as queryPartialRateHistoryPanel. Then align the Grafana field override matcher and explanatory comment with that stable fallback label, preserving the existing hidden pseudo-series behavior.
17-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
pendingis dead state.No branch of
stepever adds an element topending. It is initialized to[], reset to[]on each row, and concatenated as an empty list at Line 23 and Line 33. Removing it simplifies the fold without changing the output.♻️ Proposed simplification
local collapseRowsExcept(keepOpen, panels) = local step(acc, panel) = if panel.type == 'row' then - // Close out whatever row was open, then start collecting for this one. // The kept-open row is emitted as an ordinary expanded row and collects // nothing, so its members stay top-level. - acc { out: acc.out + acc.pending + [panel], pending: [], collecting: panel.title != keepOpen } + acc { out: acc.out + [panel], collecting: panel.title != keepOpen } else if acc.collecting then acc { out: acc.out[:std.length(acc.out) - 1] + [acc.out[std.length(acc.out) - 1] { collapse: true, collapsed: true, panels+: [panel], }] } else acc { out: acc.out + [panel] }; - local folded = std.foldl(step, panels, { out: [], pending: [], collecting: false }); - folded.out + folded.pending; + std.foldl(step, panels, { out: [], collecting: false }).out;🤖 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 `@clusters/app.ci/openshift-user-workload-monitoring/mixins/_grafana_dashboards/tide.jsonnet` around lines 17 - 33, Remove the unused pending field from collapseRowsExcept and its step accumulator. Update the row and non-row branches to build output directly, remove pending concatenations and resets, and return folded.out while preserving the existing row-collapsing behavior.
🤖 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
`@clusters/app.ci/openshift-user-workload-monitoring/mixins/_grafana_dashboards/tide.jsonnet`:
- Around line 696-717: Update mergesIncreasePinned, mergesIncreaseTrend, and
mergesIncreasePinnedSameHourYesterday so the merges_sum existence check in each
unless expression occurs before min_over_time, or otherwise removes __name__
from both operands. Preserve the existing window, offset, grouping, and fallback
behavior while ensuring only genuinely new series receive the correction.
- Around line 1164-1170: Replace the inline-styled HTML banners in both
controller panels of tide.jsonnet with Markdown headings, removing the
surrounding div and style attributes while preserving the Sync Controller and
other controller titles and descriptive text.
---
Nitpick comments:
In
`@clusters/app.ci/openshift-user-workload-monitoring/mixins/_grafana_dashboards/tide.jsonnet`:
- Around line 1143-1146: Update the comment above intervalFactor in the
dashboard configuration to explain the 30m step and why intervalFactor=1 is
required, removing the stale reference to the deleted “Merges per Hour by Repo”
panel.
- Around line 922-927: Change the options field from replacement to merge
semantics in all four bar gauge panel definitions using barGaugePanel.new:
replace options: with options+: at the four corresponding gauge blocks so their
explicitly configured values remain while preserving baseline options such as
showUnfilled from the builder.
- Around line 490-508: Update the fallback expression in the panel’s
prometheus.target call, alongside the counts query and legendFormat, to attach
an explicit org_shard label using the same labeled-fallback idiom as
queryPartialRateHistoryPanel. Then align the Grafana field override matcher and
explanatory comment with that stable fallback label, preserving the existing
hidden pseudo-series behavior.
- Around line 17-33: Remove the unused pending field from collapseRowsExcept and
its step accumulator. Update the row and non-row branches to build output
directly, remove pending concatenations and resets, and return folded.out while
preserving the existing row-collapsing behavior.
🪄 Autofix
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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 23ffefc8-6ee1-4e08-be58-a1d75ad4d728
📒 Files selected for processing (2)
clusters/app.ci/openshift-user-workload-monitoring/mixins/_grafana_dashboards/tide.jsonnetclusters/app.ci/openshift-user-workload-monitoring/mixins/grafana_dashboards_out/tide_grafanadashboard.yaml
a0c41c0 to
d0e4ec2
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Adds health/liveness checks, per-controller (sync/status) loop cadence and completeness/error/partial-result tracking, GitHub search query duration and size distributions, merge activity by org, and GraphQL API usage attribution, replacing the previous, much thinner dashboard. Same dashboard uid, so existing links/bookmarks keep working.
d0e4ec2 to
beae161
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
[REHEARSALNOTIFIER] Note: If this PR includes changes to step registry files ( Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hector-vido, petr-muller The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
@petr-muller: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
Replaces the current Tide dashboard (
clusters/app.ci/openshift-user-workload-monitoring/mixins/_grafana_dashboards/tide.jsonnet) with a much more detailed one. Same dashboarduid, so existing bookmarks/links to it keep working.New/expanded sections:
user_agent) since the GitHub App identity Tide authenticates as is also shared by a few other bots.A live preview of the rendered dashboard is up at https://ci-route-ci-grafana.apps.ci.l2s4.p1.openshiftapps.com/d/tide-preview/ as a
tide-previewGrafanaDashboard applied directly toci-grafanaon app.ci (not part of this PR) — it'll be deleted once this replaces the real dashboard.Heads up on maintenance: several panels here (query completeness/error/partial-result rates, query duration, PRs-returned-per-query) depend on Tide metrics (
tide_query_errors_total,tide_query_partial_results_total,tide_query_duration_seconds,tide_query_prs_returned,tide_pool_completeness_ratio,tide_sync_query_shards) that only exist in the experimental Tide build currently running on app.ci (quay.io/prucek/tide:sharded-queries), not in upstreamkubernetes-sigs/prow. These need to be upstreamed for this to be sustainable long-term, and if their names, labels, or semantics change during that process, the corresponding panels here will need to be retuned to match.Summary by CodeRabbit
user_agent.app.cibuild. These metrics must be upstreamed, and the dashboard may need updates if their names, labels, or semantics change.