Replace perf-review skill with multi-perspective dd-apm-sdk-review - #12349
Replace perf-review skill with multi-perspective dd-apm-sdk-review#12349robertomonteromiguel wants to merge 3 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 11aa8f1af7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| isn't relied on here, so a plain HashMap is the lighter fix. | ||
| - Also flags, independently or as part of the same finding, that the map is | ||
| unbounded and keyed by a high-cardinality value (sessionId) — a SEV-1 concern | ||
| under universal check #3, mapped to P0 or P1 per performance.md's SEV-to-P |
There was a problem hiding this comment.
When the .llm-validation suite is loaded by a standard YAML parser, the #3 begins a comment because this is an unquoted plain scalar; the indented mapping section. line is then parsed as an invalid continuation, so the entire newly added validation suite fails to load. Quote this criterion or replace #3 with wording that does not start a YAML comment.
Useful? React with 👍 / 👎.
| 2. **Sequential isolated subagents** — no parallelism available, but isolated contexts are. Run them in order. | ||
| 3. **Single-context sequential passes** — neither available. Run one pass per perspective yourself, and label the final report `DEGRADED MODE: single context, findings may bleed between perspectives`. | ||
|
|
||
| **Restrict each reviewer's own tools when your harness lets you set them per subagent.** A reviewer's job is to read the change set and the rule files and report — nothing in any lens requires writing, editing, or running a command. `_common.md`'s "read-only" rule is a prompt-level instruction; it does not stop a subagent from calling a tool it technically has, especially one that just ingested untrusted diff/pasted content that may contain adversarial instructions. When dispatching each reviewer (mode 1 or 2 above), scope its tools to read-only ones — `Read`, `Grep`, `Glob` — and exclude `Bash`, `Write`, `Edit`, and any other mutating or networked tool, even though the orchestrator itself needs `Bash` for Step 1. If your harness has no per-subagent tool scoping, note that as a capability gap in the report rather than silently running reviewers unrestricted. |
There was a problem hiding this comment.
In native subagent modes, this restriction makes two advertised perspectives unable to perform their required work: the conventions prompt explicitly requires running check-only commands such as spotlessCheck, while the cross-SDK prompt requires gh or another network-backed source for relevant changes. Because the orchestrator supplies only the diff and rule files, those reviewers must return NOT VERIFIED, which never blocks, so the new push gate can approve changes without its required formatting or cross-SDK validation. Permit narrowly scoped, non-mutating command and network access for the perspectives that require it, or have the orchestrator collect and pass those results.
AGENTS.md reference: AGENTS.md:L53-L54
Useful? React with 👍 / 👎.
| TARGET=$(gh pr view --json baseRefName -q '"origin/" + .baseRefName' 2>/dev/null) | ||
| TARGET=${TARGET:-origin/master} # no PR yet: confirm this is really the parent |
There was a problem hiding this comment.
When gh pr view fails or a stacked branch has no PR yet, this immediately substitutes origin/master even though the preceding instructions say never to assume trunk. The skill then performs and reports the review before asking whether the target looks wrong, so a stacked branch is reviewed together with all parent-branch commits and can be blocked by findings unrelated to the changes being pushed. Pause for confirmation when the base cannot be resolved instead of proceeding with the fallback.
AGENTS.md reference: AGENTS.md:L77-L77
Useful? React with 👍 / 👎.
| - **J12 — Wrong Collection Type** *(refines universal `per-call-allocation`)*. A three-step weight ladder, lighter wins: `LinkedHashMap → HashMap → POJO/record`. `LinkedHashMap` when order isn't relied on: ~16B extra per entry plus doubly-linked-list maintenance on every put/remove — only justified for a required iteration order or LRU (`accessOrder` + `removeEldestEntry`); fix: `HashMap`. `HashMap` for a fixed, small, known key set: pays hashing, boxing, and `Entry` overhead per lookup; fix: a plain record/value class — denser, EA-scalar-replaceable when non-escaping. Mis-sized collections: `ArrayList` grows 1.5×, `HashMap` doubles and rehashes, both pay allocation+copy on growth — pre-size at construction (`new HashMap<>((int)(n/0.75f)+1)` for Java 8+; `HashMap.newHashMap(n)` where the source set is known and JDK 19+ is the floor). Concurrency choice is a question, not a directive: replacing `ConcurrentHashMap` with `HashMap` on a wrong concurrency judgment trades a performance overhead for a correctness bug — frame as "if this map is thread-confined, a plain collection is cheaper — verify the access pattern," never assert it. flag-with-confidence — SEV-2/3. | ||
| - **J13 — Defensive copies at internal boundaries** *(refines universal `per-call-allocation`)*. `array.clone()`, `new ArrayList<>(other)`, and similar copies are justified at a real trust boundary (a public API, or genuinely mutable external input) but wasteful internal-to-internal, where the caller and callee are both under our control. flag-with-confidence when the copy crosses no trust boundary — SEV-2/3. Fix: return a read-only view, or establish a "don't mutate" contract instead of copying. | ||
| - **J14 — Capturing lambda allocated on every call, including cache hits** *(refines universal `per-call-allocation`)*. A non-capturing lambda is a cached singleton (zero-alloc); a capturing lambda (closes over a local or `this`) is a new instance per evaluation. The recurring trap: `map.computeIfAbsent(k, k -> compute())` allocates the lambda on *every* call, including cache hits where it's never invoked. flag-with-confidence when a capturing lambda sits in a `computeIfAbsent`/similar call on a hot path — SEV-2/3. Fix: `get` first, call `computeIfAbsent` only on a miss. | ||
| - **J15 — `Optional` construction and primitive boxing outside the JVM cache range** *(refines universal `per-call-allocation`)*. Any `Optional*` construction allocates per call and escapes. Autoboxing a primitive outside the JVM's cached range (`[-128, 127]` for `Integer`/`Long`) likewise allocates on every call. flag-with-confidence on a hot path — SEV-2/3. Fix: null checks or primitive-typed return values instead of `Optional`; fixed-arity primitive overloads instead of boxing. |
There was a problem hiding this comment.
When a reviewed hot path calls Optional.empty(), OptionalInt.empty(), OptionalLong.empty(), or OptionalDouble.empty(), the JDK returns a cached singleton rather than allocating per call, but this unconditional rule tells the reviewer to flag every such construction with confidence. That creates deterministic false findings in a gate whose own performance rubric prioritizes precision; restrict the rule to Optional-producing expressions for which a new object and escape are actually established.
Useful? React with 👍 / 👎.
Fixes 4 issues flagged in PR review: - Quote/reword the "#3" criterion in the llm-validation suite so it doesn't start a YAML comment and break the parser. - Sync the upstream core SKILL.md fix that grants the conventions and cross-sdk reviewers narrowly scoped read-only access instead of silently degrading to NOT VERIFIED under the read-only restriction. - Sync the upstream core SKILL.md fix that stops the change-set target resolution from silently falling back to origin/master and instead asks for confirmation. - Exempt cached Optional.empty()/OptionalInt.empty()/OptionalLong.empty()/ OptionalDouble.empty() singletons from the J15 allocation rule in the performance override, avoiding guaranteed false positives. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 00e16ba8be
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| The core rule is a **predicate-with-default, not a banned-API list**: don't flag "you called `String.format`"; flag *"an eager, unconditional expensive call on an instrumentation-reachable path."* Discriminator: result usually **discarded** → gate/defer; result always **needed but costly** → cheapen/cache. | ||
|
|
||
| - **`Config.get()` / `InstrumenterConfig.get()` on a hot path — flag-with-confidence.** Walks a config-resolution chain; not a free read. Fix: hoist to a `static final` field, or compute once in the constructor. The single most recurring finding in calibration — five independent occurrences. |
There was a problem hiding this comment.
When a reviewed hot path calls Config.get() or InstrumenterConfig.get(), this rule requires a confidence-level finding based on a configuration-resolution chain that does not exist: the current implementations at Config.java:6440-6442 and InstrumenterConfig.java:759-761 only return their static INSTANCE. Resolution occurs during initialization, not on every call, so the mandatory push gate will repeatedly report a nonexistent per-call cost and recommend an equivalent cached field; restrict this rule to actual ConfigProvider lookups or remove it.
AGENTS.md reference: AGENTS.md:L77-L77
Useful? React with 👍 / 👎.
|
|
||
| **J7–J11 route an *existing* universal `per-call-allocation`/`repeat-work-across-calls`/`unbounded-memory` finding to a landed reusable fix — they are not new triggers.** Don't raise a finding you wouldn't have raised anyway. | ||
|
|
||
| **Toolkit availability — cite only what exists.** Available today: `Strings`, `SubSequence`, `HashingUtils`, `Hashtable` (all `datadog.trace.util`), `RE2J` (`com.google.re2j`). Coming (name as "coming", don't imply present): `ConcurrentHashtable`, `StringIndex`, `UTF8BytesString.Cache`, wider `IntegerCache`, `DDCache` inlining. |
There was a problem hiding this comment.
When a reviewer needs an allocation-free string index, this instruction requires it to describe StringIndex as merely “coming,” even though internal-api/src/main/java/datadog/trace/util/StringIndex.java already exists in the parent revision and exposes of, indexOf, and EmbeddingSupport. This stale toolkit inventory makes the performance perspective withhold or mischaracterize a valid in-repo remediation; move StringIndex to the available list.
AGENTS.md reference: AGENTS.md:L77-L77
Useful? React with 👍 / 👎.
| | reviewer | generic prompt (core, this folder) | this repo's override (if any) | | ||
| |---|---|---| | ||
| | Coherence | [reviewers/coherence.md](./reviewers/coherence.md) | — (fully language-agnostic) | | ||
| | Security | [reviewers/security.md](./reviewers/security.md) | `dd-apm-sdk-review-overrides/reviewers/security.md` | |
There was a problem hiding this comment.
When the orchestrator builds the reviewer roster, this table says the security lens has a repo override, and report-template.md:42 likewise shows that nonexistent file as having been used. A repo-wide search under dd-apm-sdk-review-overrides/ finds no reviewers/security.md, so an implementation following the table can attempt to dispatch an unreadable rule file or falsely report security-specific coverage; mark this cell as having no override and update the template example accordingly.
AGENTS.md reference: AGENTS.md:L77-L77
Useful? React with 👍 / 👎.
|
|
||
| ## Public API surface | ||
|
|
||
| This repo's public API lives in `dd-trace-api/` (`Tracer`, `GlobalTracer`, `DDTags`, `DDSpanTypes`, the `@Trace` annotation, the `*Config` constant classes) and in `dd-trace-ot/`'s `io.opentracing.Tracer` implementation — see ARCHITECTURE.md § "dd-trace-api/" and § "dd-trace-ot/". A change adding a class or method to either is public surface and needs explicit justification; it is forever. `internal-api/` is internal despite the name — it's fair game to reshape, but check callers across `products/` and `dd-java-agent/` before calling a change there "just internal." |
There was a problem hiding this comment.
When a change adds an internal class or helper method anywhere in dd-trace-api/ or dd-trace-ot/, this rule classifies it as permanent public API regardless of Java visibility. That is already contradicted by package-private production types such as OTSpan, OTSpanContext, and TypeConverter in dd-trace-ot; adding a package-private or private member to those types does not expand what consumers can access. Restrict this check to exported public/protected symbols on externally accessible types so the mandatory design gate does not demand API justification for implementation details.
AGENTS.md reference: AGENTS.md:L77-L77
Useful? React with 👍 / 👎.
1 participant
What Does This Do
Adds the
dd-apm-sdk-reviewskill: a multi-perspective review (coherence, security,design, performance, maintainability, conventions, cross-SDK) that runs over a pending
change set and consolidates findings into a single report with an explicit verdict
(BLOCK / APPROVE_WITH_COMMENTS / APPROVE).
Includes:
.agents/skills/dd-apm-sdk-review/), this repo'scontext/overrides (
dd-apm-sdk-review-overrides/)(
.promptfoo/dd-apm-sdk-review/), an equivalent eval suite in a custom format(
.llm-validation/)AGENTS.mdreview-guidelines section making the skilla mandatory push gate
Note: Unlike the equivalent rollout in dd-trace-js, this PR also removes the standalone
perf-reviewskill (.agents/skills/perf-review/,.claude/skills/perf-review), sinceits checks are now folded in as the
performancereviewer perspective insidedd-apm-sdk-review-overrides/reviewers/performance.md.Motivation
We want a consistent, automated review pass — covering correctness, security, and
repo-specific conventions — applied before every push, instead of relying solely on
ad hoc human review. This is phase 1 of rolling that out to dd-trace-java, mirroring
the rollout already done in dd-trace-js (DataDog/dd-trace-js#10070).
Additional Notes
https://github.com/DataDog/dd-apm-sdk-review-core/pull/1
.agents/skills/dd-apm-sdk-review/in this repository.They are exact copies of the files from DataDog/dd-apm-sdk-review-core. Any changes
to them should be made in the upstream repository instead.
dd-apm-sdk-review-overrides/contains the repository- and language-specificreview rules. These are the ones that should be edited directly in this repo.
perf-reviewin favor of the new skill'sperformancereviewerperspective — no standalone performance-review skill remains after this change.
discussion upstream; both eval suites are kept for now, consistent with dd-trace-js.
Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issueJira ticket: [PROJ-IDENT]