Code Review
Code review is mandatory at Sentry. This adds overhead to each change, but ensures that simple, often overlooked problems are more easily avoided.
Code review is managed via GitHub’s Pull Requests (see below for rationale). Templates may exist on repositories, if they do not, consider creating one.
When creating a pull request, reference any tickets or Sentry issues which are being addressed. Additionally @mention an appropriate team (or teams) for review.
GitHub Teams
The following teams are defined in GitHub and should be used when creating Pull Requests:
- @getsentry/app-backend
- @getsentry/app-frontend
- @getsentry/infrastructure
- @getsentry/ops
- @getsentry/app – the entire product team, use sparingly
Additionally, language specific teams exist, primarily for SDKs:
Why Pull Requests
Because Sentry is an open source project maintained via GitHub we want to ensure that the barrier to entry for external contributions is minimal. By using GitHub features when possible, we make it easy for developers familiar with other projects on GitHub. While GitHub’s tools aren’t always the best, they’re usable enough that we’ll make do.
Commit Guidelines
We have very precise rules over how our git commit messages can be formatted. This leads to more readable messages that are easy to follow when looking through the project history.
General Rules
- Separate subject from body with a blank line
- Limit the subject line to 70 characters
- Capitalize the subject line
- Do not end the subject line with a period
- Use the imperative mood in the subject line
- Use the body to explain what and why vs. how
- Each commit should be a single, stable change
Merge vs Rebase
Sentry uses a rebase workflow. That means that every commit on its own should be a clear, functional, and stable change. This means then when you’re building a new feature, you should try to pare it down into functional steps, and when that’s not reasonable, the end patch should be a single commit. This is counter to having a Pull Request which may include “fix [unmerged] behavior”. Those commits should get squashed, and the final patch when landed should be rebased.
Remember: each commit should follow the commit message format and be stable (green build).
Rebase and Merge
The GitHub UI exposes a “Rebase and Merge” option, which, if your commits are already in following the commit guidelines, is a great way to bring your change into the codebase.
Squashing
If you are squashing your branch, it’s important to make sure you update the commit message. If you’re using GitHub’s UI it will by default create a new commit message which is a combination of all commits and does not follow the commit guidelines.
If you’re working locally, it often can be useful to --amend a commit, or utilize rebase -i to reorder, squash, and reword your commits.
Commit Message Format
Each commit message consists of a header, a body, and an optional footer.
The header has a special format that includes a type, a scope and a subject:
<type>(<scope>): <subject>
<BLANK LINE>
<body>
<BLANK LINE>
<footer>The header is mandatory and the scope of the header is optional.
Any line of the commit message cannot be longer 100 characters! This allows the message to be easier to read on GitHub as well as in various git tools.
The footer should contain a closing reference to an issue as well as a relevant Sentry issue if any.
For example:
feat(stream): Add resolve in next release action
Expose a new action labeled 'resolve in next release'. It uses the existing API
behavior and sends the 'inNextRelease=true' payload.
Fixes GH-1234As well as:
fix(stream): Handle empty reference on resolve action
Gracefully handle when a user has not selected any issues and tries to complete
the resolve action in the UI.
Fixes GH-1234
Fixes SENTRY-1234Revert
If the commit reverts a previous commit, it should begin with revert:, followed by the header of the reverted commit. In the body it should say: This reverts commit <hash>., where the hash is the SHA of the commit being reverted.
Type
Must be one of the following:
| build: | Changes that affect the build system or external dependencies (example scopes: webpack, python, npm) |
|---|---|
| ci: | Changes to our CI configuration files and scripts (example scopes: travis, zeus) |
| docs: | Documentation only changes |
| feat: | A new feature |
| fix: | A bug fix |
| perf: | A code change that improves performance |
| ref: | A code change that neither fixes a bug nor adds a feature (refactor) |
| style: | Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc) |
| test: | Adding missing tests or correcting existing tests |
| meta: | Some meta information in the repo changes (example scopes: owner files, editor config etc.) |
| license: | Changes to licenses |
Scope
The scope should be the name of the core component affected (as perceived by person reading changelog generated from commit messages). This means it should be the system impacted, not the literal file changed. For example, if the code primarily affects billing, you’d use the billing scope, even if the changes are in utility files or db schema.
The following is the list of suggested scopes:
- api
- billing
- workflow
Subject
The subject contains a succinct description of the change:
- Use the imperative, present tense: “change” not “changed” nor “changes”
- Capitalize the first letter
- No dot (.) at the end
Body
Just as in the subject, use the imperative, present tense: “change” not “changed” nor “changes”. The body should include the motivation for the change and contrast this with previous behavior.
Footer
The footer should contain any information about Breaking Changes and is also the place to reference GitHub issues that this commit Closes.
Breaking Changes should start with the word BREAKING CHANGE: with a space or two newlines. The rest of the commit message is then used for this.
References
- https://chris.beams.io/posts/git-commit/
- https://conventionalcommits.org/
- https://github.com/angular/angular/blob/master/CONTRIBUTING.md
Code Reviews are for …
Identifying problematic code
Above all, a code review should try to identify potential bugs that could cause the application to break – either now, or in the future.
- Uncaught runtime exceptions (e.g. potential for an index to be out of bounds)
- Obvious performance bottlenecks (e.g.
O(n^2)wherenis unbounded) - Code alters behavior elsewhere in an unanticipated way
- API changes are not backwards compatible (e.g. renaming or removing a key)
- Complex ORM interactions that may have unexpected query generation/performance
- Security vulnerabilities
- Tests do not verify the implemented behavior
Assessing and approving long-term impact
If you’re making significant architectural, schema, or build changes that will have long-term ramifications to the software or data, it is necessary to solicit a senior engineer’s acknowledgment and blessing.
- Large refactors
- Database schema changes, like adding or removing columns and tables
- API changes (including JSON schema changes)
- Adopting new frameworks, libraries, or tools
- New product behavior that may permanently alter performance characteristics moving forward
Double-checking expected behavior
The reviewer should make a genuine attempt to double-check that the goals of the PR appear to be satisfied by the code submitted. This requires the submitter to write a good description of the expected behavior, and why. See also: Guidelines for submitters below.
Information sharing and professional development
Code reviews are an opportunity for more people to understand forthcoming code changes, so that they might in turn teach others down the road, and be in a position to fix something if/when the original author is not be available.
Additionally, code reviews are an opportunity to learn about new techniques or approaches, and be exposed to code you might otherwise not have an opportunity to browse.
Reducing code complexity
Research shows that LOC is correlated with a higher bug count. If reviewers see an easy opportunity to significantly reduce the amount of code that is submitted, they should suggest a different approach.
For example, if a submitter has written a for loop to find an item in an array:
for (let i = 0; i < arr.length; i++) {
if (arr[i] === 'thingiwant') return i;
}
return undefined;It’s fair game to suggest they instead use:
return arr.find(x => x === 'thingiwant');This is a mostly objective improvement: there are fewer variables, fewer statements, and fewer branches, and the method name find communicates intent. Suggesting these types of uncontroversial improvements are encouraged.
Be careful though – it’s easy to go down a rabbit hole of re-writing code to be as small as possible, and in the end winding up with something ultimately more complicated. Be pragmatic and strive to reach a good balance. See also: “Code reviews are not for getting it perfect” below.
Enforcing coding standards
As much as possible, we use automation to enforce code style, code coverage, and other industry-accepted good practices. But there are exceptions that cannot necessarily automated (or perhaps more accurately, we haven’t automated yet):
- Variable, file, metric, and logger names are sensible, readable, and consistent with existing code
- Migrations have a deployment plan
- Unused or superfluous code isn’t committed accidentally
Code Reviews are not for …
Passing responsibility onto the reviewer
It is not the responsibility of the reviewer that your code is correct, is bug free, or achieves its goals. Reviewers are there to help you, but if something is wrong, it’s your responsibility to correct it.
Boasting about your programming knowledge
As a reviewer, try to stick to objective improvements and make a best-intent assumption that the submitter has done their homework. Sentry is a No Flex Zone™.
Introducing long-term architectural changes for the first time
While code reviews are great for discussion, they’re not the place to introduce large, long-term changes for the first time. Before dropping a PR that implements those changes, you should write a proposal and reach out to the relevant parties beforehand.
Getting it perfect
Code reviews are expensive. Every time you request a change, you’re probably delaying that PR by 24 hours or more. This can severely inhibit our ability to move fast.
The goal of code reviews is to reduce risk, not to produce perfect code. It’s okay to ship code in stages, and to commit to improving something later. If you’re thinking – if we don’t get it correct up-front, we’ll never come back to it – consider that if it never needs coming back to, perhaps those changes were never necessary in the first place.
Perfect is the enemy of the good. – Voltaire, probably
Please be pragmatic, and consider the cost of each incremental request for changes.
Guidelines for Submitters
Try to organize your work in a way that makes it conducive to review
- Ideally, a pull request is limited to only a single feature or behavior change.
- This might feel like more work up-front, but it can make code review faster, reduce risk by letting you ship in stages, and ultimately end up being quicker.
Describe what your PR does in a few sentences in the description field
- Additionally explain why we’re making these changes.
- If applicable, explain why other approaches were explored but not settled on.
- This gives the reviewer context, and prevents them going down the same rabbit holes that that submitter may have already explored when creating the code.
Annotate specific lines in your PR
- If you can, give context to specific lines of code that could use elaboration.
- Example: getsentry/sentry#6330
Where appropriate, label in-progress PRs as WIP (work in progress) for early feedback
- Labeling your work as WIP helps set expectations about the state of the PR.
- WIP PRs are good for having someone check-in to make sure you’re on the right path.
- Additionally, this is an opportunity to verify CI passes before involving a reviewer.
Be your own first reviewer
- After you’ve put up your PR on GitHub, walk through the code yourself, before assigning an external reviewer.
- You’ll often catch code mistakes you didn’t see when writing it.
- This is also a good time to leave comments and refresh your memory in order to write a more helpful description.
Assign no more than 1-3 reviewers
- It’s tempting to want to involve as many people as possible, but it can often be distracting, and create a situation where nobody’s clear on who should actually perform the review.
- If your work spans multiple teams (and thus, many reviewers), consider breaking up your PR into multiple compatible patches (e.g. a back-end change and a front-end change).
- Note: if you don’t know who from a team to assign, you can assign teams like @workflow and @platform.
Avoid rebasing unnecessarily
- After a rebase, previous review comments will be orphaned from their now non-existent parent commits, making review more difficult
- Rewriting history makes it difficult for reviewers to isolate the scope of their review
Let reviewers know that you’ve made changes
- Request review again via the "Reviewers" dropdown (There should be a yellow dot next to their name again).
- Don’t rely on reviewers mind-reading skills to know that you’re ready to have them look things over again.
- If you resolve a point of actionable feedback, it's helpful to leave a comment to let the reviewer know that it was addressed, ideally with a reference to the commit that addressed it, e.g. getsentry/sentry#6683 (comment)
Guidelines for Reviewers
Be polite and empathetic
- Avoid accusatory and/or judgmental comments like: “You should have done X”
Provide actionable feedback
- Instead of “This is bad”, try “I feel this could be clearer. What if you renamed variable X to Y?”
Distinguish between “requires changes” and “nitpicks”
- Consider marking a PR as approved if the only requested changes are minor nits, so as not to block the author in another asynchronous review cycle.
Respond promptly to code review requests
- We’re a team – we ride together, we die together – and you need to unblock other developers so that we can all move forward.
- We recommend checking for open code reviews at the start and end of every day.
- Github's Review Requests tab can be a helpful place to keep track of these.
Examples of pretty good code reviews
- Submitted early as WIP to solicit feedback early
- Reviewers respond same-day (once the WIP label is removed)
- Reviewers are polite and complimentary
- Feedback is objective and actionable
- Submitter responds to feedback and makes changes promptly
- PR is approved and merged within 48 hours