Improve bytebuddy class enhance #521

kylixs · 2023-04-29T16:46:45Z

Improve bytebuddy class enhance

[ x ] If this is non-trivial feature, paste the links/URLs to the design doc.
https://github.com/kylixs/kylixs.github.io/blob/master/skywalking/byteduddy-retransfrom-faliure-exploration.md
Update the documentation to include this new feature.
Tests(including UT, IT, E2E) are added to verify the new feature.
If it's UI related, attach the screenshots below.
If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #.
Update the CHANGES log.

Add bytebuddy patch to support retransform class multiple times compatible with other javaagent without class cache.
Support for hotswap classes that are enhanced by skywalking plugins, like Spring MVC Controller, etc.

wu-sheng · 2023-04-29T23:37:17Z

I will be back on this at May 3rd.
Thanks for your detailed explanation.
I will take a close look and provide feedback.

wu-sheng

2 main things should be considered

Too many hijacking through reflection. This relies on byte-buddy internal codes/implementations. We don't want to make further upgrading hard. Please work with upstream and try to find a new way, or request byte-buddy to expose new APIs for this.
As you are introducing a new way of retransfer, removing the old mechanism for the class cache is better because it isn't always perfect.

wu-sheng · 2023-05-02T07:20:27Z

...rg/apache/skywalking/apm/agent/core/plugin/interceptor/enhance/ClassEnhancePluginDefine.java

