Page MenuHomePhabricator

Code review time must be on merged patches, not closed (e.g. abandoned or merged) ones
Closed, DeclinedPublic

Description

Screenshot of the graph in question

I just noticed that under "Are Wikimedia's staff and non-staff contributions processed equally?" the answer is "Review time for open submissions (median)". That's not an answer to the question, because you measure neglected things which are *not* processed.

Moreover, the neglected contributions by staff are often part of their volunteer/pet projects coding rather than of their goals (can you imagine a patch for a bug with a deadline rotting for 150 days?); so the hats of their authors means little.

The solution is simple: reverse your query, calculate the median time that it took for *merged* patches to be merged.


Version: unspecified
Severity: normal

Attached:

KormaCR.png (768×1 px, 65 KB)

Details

Reference
bz66265

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 22 2014, 3:21 AM
bzimport set Reference to bz66265.
bzimport added a subscriber: Unknown Object (MLST).

(In reply to Nemo from comment #0)

The solution is simple: reverse your query, calculate the median time that
it took for *merged* patches to be merged.

This is what Metrics Grimoire offered at the beginning, and I guess providing that data is fairly simple. Álvaro, what do you say?

However, we want to put our attention first on the work that still needs to be done. You can be pretty fast closing the bugs that matter to you (say median 1,7 days) while absolutely ignoring all the rest. Does this make you a good maintainer? I don't think so. Good maintainers are the ones capable of keeping the house clean.

In today's ECT querterly review, Erik pointed out that the current stats include WIP patches. I knew this, but I underestimated the amount of such patches. I will file a bug to discard them.

(In reply to Quim Gil from comment #1)

However, we want to put our attention first on the work that still needs to
be done. You can be pretty fast closing the bugs that matter to you (say
median 1,7 days) while absolutely ignoring all the rest. Does this make you
a good maintainer? I don't think so. Good maintainers are the ones capable
of keeping the house clean.

I'm only talking of the graph by affiliation, the graphs by extension I don't know; for the general situation we have three graphs above on open patches.

We could have a fourth: what I often look at (because it's easy and IMHO reasonably meaningful) is the *number* of open patches by affiliation. Typically I see something like 70 % merged patches by WMF and 70 % open patches by volunteers.

Qgil lowered the priority of this task from Medium to Low.Jan 8 2015, 11:33 AM
Qgil raised the priority of this task from Low to Medium.Jun 25 2015, 8:49 AM

I think our current approach of measuring median age of open changesets is good, because it shows the dimensions of the problem we have.

A team might have a great performance crunching new patches as they come (therefore having a great statistic resolving code review), but look at these scenarios:

  • A team processes quickly new patches while ignoring a huge bag of old changesets.
  • A team processes quickly half of the open patches they receive, but ignores the other half.

If the same team is measured by the media age of their open changesets, that age will grow as long as they keep ignoring a percentage of the patches they have received. On the other hand. teams keeping their media age of open changesets low are very likely to be reviewing new changesets fast.

Based on this, I propose to decline this task.

You almost convinced me. ;-) But this report is about the graph "Are Wikimedia's staff and non-staff contributions processed equally?", rather than the others. The scenario "A team processes quickly half of the open patches they receive, but ignores the other half" can still produce sensible answers to that question in realistic situations.

Both your worst-case scenarios are IMHO extreme and about as realistic as this:

  • A team processes quickly half of the patches and speedy-abandons the other half.
Aklapper renamed this task from Code review time must be on merged patches, not closed ones to Code review time must be on merged patches, not closed (e.g. abandoned or merged) ones.Nov 7 2015, 11:18 PM
Aklapper set Security to None.
Aklapper removed a subscriber: wikibugs-l-list.
Aklapper claimed this task.

I think our current approach of measuring median age of open changesets is good, because it shows the dimensions of the problem we have.

...that's nowadays on http://korma.wmflabs.org/browser/scr.html (as there's no link in the task desc).

That page does not explicitly ask anymore "Are Wikimedia's staff and non-staff contributions processed equally?" but "Age of open changesets by affilation (monthly snapshot)".

I'm declining this request as I think that the initial question could not be directly applied to that graph.
(Plus my usual comment that the graph is based on data and algorithms that welcome(d) some fixes, see T112527, T95238, https://github.com/VizGrimoire/GrimoireLib/issues/62 .)