Infer instrumentation helper classes at build time - #12059
Conversation
This comment has been minimized.
This comment has been minimized.
98edf9d to
a09ff60
Compare
🟢 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. |
29b0bff to
a8cdba9
Compare
| * 1. Must implement [net.bytebuddy.build.Plugin] | ||
| * 2. Must have a constructor accepting a [java.io.File] parameter (target directory) | ||
| * 2. Must have a constructor accepting a [java.io.File] (target directory), or a two-`File` | ||
| * `(sourceDirectory, targetDirectory)` constructor when the plugin also needs the source folder |
There was a problem hiding this comment.
sourceDirectory is needed to determine whether it was the particular submodule that compiled the className (done in isOwnOutput(className) - if yes then inject).
There was a problem hiding this comment.
💭 thought: The fact that target moved from the 1st position to the 2nd position make the implementation weird. Can’t we infer isOwnOutput() some otherway?
There was a problem hiding this comment.
You should be able to compute the source directory using the TypeDefinition and the ClassLoader.
You can load the module as URL and infer its source folder for example.
There was a problem hiding this comment.
Good point!! Addressed this in 9139e4e by getting the module's codeSource location.
| } | ||
|
|
||
| /** | ||
| * Same as above, but reads bytecode via the passed locator (e.g. during build time when the agent |
There was a problem hiding this comment.
MuzzleGenerator needs to call HelperScanner.withClassDependencies at build-time to expand and order the helpers found, but there's no AgentClassLoader during the build (there is during runtime which is previously the only place we called HelperScanner.withClassDependencies) - so we need to pass in the build classpath's locator to use.
|
Hi! 👋 Thanks for your pull request! 🎉 To help us review it, please make sure to:
If you need help, please check our contributing guidelines. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9cea7a8021
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
More details
Generated methods preserved all four migrated instrumentations’ former helper sets and correctly handled dependency ordering, nested helpers, and unresolved-class fallback. Runtime instrumentation also loaded the generated helpers successfully for locally executable requests.
📊 Validated against 10 scenarios · Open Bits AI session
🤖 Datadog Autotest · Commit 9cea7a8 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
PerfectSlayer
left a comment
There was a problem hiding this comment.
I start reviewing and will drop few inline comments for now.
📝 notes: The plugin is enabled for all instrumentations so it will apply to all instrumentations that do not explicitly override the helper declaration method, not only the few that are migrated as demo.
| } | ||
|
|
||
| String[] orderedHelpers = | ||
| computeInjectedHelpers(module, allReferences, adviceClasses, sourceRoot); |
There was a problem hiding this comment.
I would try to separate the helper computation from the muzzle generator since the two have different roles. The common logic to discover references in muzzle can still be used but the helper computation can go in a different gradle plugin
There was a problem hiding this comment.
I experimented with a separate gradle plugin, but found that I'd need to either call ReferenceCreator again and crawl through the advice classes a second time or create some sort of per-module shared cache where the plugin could store/access results... WDYT about this follow-up PR instead that separates out the logic but keeps one pass? #12191
There was a problem hiding this comment.
I had the same feelings... Mixing both concerns does not feel right.
So it could be two different plugin, or a common umbrella (no longer the Muzzle plugin) that would do the parsing with two outcomes (Muzzle and Helper).
I could be nice to ask to @mcculls about design expectations now he’s back.
There was a problem hiding this comment.
If you want to avoid multiple passes then I would go with a generic plugin that records references (reachability, ordering, etc.) and then have the muzzle and helper plugins use that recorded data.
Or have the muzzle and helper logic as components that plug into this generic plugin. Avoid mixing muzzle and helper code in the same class though, as a fix/change to one could easily break the other.
BTW, worth taking a look at HelperScanner - we use this to expand the list of helpers to pull in all referenced classes for GraalVM (rather than just the listed helpers) and it has logic to help order the loading of classes by how they reference each other which you might find useful when doing similar ordering at build time.
| */ | ||
| public final class HelperClassPredicate { | ||
|
|
||
| static final String[] HELPER_PREFIXES = { |
| * @param ownOutput tests whether a class name was compiled by the instrumentation subproject | ||
| * itself; injected so this classifier stays independent of the build directory layout. | ||
| */ | ||
| public HelperClassPredicate(final Predicate<String> ownOutput) { |
There was a problem hiding this comment.
FWIW I found ownOutput a bit confusing - it feels like this could be a separate pruning step (if we go with having a generic advice scanner + different processors)
What Does This Do
This PR adds logic to generate each
InstrumenterModule'shelperClassNames()at build-time instead of the existing logic that requires each module's helper classes to be manually listed. TheMuzzleGeneratoralready crawls through each module's advice in order to gather its references and emit these in a$Muzzleside-class. We simultaneously use this crawl to infer the helperClasses each module needs to load, order them dependency-first, drop build-time-only classes, and write these helperClasses directly into the module'shelperClassNames()bytecode.If a module manually declares its
helperClassNames(), we use that list instead of inferring.Also, this PR migrates a few instrumentations from manual declarations of required helper classes to auto-detecting these classes in order to test the new methodology.
Motivation
Currently, instrumentation helper classes are manually listed for each
InstrumenterModule. However, we can instead use Muzzle, which already finds class references for each instrumentation, to automatically discover which helper classes need to be loaded per instrumentation. This is inspired by OTel's muzzle, which generatesgetMuzzleHelperClassNames()from their single build-time advice crawl.Additional Notes
The intention is for all instrumentations to discover required helper classes at build time; however, I wanted to keep the scope of this PR low so that it's easier to review. The rest of the instrumentations will be migrated in follow-up PRs.
A follow-up PR #12191 also extracts the helper resolver logic to its own class so that the logic is separate from muzzle reference generation and more easily testable and legible.
Contributor 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: [PROJ-IDENT]