diff --git a/.agents/skills/techdebt/SKILL.md b/.agents/skills/techdebt/SKILL.md index 6929e6e2240..f371490b9e3 100644 --- a/.agents/skills/techdebt/SKILL.md +++ b/.agents/skills/techdebt/SKILL.md @@ -1,6 +1,12 @@ --- name: techdebt -description: Analyze branch changes for technical debt, code duplication, and unnecessary complexity +description: >- + Review a code diff / branch / PR for technical debt — code duplication, + unnecessary complexity / over-engineering, and redundant or dead code. Use + whenever the user wants a tech-debt, cleanup, or refactor review, asks to check + a branch or PR for duplication / complexity / dead code before opening a PR, or + mentions "techdebt". Refactor-only: it reports issues and offers + behavior-preserving fixes; it never changes behavior. user-invocable: true context: fork allowed-tools: diff --git a/.claude/CLAUDE.md b/.claude/CLAUDE.md index 22cd857294d..330db3d7fcc 100644 --- a/.claude/CLAUDE.md +++ b/.claude/CLAUDE.md @@ -2,6 +2,6 @@ ## Claude Code workflow -- Before creating a pull request, run `/techdebt` to check for technical debt, code duplication, and unnecessary complexity in the branch changes. +- Before creating a pull request, run the [Review Guidelines](../AGENTS.md#review-guidelines) checks over the branch changes. - After running `/migrate-groovy-to-java`, run `/review-groovy-migration` on the migrated files before opening a PR. - Always write new unit tests using JUnit 5 and Java. If the target test file is `.groovy`, run the `/migrate-groovy-to-java` skill on it first. diff --git a/AGENTS.md b/AGENTS.md index 0b6114c8c21..d9852d96b43 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -38,6 +38,7 @@ docs/ Developer documentation (see below) | Testing guide (6 test types) | [docs/how_to_test.md](docs/how_to_test.md) | | Working with Gradle | [docs/how_to_work_with_gradle.md](docs/how_to_work_with_gradle.md) | | Bootstrap/premain constraints | [docs/bootstrap_design_guidelines.md](docs/bootstrap_design_guidelines.md) | +| Instrumentation/advice constraints | [docs/instrumentation_design_guidelines.md](docs/instrumentation_design_guidelines.md) | | CI/CD workflows | [.github/workflows/README.md](.github/workflows/README.md) | **When working on a topic above, read the linked file first** — they are the source of truth maintained by humans. @@ -56,14 +57,12 @@ docs/ Developer documentation (see below) ## Code conventions - **Formatting**: google-java-format enforced via Spotless. Run `./gradlew spotlessApply` before committing. -- **Static imports**: Prefer static imports over class-qualified calls for call-style helpers — JUnit `Assertions`, Mockito (`mock`, `when`, `verify`, `anyString`, `RETURNS_DEFAULTS`, ...), Hamcrest/AssertJ matchers, internal test DSLs, and similar. Same goes for production code: `Collections.emptyList()` is fine, but if you find yourself repeatedly writing `Foo.bar(...)` where `Foo` adds no information at the call site, static-import `bar`. Wildcard `import static x.*` is disallowed (enforced by IDE config in CONTRIBUTING.md). +- **Static imports**: Prefer static imports over class-qualified calls for call-style helpers, in both test (Assertions.assertEquals, Mockito.mock) and production code (Collections.emptyList). Wildcard imports disallowed — see CONTRIBUTING.md. - **Instrumentation layout**: `dd-java-agent/instrumentation/{framework}/{framework}-{minVersion}/` - **Instrumentation pattern**: Type matching → Method matching → Advice class (bytecode advice, not AOP) - **Test frameworks**: Always use JUnit 5 for unit tests. Only use Groovy / Spock tests for instrumentation and smoke tests. - **Forked tests**: Use `ForkedTest` suffix when tests need a separate JVM - **Flaky tests**: Annotate with `@Flaky` — they are skipped in CI by default -- **Instrumentation one-shot methods**: Never extract the return values of `triggerClasses()`, `contextStore()`, `classLoaderMatcher()`, or `methodAdvice()` into static constants. These are called once by the framework — extracting to a constant adds constant-pool bloat with no benefit. -- **Scope lifecycle order**: Keep the scope open until all work that needs the current active span is done — decorator calls, async callback registration, etc. Required order: decorator calls and callback registration, then `scope.close()`, then `span.finish()`. ## PR conventions @@ -74,23 +73,10 @@ docs/ Developer documentation (see below) ## Review Guidelines -Before marking a PR ready, run the applicable reviews below over the branch changes (the diff since the merge-base with `master`). These are **advisory, precision-first** checks — they surface high-severity issues early and are not merge gates. The linked guidelines are the tool-agnostic source of truth. +- **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). -### Performance Review +## Critical constraints -Follow the performance-review guidelines in [.agents/skills/perf-review/SKILL.md](.agents/skills/perf-review/SKILL.md) — the do-no-harm / assume-hot posture, the hot-path checks (universal + Java J1–J11 + ByteBuddy-Advice idioms), and the confidence/severity model, with the detailed rubric in its `references/`. Scope: hot-path allocation, unbounded memory, repeated work, escaping objects, native-boundary crossings, and JVM-specific pitfalls. - -## Bootstrap constraints (critical) - -Code running in the agent's `premain` phase must **not** use: -- `java.util.logging.*` — locks in log manager before app configures it -- `java.nio.file.*` — triggers premature provider initialization -- `javax.management.*` — causes class loading issues - -See [docs/bootstrap_design_guidelines.md](docs/bootstrap_design_guidelines.md) for details and alternatives. - -## Advice constraints (critical) - -Advice methods (OnMethodEnter/OnMethodExit) are inlined into the instrumented class's bytecode. -Calling a static interface method there, such as Context.root() or Context.current(), can cause -a VerifyError at runtime — use Java8BytecodeBridge (rootContext, currentContext, etc.) instead. +- **Bootstrap**: never use `java.util.logging.*`, `java.nio.file.*`, or `javax.management.*` in `premain` code — see [docs/bootstrap_design_guidelines.md](docs/bootstrap_design_guidelines.md). +- **Advice**: never call a static interface method from advice — can cause a `VerifyError` — see [docs/instrumentation_design_guidelines.md](docs/instrumentation_design_guidelines.md). diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 4dde1824dd7..c023a09af5e 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -63,6 +63,16 @@ For IntelliJ IDEA, we suggest the following settings and plugin. * To run test in a specific JDK use the `testJvm` property, e.g. `-PtestJvm=11` * Install the [Google Java Format](https://plugins.jetbrains.com/plugin/8527-google-java-format) plugin +### Static imports + +Prefer static imports over class-qualified calls for call-style helpers — JUnit `Assertions`, Mockito (`mock`, +`when`, `verify`, `anyString`, `RETURNS_DEFAULTS`, ...), Hamcrest/AssertJ matchers, internal test DSLs, and similar. + +Same goes for production code: `Collections.emptyList()` is fine, but if you find yourself repeatedly writing +`Foo.bar(...)` where `Foo` adds no information at the call site, static-import `bar`. + +Wildcard imports (`import x.*` or `import static x.*`) are disallowed — see the IntelliJ IDEA settings above. + ### Troubleshooting * Gradle fails with a "too many open files" error. diff --git a/docs/instrumentation_design_guidelines.md b/docs/instrumentation_design_guidelines.md new file mode 100644 index 00000000000..305ff651439 --- /dev/null +++ b/docs/instrumentation_design_guidelines.md @@ -0,0 +1,83 @@ +# Instrumentation and Advice Design Guidelines + +This document outlines design constraints and best practices when writing or refactoring instrumentation and advice code. +Following these guidelines will help avoid constant-pool bloat and subtle span/scope lifecycle bugs. + +## Background + +Instrumentation classes wire up type/method matchers and advice once, when the framework loads them. Advice methods +are inlined into the instrumented application's bytecode and run on every invocation of the matched method, so their +ordering directly affects when spans are visible to nested work. + +## Constraints to Follow + +### 1. Instrumentation one-shot methods + +**Why to avoid extracting to constants:** + +- `triggerClasses()`, `contextStore()`, `classLoaderMatcher()`, and `methodAdvice()` are called exactly once by the + framework when the instrumentation is registered +- Extracting their return values into static constants adds constant-pool bloat with no runtime benefit + +**What to do instead:** + +Return the values directly from the method; don't cache them in a field. + +```java +// BAD - unnecessary static constant +private static final ElementMatcher.Junction CL_MATCHER = hasClassesNamed("foo.Bar"); + +@Override +public ElementMatcher.Junction classLoaderMatcher() { + return CL_MATCHER; +} + +// GOOD - construct directly in the one-shot method +@Override +public ElementMatcher.Junction classLoaderMatcher() { + return hasClassesNamed("foo.Bar"); +} +``` + +### 2. Scope lifecycle order + +**Why to avoid closing the scope early:** + +- Keeping the scope open until all work needing the current active span is done ensures decorator calls and async + callback registration see the span as active +- Closing the scope before that work runs can make nested code lose track of the current span + +**What to do instead:** + +Required order: decorator calls and callback registration, then `scope.close()`, then `span.finish()`. + +```java +// GOOD +DECORATE.onOperation(span, request); +registerCallback(span); +scope.close(); +span.finish(); +``` + +### 3. Static interface methods in advice + +**Why to avoid calling them directly:** + +- Advice methods (`@Advice.OnMethodEnter`/`@Advice.OnMethodExit`) are inlined into the instrumented class's bytecode +- Calling a static interface method there, such as `Context.current()` or `Context.root()`, can cause a + `VerifyError` at runtime + +**What to use instead:** + +Use `Java8BytecodeBridge` (`currentContext()`, `rootContext()`, etc.) in place of the static interface method, +static-imported for readability. + +```java +import static datadog.trace.bootstrap.instrumentation.api.Java8BytecodeBridge.currentContext; + +// BAD - static interface method call, can cause VerifyError when inlined +Context ctx = Context.current(); + +// GOOD - use the bytecode bridge, static-imported +Context ctx = currentContext(); +```