Conversation
Real-Time Analytics now supports MySQL (PMM-15283, percona/pmm#5509); add QA automation verified against the PR-4417-11f4346 feature build: - MySQLHelper workload generator (labeled SELECT SLEEP() statements visible in sys.x$processlist) with a mySqlDbHelper fixture; defaults match the QA framework ps setup, overridable via MYSQL_* env vars. - mysql.test.ts (@RTA, 6 tests): overview shows a running MySQL query with host/database/user; details pane shows MySQL attributes and the raw processlist row; Hide-COMMIT toggle; Database/User faceted multi-select filters; MongoDB and MySQL rows side by side with payload-specific details; CSV export contains the MySQL columns. Mocked search responses use the snake_case wire format. - CLI: add/change/remove flow for rta-mysql-agent in the percona-server suite, gated on pmm-client >= 3.9.0. - RealTimeAnalyticsPage: update position-based column locators for the new layout (Database 3, User 4, Operation ID 5, Elapsed time 6 pinned right) and add locators for the new controls; fix the T2252 export filename assertion (mongodb_rta_export_ -> rta_export_). - CI: the @RTA feature-build job also provisions ps=8.4. - Add mysql2 dependency.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe change adds MySQL provisioning and helpers, CLI RTA lifecycle coverage, and Playwright coverage for MySQL RTA display, filtering, details, COMMIT visibility, mixed engines, elapsed time, and CSV export. ChangesMySQL RTA coverage
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![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
🧹 Nitpick comments (1)
e2e_tests/helpers/mysql.helper.ts (1)
42-53: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winUse a parameterized query instead of manual string escaping.
simulateLongRunningQuerybuilds the query with manual escaping (escapedLabel) instead of a placeholder. Static analysis flags this pattern as a SQL injection risk (CWE-89).queryLabelis only ever a literal from test code today, but rolling manual escaping is fragile and unnecessary whenmysql2supports?placeholders natively.Use a parameterized query to eliminate the manual escaping and the static-analysis finding.
🔒️ Proposed fix using a parameterized query
simulateLongRunningQuery = async ( options: { delayMs?: number; queryLabel?: string; } = {}, ) => { const { delayMs = Timeouts.TEN_SECONDS, queryLabel = 'rta-simulated-query' } = options; - const escapedLabel = queryLabel.replace(/\\/g, '\\\\').replace(/'/g, "''"); const delaySeconds = Math.max(1, Math.ceil(delayMs / 1_000)); - return this.pool.query(`SELECT '${escapedLabel}', SLEEP(${delaySeconds})`); + return this.pool.query('SELECT ?, SLEEP(?)', [queryLabel, delaySeconds]); };🤖 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 `@e2e_tests/helpers/mysql.helper.ts` around lines 42 - 53, Update simulateLongRunningQuery to use a mysql2 parameterized query with a ? placeholder for queryLabel and pass the label through the query parameters, removing the escapedLabel manual escaping. Preserve the existing delaySeconds calculation and SLEEP behavior.Source: Linters/SAST tools
🤖 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 `@cli/tests/perconaMySqlServer.spec.ts`:
- Around line 248-250: Update the affected test steps around the MySQL RTA agent
operations to call output.assertSuccess() after each cli.exec() and before or
alongside outContains(), matching the existing pattern near lines 241-242; apply
this consistently to the steps at the referenced ranges while preserving their
current output assertions.
- Line 235: Update the version gate in the perconaMySqlServer test to compare
the full pmm-client major and minor version against 3.9.0, rather than relying
on the minor value returned by getPmmAdminMinorVersion. Ensure 4.0.0 is accepted
and 2.10.0 is skipped while preserving the existing test.skip behavior.
In `@e2e_tests/tests/qan/rta/mysql.test.ts`:
- Around line 71-80: Update both tests’ “Simulate long running MySQL query”
steps to retain the promise returned by mySqlDbHelper.simulateLongRunningQuery,
attach error handling that preserves failures, and await or cancel it before
fixture teardown. Ensure the query does not remain floating after the
rowByQueryText assertion, avoiding delayed pool.end() cleanup while still
propagating query errors.
- Around line 10-34: Add the required string state field to the object returned
by buildMySqlQuery, using a representative value such as “executing” so the mock
conforms to the wire contract while preserving the existing query fields.
---
Nitpick comments:
In `@e2e_tests/helpers/mysql.helper.ts`:
- Around line 42-53: Update simulateLongRunningQuery to use a mysql2
parameterized query with a ? placeholder for queryLabel and pass the label
through the query parameters, removing the escapedLabel manual escaping.
Preserve the existing delaySeconds calculation and SLEEP behavior.
🪄 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: 3063f4b8-cbca-4f49-858a-02a795577c2a
⛔ Files ignored due to path filters (1)
e2e_tests/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
.github/workflows/fb-e2e-suite.ymlcli/tests/perconaMySqlServer.spec.tse2e_tests/fixtures/pmmTest.tse2e_tests/helpers/mysql.helper.tse2e_tests/package.jsone2e_tests/pages/qan/rta/realTimeAnalytics.page.tse2e_tests/tests/qan/rta/mysql.test.tse2e_tests/tests/qan/rta/overview.test.ts
- CLI: assert the exit code alongside the output message in the RTA agent add/change/remove steps. - e2e: add the state field to the mocked MySQL payload to match the wire contract. - MySQLHelper: run long queries on dedicated tracked connections and destroy them on close(), so fixture teardown no longer waits out SLEEP() statements (the two live tests drop from ~31s to ~4s each); teardown-induced aborts are suppressed while real query failures still surface.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@e2e_tests/helpers/mysql.helper.ts`:
- Around line 70-75: Create the MySQL connection within the try block in the
long-running query method, then check the helper’s closed state immediately
after creation and destroy and return without tracking it if shutdown has begun.
Track active connections only after that check, and add finally cleanup to
remove the connection from longRunningConnections and destroy it after the query
completes or fails.
🪄 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: de6dac9d-0012-40b9-8a35-08baf08a68fa
📒 Files selected for processing (3)
cli/tests/perconaMySqlServer.spec.tse2e_tests/helpers/mysql.helper.tse2e_tests/tests/qan/rta/mysql.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e_tests/tests/qan/rta/mysql.test.ts
…isibility and connection management
The RTA overview now hides Database and User by default (percona/pmm#5509), so the QA coverage is updated to match: - cells are addressed by their query-<id>-<name>-cell test id instead of by column position, which is no longer stable when columns are hidden; - elapsed time is parsed from the compact form ('1.5s', '42s') the overview renders, where splitting on a space returned NaN; - showColumns() reveals Database and User through the Show/Hide columns menu for the tests that assert or filter on them; - new tests cover the hidden-by-default columns and the compact elapsed time format.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
e2e_tests/pages/qan/rta/realTimeAnalytics.page.ts (1)
21-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a stable locator for the MongoDB code block.
code.language-mongodbis a CSS-class selector. We must not make a syntax-highlighting class part of the Grafana test contract. Use adata-testidor an accessible locator. If the UI has no stable locator, add one before using this page object. Make it so.As per coding guidelines, use
data-testidlocators where available and do not use CSS class selectors for Grafana elements.🤖 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 `@e2e_tests/pages/qan/rta/realTimeAnalytics.page.ts` around lines 21 - 24, Update detailsPaneCodeByText to remove the syntax-highlighting selector code.language-mongodb and use the MongoDB code block’s stable data-testid or accessible locator instead. If no stable locator exists in the UI, add one there first, then reference it from this page object while preserving the queryText filter.Source: Coding guidelines
🤖 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 `@e2e_tests/pages/qan/rta/realTimeAnalytics.page.ts`:
- Around line 203-205: Update the checkbox interaction in showColumns to call
check() instead of click(), ensuring repeated calls enforce column visibility
without hiding already visible columns.
---
Nitpick comments:
In `@e2e_tests/pages/qan/rta/realTimeAnalytics.page.ts`:
- Around line 21-24: Update detailsPaneCodeByText to remove the
syntax-highlighting selector code.language-mongodb and use the MongoDB code
block’s stable data-testid or accessible locator instead. If no stable locator
exists in the UI, add one there first, then reference it from this page object
while preserving the queryText filter.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f7dc2ea8-3e66-42e1-b8f8-bc024606e081
📒 Files selected for processing (2)
e2e_tests/pages/qan/rta/realTimeAnalytics.page.tse2e_tests/tests/qan/rta/mysql.test.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
percona/pmm-qa(manual)percona/pmm(manual) → reviewed against open PR#5509rta-mysql-testinstead of the default branch
check() leaves an already visible column visible, where click() toggled it back off. No test hits that today - each caller runs in its own test, where the columns start hidden - but the helper is now safe to call more than once and if a column ever becomes visible by default.
The overview keeps three decimals below ten seconds (percona/pmm#5509), so the mocked two-second query now renders as '2.000s'.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Real-Time Analytics now supports MySQL (PMM-15283, percona/pmm#5509); add QA automation verified against the PR-4417-11f4346 feature build:
Summary by CodeRabbit
New Features
Bug Fixes
rta_export_YYYYMMDD_HHMMSS.csvfilename format.