Skip to content

Drop the cardinality panel expectations and unskip PMM-T506 - #1275

Merged
travagliad merged 3 commits into
mainfrom
claude/jolly-curie-dee5l7
Sep 3, 2026
Merged

Drop the cardinality panel expectations and unskip PMM-T506#1275
travagliad merged 3 commits into
mainfrom
claude/jolly-curie-dee5l7

Conversation

@travagliad

@travagliad travagliad commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Failures fixed (investigator)

  • source: nightly CI run https://github.com/percona/pmm-qa/actions/runs/33222594797 (Nightly E2E tests Matrix (remote PMM Server), job test execution / @nightly) — also red Aug 30 and Aug 31 on the same test
  • tests:
    • codeceptjs-e2e/tests/verifyVMDashboards_test.js:9 / @nightly @dashboards @gssapi-nightly — PMM-T506 Verify metrics on VictoriaMetrics dashboard

What failed

The only red job of 17, and the only actionable failure in it (Launchable gate: 1 actionable, 0 quarantined; 70 passed, 1 failed, 44 skipped):

PMM-T506 - Verify metrics on VictoriaMetrics dashboard:

element ([data-testid^="data-testid Panel header Top 10 metrics by time series count"])
  still not present on page after 5 sec
  at async Object.verifyMetricsExistence (tests/pages/dashboardPage.js:1200:7)

Root cause — a stale expectation, not a product bug

victoriaMetricsDashboard.metrics expects a Time Series Information row holding
Top 10 metrics by time series count and Top 10 hosts by time series count.
percona/pmm#5822 (PMM-15228, merged 2026-08-28)
removed both deliberately, along with the row — in Alex's words:

Yes!! They were literally taking PMM Server down under high load, i.e. 300+ clients.

Their {__name__=~".+"} selectors force a full index scan, and PMM raises VictoriaMetrics' ceilings
past where it would refuse them (defaultVMSearchMaxUniqueTimeseries = "100000000",
defaultVMSearchMaxSamplesPerQuery = "1500000000"managed/services/supervisord/supervisord.go:52-53),
so it runs them. Worst of all on the dashboard an operator opens when the server is already
struggling. The removal is intentional and justified, so the test is what needed updating.

Contents

1. Drop the two stale expectations (dashboardPage.js) — the original fix.

2. Unskip PMM-T506 (verifyVMDashboards_test.js) — added after review.

main was muted by #1288 while this PR's objection was being resolved. This branch forked before
that landed, so in a three-way merge the base and this branch agree on the test file while main
added the skip — merging the expectation fix alone would have kept Scenario.skip, leaving the
test dark and the nightly unchanged. main is now merged in and the skip removed on top, so one
merge both corrects the expectations and turns the test back on. The skip-until: 2026-09-29 marker
goes with it, so #1288's lint tripwire has nothing dangling to fire on.

