Scaling Code Reviews

[rakyll]

OpenTelemetry is a large project with contributions from 230 companies and individual contributions. I work for a provider where we care deeply about the components our users care. We also want to provide the best in terms of non-functional features such as security and production-readiness. We review OpenTelemetry on an ongoing basis, and contribute or report critically missing things so our users can rely on OpenTelemetry in production. We are also contributing to several components including components directly talking to our products and want to increase our involvement in project. One of the major blockers today is the code review roundtrip time on the project. Even though we want to contribute more, getting someone giving a review can take days and sometimes weeks for some PRs. OpenTelemetry is a large project and it’s an inherently hard problem to scale the code reviews, but we need to have a strategy to unblock contributions for the success of the project. For example, it’s very critical for us to be able to patch AWS-specific components when there is a security issue or when we are launching a feature heavily relies on OpenTelemetry.

Proposal

I propose to create a new ownership/maintainership model where we identify critical components and provide more dedicated approvers on them. We may also consider having some dedicated members doing code reviews from time to time. Some large open source projects adopt an “oncall” model for reviews and even set SLOs so they make sure incoming changes are reviewed/approved.

We may also utilize some ownership features such as code owners to give more responsibilities to certain maintainers actively contributing to certain components. In contrib repos where there are a lot of vendor-specific components, this model can provide enough flexibility to the vendors. Their maintainership would be revoked if they are consistently not meeting the quality bars. This is also how we can attract more developers to involve in the project to become repo approvers in time.

Alternatives Considered

• Moving the vendor-specific parts outside of the OpenTelemetry organization to vendor orgs. This would work against the consistency between different vendor components and it would make it harder to propagate changes that affect all vendor code. It would also hurt the visibility and maintainability of the vendor-specific components.
• Providing more granular approvers access by splitting components into different repos. This would create additional complexity in the project. GitHub provides features such as code owners to avoid this.

[tigrannajaryan]

Thanks for opening this.

In the Collector contrib repo this has been a problem that we mitigated a bit by a revised process with codeowners and assignments of reviewers to PRs, but it is far from being a solved problem and I think there is still a lot of room for improvement.

Collector contrib repo is one of the highest PR traffic repos (if not the highest) in opentelemetry github org (211 PRs merged last month), so likely it is the repo that experiences the most delays and is the first candidate for improving. I would like to understand how your proposal can help Collector contrib and core repos. Please elaborate and describe the process in more details.

[rakyll]

Thanks for the comments and sorry for the delayed response.

My proposal is a conversation starter rather than being a mature proposal to be honest. I'm suggesting a model where we can give more autonomy to the vendors for their components. Today, we code review some of our components in a separate org/repo and then contribute them to upstream. This is neither ideal for the productivity of our developer nor for this project because it makes the code reviews invisible from the OpenTelemetry upstream. A better situation would be a directly being able to work on the components we maintain, assign reviewers and triage things. The quality and consistency of our work can be reviewed by an collector/collector-contrib approver from time to time.

If we cannot give the autonomy with its compromises, it'd be great to have an "oncall" for reviews so the vendors are not blocked by the approvers and given a review in a predictable time frame.

I'm not expecting us to find the perfect solution to this hard problem today, but it's good to note the current situation is impacting the volume of our contributions and the number of contributors we can have on the project.

[tigrannajaryan]

A better situation would be a directly being able to work on the components we maintain, assign reviewers and triage things. The quality and consistency of our work can be reviewed by an collector/collector-contrib approver from time to time.

I fully support this. Ideally every component in the contrib should have established owners. I encouraged vendors to appoint codeowners for components they contributed in the past.

A better situation would be a directly being able to work on the components we maintain, assign reviewers and triage things.

I think github permissions are not granular enough to give vendors full autonomy on just one component. It is either a full write access to the entire repo (which only Otel maintainers should have), or a full approval right on the entire repo (which only Otel approvers should have). I cannot find a way to give write access or approvals rights to a sub-directory. We have codeowners file but that only helps assign reviews and does not give any other permissions.

If we can find a way to increase the autonomy of vendors over the components they contributes I will be more than happy to do it.

[SergeyKanzhelev]

Same discussed in .NET SIG and we are looking for a model for the contrib repository. CC: @cijothomas . Developing best practices in how to granularly assign permissions and not overburden the overall repository stability would be very useful.

In K8s there is prow that auto-merge PRs with the given number of approvals from specific people. If we will not fit into GitHub limitations, adopting prow may be a good way to help with this.