What I Learned from Reviewing Someone Else's Pull Request in Zalando Logbook

Most open source advice is about sending your own PRs. I want to write about the other side: reviewing someone else's. I learned as much from one review thread as from several of my own PRs.

How it started

I had added status-code based logging to Logbook earlier (PR #2301). Then another contributor found that it did not work with the ECS log format and opened issue #2378, followed by PR #2379 to fix it.

It was a gap in my own earlier work. I did not want to send a competing PR, since the fix was already in progress. So I did the useful thing: I reviewed it.

What I checked

I read the PR with two simple questions: does it do what it says, and does it match the code around it?

I left two comments:

1. Thresholds. Does the new ECS sink use the same rules as the existing writer for choosing TRACE, WARN or ERROR, or is that logic written a second time? If it is a copy, the two can drift apart later when someone changes one of them. A simple fix is one shared place that decides the level, like this (simplified):

// One place decides the log level from the HTTP status code.
// Both sinks call this, so they can never disagree.
static Level levelFor(int statusCode) {
    if (statusCode < 400) return Level.TRACE;
    if (statusCode < 500) return Level.WARN;
    return Level.ERROR;
}

1. Docs. The documentation checkbox in the PR description was not ticked. Without a line in the README, nobody would know the ECS and status-code combination works unless they read the source.

Another reviewer then confirmed both points by reading the source and added that the ECS module had no README coverage at all, not only this combination.

What the rest of the thread taught me

The maintainers' comments taught me more than mine did.

• Public API is a promise. A maintainer pointed out that the PR removed two public classes and turned EcsSink from a class into an interface. Users might already depend on those, so it would be a breaking change. His suggestion was to keep the old classes, mark them deprecated, and add the new ones alongside. The author did exactly that.
• Where code lives matters. The same maintainer asked whether a new abstract class belonged in the API module or in the core module, because a class in the API module becomes something outside code can extend, and that should be a deliberate decision. Another reviewer noticed that a Javadoc reference pointed to a class marked internal, and asked to point to a public one instead.
• Tests are part of the review. One reviewer asked for a test using a user-defined sink, with the feature both on and off. Another found a test expectation that did not match the code. Reading tests carefully catches things that reading code alone misses.

How I review now

1. Understand what the PR is trying to fix before reading a single line.
2. Compare the new code with similar code nearby. Duplicated logic is where bugs come back later.
3. Check the boring parts: tests, docs and the description checklist.
4. Ask questions instead of giving orders. "Does this use the same thresholds as X?" opens a conversation, and "this is wrong" closes one.
5. Comment only if I have something new to add. If someone has already said it, I stay quiet.
6. Be kind. The author is a volunteer who spent time on a real fix, and it was a good one.

Why I think reviewing is worth your time

• You learn how maintainers think about compatibility and design, without risking your own PR.
• You read another person's style, which makes your own code better.
• You build trust. Maintainers notice contributors who help move other PRs forward.

Proof of my work

Everything here is public, so you can check it yourself:

• zalando/logbook PR #2379: the review thread, and issue #2378
• zalando/logbook #2301: the status-code based sink I built earlier (merged)
• zalando/logbook #2326, #2327 and #2328: my other Logbook fixes (merged)
• PicnicSupermarket/error-prone-support #2275: extended RequireNonNullExpression rule (merged)
• Adyen/adyen-java-api-library #1952: regex fix, shipped in SDK v42.0.0 (merged)
• Sionna Visualizer: my 6G + AI dashboard (live demo)
• My GitHub

Connect with me: LinkedIn | X

Next post: what I am working on this week in open source.

If you are a maintainer, a recruiter or a team looking for someone who reviews with care and works well in a team, my DMs are open. Tell me in the comments what you check first when you review a PR.