-
Notifications
You must be signed in to change notification settings - Fork 357
Add dd-apm-sdk-review skill and repo overrides, replace perf-review #12364
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
robertomonteromiguel
wants to merge
7
commits into
master
Choose a base branch
from
robertomonteromiguel/dd-apm-sdk-review-core-overrides
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+1,590
−650
Open
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
1604db5
Add dd-apm-sdk-review skill and repo overrides, replace perf-review
robertomonteromiguel 636abed
Merge branch 'master' into robertomonteromiguel/dd-apm-sdk-review-cor…
bric3 11fadad
llm validation gate (#12409)
robertomonteromiguel 02ec6ec
Merge branch 'master' into robertomonteromiguel/dd-apm-sdk-review-cor…
robertomonteromiguel 8b1be1c
fix review comments
robertomonteromiguel 151795a
review comments
robertomonteromiguel 1fccb97
Teach GitHub Codex to follow dd-apm-sdk-review without invoking the s…
robertomonteromiguel File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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. | ||
56 changes: 56 additions & 0 deletions
56
.agents/dd-apm-sdk-review-overrides/reviewers/conventions.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 (<reason>)` 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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). |
7 changes: 7 additions & 0 deletions
7
.agents/dd-apm-sdk-review-overrides/reviewers/maintainability.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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. |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.