From 3010466f38a426613f7deea67d9238771c3f80e2 Mon Sep 17 00:00:00 2001 From: Douglas Q Hawkins Date: Wed, 14 Oct 2026 13:33:27 -0400 Subject: [PATCH 1/5] Add strict dd-trace-core performance rules for reviews AGENTS.md now requires JMH before/after numbers for performance claims and forbids defensive copies from internal accessors in dd-trace-core. The perf-review skill applies these as findings for dd-trace-core and keeps its advisory posture elsewhere. Co-Authored-By: Claude Opus 5.5 --- .agents/skills/perf-review/SKILL.md | 6 ++++++ AGENTS.md | 7 +++++++ 2 files changed, 13 insertions(+) diff --git a/.agents/skills/perf-review/SKILL.md b/.agents/skills/perf-review/SKILL.md index 8d120f064fd..6de0d530c99 100644 --- a/.agents/skills/perf-review/SKILL.md +++ b/.agents/skills/perf-review/SKILL.md @@ -116,6 +116,12 @@ mechanism is certain or the severity is catastrophic. **routine adoption of a known-better idiom** (migrating to a lower-overhead builder / API / toolkit primitive — no visible downside); or a change that **ships a benchmark/JFR** (well-evidenced — recognize it). One line; a nudge, not a code-pattern finding. +- **`dd-trace-core` is stricter — see the `dd-trace-core` performance rules in `AGENTS.md`.** + For changes under `dd-trace-core/`: a performance claim (in the title, description, or + commits) without JMH before/after numbers is a finding, not a soft nudge. A new defensive + copy returned from an internal accessor is a flag-with-confidence #1 finding; the fix is a + restrictive type, a shared empty constant, and a read-only Javadoc contract. Outside + `dd-trace-core`, keep the softer posture above. ## Workflow diff --git a/AGENTS.md b/AGENTS.md index a696c26b3da..4284814aa8f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -82,6 +82,13 @@ docs/ Developer documentation (see below) - **Technical debt**: run `/techdebt` over branch changes before marking a PR ready to catch code duplication, unnecessary complexity, and dead code (refactor-only, never changes behavior) — see [.agents/skills/techdebt/SKILL.md](.agents/skills/techdebt/SKILL.md). - **Performance**: run `/perf-review` over branch changes before marking a PR ready (advisory, not a merge gate) — see [.agents/skills/perf-review/SKILL.md](.agents/skills/perf-review/SKILL.md). +### `dd-trace-core` performance rules + +These rules are strict for changes under `dd-trace-core/`; elsewhere, treat them as advice. + +- **Measure performance claims.** A PR that claims to be faster or to allocate less must include JMH before/after results from the same machine, with time and allocation (`-Pjmh.profilers=gc`). Without them, reviewers should flag the claim as unmeasured and the author should either add numbers or drop the claim. +- **No defensive copies from internal accessors.** Don't return defensive copies from accessors only the tracer calls, since they allocate on every call on hot paths such as serialization. Use a restrictive type (e.g. `List`), a shared immutable constant for the empty case, and a Javadoc read-only contract instead. + ## Critical constraints - **Bootstrap**: never use `java.util.logging.*`, `java.nio.file.*`, or `javax.management.*` in `premain` code — see [docs/bootstrap_design_guidelines.md](docs/bootstrap_design_guidelines.md). From 1e1ead77578dea2b65efad1264e165ab0e815386 Mon Sep 17 00:00:00 2001 From: Douglas Q Hawkins Date: Thu, 15 Oct 2026 08:51:59 -0400 Subject: [PATCH 2/5] Require the same JDK and benchmark settings for before/after numbers The same machine alone doesn't make two JMH runs comparable: a different JDK, JVM flags or -Pjmh settings would be attributed to the change. Co-Authored-By: Claude Opus 5.5 --- AGENTS.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/AGENTS.md b/AGENTS.md index 4284814aa8f..be7f752b4e8 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -86,7 +86,7 @@ docs/ Developer documentation (see below) These rules are strict for changes under `dd-trace-core/`; elsewhere, treat them as advice. -- **Measure performance claims.** A PR that claims to be faster or to allocate less must include JMH before/after results from the same machine, with time and allocation (`-Pjmh.profilers=gc`). Without them, reviewers should flag the claim as unmeasured and the author should either add numbers or drop the claim. +- **Measure performance claims.** A PR that claims to be faster or to allocate less must include JMH before/after results from the same machine, JDK, JVM flags and benchmark settings, with time and allocation (`-Pjmh.profilers=gc`). Without them, reviewers should flag the claim as unmeasured and the author should either add numbers or drop the claim. - **No defensive copies from internal accessors.** Don't return defensive copies from accessors only the tracer calls, since they allocate on every call on hot paths such as serialization. Use a restrictive type (e.g. `List`), a shared immutable constant for the empty case, and a Javadoc read-only contract instead. ## Critical constraints From 0308432c4fb314b9407046248aa66d629883fdc0 Mon Sep 17 00:00:00 2001 From: Douglas Q Hawkins Date: Thu, 15 Oct 2026 08:52:34 -0400 Subject: [PATCH 3/5] Scope the strict performance rules to shipped core code The rules exist for code that runs in a customer's tracer, so they now cover dd-trace-core/src/main and internal-api/src/main, and stay advice for those modules' tests, benchmarks and build files. Co-Authored-By: Claude Opus 5.5 --- .agents/skills/perf-review/SKILL.md | 13 +++++++------ AGENTS.md | 4 ++-- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/.agents/skills/perf-review/SKILL.md b/.agents/skills/perf-review/SKILL.md index 6de0d530c99..9415913c337 100644 --- a/.agents/skills/perf-review/SKILL.md +++ b/.agents/skills/perf-review/SKILL.md @@ -116,12 +116,13 @@ mechanism is certain or the severity is catastrophic. **routine adoption of a known-better idiom** (migrating to a lower-overhead builder / API / toolkit primitive — no visible downside); or a change that **ships a benchmark/JFR** (well-evidenced — recognize it). One line; a nudge, not a code-pattern finding. -- **`dd-trace-core` is stricter — see the `dd-trace-core` performance rules in `AGENTS.md`.** - For changes under `dd-trace-core/`: a performance claim (in the title, description, or - commits) without JMH before/after numbers is a finding, not a soft nudge. A new defensive - copy returned from an internal accessor is a flag-with-confidence #1 finding; the fix is a - restrictive type, a shared empty constant, and a read-only Javadoc contract. Outside - `dd-trace-core`, keep the softer posture above. +- **Shipped core code is stricter — see the core performance rules in `AGENTS.md`.** + For changes to `dd-trace-core/src/main/` or `internal-api/src/main/`: a performance claim + (in the title, description, or commits) without JMH before/after numbers is a finding, not + a soft nudge. A new defensive copy returned from an internal accessor is a + flag-with-confidence #1 finding; the fix is a restrictive type, a shared empty constant, + and a read-only Javadoc contract. Elsewhere, including those modules' tests, benchmarks + and build files, keep the softer posture above. ## Workflow diff --git a/AGENTS.md b/AGENTS.md index be7f752b4e8..39b4b105190 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -82,9 +82,9 @@ docs/ Developer documentation (see below) - **Technical debt**: run `/techdebt` over branch changes before marking a PR ready to catch code duplication, unnecessary complexity, and dead code (refactor-only, never changes behavior) — see [.agents/skills/techdebt/SKILL.md](.agents/skills/techdebt/SKILL.md). - **Performance**: run `/perf-review` over branch changes before marking a PR ready (advisory, not a merge gate) — see [.agents/skills/perf-review/SKILL.md](.agents/skills/perf-review/SKILL.md). -### `dd-trace-core` performance rules +### Core performance rules -These rules are strict for changes under `dd-trace-core/`; elsewhere, treat them as advice. +These rules are strict for shipped runtime code in `dd-trace-core/src/main/` and `internal-api/src/main/`. Elsewhere, including those modules' tests, benchmarks and build files, treat them as advice. - **Measure performance claims.** A PR that claims to be faster or to allocate less must include JMH before/after results from the same machine, JDK, JVM flags and benchmark settings, with time and allocation (`-Pjmh.profilers=gc`). Without them, reviewers should flag the claim as unmeasured and the author should either add numbers or drop the claim. - **No defensive copies from internal accessors.** Don't return defensive copies from accessors only the tracer calls, since they allocate on every call on hot paths such as serialization. Use a restrictive type (e.g. `List`), a shared immutable constant for the empty case, and a Javadoc read-only contract instead. From c8b460b77c9d95f267ca825bec717c65c4702728 Mon Sep 17 00:00:00 2001 From: Douglas Q Hawkins Date: Thu, 15 Oct 2026 08:58:47 -0400 Subject: [PATCH 4/5] Have perf-review read the PR title, description and commits The claim check needs the PR text, which the diff alone doesn't show. Step 1 now reads the branch's PR and commit messages, and says when the PR text wasn't available. Co-Authored-By: Claude Opus 5.5 --- .agents/skills/perf-review/SKILL.md | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/.agents/skills/perf-review/SKILL.md b/.agents/skills/perf-review/SKILL.md index 9415913c337..346a591fbde 100644 --- a/.agents/skills/perf-review/SKILL.md +++ b/.agents/skills/perf-review/SKILL.md @@ -144,11 +144,22 @@ git diff $MERGE_BASE --stat git diff $MERGE_BASE --name-status ``` +Also read the claims the change makes. A performance claim can live only in the PR title +or description, which the diff doesn't show: + +```bash +gh pr view --json title,body 2>/dev/null # the branch's PR, if it has one +git log --format='%s%n%b' $MERGE_BASE..HEAD # commit messages +``` + +If the branch has no PR, or `gh` can't reach it, check the commit messages and say in the +summary that the PR text wasn't available, so a claim made only there wasn't checked. + If there are no changes, say so and stop. Otherwise read the diff **and the full content of the modified source files** (not just the hunks) — the interprocedural condition (who calls this, what a helper does, where a hook's sink lands) lives outside the diff window. Ignore the PR description if the user asks for an -independent review. +independent review, except to find performance claims. ### Step 2 — Map the changed code onto hot paths From d63b7570dd6309d5a6e4598076fbbac0d752496b Mon Sep 17 00:00:00 2001 From: Douglas Q Hawkins Date: Thu, 15 Oct 2026 09:00:18 -0400 Subject: [PATCH 5/5] Drop the defensive-copies rule for now Whether an internal accessor may skip a defensive copy depends on a trust model for code inside the tracer that isn't defined yet. Leave the rule out until it is. Co-Authored-By: Claude Opus 5.5 --- .agents/skills/perf-review/SKILL.md | 6 ++---- AGENTS.md | 1 - 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/.agents/skills/perf-review/SKILL.md b/.agents/skills/perf-review/SKILL.md index 346a591fbde..9ff896bc320 100644 --- a/.agents/skills/perf-review/SKILL.md +++ b/.agents/skills/perf-review/SKILL.md @@ -119,10 +119,8 @@ mechanism is certain or the severity is catastrophic. - **Shipped core code is stricter — see the core performance rules in `AGENTS.md`.** For changes to `dd-trace-core/src/main/` or `internal-api/src/main/`: a performance claim (in the title, description, or commits) without JMH before/after numbers is a finding, not - a soft nudge. A new defensive copy returned from an internal accessor is a - flag-with-confidence #1 finding; the fix is a restrictive type, a shared empty constant, - and a read-only Javadoc contract. Elsewhere, including those modules' tests, benchmarks - and build files, keep the softer posture above. + a soft nudge. Elsewhere, including those modules' tests, benchmarks and build files, keep + the softer posture above. ## Workflow diff --git a/AGENTS.md b/AGENTS.md index 39b4b105190..06a5e7fabb7 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -87,7 +87,6 @@ docs/ Developer documentation (see below) These rules are strict for shipped runtime code in `dd-trace-core/src/main/` and `internal-api/src/main/`. Elsewhere, including those modules' tests, benchmarks and build files, treat them as advice. - **Measure performance claims.** A PR that claims to be faster or to allocate less must include JMH before/after results from the same machine, JDK, JVM flags and benchmark settings, with time and allocation (`-Pjmh.profilers=gc`). Without them, reviewers should flag the claim as unmeasured and the author should either add numbers or drop the claim. -- **No defensive copies from internal accessors.** Don't return defensive copies from accessors only the tracer calls, since they allocate on every call on hot paths such as serialization. Use a restrictive type (e.g. `List`), a shared immutable constant for the empty case, and a Javadoc read-only contract instead. ## Critical constraints