+  DynamicType.Builder<?> newClassBuilder, ClassLoader classLoader,
+  EnhanceContext context) throws PluginException {


These seem some local code style differences. Could you revert these changes?

wu-sheng · 2023-05-02T07:21:09Z

...rg/apache/skywalking/apm/agent/core/plugin/interceptor/enhance/ClassEnhancePluginDefine.java

+ .intercept(SuperMethodCall.INSTANCE.andThen(MethodDelegation.withDefaultConfiguration()
+ .to(BootstrapInstrumentBoost
+ .forInternalDelegateClass(constructorInterceptPoint
+ .getConstructorInterceptor()))));


Please revert all unnecessary changes.

wu-sheng · 2023-05-02T07:25:26Z

apm-sniffer/apm-agent/src/main/java/org/apache/skywalking/apm/agent/SkyWalkingAgent.java

@@ -161,9 +175,10 @@ public DynamicType.Builder<?> transform(final DynamicType.Builder<?> builder,
 final JavaModule javaModule,
 final ProtectionDomain protectionDomain) {
 LoadedLibraryCollector.registerURLClassLoader(classLoader);
+ DelegateNamingResolver.reset();


I am a little confused about this reset. #transform should be called for every type, as a static method, we reset every time?

There seems to be a problem here, I'm not sure if there will be nested retransform multiple classes, which usually occurs the first time a class is loaded. If so, it would be unreasonable to reset all of them, just reset the counter of the class currently being transformed.

If byte-buddy could expose the whole method signature rather than the name, I believe this would not be an issue.
How about asking for upstream first?

What do you mean? Tell me more about it.

wu-sheng · 2023-05-02T07:29:09Z

...r/bytebuddy-patch/src/main/java/net/bytebuddy/agent/builder/NativeMethodStrategySupport.java

+ Field nativeMethodStrategyField = clazz.getDeclaredField("nativeMethodStrategy");
+ nativeMethodStrategyField.setAccessible(true);
+ nativeMethodStrategyField.set(agentBuilder, new SWNativeMethodStrategy(PREFIX));


These seem a hijacking and relying on upstream(bytebuddy) doesn't change the internal implementation.
Considering bytebuddy is always very friendly to our community, you are better to submit a proposal/request to bytebuddy, and discuss the official API to do so.

OK, That's what I am thinking.

wu-sheng · 2023-05-02T07:32:00Z

...ytebuddy-patch/src/main/java/net/bytebuddy/implementation/SWAuxiliaryTypeNamingStrategy.java

+ Field specialMethodInvocationField = auxiliaryTypeClass.getDeclaredField("specialMethodInvocation");
+ specialMethodInvocationField.setAccessible(true);
+ SpecialMethodInvocation specialMethodInvocation = (SpecialMethodInvocation) specialMethodInvocationField.get(auxiliaryType);


This is also another kind hijacking, please considering working with the upstream.

wu-sheng · 2023-05-02T07:32:19Z

...ytebuddy-patch/src/main/java/net/bytebuddy/implementation/SWAuxiliaryTypeNamingStrategy.java

+
+ } else if (auxiliaryTypeClassName.endsWith("Pipe$Binder$RedirectionProxy")) {
+ // get MethodDescription from field 'sourceMethod'
+ Field sourceMethodField = auxiliaryTypeClass.getDeclaredField("sourceMethod");


Same hijacking concern.

wu-sheng · 2023-05-02T07:32:28Z

...ytebuddy-patch/src/main/java/net/bytebuddy/implementation/SWAuxiliaryTypeNamingStrategy.java

+
+ } else if (auxiliaryTypeClassName.endsWith("FieldProxy$Binder$AccessorProxy")) {
+ // get fieldDescription
+ Field fieldDescriptionField = auxiliaryTypeClass.getDeclaredField("fieldDescription");


Same hijacking concern.

wu-sheng · 2023-05-02T07:38:14Z

...tebuddy-patch/src/main/java/net/bytebuddy/implementation/SWImplementationContextFactory.java

+import net.bytebuddy.utility.RandomString;
+
+/**
+ * Design to generate fixed name for CacheValueField (cache delegating Method)


This comment doesn't fit the codes. Please reword this, you are building a ContextFactory with target type name based suffix on the top of Implementation.Context.Default.

wu-sheng · 2023-05-02T07:46:14Z

apm-sniffer/bytebuddy-patch/src/main/java/net/bytebuddy/agent/builder/SWAsmVisitorWrapper.java

+/**
+ * Remove duplicated fields of instrumentedType
+ */
+public class SWAsmVisitorWrapper implements AsmVisitorWrapper {


I am not fully following this. Are you trying to cache all original methods before the instrumentation?
The name RemoveDuplicatedFieldsClassVisitor is not very clear to me.
Please reword the comments for SWAsmVisitorWrapper

I'll check it agian.

wu-sheng · 2023-05-02T07:50:41Z

Hi @kylixs Thanks for providing your way of debugging and resolving the issue. I believe locking the field and method name for auxiliary class and method is a way to resolve the transfer issue.
But meanwhile, I am not very certain, whether this is always perfect, as SkyWalking agent kernel has been used in many other cases, such as other APMs' agent are actually a fork of upstream.
Is this way going to bring a new conflict between skywalking agent and skywalking agent fork(re-distribution)? Our answer doesn't have to be that, we support this kind of hybrid deployment, but I want to include this as a consideration.

kylixs · 2023-05-02T09:09:31Z

It's a bit complicated, and we haven't tested enough scenarios to know if there are other compatibility issues. I came up with a few ideas:

Provide a fallback transform mode. If there is a compatibility problem, users can fall back to the previous mode. We can keep improving it.
Create a retransform javaagent for automated testing every plugin for compatibility, and may need to modify testing tools to support it.

wu-sheng · 2023-05-02T09:15:11Z

Provide a fallback transform mode. If there is a compatibility problem, users can fall back to the previous mode. We can keep improving it.

Falling back usually is too complex to maintain.

About the re-transfer, once we have a clear path about how skywalking agent works in that agent, I think we are fine.
The question I left is only about whether we are going to conflict with our own fork, due to names are always same in two agents.

kylixs · 2023-05-02T09:41:58Z

About the re-transfer, once we have a clear path about how skywalking agent works in that agent, I think we are fine.
The question I left is only about whether we are going to conflict with our own fork, due to names are always same in two agents.

The key issue is the class and method that the plug-in intercepts. As you can see from the documentation of the process, the processing path is fairly clear, but it may not include all of the paths. Currently, all skywalking plug-ins are supported, but we don't know if any of the custom plug-ins have compatibility issues. At present, we have a better understanding of ByteBuddy processing process and debugging tools, which can solve the problem quickly.

kylixs · 2023-05-02T09:50:47Z

The question I left is only about whether we are going to conflict with our own fork, due to names are always same in two agents.

I don't quite understand this. Could you explain it in detail?

wu-sheng · 2023-05-02T10:23:05Z

The question I left is only about whether we are going to conflict with our own fork, due to names are always same in two agents.

I don't quite understand this. Could you explain it in detail?

The name of fields and methods are predictable and will be same.
There are other solutions of other vendors using our agent to build their own distributions, which may be APM agents or even others.
Once the names are locked, would they conflict with each others?

kylixs · 2023-05-02T11:02:45Z

ByteBuddy uses random names, which is essential in dynamic proxy/class creation, but in APM scenarios, dynamic names are bad.
We use sw_xxx$ as a specific flag for auxiliary class names, field names, and method names that do not conflict with other vendor names.
Vendors can extend with the Skywalking agent interceptor mechanism, or register their own class transformer, which is fine. The locking name only affects the Skywalking agent itself, and vendors using their own ByteBuddy instances are unchanged.

wu-sheng · 2023-05-02T11:13:04Z

I know they can, I mean many of them are not doing this.
I think you still are not following what is redistribution of open source. Thinking about Ubuntu relationship with upstream Linux. Take SkyWalking as upstream Linux, vendor agents are a kind of Ubuntu.

They don't change much, and customize many things. They just enhance some features, and release under their brand.
This is totally reasonable and common case for a popular open source project.

kylixs · 2023-05-02T11:24:32Z

If they haven't modified the Skywalking agent's plugin interceptor flow, they should be compatible. If they change the interceptor mechanism or the ByteBuddy option, they need to be compatible with the new mode. More information? Are there any known redistribution projects of Skywalking agent?

wu-sheng · 2023-05-02T11:37:30Z

I can't provide this information as I don't have their owner permission, sorry.
I just want to mention this as a case. Such as if they have a modified plugin/interceptor but targeting the same class and same method.

kylixs · 2023-05-02T12:24:37Z

I can't provide this information as I don't have their owner permission, sorry. I just want to mention this as a case. Such as if they have a modified plugin/interceptor but targeting the same class and same method.

Without enough information, it's hard to evaluate the results. Some possibilities:

Custom plugins: For example, two plugins enhance the same method of the same class. Not tested yet, don't know if this is currently supported. If we want to support it, we should define it as a feature and add test cases.
Custom class transformer: Use a different ByteBuddy/AgentBuilder instance so it is not affected by name locking.

…ome testcases

kylixs · 2023-05-28T16:05:08Z

How about two skywalking agents for one target? I don't expect we would do two interceptors for one target inside skywalking. The other one is fork distribution. Will that work?

Intercept same classes with two AgentBuilders are ok. Do you have any ideas on how to add specific test cases?

wu-sheng · 2023-05-28T16:13:30Z

How about two skywalking agents for one target? I don't expect we would do two interceptors for one target inside skywalking. The other one is fork distribution. Will that work?

Intercept same classes with two AgentBuilders are ok. Do you have any ideas on how to add specific test cases?

What do you mean OK? We only have one, right? You changed it to multiple? Or one per plugin or something?

kylixs · 2023-05-30T03:44:05Z

What do you mean OK? We only have one, right? You changed it to multiple? Or one per plugin or something?

Currently, it is supported to install an interceptor on the same method in two AgentBuilders respectively, resulting in a chain of proxy methods.

I think there are a few scenarios where conflicts need to be resolved:

The interception method of user-defined plug-ins may overlap with the existing plug-ins of Skywalking agent.
Two Skywalking agent plug-ins may handle the same method.

Interception methods can overlap in many ways, such as two plug-ins matching different annotations but having both annotations in the same class or method.

I think it is better to solve the above problems in one agent, because putting it into two agents will bring maintenance workload and confusion. I think users would prefer to maintain one custom plug-in rather than two agents.

One idea is to identify plug-ins that overlap and use different AgentBuilders to handle them. I'll try to see if this works.

wu-sheng · 2023-05-30T03:45:33Z

Generally, the ides works for me.

kylixs · 2023-06-11T09:58:12Z

Took some time to try two solutions:

Each plugin uses an isolated AgentBuilder, and it turns out that each AgentBuilder instance do a lot of repetitive processing, which makes Java application start too slowly.
Each plugin uses an isolated Transformer and found that ByteBuddy is not supported.
I made some rough changes to the ByteBuddy AgentBuilder code, and it is now running, loading two plugins at the same time to enhance the classes of Spring Controller, and they both work.

I will submit it to the ByteBuddy community and need more discussion and design.

ByteBuddy code: https://github.com/kylixs/byte-buddy/tree/separate-transformer
Custom spring mvc plugin: https://github.com/kylixs/spring-mvc-plugin-demo

wu-sheng · 2023-06-11T10:03:04Z

I remembered that @raphw mentioned, the advice annotation rather than delegator could support multiple interceptors. But I didn't try.

kylixs · 2023-06-11T10:37:53Z

I remembered that @raphw mentioned, the advice annotation rather than delegator could support multiple interceptors. But I didn't try.

We can intercept the target class method using Advice, and then filter the matching plugins in the Advice static proxy method and invoke the intercepting classes of plugins.
It seems that this method can adapt to the original plugin, but need to change the plugin mechanism, I try it.

wu-sheng · 2023-06-11T11:05:18Z

It is good to test various possibilities. Eventually, we should keep changes as less as possible, to keep stability.

raphw · 2023-06-12T21:36:43Z

Sorry, but this does not go into a direction that I see being merged into Byte Buddy. But you are very welcome to implement this on top of existing APIs and to offer it as an extension.

wu-sheng · 2023-06-22T14:55:12Z

@kylixs I am back on this as the 9.5.0 had been released. AFAIK, there are several things scoped in this PR, which makes it very hard to review and move forward from tech and stable perspectives.
So, I want to separate the issue into the following parts

Support predictable field names for interceptors
Support retransfer with other agents. I think it is easy if we landed <1>
Support multiple interceptors for one single method. This may be the hard part, but to be honest, this is not a very common use case inside SkyWalking.

So, how about we are going to do <1> only first? I think from your existing changes, it should be easy to support that without hijacking?

kylixs · 2023-06-22T15:50:08Z

@wu-sheng

So, how about we are going to do <1> only first? I think from your existing changes, it should be easy to support that without hijacking?

I agree with your idea, but I think <1> and <2> are the same things, and have been reached.
I'm not sure to support that without hijacking, maybe make it a little more elegant.

<3> Support multiple interceptors for one single method.

What has been tried:

Each plugin create a separate Transformer, requiring AgentBuilder to support isolated transformers.
With Advice transformer, the enhanced code is bloated and may be repeatedly enhanced, which is a bit troublesome to control.

Next step:
Make InstMethodsInter and so on to support for calling multiple XxxAroundInterceptors of different plugins.

wu-sheng · 2023-06-22T16:22:09Z

What has been tried:
Each plugin create a separate Transformer, requiring AgentBuilder to support isolated transformers.
With Advice transformer, the enhanced code is bloated and may be repeatedly enhanced, which is a bit troublesome to control.
Next step:
Make InstMethodsInter and so on to support for calling multiple XxxAroundInterceptors of different plugins.

That is fine.It can wait.

wu-sheng · 2023-06-22T16:26:00Z

I agree with your idea, but I think <1> and <2> are the same things, and have been reached.
I'm not sure to support that without hijacking, maybe make it a little more elegant.

I don't think we will avoid all hijackings, but we could limit the scope, currently, they are too wide. And we could add tests to verify them when we need to bump up the version of Byte-buddy.

wu-sheng · 2023-06-23T07:41:51Z

@kylixs Please open a new and clean changes PR. Let's focus on one at a time.

kylixs · 2023-06-23T08:45:55Z

move to: #561

kylixs added 9 commits April 21, 2023 01:50

add bytebuddy patch

ca2959b

fix inject NativeMethodStrategy

82601aa

generate fixed delegate field name in retransform class

434199c

Generate fixed auxiliary type name by SWAuxiliaryTypeNamingStrategy

45489b4

polish code

9278b7e

Revert pom dependencies

330d43f

add apache license

178ce18

add bytebuddy-patch module

c83c655

test retransform class without class cache

9176613

Superskyyy added the core label Apr 29, 2023

Superskyyy added this to the 8.16.0 milestone Apr 29, 2023

Superskyyy requested review from wu-sheng and kezhenxu94 April 29, 2023 18:28

wu-sheng added the enhancement New feature or request label Apr 29, 2023

Merge branch 'main' into improve-bytebuddy

b192d0f

wu-sheng reviewed May 2, 2023

View reviewed changes

kylixs added 2 commits May 2, 2023 18:07

reset the transformed class only

a4d1ede

revert code style

12ff880

kylixs added 3 commits May 14, 2023 23:57

Add SWClassFileLocator to resolve auxiliary type of first agent and s…

486f004

…ome testcases

config test path pattern

4de22bf

fix conflicts with apm-jdk-threadpool-plugin

f7b7d62

stop thread of SWClassFileLocator on close

6fbae81

wu-sheng removed this from the 8.16.0 milestone May 31, 2023

kylixs mentioned this pull request Jun 11, 2023

How to isolate multiple Transformers? raphw/byte-buddy#1454

Open

kylixs added 4 commits June 23, 2023 00:56

Resolve delegate field name by plugin instrumentation and matcher

7df7627

Make predicated delegation field name by reuse intercept points

ed52218

Generate auxiliary type name by hashcode only without description

b1e6e35

introduce SWAgentBuilderDefault

c6993f6

kylixs force-pushed the improve-bytebuddy branch from e4c3703 to c6993f6 Compare June 23, 2023 06:37

kylixs added 2 commits June 23, 2023 14:41

Merge remote-tracking branch 'upstream/main' into improve-bytebuddy

0bb7608

upgrade parent version

d52a5f4

fix retransform test

3b3350c

kylixs closed this Jun 23, 2023

		DynamicType.Builder<?> newClassBuilder, ClassLoader classLoader,
		EnhanceContext context) throws PluginException {

Improve bytebuddy class enhance #521

Improve bytebuddy class enhance #521

Conversation

kylixs commented Apr 29, 2023

Improve bytebuddy class enhance

wu-sheng commented Apr 29, 2023

wu-sheng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wu-sheng commented May 2, 2023

kylixs commented May 2, 2023

wu-sheng commented May 2, 2023

kylixs commented May 2, 2023

kylixs commented May 2, 2023

wu-sheng commented May 2, 2023

kylixs commented May 2, 2023

wu-sheng commented May 2, 2023 • edited Loading

kylixs commented May 2, 2023

wu-sheng commented May 2, 2023

kylixs commented May 2, 2023

kylixs commented May 28, 2023

wu-sheng commented May 28, 2023

kylixs commented May 30, 2023

wu-sheng commented May 30, 2023

kylixs commented Jun 11, 2023

wu-sheng commented Jun 11, 2023

kylixs commented Jun 11, 2023

wu-sheng commented Jun 11, 2023 • edited Loading

raphw commented Jun 12, 2023

wu-sheng commented Jun 22, 2023

kylixs commented Jun 22, 2023 • edited Loading

wu-sheng commented Jun 22, 2023

wu-sheng commented Jun 22, 2023

wu-sheng commented Jun 23, 2023

kylixs commented Jun 23, 2023

wu-sheng commented May 2, 2023 •

edited

Loading

wu-sheng commented Jun 11, 2023 •

edited

Loading

kylixs commented Jun 22, 2023 •

edited

Loading