Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

3030: Reduce dependency on elastic APM tracer in plugins but rather depend on a more general API. #3031

Closed
wants to merge 10 commits into from

Conversation

raphw
Copy link
Contributor

@raphw raphw commented Feb 20, 2023

POC of #3030

@cla-checker-service
Copy link

cla-checker-service bot commented Feb 20, 2023

💚 CLA has been signed

@raphw raphw marked this pull request as draft February 20, 2023 20:02
@github-actions github-actions bot added agent-java community Issues and PRs created by the community triage labels Feb 20, 2023
@github-actions
Copy link

👋 @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
Copy link
Contributor

apmmachine commented Feb 20, 2023

❕ 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
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Start Time: 2023-02-20T22:38:15.397+0000

  • Duration: 3 min 22 sec

Steps errors 2

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 raphw force-pushed the decoupled-tracer branch 2 times, most recently from cd13adf to fe6a9bb Compare February 20, 2023 21:24
@raphw raphw force-pushed the decoupled-tracer branch 4 times, most recently from fed3133 to 2a8b439 Compare February 22, 2023 11:29
…on main Elastic agent. Refactor plugins in a binary compatible manner to make use of the new API.
@raphw raphw force-pushed the decoupled-tracer branch 2 times, most recently from 4bbdee9 to b31c3d5 Compare February 22, 2023 12:55
…s as a binary compatible mirror of the current configuration.
…subtype in one module to demonstrate expandability.
…PI. Add a new tracer aware plugin class that uses the tracer API only.
@raphw raphw force-pushed the decoupled-tracer branch 5 times, most recently from d985f79 to 4b72702 Compare February 23, 2023 07:00
@raphw
Copy link
Contributor Author

raphw commented Feb 23, 2023

This change set introduces a Tracer API that is independent of the apm-agent-core module to separate plugins from the elastic agent's implementation. Instead, the suggested tracer API is located in an independent apm-agent-tracer-api module which is declared a dependency of the current apm-agent-plugin-sdk module. As a result, it becomes possible to implement a Tracer without considering the API needed for instrumentation plugins. Furthermore, it becomes possible to implement custom agents that use the custom tracer while still using the, now generalized, plugins. The change set was created largely through automatic IDE refactoring in the following steps:

  • Interface extraction for the Tracer class and its method parameter types such as Span or Transaction to create a parallel hierarchy. Manually, I generified the new Span and Transaction interfaces to allow for simple subtyping with correct return values.
  • Moved the generated interface APIs to a new module without its own dependencies.
  • Introduces a new GlobalTracer that is API compatible with the current GlobalTracer where the latter now delegates to the former.
  • Type substitution only within the plugin modules only for subprojects of the apm-agent-plugins. As these plugins never instantiate any classes of the tracer API but only act as consumers, so replacing classes with interfaces was with little friction. Also, test code was exempt from substitution as all previous code is subtyping new interfaces of the same name. To satisfy some generic typing, additional code was substituted to a minor extend.
  • Rollback of some (sub-)plugins, namely the apm-api-plugin, apm-awslambda-plugin, apm-logging-plugin, apm-micrometer-plugin, apm-opentelementry, apm-opentracing-plugin and apm-profiling-plugin. Those plugin use a very different subset of methods of the Tracer API and I took them out of scope to keep this change set small, for a very contextual definition of small.
  • Removal of unused methods of the extracted interfaces as they were not needed by the plugins that I extracted. I did some minimal clean up and was able to remove one method manually which I found was only used by a single plugin, but that was recreatable by using other methods of the interface. I did not explore this further, but discovered multiple methods that were only used few or even single places and that would likely be removable.
  • Introduces a polymorphism mechanism on the Tracer API (require/probe) where a tracer can be checked for a subclass. As an experiment, I introduced a ServiceAwareTracer subtype of Tracer. This way, plugins can require extended tracer capabilities and only use them if available. This way, the plugins that were previously excluded could be moved to the new API, too, without lifting this burden upon other plugins. Also, external plugins can simply hook into the current tracer API with extended use without code customizations.
  • Adjustment of the plugin instantiation mechanism. The code now checks for the most-specific constructor Tracer type. This way, plugins can deactivate themselves by requiring a given subtype of Tracer or initialize for limited capability. Without special requirements, they can also only declare a default constructor.
  • Interface extraction of the configuration classes and offering of MinimalConfiguration interface and an implementation of all relevant configuration types. Those can be used if one is not interested in configuring extended capabilities without breaking the current plugin configuration mechanism. The interfaces were then moved to the apm-agent-plugin-sdk module.
  • Dependency substitution of the plugins that were not set aside. Instead of depending on apm-agent-core they now depend on apm-agent-plugin-sdk.
  • Copying of some types that were used by the plugins to the apm-agent-plugin-sdk and automatic import correction on those plugins. For example, many plugins use *Util classes. Also, they rely on the SimpleMethodSignature annotation and now subclass a new TracerAwareInstrumentation. I did not delete the original classes to avoid binary incompatibilities.
  • Introduced WeakConcurrent.weakSpanMap() which exposes WeakConcurrentProviderImpl.weakSpanMapImpl() which is needed in multiple plugins.
  • Fix reminding compilation issues manually, typically with regards to generic types not being compatible when a Span class meets a Span<?> interface.

The change set is supposedly fully binary compatible, but might require some minor source code changes with regards to generic typing. I realize it is a large change, but it is mainly reorganizing the code, hopefully without any behavioral changes. One can think of it as introducing the JPA to Hibernate. While it does not add any direct value or feature to the Elastic agent, it offers a much smaller surface to plugin developers, plus allows plugin developers and their consumers, to use the implementations that you already offer.

Possible improvements on this further would be:

  • Add additional interfaces similar to the ServiceAwareTracer to the apm-agent-tracer-api module which allow supporting the skipped plugins. I already experimented with it by adding layers of a SpanAwareTracer and a MetricsAwareTracer to better understand the needs of the plugins I wanted to skip. This seems straight forward and would again mainly involve automated refactorings.
  • Extract a base agent template, from apm-agent-core that contains modules like code for locating and instantiating plugins or setting up the invoke dynamic dispatch for them. Again, this would mainly involve code relocation which can be automated.

@raphw
Copy link
Contributor Author

raphw commented Mar 1, 2023

Closed in favor of more focused PRs.

@raphw raphw closed this Mar 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agent-java community Issues and PRs created by the community triage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants