Code review sweep (run 25154254051)#18441
Merged
Merged
Conversation
Automated code review of instrumentation/jaxrs/jaxrs-1.0/javaagent.
Automated code review of instrumentation/jaxrs/jaxrs-2.0/jaxrs-2.0-annotations/javaagent.
Automated code review of instrumentation/jaxrs/jaxrs-2.0/jaxrs-2.0-cxf-3.2/javaagent.
Automated code review of instrumentation/jaxrs/jaxrs-2.0/jaxrs-2.0-jersey-2.0/javaagent.
Automated code review of instrumentation/jaxrs/jaxrs-2.0/jaxrs-2.0-payara-testing.
Automated code review of instrumentation/jaxrs/jaxrs-2.0/jaxrs-2.0-resteasy-3.0/javaagent.
trask
approved these changes
Apr 30, 2026
trask
added a commit
to trask/opentelemetry-java-instrumentation
that referenced
this pull request
Apr 30, 2026
When a PR is created with `gh pr create --label ...` (via the otelbot app token in code-review-sweep.yml), GitHub fires `opened` and `labeled` pull_request events within the same second. Both runs land in the build-pull-request.yml concurrency group with cancel-in-progress, so the (skipped, label-not-build-relevant) labeled run cancels the real opened run, leaving the PR with no build and a failing required-status-check (see e.g. PR open-telemetry#18441). Split into two steps: create the PR (otelbot app token, so the PR is authored by otelbot), then add the label as a separate step using the default GITHUB_TOKEN. GitHub's anti-recursion rule means actions taken with GITHUB_TOKEN don't trigger workflow events, so adding the label no longer fires a labeled pull_request event and the build for the PR is no longer cancelled.
trask
added a commit
to trask/opentelemetry-java-instrumentation
that referenced
this pull request
Apr 30, 2026
GitHub evaluates a workflow's `concurrency:` group BEFORE its job-level `if:` filter. When a non-build-affecting label is added to a PR (e.g. `automated code review` from code-review-sweep, any auto-label from .github/labeler.yml, or any human/bot label-add), the resulting `pull_request labeled` run enters the shared per-PR group with cancel-in-progress, cancels the real in-progress build, and only then evaluates the `if:` and skips — leaving the PR with no build and a failing required-status-check (see PR open-telemetry#18441). Route those irrelevant-label runs to a unique, per-run-id throwaway concurrency group so they neither cancel nor are cancelled by anything; the job's `if:` then skips them in ~1s. Build-affecting labels (`test native`, `test openj9`, `test windows`) and all other event types keep the shared per-PR group, preserving the existing cancel-and-rerun semantics. The fix is at the build workflow level, so it applies uniformly to every label-add source (humans, the labeler workflow, otelbot, dependabot, etc.).
trask
added a commit
to trask/opentelemetry-java-instrumentation
that referenced
this pull request
Apr 30, 2026
Adds .github/workflows/test-on-comment.yml that handles three slash-commands on PR comments from OWNER/MEMBER/COLLABORATOR users: /test native -- force-run native tests /test openj9 -- run the openj9 test variants /test windows -- run the windows smoke tests These replace the previous label-driven design (test native / test openj9 / test windows labels). Labels were removed because adding any label fires a pull_request labeled event, which entered the build's per-PR concurrency group BEFORE the job-level if: filter ran, cancelling the in-progress real build whenever any non-test label was added by code-review-sweep, dependabot, the labeler, or a human (see PR open-telemetry#18441). Even after a conditional concurrency-group fix, the labeled run's check_suite shadowed the in-progress real build's check_suite in the PR Checks UI, since GitHub keys that view by workflow file with the latest check_suite winning. The new design: * build-pull-request.yml only triggers on opened / synchronize / reopened. Its concurrency group is the simple per-PR one with cancel-in-progress, restoring the pre-bug push semantics for the regular event flow. * The native-tests path filter formerly in .github/labeler.yml is moved inline as a resolve-native job that diffs the PR head against the base, eliminating the labeler-vs-build race entirely. .github/labeler.yml and .github/workflows/label.yml are deleted (the labeler rule was the only content in those files). * build-pull-request.yml additionally accepts workflow_dispatch with explicit run-{native,openj9,windows-smoke}-tests inputs; test-on-comment.yml validates the commenter's association and dispatches it against the PR's head branch, so the resulting check_runs surface in the PR Checks tab and supersede prior runs in the same per-PR concurrency group. Limitations: * /test commands aren't supported for PRs from forks (workflow_dispatch can only target refs in the base repo). The comment workflow detects this and posts a brief explanation back. * Native tests now run for any PR whose paths match the historical glob, regardless of whether a test native label was applied. This was already the intended behavior; the label was just an artifact of how the labeler implemented the path filter.
trask
added a commit
to trask/opentelemetry-java-instrumentation
that referenced
this pull request
Apr 30, 2026
Adds .github/workflows/test-on-comment.yml that handles three slash-commands on PR comments from OWNER/MEMBER/COLLABORATOR users: /test native -- force-run native tests /test openj9 -- run the openj9 test variants /test windows -- run the windows smoke tests These replace the previous label-driven design (test native / test openj9 / test windows labels). Labels were removed because adding any label fires a pull_request labeled event, which entered the build's per-PR concurrency group BEFORE the job-level if: filter ran, cancelling the in-progress real build whenever any non-test label was added by code-review-sweep, dependabot, the labeler, or a human (see PR open-telemetry#18441). Even after a conditional concurrency-group fix, the labeled run's check_suite shadowed the in-progress real build's check_suite in the PR Checks UI, since GitHub keys that view by workflow file with the latest check_suite winning. The new design: * build-pull-request.yml only triggers on opened / synchronize / reopened. Its concurrency group is the simple per-PR one with cancel-in-progress, restoring the pre-bug push semantics for the regular event flow. * The native-tests path filter formerly in .github/labeler.yml is moved inline as a resolve-native job that diffs the PR head against the base, eliminating the labeler-vs-build race entirely. .github/labeler.yml and .github/workflows/label.yml are deleted (the labeler rule was the only content in those files). * build-pull-request.yml additionally accepts workflow_dispatch with explicit run-{native,openj9,windows-smoke}-tests inputs; test-on-comment.yml validates the commenter's association and dispatches it against the PR's head branch, so the resulting check_runs surface in the PR Checks tab and supersede prior runs in the same per-PR concurrency group. Limitations: * /test commands aren't supported for PRs from forks (workflow_dispatch can only target refs in the base repo). The comment workflow detects this and posts a brief explanation back. * Native tests now run for any PR whose paths match the historical glob, regardless of whether a test native label was applied. This was already the intended behavior; the label was just an artifact of how the labeler implemented the path filter.
trask
added a commit
to trask/opentelemetry-java-instrumentation
that referenced
this pull request
Apr 30, 2026
Adds .github/workflows/test-on-comment.yml that handles three slash-commands on PR comments from OWNER/MEMBER/COLLABORATOR users: /test native -- force-run native tests /test openj9 -- run the openj9 test variants /test windows -- run the windows smoke tests These replace the previous label-driven design (test native / test openj9 / test windows labels). Labels were removed because adding any label fires a pull_request labeled event, which entered the build's per-PR concurrency group BEFORE the job-level if: filter ran, cancelling the in-progress real build whenever any non-test label was added by code-review-sweep, dependabot, the labeler, or a human (see PR open-telemetry#18441). Even after a conditional concurrency-group fix, the labeled run's check_suite shadowed the in-progress real build's check_suite in the PR Checks UI, since GitHub keys that view by workflow file with the latest check_suite winning. The new design: * build-pull-request.yml only triggers on opened / synchronize / reopened. Its concurrency group is the simple per-PR one with cancel-in-progress, restoring the pre-bug push semantics for the regular event flow. * The native-tests path filter formerly in .github/labeler.yml is moved inline as a resolve-native job that diffs the PR head against the base, eliminating the labeler-vs-build race entirely. .github/labeler.yml and .github/workflows/label.yml are deleted (the labeler rule was the only content in those files). * build-pull-request.yml additionally accepts workflow_dispatch with explicit run-{native,openj9,windows-smoke}-tests inputs; test-on-comment.yml validates the commenter's association and dispatches it against the PR's head branch, so the resulting check_runs surface in the PR Checks tab and supersede prior runs in the same per-PR concurrency group. Limitations: * /test commands aren't supported for PRs from forks (workflow_dispatch can only target refs in the base repo). The comment workflow detects this and posts a brief explanation back. * Native tests now run for any PR whose paths match the historical glob, regardless of whether a test native label was applied. This was already the intended behavior; the label was just an artifact of how the labeler implemented the path filter.
trask
added a commit
to trask/opentelemetry-java-instrumentation
that referenced
this pull request
Apr 30, 2026
The labeled trigger caused two issues that combined into recurring CI breakage on PRs that received any label (renovate, dependabot, code-review-sweep, the labeler, or a human applied one): * GitHub evaluates a workflow's `concurrency:` group before its job-level `if:` filter, so a labeled run cancelled the in-progress real build before being filtered out. * Even after a conditional concurrency-group fix, the labeled run's check_suite displaced the in-progress real build's check_suite in the PR Checks UI (GitHub keys that view by workflow file, latest check_suite wins). Concrete failure: PR open-telemetry#18441, run https://github.com/open-telemetry/opentelemetry-java-instrumentation/actions/runs/25156724872 — every job ended within ~2 seconds with no steps executed. Fix: drop `labeled` from the trigger entirely. Read labels from `github.event.pull_request.labels` (which is present on every `pull_request` event) and gate optional jobs by `contains(...)`. Skipped jobs that are listed as required satisfy branch protection, so auto-merge still works. * `test openj9` / `test windows` labels: still consulted, but only at the moments the build naturally runs (opened / synchronize / reopened). Applying a label after the last push no longer re-triggers the build; to pick up the label, close/reopen the PR or push another commit. This trade-off avoids the labeled-event race and the wasted reruns from non-test labels. * `test native` is now detected inline from the PR's changed paths via a new `resolve-native` job that mirrors the former `.github/labeler.yml` `test native` rule. The label is no longer auto-applied; the manual label is still honored as a force-enable. * `.github/labeler.yml` and `.github/workflows/label.yml` are deleted (the labeler's only rule was the `test native` one).
trask
added a commit
to trask/opentelemetry-java-instrumentation
that referenced
this pull request
Apr 30, 2026
The labeled trigger caused two issues that combined into recurring CI breakage on PRs that received any label (renovate, dependabot, code-review-sweep, the labeler, or a human applied one): * GitHub evaluates a workflow's `concurrency:` group before its job-level `if:` filter, so a labeled run cancelled the in-progress real build before being filtered out. * Even after a conditional concurrency-group fix, the labeled run's check_suite displaced the in-progress real build's check_suite in the PR Checks UI (GitHub keys that view by workflow file, latest check_suite wins). Concrete failure: PR open-telemetry#18441, run https://github.com/open-telemetry/opentelemetry-java-instrumentation/actions/runs/25156724872 — every job ended within ~2 seconds with no steps executed. Fix: drop `labeled` from the trigger entirely. Read labels from `github.event.pull_request.labels` (which is present on every `pull_request` event) and gate optional jobs by `contains(...)`. Skipped jobs that are listed as required satisfy branch protection, so auto-merge still works. * `test openj9` / `test windows` labels: still consulted, but only at the moments the build naturally runs (opened / synchronize / reopened). Applying a label after the last push no longer re-triggers the build; to pick up the label, close/reopen the PR or push another commit. This trade-off avoids the labeled-event race and the wasted reruns from non-test labels. * `test native` is now detected inline from the PR's changed paths via a new `resolve-native` job that mirrors the former `.github/labeler.yml` `test native` rule. The label is no longer auto-applied; the manual label is still honored as a force-enable. * `.github/labeler.yml` and `.github/workflows/label.yml` are deleted (the labeler's only rule was the `test native` one).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Automated code review sweep walked the following modules in order
and stopped after accumulating at least 10 modified files:
jaxrs-1.0:javaagentjaxrs-2.0-annotations:javaagentjaxrs-2.0:jaxrs-2.0-arquillian-testingjaxrs-2.0-common:javaagentjaxrs-2.0-common:testingjaxrs-2.0-cxf-3.2:javaagentjaxrs-2.0-jersey-2.0:javaagentjaxrs-2.0:jaxrs-2.0-payara-testingjaxrs-2.0-resteasy-3.0:javaagentModule:
jaxrs-1.0:javaagentModule path:
instrumentation/jaxrs/jaxrs-1.0/javaagentSummary
Applied safe review fixes in
jaxrs-1.0javaagent and committed them ase1445521. One broader experimental-test-task issue remains unresolved because the review instructions mark it as non-auto-fix.Applied Changes
Build
File:
build.gradle.kts:34Change: Changed the single-test-task configuration from `tasks.withType<Test>().configureEach` to `tasks.test`.
Reason: `gradle-conventions.md` says single-test-task modules should use `tasks.test`; `withType<Test>().configureEach` is only justified when the module explicitly registers additional `Test` tasks.
Testing
File:
JerseyTest.java:51Change: Made JUnit lifecycle methods package-private and replaced `hasAttributesSatisfying(...)` with `hasAttributesSatisfyingExactly(...)`, enumerating the full expected server-span attribute set.
Reason: The style guide prefers package-private JUnit methods, and `testing-general-patterns.md` requires exact attribute assertions so unexpected attributes are not silently ignored.
Unresolved Items
File:
build.gradle.ktsReason: `otel.instrumentation.common.experimental.controller-telemetry.enabled=true` is still enabled on the default `test` task without a dedicated `testExperimental` task. The review instructions classify this as `Do not auto-fix`; next action is to split flag-on coverage into `testExperimental` and adjust default-test assertions for flag-off behavior.
Module:
jaxrs-2.0-annotations:javaagentModule path:
instrumentation/jaxrs/jaxrs-2.0/jaxrs-2.0-annotations/javaagentSummary
Applied 2 safe repository-guideline fixes for
jaxrs-2.0-annotationsjavaagentand committed them in2f6697b8.Applied Changes
Build
File:
build.gradle.kts:30Change: Replaced `withType<Test>().configureEach` with `tasks.test` for the module’s single explicit test task while preserving the existing `jvmArgs` and `collectMetadata` settings.
Reason: `gradle-conventions.md` says single-test-task modules should use `tasks.test`; `withType<Test>().configureEach` is reserved for modules that explicitly register additional `Test` tasks, and `latestDepTest` does not count.
Style
File:
DefaultRequestContextInstrumentation.java:30Change: Reduced `DefaultRequestContextInstrumentation` from `public` to package-private.
Reason: The style guide requires minimal necessary visibility, and `javaagent/src/main` classes are internal implementation details rather than public API; this class is only instantiated from the same package.
Module:
jaxrs-2.0:jaxrs-2.0-arquillian-testingModule path:
instrumentation/jaxrs/jaxrs-2.0/jaxrs-2.0-arquillian-testingSummary
No safe repository-guideline fixes were applied under
instrumentation/jaxrs/jaxrs-2.0/jaxrs-2.0-arquillian-testing; the scoped files already conform to the reviewed style, testing, Gradle, and metadata-related guidelines.Applied Changes
No safe automated changes were applied.
Module:
jaxrs-2.0-common:javaagentModule path:
instrumentation/jaxrs/jaxrs-2.0/jaxrs-2.0-common/javaagentSummary
No safe repository-guideline fixes were found in
instrumentation/jaxrs/jaxrs-2.0/jaxrs-2.0-common/javaagent; no source changes were applied.Applied Changes
No safe automated changes were applied.
Module:
jaxrs-2.0-common:testingModule path:
instrumentation/jaxrs/jaxrs-2.0/jaxrs-2.0-common/testingSummary
No safe repository-guideline fixes were found in
instrumentation/jaxrs/jaxrs-2.0/jaxrs-2.0-common/testing; no files were changed.Applied Changes
No safe automated changes were applied.
Module:
jaxrs-2.0-cxf-3.2:javaagentModule path:
instrumentation/jaxrs/jaxrs-2.0/jaxrs-2.0-cxf-3.2/javaagentSummary
Applied and committed safe test-pattern fixes for
jaxrs-2.0-cxf-3.2javaagent tests by hoisting inline experimental flag reads into per-classEXPERIMENTAL_ATTRIBUTESconstants.Applied Changes
Testing
File:
CxfHttpServerTest.java:21Change: Added a private static final `EXPERIMENTAL_ATTRIBUTES` field and changed `testExperimental()` to return it.
Reason: `testing-experimental-flags.md` says module experimental flags should be read once into a per-class `EXPERIMENTAL_ATTRIBUTES` constant instead of calling `Boolean.getBoolean(...)` inline.
File:
CxfJettyHttpServerTest.java:18Change: Added a private static final `EXPERIMENTAL_ATTRIBUTES` field and changed `testExperimental()` to return it.
Reason: `testing-experimental-flags.md` says module experimental flags should be read once into a per-class `EXPERIMENTAL_ATTRIBUTES` constant instead of calling `Boolean.getBoolean(...)` inline.
Module:
jaxrs-2.0-jersey-2.0:javaagentModule path:
instrumentation/jaxrs/jaxrs-2.0/jaxrs-2.0-jersey-2.0/javaagentSummary
Applied safe test-guideline fixes in the
jaxrs-2.0-jersey-2.0javaagent module by hoisting repeated experimental flag reads intoEXPERIMENTAL_ATTRIBUTESconstants.Applied Changes
Testing
File:
JerseyHttpServerTest.java:23Change: Added a private static `EXPERIMENTAL_ATTRIBUTES` constant and reused it from `testExperimental()`.
Reason: `testing-experimental-flags.md` requires module experimental flags such as `otel.instrumentation.jaxrs.experimental-span-attributes` to be read once into a per-class `EXPERIMENTAL_ATTRIBUTES` constant instead of inline `Boolean.getBoolean(...)` calls.
File:
JerseyJettyHttpServerTest.java:15Change: Added a private static `EXPERIMENTAL_ATTRIBUTES` constant and reused it from `testExperimental()`.
Reason: `testing-experimental-flags.md` requires module experimental flags such as `otel.instrumentation.jaxrs.experimental-span-attributes` to be read once into a per-class `EXPERIMENTAL_ATTRIBUTES` constant instead of inline `Boolean.getBoolean(...)` calls.
Module:
jaxrs-2.0:jaxrs-2.0-payara-testingModule path:
instrumentation/jaxrs/jaxrs-2.0/jaxrs-2.0-payara-testingSummary
Applied one safe repository-guideline fix in
jaxrs-2.0-payara-testingand committed it asc0825965.Applied Changes
Build
File:
build.gradle.kts:20Change: Changed the experimental controller telemetry JVM argument configuration from `withType<Test>().configureEach` inside `tasks {}` to `tasks.test`.
Reason: `gradle-conventions.md` says single-test-task modules should prefer the simple `tasks.test` form; `withType<Test>().configureEach` is reserved for modules that explicitly register additional `Test` tasks.
Module:
jaxrs-2.0-resteasy-3.0:javaagentModule path:
instrumentation/jaxrs/jaxrs-2.0/jaxrs-2.0-resteasy-3.0/javaagentSummary
Applied and committed safe review fixes for
jaxrs-2.0-resteasy-3.0javaagent; one muzzleassertInverseguideline item remains unresolved because the deterministic fix failed local:muzzleconfiguration.Applied Changes
Javaagent
File:
Resteasy30Singletons.java:14Change: Renamed the private singleton backing field from `INSTANCE` to `instrumenter` and kept `instrumenter()` returning that field.
Reason: `javaagent-singletons-patterns.md` says exported singleton accessors should use a lower-camel backing field with the same name as the accessor, while uppercase fields are reserved for constant-like values.
Testing
File:
ResteasyHttpServerTest.java:19Change: Hoisted the inline `Boolean.getBoolean("otel.instrumentation.jaxrs.experimental-span-attributes")` call into a per-class `EXPERIMENTAL_ATTRIBUTES` constant and returned that from `testExperimental()`.
Reason: `testing-experimental-flags.md` requires experimental flags to be read once into a per-class `EXPERIMENTAL_ATTRIBUTES` constant instead of duplicating inline property reads at call sites.
File:
ResteasyJettyHttpServerTest.java:15Change: Hoisted the inline `Boolean.getBoolean("otel.instrumentation.jaxrs.experimental-span-attributes")` call into a per-class `EXPERIMENTAL_ATTRIBUTES` constant and returned that from `testExperimental()`.
Reason: `testing-experimental-flags.md` requires experimental flags to be read once into a per-class `EXPERIMENTAL_ATTRIBUTES` constant instead of duplicating inline property reads at call sites.
Unresolved Items
File:
build.gradle.ktsReason: `gradle-conventions.md` says bounded muzzle `pass` ranges should include `assertInverse.set(true)`, but adding it to both split `resteasy-jaxrs` pass ranges failed `./gradlew :instrumentation:jaxrs:jaxrs-2.0:jaxrs-2.0-resteasy-3.0:javaagent:muzzle` with duplicate generated `muzzle-AssertFail-org.jboss.resteasy-resteasy-jaxrs-2.3.6.Final` configuration; this needs broader Gradle/muzzle convention handling rather than a local safe fix.
Download code review diagnostics