Revisit "Contributing Guide"

[jpkrohling]

The Contributing Guide recommends that new components should be split into 3 PRs or more in order to get expedited reviews. However, this isn't improving the delay in reviewing PRs: the only PR that I submitted following this process took 10 days between the first PR getting merged and the last:

• Authentication Helper 1/4 - Add configauth #1807
• Authentication processor 2/4 - Add auth context and interface #1808
• Authentication processor 3/4 - Add implementation #1809
• Authentication processor 4/4 - Add configauth to configgrpc #1810

In addition to not being a better process, it makes contributing really hard, as it requires to either:

1. plan ahead and know with relative certainty how the component is going to look like in the end; or
2. implement the component as a working unit and later on split it into several PRs

The problem with the first is that we rarely know how it will finally look like: development is an iterative process, followed by successive refactoring sessions. The first and last commits might be predictable, but if anything in the implementation is bigger than 500 LoC, it should be split, and the final implementation is rarely predictable.

The problem with the second is that any changes required/suggested during the review does impact every PR that was made based on it. The practical effect is that I ended up trying to avoid changing my PRs to address review comments, opening a tracker issue instead to capture the non-blocking review changes.

In addition to the problems above, splitting a component/feature into several PRs may cause a component to be partially released: v0.11.0 has only the first commit out of the four linked above.

As a reviewer, I'm also concerned that I'll not get the full picture for a component just by looking at small PRs. I'm also not quite sure I'd benefit from reviewing several PRs instead of one: I typically know what to expect in a factory.go, config.go and other common files for a component. It's easy to review them, even when they are part of a bigger PR. But I'm sure other reviewers might disagree here, hence the current recommendation.

My proposal is to still recommend people to follow this structure in terms of commits that compose a PR instead of having multiple individual PRs. Example: https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/1231/commits . This PR has 3000+ LoC, but 16 commits, each one easy to be reviewed individually if the reviewer so wishes. Other reviewers can checkout the code locally and use a local code review tool to analyze it as a whole. A final benefit is that this single PR still results in a fully working unit providing the full picture for the component and without the risk to getting only partially merged.

[jpkrohling]

To be clear: I'm not advocating for big PRs, just that the initial implementation of a component might already not be so small as other types of PRs. I am totally in favor of iterative development processes, implementing only the required first and expanding the initial implementation later.

In the issue's description, I also failed to acknowledge the heroic work that @bogdandrutu is doing here but be sure that I appreciate your work!

[tigrannajaryan]

@jpkrohling thanks for the proposal.

One of the problems that we had in the past is that large PRs require a significant continuous chunk of time from reviewer. A PR with several thousands lines of code requires hours for proper review. Reviewers often do not have so much continuous time available. The result is either poor review quality because reviewers have to rush or reviewing process is split into non-continuous intervals. The later adds a lot of overhead because the reviewer loses context and needs to restart the work.

This is one of the things that we wanted to optimize for when we introduced the PR guidelines. We are open to improving the process but it is important that the process addresses the problem that I mentioned.

[tigrannajaryan]

@jpkrohling some additional arguments in favor of small PRs here: https://github.com/kubernetes/community/blob/master/contributors/guide/pull-requests.md#smaller-is-better-small-commits-small-pull-requests

[jpkrohling]

I'm absolutely in favor of the smallest possible PR for the unit of work (feature, bug fix, refactoring) and I think I'm aligned with the document you linked:

We want every pull request to be useful on its own, so use your best judgment on what should be a pull request vs. a commit.

Note also that what I advocated for in my previous message is explicitly mentioned there under "Breaking up commits":

Break up your pull request into multiple commits, at logical break points.

If we are to follow that document, then we should indeed change our current guidelines ;-)

[jpkrohling]

I wanted to leave here another feature that got split into separate PRs and didn't help in getting speedy reviews:

Original single PR: open-telemetry/opentelemetry-collector-contrib#1231

Broken down into:
open-telemetry/opentelemetry-collector-contrib#1348
open-telemetry/opentelemetry-collector-contrib#1349
open-telemetry/opentelemetry-collector-contrib#1412

At this point, I sincerely doubt breaking PRs is helping at all. In fact, I believe it might be delaying PRs, as the context switch costs is really high. Not to mention the constant merge conflicts, mostly affecting go.mod in particular.

I think I mentioned this before, but as a reviewer, I'd rather review an entire feature at once, in a bigger block, than the same feature split across PRs, one PR a day.

[tigrannajaryan]

@jpkrohling it looks like different reviewers prefer different approaches. What if we keep the current recommendation to split the PRs but for reviewers who prefer to see the entire picture at once the reviewer can ask the contributor to show the entirety of it? So, basically allow the reviewers to choose what is best for them.
We will need to mention this in the contributing guidelines to set the right expectations, so that the contributors are not frustrated because we first ask them to show small PRs and then ask them to show the entire feature.

[jpkrohling]

I'd keep it simple for contributors, so, having multiple PRs by default is better than having it a per-reviewer decision.

[jpkrohling]

I'm closing, as this is apparently working for most people :-)