From ac8c4360fe7b6b79dfd990af5f658c5250a4e4b9 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Thu, 12 Mar 2026 09:39:26 +0000 Subject: [PATCH 1/5] Add AGENTS.md file Created by AI and tweaked by me; prompt was: can you make an AGENTS.md file based on the last 20 PRs that were accepted, telling agents what maintainers like Signed-off-by: Bryan Boreham --- AGENTS.md | 186 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 186 insertions(+) create mode 100644 AGENTS.md diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 0000000000..8f3bc98f07 --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,186 @@ +# Agents Guide for Prometheus + +This document captures patterns and preferences observed from maintainer reviews +of recently merged pull requests. Use it to align your contributions with what +maintainers expect. + +--- + +## PR Title Format + +Titles must follow `area: short description`, using a prefix that identifies the +subsystem. Examples from merged PRs: + +``` +tsdb/wlog: optimize WAL watcher reads +fix(PromQL): do not skip histogram buckets when trimming +feat(agent): fix ST append; add compliance RW sender test +chore: fix emptyStringTest issues from gocritic +ci: add statuses write permission to prombench workflow +docs: clarify that `lookback_delta` query parameter takes either a duration or number of seconds +``` + +Common area prefixes: `tsdb`, `tsdb/wlog`, `promql`, `discovery/`, `agent`, +`alerting`, `textparse`, `ui`, `build`, `ci`, `docs`, `chore`. + +For performance work, append `[PERF]` to the area segment or use the `perf(area):` +convention. + +--- + +## Commits + +- Each commit must compile and pass tests independently. +- Keep commits small and focused. Do not bundle unrelated changes in one commit. +- Sign off every commit with `git commit -s` to satisfy the DCO requirement. +- Do not include unrelated local changes in the PR (reviewers will ask you to + remove them — see PR #18223). + +--- + +## Release Notes Block + +Every PR must include a `release-notes` fenced code block in the description. +If there is no user-facing change, write `NONE`: + +```` +```release-notes +NONE +``` +```` + +Otherwise use one of these prefixes, matching the CHANGELOG style: + +``` +[FEATURE] new capability +[ENHANCEMENT] improvement to existing behaviour +[PERF] performance improvement +[BUGFIX] bug fix +[SECURITY] security fix +[CHANGE] breaking or behavioural change +``` + +Example: +```` +```release-notes +[BUGFIX] PromQL: Do not skip histogram buckets in queries where histogram trimming is used. +``` +```` + +--- + +## Tests + +- Bug fixes require a test that reproduces the bug. +- New behaviour or exported API changes require unit or e2e tests. +- Use only exported APIs in tests where possible — this keeps tests closer to + real library usage and simplifies review. +- Performance improvements require a benchmark that proves the improvement + (maintainers will ask for one if missing — see PR #18252). +- When refactoring test utilities for reuse, move them to a dedicated package + (e.g. `util/testwal`) rather than duplicating them — see PR #18218. +- Test helpers that interact with the filesystem should use atomic writes + (`os.WriteFile` → temp file + `os.Rename`) to avoid race conditions with + file watchers — see PR #18259. + +--- + +## Performance Work + +Maintainers take performance seriously. For any PERF PR: + +- Provide benchmark numbers in the PR body using `go test -bench` output with + a comparison table (before vs after). +- Consider running prombench for significant changes: comment `/prombench main` + to trigger the benchmarking workflow. +- Reuse allocations where possible (slices, buffers). The WAL watcher + optimization (PR #18250) achieved 540x less B/op by reusing `Ref*` buffers. +- When reusing buffers passed to interfaces, document that callers must copy + the contents and must not retain references — reviewers will ask for this + note if it is missing. +- Link to supporting analysis (Google Doc, issue, etc.) for complex changes. + +--- + +## Code Style + +- Follow [Go Code Review Comments](https://go.dev/wiki/CodeReviewComments) + and the formatting/style section of + [Go: Best Practices for Production Environments](https://peter.bourgon.org/go-in-production/#formatting-and-style). +- All exposed objects must have a doc comment. +- All comments must start with a capital letter and end with a full stop. +- Run `make lint` before submitting. The project uses `golangci-lint` including + `gocritic` rules such as `emptyStringTest` — fix linter findings rather than + suppressing them with `//nolint` unless there is a clear false-positive. +- Use `//nolint:linter1[,linter2,...]` sparingly; prefer fixing the code. + +--- + +## Linking Issues + +Use GitHub closing keywords in the PR body so the linked issue closes +automatically on merge: + +``` +Fixes #18243 +``` + +or + +``` +Fixes https://github.com/prometheus/prometheus/issues/18243 +``` + +--- + +## Scope Discipline + +- Do not include unrelated changes in a PR. Reviewers notice and will ask for + them to be removed (PR #18223: "there seem to be a bunch of unrelated local + changes that got checked in"). +- If a PR is large, split it into preparatory and follow-up PRs and reference + them with "Part of #NNNN" or "Depends on #NNNN". + +--- + +## Documentation Changes + +- Docs PRs are welcome for clarifying ambiguous parameter descriptions, + fixing Markdown formatting, and keeping the OpenAPI spec consistent with + the implementation. +- When changing documented behaviour, check whether the OpenAPI spec also + needs updating (reviewers will ask — see PR #18245). + +--- + +## CI / Workflow Changes + +- Workflow files need the correct GitHub token permissions declared explicitly. + Missing permissions (e.g. `statuses: write`) cause silent 403 failures + (PR #18246). +- Use the official `prometheus/promci-artifacts` action for artifact uploads + rather than deprecated alternatives (PR #18277). + +--- + +## What Maintainers Notice + +These observations come directly from reviewer comments on merged PRs: + +- **Allocations in hot paths** — maintainers profile and benchmark carefully; + unnecessary allocations in WAL, PromQL evaluation, or chunk encoding will be caught. +- **Correctness of optimization assumptions** — if you skip work because a + value is "always zero", show why that is true in the PR description + (PR #18252 documents exactly why `otherC` buckets are always zero). +- **Interface contracts** — when a performance change changes ownership or + lifetime semantics (e.g. buffer reuse), document it at the interface + definition, not just in the implementation. +- **Test realism** — tests should mirror production startup/shutdown sequences, + not reach into internal methods (PR #18220). +- **Benchmark regressions** — unusual regression numbers in benchmark output + will be questioned; address them or explain why the case is not realistic + (PR #18247). +- **AI-generated code** — maintainers have noted that AI assistants + (including Claude) can confidently produce incorrect analyses of unfamiliar + algorithms (PR #18252 review). Do not blindly apply AI-suggested changes to + numeric or algorithmic code without understanding the invariants. \ No newline at end of file From 122ea9a7f06833869265cd91be2b530bbdac9f8e Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Thu, 12 Mar 2026 10:00:25 +0000 Subject: [PATCH 2/5] Apply suggestions from code review Co-authored-by: Bartlomiej Plotka Signed-off-by: Bryan Boreham --- AGENTS.md | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 8f3bc98f07..736df5f632 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -91,10 +91,7 @@ Maintainers take performance seriously. For any PERF PR: - Provide benchmark numbers in the PR body using `go test -bench` output with a comparison table (before vs after). -- Consider running prombench for significant changes: comment `/prombench main` - to trigger the benchmarking workflow. -- Reuse allocations where possible (slices, buffers). The WAL watcher - optimization (PR #18250) achieved 540x less B/op by reusing `Ref*` buffers. +- Reuse allocations where possible (slices, buffers). - When reusing buffers passed to interfaces, document that callers must copy the contents and must not retain references — reviewers will ask for this note if it is missing. From 1b8efe937e9a8c9f3afd78c26e74a6653df72d98 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Thu, 12 Mar 2026 11:11:31 +0000 Subject: [PATCH 3/5] Tell Claude to read it too Signed-off-by: Bryan Boreham --- CLAUDE.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 CLAUDE.md diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 0000000000..43c994c2d3 --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1 @@ +@AGENTS.md From 82563e31c2d3b80ea2193b2da235802aa9d38c4d Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Fri, 13 Mar 2026 10:22:07 +0000 Subject: [PATCH 4/5] More review feedback Signed-off-by: Bryan Boreham --- AGENTS.md | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 736df5f632..268eff10e5 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -30,11 +30,10 @@ convention. ## Commits -- Each commit must compile and pass tests independently. +- Each commit must compile and pass tests independently, except when one commit adds a test to expose a bug and then the next commit fixes the bug. - Keep commits small and focused. Do not bundle unrelated changes in one commit. - Sign off every commit with `git commit -s` to satisfy the DCO requirement. -- Do not include unrelated local changes in the PR (reviewers will ask you to - remove them — see PR #18223). +- Do not include unrelated local changes in the PR. --- From 9bc2d73930eb632bdf852c9f06c77617d922e5fd Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Mon, 23 Mar 2026 08:29:27 +0100 Subject: [PATCH 5/5] Address review feedback Remove references to specific PR numbers. Move "What Maintainers Notice" advice into other sections. Thanks to @roidelapluie for suggestions. Signed-off-by: Bryan Boreham --- AGENTS.md | 64 +++++++++++++------------------------------------------ 1 file changed, 15 insertions(+), 49 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 268eff10e5..bdc352b8f0 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -72,15 +72,9 @@ Example: - Bug fixes require a test that reproduces the bug. - New behaviour or exported API changes require unit or e2e tests. +- Tests should attempt to mirror realistic data and/or behaviour. - Use only exported APIs in tests where possible — this keeps tests closer to real library usage and simplifies review. -- Performance improvements require a benchmark that proves the improvement - (maintainers will ask for one if missing — see PR #18252). -- When refactoring test utilities for reuse, move them to a dedicated package - (e.g. `util/testwal`) rather than duplicating them — see PR #18218. -- Test helpers that interact with the filesystem should use atomic writes - (`os.WriteFile` → temp file + `os.Rename`) to avoid race conditions with - file watchers — see PR #18259. --- @@ -88,12 +82,13 @@ Example: Maintainers take performance seriously. For any PERF PR: -- Provide benchmark numbers in the PR body using `go test -bench` output with - a comparison table (before vs after). -- Reuse allocations where possible (slices, buffers). +- Performance improvements require a benchmark that demonstrates the improvement. +- Run benchmarks before and after the change using `go test -count=6 -benchmem -bench ` +- Provide benchmark numbers in the PR body using `benchstat` output. +- If a subset of benchmark results show a regression, address this or explain why the case is not important. +- Reuse allocations in hot paths where possible (slices, buffers). - When reusing buffers passed to interfaces, document that callers must copy - the contents and must not retain references — reviewers will ask for this - note if it is missing. + the contents and must not retain references. - Link to supporting analysis (Google Doc, issue, etc.) for complex changes. --- @@ -103,6 +98,9 @@ Maintainers take performance seriously. For any PERF PR: - Follow [Go Code Review Comments](https://go.dev/wiki/CodeReviewComments) and the formatting/style section of [Go: Best Practices for Production Environments](https://peter.bourgon.org/go-in-production/#formatting-and-style). +- State your assumptions. +- Interface contracts: when ownership or lifetime semantics (e.g. buffer reuse) are important, + document it at the interface definition, not just in the implementation. - All exposed objects must have a doc comment. - All comments must start with a capital letter and end with a full stop. - Run `make lint` before submitting. The project uses `golangci-lint` including @@ -121,19 +119,12 @@ automatically on merge: Fixes #18243 ``` -or - -``` -Fixes https://github.com/prometheus/prometheus/issues/18243 -``` - --- ## Scope Discipline -- Do not include unrelated changes in a PR. Reviewers notice and will ask for - them to be removed (PR #18223: "there seem to be a bunch of unrelated local - changes that got checked in"). +- Do not include unrelated changes in a PR; make a separate PR instead. +- If a refactor is necessary to make a change, do those in separate commits. - If a PR is large, split it into preparatory and follow-up PRs and reference them with "Part of #NNNN" or "Depends on #NNNN". @@ -144,39 +135,14 @@ Fixes https://github.com/prometheus/prometheus/issues/18243 - Docs PRs are welcome for clarifying ambiguous parameter descriptions, fixing Markdown formatting, and keeping the OpenAPI spec consistent with the implementation. -- When changing documented behaviour, check whether the OpenAPI spec also - needs updating (reviewers will ask — see PR #18245). +- When changing documented behaviour, update any relevant text in the docs/ directory. + Check whether the OpenAPI spec also needs updating. --- ## CI / Workflow Changes - Workflow files need the correct GitHub token permissions declared explicitly. - Missing permissions (e.g. `statuses: write`) cause silent 403 failures - (PR #18246). -- Use the official `prometheus/promci-artifacts` action for artifact uploads - rather than deprecated alternatives (PR #18277). + Missing permissions (e.g. `statuses: write`) cause silent 403 failures. --- - -## What Maintainers Notice - -These observations come directly from reviewer comments on merged PRs: - -- **Allocations in hot paths** — maintainers profile and benchmark carefully; - unnecessary allocations in WAL, PromQL evaluation, or chunk encoding will be caught. -- **Correctness of optimization assumptions** — if you skip work because a - value is "always zero", show why that is true in the PR description - (PR #18252 documents exactly why `otherC` buckets are always zero). -- **Interface contracts** — when a performance change changes ownership or - lifetime semantics (e.g. buffer reuse), document it at the interface - definition, not just in the implementation. -- **Test realism** — tests should mirror production startup/shutdown sequences, - not reach into internal methods (PR #18220). -- **Benchmark regressions** — unusual regression numbers in benchmark output - will be questioned; address them or explain why the case is not realistic - (PR #18247). -- **AI-generated code** — maintainers have noted that AI assistants - (including Claude) can confidently produce incorrect analyses of unfamiliar - algorithms (PR #18252 review). Do not blindly apply AI-suggested changes to - numeric or algorithmic code without understanding the invariants. \ No newline at end of file