Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Status Chart: Track the combined age of the PR backlog #1043

Closed
StephanTLavavej opened this issue Jul 14, 2020 · 2 comments · Fixed by #1056
Closed

Status Chart: Track the combined age of the PR backlog #1043

StephanTLavavej opened this issue Jul 14, 2020 · 2 comments · Fixed by #1056
Assignees
@StephanTLavavej
Labels
documentation Related to documentation or comments fixed Something works now, yay!

Comments

@StephanTLavavej
Copy link
Member

Suggested by @AlexGuteniev: #921 (comment)

Tracking the size of the PR backlog is useful, but its age is another important dimension. (If we had 200 PRs open at any given time, but PRs spent no more than 1 month being reviewed, that would be amazing.) Among the possible options:

  • Max age seems minimally informative and useful; it would ignore everything that's happening except for the oldest PR. Working on the oldest PR is important, but not necessarily all-important.
  • Average age (whether mean or median) seems like a misleading metric. An influx of new PRs would lower the average age, but would be "more work", so that's the wrong direction.
  • Total age seems to capture the essence of how the PR backlog weighs on my mind. It increases N days per day, and newly opened PRs don't immediately improve or harm the metric, they just cause it to increase faster. Merging an old PR is then "rewarded" in proportion to its age, which feels right. Merging a bunch of new PRs at least limits the growth rate.

This information is within my newfound power as a JavaScript programmer to gather. 😺

I think it will need to be a separate chart (on the same page), though - the units are completely different from the existing lines, and the line will behave differently too.
@StephanTLavavej StephanTLavavej added the documentation Related to documentation or comments label Jul 14, 2020
@StephanTLavavej StephanTLavavej self-assigned this Jul 14, 2020
@cbezault
Copy link
Contributor

This might be beyond your JS abilities right now but I think the salient metric is total number of days since a maintainer posted a review with changes requested (or if it's a new PR days since creation).

This is just a variant of your "total age" option which I agree seems the most useful of the above metrics.

@StephanTLavavej
Copy link
Member Author

That appears to be doable (with significantly more logic). I see how to list PR reviews, and I believe I see how to query whether a user is a member of the vclibs team. I'll probably explore that after getting the chart initially working. Thanks!

StephanTLavavej added a commit to StephanTLavavej/STL that referenced this issue Jul 19, 2020
@StephanTLavavej
Add GitHub API script, daily table, Combined PR Age chart.
06e958d 
Fixes microsoft#1043.

* `gather_stats.js`
  + This uses the GitHub API to gather PR/issue statistics. It runs on the console (not in the browser), using the latest version of Node.js (currently 14.5.0). I've significantly improved it since my initial version in microsoft#1038 that wasn't checked into the repo:
    - This loads a personal access token from a `.env` file, avoiding the danger of hardcoding it in the script.
    - Because it currently takes about 20 seconds to list all PRs/issues through the GitHub API, the script displays a command-line progress bar.
    - For efficiency, the script transforms PR/issue data as it's received (instead of repeatedly during analysis).
    - The script now analyzes the data with daily resolution. Accordingly, I've adjusted the beginning date.
    - The script now writes a file, `daily_table.js`, complete with a "generated file" warning comment.
    - The script now calculates combined PR age.
* `daily_table.js`
  + The generation process is stable, so regenerating this file will typically add rows to the bottom with no other changes. However, it's possible for previous rows to change (e.g. due to relabeling or reopening).
* `weekly_table.js`
  + I extracted the classic table, still updated manually, for symmetry. This also makes the source and history of `index.html` easier to read. Eventually, I plan to generate the `cxx20` and `lwg` stats, and we'll stop tracking `vso` after porting all old bugs, leaving only `libcxx` that can't be easily generated yet.
* `.gitignore`
  + We need to ignore the `node_modules` directory that `npm` creates. We also need to ignore the `.env` file, but I can't tell you why - it's a secret.
* `package.json`
  + This is generated by `npm init` and is updated when dependencies are installed. I filled out some reasonable information in response to its questions, but I don't intend to publish this in the registry (as it is hardcoded for our repo and not intended for general use, although it could serve as the basis for such work).
* `package-lock.json`
  + This is also a generated file, and I don't really understand its purpose, except that it's supposed to be checked in (something about pinning the exact versions of all transitive dependencies).
* `index.html`
  + Add `hr` styling, to separate the two charts.
  + Include `daily_table.js` and `weekly_table.js`.
  + Move the main `<script>` into the `<head>`, making it easier to read the logic versus the content. (This required only one change, moving the "Toggle Timeframe" button's `addEventListener` to `window.onload`, which is invoked after the page has finished loading.)
  + Generalize `get_values` because there are two tables now.
  + Rename `chart_data` to `status_data` (and similarly for `chart_options`) because there are two charts now.
  + Add `age_data`.
  + Give `timeframe_all` and `timeframe_github` names, making it easier to refer to them later (`timeframes[0]` was unnecessarily mysterious).
  + Extract `common_options` used by both charts.
  + Extract a `make_xAxes` function, as both charts use the same x-axis, but with different timeframes.
  + `...common_options` is "spread syntax".
  + Add `age_options` for the new chart. Note that attempting to centralize their `title` configuration (where `display` and `fontSize` are the same, but `text` is different) seems like more trouble than it's worth.
  + Construct the `age_chart`, and move the button logic as previously mentioned.
  + Add the `<canvas id="ageChart">` followed by an explanation.
@StephanTLavavej StephanTLavavej mentioned this issue Jul 19, 2020
Merged
@StephanTLavavej StephanTLavavej linked a pull request Jul 19, 2020 that will close this issue
Status Chart: Add Combined PR Age and GitHub API script #1056
Merged
@StephanTLavavej StephanTLavavej added fixed Something works now, yay! and removed work in progress labels Jul 20, 2020
@StephanTLavavej StephanTLavavej mentioned this issue Jul 28, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@StephanTLavavej StephanTLavavej

Labels
documentation Related to documentation or comments fixed Something works now, yay!
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Status Chart: Add Combined PR Age and GitHub API script StephanTLavavej/STL
2 participants
@cbezault @StephanTLavavej