Skip to content

Conversation

alecgrieser
Copy link
Collaborator

@alecgrieser alecgrieser commented Sep 22, 2025

This adds an automatic tool to review metrics files. It should run as part of PRB and post back the results of computing the diff.

You can see a version of its sample output here: https://github.com/FoundationDB/fdb-record-layer/actions/runs/17975684135?pr=3620 That particular version was modified so that it would use 4.5.10.0 as the base branch. The extra queries there account for why there were so many identified in the analysis.

The basic structure of the report is:

  1. A basic summary of the number of queries added, dropped, and changed
  2. The number of new queries (per file)
  3. A list of dropped queries
  4. Statistics for query metric changes for queries that have modified their plans, along with a list of outlier queries with large regressions
  5. The same for queries where only the metrics changed

Those two categories of queries are separated out as we expect some amount of churn whenever a query plan changes, as it requires that there is some larger kind of planner change. When only the metrics change (particularly when there is a regression), that can be indicative of some kind of problem, as the planner may have done more work without seeing a better outcome.

One thing that I have not been able to test is the ability to post back comments to the PR because pull_request events don't have write access (even for comments). The current version uses pull_request_target to do that, which should be able to post comments back to the PR (including in-line comments), though I have not been able to test that. I did also look at using GitHub annotations, which would be more forgiving, but those have the downside of being limited to something like 10 in a single step as well as not supporting markdown formatting. So comments seem preferable if we can get them to work.

@alecgrieser alecgrieser added the testing improvement Change that improves our testing label Sep 23, 2025
@alecgrieser alecgrieser marked this pull request as ready for review September 24, 2025 13:12
@normen662 normen662 self-requested a review September 24, 2025 13:57
Copy link
Contributor

@normen662 normen662 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing!

@alecgrieser alecgrieser merged commit 9c61f92 into FoundationDB:main Sep 24, 2025
8 checks passed
@alecgrieser alecgrieser deleted the metrics-compare-tool branch September 24, 2025 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing improvement Change that improves our testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants