You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@skywalking.apache.org by "kezhenxu94@apache" <ke...@apache.org> on 2020/05/16 10:04:34 UTC
[Proposal] to simplify the agent plugin instrumentation APIs
Hi the SkyWalking Community
I’m writing to propose some changes in the Java agent instrumentation APIs, nowadays, we’re using anonymous inner classes (AIC) as follows:
```java
public ConstructorInterceptPoint[] getConstructorsInterceptPoints() {
return new ConstructorInterceptPoint[] {
new ConstructorInterceptPoint() {
@Override
public ElementMatcher<MethodDescription> getConstructorMatcher() {
return any();
}
@Override
public String getConstructorInterceptor() {
return "org.apache.skywalking.apm.plugin.elasticsearch.v5.GenericActionConstructorInterceptor";
}
}
};
}
```
There’re some disadvantages with AIC:
1. Every AIC (`new ConstructorInterceptPoint()`) in every plugin definition is a completely unique class, as they are compiled into something like `ActiveMQConsumerInstrumentation$1.class` and `ActiveMQConsumerInstrumentation$2.class`, and thus loading them requires more metaspace, small though.
2. The API is somewhat verbose, and exposes too many concepts, like `ConstructorsInterceptPoint`, `StaticMethodsInterceptPoint`, `InstanceMethodsInterceptPoint`, we can have a unified API to provide all of these.
Hereby I propose the new APIs, which eliminates the unnecessary classes, and should be more semantic, like this:
```java
public ConstructorInterceptPoint[] getConstructorsInterceptPoints() {
return Intercept.constructor(takesArgumentWithType(0, CONSTRUCTOR_INTERCEPT_TYPE))
.with(CONSTRUCTOR_INTERCEPTOR_CLASS)
.build();
}
public InstanceMethodsInterceptPoint[] getInstanceMethodsInterceptPoints() {
return Intercept.instanceMethod(named(ENHANCE_METHOD)
.and(takesArgumentWithType(0, "javax.jms.Destination"))
.and(takesArgumentWithType(1, "javax.jms.Message")))
.with(INTERCEPTOR_CLASS)
.build();
}
public StaticMethodsInterceptPoint[] getStaticMethodsInterceptPoints() {
return Intercept.staticMethod(named("makeWrapper"))
.with("org.apache.skywalking.apm.plugin.dubbo.patch.MakeWrapperInterceptor")
.build();
}
```
We could keep the old APIs there and mark them as deprecated for compatibility, or simply replace them with the new APIs, what do you think?
If there is no objection from the community, I can open an issue to track this.
GitHub @kezhenxu94
Apache SkyWalking, Apache Dubbo
Re: [Proposal] to simplify the agent plugin instrumentation APIs
Posted by "kezhenxu94@163" <ke...@163.com>.
> On May 17, 2020, at 00:47, Daming <zt...@foxmail.com> wrote:
>
>
> Hi Zhenxu,
>
> It is mostly good to me. But I don’t understand how to intercept multi-methods in the New API.
Forgive me I didn’t provide the API to intercept multiple methods, it should be something like:
```java
Intercept.instanceMethod(named("foo"))
.overrideArgs()
.with("com.my.interceptor.FooInterceptor")
.and()
.instanceMethod(named("bar"))
.overrideArgs()
.with("com.my.interceptor.BarInterceptor")
.build()
```
> I think the new api to support to intercept multi-methods when use `.interceptMethod()` after `.with()`. It is very like ByteBuddy API style. But I prefer the following.
>
> ```java
> public InstanceMethodsInterceptPoint[] getInstanceMethodsInterceptPoints() {
> return Intercepts.addInterceptor(Intercept.instanceMethod(named(ENHANCE_METHOD)
> .and(takesArgumentWithType(0, "javax.jms.Destination"))
> .and(takesArgumentWithType(1, "javax.jms.Message")))
> .with(INTERCEPTOR_CLASS)
> .build())
> .addIntercceptor(Intercept.instanceMethod(name(ENHANCE_METHOD).and(takesArguments(1)))
> .with(INTERCEPTOR_CLASS)
> .build())
> .build();
> }
> ```
>
> ———————
> Daming(@dmsolr)
> Apache SkyWalking
>
>
>> 在 2020年5月16日，下午6:04，kezhenxu94@apache <ke...@apache.org> 写道：
>>
>> Hi the SkyWalking Community
>>
>> I’m writing to propose some changes in the Java agent instrumentation APIs, nowadays, we’re using anonymous inner classes (AIC) as follows:
>>
>>
>> ```java
>> public ConstructorInterceptPoint[] getConstructorsInterceptPoints() {
>> return new ConstructorInterceptPoint[] {
>> new ConstructorInterceptPoint() {
>> @Override
>> public ElementMatcher<MethodDescription> getConstructorMatcher() {
>> return any();
>> }
>>
>> @Override
>> public String getConstructorInterceptor() {
>> return "org.apache.skywalking.apm.plugin.elasticsearch.v5.GenericActionConstructorInterceptor";
>> }
>> }
>> };
>> }
>> ```
>>
>>
>> There’re some disadvantages with AIC:
>>
>> 1. Every AIC (`new ConstructorInterceptPoint()`) in every plugin definition is a completely unique class, as they are compiled into something like `ActiveMQConsumerInstrumentation$1.class` and `ActiveMQConsumerInstrumentation$2.class`, and thus loading them requires more metaspace, small though.
>>
>> 2. The API is somewhat verbose, and exposes too many concepts, like `ConstructorsInterceptPoint`, `StaticMethodsInterceptPoint`, `InstanceMethodsInterceptPoint`, we can have a unified API to provide all of these.
>>
>> Hereby I propose the new APIs, which eliminates the unnecessary classes, and should be more semantic, like this:
>>
>>
>> ```java
>> public ConstructorInterceptPoint[] getConstructorsInterceptPoints() {
>> return Intercept.constructor(takesArgumentWithType(0, CONSTRUCTOR_INTERCEPT_TYPE))
>> .with(CONSTRUCTOR_INTERCEPTOR_CLASS)
>> .build();
>> }
>>
>> public InstanceMethodsInterceptPoint[] getInstanceMethodsInterceptPoints() {
>> return Intercept.instanceMethod(named(ENHANCE_METHOD)
>> .and(takesArgumentWithType(0, "javax.jms.Destination"))
>> .and(takesArgumentWithType(1, "javax.jms.Message")))
>> .with(INTERCEPTOR_CLASS)
>> .build();
>> }
>>
>> public StaticMethodsInterceptPoint[] getStaticMethodsInterceptPoints() {
>> return Intercept.staticMethod(named("makeWrapper"))
>> .with("org.apache.skywalking.apm.plugin.dubbo.patch.MakeWrapperInterceptor")
>> .build();
>> }
>> ```
>>
>>
>> We could keep the old APIs there and mark them as deprecated for compatibility, or simply replace them with the new APIs, what do you think?
>>
>>
>> If there is no objection from the community, I can open an issue to track this.
>>
>>
>>
>>
>> GitHub @kezhenxu94
>> Apache SkyWalking, Apache Dubbo
>>
>>
Re: [Proposal] to simplify the agent plugin instrumentation APIs
Posted by Daming <zt...@foxmail.com>.
Hi Zhenxu,
It is mostly good to me. But I don’t understand how to intercept multi-methods in the New API. I think the new api to support to intercept multi-methods when use `.interceptMethod()` after `.with()`. It is very like ByteBuddy API style. But I prefer the following.
```java
public InstanceMethodsInterceptPoint[] getInstanceMethodsInterceptPoints() {
return Intercepts.addInterceptor(Intercept.instanceMethod(named(ENHANCE_METHOD)
.and(takesArgumentWithType(0, "javax.jms.Destination"))
.and(takesArgumentWithType(1, "javax.jms.Message")))
.with(INTERCEPTOR_CLASS)
.build())
.addIntercceptor(Intercept.instanceMethod(name(ENHANCE_METHOD).and(takesArguments(1)))
.with(INTERCEPTOR_CLASS)
.build())
.build();
}
```
———————
Daming(@dmsolr)
Apache SkyWalking
> 在 2020年5月16日，下午6:04，kezhenxu94@apache <ke...@apache.org> 写道：
>
> Hi the SkyWalking Community
>
> I’m writing to propose some changes in the Java agent instrumentation APIs, nowadays, we’re using anonymous inner classes (AIC) as follows:
>
>
> ```java
> public ConstructorInterceptPoint[] getConstructorsInterceptPoints() {
> return new ConstructorInterceptPoint[] {
> new ConstructorInterceptPoint() {
> @Override
> public ElementMatcher<MethodDescription> getConstructorMatcher() {
> return any();
> }
>
> @Override
> public String getConstructorInterceptor() {
> return "org.apache.skywalking.apm.plugin.elasticsearch.v5.GenericActionConstructorInterceptor";
> }
> }
> };
> }
> ```
>
>
> There’re some disadvantages with AIC:
>
> 1. Every AIC (`new ConstructorInterceptPoint()`) in every plugin definition is a completely unique class, as they are compiled into something like `ActiveMQConsumerInstrumentation$1.class` and `ActiveMQConsumerInstrumentation$2.class`, and thus loading them requires more metaspace, small though.
>
> 2. The API is somewhat verbose, and exposes too many concepts, like `ConstructorsInterceptPoint`, `StaticMethodsInterceptPoint`, `InstanceMethodsInterceptPoint`, we can have a unified API to provide all of these.
>
> Hereby I propose the new APIs, which eliminates the unnecessary classes, and should be more semantic, like this:
>
>
> ```java
> public ConstructorInterceptPoint[] getConstructorsInterceptPoints() {
> return Intercept.constructor(takesArgumentWithType(0, CONSTRUCTOR_INTERCEPT_TYPE))
> .with(CONSTRUCTOR_INTERCEPTOR_CLASS)
> .build();
> }
>
> public InstanceMethodsInterceptPoint[] getInstanceMethodsInterceptPoints() {
> return Intercept.instanceMethod(named(ENHANCE_METHOD)
> .and(takesArgumentWithType(0, "javax.jms.Destination"))
> .and(takesArgumentWithType(1, "javax.jms.Message")))
> .with(INTERCEPTOR_CLASS)
> .build();
> }
>
> public StaticMethodsInterceptPoint[] getStaticMethodsInterceptPoints() {
> return Intercept.staticMethod(named("makeWrapper"))
> .with("org.apache.skywalking.apm.plugin.dubbo.patch.MakeWrapperInterceptor")
> .build();
> }
> ```
>
>
> We could keep the old APIs there and mark them as deprecated for compatibility, or simply replace them with the new APIs, what do you think?
>
>
> If there is no objection from the community, I can open an issue to track this.
>
>
>
>
> GitHub @kezhenxu94
> Apache SkyWalking, Apache Dubbo
>
>
Re: [Proposal] to simplify the agent plugin instrumentation APIs
Posted by Sheng Wu <wu...@gmail.com>.
Basically, this makes sense to me. Please go for it.
Sheng Wu 吴晟
Twitter, wusheng1108
kezhenxu94@163 <ke...@163.com> 于2020年5月17日周日 下午1:19写道：
>
>
> > On May 16, 2020, at 18:36, Sheng Wu <wu...@apache.org> wrote:
> >
> > Hi,
> >
> > This should be more accurate, first of all, this is not a change, it is
> an
> > new set of APIs about Interceptor Points.
>
> It’s a new set of APIs, indeed.
>
> > And it should be fine to add this to reduce the number of agent
> > classes.agentes. But the effect should be very limited. Is there any real
> > concern rather than style?
>
> Not much concern about the classes number and didn’t inspect on the effect
> that it takes, it’s more about the APIs style.
>
> > At last, the names should be shaped better. Interceptor.xxx are very
> > misguided.
> >
>
> I wrote `Intercept`, not `Interceptor`, and I should have wanted to write
> `Intercepts`, it’s a common convention in Java that `XXXs` provides
> utilities to `XXX`, and this is just to make the APIs fluent, reading
> something like `intercepts constructor` or `intercepts static method`, etc.
> The naming strategy, however, can be determined later, it’s not finalized.
>
> > Sheng
> >
> > kezhenxu94@apache <ke...@apache.org>于2020年5月16日 周六下午6:04写道：
> >
> >> Hi the SkyWalking Community
> >>
> >> I’m writing to propose some changes in the Java agent instrumentation
> >> APIs, nowadays, we’re using anonymous inner classes (AIC) as follows:
> >>
> >>
> >> ```java
> >> public ConstructorInterceptPoint[] getConstructorsInterceptPoints() {
> >> return new ConstructorInterceptPoint[] {
> >> new ConstructorInterceptPoint() {
> >> @Override
> >> public ElementMatcher<MethodDescription>
> >> getConstructorMatcher() {
> >> return any();
> >> }
> >>
> >> @Override
> >> public String getConstructorInterceptor() {
> >> return
> >>
> "org.apache.skywalking.apm.plugin.elasticsearch.v5.GenericActionConstructorInterceptor";
> >> }
> >> }
> >> };
> >> }
> >> ```
> >>
> >>
> >> There’re some disadvantages with AIC:
> >>
> >> 1. Every AIC (`new ConstructorInterceptPoint()`) in every plugin
> >> definition is a completely unique class, as they are compiled into
> >> something like `ActiveMQConsumerInstrumentation$1.class` and
> >> `ActiveMQConsumerInstrumentation$2.class`, and thus loading them
> requires
> >> more metaspace, small though.
> >>
> >> 2. The API is somewhat verbose, and exposes too many concepts, like
> >> `ConstructorsInterceptPoint`, `StaticMethodsInterceptPoint`,
> >> `InstanceMethodsInterceptPoint`, we can have a unified API to provide
> all
> >> of these.
> >>
> >> Hereby I propose the new APIs, which eliminates the unnecessary classes,
> >> and should be more semantic, like this:
> >>
> >>
> >> ```java
> >> public ConstructorInterceptPoint[] getConstructorsInterceptPoints() {
> >> return Intercept.constructor(takesArgumentWithType(0,
> >> CONSTRUCTOR_INTERCEPT_TYPE))
> >> .with(CONSTRUCTOR_INTERCEPTOR_CLASS)
> >> .build();
> >> }
> >>
> >> public InstanceMethodsInterceptPoint[]
> getInstanceMethodsInterceptPoints()
> >> {
> >> return Intercept.instanceMethod(named(ENHANCE_METHOD)
> >> .and(takesArgumentWithType(0,
> >> "javax.jms.Destination"))
> >> .and(takesArgumentWithType(1,
> >> "javax.jms.Message")))
> >> .with(INTERCEPTOR_CLASS)
> >> .build();
> >> }
> >>
> >> public StaticMethodsInterceptPoint[] getStaticMethodsInterceptPoints() {
> >> return Intercept.staticMethod(named("makeWrapper"))
> >>
> >>
> .with("org.apache.skywalking.apm.plugin.dubbo.patch.MakeWrapperInterceptor")
> >> .build();
> >> }
> >> ```
> >>
> >>
> >> We could keep the old APIs there and mark them as deprecated for
> >> compatibility, or simply replace them with the new APIs, what do you
> think?
> >>
> >>
> >> If there is no objection from the community, I can open an issue to
> track
> >> this.
> >>
> >>
> >>
> >>
> >> GitHub @kezhenxu94
> >> Apache SkyWalking, Apache Dubbo
> >>
> >> --
> > Sheng Wu 吴晟
> >
> > Apache SkyWalking
> > Apache Incubator
> > Apache ShardingSphere, ECharts, DolphinScheduler podlings
> > Zipkin
> > Twitter, wusheng1108
>
>
>
Re: [Proposal] to simplify the agent plugin instrumentation APIs
Posted by "kezhenxu94@163" <ke...@163.com>.
> On May 16, 2020, at 18:36, Sheng Wu <wu...@apache.org> wrote:
>
> Hi,
>
> This should be more accurate, first of all, this is not a change, it is an
> new set of APIs about Interceptor Points.
It’s a new set of APIs, indeed.
> And it should be fine to add this to reduce the number of agent
> classes.agentes. But the effect should be very limited. Is there any real
> concern rather than style?
Not much concern about the classes number and didn’t inspect on the effect that it takes, it’s more about the APIs style.
> At last, the names should be shaped better. Interceptor.xxx are very
> misguided.
>
I wrote `Intercept`, not `Interceptor`, and I should have wanted to write `Intercepts`, it’s a common convention in Java that `XXXs` provides utilities to `XXX`, and this is just to make the APIs fluent, reading something like `intercepts constructor` or `intercepts static method`, etc. The naming strategy, however, can be determined later, it’s not finalized.
> Sheng
>
> kezhenxu94@apache <ke...@apache.org>于2020年5月16日 周六下午6:04写道：
>
>> Hi the SkyWalking Community
>>
>> I’m writing to propose some changes in the Java agent instrumentation
>> APIs, nowadays, we’re using anonymous inner classes (AIC) as follows:
>>
>>
>> ```java
>> public ConstructorInterceptPoint[] getConstructorsInterceptPoints() {
>> return new ConstructorInterceptPoint[] {
>> new ConstructorInterceptPoint() {
>> @Override
>> public ElementMatcher<MethodDescription>
>> getConstructorMatcher() {
>> return any();
>> }
>>
>> @Override
>> public String getConstructorInterceptor() {
>> return
>> "org.apache.skywalking.apm.plugin.elasticsearch.v5.GenericActionConstructorInterceptor";
>> }
>> }
>> };
>> }
>> ```
>>
>>
>> There’re some disadvantages with AIC:
>>
>> 1. Every AIC (`new ConstructorInterceptPoint()`) in every plugin
>> definition is a completely unique class, as they are compiled into
>> something like `ActiveMQConsumerInstrumentation$1.class` and
>> `ActiveMQConsumerInstrumentation$2.class`, and thus loading them requires
>> more metaspace, small though.
>>
>> 2. The API is somewhat verbose, and exposes too many concepts, like
>> `ConstructorsInterceptPoint`, `StaticMethodsInterceptPoint`,
>> `InstanceMethodsInterceptPoint`, we can have a unified API to provide all
>> of these.
>>
>> Hereby I propose the new APIs, which eliminates the unnecessary classes,
>> and should be more semantic, like this:
>>
>>
>> ```java
>> public ConstructorInterceptPoint[] getConstructorsInterceptPoints() {
>> return Intercept.constructor(takesArgumentWithType(0,
>> CONSTRUCTOR_INTERCEPT_TYPE))
>> .with(CONSTRUCTOR_INTERCEPTOR_CLASS)
>> .build();
>> }
>>
>> public InstanceMethodsInterceptPoint[] getInstanceMethodsInterceptPoints()
>> {
>> return Intercept.instanceMethod(named(ENHANCE_METHOD)
>> .and(takesArgumentWithType(0,
>> "javax.jms.Destination"))
>> .and(takesArgumentWithType(1,
>> "javax.jms.Message")))
>> .with(INTERCEPTOR_CLASS)
>> .build();
>> }
>>
>> public StaticMethodsInterceptPoint[] getStaticMethodsInterceptPoints() {
>> return Intercept.staticMethod(named("makeWrapper"))
>>
>> .with("org.apache.skywalking.apm.plugin.dubbo.patch.MakeWrapperInterceptor")
>> .build();
>> }
>> ```
>>
>>
>> We could keep the old APIs there and mark them as deprecated for
>> compatibility, or simply replace them with the new APIs, what do you think?
>>
>>
>> If there is no objection from the community, I can open an issue to track
>> this.
>>
>>
>>
>>
>> GitHub @kezhenxu94
>> Apache SkyWalking, Apache Dubbo
>>
>> --
> Sheng Wu 吴晟
>
> Apache SkyWalking
> Apache Incubator
> Apache ShardingSphere, ECharts, DolphinScheduler podlings
> Zipkin
> Twitter, wusheng1108
Re: [Proposal] to simplify the agent plugin instrumentation APIs
Posted by Sheng Wu <wu...@apache.org>.
Hi,
This should be more accurate, first of all, this is not a change, it is an
new set of APIs about Interceptor Points.
And it should be fine to add this to reduce the number of agent
classes.agentes. But the effect should be very limited. Is there any real
concern rather than style?
At last, the names should be shaped better. Interceptor.xxx are very
misguided.
Sheng
kezhenxu94@apache <ke...@apache.org>于2020年5月16日 周六下午6:04写道：
> Hi the SkyWalking Community
>
> I’m writing to propose some changes in the Java agent instrumentation
> APIs, nowadays, we’re using anonymous inner classes (AIC) as follows:
>
>
> ```java
> public ConstructorInterceptPoint[] getConstructorsInterceptPoints() {
> return new ConstructorInterceptPoint[] {
> new ConstructorInterceptPoint() {
> @Override
> public ElementMatcher<MethodDescription>
> getConstructorMatcher() {
> return any();
> }
>
> @Override
> public String getConstructorInterceptor() {
> return
> "org.apache.skywalking.apm.plugin.elasticsearch.v5.GenericActionConstructorInterceptor";
> }
> }
> };
> }
> ```
>
>
> There’re some disadvantages with AIC:
>
> 1. Every AIC (`new ConstructorInterceptPoint()`) in every plugin
> definition is a completely unique class, as they are compiled into
> something like `ActiveMQConsumerInstrumentation$1.class` and
> `ActiveMQConsumerInstrumentation$2.class`, and thus loading them requires
> more metaspace, small though.
>
> 2. The API is somewhat verbose, and exposes too many concepts, like
> `ConstructorsInterceptPoint`, `StaticMethodsInterceptPoint`,
> `InstanceMethodsInterceptPoint`, we can have a unified API to provide all
> of these.
>
> Hereby I propose the new APIs, which eliminates the unnecessary classes,
> and should be more semantic, like this:
>
>
> ```java
> public ConstructorInterceptPoint[] getConstructorsInterceptPoints() {
> return Intercept.constructor(takesArgumentWithType(0,
> CONSTRUCTOR_INTERCEPT_TYPE))
> .with(CONSTRUCTOR_INTERCEPTOR_CLASS)
> .build();
> }
>
> public InstanceMethodsInterceptPoint[] getInstanceMethodsInterceptPoints()
> {
> return Intercept.instanceMethod(named(ENHANCE_METHOD)
> .and(takesArgumentWithType(0,
> "javax.jms.Destination"))
> .and(takesArgumentWithType(1,
> "javax.jms.Message")))
> .with(INTERCEPTOR_CLASS)
> .build();
> }
>
> public StaticMethodsInterceptPoint[] getStaticMethodsInterceptPoints() {
> return Intercept.staticMethod(named("makeWrapper"))
>
> .with("org.apache.skywalking.apm.plugin.dubbo.patch.MakeWrapperInterceptor")
> .build();
> }
> ```
>
>
> We could keep the old APIs there and mark them as deprecated for
> compatibility, or simply replace them with the new APIs, what do you think?
>
>
> If there is no objection from the community, I can open an issue to track
> this.
>
>
>
>
> GitHub @kezhenxu94
> Apache SkyWalking, Apache Dubbo
>
> --
Sheng Wu 吴晟
Apache SkyWalking
Apache Incubator
Apache ShardingSphere, ECharts, DolphinScheduler podlings
Zipkin
Twitter, wusheng1108