From 2887d07ca4477f2aa9aa855037c46300e4a06526 Mon Sep 17 00:00:00 2001 From: Roberto Montero Date: Fri, 11 Sep 2026 11:55:25 +0200 Subject: [PATCH 1/5] chore(agents): add dd-trace-java overrides for dd-apm-sdk-review Repo-specific reviewer facts, AGENTS.md hook, llm-validation suite, and replace the retired perf-review skill. Stacked on the verbatim skill copy. --- .../repo-context.md | 13 + .../reviewers/conventions.md | 56 +++ .../reviewers/design.md | 43 +++ .../reviewers/maintainability.md | 7 + .../reviewers/performance.md | 65 ++++ .../reviewers/security.md | 42 ++ .agents/skills/perf-review/SKILL.md | 245 ------------ .../skills/perf-review/references/.gitignore | 3 - .../skills/perf-review/references/checks.md | 65 ---- .../perf-review/references/example-review.md | 59 --- .../skills/perf-review/references/guide.md | 276 ------------- .claude/skills/perf-review | 1 - .github/CODEOWNERS | 2 + .gitignore | 5 + .gitlab-ci.yml | 23 +- .llm-validation/README.md | 163 ++++++++ .llm-validation/config.yaml | 47 +++ .llm-validation/suites/dd-apm-sdk-review.yaml | 362 ++++++++++++++++++ AGENTS.md | 3 +- 19 files changed, 817 insertions(+), 663 deletions(-) create mode 100644 .agents/dd-apm-sdk-review-overrides/repo-context.md create mode 100644 .agents/dd-apm-sdk-review-overrides/reviewers/conventions.md create mode 100644 .agents/dd-apm-sdk-review-overrides/reviewers/design.md create mode 100644 .agents/dd-apm-sdk-review-overrides/reviewers/maintainability.md create mode 100644 .agents/dd-apm-sdk-review-overrides/reviewers/performance.md create mode 100644 .agents/dd-apm-sdk-review-overrides/reviewers/security.md delete mode 100644 .agents/skills/perf-review/SKILL.md delete mode 100644 .agents/skills/perf-review/references/.gitignore delete mode 100644 .agents/skills/perf-review/references/checks.md delete mode 100644 .agents/skills/perf-review/references/example-review.md delete mode 100644 .agents/skills/perf-review/references/guide.md delete mode 120000 .claude/skills/perf-review create mode 100644 .llm-validation/README.md create mode 100644 .llm-validation/config.yaml create mode 100644 .llm-validation/suites/dd-apm-sdk-review.yaml diff --git a/.agents/dd-apm-sdk-review-overrides/repo-context.md b/.agents/dd-apm-sdk-review-overrides/repo-context.md new file mode 100644 index 00000000000..ddd9bb6c70b --- /dev/null +++ b/.agents/dd-apm-sdk-review-overrides/repo-context.md @@ -0,0 +1,13 @@ +# Repo context — dd-trace-java + +Read only by the orchestrator (Step 0 of `SKILL.md`), not by individual reviewers. Repo-specific; not part of the shared core. This whole `.agents/dd-apm-sdk-review-overrides/` folder is owned by this repo — edit it freely, unlike `.agents/skills/dd-apm-sdk-review/`, which is a verbatim copy of the shared core. + +## Related skills in this repo + +The other skills in this repo author or review specific things; this one is the general multi-perspective push gate. Cite them as authoritative for their own area, do not invoke them, and note they must not invoke this skill either: + +- `techdebt` — duplication / unnecessary complexity / dead-code review, run before marking a PR ready. +- `review-groovy-migration`, `migrate-groovy-to-java` — Groovy→Java test migration tooling and its review pass. +- `apm-integrations` — instrumentation authoring. +- `migrate-junit-source-to-tabletest` — test-source migration tooling. +- `clarify-java-comments` — Javadoc and explanatory-comment review. Defer for documentation-only comment work; do not invoke it. diff --git a/.agents/dd-apm-sdk-review-overrides/reviewers/conventions.md b/.agents/dd-apm-sdk-review-overrides/reviewers/conventions.md new file mode 100644 index 00000000000..3558bf9d49f --- /dev/null +++ b/.agents/dd-apm-sdk-review-overrides/reviewers/conventions.md @@ -0,0 +1,56 @@ +Override for `reviewers/conventions.md` (in the core skill folder) — read that file first, then this. + +# Codebase conventions — dd-trace-java specifics + +## The repo's stated rules + +Start at **AGENTS.md § "Key documentation"** — that table is the index. Open the linked file for the topic under review; do not restate it here. + +Also not in that table, and in scope for this lens: + +- `.editorconfig` and `gradle/spotless.gradle` — the mechanically enforced format (google-java-format via Spotless). Human-facing write-up is **CONTRIBUTING.md § "Automatic code formatting"** and **§ "Static imports"**. +- `.github/pull_request_template.md` — PR body contract. +- `.github/CODEOWNERS` — new paths need an owner when this repo's existing pattern would assign one. +- `metadata/supported-configurations.json` — the config/integration registry CI validates (`validate_supported_configurations_v2_local_file` in `.gitlab-ci.yml`). +- `.agents/skills/apm-integrations/SKILL.md` (+ `references/`) — instrumentation authoring, including integration-name registration and the Groovy-test exception. Cite it; do not invoke it (see `.agents/dd-apm-sdk-review-overrides/repo-context.md`). + +Bootstrap / advice constraints in **AGENTS.md § "Critical constraints"** belong to the design lens, not this one. + +## Mechanical checks — run these, don't eyeball them + +Check-mode only. Anything that would rewrite files is the author's to run; if a check fails, report it. + +Read **AGENTS.md § "Code conventions"** and **CONTRIBUTING.md § "Automatic code formatting"** for the rules, then run the check against the changed modules: + +```bash +./gradlew spotlessCheck # whole repo +./gradlew :path:to:module:spotlessCheck # prefer this when the diff is scoped +# Do NOT run spotlessApply. +``` + +There is no eslint / `tsc` equivalent. Spotless *does* cover Markdown, but only under `gradle/spotless.gradle`'s `format 'markdown'` target: root-level `*.md`, `.github/**/*.md`, `src/**/*.md`, and `application/**/*.md`. Markdown outside those paths — e.g. under `.agents/skills/**` — is not covered; `.editorconfig` is what applies there. If Gradle or the JDK is missing, report `NOT VERIFIED ()` rather than eyeballing format. + +## Config options — registration path + +Read **docs/add_new_configurations.md**. It owns the steps, the files, source priority, and the `supported-configurations.json` schema. Do not restate them from memory; open that doc and check the diff against it. + +Only the parts that doc does not state as a severity: + +- A new `DD_*` / `dd.*` read that is missing from `metadata/supported-configurations.json` is a CI failure (`validate_supported_configurations_v2_local_file`), not a nit — Blocking. +- Integration *names* (the strings passed to `super(...)` / `instrumentationNames()`) also need entries there. That shape is in `.agents/skills/apm-integrations/references/supported-configurations.md`, not in `add_new_configurations.md`. + +## Instrumentations and tests + +- New instrumentation: **docs/add_new_instrumentation.md** (Gradle include, layout, class/package naming) plus **docs/how_instrumentations_work.md § "Naming"** and **§ "Files/Directories"**. Missing `:dd-java-agent:instrumentation:…` include in `settings.gradle.kts` is silent non-build — P0. +- Tests: **docs/how_to_test.md** (and **docs/how_to_test_with_junit.md** when the change is JUnit). **AGENTS.md § "Code conventions"** is the one-line summary; the how-to is the spec. +- New `.groovy` test files are blocked by CI unless the PR has `tag: override groovy enforcement`. Instrumentation tests are the intended exception — see `.agents/skills/apm-integrations/SKILL.md`. + +## Commit and PR hygiene + +Read **CONTRIBUTING.md § "Pull request guidelines"** (draft-first, title, labels, merge queue) and **AGENTS.md § "PR conventions"** (adds `tag: ai generated`). Those own the rules. + +Only the parts not stated there: + +- `.github/workflows/check-pull-requests.yaml` fails a ready PR whose title contains a bracketed tag (`[...]`, e.g. `[CORE]`). That part is a CI gate — flag it as a failing check. Other title quality (imperative verb, changelog wording) is a house rule plus changelog input, not a CI gate — flag a bad title, do not invent a missing-linter finding. +- There is no changelog file: the PR title is the release note. Audit the title and `tag: no release notes` rather than asking for a CHANGELOG entry. +- No in-repo rule mandates `gh --repo` flags or a fork-vs-branch policy; do not invent one. diff --git a/.agents/dd-apm-sdk-review-overrides/reviewers/design.md b/.agents/dd-apm-sdk-review-overrides/reviewers/design.md new file mode 100644 index 00000000000..48fd4367942 --- /dev/null +++ b/.agents/dd-apm-sdk-review-overrides/reviewers/design.md @@ -0,0 +1,43 @@ +Override for `reviewers/design.md` (in the core skill folder) — read that file first, then this. + +# Design — dd-trace-java specifics + +## Module map and layer boundaries + +Start at **ARCHITECTURE.md § "Codemap"** — it owns the module boundaries and what belongs where; do not restate it from memory. Layering rules it states explicitly, in scope for this lens: + +- `dd-trace-core` and `internal-api` "grew organically" and now host multi-product code beyond their original scope. Genuinely product-*agnostic* infrastructure being pulled out of either belongs in `components/`; product-*specific* implementation belongs in `products/`. A new file added to either just because "that's where similar code already lives" is the duplication-of-drift this lens should catch. +- `components/` must stay bootstrap-safe, product-agnostic, and free of *external* dependencies (see ARCHITECTURE.md § "components/"). A new external dependency, or a product-specific type, landing there is a shape violation — but one `components/*` module depending on another bootstrap-safe `components/*` module (e.g. `native-loader` on `environment`) is normal layering, not a violation. +- `products/` modules typically follow the `{product}-api` / `{product}-bootstrap` / `{product}-lib` / `{product}-agent` layering, but no existing product implements it exactly: `metrics` has no `-bootstrap`; `feature-flagging` adds an extra `-config` submodule. Don't flag a missing or extra submodule name against this list — the layering shape is aspirational, not enumerable. What *is* a hard rule regardless of which submodules a product has: implementation weight added to a thin/boundary submodule (`-api`, `-bootstrap`, `-config`) instead of `-lib` is a layer violation, not a style choice. + +## Public API surface + +This repo's public API lives in `dd-trace-api/` (`Tracer`, `GlobalTracer`, `DDTags`, `DDSpanTypes`, the `@Trace` annotation, the `*Config` constant classes), in `dd-trace-ot/`'s `io.opentracing.Tracer` implementation, and in exported product `{product}-api` modules — today `products/feature-flagging/feature-flagging-api`, published as `com.datadoghq:dd-openfeature`. See ARCHITECTURE.md § "dd-trace-api/", § "dd-trace-ot/", and § "products/". A change adding a `public`/`protected` class or method to an exported, externally-accessible type in any of these is public surface and needs explicit justification; it is forever. A package-private or private addition to a non-exported type (e.g. `OTSpan`, `OTSpanContext`, `TypeConverter` in `dd-trace-ot`) is not externally reachable and does not need this justification. `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." + +## Configuration surface + +Read **docs/add_new_configurations.md** — it owns the registration steps; check the diff against it, don't restate it here. One design-shaped consequence that doc doesn't state: `internal-api`'s split between `Config` and `InstrumenterConfig` exists for a build-time reason, not convenience — GraalVM native-image builds freeze instrumentation-affecting decisions into the binary at build time, so a setting that controls which classes/integrations get instrumented belongs in `InstrumenterConfig`; a setting that's runtime-only (endpoints, service name, sampling rate) belongs in `Config` (see ARCHITECTURE.md § "internal-api/"). Landing a native-image-relevant setting in the wrong one breaks native-image builds silently — flag it even if the config-registration mechanics (which belongs to the conventions lens) are otherwise followed correctly. + +## Extension points (instrumentations) + +An instrumentation must go through `InstrumenterModule` + the `Instrumenter` type-matching interfaces (`ForSingleType`, `ForKnownTypes`, `ForTypeHierarchy`, `ForBootstrap`) and be discovered via `@AutoService(InstrumenterModule.class)` — see ARCHITECTURE.md § "agent-tooling/" and **docs/add_new_instrumentation.md** / **docs/how_instrumentations_work.md**. A bespoke `ClassFileTransformer` or advice registered outside this mechanism bypasses Muzzle's build-time version-safety checks entirely — that's a P0 shape problem, not a nit, independent of whether the bespoke code works. + +## Lifecycle / bootstrap + +The bootstrap and advice correctness rules for this code live in **AGENTS.md § "Critical constraints"** and **docs/bootstrap_design_guidelines.md** / **docs/instrumentation_design_guidelines.md** — this lens owns them; do not restate them from memory, open the doc and check the diff against it. (The performance override's "Bootstrap / startup-latency note" covers the same code from the cost angle — that's a different finding on the same lines, not a duplicate.) Respect the ordering in ARCHITECTURE.md § "Startup Sequence": `AgentBootstrap.premain()` must stay tiny and side-effect-free; anything heavier belongs in `Agent.start()` or a product's own `*System.start()`, never in premain-reachable code. + +## Cross-cutting mechanisms already in the repo + +Before approving a new cross-cutting abstraction, check whether one already exists — see ARCHITECTURE.md § "internal-api/": + +- `gateway/` — the Instrumentation Gateway event bus. AppSec and IAST use it to hook the HTTP request lifecycle *without* touching instrumentations directly. A new instrumentation reaching into AppSec/IAST internals directly, instead of publishing through the gateway, is a layering violation. +- `cache/` — `DDCache`, `FixedSizeCache`, `RadixTreeCache`. +- `naming/` — span/service naming schemas (v0, v1). + +A second bespoke event bus, cache, or naming scheme is a P1 duplication finding at minimum, per the generic file's "Duplication of an existing mechanism" check. + +## Not this lens's job + +- Config-registration file mechanics (`supported-configurations.json`, the CI validator) — conventions lens. +- Allocation cost, hot-path multipliers, or JIT behavior of a given shape — performance lens. +- Instrumentation package/class naming and Gradle layout mechanics — conventions lens (the same docs are cited there too; this file only owns whether the extension *mechanism* chosen is the right one, not how it's named or laid out). diff --git a/.agents/dd-apm-sdk-review-overrides/reviewers/maintainability.md b/.agents/dd-apm-sdk-review-overrides/reviewers/maintainability.md new file mode 100644 index 00000000000..2c43965e8d6 --- /dev/null +++ b/.agents/dd-apm-sdk-review-overrides/reviewers/maintainability.md @@ -0,0 +1,7 @@ +# Maintainability — dd-trace-java overrides + +This repo's release-note policy is defined in [`conventions.md`](./conventions.md), not here — read +that override for the actual policy text. + +There is no repo-specific public-API definition beyond what [`design.md`](./design.md) states; fall +back to judgment as `reviewers/maintainability.md` (core) instructs. diff --git a/.agents/dd-apm-sdk-review-overrides/reviewers/performance.md b/.agents/dd-apm-sdk-review-overrides/reviewers/performance.md new file mode 100644 index 00000000000..150233ada2b --- /dev/null +++ b/.agents/dd-apm-sdk-review-overrides/reviewers/performance.md @@ -0,0 +1,65 @@ +Override for `reviewers/performance.md` (in the core skill folder) — read that file first, then this. + +Seeded from this repo's former `.agents/skills/perf-review/references/{guide.md,checks.md}` (the previously-shipped, now-retired Java performance rubric; the skill folder was removed to avoid two competing performance-review entry points). Everything language-agnostic from that rubric moved to the core `reviewers/performance.md` (confidence axis, severity model, domain-adjusted severity, universal checks); everything Java-specific is the addenda below (J1-J15), the instrumentation idioms, the deterministic-lint candidates, and the bootstrap note. + +# Performance — dd-trace-java specifics + +## Posture addenda + +- **Resolve-via-sink before flagging callbacks and hooks.** A per-span callback or scope-hook registration *looks* like a hot-path cost but may be safe once you follow into the registered listener. If every reachable sink is an atomic counter (`LongAdder`, `AtomicLong`) or a no-op-when-disabled, stay silent — a "verify contention" nudge there is noise. Follow the full listener chain before emitting a finding. +- **Hot-path multiplier map.** Cost matters at: span lifecycle (create/setTag/finish), tag map ops, serialization/encoding, the metrics/stats path, decorators, propagation (header read/write). **Multiplier: per-span × spans/request × requests/sec.** A per-span cost is multiplied massively; a per-process/once cost is negligible — reason about which multiplier applies before flagging. + +## Scope note + +**Scope (2026-07-08):** the primary optimization target is **C2 / Java 11+ (HotSpot)** — the mechanisms below are stated in those terms (inline-cache/`TypeProfileWidth` model, C2 speculative inlining, EA — Escape Analysis). C1-only, OpenJ9/J9, GraalVM, and Java 8 should still benefit but are the minority case this rubric doesn't *tune* for. Pairs with benchmark JVM standardization (Java 17 HotSpot). + +## Java addendum to the universal checks (mechanism authored with JIT-developer authority — calibrate production-priority against your own escalation history) + +- **J1 — Escaping allocation defeats Escape Analysis** *(refines universal `per-call-allocation`, `escape-elision-defeated`)*. The JVM scalar-replaces only *non-escaping* short-lived objects. An object stored in the tag map / span / a collection, iterated at serialization, returned, or passed to a **non-inlined / megamorphic** call **escapes** → EA can't elide it → real heap allocation. An ordinary monomorphic/bimorphic virtual call that C2 can inline does **not** prove escape — `flag-as-measure` / verify in JFR (this repo's `MaybeUsagePatternsBenchmark.passedToInlinedStrategy` is 0 B/op). The trap: *"the JIT will scalar-replace it" is false for escaping objects.* **Verify in JFR — EA'd objects don't appear in alloc profiles, so a surviving object in the profile *proves* it escapes.** **Treat a zero-allocation microbenchmark as unverified when the object wraps I/O** (a scope or wrapper spanning a blocking/native call) — C2 cannot inline through a blocking I/O boundary, so a tight-loop benchmark with no real I/O will show scalar-replacement that production won't get; ask for a benchmark that includes realistic I/O before accepting an EA claim for such an object. +- **J2 — Megamorphic dispatch** *(refines universal `polymorphic-dispatch`)*. **PARKED for PR-review flagging (2026-07-08): do NOT raise megamorphism findings in review yet.** Kept as author-reference + a standing-audit target, not an active review idiom — it's too in-the-weeds to land with most devs, and the rubric needs to bank *legible* wins first (allocation, unbounded memory, regex) to earn trust. A hot call site seeing ≥3 receiver types with no dominant one goes megamorphic; ≤2 types stays bimorphic (inlinable); a dominant receiver (≥90%, `TypeProfileMajorReceiverPercent`) still gets guarded mono-inline. The worst sites accumulate silently across many PRs — a per-PR check only catches a PR that *widens* a site, so this wants a periodic `PrintInlining` census of known hot sites, independent of any single review. +- **J3 — JNI / native crossing: overhead + virtual-thread pinning** *(refines universal `native-boundary-crossing`)*. JNI call ≈ 100ns–1µs (state transition, arg pin/copy, no inlining across); string args via `GetStringUTFChars` = UTF-16→UTF-8 copy. **A JNI call from a virtual thread pins the carrier** → no other vthreads on that carrier run while pinned → concurrency collapse for vthread-reliant apps. Fix: batch at flush on the writer (platform) thread, keep the app-vthread path pure-Java, transport interned IDs not strings. Do not recommend `@CriticalNative` — that is an Android ART annotation, not available on HotSpot / OpenJDK and not present in this repo. +- **J4 — GC pressure → tail latency** *(refines universal `per-call-allocation`)*. Hot-path allocation → more GC → STW pauses → app tail latency, not just throughput (the tracer shares the app heap). Assume G1/Parallel (ZGC has short pauses but isn't common). flag-as-measure. +- **J5 — Cardinality-sensitive aggregator** *(domain-specialized universal `unbounded-memory`)*. A config- or user-driven value (tag key, resource name, HTTP URL) feeding a cardinality-sensitive aggregator (e.g. a conflating metrics aggregator with a `maxAggregates` cap) is invisible to the generic "unbounded collection" check because the risk is *cardinality*, not raw size — high-cardinality input thrashes the cap: constant eviction, garbled metrics. flag-with-confidence — **SEV-1**. **Externally-driven caps:** when a collection's growth is controlled by Remote Config, user input, or another external control plane, a PR that removes an existing cap with no replacement bound is flag-with-confidence regardless of whether the diff looks otherwise safe — ask whether the removal was intentional, since the decision may be correct but must be explicit, not incidental. +- **J6 — Reference strengthening in weak-cache scans** *(refines universal `per-call-allocation`, `repeat-work-across-calls`)*. Calling `WeakReference.get()`/`SoftReference.get()` inside a cache-probe loop **strengthens** the reference only for as long as the returned value is retained — a probe that immediately compares or discards the result releases it right away and doesn't defeat collection. flag-with-confidence only when the referent is retained beyond the probe (stored in a field/collection, or kept live across expensive work) — SEV-2/3. Fix: compare a stable key (e.g. `System.identityHashCode`) first; call `.get()` only on a key match or to detect eviction. +- **J7 — `substring`/slice → `SubSequence` zero-copy view** *(refines universal `per-call-allocation`, `escape-elision-defeated`)*. `String.substring`/`subSequence` allocates a fresh backing array per call. On a hot parse path where the slice is **transient** (compared/parsed/appended then discarded), a `SubSequence` view is zero-copy and EA-elided *iff* the consumer takes a `CharSequence`/range. flag-as-measure for the transient case. The retention trap is flag-with-confidence: a `SubSequence` stored in a field/tag/collection pins the *entire* backing string — a small window over a large string is a net memory loss. +- **J8 — Backtracking regex on external input → RE2J / bounded input** *(distinct from universal `repeat-work-across-calls`'s compile-per-call)*. `java.util.regex` backtracks → exponential worst case (ReDoS) on adversarial input. Flag the conjunction: input is user/external-controllable AND the pattern is backtracking-prone (nested/overlapping quantifiers, unanchored `.*` around a quantified group). flag-with-confidence when both hold — SEV-2 (tail latency); **SEV-1** on a per-request security-scan path. That AppSec split isn't uniform: the WAF evaluates rules on a request lifecycle event, not per byte scanned, so a slow WAF rule is comparatively cold relative to IAST, which taints and re-scans data flow through the app on every relevant sink call, closer to a true per-request hot path — weigh IAST regex changes more heavily than WAF rule changes when triaging this addendum. Fix: RE2J (`com.google.re2j`), anchor/de-nest, or hard-cap input length. +- **J9 — `Objects.hash(...)` varargs / boxing hash on a hot path → `HashingUtils`** *(refines universal `per-call-allocation`)*. `Objects.hash(a, b, …)` allocates an `Object[]` per call and boxes every primitive arg. Fix: `datadog.trace.util.HashingUtils` primitive overloads. flag-with-confidence for the varargs/boxing form — SEV-2/3. Do NOT flag allocation-free hand-rolled combines (`31*h + Long.hashCode(x)`) — they already cost nothing. +- **J10 — hot-path `String.format` / string munging** *(refines universal `repeat-work-across-calls`)*. `String.format` parses the format string, boxes its args, and allocates on every call. Fix: direct concatenation or a pre-sized `StringBuilder`. `datadog.trace.util.Strings`/`SubSequence` do not provide a formatting or interpolation replacement — only recommend them when one of their actual helpers (e.g. `replaceAll`, `subSequence`) fits the specific munging pattern. flag-with-confidence for `String.format` on a hot path; flag-as-measure for borderline munging. +- **J11 — composite / multi-dimensional key maps on a hot path → `Hashtable` / `ConcurrentHashtable`** *(refines universal `per-call-allocation`, `unbounded-memory`)*. `Map>` nesting, or a `HashMap` keyed by a composite key, allocates nested maps + boxed keys on the hot aggregation path. Fix: `datadog.trace.util.Hashtable` or `ConcurrentHashtable` (both landed). flag-as-measure — SEV-2/3. +- **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 also when the callee needs a stable snapshot across threads or ownership of the data — a read-only view only blocks mutation *through that view* and still reflects later changes to the backing collection, so swapping in a view where a real copy was needed trades an allocation for an aliasing/concurrency bug. flag-with-confidence only when neither a trust boundary nor a stable-snapshot/ownership-transfer need is established — 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`, `escape-elision-defeated`)*. 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 — but when the call site is inlined and the mapping function isn't retained past the `computeIfAbsent` call, escape analysis can still scalar-replace it on a cache hit. The recurring trap: `map.computeIfAbsent(k, k -> compute())` looks like it allocates the lambda on *every* call including cache hits, but whether it actually survives is optimizer-dependent. flag-as-measure — verify in JFR/an allocation profile that the lambda escapes before treating it as a confirmed cost; only escalate to flag-with-confidence once escape is established (e.g. the mapping function is stored, passed to a virtual call, or the call site is megamorphic) — 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`)*. `Optional.empty()` / `OptionalInt.empty()` / `OptionalLong.empty()` / `OptionalDouble.empty()` return cached singletons and don't allocate. A non-empty `Optional*` construction allocates a new instance, but C2 can scalar-replace it when the value is created and immediately consumed in an inlined method — same as J14's local-object case. flag-with-confidence only when the Optional is returned, stored in a field/collection, or otherwise proven to escape; flag-as-measure for local create-and-consume (verify in JFR before treating it as a confirmed cost). Autoboxing a primitive outside the JVM's cached range (`[-128, 127]` for `Integer`/`Long`) likewise allocates when the boxed value escapes: flag-with-confidence then, flag-as-measure for a locally consumed box. Fix: null checks or primitive-typed return values instead of `Optional`; fixed-arity primitive overloads instead of boxing. SEV-2/3. + +**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`, `ConcurrentHashtable`, `StringIndex` (all `datadog.trace.util`), `RE2J` (`com.google.re2j`). Coming (name as "coming", don't imply present): `UTF8BytesString.Cache`, wider `IntegerCache`, `DDCache` inlining. + +## Instrumentation (ByteBuddy Advice) idioms — dd-trace-java-specific fixes + +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 — do NOT flag.** Both just return a static `INSTANCE` field (`Config.java`, `InstrumenterConfig.java`) — resolution happens once at initialization, not per call. This is a free read; flagging it is a guaranteed false positive. Reserve this idiom for an actual `ConfigProvider` lookup performed repeatedly on a hot path, not for the singleton getters themselves. +- **`@Advice.AllArguments()` — deterministic lint.** Materializes a new `Object[]` boxing all arguments on every advised call; always escapes. Fix: `@Advice.Argument(value=N)`. +- **`@Advice.SkipOn(OnDefaultValue.class)` + cached boolean — the preferred feature-flag pattern.** Compute a `static final boolean` once, return it from `@Advice.OnMethodEnter`, suppress exit advice when disabled. +- **`@Advice.Local` — prefer over `ThreadLocal`.** Carries per-invocation state from `OnMethodEnter` to `OnMethodExit` with no map lookup. +- **Reflective `@Advice.Origin Method`/`Constructor` on hot advice — flag it.** A **String** origin (`@Advice.Origin("#m")`) is a compile-time constant, don't flag it — flag only reflective `Method`/`Constructor`/`Executable` origins. +- **Java Stream API on an advice/hot path — flag-with-confidence.** `stream()`/`IntStream` allocate `Spliterator` + pipeline objects per call. Fix: a plain `for` loop. +- **`switch(String)` — three-tier fix ranking.** (1) resolve to a constant `long` id; (2) open-addressed table; (3) plain `switch` (fine for small, inlinable dispatch). Flag *large* switches; flag *small* switches on hot constant-arg paths only as a soft-alert. + +## Deterministic-lint candidates (don't spend review budget — make these real lints) + +Fixed-signature, mechanically checkable patterns. Flag them if seen, but push to convert them into an actual lint/checkstyle rule rather than relying on this review to catch them every time: + +- A per-call cache/regex/expensive-object creation that should be `static`/computed once. +- Boxing in specific hot APIs that already have a primitive overload. +- Using a string-keyed API where an id-keyed API exists on a hot decorator. +- Violations of an existing convention rule (e.g. extracting a one-shot instrumentation method to a constant) that the conventions lens's tooling could already catch mechanically. + +## Bootstrap / startup-latency note + +Startup latency and bootstrap correctness share a lens: never use `java.util.logging.*`, `java.nio.file.*`, or `javax.management.*` in `premain` code (see `docs/bootstrap_design_guidelines.md`) — that is the correctness side; eager class loading, native library loads, reflection setup, config-regex compilation, eager I/O, and thread creation in premain-reachable code are the performance side, SEV-2. Both route to the platform team as a finding, not a contributor nudge. + +## Evidence + +Benchmark harness: JMH. The two central suites are `dd-java-agent/benchmark/` and `dd-java-agent/benchmark-integration/`, but many modules also carry their own `src/jmh` source set — e.g. `dd-trace-core/src/jmh`, `internal-api/src/jmh`, `dd-trace-ot/src/jmh`, `telemetry/src/jmh`, `components/json/src/jmh`, `dd-java-agent/agent-tooling/src/jmh`, `dd-java-agent/agent-iast/src/jmh`, `dd-java-agent/agent-bootstrap/src/jmh`, `dd-java-agent/appsec/src/jmh`. CI wiring in `.gitlab/benchmarks.yml`. Before concluding no benchmark exists, check for a `src/jmh` set in the changed module, not just the two central suites. If the changed path has (or should have) a JMH benchmark, say whether one exists, whether it ran, and what it showed. If the diff instead includes JFR profiles, use them as evidence too. Otherwise apply the generic file's "Evidence" section (unmeasured hot-path change → flag as unmeasured, don't invent numbers). `./gradlew spotlessApply`/`spotlessCheck` are formatting commands, not performance evidence — they belong to the conventions lens, not here. diff --git a/.agents/dd-apm-sdk-review-overrides/reviewers/security.md b/.agents/dd-apm-sdk-review-overrides/reviewers/security.md new file mode 100644 index 00000000000..d64d69c7558 --- /dev/null +++ b/.agents/dd-apm-sdk-review-overrides/reviewers/security.md @@ -0,0 +1,42 @@ +Override for `reviewers/security.md` (in the core skill folder) — read that file first, then this. + +# Security — dd-trace-java specifics + +This file starts with one confirmed pattern and should grow as more findings are reviewed — do not treat it as exhaustive. + +## "Set it before checking it" — a security control that silently does nothing + +**The pattern:** code turns on a powerful, process-wide capability (a JVM crash handler, a loaded native library, an active instrumentation hook), and only afterwards checks whether the target of that capability is safe to trust. If the check fails, the capability should turn back off — but usually it doesn't, because the code only reacts to a failed check by skipping some *later*, unrelated step. + +In pseudocode: + +``` +setHandler(path) // capability is live now +if (!isTrusted(path)) { + return // too late — setHandler already ran +} +writeConfig(path) +``` + +The fix just swaps the order: + +``` +if (isTrusted(path)) { + setHandler(path) + writeConfig(path) +} +``` + +**Why it matters:** this is not a race condition — one thread, no timing needed, nothing concurrent. It's a plain bug: the trust check exists and runs, but by the time it fails it can no longer stop anything. Treat it as **P0**, not a P1 ordering nit, whenever the bypassed check was the only thing standing between an untrusted path and code/script execution. + +**Where to look for it in this codebase:** +- Anything that arms a crash/error handler before validating the script or path it points to. +- Code that reuses a pre-existing directory or file through the shared `TempLocationManager`. +- A native library load (`System.load`/`loadLibrary`) whose path comes from config. +- A remote-config value applied to a live component before it's schema/bounds-checked. +- An instrumentation hook that activates before its own safety gate — if that gate is Muzzle, check `.agents/dd-apm-sdk-review-overrides/reviewers/design.md` first so you don't report the same thing twice under two lenses. + +## Do not + +- Don't call this a design or coherence issue — it's a security control that runs but has no effect. +- Don't flag ordinary validate-then-use code just because the check and the use live in different methods or classes. The bug is a missing link between the check's *result* and the action — confirm that link is actually broken before reporting. diff --git a/.agents/skills/perf-review/SKILL.md b/.agents/skills/perf-review/SKILL.md deleted file mode 100644 index 5102008ac19..00000000000 --- a/.agents/skills/perf-review/SKILL.md +++ /dev/null @@ -1,245 +0,0 @@ ---- -name: perf-review -description: >- - Performance-overhead review of a code diff / branch / PR for the dd-trace-java - tracer. Flags hot-path allocation, unbounded memory, repeated work, escaping - objects, native-boundary crossings, and JVM-specific pitfalls (escape analysis, - JNI / virtual-thread pinning, backtracking-regex ReDoS, varargs/boxing hashing, - String.format, ByteBuddy-Advice anti-patterns) using the tracer performance - rubric. Use whenever the user wants a performance / overhead / hot-path review, - asks to check a diff or PR for allocation / GC / memory / latency / startup cost, - or mentions the "perf rubric" or the "do no harm / assume hot" tracer posture — - even if they just say "review this for perf" without naming the rubric. Advisory and READ-ONLY: it reports ranked, - verify-first findings; it never edits code. -user-invocable: true -context: fork -allowed-tools: - - Bash - - Read - - Grep - - Glob ---- - -# Performance Review - -Review the current branch's changes for performance overhead in the dd-trace-java -tracer, using the tracer performance rubric bundled in `references/`. This is a -**low-friction advisory nudge**, not a gate: it reports findings and stops. It -never edits code. - -## Why this exists (read first — it sets the whole posture) - -The tracer shares the customer's process, heap, and latency budget. **Do no harm**: -overhead is a form of incorrect behavior that can escalate to real customer harm — -missed SLAs, OOM kills, container restarts, cold-start churn. So the review's job is -to catch overhead the customer would feel, and to do it *without becoming noise*. - -Two forces are in tension, and the resolution defines everything below: - -- **Assume hot.** We don't know what's on a customer's critical path. Absent positive - evidence of cold, assume the code runs on every request, under load, at full - concurrency. The burden of proof runs toward *cold*: ask "is there evidence this is - cold or guarded?" — not "is there evidence this is hot?" (that rationalizes itself - into "probably not"). -- **Precision over recall — be silent when unsure.** A false-positive-prone review - dies of being ignored. Over-flagging kills it faster than under-flagging. This - actively fights your default to be comprehensive and helpful: here, *not* flagging - a borderline case is the correct, skilled move — not a miss. - -You reconcile them with the **confidence axis** and **verify-don't-verdict** (below): -assume-hot makes you *look* everywhere; precision makes you *speak* only when the -mechanism is certain or the severity is catastrophic. - -## Core rules - -- **Findings are prompts to *verify*, not verdicts.** You reason statically; you - cannot render a performance verdict from a code read. Every finding routes into - **Benchmark → Profile → Improve → Guard**. Phrase each as *"this looks like X; - verify with Y"* — never "this is slow." -- **Confidence axis on every finding:** - - **flag-with-confidence** — the cost is *mechanism-determined* and visible in the - code: allocation, boxing, copying, unbounded growth, a native crossing. State it - plainly. - - **flag-as-measure** — the cost depends on JIT/GC/optimizer decisions you can't - see from source: escape elision, inlining/devirtualization, GC impact. Phrase as - "may X; verify with a profiler/benchmark," never as a certainty. -- **Predicate-with-default, not a banned-API list.** Don't flag "you called - `String.format`." Flag *"an eager, unconditional expensive call on a hot, - instrumentation-reachable path."* The same API is fine on a cold path. Two failure - shapes, different fixes: result usually **discarded** → gate/defer; result always - **needed but costly** → cheapen/cache. -- **Resolve interprocedurally — this is the review's whole reason to exist.** A - peephole lint can't answer "reachable from a hot entry, unconditional along the - way." Trace *up* (who calls this? is it reachable from an `@Advice` root / per-span - callback / request handler?) and *down* (follow callbacks, hooks, and listeners to - their **sink** before flagging). If a per-span hook's every reachable sink is an - atomic counter (`LongAdder`, `AtomicLong`) or a no-op-when-disabled, stay silent — a - "verify contention" nudge there is noise. -- **Make the reachability path the headline.** The reachability claim is the most - valuable *and* least reliable part of a finding — residual false positives cluster - in "called it unconditional, missed an upstream guard." Say *"reachable from - `Foo.onEnter` via A→B→C, no guard on that path"* so the reader can check the - shakiest link at a glance. -- **Only flag toward a fix that exists.** A finding must be actionable *now*. Route to - a mechanism that has landed (see the toolkit note in `checks.md` — cite only what - exists; name "coming" primitives as coming). Don't flag a pattern whose only fix is - a mechanism that isn't built yet. -- **Triage by severity.** Flag SEV-1 (unbounded memory / OOM, cardinality blowups) - *aggressively* — a false positive there is cheap insurance against a container kill. - Flag low-severity CPU-micro *conservatively or not at all* — false positives there - only erode trust. -- **Never flag the *absence* of a cache on high-cardinality input.** For open-cardinality - data (raw SQL with literals, per-request strings), *not* caching is the correct - choice — caching it would be the worse SEV-1. Flag a cache *keyed by* high-cardinality - data; never flag the decision not to cache. -- **A *visibly contestable* perf tradeoff shipped without data → one soft flag-as-measure.** - The trigger is narrow: the change makes a **visible tradeoff that could itself regress** — - it removes a lock / guard / synchronization, swaps in a hand-rolled cache or data - structure, or explicitly claims "faster / optimized" — **and** ships no benchmark or - profile. There a static read genuinely can't tell a win from a regression, so raise one - soft *flag-as-measure* nudge: *"this trades ; verify with a JMH benchmark / JFR."* - Do **not** fire it otherwise — if nothing in the diff could plausibly regress, there is - nothing to measure, so stay silent. Specifically not for: a **mechanically-obvious win** - (hoisting an invariant out of a loop, a denser data structure, removing an allocation); - **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. - -## Workflow - -### Step 1 — Get the code to review - -**If the user points you at specific files or pasted code** ("review this class / this -method for perf"), review those directly — skip the diff and go to Step 2 with the same -hot-path mapping and checks. - -**Otherwise, review the branch changes.** Find the merge-base against the DataDog -upstream `master` and diff against it: - -```bash -UPSTREAM=$(git remote -v | grep -E 'DataDog/[^/]+(\.git)?\s' | head -1 | awk '{print $1}') -[ -z "$UPSTREAM" ] && UPSTREAM="origin" -MERGE_BASE=$(git merge-base HEAD ${UPSTREAM}/master) -echo "Reviewing changes since $MERGE_BASE" -git diff $MERGE_BASE --stat -git diff $MERGE_BASE --name-status -``` - -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. - -### Step 2 — Map the changed code onto hot paths - -For each changed method, decide *which multiplier applies* before flagging anything. - -**Hot anchors** (reachable ⇒ assume hot): `@Advice.OnMethodEnter`/`OnMethodExit`, -per-span / per-trace callbacks, request / message handlers, streaming chunk handlers. -**Hot-path map** (where cost is multiplied per-span × spans/request × requests/sec): -span lifecycle (create / setTag / finish), tag-map ops, serialization/encoding, the -metrics/stats path, decorators, propagation (header read/write). - -**Cold only with positive evidence:** one-time init, startup-only path, a genuinely -rare error branch, or behind a guard that provably fires rarely. Watch the -**interprocedural trap** — a method three helpers deep from an `@Advice` entry is -still hot. And note **domain adjustment**: large-denominator domains (LLMObs, CI -Visibility, DSM) absorb per-call CPU/alloc cost, but the risk *inverts* to payload -memory (SEV-1); streaming handlers fire per-chunk, so the large-denominator relief -suspends inside them. See `guide.md` §6. - -### Step 3 — Apply the checks - -Run the changed hot-path code against the rubric. Keep the check index below in mind; -open the references for the precise conditions, confidence, severity, and fix: - -- **`references/guide.md`** — the narrative "how": severity model, hotness rubric, - the 6 categories with worked examples, and the false-positive traps. Read this first - if you're calibrating judgment. -- **`references/checks.md`** — the precise cost-model: 7 universal checks + the Java - addendum (J1–J11) + the ByteBuddy-Advice fix idioms + the toolkit-availability note. - Read this for the exact confidence/severity/fix of a specific pattern. - -### Step 4 — Resolve, then emit - -Before writing a finding: confirm the reachability path, confirm it's unconditional -along that path (check for upstream guards), and follow any hook/callback to its sink. -Drop anything that resolves to benign. Then report in the format below. - -**How many findings to report — scale with diff size:** -- **Small, focused diff** (one method, a handful of files): report *every* genuinely - high-confidence finding, ranked by severity. A tight diff with four real allocation - smells should list all four (as the worked example does). -- **Large PR:** lead with the 1–3 highest-severity findings and note that lower-severity - ones may exist — don't bury the important one under a wall of CPU-micro nits. -- Either way, the gate is *confidence*, not a count: silence on the uncertain ones is - what earns the review its credibility. - -## Output format - -Follow this structure (see `references/example-review.md` for a full worked instance — -). Showing your suppressed lookalikes and what you cleared is not -filler: it demonstrates the precision that makes the findings trustworthy. - -When providing suggestions as code review comments, prefix the comments with "perf: " -```markdown -# Perf Review — - -**Scope reviewed:** - -## Confirmed findings - -### 1. - -- **Confidence:** flag-with-confidence | flag-as-measure -- **Reachability:** -- **Rubric check:** <#N / JN> -- **Severity:** SEV- -- **Fix / verify-with:** - -## Correctly suppressed (not flagged) - - -## Checked, no issue - - -## Summary - -``` - -If nothing survives the confidence bar, say so plainly — "No high-confidence hot-path -findings; here's what I checked and cleared." A clean review is a valid, valuable -result, not a failure to find something. - -## Check index (the map — details in the references) - -**Universal (language-agnostic):** -1. Per-span/per-call allocation on a hot path (retained/escaping) — SEV-2/3 -2. Repeat work across calls (regex compile / format / parse / concat recomputed) — SEV-2/3 -3. Unbounded memory / collection, or keyed by high-cardinality input — **SEV-1** -4. Expensive work on the critical path that could be deferred — SEV-1/2 -5. Polymorphic dispatch defeating inlining/devirt — flag-as-measure — SEV-2/3 -6. FFI / native-boundary crossing per-item (not batched) — SEV-1/2 -7. Escape / allocation-elision defeated by a refactor — flag-as-measure — SEV-2/3 - -**Java addendum (JVM-specific — full text + mechanism in `checks.md`):** -- **J1** escaping allocation defeats Escape Analysis · **J3** JNI crossing + virtual-thread - pinning · **J4** GC pressure → tail latency · **J5** cardinality-sensitive aggregator - (**SEV-1**) · **J6** `WeakReference.get()` in a probe loop strengthens the ref · - **J7** `substring` → `SubSequence` zero-copy view · **J8** backtracking regex on - external input → RE2J (ReDoS) · **J9** `Objects.hash(...)` varargs/boxing → - `HashingUtils` · **J10** hot-path `String.format` → `Strings` · **J11** composite-key - maps → `Hashtable`. -- **J2** megamorphic dispatch is **PARKED** — do *not* raise megamorphism findings in - review yet (kept as author reference only; it needs a standing audit, not per-PR - flagging). See `checks.md` for why. -- ByteBuddy-Advice idioms (`Config.get()` hoisting, `@Advice.AllArguments` → - `@Advice.Argument`, `@Advice.SkipOn`+cached-boolean, `@Advice.Local`, `switch(String)` - three-tier) — in `checks.md`. - -J7–J11 route an *existing* #1/#2/#3 finding to a landed reusable fix — they are not new -triggers. Don't raise a finding you wouldn't have raised anyway. diff --git a/.agents/skills/perf-review/references/.gitignore b/.agents/skills/perf-review/references/.gitignore deleted file mode 100644 index a9f260ab1a9..00000000000 --- a/.agents/skills/perf-review/references/.gitignore +++ /dev/null @@ -1,3 +0,0 @@ -# Local rubric-maintenance lab file — the delta-since-last-roll-up changelog. -# Kept local (not shipped): folded into the local rubric + rolled up to the published copies periodically. -unpublished.md diff --git a/.agents/skills/perf-review/references/checks.md b/.agents/skills/perf-review/references/checks.md deleted file mode 100644 index cdf601f4083..00000000000 --- a/.agents/skills/perf-review/references/checks.md +++ /dev/null @@ -1,65 +0,0 @@ -# Performance Review Checks — AI-review cost model - -Operationalizes the narrative **Performance Review Guide** (`guide.md`) into diff-applicable checks for AI PR review. Complements benchmark-based regression-blocking: benchmarks catch *measured* regressions on *covered* ops; this catches *un-benchmarked* pattern-smells in *any* diff. Grounded in the guide's **Tracer Principles** (do-no-harm, assume-hot) and its severity model. Read this alongside `guide.md` — the guide is the narrative "how"; this is the precise confidence/severity cost-model. - -## Posture (read first) -- **Advisory, not blocking** initially. A nondeterministic, false-positive-prone check that blocks merges dies of being ignored (false positives) or gives false confidence (false negatives). Earn blocking only after precision is proven, and likely only on the deterministic-lint subset. -- **Precision over recall — silent when unsure.** Only flag high-confidence issues on real hot paths. Over-flagging kills the check. (This fights the model's default to be comprehensive/helpful — state it explicitly in the prompt.) -- **Resolve-via-sink before flagging callbacks and hooks.** A per-span callback or scope hook registration *looks* like a hot-path cost but may be safe once you follow into the registered listener. If every reachable sink is an atomic counter (`LongAdder`, `AtomicLong`) or no-op-when-disabled, stay silent — a "verify contention" nudge there is noise. Follow the full listener chain before emitting a finding. -- **Findings are prompts to *verify*, not verdicts.** The AI reasons statically — by the doc's own "Don't Assume; Measure," it *cannot* render a perf verdict. Every finding routes into Benchmark → Profile → Improve → Guard. Phrase as "this looks like X; verify with Y." -- **Triage by severity** (from the doc): flag SEV-1 (memory/OOM) patterns *aggressively* (a false positive is worth catching a container-kill); flag low-severity CPU-micro *conservatively or not at all* (false positives there only erode trust). -- **Scope**: the diff × known hot paths. Don't review the world. -- **Confidence axis** on every finding: **flag-with-confidence** (mechanism-determined: allocation, dispatch, copying, crossing, unboundedness) vs **flag-as-measure** (opaque: depends on JIT/GC/optimizer decisions — escape elision, inlining, GC impact). -- **Visibly-contestable perf tradeoff without data → soft flag-as-measure.** Narrow trigger: the change makes a **visible tradeoff that could itself regress** — removes a lock/guard/synchronization, swaps in a hand-rolled cache/structure, or explicitly claims "faster/optimized" — **and** ships no benchmark/profile. Then a static read can't tell win from regression → one soft flag-as-measure nudge ("trades X for Y — verify with a JMH benchmark / JFR"). If nothing in the diff could plausibly regress, there's nothing to measure → stay silent. Explicitly NOT fired for: a mechanically-obvious win (hoist-invariant, denser structure, removed allocation); routine adoption of a known-better idiom (lower-overhead builder/API/toolkit primitive, no visible downside); or a change that ships a benchmark/JFR (well-evidenced). One line, a nudge not a code-pattern finding. - -## Hot-path map (where cost matters — and the multiplier) -Span lifecycle (create / setTag / finish), tag map ops, serialization/encoding, the metrics/stats path, decorators, propagation (header read/write). **Multiplier: per-span × spans/request × requests/sec.** A per-span cost is multiplied massively; a per-process/once cost is negligible. The AI must reason about *which multiplier applies* before flagging. - -## Universal checks (language-agnostic) -Format: **pattern** — *expensive when (the interprocedural condition to trace)* — confidence — severity — fix. - -1. **Per-span/per-call allocation on a hot path** — *per-span (or hotter) AND the object isn't trivially short-lived (it's retained, returned, captured, or passed across a boundary)* — flag-with-confidence if clearly per-span + retained; flag-as-measure if lifetime/escape is borderline — SEV-2/3 (→SEV-1 if unbounded) — fix: reuse, pool, dense/positional storage, defer out of the hot path. -2. **Repeat work across calls/traces** — *string concat / case-conversion / regex *compile* / format / parse recomputed each hot-path call, on a recurring (low-cardinality) input or allocating each time* — flag-with-confidence — SEV-2/3 — fix: memoize (bounded — see #3) or compile-once (hoist regex to static). -3. **Unbounded memory / collection** — *a cache/map/collection with no size+byte bound, or keyed by a high-cardinality input (per-request data, raw strings)* — flag-with-confidence (unboundedness is structurally visible) — **SEV-1 (OOM / container-kill — top severity)** — fix: bound by count *and* bytes; or don't cache high-cardinality inputs (opt out). -4. **Expensive work on the critical path that could be deferred** — *heavy compute / parse / normalize / serialize / I/O / lock on the synchronous request or span-finish path, that could be moved* — flag-as-*consider* (deferability is contextual) — SEV-1/2 — fix: defer to background/writer thread, lazy-compute, batch. -5. **Polymorphic dispatch on a hot path** — *a hot call site becomes polymorphic enough to defeat the runtime's inlining/devirtualization (real for JIT runtimes — JVM/.NET/V8; AOT/interpreted differ)* — **flag-as-measure** ("may defeat devirtualization; verify on the target runtime") — SEV-2/3 — fix: keep hot call sites mono/bi-morphic; specialize. -6. **FFI / native-boundary crossing on a hot path** *(central to the shared-core effort)* — *a native crossing per-span/per-item (not batched), or transporting strings/objects rather than primitives/IDs* — flag-with-confidence (boundary cost is mechanism-determined; runtime-specific pinning → addendum) — SEV-1/2 (SEV-1 if it blocks/pins under concurrency) — fix: batch (one per flush, not per item); transport interned IDs not strings; keep crossings off the hot/concurrency path. -7. **Escape / allocation-elision defeated** *(Java/Go/.NET/V8 all have a version)* — *a refactor makes a previously-local object escape (stored, returned, captured by a closure, passed to a virtual/non-inlined call) → silent heap allocation on a hot path* — **flag-as-measure** ("may now escape and allocate; verify with an allocation profiler") — SEV-2/3 — fix: keep it local; avoid the escaping store/capture. - -## Deterministic-lint candidates (DON'T spend AI budget — make these real lints) -Fixed-signature, mechanically checkable: -- per-call cache / regex / expensive-object creation that should be static/once -- boxing in specific hot APIs -- using a string-API where an id-API exists on a hot decorator -- the existing convention rules (e.g. don't extract one-shot instrumentation methods to constants) -- *(grows as patterns prove mechanically checkable — migrate them off the AI as they stabilize)* - -## Java addendum (JVM-specific — mechanism authored with JIT-developer authority; **calibrate production-priority against your own escalation history**) -Refines the universal checks with JVM mechanics. Quarantined here, for the Java audience that has the substrate. - -**Scope (2026-07-08):** the primary optimization target is **C2 / Java 11+ (HotSpot)** — the mechanisms below are stated in those terms (inline-cache/`TypeProfileWidth` model, C2 speculative inlining, EA). C1-only, OpenJ9/J9, GraalVM, and Java 8 should still benefit but are the minority case we don't *tune* for. Pairs with the benchmark JVM standardization (Java 17 HotSpot). - -- **J1 — Escaping allocation defeats Escape Analysis** *(refines #1, #7)*. The JVM scalar-replaces only *non-escaping* short-lived objects. An object stored in the tag map / span / a collection, iterated at serialization, or passed to a virtual/megamorphic call **escapes** → EA can't elide it → real heap allocation. The trap: *"the JIT will scalar-replace it" is false for escaping objects* — the dense-store −48% came from removing exactly the escaping per-tag wrappers; an earlier no-Entry change measured ~0 because *those* entries were transient/EA-eligible. **Verify in JFR — EA'd objects don't appear in alloc profiles, so a surviving `Entry` in the profile *proves* it escapes.** -- **J2 — Megamorphic dispatch** *(refines #5)*. **Status — PARKED for PR-review flagging (2026-07-08): do NOT raise megamorphism findings in review yet.** Kept as author-reference + the standing-audit target described below, not as an active review idiom. Rationale: it's too in-the-weeds to land with most devs, and the rubric must first bank *legible* wins (allocation, unbounded memory, regex — what anyone can see in a profiler) to earn trust before deploying the subtle JIT checks. Revisit once the rubric has a track record. (Mechanism below stands; it's the flagging that's held.) A hot call site seeing **≥3 receiver types with no dominant one** goes megamorphic. **Do not frame this as "the virtual call is slow" — a well-predicted indirect branch is a couple of cycles; the dispatch is a rounding error.** The cost is the **optimization fence the un-inlinable call erects on *both* sides**: caller-side, the args escape into an opaque callee → no scalar-replacement/EA on them, no constant-propagation *into* the call, caller-saved registers spilled across it, no hoist/reorder across the boundary; callee-side, it's never specialized to *this* caller, so the arg types/constants that would have collapsed its internal branches and devirtualized its *own* downstream calls stay invisible. Inlining is what lets the two bodies optimize as **one unit**; the megamorphic call severs that — on the hottest paths that is the whole bill. **C2 rescue conditions (the primary target):** ≤2 types stays **bimorphic** (inlinable); a **dominant receiver** (≥`TypeProfileMajorReceiverPercent`, default 90%) still gets guarded mono-inline + uncommon-trap fallback, so a *skewed* site is usually fine — but watch **bimodal oscillation** (a recurring rare type → deopt thrash). The danger zone is the **flat ≥3 distribution** (`TypeProfileWidth`=2 → profile overflow → itable v-call, no inline). Fixes, cheapest first: keep the site to ≤2 types; **gate-when-empty** so the common case skips the fan-out (a default-empty listener array); **collapse N impls into one** `final` class with an internal state/size-class switch (e.g. retire a legacy map impl so the optimized one is the sole impl → every `TagMap` site monomorphic); or **call-site-split/unroll** a stable-order fan-out (catalog #7). **flag-as-measure** — opaque; whether it bites depends on which impls actually load + the runtime receiver mix. Confirm with `-XX:+PrintInlining` (`not inlined (megamorphic)`) / JITWatch, not a code read. **Diff-review blind spot → needs a standing audit.** The worst megamorphic sites *accumulate*: no single PR introduces them (each only nudges the type count by one), so a per-PR check catches only a PR that *widens* a site — the pre-existing hazards are invisible to it. The heaviest is `Context.get`/`with` dispatching across `Empty`/`Singleton`/`Indexed`(+wrapper) impls — the hottest path in the system (catalog #11). These want a **periodic PrintInlining census of the known hot sites**, run independent of any PR, not diff review. -- **J3 — JNI / native crossing: overhead + virtual-thread pinning** *(refines #6)*. JNI call ≈ 100ns–1µs (state transition, arg pin/copy, no inlining across); string args via `GetStringUTFChars` = UTF-16→UTF-8 copy. **A JNI call from a virtual thread pins the carrier** → no other vthreads on that carrier run while pinned → concurrency collapse for vthread-reliant apps. Fix: batch at flush on the **writer (platform) thread**, keep the app-vthread path pure-Java, transport interned IDs not strings; `@CriticalNative` only for short primitive ops (no `JNIEnv`, no object args, no GC-safe state — severe constraints). Flag the crossing + pinning risk (mechanism); overhead magnitude → measure. -- **J4 — GC pressure → *tail* latency** *(refines #1)*. Hot-path allocation → more GC → STW pauses → app **tail** latency, not just throughput (the tracer shares the app heap). ZGC has short pauses but isn't common — assume G1/Parallel. flag-as-measure ("may raise tail latency; verify under load at a realistic heap"). -- **J6 — Reference strengthening in weak-cache scans** *(refines #1, #2)*. Calling `WeakReference.get()` (or `SoftReference.get()`) inside a cache-probe loop to identify the referent **strengthens** the reference — the returned strong ref keeps the object alive until it goes out of scope, defeating the purpose of the weak reference. Pattern to flag: a loop over a weak-ref cache that calls `.get()` for identity/equality comparison on every slot probed. Fix: store a stable key (e.g. `System.identityHashCode(context)`) in the wrapper at construction time; compare the key first (plain int, no strengthening); call `.get()` only on a key match (the right moment — you're about to use the referent anyway) or to detect eviction (`get() == null`). **flag-with-confidence** when `.get()` appears inside a probe loop on a hot path — SEV-2/3. - -- **J7 — `substring`/slice → `SubSequence` zero-copy view** *(refines #1, #7)*. `String.substring`/`subSequence` allocates a fresh backing array per call. On a hot parse path (headers, tags, query strings, SQL/DBM, propagation) where the slice is **transient** — compared (`equals`/`startsWith`/`contains`/`indexOf`), parsed, or appended, then discarded — a `SubSequence` (offset+length view) is zero-copy and EA-elided **iff** the consumer takes a `CharSequence`/range (else the boundary `toString()` erases the win → add the overload or skip). **flag-as-measure** for the transient case (EA-dependent). The retention trap is **flag-with-confidence**: a `SubSequence` stored in a field/tag/collection/cache pins its *entire* backing String — a small window over a large string is a net memory loss — so a retained view must be materialized or `compact()`'d. Discriminator = transient (view, measure) vs retained (must detach). -- **J8 — Backtracking regex on external input → RE2J / bounded input** *(distinct from #2 compile-per-call)*. `java.util.regex` backtracks → exponential worst-case (ReDoS) on adversarial input — a CPU / tail-latency / DoS hazard, **not** an allocation one. Flag the **conjunction**: (a) input is user/external-controllable AND (b) the pattern is backtracking-prone (nested/overlapping quantifiers, `(a+)+`, unanchored `.*` around a quantified group). **flag-with-confidence** when both hold — SEV-2 (tail latency), **SEV-1** on a per-request AppSec/security-scan path (IAST Reporter / WAF run regex on untrusted input every request). Fix: RE2J (`com.google.re2j`, guaranteed linear; no backrefs/lookaround), anchor/de-nest, or hard-cap input length. -- **J9 — `Objects.hash(...)` varargs / boxing hash on a hot path → `HashingUtils`** *(refines #1)*. The allocation is specific to the **varargs/boxing forms**: `Objects.hash(a, b, …)` allocates an `Object[]` per call and **boxes every primitive** arg; same for boxing primitives into a `new Object[]{…}` (or `Arrays.hashCode` over such an array). Per-span tag/key building, or a hot value object's `hashCode()` built this way, → a guaranteed per-call allocation + boxing. Fix: `datadog.trace.util.HashingUtils` — primitive `hash(long/int/boolean/char/…)` overloads (no boxing), `hash(Object,Object)` and `hash(int,int)` combiners (no array); for >2 fields fold pairwise through `hash(int,int)` (there is no varargs form, by design). flag-with-confidence for the varargs/boxing form — SEV-2/3. **Do NOT flag allocation-free combines** — a hand-rolled `31*h + Long.hashCode(x)` / `31*h + intField`, or `Arrays.hashCode` over an *existing primitive array*, allocates nothing (`HashingUtils` is itself 31-based); flagging them would recommend replacing already-correct code. -- **J10 — hot-path `String.format` / string munging → `Strings` (+ `SubSequence`)** *(refines #2)*. `String.format` parses the format string, boxes its args, and allocates on every call — never on a hot path; hand-rolled case-conversion, class/resource-name munging, blank-checks, and truncation recomputed per call qualify too. Fix: `datadog.trace.util.Strings` — allocation-aware `replace`/`truncate(CharSequence)`/`isBlank`/`getResourceName`/`getClassName`/…; for **transient substring compares** prefer a `SubSequence` view (J7); for plain assembly, direct concatenation beats `format`. flag-with-confidence for `String.format` on a hot path; flag-as-measure for borderline munging — SEV-2/3. -- **J11 — composite / multi-dimensional key maps on a hot path → `Hashtable` / `ConcurrentHashtable`** *(refines #1, #3)*. `Map>` nesting, or a `HashMap` keyed by a composite key (client-side stats, per-`(service, operation, …)` aggregation), allocates nested maps + `Entry` objects + boxes keys on the hot aggregation path. Fix: `datadog.trace.util.Hashtable` (single-threaded, composite-key D1/D2 tables — landed) or `datadog.trace.util.ConcurrentHashtable` (lock-free concurrent, **coming**) — positional composite keys, fewer allocations. flag-as-measure — SEV-2/3. -- **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 it's present): `ConcurrentHashtable`, `StringIndex` (immutable string set/map), `UTF8BytesString.Cache` (recurring-string interner), wider `IntegerCache` (http-status/port boxing), `DDCache` inlining. **J7–J11 route an *existing* #1/#2/#3 finding to a reusable fix — they are not new flag-triggers. Don't raise a finding you wouldn't have raised anyway; the posture (precision, silent-when-unsure, findings-cap-scales-with-diff — see `SKILL.md`) is unchanged.** -- **J5 — Cardinality-sensitive aggregator** *(domain-specialized #3)*. Some structures are invisible to the generic "unbounded collection" check because the risk is *cardinality*, not raw size: a config- or user-driven value (tag key, resource name, HTTP URL) feeding a **cardinality-sensitive aggregator** (e.g. the conflating metrics aggregator — each unique label combination = one aggregate; a `maxAggregates` cap bounds OOM but high-cardinality input *thrashes* it: constant eviction, garbled metrics). flag-with-confidence when config/user-driven values reach an aggregator with a per-key budget — **SEV-1** (same class as unbounded memory: correctness + heap impact). Fix: bound the source cardinality before it enters the aggregator, or use sentinel substitution for over-cap values. This surfaced on merged production code more than once in back-test calibration — the capstone pattern where the bot's value concentrates. - -## Instrumentation (ByteBuddy Advice) idioms — dd-trace-java-specific fixes -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."* Two failure shapes, different fixes: (1) the result is usually **discarded** → **gate/defer** (parameterized logging, `isEnabled()` guard) — the cost is avoidable; (2) the result is always **needed but costly** → **cheapen/cache** — gating does nothing. The discriminator (eager? unconditional? guarded? result-needed?) is interprocedural — that's the review's job (§ workflow). These idioms are the actionable fixes when a universal/Java finding lands on an `@Advice` path: - -- **`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. Common trap: the call is buried in a helper invisible at the advice site — grep transitively. (The single most recurring finding in calibration — five independent occurrences.) -- **`@Advice.AllArguments()` — deterministic lint.** Materializes a new `Object[]` boxing all arguments on every advised call; always escapes, EA cannot elide it. Fix: `@Advice.Argument(value=N)` for the one argument and type needed. -- **`@Advice.SkipOn(OnDefaultValue.class)` + cached boolean — the preferred feature-flag pattern.** Compute a `static final boolean` once, return it from `@Advice.OnMethodEnter`, suppress exit advice when disabled. Residual cost: one JIT-hoistable field read per call. Recommend this whenever a `Config.get()` shows up in advice. -- **`@Advice.Local` — prefer over `ThreadLocal`.** Carries per-invocation state from `OnMethodEnter` to `OnMethodExit` with no map lookup. -- **Reflective `@Advice.Origin Method` / `Constructor` on a hot advice.** The reflective origin object is the costly form (per-access reflective lookup). A **String** origin (`@Advice.Origin("#m") String`) is injected as a compile-time constant — no per-call allocation, **don't flag it** (it's the intended cheap form, widely used). Flag only reflective `Method`/`Constructor`/`Executable` origins on a hot advice path; fix: pass the constant String pattern (`#m`, `#t`) instead of the reflective object. -- **Java Stream API on an advice/hot path — flag-with-confidence.** `stream()`/`IntStream` allocate `Spliterator` + pipeline objects per call; JIT elision is fragile. Fix: a plain `for` loop (`cstyleFor`/`enhancedFor`/`forEach`/`iterator` are all on par, ≈0 alloc). See guide §2 for the benchmark numbers and the three silencing exceptions (cache-miss body, length-guarded error path, cold/startup). -- **`switch(String)` — three-tier fix ranking.** (1) resolve to a constant `long` id (folds on any JIT); (2) open-addressed table (never folds but always inlinable, low-variance); (3) plain `switch` (fine for small, inlinable dispatch). Flag *large* switches (inline-budget exhaustion is mechanism-certain); flag *small* switches on hot constant-arg paths only as a version-conditional soft-alert. diff --git a/.agents/skills/perf-review/references/example-review.md b/.agents/skills/perf-review/references/example-review.md deleted file mode 100644 index de17100768c..00000000000 --- a/.agents/skills/perf-review/references/example-review.md +++ /dev/null @@ -1,59 +0,0 @@ -# Perf Review — PR #11903 (Bucket4j instrumentation, demo) - -**PR:** https://github.com/DataDog/dd-trace-java/pull/11903 -**Rubric:** `checks.md` + `guide.md` (this skill's references) -**Scope reviewed:** `Bucket4jDecorator.onConsume` — runs on every `Bucket#tryConsume` call (tracing hot path; multiplier = per-call × calls/sec). -**Method:** Diff reviewed independently of the PR description (description ignored per request). - -## Confirmed findings - -### 1. Per-call config lookup + Set allocation -```java -InstrumenterConfig.get().isIntegrationEnabled(singleton("bucket4j-tier"), true) -``` -`Collections.singleton(...)` allocates a new `SingletonSet` every call, plus a config lookup, for a value that doesn't change per-call. -- **Confidence:** flag-with-confidence -- **Rubric check:** #2 (repeat work on invariant input) -- **Severity:** SEV-2/3 -- **Fix:** hoist to a `static final boolean` (or cache in a field) computed once; eliminate the per-call allocation. - -### 2. `Arrays.stream(...).filter(...).findFirst()` in the hot path -Builds a Stream pipeline (Stream + Spliterator + pipeline stages + captured lambda) every call just to find the first threshold ≥ tokens. -- **Confidence:** flag-with-confidence -- **Rubric check:** #1 / #5 (per-call allocation + unnecessary indirection) -- **Severity:** SEV-3 -- **Fix:** plain `for` loop over `TIER_THRESHOLDS`, no Stream. - -### 3. `Objects.hash(bucket, tokens, consumed)` in `onConsume` -Varargs `Object[]` allocation + boxing of `tokens` (long) and `consumed` (boolean) on every call. Runs unconditionally, not gated behind the tier flag. -- **Confidence:** flag-with-confidence -- **Rubric check:** J9 -- **Severity:** SEV-2/3 -- **Fix:** `datadog.trace.util.HashingUtils` (no boxing, no array). - -### 4. Eager string concatenation in `LOGGER.debug(...)` -```java -LOGGER.debug("bucket4j tryConsume tokens=" + tokens + " consumed=" + consumed + " bucket=" + bucket); -``` -Builds the string (StringBuilder + `bucket.toString()`) unconditionally, even when debug logging is disabled. -- **Confidence:** flag-with-confidence -- **Rubric check:** #2 / J10 -- **Severity:** SEV-2/3 -- **Fix:** SLF4J parameterized form `LOGGER.debug("bucket4j tryConsume tokens={} consumed={} bucket={}", tokens, consumed, bucket)`, or guard with `isDebugEnabled()`. - -## Correctly suppressed (not flagged) - -`private static final int DEFAULT_LIMIT_KEY = Objects.hash("default", 100L);` - -Textually the same `Objects.hash` pattern as finding #3, but this one runs once at class-init (cold path), not per-call. Per the rubric's precision-over-recall posture (silent when unsure / don't erode trust with lookalike false positives), this is correctly **not** flagged. - -## Checked, no issue - -- No unbounded memory / cardinality-sensitive aggregator (check #3, J5) — nothing cached. -- No FFI/native-boundary crossing (check #6, J3). -- No megamorphic-dispatch finding raised — J2 is explicitly parked in the rubric, not an active review idiom. -- String-literal tag keys (`"bucket4j.tier"`, etc.) — JVM interns literals automatically, no per-call allocation cost. - -## Summary - -4 confirmed hot-path findings, all SEV-2/3 (allocation/CPU — none unbounded or OOM-adjacent). 1 lookalike correctly suppressed as cold-path. diff --git a/.agents/skills/perf-review/references/guide.md b/.agents/skills/perf-review/references/guide.md deleted file mode 100644 index 6f847ef39d0..00000000000 --- a/.agents/skills/perf-review/references/guide.md +++ /dev/null @@ -1,276 +0,0 @@ -# Java Tracer Performance Review Guide - ---- - -## Tracer Principles - -Two lines establish everything that follows. - -**Do no harm.** The tracer shares the customer's process, heap, and latency budget. Harm is -ordered by severity: crashes first, then security, then incorrect behavior, then adverse -performance. Performance overhead is a form of incorrect behavior — a non-directly-observable -side effect that can rise to directly observable customer harm: missed SLAs, OOM kills, container -restarts, cold-start churn. - -**Assume hot.** We don't know a priori what will be on the critical path in a customer's -application. In the absence of evidence, assume the code runs on every request, under load, at -full concurrency. There are exceptions — schedulers, startup code, I/O-heavy paths — but the -default is: *assume hot unless there is positive evidence of cold*. - -**Advisory, not blocking.** The rubric is a low-friction nudge alongside the developer's path — -not a wall across it. Flag the 1–2 highest-severity findings per PR. Stay silent when unsure. -Over-flagging kills the check faster than under-flagging. - ---- - -## Severity Guidelines - -| Severity | Type | Usual Cause | -|---|---|---| -| **SEV-1** | OOM / container kill | Unbounded memory growth | -| **SEV-1/2** | Response time — median | Expensive work on the critical path | -| **SEV-1/2** | Response time — tail latency | Allocation rate → GC pauses (shared heap) | -| **SEV-2** | Startup latency | Eager class loading, init, transformation | -| **SEV-2/3** | CPU overhead | General tracer activity, background work | - -CPU overhead alone is the lowest priority — it's a cost issue, not a correctness one, and -escalates only when it causes latency. - -**The denominator matters.** Severity is cost relative to the instrumented operation. A 2 µs tag -op on a sub-millisecond HTTP span is a large fraction of the operation. The same 2 µs on a 500 ms -LLM call is negligible. Large-denominator domains (LLMObs, CI Visibility, DSM) get lower -CPU/alloc severity — but the risk *inverts*: payload memory (large prompts, job metadata, -accumulated output) becomes SEV-1. - -**Default-state changes multiply severity.** A one-line `DEFAULT_X_ENABLED = true` flip applies -the enabled-path cost to every user. Scrutinize heavily regardless of diff size. - ---- - -## Hotness Rubric - -The key question when reviewing any code: *is this on a hot path?* - -The default answer is yes. The burden of proof runs toward cold. Ask "is there evidence this is -cold or guarded?" — not "is there evidence this is hot?" (that rationalizes itself into "probably -not"). - -**Hot anchors.** Paths are hot when reachable from: -- `@Advice.OnMethodEnter` / `@Advice.OnMethodExit` (per-advised-call) -- Per-span or per-trace callbacks -- Request or message handlers -- Streaming chunk handlers (even if the overall stream is slow — see §6) - -**Cold only with positive evidence.** A path is cold if it is: a one-time init, a startup-only -path, a genuinely rare error branch, or behind a guard that provably fires rarely. - -**Watch for the interprocedural trap.** Hot entry points are often indirect. A method buried -three helpers deep from an `@Advice` entry is still hot. Trace up before assuming cold. - ---- - -## Categories of Issues - -In approximate order of severity and frequency: - -1. **Unbounded Memory** — collections or aggregators that grow without a bound -2. **Repeated Allocation on Hot Paths** — regex compile, format strings, streams per call -3. **Per-span Escaping Allocation** — wrapper objects, defensive copies, capturing lambdas -4. **Wrong Collection Type** — heavier type than needed, missing pre-sizing -5. **Startup Latency** — eager work on the premain critical path -6. **Domain-Adjusted Severity** — large-denominator domains, streaming handlers - ---- - -## 1. Unbounded Memory (SEV-1 — flag aggressively) - -A collection that grows without a bound can kill the customer's container. The tracer shares the -application heap — there is no isolation. A false positive here is cheap insurance against a -container kill. Flag aggressively. - -**Raw unbounded cache.** No size or byte bound, keyed by data that grows with load (URL, SQL, -resource names). Fix: `DDCaches.newFixedSizeWeightedCache(n, weigher, maxBytes)`. - -**Cardinality-sensitive aggregator.** A collection with a nominal size bound, but keyed by data -that explodes in cardinality (tag combinations, user-supplied dimensions). High-cardinality input -thrashes the eviction policy — the nominal cap doesn't help. Flag when config or user-driven -values feed such an aggregator without a key-space constraint. - -**Open-cardinality keys.** Any field that varies per-message — timestamp, offset, correlation ID -— used as a key component makes the aggregator grow without bound. Fix: remove the -open-cardinality dimension, or replace raw timestamps with time-buckets. - -**Externally-driven caps.** Any collection grown by Remote Config, user input, or an external -control plane has its growth controlled by the external source. When a PR removes an existing cap -with no replacement bound, flag and ask — the decision may be intentional but must be explicit. - -> **False-positive trap.** Flagging the *absence* of a cache on open-cardinality input (raw SQL -> with inline literals, per-request strings) is wrong. Not caching high-cardinality data *is* the -> correct choice — caching it would be the worse SEV-1. Flag a cache *keyed by* high-cardinality -> data; never flag the decision not to cache. - -*Examples (patterns from back-test calibration):* -- A `LoadingCache` keyed by URL and SQL text with no size or byte limit — the capstone - pattern: unbounded growth tied directly to traffic volume. -- A DSM pathway hash that included a timestamp field, making the aggregator's slot count - grow without bound. Removing the timestamp from the key is a SEV-1 prevention. - ---- - -## 2. Repeated Allocation on Hot Paths (SEV-2/3) - -Tracing is repetitive. Work repeated per-span or per-trace compounds quickly. The focus is on -*allocating* repeat work — patterns that produce garbage the GC must collect — not pure CPU-micro -work like an `.equals()` call. - -**Regex compile per call.** `Pattern.compile(...)` at a non-static site allocates and compiles on -every call. Fix: `static final Pattern`. - -**`String.format` / format-string parsing.** Re-parses and allocates per call. Fix: direct -concatenation, or pre-compute the result. Also watch for locale-dependent formatting crossing the -wire — a correctness issue on top of the perf one. - -**`Config.get()` per call.** Walks a config-resolution chain; not a free read. Fix: hoist to a -`static final` field at class initialization. This is the single most recurring DBM finding — -surfaced independently in five separate PRs. - -**Java Streams on hot paths.** `stream()` and `parallelStream()` always allocate `Spliterator` -and pipeline objects. JIT elision is fragile — small changes to the pipeline or surrounding code -break it silently. Fix: plain `for` loop. Any of `cstyleFor`, `enhancedFor`, `forEach`, or -`iterator` are equivalent and zero-allocation. Benchmark evidence (Java 17, M1, 8 threads, -@Fork(2)): plain loops allocate ≈ 10⁻⁷ B/op; `stream()` always allocates 56–88 B/op; -`parallelStream()` scales from 128 B/op (empty list) to 5 200 B/op (100-element list). - -**`@Advice.AllArguments()`.** Materializes a new `Object[]` boxing all method arguments on every -advised call — always escapes, EA cannot elide it. Fix: `@Advice.Argument(value=N)` for the -specific argument and type needed. - -*Examples (patterns from back-test calibration):* -- A per-trace path with regex compile per call + `String.format` + a locale-dependent formatting - bug — the clearest recall case for mechanism-certain patterns. -- A `StringBuilder(1024)` per query for a ~200-character result. Two-sided error: - under-size causes realloc, over-size wastes memory. Target accurate, not generous. - ---- - -## 3. Per-span Escaping Allocation (SEV-2/3, can reach SEV-1 via tail latency) - -The tracer shares the application heap. Additional allocation contributes to GC and raises -stop-the-world pauses — directly increasing tail latency for the customer's application. The JVM's -escape analysis eliminates *local* short-lived allocations, but only when the object stays local. -Stored in a map, returned, captured by a lambda, or passed to a non-inlined virtual call: it -escapes, and it's real. - -**EA claims for scope/wrapper objects spanning I/O — treat as unverified.** A microbenchmark -tight-loop can show zero allocation for a scope or wrapper object because C2 inlines through -everything and scalar-replaces it. In production, scopes almost always wrap I/O — and C2 cannot -inline through native/blocking I/O boundaries. The object's lifetime extends across the call, it -escapes, and it allocates. A benchmark without I/O-wrapping is not a credible check. Treat EA -claims about per-span scope objects as unverified unless the benchmark explicitly includes -realistic I/O usage. - -**Defensive copies at internal boundaries.** `array.clone()`, `new ArrayList<>(other)` — justified -at real trust boundaries (public API, genuinely mutable external input); wasteful -internal-to-internal where we control all callers. Fix: return a read-only view, or establish a -"don't mutate" contract. - -**Capturing lambda on a hot path.** 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. Common trap: -`map.computeIfAbsent(k, k -> compute())` allocates the lambda on *every* call including cache hits -where it is never invoked. Fix: `get` first, `computeIfAbsent` only on miss. - -**`Optional` and primitive boxing.** Any `Optional*` construction allocates per call and escapes. -Autoboxing outside the JVM cache range ([-128, 127] for `Integer`/`Long`) likewise. Fix: null -checks, primitive return values, or fixed-arity overloads. - -*Examples (patterns from back-test calibration):* -- Capturing lambdas allocated per-span to register per-request callbacks. Allocation - accepted: it buys correctness (fixes a span leak). Cost nominates; the do-no-harm hierarchy - adjudicates. -- A per-instance `TagValue` on a per-trace path — the same shape as the regex and format-string - cases above. - ---- - -## 4. Wrong Collection Type (SEV-2/3) - -Three-step ladder: `LinkedHashMap → HashMap → POJO/record`. Lighter wins. - -**`LinkedHashMap` when order isn't relied on.** ~16 B extra per entry + doubly-linked list -maintenance on every put/remove. Only justified when iteration order is required (insertion-order) -or for LRU (`accessOrder` + `removeEldestEntry`). Fix: `HashMap`. - -**`HashMap` for a fixed, small, known key set.** Pays hashing, boxing, and `Entry` object overhead -per lookup. Fix: a plain record or value class — denser, EA-scalar-replaceable when non-escaping, -type-safe. A 5-line record is often *easier* to write than a map. - -**Mis-sized collections.** `ArrayList` grows 1.5×; `HashMap` doubles and rehashes. Both pay -allocation + copy on growth. Fix: pre-size accurately at construction. Note: `new HashMap<>(n)` -still rehashes at 75% fill — pre-size with `new HashMap<>((int) (n / 0.75f) + 1)` (Java 8-safe); `HashMap.newHashMap(n)` is cleaner but only where the source set is known JDK 19+. - -**Concurrency choice — nominate, don't prescribe.** Replacing `ConcurrentHashMap` with `HashMap` -on a wrong concurrency judgment introduces a data race — trading a performance overhead for a -correctness bug, descending the do-no-harm hierarchy. Frame as a question ("if this map is -thread-confined, a plain collection is cheaper — verify the access pattern"), never a directive. - -*Examples (patterns from back-test calibration):* -- A per-checkpoint `LinkedHashMap` collapsed to a record-like value type. The full - three-step collapse: eliminated per-entry `Entry` overhead, boxing, and linked-list maintenance. - ~20% throughput improvement. -- An oversized `StringBuilder(1024)` is the collection-sizing anti-pattern in a - different form. Accurate sizing, not generous sizing, is the target. - ---- - -## 5. Startup Latency (SEV-2) - -"Once per process" treats startup costs as negligible — but that breaks for serverless (cold starts -are routine), short-lived CI jobs, and deployments that track startup time. - -Flag in premain-reachable code: eager class loading, native library loads (`Native.load`), -reflection setup, config-regex compilation, eager file/network I/O, and thread creation. Fix: -defer to first-use off the hot path, or a background thread post-startup. - -Startup latency and bootstrap correctness share a lens. The bootstrap constraints (no -`java.util.logging` / `java.nio` / `javax.management` in premain) are the correctness side; -startup latency is the performance side. Both route to the platform team — not as contributor -nudges. - -*Examples:* -- `Native.load` inside a `write()` method. If reached on the startup path, it's a - present startup-latency cost (loading libc + building the JNA proxy), not just a latent one. -- **Instrumentation static initializers** — any static field initialization in an `Instrumenter` - subclass that triggers class loading or I/O on first reference is premain-reachable. - ---- - -## 6. Domain-Adjusted Severity - -**Large-denominator domains.** LLMObs, CI Visibility, DSM instrument large units of work (LLM -calls 500 ms+, Spark jobs seconds–minutes, CI test steps milliseconds–minutes). Per-"span" -CPU/alloc severity collapses. But the risk *inverts*: payload memory (large prompts, job metadata, -accumulated output) becomes SEV-1. A CPU-weighted reviewer flags the wrong things and misses the -real one. - -**Streaming handlers — large-denominator rule suspends at the chunk level.** The per-call -denominator applies to costs that fire once per call. Costs inside a streaming handler fire -per-chunk — SSE, chunked HTTP, gRPC streaming can produce hundreds of events per response. An -unbounded accumulator inside a streaming handler (buffering all chunks until stream close) is -SEV-1 regardless of how slow the overall stream is. - -**AppSec sub-domain split.** The WAF blocking path fires only when a block action is triggered — -genuinely cold, SILENT. The IAST taint/sink Reporter can fire frequently during an active security -scan. Treat stream usage and per-call allocations on the IAST Reporter path as SOFT-ALERT, not -cold. Do not apply "AppSec = cold" uniformly across AppSec sub-products. - -*Examples:* -- An LLMObs 5 MB mapper buffer. Same "big buffer" shape as the oversized `StringBuilder` - above, *opposite verdict*: the large-denominator (500 ms+ LLM call) absorbs the cost. - The right call was to accept it. -- An LLMObs streaming helper that accumulated all SSE chunks into an `ArrayList` held - until stream close. The large-denominator rule would have suppressed this; the chunk-level - carve-out catches it: SEV-1, regardless of stream duration. - ---- - -*Companion references in this skill: `checks.md` (the full check list + confidence/severity cost-model + Java addendum) · `example-review.md` (a worked review to calibrate output).* diff --git a/.claude/skills/perf-review b/.claude/skills/perf-review deleted file mode 120000 index 60946879935..00000000000 --- a/.claude/skills/perf-review +++ /dev/null @@ -1 +0,0 @@ -../../.agents/skills/perf-review \ No newline at end of file diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index cc17c8b4b9e..ee9d7502ed9 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -7,6 +7,8 @@ /.agents/ @DataDog/apm-java /.claude/ @DataDog/apm-java /.cursor/ @DataDog/apm-java +/.agents/dd-apm-sdk-review-overrides/ @DataDog/apm-java +/.llm-validation/ @DataDog/apm-java /AGENTS.md @DataDog/apm-java /ARCHITECTURE.md @DataDog/apm-java /CONTRIBUTING.md @DataDog/apm-java diff --git a/.gitignore b/.gitignore index 1230b955e33..5e07d817bdc 100644 --- a/.gitignore +++ b/.gitignore @@ -95,3 +95,8 @@ mise*.local.toml # Exclude kotlin build files .kotlin + +# LLM Validation local/CI artifacts +.llm-validation/results.json +.llm-validation/report.md +.llm-validation/details.json diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 0e26911aa0b..6cf7321c353 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -19,6 +19,15 @@ include: file: '.gitlab/ci-java-post-pr-comment.yml' ref: *apm_sdks_benchmarks_sha - local: ".gitlab/java-benchmark-configs.yml" + - project: "ddoghq/llm-validation-platform" + ref: &llmval_platform_sha "d59e4af6d6666092f65740b9a4d1bf3651a64321" + file: "/ci/llm-validation.gitlab-ci.yml" + +"llm validation": + variables: + LLMVAL_PLATFORM_PROJECT: "ddoghq/llm-validation-platform" + LLMVAL_PLATFORM_REF: *llmval_platform_sha + stages: - build @@ -46,9 +55,6 @@ stages: variables: APM_SDKS_BENCHMARKS_SHA: *apm_sdks_benchmarks_sha - # Test the OpenTelemetry Operator-compatible Java image jobs from one-pipeline. - OTEL_OPERATOR_IMAGES_ENABLED: "true" - OTEL_OPERATOR_LANGUAGE: java # Gitlab runner features; see https://docs.gitlab.com/runner/configuration/feature-flags.html # Fold and time all script sections FF_SCRIPT_SECTIONS: 1 @@ -355,7 +361,7 @@ build: script: - if [ $CI_PIPELINE_SOURCE == "schedule" ] ; then ./gradlew resolveAndLockAll --write-locks $GRADLE_ARGS; fi - ./gradlew --version - - ./gradlew clean :dd-java-agent:shadowJar :dd-java-agent:check :dd-trace-api:jar :dd-trace-ot:shadowJar :products:feature-flagging:feature-flagging-api:jar -PskipTests -x spotlessCheck $GRADLE_ARGS + - ./gradlew clean :dd-java-agent:shadowJar :dd-java-agent:check :dd-trace-api:jar :dd-trace-ot:shadowJar -PskipTests -x spotlessCheck $GRADLE_ARGS - echo UPSTREAM_TRACER_VERSION=$(java -jar workspace/dd-java-agent/build/libs/*.jar) >> upstream.env - echo "BUILD_JOB_NAME=$CI_JOB_NAME" >> build.env - echo "BUILD_JOB_ID=$CI_JOB_ID" >> build.env @@ -365,7 +371,6 @@ build: - 'workspace/dd-java-agent/build/libs/*.jar' - 'workspace/dd-trace-api/build/libs/*.jar' - 'workspace/dd-trace-ot/build/libs/*.jar' - - 'workspace/products/feature-flagging/feature-flagging-api/build/libs/*.jar' - 'upstream.env' - '.gradle/daemon/*/*.out.log' reports: @@ -482,11 +487,9 @@ publish-artifacts-to-s3: - aws s3 cp workspace/dd-java-agent/build/libs/dd-java-agent-${VERSION}.jar s3://dd-trace-java-builds/${CI_COMMIT_REF_NAME}/dd-java-agent.jar - aws s3 cp workspace/dd-trace-api/build/libs/dd-trace-api-${VERSION}.jar s3://dd-trace-java-builds/${CI_COMMIT_REF_NAME}/dd-trace-api.jar - aws s3 cp workspace/dd-trace-ot/build/libs/dd-trace-ot-${VERSION}.jar s3://dd-trace-java-builds/${CI_COMMIT_REF_NAME}/dd-trace-ot.jar - - aws s3 cp workspace/products/feature-flagging/feature-flagging-api/build/libs/dd-openfeature-${VERSION}.jar s3://dd-trace-java-builds/${CI_COMMIT_REF_NAME}/dd-openfeature.jar - aws s3 cp workspace/dd-java-agent/build/libs/dd-java-agent-${VERSION}.jar s3://dd-trace-java-builds/${CI_PIPELINE_ID}/dd-java-agent.jar - aws s3 cp workspace/dd-trace-api/build/libs/dd-trace-api-${VERSION}.jar s3://dd-trace-java-builds/${CI_PIPELINE_ID}/dd-trace-api.jar - aws s3 cp workspace/dd-trace-ot/build/libs/dd-trace-ot-${VERSION}.jar s3://dd-trace-java-builds/${CI_PIPELINE_ID}/dd-trace-ot.jar - - aws s3 cp workspace/products/feature-flagging/feature-flagging-api/build/libs/dd-openfeature-${VERSION}.jar s3://dd-trace-java-builds/${CI_PIPELINE_ID}/dd-openfeature.jar - | cat << EOF > links.json { @@ -496,12 +499,6 @@ publish-artifacts-to-s3: "label": "Public Link to dd-java-agent.jar", "url": "https://s3.us-east-1.amazonaws.com/dd-trace-java-builds/${CI_PIPELINE_ID}/dd-java-agent.jar" } - }, - { - "external_link": { - "label": "Public Link to dd-openfeature.jar", - "url": "https://s3.us-east-1.amazonaws.com/dd-trace-java-builds/${CI_PIPELINE_ID}/dd-openfeature.jar" - } } ] } diff --git a/.llm-validation/README.md b/.llm-validation/README.md new file mode 100644 index 00000000000..00dcf8202fd --- /dev/null +++ b/.llm-validation/README.md @@ -0,0 +1,163 @@ +# LLM Validation — `dd-apm-sdk-review` + +This directory is a [LLM Validation Platform](https://github.com/ddoghq/llm-validation-platform) +suite. It is **not** a JUnit / Gradle test. The cases live here; the runner lives in the +internal platform repo (`ddoghq/llm-validation-platform`). + +It answers: *did an edit to `dd-apm-sdk-review` (SKILL.md, a reviewer, or an override) +make the agent better or worse?* — by comparing **baseline** (those files at +`--base-sha`, usually `master`) against **candidate** (this working tree) under the same +model, judge, and case set. + +Same gate as [`DataDog/dd-trace-dotnet#8845`](https://github.com/DataDog/dd-trace-dotnet/pull/8845) +and [`DataDog/dd-trace-js#10137`](https://github.com/DataDog/dd-trace-js/pull/10137). +CI includes the reusable `"llm validation"` job from the platform repo (see the top-level +`.gitlab-ci.yml`). It lives in the built-in `.post` stage, skips early when no monitored +file changed, and uses this directory's `default_level` (`gate`) unless `LLMVAL_LEVEL` is set. + +## Layout + +| Path | Role | +|---|---| +| [`config.yaml`](./config.yaml) | Monitored instruction files, model, `--level` presets, gate policy | +| [`suites/dd-apm-sdk-review.yaml`](./suites/dd-apm-sdk-review.yaml) | Cases (auto-discovered; do not pass this path to the CLI) | + +## Prerequisites + +- Docker (for the published platform image), **or** a .NET 8/10 SDK + `claude` on `PATH` +- `ddtool` on the host for real (non-`--fake`) runs — to mint a gateway token +- This `dd-trace-java` checkout, with `.llm-validation/` present + +You do **not** need a checkout of `llm-validation-platform` when using Docker. + +## Run locally (Docker) + +The platform publishes the runner image (CLI + toolchain baked in): + +`registry.ddbuild.io/ci/llm-validation-platform/llmval` + +Use `:latest` or pin a pipeline id from the platform's manual `publish-llmval-image` job. +Run from the **`dd-trace-java` repo root** (the directory that contains `.llm-validation/`): + +```bash +export LLMVAL_IMAGE=registry.ddbuild.io/ci/llm-validation-platform/llmval:latest +docker pull "$LLMVAL_IMAGE" + +# Offline smoke — no gateway, no Claude (1 case) +docker run --rm -v "$PWD:/repo" "$LLMVAL_IMAGE" \ + --repo /repo --base-sha master --level minimum --fake + +# Cheap real smoke — still 1 case (`minimum` is a case filter, not "run everything cheaply") +export LLMVAL_AUTH_HEADER="$(ddtool auth token rapid-ai-platform --datacenter us1.staging.dog --http-header)" +docker run --rm -e LLMVAL_AUTH_HEADER -v "$PWD:/repo" "$LLMVAL_IMAGE" \ + --repo /repo --base-sha master --level minimum --runs 1 + +# All cases in suites/ (one repeat each) +docker run --rm -e LLMVAL_AUTH_HEADER -v "$PWD:/repo" "$LLMVAL_IMAGE" \ + --repo /repo --base-sha master --level full --runs 1 + +# CI-shaped set (8 cases) +docker run --rm -e LLMVAL_AUTH_HEADER -v "$PWD:/repo" "$LLMVAL_IMAGE" \ + --repo /repo --base-sha master --level gate --runs 1 + +# One named case (id from suites/dd-apm-sdk-review.yaml) +docker run --rm -e LLMVAL_AUTH_HEADER -v "$PWD:/repo" "$LLMVAL_IMAGE" \ + --repo /repo --base-sha master --case java-security-crash-handler-before-trust --runs 1 +``` + +`--level` picks **which cases** run. `--runs` only changes how many times **those** cases +repeat. `--case`, `--runs`, `--max-cases`, `--concurrency` override the `config.yaml` +preset. Artifacts land in this directory (`results.json`, `report.md`, `details.json`). + +`ANTHROPIC_BASE_URL` defaults to the staging gateway inside the image. Override if needed: + +```bash +docker run --rm \ + -e LLMVAL_AUTH_HEADER \ + -e ANTHROPIC_BASE_URL=https://ai-gateway.us1.ddbuild.io \ + -v "$PWD:/repo" \ + "$LLMVAL_IMAGE" \ + --repo /repo --base-sha master --level minimum +``` + +Renew `LLMVAL_AUTH_HEADER` when it expires (typical symptom: Claude/`api_error_status`:401). + +## Run locally (host .NET) + +From the **platform** repo, point `--repo` at **this** `dd-trace-java` checkout (the +directory that contains `.llm-validation/`), not at the suite YAML: + +```bash +cd /path/to/llm-validation-platform + +dotnet run --project src/Datadog.LlmValidation.Cli -- run \ + --repo /path/to/dd-trace-java \ + --base-sha master \ + --level minimum \ + --out results.json \ + --report report.md \ + --details details.json +``` + +Start with `--level minimum`. `gate` is the CI-shaped run and is slow / expensive. + +### `--level` presets + +Defined in [`config.yaml`](./config.yaml). **`--level` is a case filter, not a +cheapness knob.** `--runs N` does not expand the set — it only repeats the cases +that level already selected. + +| Level | Cases | Default runs | Use | +|---|---|---|---| +| `minimum` | **1** (`java-perf-lens-wrong-collection-001`) | 2 | First smoke | +| `gate` (default) | **8** listed in `config.yaml` | 2 | CI-shaped | +| `full` | **every** case in `suites/` | 2 | Broader pass | + +So this command runs **one** case once, not the whole suite: + +```bash +docker run --rm -e LLMVAL_AUTH_HEADER -v "$PWD:/repo" "$LLMVAL_IMAGE" \ + --repo /repo --base-sha master --level minimum --runs 1 +``` + +To run every case once, use `--level full`. To run the CI set once, use `--level gate`. + +### One specific case + +`--case` takes the `id` from [`suites/dd-apm-sdk-review.yaml`](./suites/dd-apm-sdk-review.yaml) +(e.g. `java-perf-lens-wrong-collection-001`, `java-security-crash-handler-before-trust`). +It overrides the preset’s case list; `--level` still supplies default `--runs` unless you +pass `--runs`. + +```bash +# Docker +docker run --rm -e LLMVAL_AUTH_HEADER -v "$PWD:/repo" "$LLMVAL_IMAGE" \ + --repo /repo --base-sha master --case java-security-crash-handler-before-trust --runs 1 + +# Host .NET (from the platform repo) +dotnet run --project src/Datadog.LlmValidation.Cli -- run \ + --repo /path/to/dd-trace-java \ + --base-sha master \ + --case java-security-crash-handler-before-trust \ + --runs 1 \ + --out results.json --report report.md --details details.json +``` + +### What a pass means + +This is an A/B comparison, not an absolute score of the suite: + +- **Candidate** = the working tree (`File.ReadAllText`). Uncommitted edits count; you do + not need a commit. +- **Baseline** = `git show :`. If a file is not on `master` yet, the CLI + prints `note: not found at master — treated as added in the PR` and compares + *no skill vs this skill*. That is a smoke test, not “does this rubric catch the bug?” + +The gate fails only on a **confident regression** (a new safety / bad signal, or a tight +pairwise loss). Noisy or marginal changes WARN and do not block. + +The report also prints an advisory **Candidate criteria coverage** line (how many +`expected_criteria` the candidate met) and **Candidate bad signals (all)** (every trip, +not only ones newly introduced vs baseline). Neither changes PASS/WARN/FAIL. + +Per-response `expected_criteria` / `bad_signals` / `criteria_met` land in `details.json`. diff --git a/.llm-validation/config.yaml b/.llm-validation/config.yaml new file mode 100644 index 00000000000..1e4632ba076 --- /dev/null +++ b/.llm-validation/config.yaml @@ -0,0 +1,47 @@ +model: claude-opus-4-8 +runs: 2 + +instruction_files: + - .agents/skills/dd-apm-sdk-review/SKILL.md + - .agents/skills/dd-apm-sdk-review/reviewers/_common.md + - .agents/skills/dd-apm-sdk-review/reviewers/correctness.md + - .agents/skills/dd-apm-sdk-review/reviewers/performance.md + - .agents/skills/dd-apm-sdk-review/reviewers/report-template.md + - .agents/skills/dd-apm-sdk-review/review-without-harness.md + - .agents/skills/dd-apm-sdk-review/reviewers/coherence.md + - .agents/skills/dd-apm-sdk-review/reviewers/security.md + - .agents/skills/dd-apm-sdk-review/reviewers/design.md + - .agents/skills/dd-apm-sdk-review/reviewers/maintainability.md + - .agents/skills/dd-apm-sdk-review/reviewers/conventions.md + - .agents/skills/dd-apm-sdk-review/reviewers/cross-sdk.md + - .agents/dd-apm-sdk-review-overrides/repo-context.md + - .agents/dd-apm-sdk-review-overrides/reviewers/security.md + - .agents/dd-apm-sdk-review-overrides/reviewers/performance.md + - .agents/dd-apm-sdk-review-overrides/reviewers/design.md + - .agents/dd-apm-sdk-review-overrides/reviewers/conventions.md + - .agents/dd-apm-sdk-review-overrides/reviewers/maintainability.md + +default_level: gate +presets: + gate: + cases: + - java-perf-lens-wrong-collection-001 + - java-perf-pipeline-full-review-002 + - java-security-crash-handler-before-trust + - java-correctness-capture-before-send + - java-correctness-sqs-queue-name-incomplete + - java-maintainability-resource-leak-streams + - java-correctness-span-events-list-only + - java-correctness-mapper-state-leak + runs: 2 + minimum: + cases: ["java-perf-lens-wrong-collection-001"] + runs: 2 + full: + runs: 2 + +policy: + noise_threshold: 1.0 + pairwise_win_floor: 0.45 + blocking_fail_floor: 0.45 + blocking_fail_ci_upper: 0.55 diff --git a/.llm-validation/suites/dd-apm-sdk-review.yaml b/.llm-validation/suites/dd-apm-sdk-review.yaml new file mode 100644 index 00000000000..6ddf4521f5d --- /dev/null +++ b/.llm-validation/suites/dd-apm-sdk-review.yaml @@ -0,0 +1,362 @@ +name: dd-apm-sdk-review +version: "0.1" + +cases: + - id: java-perf-lens-wrong-collection-001 + files: + - .agents/skills/dd-apm-sdk-review/reviewers/_common.md + - .agents/skills/dd-apm-sdk-review/reviewers/performance.md + - .agents/dd-apm-sdk-review-overrides/reviewers/performance.md + input: | + Apply this repo's performance-review criteria (reviewers/performance.md, its + .agents/dd-apm-sdk-review-overrides/reviewers/performance.md override, and _common.md) to the + following method. No git checkout available — this snippet is the entire change to + review. + + ```java + private final Map sessionCache = new LinkedHashMap<>(); + + void recordSession(String sessionId, Object payload) { + sessionCache.put(sessionId, payload); + } + ``` + expected_criteria: + - "Flags the LinkedHashMap as the wrong collection type per addendum J12 — order 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 three, mapped to P0 or P1 per performance.md's SEV-to-P mapping section." + - "Names HashMap as the concrete fix, not just \"reconsider the data structure.\"" + bad_signals: + - "Treats the LinkedHashMap choice as a pure style nit with no cost explanation." + - "Misses the unbounded / high-cardinality aspect entirely." + - "Invents a Datadog-internal collection type name that doesn't exist in this repo's toolkit (Strings, SubSequence, HashingUtils, Hashtable, ConcurrentHashtable, StringIndex, RE2J)." + + - id: java-perf-pipeline-full-review-002 + files: + - .agents/skills/dd-apm-sdk-review/SKILL.md + - .agents/skills/dd-apm-sdk-review/reviewers/_common.md + - .agents/skills/dd-apm-sdk-review/reviewers/coherence.md + - .agents/skills/dd-apm-sdk-review/reviewers/correctness.md + - .agents/skills/dd-apm-sdk-review/reviewers/security.md + - .agents/skills/dd-apm-sdk-review/reviewers/design.md + - .agents/skills/dd-apm-sdk-review/reviewers/performance.md + - .agents/skills/dd-apm-sdk-review/reviewers/maintainability.md + - .agents/skills/dd-apm-sdk-review/reviewers/conventions.md + - .agents/skills/dd-apm-sdk-review/reviewers/cross-sdk.md + - .agents/skills/dd-apm-sdk-review/reviewers/report-template.md + - .agents/dd-apm-sdk-review-overrides/repo-context.md + - .agents/dd-apm-sdk-review-overrides/reviewers/security.md + - .agents/dd-apm-sdk-review-overrides/reviewers/design.md + - .agents/dd-apm-sdk-review-overrides/reviewers/performance.md + - .agents/dd-apm-sdk-review-overrides/reviewers/conventions.md + - .agents/dd-apm-sdk-review-overrides/reviewers/maintainability.md + input: | + Run dd-apm-sdk-review on the following change. There is no git repository to + inspect — this diff is the entire change set; do not attempt any git commands, go + straight to running the reviewers on the pasted content below. + + ```diff + diff --git a/dd-trace-core/src/main/java/datadog/trace/core/SpanCache.java b/dd-trace-core/src/main/java/datadog/trace/core/SpanCache.java + index 1111111..2222222 100644 + --- a/dd-trace-core/src/main/java/datadog/trace/core/SpanCache.java + +++ b/dd-trace-core/src/main/java/datadog/trace/core/SpanCache.java + @@ -10,6 +10,10 @@ class SpanCache { + - private final Map byResource = new HashMap<>(); + + private final Map byResource = new LinkedHashMap<>(); + + + + String describe(String resourceName) { + + return String.format("resource=%s", resourceName); + + } + ``` + expected_criteria: + - "The report's Mode line states pasted diff / no git (e.g. \"pasted diff, no git\")." + - "Includes a \"Rule files used\" section listing which reviewer files were used, per report-template.md." + - "Raises a performance finding on the LinkedHashMap swap (J12) and/or the hot-path String.format call (J10), classified under the report's P0/P1/P2 sections, not the internal SEV vocabulary." + - "States an explicit verdict line (BLOCK / APPROVE_WITH_COMMENTS / APPROVE) and the matching gate line: `DO NOT PUSH` on BLOCK, `WAITING ON HUMAN` on APPROVE_WITH_COMMENTS, or `READY TO PUSH` on APPROVE." + - "The maintainability lens does not invent its own release-note or public-API policy — per its override (.agents/dd-apm-sdk-review-overrides/reviewers/maintainability.md), it defers to conventions.md's actual policy (no changelog file; the PR title is the release note) rather than asking for a CHANGELOG.md entry." + bad_signals: + - "Attempts to run git commands despite the change set being pasted inline." + - "The maintainability lens asks for a CHANGELOG.md or changelog entry to be added." + - "Skips straight to a verdict with no per-lens findings or \"Rule files used\" section." + - "Reports SEV-1 / SEV-2 labels in the final report without resolving them to P0/P1/P2." + + - id: java-security-crash-handler-before-trust + files: + - .agents/skills/dd-apm-sdk-review/SKILL.md + - .agents/skills/dd-apm-sdk-review/reviewers/_common.md + - .agents/skills/dd-apm-sdk-review/reviewers/security.md + - .agents/skills/dd-apm-sdk-review/reviewers/report-template.md + - .agents/dd-apm-sdk-review-overrides/reviewers/security.md + input: | + Run dd-apm-sdk-review on the following change. There is no git repository to + inspect — this diff is the entire change set; do not attempt any git commands, go + straight to running the reviewers on the pasted content below. + + ```diff + diff --git a/dd-java-agent/agent-crashtracking/src/main/java/datadog/crashtracking/Initializer.java b/dd-java-agent/agent-crashtracking/src/main/java/datadog/crashtracking/Initializer.java + @@ -358,10 +358,16 @@ private static void initializeCrashUploader(FlagAccess flags) { + + // set the JVM flag + + boolean rslt = flags.setValue("OnError", onErrorVal); + + if (!rslt && LOG.isDebugEnabled()) { + + LOG.debug( + + SEND_TELEMETRY, + + "Unable to set OnError flag to {}. Crash-tracking may not work.", + + onErrorVal); + + } + + + + // validate the script directory/file is owned by this JVM user and not + + // group/world-writable before trusting it; if a pre-existing dir/script + + // fails this check, initialize() returns false and skips writing config. + + CrashUploaderScriptInitializer.initialize(uploadScript, onErrorFile); + ``` + expected_criteria: + - "Flags that the JVM handler flag (`OnError`) is set to the target script/path BEFORE `CrashUploaderScriptInitializer.initialize(...)` — the ownership/trust validation — has run and returned a result. It is not enough to note that a validation step exists; the report must call out the ordering problem — if `initialize(...)` later rejects the path, the flag has already been committed to the untrusted path and the JVM crash handler will still execute it on a crash." + - "Classifies this as a security finding (P0 or P1, not merely a style/maintainability nit) — a security control that can be silently bypassed." + - "Recommends gating the flag-setter on the validator's result (e.g. `if (initialize(...)) { flags.setValue(...) }`) or clearing/restoring the flag on validation failure — not just \"add more logging\" or \"add a comment\"." + - "States the verdict line as exactly Verdict BLOCK and ends with DO NOT PUSH." + bad_signals: + - "Misses the ordering problem and only notes that a validation step exists." + - "Treats this as a style or logging nit rather than a security bypass." + - "Approves the change or uses APPROVE / APPROVE_WITH_COMMENTS / READY TO PUSH." + + - id: java-correctness-capture-before-send + files: + - .agents/skills/dd-apm-sdk-review/SKILL.md + - .agents/skills/dd-apm-sdk-review/reviewers/_common.md + - .agents/skills/dd-apm-sdk-review/reviewers/correctness.md + - .agents/skills/dd-apm-sdk-review/reviewers/performance.md + - .agents/skills/dd-apm-sdk-review/reviewers/report-template.md + input: | + Run dd-apm-sdk-review on the following change. There is no git repository to + inspect — this diff is the entire change set; do not attempt any git commands, go + straight to running the reviewers on the pasted content below. + + ```diff + diff --git a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/probe/LogProbe.java b/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/probe/LogProbe.java + @@ -700,6 +700,20 @@ public class LogProbe extends ProbeDefinition implements Sampled, CapturedContext.Status { + + // PR #12207: processCaptureExpressions() runs for every hit, regardless of + + // whether the probe's condition/sampler has already decided this hit will + + // not be sent. logStatus.shouldSend() reflects that effective send decision + + // and is already computed by the caller before this method runs. + + private void processCaptureExpressions(CapturedContext context, LogStatus logStatus) { + + if (captureExpressions == null) { + + return; + + } + + for (CaptureExpression captureExpression : captureExpressions) { + + try { + + context.addCaptureExpression(captureExpression.evaluate(context)); + + } catch (Exception ex) { + + DEBUGGER_METRICS.increment(EVALUATION_ERROR); + + reportEvaluationError(captureExpression, ex); + + } + + } + + } + ``` + expected_criteria: + - "Flags that `processCaptureExpressions()` evaluates every capture expression as soon as `captureExpressions` is non-null, without checking `logStatus.shouldSend()` — the effective send/sampling decision already computed by the caller." + - "Explains the concrete consequence — a hit that is rejected or rate-limited still pays for expression evaluation, so a broken capture expression on a hot method produces an unthrottled stream of evaluation errors for every hit." + - "Recommends gating the evaluation loop on the send decision, e.g. `if (captureExpressions == null || !logStatus.shouldSend()) { return; }`." + - "States an explicit verdict line and the matching gate line: `DO NOT PUSH` on BLOCK, `WAITING ON HUMAN` on APPROVE_WITH_COMMENTS, or `READY TO PUSH` on APPROVE." + bad_signals: + - "Only notes the null check and misses the missing `shouldSend()` gate." + - "Treats this as a cosmetic style issue rather than reliability/performance." + - "Suggests only \"add rate limiting to error reporting\" without gating evaluation." + + - id: java-correctness-sqs-queue-name-incomplete + files: + - .agents/skills/dd-apm-sdk-review/SKILL.md + - .agents/skills/dd-apm-sdk-review/reviewers/_common.md + - .agents/skills/dd-apm-sdk-review/reviewers/correctness.md + - .agents/skills/dd-apm-sdk-review/reviewers/report-template.md + input: | + Run dd-apm-sdk-review on the following change. There is no git repository to + inspect — this diff is the entire change set; do not attempt any git commands, go + straight to running the reviewers on the pasted content below. + + ```diff + diff --git a/dd-java-agent/instrumentation/aws-java/aws-java-sdk-2.2/src/main/java/datadog/trace/instrumentation/aws/v2/AwsSdkClientDecorator.java b/dd-java-agent/instrumentation/aws-java/aws-java-sdk-2.2/src/main/java/datadog/trace/instrumentation/aws/v2/AwsSdkClientDecorator.java + @@ -150,10 +150,14 @@ public class AwsSdkClientDecorator extends HttpClientDecorator { + + // PR #12159: tag the SQS queue name for observability dashboards. The SDK + + // request model exposes both a "QueueUrl" field (always present on + + // SendMessage/DeleteMessage/batch receive calls) and an optional + + // "QueueName" field (only populated by some request types). + + request + + .getValueForField("QueueUrl", String.class) + + .ifPresent( + + url -> { + + span.setTag(InstrumentationTags.AWS_QUEUE_URL, url); + + setPeerService(span, InstrumentationTags.AWS_QUEUE_URL, url); + + }); + + request.getValueForField("QueueName", String.class).ifPresent(name -> setQueueName(span, name)); + ``` + expected_criteria: + - "Flags that the queue-name tag (`setQueueName`) is only set from the optional `QueueName` field, and is never derived from the always-tagged `QueueUrl` field when `QueueName` is absent." + - "Explains the concrete consequence — SQS request types that only populate `QueueUrl` silently ship with no queue-name tag — incomplete telemetry, not a crash." + - "Recommends deriving the queue name from the URL (e.g. last path segment) as a fallback whenever `QueueName` is absent." + - "States an explicit verdict line and the matching gate line: `DO NOT PUSH` on BLOCK, `WAITING ON HUMAN` on APPROVE_WITH_COMMENTS, or `READY TO PUSH` on APPROVE." + bad_signals: + - "Only notes that `QueueName` is Optional without calling out the missing URL fallback." + - "Treats this as a crash/exception bug rather than a silent data-completeness gap." + - "Suggests only logging when QueueName is missing, with no URL-derived fallback." + + - id: java-maintainability-resource-leak-streams + files: + - .agents/skills/dd-apm-sdk-review/SKILL.md + - .agents/skills/dd-apm-sdk-review/reviewers/_common.md + - .agents/skills/dd-apm-sdk-review/reviewers/maintainability.md + - .agents/skills/dd-apm-sdk-review/reviewers/correctness.md + - .agents/skills/dd-apm-sdk-review/reviewers/report-template.md + - .agents/dd-apm-sdk-review-overrides/reviewers/maintainability.md + input: | + Run dd-apm-sdk-review on the following change. There is no git repository to + inspect — this diff is the entire change set; do not attempt any git commands, go + straight to running the reviewers on the pasted content below. + + ```diff + diff --git a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/symbol/SymbolAggregator.java b/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/symbol/SymbolAggregator.java + @@ -270,10 +270,10 @@ public class SymbolAggregator { + + // PR #12143: parse a single .class entry out of the scanned jar and hand its + + // bytes to parseClass(). jarFile itself is opened in a try-with-resources + + // block by the caller. + + private void parseJarEntry( + + SymDBReport symDBReport, + + JarFile jarFile, + + JarEntry jarEntry, + + Path jarPath, + + ByteArrayOutputStream baos, + + byte[] buffer) { + + LOGGER.debug("parsing jarEntry class: {}", jarEntry.getName()); + + try { + + InputStream inputStream = jarFile.getInputStream(jarEntry); + + int readBytes; + + baos.reset(); + + while ((readBytes = inputStream.read(buffer)) != -1) { + + baos.write(buffer, 0, readBytes); + + } + + parseClass(symDBReport, jarEntry.getName(), baos.toByteArray(), jarPath.toString()); + + } catch (IOException ex) { + + LOGGER.warn("Failed to parse jar entry {}", jarEntry.getName(), ex); + + } + + } + + + + // Directory-scan counterpart of the jar path above: walks jarPath looking for + + // .class files on disk instead of inside a jar. + + private void scanDirectory( + + Path jarPath, + + Set alreadyScannedJars, + + ByteArrayOutputStream baos, + + byte[] buffer, + + SymDBReport symDBReport) { + + try { + + Files.walk(jarPath) + + // explicitly no follow links walking the directory to avoid cycles + + .filter(path -> Files.isRegularFile(path, LinkOption.NOFOLLOW_LINKS)) + + .filter(path -> path.toString().endsWith(".class")) + + .forEach(path -> parseClassFile(symDBReport, path, baos, buffer)); + + } catch (IOException ex) { + + LOGGER.warn("Failed to walk directory {}", jarPath, ex); + + } + + alreadyScannedJars.add(jarPath.toString()); + + } + ``` + expected_criteria: + - "Flags that `parseJarEntry()` obtains `inputStream` from `jarFile.getInputStream(jarEntry)` and never closes it — no try-with-resources and no explicit `close()` in a `finally`." + - "Explains the concrete consequence as a resource leak (Inflater / native memory pressure when scanning a large jar), not a style nit." + - "Separately flags that `scanDirectory()`'s `Files.walk(jarPath)` stream is also never closed — its own finding, not folded into the InputStream one." + - "Recommends wrapping both the `InputStream` and the `Files.walk()` stream in try-with-resources." + - "States an explicit verdict line and the matching gate line: `DO NOT PUSH` on BLOCK, `WAITING ON HUMAN` on APPROVE_WITH_COMMENTS, or `READY TO PUSH` on APPROVE." + bad_signals: + - "Misses the unclosed InputStream or the unclosed Files.walk stream." + - "Folds both leaks into one vague \"resource handling\" note without naming both." + - "Suggests only broader exception catching / logging without try-with-resources." + + - id: java-correctness-span-events-list-only + files: + - .agents/skills/dd-apm-sdk-review/SKILL.md + - .agents/skills/dd-apm-sdk-review/reviewers/_common.md + - .agents/skills/dd-apm-sdk-review/reviewers/correctness.md + - .agents/skills/dd-apm-sdk-review/reviewers/report-template.md + input: | + Run dd-apm-sdk-review on the following change. There is no git repository to + inspect — this diff is the entire change set; do not attempt any git commands, go + straight to running the reviewers on the pasted content below. + + ```diff + diff --git a/dd-trace-core/src/main/java/datadog/trace/common/writer/ddagent/TraceMapperV1.java b/dd-trace-core/src/main/java/datadog/trace/common/writer/ddagent/TraceMapperV1.java + @@ -203,12 +203,12 @@ public final class TraceMapperV1 implements TraceMapper { + + // PR #12107: eventsObject comes from the SPAN_EVENTS tag. Most call paths set + + // it as a List (structured events built in-process), but the OTel bridge sets + + // it as a pre-serialized JSON string (a CharSequence) instead. + + private void encodeSpanEvents(Writable writable, int fieldId, Object eventsObject) { + + writable.writeInt(fieldId); + + if (!(eventsObject instanceof List) || ((List) eventsObject).isEmpty()) { + + writable.startArray(0); + + return; + + } + + + + List events = (List) eventsObject; + + int encodableCount = 0; + + for (Object event : events) { + + if (isEncodableSpanEvent(event)) { + + encodableCount++; + + } + + } + + writable.startArray(encodableCount); + + for (Object event : events) { + + if (isEncodableSpanEvent(event)) { + + encodeSpanEvent(writable, (Map) event); + + } + + } + + } + ``` + expected_criteria: + - "Flags that `encodeSpanEvents()` treats any non-`List` `eventsObject` as empty, even though the comment states the OTel bridge supplies span events as a pre-serialized JSON string (`CharSequence`)." + - "Explains the concrete consequence — OTel-bridge spans silently drop span events from the encoded v1 payload — empty events array, no exception — silent data loss." + - "Recommends recognizing/normalizing the `CharSequence`/JSON-string representation into a `List` before the List-shaped encoding logic runs." + - "States an explicit verdict line and the matching gate line: `DO NOT PUSH` on BLOCK, `WAITING ON HUMAN` on APPROVE_WITH_COMMENTS, or `READY TO PUSH` on APPROVE." + bad_signals: + - "Only praises the empty/null defensive handling and misses the CharSequence path." + - "Treats this as a crash bug rather than silent data loss." + - "Suggests only \"add a null check\" or \"log a warning\" without parsing the string form." + + - id: java-correctness-mapper-state-leak + files: + - .agents/skills/dd-apm-sdk-review/SKILL.md + - .agents/skills/dd-apm-sdk-review/reviewers/_common.md + - .agents/skills/dd-apm-sdk-review/reviewers/correctness.md + - .agents/skills/dd-apm-sdk-review/reviewers/report-template.md + input: | + Run dd-apm-sdk-review on the following change. There is no git repository to + inspect — this diff is the entire change set; do not attempt any git commands, go + straight to running the reviewers on the pasted content below. + + ```diff + diff --git a/dd-trace-core/src/main/java/datadog/trace/common/writer/ddagent/TraceMapperV0_4.java b/dd-trace-core/src/main/java/datadog/trace/common/writer/ddagent/TraceMapperV0_4.java + @@ -300,10 +300,20 @@ public final class TraceMapperV0_4 implements TraceMapper { + + // PR #12096: process/propagation tags (dd-p-*) must be written once per + + // payload, on the first span of the trace chunk currently being mapped. + + // This TraceMapperV0_4 instance is held by DDAgentWriter and reused across + + // every trace chunk sent for the lifetime of the writer. + + private boolean firstSpanWritten; + + + + @Override + + public void map(List> trace, Writable writable) { + + writable.startArray(trace.size()); + + for (int i = 0; i < trace.size(); i++) { + + CoreSpan span = trace.get(i); + + writeSpanFields(writable, span); + + span.processTagsAndBaggage( + + metaWriter + + .withWritable(writable) + + .forSpan(i == 0, i == trace.size() - 1, !firstSpanWritten)); + + firstSpanWritten = true; + + } + + } + ``` + expected_criteria: + - "Flags that `firstSpanWritten` is an instance field that is only ever set to `true` and is never reset to `false` at the start of `map()`." + - "Explains the concrete consequence given mapper reuse across the writer's lifetime: after the first chunk, subsequent chunks never write process/ propagation tags (`dd-p-*`) on their first span — silent cross-payload data loss." + - "Recommends resetting `firstSpanWritten` to `false` at the start of each `map()` call (or an explicit `reset()` before each `map()`)." + - "States an explicit verdict line and the matching gate line: `DO NOT PUSH` on BLOCK, `WAITING ON HUMAN` on APPROVE_WITH_COMMENTS, or `READY TO PUSH` on APPROVE." + bad_signals: + - "Notes the flag exists but misses that nothing re-initializes it per chunk." + - "Suggests only \"make the field volatile\" or \"add logging\" without a per-map reset." + - "Treats this as a crash rather than silent data loss from leaked state." diff --git a/AGENTS.md b/AGENTS.md index 9aa2409f4e8..9d8acf83091 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -76,7 +76,8 @@ docs/ Developer documentation (see below) ## Review Guidelines - **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). +- **Local agent with a skill harness (pilot):** before pushing, run [dd-apm-sdk-review](.agents/skills/dd-apm-sdk-review/SKILL.md) on your unpushed changes unless the user says not to. If any `P0` finding is reported, fix it or get explicit authorization and record the unresolved finding verbatim in the PR description. `P1`/`P2` findings should be fixed before pushing but can be dismissed by the human. Security findings are never pasted into the PR description — route them privately. (Performance review, previously the standalone `/perf-review` skill, is now one of this skill's perspectives — see `.agents/dd-apm-sdk-review-overrides/reviewers/performance.md`.) +- **Reviewer without a skill harness** (for example, GitHub Codex): read and follow `.agents/skills/dd-apm-sdk-review/review-without-harness.md`. Do not load `SKILL.md` or `reviewers/report-template.md`. ## Critical constraints From 2ab24b4dc927a3c858c09dd571bfbb41a2af7ead Mon Sep 17 00:00:00 2001 From: Roberto Montero Date: Fri, 11 Sep 2026 11:56:12 +0200 Subject: [PATCH 2/5] fix(ci): keep current master gitlab-ci when adding llm-validation The previous commit copied .gitlab-ci.yml from the old combined branch and dropped unrelated master changes. --- .gitlab-ci.yml | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 6cf7321c353..bcf963c1eee 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -55,6 +55,9 @@ stages: variables: APM_SDKS_BENCHMARKS_SHA: *apm_sdks_benchmarks_sha + # Test the OpenTelemetry Operator-compatible Java image jobs from one-pipeline. + OTEL_OPERATOR_IMAGES_ENABLED: "true" + OTEL_OPERATOR_LANGUAGE: java # Gitlab runner features; see https://docs.gitlab.com/runner/configuration/feature-flags.html # Fold and time all script sections FF_SCRIPT_SECTIONS: 1 @@ -361,7 +364,7 @@ build: script: - if [ $CI_PIPELINE_SOURCE == "schedule" ] ; then ./gradlew resolveAndLockAll --write-locks $GRADLE_ARGS; fi - ./gradlew --version - - ./gradlew clean :dd-java-agent:shadowJar :dd-java-agent:check :dd-trace-api:jar :dd-trace-ot:shadowJar -PskipTests -x spotlessCheck $GRADLE_ARGS + - ./gradlew clean :dd-java-agent:shadowJar :dd-java-agent:check :dd-trace-api:jar :dd-trace-ot:shadowJar :products:feature-flagging:feature-flagging-api:jar -PskipTests -x spotlessCheck $GRADLE_ARGS - echo UPSTREAM_TRACER_VERSION=$(java -jar workspace/dd-java-agent/build/libs/*.jar) >> upstream.env - echo "BUILD_JOB_NAME=$CI_JOB_NAME" >> build.env - echo "BUILD_JOB_ID=$CI_JOB_ID" >> build.env @@ -371,6 +374,7 @@ build: - 'workspace/dd-java-agent/build/libs/*.jar' - 'workspace/dd-trace-api/build/libs/*.jar' - 'workspace/dd-trace-ot/build/libs/*.jar' + - 'workspace/products/feature-flagging/feature-flagging-api/build/libs/*.jar' - 'upstream.env' - '.gradle/daemon/*/*.out.log' reports: @@ -487,9 +491,11 @@ publish-artifacts-to-s3: - aws s3 cp workspace/dd-java-agent/build/libs/dd-java-agent-${VERSION}.jar s3://dd-trace-java-builds/${CI_COMMIT_REF_NAME}/dd-java-agent.jar - aws s3 cp workspace/dd-trace-api/build/libs/dd-trace-api-${VERSION}.jar s3://dd-trace-java-builds/${CI_COMMIT_REF_NAME}/dd-trace-api.jar - aws s3 cp workspace/dd-trace-ot/build/libs/dd-trace-ot-${VERSION}.jar s3://dd-trace-java-builds/${CI_COMMIT_REF_NAME}/dd-trace-ot.jar + - aws s3 cp workspace/products/feature-flagging/feature-flagging-api/build/libs/dd-openfeature-${VERSION}.jar s3://dd-trace-java-builds/${CI_COMMIT_REF_NAME}/dd-openfeature.jar - aws s3 cp workspace/dd-java-agent/build/libs/dd-java-agent-${VERSION}.jar s3://dd-trace-java-builds/${CI_PIPELINE_ID}/dd-java-agent.jar - aws s3 cp workspace/dd-trace-api/build/libs/dd-trace-api-${VERSION}.jar s3://dd-trace-java-builds/${CI_PIPELINE_ID}/dd-trace-api.jar - aws s3 cp workspace/dd-trace-ot/build/libs/dd-trace-ot-${VERSION}.jar s3://dd-trace-java-builds/${CI_PIPELINE_ID}/dd-trace-ot.jar + - aws s3 cp workspace/products/feature-flagging/feature-flagging-api/build/libs/dd-openfeature-${VERSION}.jar s3://dd-trace-java-builds/${CI_PIPELINE_ID}/dd-openfeature.jar - | cat << EOF > links.json { @@ -499,6 +505,12 @@ publish-artifacts-to-s3: "label": "Public Link to dd-java-agent.jar", "url": "https://s3.us-east-1.amazonaws.com/dd-trace-java-builds/${CI_PIPELINE_ID}/dd-java-agent.jar" } + }, + { + "external_link": { + "label": "Public Link to dd-openfeature.jar", + "url": "https://s3.us-east-1.amazonaws.com/dd-trace-java-builds/${CI_PIPELINE_ID}/dd-openfeature.jar" + } } ] } From f8f15a0d730d039fda5ed91f85a6cd8d79f2988c Mon Sep 17 00:00:00 2001 From: Roberto Montero Date: Fri, 11 Sep 2026 14:47:45 +0200 Subject: [PATCH 3/5] chore(ci): pin llm-validation-platform to c33169664 Use the current platform SHA for the GitLab include and the job clone. --- .gitlab-ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index bcf963c1eee..52db20e92df 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -20,7 +20,7 @@ include: ref: *apm_sdks_benchmarks_sha - local: ".gitlab/java-benchmark-configs.yml" - project: "ddoghq/llm-validation-platform" - ref: &llmval_platform_sha "d59e4af6d6666092f65740b9a4d1bf3651a64321" + ref: &llmval_platform_sha "c331696647a78672101002ba48ccebbd41b0fae8" file: "/ci/llm-validation.gitlab-ci.yml" "llm validation": From 7a96d17e1f1b7c40ac3371c57b533b352d56be0f Mon Sep 17 00:00:00 2001 From: Roberto Montero Date: Fri, 11 Sep 2026 15:15:42 +0200 Subject: [PATCH 4/5] fix(agents): score the Java security and mapper eval cases correctly Evaluate crash-handler ordering as a security-lens report, not the orchestrator's redacted output. Require firstSpanWritten to reset on the payload-flush hook, not at every map(). --- .llm-validation/suites/dd-apm-sdk-review.yaml | 28 +++++++++---------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/.llm-validation/suites/dd-apm-sdk-review.yaml b/.llm-validation/suites/dd-apm-sdk-review.yaml index 6ddf4521f5d..63d89112d95 100644 --- a/.llm-validation/suites/dd-apm-sdk-review.yaml +++ b/.llm-validation/suites/dd-apm-sdk-review.yaml @@ -80,15 +80,15 @@ cases: - id: java-security-crash-handler-before-trust files: - - .agents/skills/dd-apm-sdk-review/SKILL.md - .agents/skills/dd-apm-sdk-review/reviewers/_common.md - .agents/skills/dd-apm-sdk-review/reviewers/security.md - - .agents/skills/dd-apm-sdk-review/reviewers/report-template.md - .agents/dd-apm-sdk-review-overrides/reviewers/security.md input: | - Run dd-apm-sdk-review on the following change. There is no git repository to - inspect — this diff is the entire change set; do not attempt any git commands, go - straight to running the reviewers on the pasted content below. + Apply this repo's security-review criteria (reviewers/security.md, its + .agents/dd-apm-sdk-review-overrides/reviewers/security.md override, and _common.md) to + the following change. No git checkout available — this snippet is the entire change to + review. Report the security lens findings (location, failure mode, fix). This is not + the orchestrator's pasteable final report. ```diff diff --git a/dd-java-agent/agent-crashtracking/src/main/java/datadog/crashtracking/Initializer.java b/dd-java-agent/agent-crashtracking/src/main/java/datadog/crashtracking/Initializer.java @@ -111,11 +111,10 @@ cases: - "Flags that the JVM handler flag (`OnError`) is set to the target script/path BEFORE `CrashUploaderScriptInitializer.initialize(...)` — the ownership/trust validation — has run and returned a result. It is not enough to note that a validation step exists; the report must call out the ordering problem — if `initialize(...)` later rejects the path, the flag has already been committed to the untrusted path and the JVM crash handler will still execute it on a crash." - "Classifies this as a security finding (P0 or P1, not merely a style/maintainability nit) — a security control that can be silently bypassed." - "Recommends gating the flag-setter on the validator's result (e.g. `if (initialize(...)) { flags.setValue(...) }`) or clearing/restoring the flag on validation failure — not just \"add more logging\" or \"add a comment\"." - - "States the verdict line as exactly Verdict BLOCK and ends with DO NOT PUSH." bad_signals: - "Misses the ordering problem and only notes that a validation step exists." - "Treats this as a style or logging nit rather than a security bypass." - - "Approves the change or uses APPROVE / APPROVE_WITH_COMMENTS / READY TO PUSH." + - "Approves the change or reduces it to a private-routing notice with no location or failure mode." - id: java-correctness-capture-before-send files: @@ -332,9 +331,10 @@ cases: diff --git a/dd-trace-core/src/main/java/datadog/trace/common/writer/ddagent/TraceMapperV0_4.java b/dd-trace-core/src/main/java/datadog/trace/common/writer/ddagent/TraceMapperV0_4.java @@ -300,10 +300,20 @@ public final class TraceMapperV0_4 implements TraceMapper { + // PR #12096: process/propagation tags (dd-p-*) must be written once per - + // payload, on the first span of the trace chunk currently being mapped. + + // payload, on the first span of the first chunk in that payload. + // This TraceMapperV0_4 instance is held by DDAgentWriter and reused across - + // every trace chunk sent for the lifetime of the writer. + + // every trace chunk sent for the lifetime of the writer. The writer calls + + // mapper.reset() after each payload flush; map() may run many times per payload. + private boolean firstSpanWritten; + + @Override @@ -352,11 +352,11 @@ cases: + } ``` expected_criteria: - - "Flags that `firstSpanWritten` is an instance field that is only ever set to `true` and is never reset to `false` at the start of `map()`." - - "Explains the concrete consequence given mapper reuse across the writer's lifetime: after the first chunk, subsequent chunks never write process/ propagation tags (`dd-p-*`) on their first span — silent cross-payload data loss." - - "Recommends resetting `firstSpanWritten` to `false` at the start of each `map()` call (or an explicit `reset()` before each `map()`)." + - "Flags that `firstSpanWritten` is an instance field that is only ever set to `true` and is never cleared between payloads." + - "Explains the concrete consequence given mapper reuse across the writer's lifetime: after the first payload, later payloads never write process/propagation tags (`dd-p-*`) — silent cross-payload data loss." + - "Recommends clearing `firstSpanWritten` in `reset()` (the payload-flush hook), not at the start of every `map()` — a per-map reset would write the tags on every chunk of the same payload." - "States an explicit verdict line and the matching gate line: `DO NOT PUSH` on BLOCK, `WAITING ON HUMAN` on APPROVE_WITH_COMMENTS, or `READY TO PUSH` on APPROVE." bad_signals: - - "Notes the flag exists but misses that nothing re-initializes it per chunk." - - "Suggests only \"make the field volatile\" or \"add logging\" without a per-map reset." + - "Notes the flag exists but misses that nothing re-initializes it per payload." + - "Suggests resetting the field at the start of every `map()` (or only \"make it volatile\" / \"add logging\")." - "Treats this as a crash rather than silent data loss from leaked state." From 8cbdbb070e4ee74559c2ff537eb8593972e43a43 Mon Sep 17 00:00:00 2001 From: Roberto Montero Date: Fri, 11 Sep 2026 16:19:56 +0200 Subject: [PATCH 5/5] fix(agents): scope Java transformer P0 and seed full-review perf facts Auto-instrumentations still require InstrumenterModule; product transformers do not. The full-review fixture now states the J10/J12 preconditions the performance override already requires. --- .agents/dd-apm-sdk-review-overrides/reviewers/design.md | 4 +++- .llm-validation/suites/dd-apm-sdk-review.yaml | 4 ++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/.agents/dd-apm-sdk-review-overrides/reviewers/design.md b/.agents/dd-apm-sdk-review-overrides/reviewers/design.md index a495c826839..85499b668c2 100644 --- a/.agents/dd-apm-sdk-review-overrides/reviewers/design.md +++ b/.agents/dd-apm-sdk-review-overrides/reviewers/design.md @@ -20,7 +20,9 @@ Read **docs/add_new_configurations.md** — it owns the registration steps; chec ## Extension points (instrumentations) -An instrumentation must go through `InstrumenterModule` + the `Instrumenter` type-matching interfaces (`ForSingleType`, `ForKnownTypes`, `ForTypeHierarchy`, `ForBootstrap`) and be discovered via `@AutoService(InstrumenterModule.class)` — see ARCHITECTURE.md § "agent-tooling/" and **docs/add_new_instrumentation.md** / **docs/how_instrumentations_work.md**. A bespoke `ClassFileTransformer` or advice registered outside this mechanism bypasses Muzzle's build-time version-safety checks entirely — that's a P0 shape problem, not a nit, independent of whether the bespoke code works. +An **auto-instrumentation** under `dd-java-agent/instrumentation/` must go through `InstrumenterModule` + the `Instrumenter` type-matching interfaces (`ForSingleType`, `ForKnownTypes`, `ForTypeHierarchy`, `ForBootstrap`) and be discovered via `@AutoService(InstrumenterModule.class)` — see ARCHITECTURE.md § "agent-tooling/" and **docs/add_new_instrumentation.md** / **docs/how_instrumentations_work.md**. A bespoke `ClassFileTransformer` or advice registered outside this mechanism for an auto-instrumentation bypasses Muzzle's build-time version-safety checks entirely — that's a P0 shape problem, not a nit, independent of whether the bespoke code works. + +This rule is scoped to auto-instrumentations. Product subsystems that already register purpose-built transformers outside `InstrumenterModule` (CI Visibility coverage, debugger, IAST, AppSec, telemetry) are not in scope — do not raise a P0 solely because those products use a standalone `ClassFileTransformer`. ## Lifecycle / bootstrap diff --git a/.llm-validation/suites/dd-apm-sdk-review.yaml b/.llm-validation/suites/dd-apm-sdk-review.yaml index 674e1fbddca..2cd716ed13e 100644 --- a/.llm-validation/suites/dd-apm-sdk-review.yaml +++ b/.llm-validation/suites/dd-apm-sdk-review.yaml @@ -57,6 +57,10 @@ cases: inspect — this diff is the entire change set; do not attempt any git commands, go straight to running the reviewers on the pasted content below. + `byResource` is a lookup-by-key cache: callers only `get(resourceName)`. Insertion + order is not part of the contract. `describe()` is invoked on the per-span write + path (hot). + ```diff diff --git a/dd-trace-core/src/main/java/datadog/trace/core/SpanCache.java b/dd-trace-core/src/main/java/datadog/trace/core/SpanCache.java index 1111111..2222222 100644