Repository navigation
Move SpotBugs and forbidden-apis configuration to convention plugins - #12933
gh-worker-dd-mergequeue-cf854d[bot] merged 5 commits into
Conversation
bric3
added
tag: no release notes
In order, to anticipate the static analysis skill, let's migrate the spotless and forbidden api script plugins to convention plugins.
2dc368f to
15b256e
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
More details
The migrated conventions retain the existing task filters, detector exclusions, and module overrides. Gradle compilation and task execution remain unverified.
🤖 Bits Code Review · Commit 15b256e
🟢 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. |
dougqh
left a comment
dougqh
left a comment
There was a problem hiding this comment.
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?
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.
There was a problem hiding this comment.
More details
The convention plugins compiled successfully. Targeted execution checks remain unverified because required dependencies were not cached for offline use.
🤖 Bits Code Review · Commit b34800f
72ecf16
into
master
2 participants
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
buildSrcstill target Java 8 for muzzle workers.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 issue