Add @NoSyntheticAccessors and remove CoreTracer's synthetic accessors (AI perf review)

[dougqh]

What Does This Do

Adds a @NoSyntheticAccessors perf contract, a test helper that enforces it, and applies it to CoreTracer.

• @NoSyntheticAccessors (datadog.perfcontract): on a top-level class, declares that no class in its nest needs a synthetic accessor. The nest is the class plus everything nested in it, including anonymous and local classes. The rule covers the whole nest because the accessor lands in the class that owns the private member, while the cause is the class that uses it.
• SyntheticAccessors.assertNone(Class) (utils/test-utils): fails if any class in the nest has an unsuppressed synthetic access$* method or synthetic constructor, and names the member each one reaches. It reads the nest's class files with ASM (Byte Buddy's bundled copy, already a test-utils dependency), since only the bytecode says what an accessor reaches and @SuppressPerfContract has CLASS retention. It finds anonymous and local classes by listing the TopLevel$* class files beside the top-level class. Lambdas and bridge methods aren't counted.
• Suppression: an accessor is exempt when the member it reaches carries @SuppressPerfContract(value = NoSyntheticAccessors.class, reason = ...), directly or through a reusable suppression annotation. That lets a member used across the nest only off the hot path stay private, with the reason at the declaration. A blank reason, or a suppression that doesn't name this contract, is reported.
• CoreTracer is the first class marked. Its nest had 14 access$ methods and 2 synthetic constructors. Most came from CoreSpanBuilder.buildSpanContext, a static method that read 12 of CoreTracer's private fields (idGenerationStrategy, propagationTagsFactory, localRootSpanTags and others) through accessors on every buildSpan(). It now lives in CoreTracer as a package-private instance method, so those fields are read directly. The move is mechanical: the tracer parameter becomes this. The remaining cross-nest members are cold (the constructor, invertMap, ShutdownHook's constructor, and the three fields ConfigSnapshot reads), so they stay private with suppressions. Compared with master, no CoreTracer member is opened.
• perf-review: checks.md lists this under deterministic-lint candidates as already checked, so the AI reviewer doesn't spend effort on it.

Motivation

We compile to Java 8 bytecode, which has no nestmates (JEP 181 arrived in JDK 11). So when one class in a nest uses another's private member, javac generates a static access$NNN method and routes the call through it. A private constructor called across the nest also gets a synthetic constructor with an extra parameter typed as a generated class.

Each accessor is a real call in the interpreter and C1. C2 inlines them as trivial methods, but each one uses up an inlining level, so on a deep hot chain it can push the real callee past MaxInlineLevel. Accessors also add methods and classes to load at startup.

Accessors also hide coupling: a nested class can reach into its outer class's private state and nothing in the source shows it. buildSpanContext had grown into a 290-line method in the builder that read 12 private CoreTracer fields; the contract turns that kind of reach into a test failure.

Unlike the other perf contracts, this one is fully mechanical, so it gets a deterministic check rather than an AI-review rule.

Additional Notes

• One test per marked class. The annotation has CLASS retention like the other perf contracts, so reflection can't discover marked classes. Each marked class gets a one-line test calling assertNone. A build-time scan for the annotation could replace that later.
• Suppress only cold members. A suppression keeps the accessor, so it belongs on members used across the nest during setup, reloads or shutdown, not on hot paths.
• Other candidates, by accessor count in dd-trace-core and internal-api: TracerHealthMetrics (58), LLMObsSpanMapper (24), DefaultDataStreamsMonitoring (19), PendingTraceBuffer$DelayingPendingTraceBuffer (13). Which of those are on hot paths still needs checking.

Contributor Checklist

• :utils:test-utils:test (SyntheticAccessorsTest: detects both accessor kinds and names their targets, ignores package-private members and lambdas, honours direct and meta-annotation suppressions, and rejects a blank reason or a suppression of another contract)
• Removing one of CoreTracer's suppressions fails CoreTracerSyntheticAccessorsTest with CoreTracer.access$000 -> CoreTracer.invertMap
• :dd-trace-core:test, full suite after the buildSpanContext move (4,710 tests; the only failures were a timing flake that passes on rerun and two Docker-dependent tests with no local Docker). The new test fails on master with all 16 accessors listed
• :internal-api:javadoc, :utils:test-utils:javadoc, spotlessApply

