-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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
Track PR merge times #56304
Comments
I imagine you'd probably need to have a bot scrape through the PRs to get this information. @kartben |
It looks indeed like the LFX data might not be accurate. I have it on my agenda for next week to discuss with the LFX team how to fix the discrepancies we're seeing there. In the mean time, I've pulled the entire PR history and played with the data a bit: Note that this probably needs to be looked at together with number of outstanding PRs |
Just realized I looked at all closed PRs, not those that were closed by merging. |
Closed+Merged PRs only: This doesn't look too far off from the LFX data now. |
The list of PRs used for this would need to be set somehow, because things like hotfixes should be excluded as they will completely skew the results |
And maybe remove doc only changes too, since those also aren't relevant and have very short merge times, especially around release times |
Could you clarify what you mean by hotfixes? And what would be the criteria to flag them?
Mmmh ya, maybe, but in the other hand I would argue that looking at median/high-percentile/low-percentile might be enough to automagically not put too much emphasis on some of the outliers, while still allowing us to identify long-term trends. |
Anything with the hotfix label: https://github.com/zephyrproject-rtos/zephyr/pulls?q=is%3Apr+label%3AHotfix+is%3Aclosed
From my view, it's more important to see merge times for features or additions, nor minor changes. Anyone can review documentation, not everyone can review code or code for specific area/driver/system. Maybe even grouping things e.g. average time for a board to get merged, a driver, etc. |
It could be useful to break out PRs with the "Trivial" label as well: https://github.com/zephyrproject-rtos/zephyr/pulls?q=is%3Apr+is%3Amerged+label%3Atrivial+
+1: tracking PR velocity for new features is the main reason I opened this issue. |
@kartben - Your chart in #56304 (comment) looks like a great start on this effort. I'm curious how much the "days to close" increases once trivial and hotfixes are excluded. This issue has been added to the next Process Working Group agenda. @kartben and @nordicjm, do you think you'll be able to attend that meeting? |
The meeting is at 10:00 (UTC-7). Here's a link to the invite (requires a login): https://lists.zephyrproject.org/g/tsc/viewevent?repeatid=17893&eventid=1726025&calstart=2023-04-05 |
yes I plan on attending. |
@kartben I just saw your post on the tsc channel. Did you use https://github.com/kartben/zephyr-repo-metrics to generate this data? |
Hey @keith-zephyr -- I actually put the scripts for the PR "dump" here: https://github.com/kartben/gh-pr-stats Edit: there's also Excel and PowerBI files that can be used as a starting point |
Process WG:
|
I updated and ran some of my scripts to extract more info re: first-time contributors, and thought I would share an update -- both in terms of charts and words (since I feel like my dataviz fu could be improved :)) What you're looking at is (still) the age of a merged PR at the time it is being merged. The repetition of the word "merge" is intentional, as it's important to keep in mind that these charts don't give any insight into "rotting" PRs -i.e. still opened- or PRs that were closed without being merged, and there would probably be some work to be done to extract insights related to these categories of PRs. Ways to read the data:
All PRs: PRs for first-time contributors: /cc @carlescufi |
How hard would it be to add 3 more columns to this data? I would love to see:
This would also allow us to start looking at other factors for PR lifespans such as churn. I know I dread getting a comment like "you missed a period in the documentation" after I already got 1+ approvals and throwing the approval away for a typo. |
The last 2 are pretty easy but not sure there is even a way to get #1, but I'll look into it. Edit: number of force pushes is probably available through https://docs.github.com/en/rest/issues/timeline?apiVersion=2022-11-28 FWIW if we do think about trying to make something useful out of the number of force pushes we need to find a way to exclude the ones made while a PR might have been in draft state. |
@kartben - can you provide some updated charts on this issue? It would be helpful to revisit this data, determine if there are any conclusions we can determine from the data and if there are action items to fix problems. |
@keith-zephyr - suggests running this once a quarter |
Proposal
With the recent publication of Zephyr's Contributor and Reviewer Expectations, it would be valuable to measure and track PR merge times. This data could inform whether policy changes are helping or hindering the velocity of the project.
LFX insights provides some of this data, but I don't know the methodology used, and the numbers don't look correct. The times seem far too low and the most recent data might be incomplete.
GitHub's Insights view only shows the total number of commits per week, without any data for merge and review times.
Tracking the median time for PRs to get the first review and to merge would be a good place to start. This tracking could later be enhanced to distinguish between bug fixes and new features. The expectation is that bug fixes will merge fairly quickly, while new features go through additional review.
The text was updated successfully, but these errors were encountered: