Move plugin-related utilities and API to SDK/tracer

[raphw]

This API separates API that is mainly used within plugins from the core module but makes it available via the tracer-api (if functional) or the sdk module (if technical). The scope of the latter can be discussed as this exposes it to user plugins, but I found that there are several legitimate uses of some of the Byte Buddy extensions (several custom annotations and matchers) from within non-built-in-plugins. This is also true for several util classes which are mainly used from plugins where technical utilities are moved to the SDK and functional utilities are moved to the tracer module. Also, the PluginClassLoaderRootPackageCustomizer is moved to the SDK (the question being if it should be exposed, but I think there are use cases). Finally, the TraceAwareInstrumentation class is removed as it creates an unfortunate binding between the SDK and tracer module which does not otherwise exist elsewhere. By removing this class, the two modules can remain separated.

[github-actions]

👋 @raphw Thanks a lot for your contribution!

It may take some time before we review a PR, so even if you don’t see activity for some time, it does not mean that we have forgotten about it.

Every once in a while we go through a process of prioritization, after which we are focussing on the tasks that were planned for the upcoming milestone. The prioritization status is typically reflected through the PR labels. It could be pending triage, a candidate for a future milestone, or have a target milestone set to it.

[apmmachine]

❕ Build Aborted

The PR is not allowed to run in the CI yet

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: 2023-07-03T11:39:18.942+0000

• Duration: 4 min 48 sec

Steps errors

Expand to view the steps failures

Load a resource file from a library

• Took 0 min 0 sec . View more details here
• Description: approval-list/elastic/apm-agent-java.yml

Error signal

• Took 0 min 0 sec . View more details here
• Description: githubApiCall: The REST API call https://api.github.com/orgs/elastic/members/raphw return the message : java.lang.Exception: httpRequest: Failure connecting to the service https://api.github.com/orgs/elastic/members/raphw : httpRequest: Failure connecting to the service https://api.github.com/orgs/elastic/members/raphw : Code: 404Error: {"message":"User does not exist or is not a member of the organization","documentation_url":"https://docs.github.com/rest/reference/orgs#check-organization-membership-for-a-user"}

🤖 GitHub comments

Expand to view the 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!)

[raphw]

I resolved the import conflicts. @SylvainJuge - did you have time to review? Almost there!

[SylvainJuge]

Hi @raphw , thanks for keeping this PR up-to-date.
I've already had a couple of large PR reviews this week, but I'll try to prioritize yours next week.

[SylvainJuge]

Yet another great refactor, thanks for this @raphw !

I think it's mostly fine to merge, and I only have very minor comments.

The extra indirection with service providers seems a bit overkill, but I don't think we have a better way to implement that without exposing implementation details from the core module to the plugins sdk, so I think it's fine. As it's only a few static methods maybe having a single provider/implementation could maybe simplify it a bit, but that would just group unrelated methods together in a single interface.

[JonasKunz]

LGTM as well, I just noticed that a lot of stuff has been moved into the sdk-project, which is intended to be used as public api for external plugins.
Should we maybe move those classes to the .internal package, to mark them as not so public?

And regarding the PluginClassLoaderRootPackageCustomizer: Even if it is now visible to external plugins, it won't be used. For external plugins we always copy all of the classes from the external plugin jar into the plugin classloaders.
For that reason, I would move that one into an internal package aswell.

[jackshirazi]

we shouldn't be expanding the public API at all, as it's intended to be deprecated in favour of the OpenTelemetry API

[raphw]

The premise was that it should be possible to implement Elastic's plugins with the SDK and tracer APIs. As some of the APIs build on top of the current SDK, this was the only way to go. Another option would be to define a sdk-ext module which houses these extensions and to declare them not safe for public use. If one wanted to make these classes available later, they could still be moved to the non-ext module.

[JonasKunz]

we shouldn't be expanding the public API at all, as it's intended to be deprecated in favour of the OpenTelemetry API

I think we need to distinguish between the API used for creating spans (=opentelemetry or our "public API") vs the API required for defining the bootstraping of the external plugin, e.g. ElasticApmInstrumentation. The latter one is represented by our sdk project and there is no compatible OTel pendant yet to my knowledge.

[JonasKunz]

Another option would be to define a sdk-ext module which houses these extensions and to declare them not safe for public use. If one wanted to make these classes available later, they could still be moved to the non-ext module.

I think that would be a bit overkill and add yet another module, of which we already have plenty 😅 . Therefore my suggestion to mark them as not intended for public uses by moving them to an internal package within the same module. While this does not prevent usage in external plugins, it at least discourages it.

[raphw]

In Mockito, we use an Incubating annotation which is more flexible as it can be placed on methods, too. But I am happy to adjust either way.

[JonasKunz]

We decided to go with just the internal package. I'll update your PR and merge it afterwards.

[JonasKunz]

run elasticsearch-ci/docs