From 2d12ee7bbb7405fec5c0fd58d00d371d945399bd Mon Sep 17 00:00:00 2001 From: Douglas Q Hawkins Date: Thu, 22 Oct 2026 19:45:36 -0400 Subject: [PATCH] Move contract annotations into one datadog.contract package datadog.perfcontract and datadog.apicontract each grew around the first annotation that needed them. The meta-annotation already says which kind of contract an annotation is, so one package is enough. Mechanical move; no annotation changes. Co-Authored-By: Claude Opus 5.5 --- .agents/skills/perf-review/references/checks.md | 4 ++-- .github/CODEOWNERS | 3 +-- .../communication/serialization/msgpack/MsgPackWriter.java | 4 ++-- gradle/forbiddenApiFilters/instrumentation.txt | 2 +- .../java/datadog/{perfcontract => contract}/ImpliedFor.java | 2 +- .../java/datadog/{perfcontract => contract}/PerfContract.java | 2 +- .../java/datadog/{apicontract => contract}/Restricted.java | 2 +- .../java/datadog/{perfcontract => contract}/Singleton.java | 2 +- .../datadog/{perfcontract => contract}/StaticLifetime.java | 2 +- .../{perfcontract => contract}/SuppressPerfContract.java | 2 +- internal-api/src/main/java/datadog/trace/api/TagMap.java | 2 +- .../src/main/java/datadog/trace/api/function/NoEscape.java | 4 ++-- .../src/main/java/datadog/trace/api/function/Strategy.java | 2 +- .../java/datadog/trace/api/function/StrategyConsumer.java | 2 +- 14 files changed, 17 insertions(+), 18 deletions(-) rename internal-api/src/main/java/datadog/{perfcontract => contract}/ImpliedFor.java (98%) rename internal-api/src/main/java/datadog/{perfcontract => contract}/PerfContract.java (96%) rename internal-api/src/main/java/datadog/{apicontract => contract}/Restricted.java (97%) rename internal-api/src/main/java/datadog/{perfcontract => contract}/Singleton.java (97%) rename internal-api/src/main/java/datadog/{perfcontract => contract}/StaticLifetime.java (99%) rename internal-api/src/main/java/datadog/{perfcontract => contract}/SuppressPerfContract.java (98%) diff --git a/.agents/skills/perf-review/references/checks.md b/.agents/skills/perf-review/references/checks.md index 3de39dec2a0..3cae0efd32b 100644 --- a/.agents/skills/perf-review/references/checks.md +++ b/.agents/skills/perf-review/references/checks.md @@ -25,8 +25,8 @@ Format: **pattern** — *expensive when (the interprocedural condition to trace) 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. -8. **`@NoEscape` field-storage violation** — *a field (instance or static, directly or as a generic type argument) declared with an `@NoEscape`-annotated type (`datadog.trace.api.function.NoEscape`), **or** initialized directly from a call to an `@NoEscape`-annotated method — with no comment at the declaration justifying the retention*. The type form marks a concrete type (e.g. `SubSequence`); the method form exists for a return value whose concrete type can't itself carry the annotation (an anonymous class or lambda implementing a JDK interface). The annotation's own javadoc carries a self-contained "Checker contract" section (trigger / not-a-trigger / violation example / compliant example, both forms) written so this can be checked from the diff alone, with no other context needed. The underlying rule is "should", not "must" (RFC-2119 sense): a trigger is a presumptive finding, not an automatic failure — compliant if the declaration carries either a `// Retained on purpose: `-style comment, or `@SuppressPerfContract(value = NoEscape.class, reason = "...")` (`datadog.perfcontract.SuppressPerfContract` — the generic exemption mechanism shared by every perf-contract marker, not `@NoEscape`-specific), or a named canned-exception annotation itself meta-annotated `@SuppressPerfContract` with `NoEscape.class` among its `value` (e.g. a hypothetical `@Borrowed`) — treat the meta-annotation's own `reason` as satisfying the requirement. Don't flag a `SuppressPerfContract`/canned-exception use as non-compliant just for being terse; that mirrors the annotation's own Checker contract. **flag-with-confidence** — SEV-2/3 (SEV-1 if the annotated type/method shares backing storage with something large, per its own javadoc). Current wearers: `SubSequence`, `Maybe` (type form; see J7 below for `SubSequence`'s specific retention-vs-transient discriminator). **No lint enforces this yet — the AI reviewer is the only check, so this stays here (not under deterministic-lint candidates below) until a checker lands and it can migrate down.** -9. **`@StaticLifetime` field-lifetime violation** — *a field annotated `@StaticLifetime` (`datadog.perfcontract.StaticLifetime`), **or** an unannotated field whose declared type is listed in the contract's `@ImpliedFor` (`datadog.perfcontract.ImpliedFor`; read the types from the `@ImpliedFor` on `StaticLifetime` itself, subtypes included), that is neither `static final` nor a `final` instance field declared on a class annotated `@Singleton` (`datadog.perfcontract.Singleton`)*. Declaration-scan only, same shape as the `@NoEscape` field-storage check above — it does not attempt to prove a field is actually reused enough to be worth caching, and it does not catch an unannotated per-instance cache of a non-implied type; it only guards fields that opt in or whose type implies the contract. A deliberate per-instance holder of an implied type is compliant with `@SuppressPerfContract(value = StaticLifetime.class, reason = "...")`. `@Singleton`, applied to a class, is a trusted, unverified declaration (v1 posture — the same stance the other perf-contract markers take toward `static final` itself) that the class has exactly one process-wide instance; declaring it satisfies `@StaticLifetime` for that class's `final` instance fields. **flag-with-confidence** — SEV-2/3, motivated by a real production defect (a per-request-constructed cache that never actually amortized anything — see the annotation's own javadoc for the full trigger/accepted-form table). Refines the "per-call cache / expensive-object creation that should be static/once" deterministic-lint candidate below by giving that pattern's opted-in subset an explicit annotation and trigger; the unannotated general case is still that line's job. **No lint enforces this yet — the AI reviewer is the only check, so this stays here (not under deterministic-lint candidates below) until a checker lands and it can migrate down.** +8. **`@NoEscape` field-storage violation** — *a field (instance or static, directly or as a generic type argument) declared with an `@NoEscape`-annotated type (`datadog.trace.api.function.NoEscape`), **or** initialized directly from a call to an `@NoEscape`-annotated method — with no comment at the declaration justifying the retention*. The type form marks a concrete type (e.g. `SubSequence`); the method form exists for a return value whose concrete type can't itself carry the annotation (an anonymous class or lambda implementing a JDK interface). The annotation's own javadoc carries a self-contained "Checker contract" section (trigger / not-a-trigger / violation example / compliant example, both forms) written so this can be checked from the diff alone, with no other context needed. The underlying rule is "should", not "must" (RFC-2119 sense): a trigger is a presumptive finding, not an automatic failure — compliant if the declaration carries either a `// Retained on purpose: `-style comment, or `@SuppressPerfContract(value = NoEscape.class, reason = "...")` (`datadog.contract.SuppressPerfContract` — the generic exemption mechanism shared by every perf-contract marker, not `@NoEscape`-specific), or a named canned-exception annotation itself meta-annotated `@SuppressPerfContract` with `NoEscape.class` among its `value` (e.g. a hypothetical `@Borrowed`) — treat the meta-annotation's own `reason` as satisfying the requirement. Don't flag a `SuppressPerfContract`/canned-exception use as non-compliant just for being terse; that mirrors the annotation's own Checker contract. **flag-with-confidence** — SEV-2/3 (SEV-1 if the annotated type/method shares backing storage with something large, per its own javadoc). Current wearers: `SubSequence`, `Maybe` (type form; see J7 below for `SubSequence`'s specific retention-vs-transient discriminator). **No lint enforces this yet — the AI reviewer is the only check, so this stays here (not under deterministic-lint candidates below) until a checker lands and it can migrate down.** +9. **`@StaticLifetime` field-lifetime violation** — *a field annotated `@StaticLifetime` (`datadog.contract.StaticLifetime`), **or** an unannotated field whose declared type is listed in the contract's `@ImpliedFor` (`datadog.contract.ImpliedFor`; read the types from the `@ImpliedFor` on `StaticLifetime` itself, subtypes included), that is neither `static final` nor a `final` instance field declared on a class annotated `@Singleton` (`datadog.contract.Singleton`)*. Declaration-scan only, same shape as the `@NoEscape` field-storage check above — it does not attempt to prove a field is actually reused enough to be worth caching, and it does not catch an unannotated per-instance cache of a non-implied type; it only guards fields that opt in or whose type implies the contract. A deliberate per-instance holder of an implied type is compliant with `@SuppressPerfContract(value = StaticLifetime.class, reason = "...")`. `@Singleton`, applied to a class, is a trusted, unverified declaration (v1 posture — the same stance the other perf-contract markers take toward `static final` itself) that the class has exactly one process-wide instance; declaring it satisfies `@StaticLifetime` for that class's `final` instance fields. **flag-with-confidence** — SEV-2/3, motivated by a real production defect (a per-request-constructed cache that never actually amortized anything — see the annotation's own javadoc for the full trigger/accepted-form table). Refines the "per-call cache / expensive-object creation that should be static/once" deterministic-lint candidate below by giving that pattern's opted-in subset an explicit annotation and trigger; the unannotated general case is still that line's job. **No lint enforces this yet — the AI reviewer is the only check, so this stays here (not under deterministic-lint candidates below) until a checker lands and it can migrate down.** ## Deterministic-lint candidates (DON'T spend AI budget — make these real lints) Fixed-signature, mechanically checkable: diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 09e44b790e1..82b6d9e85f1 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -88,8 +88,7 @@ /internal-api/src/*/*/datadog/trace/api/endpoint/ @DataDog/apm-java /internal-api/src/*/*/datadog/trace/api/env/ @DataDog/apm-java /internal-api/src/*/*/datadog/trace/api/function/ @DataDog/apm-java -/internal-api/src/*/*/datadog/apicontract/ @DataDog/apm-java -/internal-api/src/*/*/datadog/perfcontract/ @DataDog/apm-java +/internal-api/src/*/*/datadog/contract/ @DataDog/apm-java /internal-api/src/*/*/datadog/trace/api/intake/ @DataDog/apm-java /internal-api/src/*/*/datadog/trace/api/logging/ @DataDog/apm-java /internal-api/src/*/*/datadog/trace/api/metrics/ @DataDog/apm-java diff --git a/communication/src/main/java/datadog/communication/serialization/msgpack/MsgPackWriter.java b/communication/src/main/java/datadog/communication/serialization/msgpack/MsgPackWriter.java index 5044877f1cf..d256f2ec0d7 100644 --- a/communication/src/main/java/datadog/communication/serialization/msgpack/MsgPackWriter.java +++ b/communication/src/main/java/datadog/communication/serialization/msgpack/MsgPackWriter.java @@ -8,8 +8,8 @@ import datadog.communication.serialization.StreamingBuffer; import datadog.communication.serialization.ValueWriter; import datadog.communication.serialization.WritableFormatter; -import datadog.perfcontract.StaticLifetime; -import datadog.perfcontract.SuppressPerfContract; +import datadog.contract.StaticLifetime; +import datadog.contract.SuppressPerfContract; import datadog.trace.bootstrap.instrumentation.api.UTF8BytesString; import java.nio.BufferOverflowException; import java.nio.ByteBuffer; diff --git a/gradle/forbiddenApiFilters/instrumentation.txt b/gradle/forbiddenApiFilters/instrumentation.txt index 5ffb1f7868d..12ad41d1de6 100644 --- a/gradle/forbiddenApiFilters/instrumentation.txt +++ b/gradle/forbiddenApiFilters/instrumentation.txt @@ -4,6 +4,6 @@ datadog.trace.bootstrap.instrumentation.api.AgentSpan#setServiceName(java.lang.String) # TagMap.internals() skips checks the tracer core has already made; it is @Restricted to the core -# (see datadog.apicontract.Restricted). Instrumentation uses TagMap's checked methods. +# (see datadog.contract.Restricted). Instrumentation uses TagMap's checked methods. @defaultMessage TagMap.internals() is restricted to the tracer core; use TagMap's checked methods instead. datadog.trace.api.TagMap#internals() diff --git a/internal-api/src/main/java/datadog/perfcontract/ImpliedFor.java b/internal-api/src/main/java/datadog/contract/ImpliedFor.java similarity index 98% rename from internal-api/src/main/java/datadog/perfcontract/ImpliedFor.java rename to internal-api/src/main/java/datadog/contract/ImpliedFor.java index 77619accee6..69515ef7b55 100644 --- a/internal-api/src/main/java/datadog/perfcontract/ImpliedFor.java +++ b/internal-api/src/main/java/datadog/contract/ImpliedFor.java @@ -1,4 +1,4 @@ -package datadog.perfcontract; +package datadog.contract; import java.lang.annotation.Documented; import java.lang.annotation.ElementType; diff --git a/internal-api/src/main/java/datadog/perfcontract/PerfContract.java b/internal-api/src/main/java/datadog/contract/PerfContract.java similarity index 96% rename from internal-api/src/main/java/datadog/perfcontract/PerfContract.java rename to internal-api/src/main/java/datadog/contract/PerfContract.java index 46fb99a3a21..23eb345a586 100644 --- a/internal-api/src/main/java/datadog/perfcontract/PerfContract.java +++ b/internal-api/src/main/java/datadog/contract/PerfContract.java @@ -1,4 +1,4 @@ -package datadog.perfcontract; +package datadog.contract; import datadog.trace.api.function.NoEscape; import java.lang.annotation.Documented; diff --git a/internal-api/src/main/java/datadog/apicontract/Restricted.java b/internal-api/src/main/java/datadog/contract/Restricted.java similarity index 97% rename from internal-api/src/main/java/datadog/apicontract/Restricted.java rename to internal-api/src/main/java/datadog/contract/Restricted.java index ab0372fa06f..20a8b9f0a67 100644 --- a/internal-api/src/main/java/datadog/apicontract/Restricted.java +++ b/internal-api/src/main/java/datadog/contract/Restricted.java @@ -1,4 +1,4 @@ -package datadog.apicontract; +package datadog.contract; import java.lang.annotation.Documented; import java.lang.annotation.ElementType; diff --git a/internal-api/src/main/java/datadog/perfcontract/Singleton.java b/internal-api/src/main/java/datadog/contract/Singleton.java similarity index 97% rename from internal-api/src/main/java/datadog/perfcontract/Singleton.java rename to internal-api/src/main/java/datadog/contract/Singleton.java index 368f18a7b1b..5bf20159343 100644 --- a/internal-api/src/main/java/datadog/perfcontract/Singleton.java +++ b/internal-api/src/main/java/datadog/contract/Singleton.java @@ -1,4 +1,4 @@ -package datadog.perfcontract; +package datadog.contract; import java.lang.annotation.Documented; import java.lang.annotation.ElementType; diff --git a/internal-api/src/main/java/datadog/perfcontract/StaticLifetime.java b/internal-api/src/main/java/datadog/contract/StaticLifetime.java similarity index 99% rename from internal-api/src/main/java/datadog/perfcontract/StaticLifetime.java rename to internal-api/src/main/java/datadog/contract/StaticLifetime.java index 47c3814b8c2..d4bf2d94fda 100644 --- a/internal-api/src/main/java/datadog/perfcontract/StaticLifetime.java +++ b/internal-api/src/main/java/datadog/contract/StaticLifetime.java @@ -1,4 +1,4 @@ -package datadog.perfcontract; +package datadog.contract; import java.lang.annotation.Documented; import java.lang.annotation.ElementType; diff --git a/internal-api/src/main/java/datadog/perfcontract/SuppressPerfContract.java b/internal-api/src/main/java/datadog/contract/SuppressPerfContract.java similarity index 98% rename from internal-api/src/main/java/datadog/perfcontract/SuppressPerfContract.java rename to internal-api/src/main/java/datadog/contract/SuppressPerfContract.java index 1c255854c20..ba5937bd230 100644 --- a/internal-api/src/main/java/datadog/perfcontract/SuppressPerfContract.java +++ b/internal-api/src/main/java/datadog/contract/SuppressPerfContract.java @@ -1,4 +1,4 @@ -package datadog.perfcontract; +package datadog.contract; import java.lang.annotation.Annotation; import java.lang.annotation.Documented; diff --git a/internal-api/src/main/java/datadog/trace/api/TagMap.java b/internal-api/src/main/java/datadog/trace/api/TagMap.java index be07fadbeeb..368da589add 100644 --- a/internal-api/src/main/java/datadog/trace/api/TagMap.java +++ b/internal-api/src/main/java/datadog/trace/api/TagMap.java @@ -1,6 +1,6 @@ package datadog.trace.api; -import datadog.apicontract.Restricted; +import datadog.contract.Restricted; import datadog.trace.api.function.NoEscape; import datadog.trace.api.function.TriConsumer; import datadog.trace.api.internal.VisibleForTesting; diff --git a/internal-api/src/main/java/datadog/trace/api/function/NoEscape.java b/internal-api/src/main/java/datadog/trace/api/function/NoEscape.java index 969420b598a..ed32fcffccc 100644 --- a/internal-api/src/main/java/datadog/trace/api/function/NoEscape.java +++ b/internal-api/src/main/java/datadog/trace/api/function/NoEscape.java @@ -1,7 +1,7 @@ package datadog.trace.api.function; -import datadog.perfcontract.PerfContract; -import datadog.perfcontract.SuppressPerfContract; +import datadog.contract.PerfContract; +import datadog.contract.SuppressPerfContract; import java.lang.annotation.Documented; import java.lang.annotation.ElementType; import java.lang.annotation.Retention; diff --git a/internal-api/src/main/java/datadog/trace/api/function/Strategy.java b/internal-api/src/main/java/datadog/trace/api/function/Strategy.java index 8d9c62d973b..9f90ce7e440 100644 --- a/internal-api/src/main/java/datadog/trace/api/function/Strategy.java +++ b/internal-api/src/main/java/datadog/trace/api/function/Strategy.java @@ -1,6 +1,6 @@ package datadog.trace.api.function; -import datadog.perfcontract.PerfContract; +import datadog.contract.PerfContract; import java.lang.annotation.Documented; import java.lang.annotation.ElementType; import java.lang.annotation.Inherited; diff --git a/internal-api/src/main/java/datadog/trace/api/function/StrategyConsumer.java b/internal-api/src/main/java/datadog/trace/api/function/StrategyConsumer.java index fc232bf17ff..ae090cc98db 100644 --- a/internal-api/src/main/java/datadog/trace/api/function/StrategyConsumer.java +++ b/internal-api/src/main/java/datadog/trace/api/function/StrategyConsumer.java @@ -1,6 +1,6 @@ package datadog.trace.api.function; -import datadog.perfcontract.PerfContract; +import datadog.contract.PerfContract; import java.lang.annotation.Documented; import java.lang.annotation.ElementType; import java.lang.annotation.Retention;