WIP: Migrate es-restclient to indy plugin #1356
Closed
Conversation
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
videnkz commented Aug 26, 2020
| } | ||
| | ||
| | ||
| public static class ElasticsearchRestClientAsyncAdvice { |
Contributor Author
There was a problem hiding this comment.
@felixbarny after running tests, I got next exception,
this condition classLoader instanceof ExternalPluginClassLoader in ElasticApmAgent#validateAdvice return false:
java.lang.IllegalStateException: Indy-dispatched advice class must be at the root of the instrumentation plugin. at co.elastic.apm.agent.bci.ElasticApmAgent.validateAdvice(ElasticApmAgent.java:461) at co.elastic.apm.agent.bci.ElasticApmAgent.getTransformer(ElasticApmAgent.java:397) at co.elastic.apm.agent.bci.ElasticApmAgent.applyAdvice(ElasticApmAgent.java:359) at co.elastic.apm.agent.bci.ElasticApmAgent.initAgentBuilder(ElasticApmAgent.java:282) at co.elastic.apm.agent.bci.ElasticApmAgent.initInstrumentation(ElasticApmAgent.java:230) at co.elastic.apm.agent.bci.ElasticApmAgent.initInstrumentation(ElasticApmAgent.java:150) at co.elastic.apm.agent.bci.ElasticApmAgent.initInstrumentation(ElasticApmAgent.java:141) at co.elastic.apm.agent.AbstractInstrumentationTest.beforeAll(AbstractInstrumentationTest.java:60) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.base/java.lang.reflect.Method.invoke(Method.java:566) at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59) at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12) at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56) at org.junit.internal.runners.statements.RunBefores.invokeMethod(RunBefores.java:33) at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:24) at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27) at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306) at org.junit.runners.ParentRunner.run(ParentRunner.java:413) at org.junit.runner.JUnitCore.run(JUnitCore.java:137) at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:68) at com.intellij.rt.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:33) at com.intellij.rt.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:230) at com.intellij.rt.junit.JUnitStarter.main(JUnitStarter.java:58) Member
There was a problem hiding this comment.
You'll have to rename the package to co.elastic.apm.agent.es.restclient.v5_6 co.elastic.apm.agent.esrestclient5_6, or similar.
The reason is that we rely on package scanning to determine which classes to load from the IndyPluginClassLoader. Once we have completed the migration, we might be able to package each plugin in its own jar and inject the whole jar instead of scanning a package.
I just noticed another issue: it's not easy to migrate this plugin as apm-es-restclient-plugin-common obviously has a different package name than apm-es-restclient-plugin-5_6 and thus the package scanning would fail. This means the common plugin would still be loaded form the bootstrap classloader. This would result in NoClassDefFoundErrors for org.elasticsearch.client.Response, for example.
I don't have a clear answer how to fix it. One approach could be to remove the dependency on the ES client in apm-es-restclient-plugin-common. Not sure if that's feasible though.
This is a tricky plugin to migrate.
Contributor
There was a problem hiding this comment.
I think it should be OK if you remove the v5_6 and v6_4 packages entirely and use the versions in the Instrumentation and Advice class names instead. If all advices are in the co.elastic.apm.agent.es.restclient it should work.
videnkz changed the title 
❕ Build Aborted
Expand to view the summary
Build stats
Steps errors Expand to view the steps failures
|
videnkz changed the title videnkz changed the title videnkz force-pushed the issue-1337-migrate-indy-es-restclient-plugin branch from a8684d3 to 59ef6ba Compare 
37 tasks
Member
| Unfortunately, the instrumentations have changed too much in order to meaningfully rebase. I guess we'll have to start over. |
Member
| Here's a new version of it: #2088 |
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
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.
relates to #1337
migration of es-restclient to indy plugin