Move SpotBugs and forbidden-apis configuration to convention plugins by bric3 · Pull Request #12933 · DataDog/dd-trace-java

bric3 · 2026-10-19T14:45:24Z

What Does This Do

Move the shared SpotBugs and forbidden-apis configuration from Groovy scripts to Kotlin convention plugins in buildSrc.

Existing task selection, filters, detector exclusions, reports and launchers are preserved.

Motivation

Replace script application with typed, reusable conventions that declare their prerequisites and allow modules to customise the shared defaults.

The main motivation is to prepare support for #12816.

Additional Notes

The SpotBugs's dependency is Java 11 and need to be explicitly allowed, it's ok because the plugin runs in the Gradle daemon. buildSrc still target Java 8 for muzzle workers.
Root project didn't need SpotBugs, removing it also removes its lockfile.
SpotBugs was just a bit tweak to move default config to the spotbugs extension, only keeping task specific config.

Contributor Checklist

Format the title according to the contribution guidelines
Assign the type: and (comp: or inst:) labels in addition to any other useful labels
Avoid using close, fix, or any linking keywords when referencing an issue
Use solves instead, and assign the PR milestone to the issue
Update the CODEOWNERS file on source file addition, migration, or deletion
Update public documentation with any new configuration flags or behaviors
Once approved, use merge queue to merge the PR

In order, to anticipate the static analysis skill, let's migrate the spotless and forbidden api script plugins to convention plugins.

datadog-prod-us1-5 · 2026-10-19T15:12:36Z

🎯 Code Coverage (details)
• Patch Coverage: 100.00%
• Overall Coverage: 59.84% (-0.01%)

_{This comment will be updated automatically if new data arrives.

🔗 Commit SHA: b34800f | Docs | Give us feedback!}

chatgpt-codex-connector · 2026-10-19T15:18:47Z

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-19T15:23:01.780506Z	`15b256e`	Draft marked ready
🔒 Security Review	✅ Completed 2026-10-19T15:22:30.866283Z	`15b256e`	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-prod-us1-5

More details

The migrated conventions retain the existing task filters, detector exclusions, and module overrides. Gradle compilation and task execution remain unverified.

Was this helpful? React 👍 or 👎

_{Open Bits AI session}

_{🤖 Bits Code Review · Commit 15b256e}

dd-octo-sts · 2026-10-19T15:46:42Z

🟢 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	13.89 s	13.92 s	[-1.0%; +0.5%] (no difference)
startup:insecure-bank:tracing:Agent	12.82 s	12.91 s	[-1.6%; +0.3%] (no difference)
startup:petclinic:appsec:Agent	16.99 s	16.92 s	[-0.4%; +1.2%] (no difference)
startup:petclinic:iast:Agent	16.88 s	16.98 s	[-1.5%; +0.3%] (no difference)
startup:petclinic:profiling:Agent	16.54 s	16.75 s	[-2.0%; -0.4%] (maybe better)
startup:petclinic:sca:Agent	16.52 s	16.74 s	[-5.6%; +3.0%] (no difference)
startup:petclinic:tracing:Agent	16.02 s	16.03 s	[-1.2%; +1.1%] (no difference)

Commit: b34800f0 · 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.

dougqh

Looks behavior-preserving to me (ignoreFailures, omitted visitors, HTML report, smoke-test skip, Main/Main_java11 filter, otel-bootstrap override all carry over). A few comments inline — the Java 8 worker one is the only one I'd want settled before merge.

One more that I can't anchor inline since the file isn't in the diff: dd-trace-java.module.instrumentation.gradle.kts still applies de.thetaphi.forbiddenapis itself and re-declares the main.txt signatures path, even though the new forbidden-apis convention already arrives via java_no_deps. Could it rely on the convention and only add instrumentation.txt, so the main.txt config lives in one place?