🤖 Generated with Claude Code

[dd-octo-sts]

🟢 Java Benchmark SLOs — All performance SLOs passed

Suite	Status
Startup	🟢 pass

SLO thresholds are defined here based on automatically generated metrics. A warning is raised when results are within 5% of the threshold.

PR vs. master results

Scenario	Candidate	master	Δ (95% CI of mean)
startup:insecure-bank:iast:Agent	14.73 s	14.59 s	[+0.1%; +1.9%] (maybe worse)
startup:insecure-bank:tracing:Agent	13.49 s	13.64 s	[-1.9%; -0.3%] (maybe better)
startup:petclinic:appsec:Agent	17.67 s	17.56 s	[-0.3%; +1.5%] (no difference)
startup:petclinic:iast:Agent	17.37 s	17.60 s	[-2.1%; -0.5%] (maybe better)
startup:petclinic:profiling:Agent	17.03 s	17.23 s	[-2.3%; -0.1%] (maybe better)
startup:petclinic:sca:Agent	17.61 s	17.51 s	[-0.1%; +1.3%] (no difference)
startup:petclinic:tracing:Agent	16.17 s	16.30 s	[-6.9%; +5.3%] (unstable)

Commit: 07e5191f · CI Pipeline · Benchmarking Platform UI

---

Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion.

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-13T21:51:48.571437Z	52b997b	Draft marked ready

ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[datadog-datadog-us1-prod]

The new tests for SyntheticAccessors cover only directory-backed fixtures, leaving the JAR scanner and error branches uncovered. Because the module enforces a mandatory 90% per-class branch and instruction coverage threshold with no exemption for this helper, baseCheck/baseTest -PcheckCoverage will fail until the missing paths are covered or an explicit exclusion is added.

_{Open Bits AI session}

_{🤖 Bits Code Review · Commit 52b997b}

[PerfectSlayer]

I would rather not open core tracer fields / encapsulation for performance reason.

[dougqh]

I would rather not open core tracer fields / encapsulation for performance reason.

@PerfectSlayer I understand, but we're not really opening encapsulation to dd-trace-java as a whole. We're opening just to the one package -- not public.

We already do that for testing purposes. I'd argue that an end user performance gain is a stronger justification.
Unfortunately while small synthetic accessors eat into the inlining depth limit which then interferes with other optimizations, so this is rather necessary.

And while I understand the desire to protect the core, the core is also the most important place performance wise, so we have to find a balance. Admittedly, we should be careful about what we open-up and how.

For the immutable fields, I'd prefer to keep them package visible to avoid eating into the inlining depth limit. However for mutable fields, I think we could create package visible helpers in many situations to maintain some encapsulation.

We could also scope this field-by-field, method-by-method, but I didn't do that because that makes it easy for someone to introduce a regression. I could also make this work with the SuppressPerfContract annotation - if we want to opt-out certain fields.

[dougqh]

@PerfectSlayer I reworked this so it no longer opens anything in CoreTracer. Compared with master, every member that was private is still private.

• The hot reads moved instead of the fields opening. Most of the accessors came from CoreSpanBuilder.buildSpanContext, a static method that read 12 private CoreTracer fields on every span. It's now a package-private instance method on CoreTracer. That's a mechanical move: the tracer parameter becomes this. The fields stay private and are read directly.
• The cold cross-nest members stay private with a documented suppression. The constructor, invertMap, ShutdownHook's constructor and the three fields ConfigSnapshot reads are only used across the nest during setup, reloads or shutdown. @SuppressPerfContract(value = NoSyntheticAccessors.class, reason = ...) on each one keeps it private and records why the accessor is acceptable. The checker reads the bytecode to match each accessor to the member it reaches, and rejects a blank reason.

So the contract doesn't push us toward weaker encapsulation. It flags when a nested class reaches into private state, and we either move the code next to the data or record that it's a deliberate, off-hot-path exception.

🤖 Generated with Claude Code