patterns-vs-guidelines/analysis/pr-discussions-analysis.md

PR Discussions: How New Patterns Are Debated and Landed

The most revealing artifact of codebase evolution is not the code — it's the conversation. Here's what the PR discussions tell us about how patterns actually get introduced.

---

CockroachDB PR #142059: The Handle Pattern

Title: "stop: prefer launching goroutines at caller" Author: Tobias Grieger (tbg) Created: 2025-02-26, Merged: 2025-05-14 (~2.5 months) Size: +314/-84 lines

The Problem Statement (from PR body)

This has the undesirable property of recording a location in Stopper.RunAsyncTaskEx as the async task's lowermost call frame. Anyone who's ever looked at any profile will be familiar with this fact.

The motivation wasn't correctness — RunAsyncTask was correct. It was observability. Every goroutine looked the same in profiling tools.

The Discussion

Reviewer (kvoli) asked: "Noticed there were some span after use related test failures, do you know what's going on there?"

Author (tbg) explained: "Previously, the stopper created the trace span on the caller's goroutine, and would then hand it over to the goroutine it spawned. There was an ominous comment about 'we can't create the span on the goroutine itself because the parent may at that time have finished.' I didn't think our code would so aggressively enforce that ordering."

What this reveals: The new pattern exposed a hidden assumption in the tracing infrastructure. The old API hid the span lifecycle problem by accident. The new API made it visible, requiring explicit handling.

Migration Strategy (explicit in PR body)

The "old way" to start a task via StartAsyncTaskEx remains in place. The hope is that we'll convert all important tasks to GetHandle. This can be done as follow-up work.

Decision: Coexistence, not replacement. Zero migration commitment in the PR. Conversion is "hope" and "follow-up work." No timeline.

Benchmark Evidence (in PR body)

AsyncTaskEx-10   3096800   1169 ns/op   72 B/op   3 allocs/op
Handle-10        3165979   1173 ns/op   48 B/op   1 allocs/op

Same performance (1173 vs 1169 ns/op) but fewer allocations (1 vs 3). The PR isn't sold on performance — it's sold on profiling clarity. Benchmarks are proof of "at least not worse."

---

Prometheus PR #14906: slog Migration

Title: "chore!: adopt log/slog, remove go-kit/log" Author: TJ Hoplock (tjhop) Created: 2024-09-15, Merged: 2024-10-07 (~3 weeks) Size: 162 files, +1534/-1691 lines