dougqh · 2026-10-19T22:33:14Z

 dependencies {
  implementation(gradleApi())
  implementation(libs.forbiddenapis)
+  implementation(libs.spotbugs) {


Is the daemon really the only JVM this reaches? MuzzleTask.kt (~L147) notes that processIsolation leaks Gradle/buildSrc dependencies into the child process, and InstrumentPostProcessingAction.kt uses processIsolation too. If one of those workers runs on a Java 8 launcher, anything that scans the classpath / hits a ServiceLoader and touches the SpotBugs plugin (or its Java 11+ deps) would throw UnsupportedClassVersionError. Probably fine in practice, but a green muzzle run on a Java 8 launcher would confirm it.

You're right that process isolation inherits the work implementation's classpath. I removed that justification from the comment locally. Worker isolation is addressed separately in #12922. The existing MuzzleAction already calls the Java 9+ getPlatformClassLoader() before this change, so a green Java 8 Muzzle run remains outstanding.

dougqh · 2026-10-19T22:33:14Z

  implementation(libs.forbiddenapis)
+  implementation(libs.spotbugs) {
+    // buildSrc targets Java 8 for muzzle workers, but the SpotBugs plugin requires Java 11.
+    // This is fine because the spotbugs plugin runs in the Gradle Daemon JVM (Java 25).


nit: "Java 25" duplicates gradle/gradle-daemon-jvm.properties and will go stale on the next daemon bump. The actual requirement is just "daemon JVM ≥ 11".

Removed the daemon version in the comment. The goal of the comment is explain only why we select the plugin's Java 11 variant while buildSrc still targets Java 8.

dougqh · 2026-10-19T22:33:14Z

+  ignoreFailures = false
+  excludeFilter = rootProject.layout.projectDirectory.file("gradle/spotbugFilters/exclude.xml")
+  showProgress = !providers.environmentVariable("CI").isPresent
+  omitVisitors = listOf(


Subtle change: previously omitVisitors was set per-task, so modules couldn't override it. As an extension convention, a module writing spotbugs { omitVisitors = [...] } to add one detector would replace the shared list and silently re-enable things like FindNullDeref / MutableStaticFields. Worth either keeping it on the task, or adding a comment that overrides should append (omitVisitors.addAll(...)).

Indeed, I initially did that as an optimization, but having per source set configuration makes sense. I've restored the omitVisitors list on production tasks locally.
Also, currently no module overrides it.

Test analysis is currently disabled and inherits none of these omissions, I've kept it that way in this PR. But the code is now in a bit better place so the static analysis skill can later tune this for test tasks.

dougqh · 2026-10-19T22:33:14Z

+
+spotbugs {
+  ignoreFailures = false
+  excludeFilter = rootProject.layout.projectDirectory.file("gradle/spotbugFilters/exclude.xml")


nit: the forbidden-apis convention uses rootDir.resolve(...) for its root-relative file; this one goes through rootProject.layout. Would be nice to pick one — and avoiding rootProject from subprojects is friendlier to isolated projects.

Changed to rootDir.resolve(...) locally. I also checked this with a small isolated-projects build: rootProject.layout fails with a cross-project access violation, while rootDir.resolve(...) passes.

dougqh · 2026-10-19T22:33:14Z

+}
+
+tasks.withType<SpotBugsTask>().configureEach {
+  if (project.path.startsWith(":dd-smoke-tests") ||


nit: the :dd-smoke-tests check only depends on the project, but it's evaluated per SpotBugs task, and smoke-test modules still get the plugin/tasks configured just to disable them. Checking once at plugin-apply time (or not applying it under dd-smoke-tests) would be cheaper.

Could we keep the plugin applied for now? Removing it would change the available tasks and configurations. The when checks smoke-test projects first and disables their analysis; configureEach limits the prefix check to realised tasks. We haven't measured a meaningful cost from that check.

dougqh · 2026-10-19T22:33:14Z

+}
+
+dependencies {
+  compileOnly(versionCatalogs.named("libs").findLibrary("forbiddenapis").get())


nit: the spotbugs convention uses a val libs = versionCatalogs.named("libs") holder; this inlines it. Same pattern in both would keep the sibling conventions uniform.

I kept the local variable where the catalogue is used twice, and the inline lookup where it's used once. I'd prefer to leave these as they are unless we want a project-wide convention.

dougqh · 2026-10-19T22:33:14Z

 gradle-tooling-api = { module = "org.gradle:gradle-tooling-api", version.ref = "gradle-tooling-api" }
 jsr305 = { module = "com.google.code.findbugs:jsr305", version.ref = "jsr305" }
 spotbugs-annotations = { module = "com.github.spotbugs:spotbugs-annotations", version.ref = "spotbugs_annotations" }
+spotbugs = { module = "com.github.spotbugs.snom:spotbugs-gradle-plugin", version.ref = "spotbugs-gradle-plugin" }


nit: spotbugs here is the Gradle plugin, while the neighbouring spotbugs-annotations / spotbugs_annotations refer to the SpotBugs tool. Someone bumping libs.spotbugs expecting the analyzer would change the plugin. spotbugs-gradle-plugin as the alias would match the version key.

That's a fair point. I originally did that, but other plugins were not identified as such, so I kept the existing naming convention. I'll use the suggested name, but I will update the other names in a later PR.

Enabling SpotBugs on main_java17 exposed two rules: - AT_STALE_THREAD_WRITE_OF_PRIMITIVE: the send and receive buffer sizes are shared without synchronization. Make them volatile so readers observe updates to these independent settings. - NCR_NOT_PROPERLY_CHECKED_READ: read() assumes every result except -1 filled its byte. A zero-byte read on timeout fabricated a NUL byte. Wait for data, EOF or an exception. Throw SocketTimeoutException when the read timeout expires. Retry selector wakeups within the original timeout and stop interrupted reads so the retry loop cannot spin. Inspecting the delegated bulk read also exposed byte loss: it could consume more bytes from the channel than requested and discard the rest. Limit the channel read to the requested length and handle empty reads. Suppressing the rules would retain real visibility and stream-contract bugs. Add coverage for byte preservation, EOF, empty reads, timeout, selector wakeup and interruption while preserving asynchronous close.

Enabling SpotBugs on main_java21 exposed PA_PUBLIC_PRIMITIVE_ATTRIBUTE on TestEventsHandlerHolder.TEST_EVENTS_HANDLER. Only KarateTracingListener and ExecutionContext read this field. Both are helpers in the same package and are injected with the holder into the same class loader. Inlined advice does not access the field, and tests use the public start() and stop() methods. Make the field package-private. Public access is unnecessary, so reduce its visibility without a SuppressFBWarnings annotation or an additional accessor. Keep volatile publication unchanged.

datadog-prod-us1-5

More details

The convention plugins compiled successfully. Targeted execution checks remain unverified because required dependencies were not cached for offline use.

Was this helpful? React 👍 or 👎

_{Open Bits AI session}

_{🤖 Bits Code Review · Commit b34800f}

bric3 added tag: no release notes Changes to exclude from release notes type: refactoring comp: tooling Build & Tooling tag: ai generated Largely based on code generated by an AI or LLM labels Oct 19, 2026

bric3 added 2 commits October 19, 2026 17:05

refactor: Move spotbugs and spotless to convention plugins

d89ef3f

In order, to anticipate the static analysis skill, let's migrate the spotless and forbidden api script plugins to convention plugins.

refactor: Separate task level behavior from plugin conventin default

15b256e

bric3 mentioned this pull request Oct 19, 2026

Overhaul script plugins as convention plugins #12258

Open

bric3 force-pushed the bdu/spotbugs-forbidden-apis-conventions branch from 2dc368f to 15b256e Compare October 19, 2026 15:10

bric3 marked this pull request as ready for review October 19, 2026 15:18

bric3 requested a review from a team as a code owner October 19, 2026 15:18

bric3 requested review from dougqh and removed request for a team October 19, 2026 15:18

datadog-prod-us1-5 Bot reviewed Oct 19, 2026

View reviewed changes

dougqh reviewed Oct 19, 2026

View reviewed changes

refactor: Improve configurability of spotbugs task per sourceset

7c8a575

bric3 requested a review from dougqh October 20, 2026 12:24

bric3 added 2 commits October 20, 2026 16:43

bric3 requested review from a team as code owners October 20, 2026 14:51

bric3 requested review from jordan-wong and removed request for a team October 20, 2026 14:51

datadog-prod-us1-5 Bot reviewed Oct 20, 2026

View reviewed changes

Conversation

bric3 commented Oct 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What Does This Do

Motivation

Additional Notes

Contributor Checklist

Uh oh!

datadog-prod-us1-5 Bot commented Oct 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chatgpt-codex-connector Bot commented Oct 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codex Review Summary

Uh oh!

datadog-prod-us1-5 Bot left a comment

Choose a reason for hiding this comment

Uh oh!

dd-octo-sts Bot commented Oct 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🟢 Java Benchmark SLOs — All performance SLOs passed

Uh oh!

dougqh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

datadog-prod-us1-5 Bot left a comment

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bric3 commented Oct 19, 2026 •

edited

Loading

datadog-prod-us1-5 Bot commented Oct 19, 2026 •

edited

Loading

chatgpt-codex-connector Bot commented Oct 19, 2026 •

edited

Loading

dd-octo-sts Bot commented Oct 19, 2026 •

edited

Loading