#1288's guard and weekly lint schedule stay in place for future skips. The separate follow-up PR
(#1310) is closed as redundant.

Note: the approval on this PR was dismissed by the push that added part 2 — re-review requested.

Verification

Reproduced on a throwaway Linode VM at perconalab/pmm-server:3-dev-latest, digest
sha256:fe34e198…the identical image the failing nightly ran against. The Grafana API on the
live server confirms both panels absent:

NOT FOUND: Top 10 metrics by time series count
NOT FOUND: Top 10 hosts by time series count

Before/after on that VM:

branch result
main FAIL — same step and stack frames as the nightly (dashboardPage.js:1201)
this branch OK | 1 passed // 1m

On this branch as it now stands:

  • Net diff against main is exactly the two intended changes, nothing else: -2 in dashboardPage.js, -2/+1 in verifyVMDashboards_test.js.
  • PMM-T506 reports rather than S — it executes instead of being skipped.
  • 0 of 393 tracked .js/.ts files carry a skip-until marker; the skip-until expiry check runs clean (rc=0).
  • Repo linters clean on both changed files.

One honest caveat: nothing in this PR's CI runs the @dashboards scenario, so the green checks mean
nothing broke, not that the fix works — the VM before/after is the evidence for that, and the next
nightly is the confirmation under real CI conditions.

Note — not fixed here

prometheusDashboard (dashboardPage.js:115-116) carries the same two stale titles, but it is dead
code: nothing references it, and its URL graph/d/prometheus/prometheus is a PMM 1.x dashboard that
no longer exists. Left alone to keep this diff minimal; worth deleting the whole unused block separately.


🤖 Generated with Claude Code

https://claude.ai/code/session_01SKRuqWTQVW4p6DDSYcFQEf

PMM-T506 expects a 'Time Series Information' row on the VictoriaMetrics
dashboard holding 'Top 10 metrics by time series count' and 'Top 10 hosts
by time series count'. percona/pmm#5822 deliberately removed both: their
match-all selectors force a full index scan and can take a large server
down. Drop them from the expected-metrics list.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Claude <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Aug 29, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 69c9d0ca-eea7-44d7-815d-dbd13a580ac3

📥 Commits

Reviewing files that changed from the base of the PR and between 0b9d4d9 and 3b931fd.

📒 Files selected for processing (1)
  • codeceptjs-e2e/tests/pages/dashboardPage.js
🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

  • percona/pmm-qa (manual)
  • percona/pmm (manual)
💤 Files with no reviewable changes (1)
  • codeceptjs-e2e/tests/pages/dashboardPage.js

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


Walkthrough

The dashboard metric list removes Top 10 metrics by time series count and Top 10 hosts by time series count. The remaining metrics are unchanged.

Merge Risk: ⚪ Minimal · up to 3b931

This change removes two dashboard expectations that no longer exist, without changing product behavior. No actionable merge-blocking risk remains beyond normal checks and review.

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Description check ✅ Passed The description clearly explains that the test expectations were updated because PMM-15228 removed the two VictoriaMetrics cardinality panels. It also documents the failure, root cause, and verificati…
Title check ✅ Passed The title clearly identifies the removal of the two stale cardinality-panel expectations and references PMM-15228.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

The description clearly explains that the test expectations were updated because PMM-15228 removed the two VictoriaMetrics cardinality panels. It also documents the failure, root cause, and verification.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@claude

claude Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

LGTM!

Gate gap, not a finding on the diff: nothing in CI covers this change. lint.yml runs eslint only for the e2e_tests and cli TypeScript workspaces, so no linter reads codeceptjs-e2e/**/*.js, and no check on this PR runs the @dashboards scenario the diff fixes — the before/after on the Linode VM in the body is the only evidence for it.

Comment thread codeceptjs-e2e/tests/pages/dashboardPage.js
@travagliad

Copy link
Copy Markdown
Contributor Author

@claude unskip the test that we skipped because of this failure

@claude

claude Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Claude finished @travagliad's task in 6m 28s —— View job


Unskip PMM-T506 — blocked here, and it's a merge-order problem, not an edit

  • Read the trigger request
  • Locate the skip
  • Check whether it can be removed on this branch
  • Push the unskip — can't from this job; exact steps below

Where the skip is

main, not this branch — codeceptjs-e2e/tests/verifyVMDashboards_test.js:9-10:

// skip-until: 2026-09-29 -- cardinality panels removed by percona/pmm#5822; unskip via pmm-qa#1275, or when the panels return.
Scenario.skip(
  'PMM-T506 - Verify metrics on VictoriaMetrics dashboard @nightly  @dashboards @gssapi-nightly',

That is the only skip attributable to this failure — a git grep for skip-until and for 5822/PMM-15228 across main returns that one line and nothing else.

Why I can't just delete it here

This branch forked before the skip landed (it's 77 files behind main), so its copy of the file still reads a plain Scenario( — there is nothing to delete on this side. And that's exactly the trap: in the three-way merge, the base and this branch have identical content for that file while main added the skip, so merging this PR as-is keeps Scenario.skip. The test stays off, and the skip-until: 2026-09-29 tripwire in lint-changed.sh fires on that date.

Fixing it requires main merged into this branch first, then the skip removed on top. I can't do that from this job:

  • git fetch / git merge / gh are all outside this run's allowed tools (main is here at depth 1, with no merge base).
  • Even with them, the push would be rejected: main has moved .github/workflows/lint.yml since the fork, and a GitHub App can't push a commit that touches .github/workflows/**.

Two ways to land it

Easiest — and it puts the last step back in my hands: click Update branch on this PR (that merge commit is yours, so the workflow-file restriction doesn't apply), then comment @claude unskip PMM-T506. Once the branch actually contains the skip, removing it touches no workflow file and I can commit it. [Or hand it to a fresh session →](https://claude.ai/code?q=In%20percona%2Fpmm-qa%20on%20branch%20claude%2Fjolly-curie-dee5l7%20(PR%20%231275)%3A%20merge%20origin%2Fmain%20into%20the%20branch%2C%20then%20in%20codeceptjs-e2e%2Ftests%2FverifyVMDashboards_test.js%20delete%20the%20%60%2F%2F%20skip-until%3A%202026-09-29%60%20comment%20and%20change%20%60Scenario.skip(%60%20back%20to%20%60Scenario(%60%20for%20PMM-T506%2C%20then%20commit%20and%20push.&repo=percona/pmm-qa)

Or by hand:

git fetch origin main
git switch claude/jolly-curie-dee5l7
git merge origin/main
# codeceptjs-e2e/tests/verifyVMDashboards_test.js: drop the skip-until comment,
# change Scenario.skip( back to Scenario(
git commit -am "Unskip PMM-T506 now the stale expectations are gone"
git push

Merging this PR first and unskipping in a two-line follow-up off main works too — same end state, one extra PR.

Not done

No commit was pushed: there was nothing on this branch to edit, and constructing the merge here isn't something this job can push. The diff under review is unchanged (-2 lines in dashboardPage.js).
· claude/jolly-curie-dee5l7

pmm-qa#1288 muted the test while the VictoriaMetrics panel question was
open. This PR drops the two expectations the panels no longer satisfy,
so the mute has done its job and is removed here rather than in a
follow-up: merging the expectation fix alone would keep Scenario.skip
(main added it after this branch forked) and leave the test dark.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SKRuqWTQVW4p6DDSYcFQEf
Signed-off-by: Claude <noreply@anthropic.com>
@travagliad
travagliad merged commit 0564b11 into main Sep 3, 2026
2 of 34 checks passed
@travagliad
travagliad deleted the claude/jolly-curie-dee5l7 branch September 3, 2026 12:07
@travagliad travagliad changed the title Drop the two cardinality panels PMM-15228 removed Drop the cardinality panel expectations and unskip PMM-T506 Sep 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants