You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by GitBox <gi...@apache.org> on 2021/05/08 12:36:56 UTC
[GitHub] [skywalking] wu-sheng opened a new issue #6915: Introduce method interceptor API v2
wu-sheng opened a new issue #6915:
URL: https://github.com/apache/skywalking/issues/6915
Hi @apache/skywalking-committers
We have a very stable and powerful plugin for 4 years, all of our plugins are being built on top of it. With more scenarios reported, I noticed there is a small gap we should fill.
In `InstanceMethodsAroundInterceptor` and `StaticMethodsAroundInterceptor` have `before` and `after` methods, if we want to propagate information between them in one intercepting operation, we right now have
1. Dynamic field
2. RunnintContext
3. Enhanced parameter
But when we face concurrency calling of one instance, highly performance-sensitive, and only have `primitive data types` and `string` as parameter, all above three are not good options.
So, we should consider to provide a new instance/static method interceptor API(v2) like this.
```java
/**
* A interceptor, which intercept method's invocation. The target methods will be defined in {@link
* ClassEnhancePluginDefine}'s subclass, most likely in {@link ClassInstanceMethodsEnhancePluginDefine}
*/
public interface InstanceMethodsAroundInterceptorV2 {
/**
* called before target method invocation.
*
* @param result change this result, if you want to truncate the method.
*/
void beforeMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes,
MethodInterceptResult result, MethodInvocationContext mic) throws Throwable;
/**
* called after target method invocation. Even method's invocation triggers an exception.
*
* @param ret the method's original return value. May be null if the method triggers an exception.
* @return the method's actual return value.
*/
Object afterMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes,
Object ret, MethodInvocationContext mic) throws Throwable;
/**
* called when occur exception.
*
* @param t the exception occur.
*/
void handleMethodException(EnhancedInstance objInst, Method method, Object[] allArguments,
Class<?>[] argumentsTypes, Throwable t, MethodInvocationContext mic);
}
```
A new object of `class MethodInvocationContext` is shared per method invocation, which should be created and managed by core inter(s), such as `InstMethodsInter`.
With this, we could share status in the method static and 100% isolation with other methods and static. When we use this with RuntimeContext, in `afterMethod` and `handleMethodException`, we could be easy to know whether some variable in the RunningContext is put or updated by `beforeMethod`.
Notice, this change also affects the bootstrap instrumentation, so, we need the v2 inter templates too.
___
To anyone who wants to handle this, you should know, this is a very core level API enhancement. We expect 100% forward compatibility and recommend the new plugin implementing new APIs. v1 may not be removed ever.
This task only recommends to SkyWalking committer or you are confident you have already known SkyWalking's agent core very well. Newcomers, please don't try this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] dmsolr commented on issue #6915: Introduce method interceptor API v2
Posted by GitBox <gi...@apache.org>.
dmsolr commented on issue #6915:
URL: https://github.com/apache/skywalking/issues/6915#issuecomment-836149741
For performance, I rather copy `InstMethodsInterV2` than add any condition in `InstMethodsInter#intercept(...)`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng closed issue #6915: Introduce method interceptor API v2
Posted by GitBox <gi...@apache.org>.
wu-sheng closed issue #6915:
URL: https://github.com/apache/skywalking/issues/6915
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on issue #6915: Introduce method interceptor API v2
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #6915:
URL: https://github.com/apache/skywalking/issues/6915#issuecomment-836438202
Generally agree to change `AbstractClassEnhancePluginDefine`. I want to add a layer on the top of `AbstractClassEnhancePluginDefine`, which should implement `getInstanceMethodsInterceptV2Points` and `getStaticMethodsInterceptV2Points`. To reduce code change, we could still call this new layer as `AbstractClassEnhancePluginDefine`. and rename the current `AbstractClassEnhancePluginDefine` as `AbstractClassEnhancePluginCoreDefine`. Also, we need a new `AbstractClassEnhancePluginDefineV2`
Notice, I did a naming style change, `getInstanceMethodsInterceptPointV2s` to `getInstanceMethodsInterceptV2Points`
> For performance, I rather copy `InstMethodsInterV2` than add any condition in `InstMethodsInter#intercept(...)`.
Yes. We need to copy them
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on issue #6915: Introduce method interceptor API v2
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #6915:
URL: https://github.com/apache/skywalking/issues/6915#issuecomment-836438510
Also, don't forget the bootstrap instrumentation v2.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on issue #6915: Introduce method interceptor API v2
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #6915:
URL: https://github.com/apache/skywalking/issues/6915#issuecomment-835342965
cc @Ax1an due to you asked related things.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on issue #6915: Introduce method interceptor API v2
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #6915:
URL: https://github.com/apache/skywalking/issues/6915#issuecomment-835369881
> That means lifecycle of `MethodInvocationContext` started before `beforeMethod(...)` and destroied after `afterMethod(...)`?
Yes, and it should be created and disposed of when v2 APIs activated. So v2 APIs should be an option and enhanced version of v1. That is why I said, v1 may not be removed ever. Even with a tiny extra load(one more object created and GC), V2 is slower(very little) than V1, purely from intercepting perspective.
But when we use it in the right place, to provide more context for `afterMethod` and make RunningContext easier and less to use, we could have a better performance from agent + plugin perspective, which is end user's real feeling.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] dmsolr commented on issue #6915: Introduce method interceptor API v2
Posted by GitBox <gi...@apache.org>.
dmsolr commented on issue #6915:
URL: https://github.com/apache/skywalking/issues/6915#issuecomment-836146677
I'd like to talk about this deeply.
IMO, to extend `ClassEnhancePluginDefine` API on the user end. We add the following two methods.
```java
public abstract class AbstractClassEnhancePluginDefine {
/**
* Instance methods intercept point v2. See {@link InstanceMethodsInterceptPointV2}
*
* @return collections of {@link InstanceMethodsInterceptPointV2}
*/
public abstract InstanceMethodsInterceptPointV2[] getInstanceMethodsInterceptPointV2s();
/**
* Instance methods intercept point v2. See {@link InstanceMethodsInterceptPointV2}
*
* @return collections of {@link InstanceMethodsInterceptPointV2}
*/
public abstract StaticMethodsInterceptPointV2[] getStaticMethodsInterceptPointV2s();
}
```
In other words, users can impleme interceptor v1 and v2 APIs in the same class.
And then, we need to add a new method, named "enhanceV2(...)", to get all intercepting points from user-defined and to achieve target method instrumentation.
```java
protected DynamicType.Builder<?> enhanceV2(TypeDescription typeDescription, DynamicType.Builder<?> newClassBuilder,
ClassLoader classLoader, EnhanceContext context) throws PluginException {
newClassBuilder = this.enhanceClassV2(typeDescription, newClassBuilder, classLoader);
newClassBuilder = this.enhanceInstanceV2(typeDescription, newClassBuilder, classLoader, context);
return newClassBuilder;
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] dmsolr commented on issue #6915: Introduce method interceptor API v2
Posted by GitBox <gi...@apache.org>.
dmsolr commented on issue #6915:
URL: https://github.com/apache/skywalking/issues/6915#issuecomment-835349011
That means lifecycle of `MethodInvocationContext` started before `beforeMethod(...)` and destroied after `afterMethod(...)`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org