Trim AGENTS.md of duplicate content#12003
Conversation
mcculls
added
type: documentation
tag: no release notes
|
@codex review |
![datadog-datadog-prod-us1-2[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
More details
Documentation refactoring that consolidates duplicate content from AGENTS.md into dedicated, detailed guideline files. All critical constraints (bootstrap, instrumentation/advice) are preserved with enhanced explanations and code examples. Referential integrity verified — all links resolve correctly.
📊 Validated against 5 scenarios · Open Bits AI session
🤖 Datadog Autotest · Commit 8832d8c · What is Autotest? · Any feedback? Reach out in #autotest
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
There was a problem hiding this comment.
Pull request overview
This PR trims duplicated guidance from AGENTS.md by moving instrumentation/advice-specific constraints into a dedicated documentation page, while keeping concise links and critical reminders in AGENTS.md. It also centralizes the static-import guidance in CONTRIBUTING.md so the main contributor doc remains the source of truth.
Changes:
- Added a new
docs/instrumentation_design_guidelines.mddocument capturing key advice/instrumentation constraints and examples. - Updated
AGENTS.mdto link to the new instrumentation/advice guidelines and to reduce duplicated sections (static imports, perf review, and critical constraints). - Added an explicit “Static imports” section to
CONTRIBUTING.mdto serve as the primary reference.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| docs/instrumentation_design_guidelines.md | New focused doc for instrumentation/advice constraints (one-shot methods, scope lifecycle ordering, static interface methods in advice). |
| CONTRIBUTING.md | Adds a dedicated static-imports section to make CONTRIBUTING the canonical reference. |
| AGENTS.md | Removes duplicated details, adds links to the new guidelines, and keeps critical constraints as concise bullets. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
bm1549
left a comment
bm1549
left a comment
There was a problem hiding this comment.
Most of my questions are if we can convert some of the prose into static analysis
Even if the static checks are hacky, it pays dividends in token usage since it lets us shorten the instructions for LLMs while providing stronger guarantees
| 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). |
There was a problem hiding this comment.
[optional suggestion] Is it possible to have this as static analysis, rather than prose? If so, we can spend the tokens on this up-front rather than having every agent need to read this and enforce it every time
There was a problem hiding this comment.
The tricky part is identifying advice classes which must avoid static interface methods, vs helpers which don't need to (where doing that would just add fluff) - it's basically methods annotated with @Advice.OnMethodEnter / @Advice.OnMethodExit but we'd need to roll our own analyzer to filter on that ... or expand instrumentation-annotation-processor to also dig into the advice bytecode.
That's possible but that also would just add a guard - it's still helpful to flag the existence of Java8BytecodeBridge and guide the agent to do the right thing to begin with.
There was a problem hiding this comment.
Depending on how prevalent it is through the codebase, the analyzer could either run everywhere and be suppressed in the locations where it's safe or we could make the finer-grained targeting you're describing
As for guarding vs guiding, you can do both but as the models evolve, guarding tends to be the piece that survives while the guiding bits can be deleted
There was a problem hiding this comment.
Basically I've had agents introduce static interface methods a lot during recent refactorings and it's been annoying to remind them not to do this - a guard would stop them from reaching a reviewable PR, but you'd still have that loop of a) initial refactor b) fails build check c) find alternative (Java8BytecodeBridge) d) fix code e) pass build check
There was a problem hiding this comment.
Yep that's correct, so having both would be the belt-and-suspenders so to say
There was a problem hiding this comment.
I do agree with this, generally I imagine this as a funnel where we generally:
- use skills as a catch-all for advice/ documentation, there will be items that only apply to specific situations where an LLM can figure out when theyre relevant
- certain parts we can figure out where to encode as general static checks
- we generally move from skill -> static checks as we refine the skills/ optimize
There was a problem hiding this comment.
Agree, will look into this as a follow-up item
|
|
||
| - **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. |
There was a problem hiding this comment.
Wildcard imports disallowed
Isn't this already part of the formatter? What reason is it here as well?
There was a problem hiding this comment.
The formatter will detect use of wildcard imports, but it won't tell you to avoid continually writing Collections.emptyList() tens of times in the same file and instead prefer to static import Collections.emptyList and then use emptyList() everywhere.
There was a problem hiding this comment.
So we can drop the bit about wildcard imports, but we still need the rest
There was a problem hiding this comment.
The formatter will detect use of wildcard imports, but it won't tell you to avoid continually writing Collections.emptyList() tens of times in the same file and instead prefer to static import Collections.emptyList and then use emptyList() everywhere.
Is there a benefit to doing this or is this just personal preference for the codebase?
There was a problem hiding this comment.
Helps readability - we've found agents don't use static imports as much, and that can make the code harder to read - especially for APIs that were designed to be used with static imports like JUnit/Mockito
There was a problem hiding this comment.
Not blocking, but is there any path to doing these as static analysis as well? Said differently, is there a big downside to a static import for a single emptyList()?
There was a problem hiding this comment.
Will look into that as a follow-up item
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
jordan-wong
left a comment
jordan-wong
left a comment
There was a problem hiding this comment.
- no notes on content, LGTM
- In future it would be nice to consider the idea of seeing what we can move to static checks, which will help to optimize skills (slimming down to not overload context). There is a point that we want to give context to agents AS it's generating code, not after, I imagine this requires some nuance per case (some things are harder for agents to conform to when it's just in skill vs static check)
PerfectSlayer
left a comment
PerfectSlayer
left a comment
There was a problem hiding this comment.
Approving. I would not push for static analyzer in this PR as the goal was to reduce the size of agents.md
I agree that's an interesting idea but I would rather see the agents file addressed, and improve checker later.
This comment has been minimized.
This comment has been minimized.
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
c7901a5
into
master
Motivation
Reduces
AGENTS.mdby 25% while still keeping critical advice close byContributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issueJira ticket: APMLP-1609