Repository navigation
Expand package name checks to javaagent modules - #18428
Merged
Merged
Conversation
trask
marked this pull request as draft
April 30, 2026 02:19
trask
changed the title
trask
force-pushed
the
next
branch
2 times, most recently
from
April 30, 2026 23:00
89e469d to
de7e738
Compare
This was referenced May 12, 2026
This was referenced May 13, 2026
This was referenced May 13, 2026
This was referenced May 14, 2026
…current modules Review finding: This line turns on package-name checking for javaagent source sets, and the checker now exits 1 on this very repository, so the `build / common / check-package-names` CI job fails on this PR head. Running `.github/scripts/check-package-names.sh` at commit aa28f2d reports `ERROR: instrumentation/apache-dbcp-2.0/javaagent/src/main/java/org/apache/commons/dbcp2`, and a probe copy that keeps going instead of exiting reports six unlisted javaagent directories in total: `instrumentation/apache-dbcp-2.0/javaagent/src/main/java/org/apache/commons/dbcp2`, `instrumentation/hbase/hbase-client-common-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/hbase/client/common`, `instrumentation/hbase/hbase-client-common-1.0/javaagent/src/main/java/org/apache/hadoop/hbase/ipc`, `instrumentation/jedis/jedis-4.0/javaagent/src/main/java/redis/clients/jedis`, `instrumentation/opentelemetry-instrumentation-api/opentelemetry-instrumentation-api-1.14/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/instrumentationapi/v1_14`, and `instrumentation/tomcat/tomcat-dbcp-8.0/javaagent/src/main/java/org/apache/tomcat/dbcp/dbcp2`. Four of them hold a single package-private `*Util` helper compiled into the instrumented library's own package (`OpenTelemetryBasicDataSourceUtil`, `OpenTelemetryCallUtil`, `DefaultJedisSocketFactoryUtil`), which is exactly the case the script's existing `advice packages that must live under the instrumented library's own namespace` block covers. One is the self-instrumentation module `opentelemetry-instrumentation-api-1.14`. One, `hbase-client-common-1.0`, is real package debt: its javaagent package is `io.opentelemetry.javaagent.instrumentation.hbase.client.common` and the module name asks for a trailing `v1_0` segment. Fix: add the four advice directories to the advice case block, cover the nested `opentelemetry-instrumentation-api-1.14` javaagent module in the self-instrumentation case block, and add `hbase-client-common-1.0` to the `historical javaagent modules` case block, recording the package rename as a future cleanup in `package-name-exceptions-plan.md` the same way the plan tracks every other outstanding rename. Review finding: This added self-instrumentation exception pattern matches no directory in the repository, so it is dead from the moment it is introduced. There is no `instrumentation/opentelemetry-instrumentation-api/javaagent` module at this head; the module is nested one level deeper as `instrumentation/opentelemetry-instrumentation-api/opentelemetry-instrumentation-api-1.14/javaagent`, and its sources live in `.../src/main/java/io/opentelemetry/javaagent/instrumentation/instrumentationapi/v1_14`. Because the pattern anchors on `opentelemetry-instrumentation-api/javaagent/`, the nested module is never matched and the checker reports it as an error. Fix: change the pattern to `instrumentation/opentelemetry-instrumentation-api/opentelemetry-instrumentation-api-1.14/javaagent/*` so it names the module that actually exists. Analysis: Both findings have one root cause. The rewritten checker started checking javaagent source sets against a curated exception list, and that list no longer matches the modules on this branch after main was merged in. Four of the six unlisted directories hold one public *Util helper each, compiled into the instrumented library's own package so it can reach types and members that are not public: OpenTelemetryBasicDataSourceUtil for Apache DBCP and Tomcat DBCP, OpenTelemetryCallUtil for the package-private HBase Call type, and DefaultJedisSocketFactoryUtil for Jedis. Those belong in the first case block, whose comment now says "advice and injected helper packages" because they are injected helpers rather than advice. The fifth is the self-instrumentation module, which commit ae8e1ec renamed from opentelemetry-instrumentation-api to the nested, versioned opentelemetry-instrumentation-api-1.14; its exception pattern is updated to the path that exists. The sixth, hbase-client-common-1.0, is real package debt: the module name carries the 1.0 base version but the package io.opentelemetry.javaagent.instrumentation.hbase.client.common does not carry v1_0. Per this branch's own process, package renames go upstream in their own PR against main and the checker keeps a historical exception until then, so it goes in the historical block and package-name-exceptions-plan.md gains a PR 31 entry that describes the rename. The plan's PR 14 section is corrected to name the versioned module and its instrumentationapi.v1_14 package, and the advice-native bucket in "Do Later" lists the four newly excepted namespaces. Upsides: `.github/scripts/check-package-names.sh` exits 0 again, so the `build / common / check-package-names` job can pass. Every new exception is narrow: the four injected-helper entries and the historical HBase entry match one exact directory each, so a later package added under them is still checked. The plan document keeps tracking the one genuine rename instead of silently absorbing it. Downsides: The historical exception hides the HBase package misalignment until the upstream rename lands, and the plan entry is the only thing that keeps it visible. Each merge from main can surface new modules the same way, because the exception lists are curated by hand. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot comment: [General] This pipeline can silently disable the checker. With `errexit` but no `pipefail`, an `rg`/`grep` failure is masked by the successful `sort`, leaving `source_dirs` empty; both checks then complete successfully without inspecting anything. Enable `pipefail` before collecting the paths so discovery errors fail the script. Analysis: The source-directory pipelines run inside command substitutions. Bash's errexit option sees only the final sort status unless pipefail is enabled, so an earlier discovery command can fail while the checker continues with no input. Upsides: Discovery failures now stop the checker instead of producing a false success. Downsides: A grep with no matching source directories now fails the check, which is the intended result because the checker would otherwise inspect nothing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot comment: [Documentation] This action is stale: open-telemetry#18747 is already closed without merging, as line 151 also states. Remove this follow-up and update the heading on line 166 so the tracking plan consistently records the PR as closed. Copilot comment: [Documentation] This planned PR contradicts the final decision immediately above to keep `opentelemetry-api-1.0` in its existing self-instrumentation package, and the checker now explicitly exempts it on that basis. Remove this section or mark PR 19 canceled and state that no package rename is planned, otherwise the tracking plan directs a future contributor to undo the adopted decision. Copilot comment: [Documentation] This summary is outdated: the OpenTelemetry package renames were rejected and open-telemetry#18747 was closed without merging, while the Akka/Scala forkjoin rename already merged in open-telemetry#18772. Leaving both described as planned makes the remaining-work list inaccurate; only AWS SDK is still deferred here. Analysis: The plan mixed final decisions with old follow-up work. PR open-telemetry#18747 is closed, the self-instrumentation packages will stay as they are, and the Akka and Scala forkjoin rename has merged. Removing the obsolete PR 19 section and updating the status text leaves one consistent account of the remaining work. Upsides: Future cleanup work will preserve the adopted self-instrumentation package layout and will not repeat completed or canceled work. Downsides: The plan no longer retains the discarded PR 19 proposal as a standalone section. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… plan Copilot comment: [Documentation] This final sentence still schedules the root `servlet-common` helper move as future work, but the completed-cleanup list and open-telemetry#18855 entry above say that move already landed. Remove the stale sentence so future batching guidance does not direct contributors to repeat completed work. ``` Batch sizing: keep submitting four PRs at a time, but each PR can include up to about 40 changed files when the modules are closely related or the reference spread is easy to audit. Count dependent import updates and test package moves in that limit. Avoid combining package moves that require broad cross-module rewrites just to fill the file budget. The known outlier is the root `servlet-common` helper package move, which likely exceeds 40 changed files because many servlet modules import those helpers; treat that as a dedicated final PR unless a clean split emerges during implementation. ``` Copilot comment: [Documentation] This still describes the PR 26/28 Spring testing work as planned, although both sections above record that it merged together as open-telemetry#18839. Mark those follow-ups completed so this remaining-work summary matches their current status. ``` - `spring-webmvc-common` was completed in PR 25, its testing package follow-up is planned above as PR 28, and the `spring-cloud-gateway-common` testing package decision is planned above as PR 26. `jetty-common`, `tomcat-common`, and `opensearch-rest-common` were completed in PRs 23-24. ``` Analysis: The remaining-work guidance still included two tasks that the plan already records as complete. The root servlet helper move landed in open-telemetry#18778, and the Spring testing package work landed in open-telemetry#18839. Upsides: The plan no longer sends contributors toward package moves that have already merged. Downsides: No material downside identified. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Review finding: These two `historical javaagent modules` exceptions match no directory in the repository, so they are dead from the moment this PR introduces them. Both patterns name a package under `io/opentelemetry/instrumentation/...`, which is the *library* prefix, inside a `javaagent` source set. The javaagent source directories that actually exist at this head are `instrumentation/aws-sdk/aws-sdk-1.11/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/awssdk/v1_11` and `instrumentation/aws-sdk/aws-sdk-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/awssdk/v2_2`; there is no `io/opentelemetry/instrumentation/awssdk/...` directory under either javaagent module, and `aws-sdk-2.2/javaagent` has no `internal` subpackage at all. Those two real directories also need no exception: the checker strips slashes before comparing, so the expected `io/opentelemetry/javaagent/instrumentation/aws/sdk/v1_11` and the actual `io/opentelemetry/javaagent/instrumentation/awssdk/v1_11` normalize to the same string and already pass. I confirmed this by matching every exception pattern in the new script against the full 536-directory source list the script itself builds: these are the only two patterns that match nothing, and the checker still exits 0 with them removed. Keeping them is misleading, because a reader takes them as evidence that the AWS SDK javaagent packages violate the convention when they do not. Fix: delete both `aws-sdk` lines from the `historical javaagent modules` case block. Analysis: The checker builds its directory list from every `*.java` file under `*/library/src/main/java*/` and `*/javaagent/src/main/java*/`, which yields 536 directories at this head. Matching all 32 path exception patterns in the script against that list shows only the two aws-sdk patterns match nothing. Both name `io/opentelemetry/instrumentation/awssdk/...` under a `javaagent` source set, but the javaagent modules place their sources under `io/opentelemetry/javaagent/instrumentation/awssdk/v1_11` and `io/opentelemetry/javaagent/instrumentation/awssdk/v2_2`, and the 2.2 javaagent module has no `internal` subpackage. The two real directories need no exception either: the comparison strips slashes from both sides, so the expected `io/opentelemetry/javaagent/instrumentation/aws/sdk/v1_11` and the actual `io/opentelemetry/javaagent/instrumentation/awssdk/v1_11` normalize to the same string. Removing the two lines keeps the checker passing with ripgrep present and with the `find` fallback. Upsides: The `historical javaagent modules` block now lists only modules that really depart from the convention, so a reader cannot mistake the AWS SDK javaagent packages for package debt. Removing two never-matching patterns also removes two entries that would silently mask a future real violation in those exact directories. Downsides: No material downside identified. `package-name-exceptions-plan.md` defers AWS SDK package renames rather than claiming a checker exception exists, so it stays accurate. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot comment: [Documentation] The detailed remaining-work list repeats the summary omission: the checker still contains exceptions for the Elasticsearch REST and Thrift helper packages, but this purported list of library-specific third-party packages excludes them. Include these packages so future cleanup work does not overlook active exceptions. ``` - Library-specific third-party packages: `io.grpc.override`, `io.lettuce.core.protocol`, `io.nats.client.impl`, and `rx` are likely intentional shims or package-private access points. ``` Copilot comment: [Documentation] This audit omits the remaining `elasticsearch-rest-7.0` and `thrift-0.13` library exceptions. The checker still skips `org.elasticsearch.client`, `org.apache.thrift`, and `org.apache.thrift.server` at lines 50-57, and those source packages remain present, so the four-bucket summary is incomplete. ``` | Library packages under third-party namespaces | `grpc`, `lettuce`, `nats`, `rxjava` each have 1 file | Leave for later; these are likely compatibility/shim packages. | ``` Analysis: The checker still exempts the Elasticsearch REST and Thrift packages. The source tree contains one Elasticsearch REST helper and two Thrift helpers, so both the summary counts and the detailed package list were incomplete. Upsides: The plan now matches every active library-package exception and gives future cleanup work accurate package names and file counts. Downsides: No material downside identified. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot comment: [Documentation] This summary no longer matches the checker: the historical case now contains only HBase common, Java HTTP client/server, and JUL. Only HBase is queued for cleanup, while the other three are explicitly deferred below, so describing this as “many” modules and the best bucket to keep chipping away at misstates the remaining work. ``` | Historical javaagent packages | many one-dir modules remain, usually with one source directory | Best place to keep chipping away. | ``` Copilot comment: [Documentation] Every cleanup listed in this section has already landed and its historical exception is already absent from the updated checker. This imperative therefore records nonexistent pending work; describe the removals as completed instead. ``` These PRs have merged. Remove their historical javaagent package-name exceptions from `.github/scripts/check-package-names.sh` on `next` after the corresponding cleanup lands upstream: ``` Analysis: The checker has four historical javaagent exceptions. HBase common is the only queued cleanup. Java HTTP client, Java HTTP server, and JUL are deferred. The completed-cleanup section also listed work that the checker no longer needs. Upsides: The summary now identifies the one active cleanup and the three deferred exceptions. The completed section no longer tells readers to repeat removals that already happened. Downsides: No material downside identified. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot comment:
[General] On CYGWIN/MINGW/MSYS this branch passes `//` to ripgrep, but `--path-separator` requires exactly one byte, so source discovery exits before any packages are checked. Keep `/` as the separator and disable MSYS argument conversion for the `rg` invocation instead.
```
CYGWIN* | MINGW* | MSYS*) path_separator="//" ;;
```
Analysis: MSYS rewrites a bare `/` path-like argument, while `--path-separator` needs the literal slash. The old `//` workaround depends on MSYS rewriting that value. `MSYS2_ARG_CONV_EXCL` passes the exact separator on MSYS and has no effect on other platforms.
Upsides: The ripgrep invocation now states the required separator directly and does not rely on platform-specific rewriting of a two-byte workaround.
Downsides: The invocation sets one MSYS-specific environment variable that other platforms ignore.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot comment:
[General] The normalized prefix comparison does not enforce a package-segment boundary. For example, a `netty-4.1` source moved to `.../netty/v4_10` still passes because its normalized path starts with the expected `...nettyv4_1`, defeating the version check. Accumulate normalized package segments until the expected name is matched exactly, then allow only subsequent segments as subpackages.
```
if [[ "$package_name_normalized" != "$expected_package_name_normalized"* ]]; then
```
Analysis: Removing every separator and checking a string prefix lets a longer package segment satisfy a shorter expected version. Accumulating complete package segments preserves the supported module-to-package normalization but requires an exact match at a segment boundary.
Upsides: The checker now rejects version-prefix collisions such as `v4_10` for a `v4_1` module while continuing to allow valid subpackages below `v4_1`.
Downsides: The comparison now loops over package segments instead of using one prefix expression.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: trask <218610+trask@users.noreply.github.com>
Co-authored-by: trask <218610+trask@users.noreply.github.com>
Co-authored-by: trask <218610+trask@users.noreply.github.com>
Co-authored-by: trask <218610+trask@users.noreply.github.com>
Co-authored-by: trask <218610+trask@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This was referenced Oct 29, 2026
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
laurit
approved these changes
Oct 30, 2026
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
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.
Extends the package-name checker to validate both library and javaagent source sets.
Replaces individual module-name allowances with bounded rules for
jaxrs-<major.minor>-{annotations,common}and unversioned*-common,*-common-javax, and*-common-jakartamodules. Expected package names are still checked; other roles and variants are not covered by these rules.The checker derives expected packages from versioned module names and fails when source discovery commands fail or find no sources.