Fix agent class package definition

[SylvainJuge]

What does this PR do?

Fixes agent class package definition in shaded classloader

• relates to Elasticsearch instrumentation #2550

Checklist

• This is a bugfix
  • I have updated CHANGELOG.asciidoc
  • I have added tests that would fail without this fix
  • manually testing within ES that it fixes the issue.

[ghost]

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

• Start Time: 2022-04-01T11:40:29.969+0000

• Duration: 44 min 50 sec

Test stats 🧪

Test	Results
Failed	0
Passed	2837
Skipped	20
Total	2857

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

• /test : Re-trigger the build.

• run benchmark tests : Run the benchmark tests.

• run jdk compatibility tests : Run the JDK Compatibility tests.

• run integration tests : Run the Agent Integration tests.

• run end-to-end tests : Run the APM-ITs.

• run windows tests : Build & tests on windows.

• run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

[felixbarny]

Probably a matter of taste but I'd always use affirmative rather than negative method names.
See also https://softwareengineering.stackexchange.com/a/196837

[felixbarny]

Do we even need to call getDefinedPackage or is getPackage good enough? Maybe its preferable to have the same semantics regardless of the Java version?

On Slack you said

If migrating the call to the deprecated getPackage(...) and using getDefinedPackage(...) the difference is that now it does not tries to delegate to parent nor Bootstrap CL when checking if the package is defined.
Given the classes that are loaded by the shaded CL can only be loaded by this CL, I don't think there is any need to ask the parent CL in this case.

So it seems there's not really a benefit of calling getDefinedPackage?

[felixbarny]

Ah, I misunderstood. The getPackage delegates to the parent which may be an issue in certain cases if the package has been defined in a parent CL but not the current CL. Probably not an issue either way in our case, as the parent (platform CL or bootstrap CL) will never have defined an agent package.

I'm fine either way, I guess. Calling getDefinedPackage is technically more correct and avoids calling a deprecated API but adds a bit of boiler plate.

[SylvainJuge]

That's a very good question !

In theory there is no case where the ShadedClassLoader will load classes that are defined in packages that have been defined by a parent classloader, so using getDefinedPackage should be enough and we can skip the extra lookups that are done by getPackage. Performance wise, it should be a trivial difference as the packages are stored in a concurrent map.

In practice, we still have cases where the call the definePackage throws an IllegalArgumentException, which means a parent CL is expected to sometimes create those packages. So if packages can be defined by another CL, hence delegation to parent CL does make sense.
On top of that, we only check for the package presence with a null check and don't use the returned Package instance, so there is no difference on the origin of the Package instance.

As a conclusion I now think it's not worth the extra complexity to use a method handle here, and we can always revisit this later. I'll add a comment to clarify the usage of getPackage vs getDefinedPackage.

[github-actions]

/test