-
Notifications
You must be signed in to change notification settings - Fork 843
Add Jetpack performance testing CI infrastructure #46287
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: trunk
Are you sure you want to change the base?
Conversation
Introduces automated LCP (Largest Contentful Paint) measurement for the wp-admin dashboard with simulated Jetpack WordPress.com connection. Key components: - Docker environment with WordPress + simulated Jetpack connection - CPU throttling calibration for consistent results across CI agents - Playwright-based LCP measurement - CodeVitals integration for metric tracking Metric posted: wp-admin-dashboard-connection-sim-largestContentfulPaint
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
- Remove outdated 4-scenario comment (only 1 scenario exists) - Remove dead baseline comparison code that never executed - Trim JSDoc to single-line descriptions across all scripts - Update eslint config to allow minimal JSDoc
Changed the Docker startup sequence to prevent race conditions where WordPress containers interfere with WP-CLI's database operations: 1. Start only the db container 2. Wait for MySQL to be ready 3. Run WP-CLI setup (WordPress container NOT running) 4. Start WordPress containers This ensures WP-CLI has exclusive database access during setup, eliminating "table doesn't exist" errors caused by concurrent access. Changes: - run-performance-tests.js: Sequential container startup - setup-wordpress.sh: Simplified (removed HTTP wait logic) - docker-compose.yml: Removed wpcli depends_on wordpress
The import plugin is not available in this context, and the base config handles import resolution. Also ensure jsdoc rules are disabled for these utility scripts.
Dependencies (playwright, dotenv) are in tools/performance/package.json, not the monorepo root, so the import resolver can't find them.
- Log calibration file path and existence - Show throttle rate, target score, calibration time, and samples - Confirm throttling is applied via CDP on first iteration
…or tools/performance
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Code Coverage SummaryThis PR did not change code coverage! That could be good or bad, depending on the situation. Everything covered before, and still is? Great! Nothing was covered before? Not so great. 🤷 |
…wordpress-jetpack-connected service, ensuring it starts only after the service is initiated, which helps prevent race conditions during container startup.
- Updated README.md to include new environment variables: CODEVITALS_URL, GIT_BRANCH, WP_ADMIN_USER, and WP_ADMIN_PASS. - Modified post-to-codevitals.js to streamline metric extraction by removing unused baseMetrics. - Improved run-performance-tests.js to prioritize GIT_COMMIT environment variable for git hash retrieval, ensuring accurate tracking during CI runs.
- Introduced an empty baseMetrics object in the payload to clarify that baseline normalization is not utilized in the performance metrics submission.
- Simplified browser launch to always use headless mode for consistency in performance calibration. - Removed conditional logic for headful mode, ensuring a streamlined execution in both local and CI environments.
|
Hey @anomiex👋 would you mind taking a look at this when you get a chance? This is the performance testing infrastructure I built during HACK week (more details here pc9hqz-3Rb-p2) it measures wp-admin dashboard LCP for Jetpack and posts results to CodeVitals.
The mu-plugin is probably the most relevant bit for review from a Jetpack perspective as it intercepts pre_http_request to return mock responses for various WP.com endpoints. Happy to walk through any of it if that's easier. |
anomiex
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems ok from a monorepo perspective. I didn't look too closely at the code.
If you're wanting a review of the faked-connection stuff in the mu-plugin, @Automattic/jetpack-vulcan would be the team to ask.
tools/performance/README.md
Outdated
|
|
||
| ## CI Usage | ||
|
|
||
| The test suite is designed to run in TeamCity. See build configuration for setup. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it'd make more sense to run it in Actions rather than TeamCity, on each commit to trunk instead of backfilling weekly.
If you're running it in TeamCity, are you looking at the monorepo, or at https://github.com/Automattic/jetpack-production which has the already-built plugin?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One of the exploration intents is to see about reusing work. With our GHE instance, TeamCity is the runner we have to use, so seeing the pros/cons of TC here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it'd make more sense to run it in Actions rather than TeamCity, on each commit to trunk instead of backfilling weekly.
GitHub Actions runners can have unpredictable performance variability due to shared infrastructure. For detecting small regressions (5-10%), this noise can drown out minor regressions over time. The TeamCity build runs on a dedicated machine with no other agents interfering.
If you're running it in TeamCity, are you looking at the monorepo, or at Automattic/jetpack-production which has the already-built plugin?
The monorep is used instead of the already-built plugin mainly so we can leverage commit level tracking, so we can have the ability to bisect when issues appear (where needed). It may not be needed for now though, but you can see within CodeVitals that a point can be clicked and we can see the commit details when clicking on the commit SHA.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that every commit that changes anything in Jetpack gets mirrored to Automattic/jetpack-production, absent rare cases where something goes wrong with the mirroring. Each commit there also includes a footer like Upstream-Ref: Automattic/jetpack@d5b54134bb471f3a54d04b12e128e6e0e2d77bde to make it easy to find the corresponding monorepo commit.
It's up to you, but it may save having to build each monorepo commit before you can test it.
It'll also either save you testing commits that don't affect Jetpack-the-plugin at all or save you having to maintain code to try to identify which ones do. I see your P2 post mentions
that touched PHP files in Jetpack or packages
That would unnecessarily test changes to packages like packages/jetpack-mu-wpcom, unless you have a list of packages to ignore. It could also miss JS-only changes that might still affect the LCP timing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that every commit that changes anything in Jetpack gets mirrored to Automattic/jetpack-production, absent rare cases where something goes wrong with the mirroring. Each commit there also includes a footer like Upstream-Ref: d5b5413 to make it easy to find the corresponding monorepo commit.
Actually yes! I should have checked this before hand, I didn't realise Automattic/jetpack-production has bissectable commits like this. It'll also help regarding the issue you mentioned for testing commits that don't affect Jetpack-the-plugin also. Thanks, I'll update the TeamCity build to use that instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to use jetpack-production, the implementation now:
- Clones jetpack-production instead of building from the monorepo
- Parses Upstream-Ref from each mirror commit to track the original monorepo SHA in CodeVitals
- Removes file filtering from the TC scheduler
The monorepo VCS root is still used for the tools/performance/ scripts, but the plugin itself comes directly from the pre-built mirror.
tools/performance/eslint.config.mjs
Outdated
| // Dependencies are in tools/performance/package.json, not monorepo root | ||
| // so the import resolver can't find them. Disable this rule. | ||
| 'import/no-unresolved': 'off', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That doesn't matter, eslint will look in tools/performance/node_modules for files under here.
Also, when I remove this and run eslint, it doesn't seem to complain about anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch! Removed the import/no-unresolved rule and its comment. Also simplified the config to match tools/cli/eslint.config.mjs
kraftbj
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure where it came from; I was given a noticed that MacOS's rsync doesn't handle symlinks well and stating I should brew install rsync. It let me proceed, but it failed.
Installing rsync via brew install rsync and trying again worked. I don't think it's a blocker but wanted to note it.
Probably from jetpack/tools/cli/commands/rsync.js Lines 45 to 93 in 55e48ba
|
Yup! Thanks! I scanned the PR but didn't check the existing commands. I wouldn't consider it a blocker for the PR, but it is required to use the fully-leaded version of |
- Consolidated rules into a single object for improved readability and maintainability. - Removed unnecessary comments and streamlined the configuration structure.
Actually, this is no longer relevant - I've switched to using jetpack-production (the pre-built mirror) instead of |
- Added 'plugin/' directory to .gitignore to exclude cloned plugin files. - Updated README.md with detailed setup instructions and clarified usage of the pre-built Jetpack plugin. - Modified docker-compose.yml to mount the plugin from the new directory structure. - Refactored run-performance-tests.js to clone the Jetpack plugin from the production mirror instead of using rsync, ensuring a more straightforward setup process.
|
Hey @Automattic/jetpack-vulcan 👋 I've been working on performance testing infrastructure for Jetpack (measuring wp-admin LCP with Jetpack connected). It currently:
Specifically looking for feedback on:
|
|
Hi @LiamSarsfield , Thanks for the ping and working on this!
Yes 👍
This would depend on the Jetpack plugin module configuration. I can confirm the endpoints that are called by Connection and Sync packages, but we can't know how each consumer of the Jetpack Connection behaves. That said, it might make sense to add some logging that would answer this question within the performance testing logic itself? This could be helpful in case more endpoints are added in the future, that we don't handle within the testing infrastructure.
This is not a blocker, but you could consider refactoring |
- Introduced a mechanism to log unhandled endpoints, aiding in the identification of missing mock responses. - Added a flag to track if a specific endpoint handler was matched, improving response handling clarity. - Updated comments for legacy endpoints to indicate early returns without logging.
|
@fgiannar Thanks for the thorough review!
Good point. I initially took the conservative approach to avoid errors from modules expecting real WP.com responses, but enabling all modules would give us a more realistic measurement. I'll look into expanding the module list and adding mock responses for any additional endpoints they require.
Great idea, I added logging for any intercepted requests that hit the fallback response. That way it'll catch any unhandled endpoints as they appear instead of discovering them later.
Agreed that the current implementation is a bit unwieldy. I'll refactor to a registry/factory pattern which will also make it easier to add per endpoint configuration.
Love this idea. Using actual latency data would make the measurements much more representative. I created a follow-up issue to:
For now I'll focus on the logging improvement as a quick win, thanks again for the detailed feedback! 🙏 |
Addresses HOG-438: Create Jetpack Performance Tooling for LCP
Proposed changes:
tools/performance/to measure wp-admin dashboard LCP (LargestContentful Paint) with Jetpack connected
with 200ms latency)
Other information:
a script to run)?
Jetpack product discussion
pc9hqz-3Rb-p2
Does this pull request change what data or activity we track or use?
No
Testing instructions:
Prerequisites: Docker running, Node 18+
The test suite will automatically clone the pre-built plugin from jetpack-production on first run.
Expected output: LCP measurement for wp-admin dashboard with Jetpack connected (simulated)