Why It Happened (from linked issue #14355)

Go stdlib added log/slog in Go 1.21 (Aug 2023). Prometheus already depended on go-kit/log, a third-party structured logging library. The stdlib version offers:

• No external dependency
• Standard interface other libraries adopt
• Better performance

The Discussion (26 comments)

Key themes from the thread:

1. "Merge and iterate" — SuperQ (reviewer): "Awesome work. I think we should merge this asap and iterate on improvements." Not: "make it perfect first."

2. Formatting debate — roidelapluie: "didn't we go for ts?" (about timestamp format). Decision: since this is a 3.0 breaking change anyway, adopt slog's time key as-is.

3. Source file paths — roidelapluie: "I approve this but I would appreciate if we do not have the full path to source file." SuperQ: "We haven't been able to figure out a way to get the relative path." Unresolved — merged anyway.

4. Upstream dependencies — Required changes in prometheus/common that weren't released yet. Solution: pin to commit in go.mod, replace after release.

5. Self-review — tjhop: "Did a thorough self-review and see a few other spots to tidy up." The author caught issues after posting.

6. Merge conflicts — tjhop: "Please excuse the frequent force pushes -- this PR touches a ton of stuff and I'm trying to stay on top of all the merge conflicts." Large migrations have coordination costs.

What This Reveals About Pattern Introduction

• The breaking change was held for a major version (3.0). They didn't try to do it incrementally.
• 162 files in one commit — total replacement, not gradual migration. This is the opposite of CockroachDB's approach.
• Multiple reviewers said "let's merge and iterate" rather than blocking on polish. Ship the migration, fix details in follow-ups.
• Upstream coordination required (prometheus/common had to release first).

---

Prometheus Issue #17632: AppenderV2 Design

Title: "storage: Switch to unified, transactional AppenderV2 API; remove Appender." Author: bwplotka Status: In progress (M1 complete, iterating)

Why V2?

The original Appender interface grew organically:

• Appender.Append() — float samples
• ExemplarAppender.AppendExemplar() — per-series
• HistogramAppender.AppendHistogram() — native histograms
• StartTimestampAppender — for OTel start timestamps

Each requires a type assertion. The scrape loop has multiple code paths depending on which interfaces the storage supports.

The Debate

roidelapluie (maintainer) pushed back:

"I would like to see scrape and prometheus continuing to support v1. The changes in some areas like scrape seem really high. We could move everything to v2 in Prometheus v4."

bwplotka (author) challenged:

"I'd like to challenge this. It's not a breaking change. It's not even a user facing change. So why waiting for v4?"

Resolution: Author won. V2 is being adopted now (target: remove V1 by Q2 2026) rather than waiting for a hypothetical v4.

External validation:

"We got amazing feedback from OTel collector side — AppenderV2 is helpful."

The OTel collector team confirmed the unified interface solves real problems for consumers.

Implementation Strategy

The issue tracks a detailed milestone plan:

• M1: Add interface + TSDB impl (done in parts 1-5)
• Adapter layer for coexistence
• Switch each consumer one at a time
• Understand impact on downstream (Thanos, Mimir, Cortex)

This is a 12-PR migration — each PR switches one consumer. The interface was introduced first, then consumers migrated one by one.

---

Oban PR #684: Inline Testing Mode

Title: "Inline testing mode" Author: Parker Selbert (sorentwo) Created: 2022-04-11, Merged: 2022-04-12 (< 24 hours) Size: (not available — private discussion) Comments/Reviews: 0

What This Reveals

Parker Selbert is the solo maintainer. The PR was merged in less than 24 hours with zero discussion. This isn't poor process — it's the reality of solo-maintained open source:

• The design decision happened in his head (or in the linked issue discussion)
• No external review needed because he owns the whole system
• The commit message is the entire design document

The Feature

Inline testing immediately executes jobs as they are inserted within the current process and without ever touching the database. It's an alternative to the standard insert/verify/execute flow for testing.

This single feature eliminated the #1 source of test flakiness for all Oban consumers. No polling, no sleep, no async assertions.

---

Cross-Cutting Lessons from PR Discussions

1. "Merge and iterate" > "Perfect before merge"

Both the slog migration and the Handle pattern were merged with known imperfections. The slog PR merged with full file paths in logs (unresolved). The Handle PR merged with trace span issues discovered during review.

Lesson for review: Don't block a pattern introduction on polish. Block on correctness. Accept imperfect-but- correct as mergeable.

2. Breaking changes need a major version OR gradual migration

• Prometheus: slog migration in one commit, held for 3.0
• CockroachDB: Handle API coexists with RunAsyncTask, no major version needed
• Prometheus AppenderV2: NOT a user-facing change, so no need to wait for v4

Lesson: The decision to batch vs. gradual depends on whether it's user-facing. Internal refactors can ship incrementally. User-facing API changes need version boundaries.

3. External consumers validate pattern decisions

AppenderV2 was validated by the OTel collector team. The Handle pattern was validated by profiling data. The slog migration was validated by "the stdlib does it."

Lesson: Pattern introductions that cite external evidence (benchmarks, user feedback, ecosystem alignment) get approved faster.

4. Large migrations require coordination

The slog PR needed prometheus/common changes first. AppenderV2 is a 12-PR sequence. Large changes need dependency ordering.

Lesson for review: When reviewing the first PR in a migration, ask about the full plan. Is this standalone? What comes next? What depends on this?

5. Solo maintainers decide faster

Oban's inline testing: conceived, implemented, merged in <24 hours. CockroachDB's Handle pattern: 2.5 months. Prometheus slog: 3 weeks. Prometheus AppenderV2: multi- month ongoing.

Lesson: Team size correlates with decision latency, not decision quality. Parker's inline testing mode was the right call, arrived fast, and has been stable for 3+ years.