Ibotta / gradle-aspectj-pipeline-plugin

A Gradle plugin for Android projects which performs AspectJ weaving using Android's bytcode manipulation pipeline.
Apache License 2.0
76 stars 14 forks source link

Impossible to waeve code into external dependencies like appcompat or okhttp #9

Open mlilienberg opened 3 years ago

mlilienberg commented 3 years ago

I planned to use this library for tracing purposes to add an interceptor to okhttpclient. I guess targeting classes outside of the project is a limitation of this library, so that targeting classes within okhttp3 package is impossible. It might be good to mention this limitation and others in the README to avoid late suprises.

eschlenz commented 3 years ago

Thanks @mlilienberg. Question for you. Do you currently have this configuration setting defined? Example:

aopWeave {
    filter = "com/example/myapp"
}

If so, have you tried removing that filter configuration? The filter was introduced to actually avoid what you're trying to do. In other words, we found that AOP weaving was occurring on more than just our own code, which was undesirable for us. Sounds like you actually want it though. And this may be a solution.

jdvp commented 3 years ago

I think likely related to #2

bsautner commented 3 years ago

I just happen to be working on implementing this plugin in a project that has a lot of okhttp clients and interceptors and can confirm removing the filter allows me to weave into interceptors.

    @Before("execution(* *.intercept(..))")
    fun intercept(joinPoint: JoinPoint) {
        println("attached intercept: " + joinPoint.sourceLocation + joinPoint.signature.name)
    }

System.out: attached intercept: OkHttpClient.kt:0intercept

mlilienberg commented 3 years ago

Thanks for following up on my question. I am not setting any aopWeave configuration. But i see in aop.log that other aspects in our classpath are recognized. When i use a path to target only project specific Aspects the log looks fine and only our own Aspects are processed. For me this looks all good as i can control what Aspects should be processed but i cannot target third party dependencies in those.

What i need is targeting a class living in okhttp package itself. @bsautner i think your aspect is working because your project contains implementations of okhttp3.Interceptor which become target of your aspect. It would not weave code into interceptors that are defined in third party libraries.

My goal is to trace all network communication through okhttp like firebase.performance or Instana does. For this instana is using such an Aspect processed with https://github.com/Archinamon/android-gradle-aspectj . But this does not support the latest android gradle plugin (4.2.0) anymore.

object MyInterceptor: Interceptor {
    override fun intercept(chain: Interceptor.Chain): Response {
        return chain.proceed(chain.request())
    }
}

@Aspect
class KotlinAspect {
    @Before("execution(* okhttp3.OkHttpClient.Builder.build(..))")
    fun addNetworkInterceptor(joinPoint: JoinPoint) {
        val target = joinPoint.target as OkHttpClient.Builder
        target.addInterceptor(MyInterceptor)
    }
}
eschlenz commented 3 years ago

Thanks all. @mlilienberg I'm going to set up a sample project locally and investigate. I'll report back.

eschlenz commented 3 years ago

@mlilienberg I discovered something new (well, at lest, new to me). And I think it might be a simple solution to your particular problem. Have you looked into using call instead of execution?

As we've established, In your example code you are trying to weave the okhttp3.OkHttpClient.Builder.build(..) method, and therefore, the Builder class which is in the OkHttp3 library. Yet our plugin, by default, attempts to only weave your project code.

The difference between call and execution is where the weave happens. If all of the calls to okhttp3.OkHttpClient.Builder.build(..) occur from code in your project, then you can use call instead, and it will work. I tested this out myself and I didn't even have to change your Aspect code. It just works, because it weaves the code calling the build() method.

Example:

    @Before("call(* okhttp3.OkHttpClient.Builder.build(..))")
    fun addNetworkInterceptor(joinPoint: JoinPoint) {
        val target = joinPoint.target as OkHttpClient.Builder
        target.addInterceptor(MyInterceptor)
    }

More reading on call vs execution: http://perfspy.blogspot.com/2013/09/differences-between-aspectj-call-and.html

(Not the most intelligible resource, but manages to get the core concept across 😆 ^)

mlilienberg commented 3 years ago

@eschlenz thanks for your investigations. You solution is great for a lot of use cases. Unfortunately it is not applicable when the main goal of using AOP is not only separation of concerns but mostly to weave code into libraries you normally have no access to. With firebase performance or Instana we got to know about a lot of network traffic we were not aware of, because they weave code into all UrlConnectionClient or OkttpClient instances. With a single aspect we could inspect traffic from image loading libraries like coil or glide or any other third party sdk that is using those network clients. Weaving code into appcompat classes like fragment or activity lifecycle methods could be another use case. The limitation is also mentioned at the end of this article: https://jdvp.me/articles/Switching-AspectJ-Plugins-Android

I am not sure if this demand can be handled with the pipeline approach at all but it might help others, when this limitation is mentioned in the README. Even if this SDK is used to target internal classes only it might block later modularization attempts when the project becomes bigger.

eschlenz commented 3 years ago

I totally understand, and hear what you're saying. And I agree. I've been doing more research on this today in hopes of a quick fix, but haven't had much success.

I tried including both the project classes and the OkHttp library as targets for AspectJ. While this did produce the woven bytecode classes for OkHttp, it put that output beside the project classes. And then it seems as though the original OkHttp dependency (not woven) ended up being selected during APK assembly. So at runtime, the unwoven classes were still used.

I spent a bunch of time seeing if I could rewire the Gradle tasks a bit, but hit a wall.

I still think this is doable, though. I believe the correct way to approach this is to make use of Gradle Transforms. Transforms essentially let you modify dependency libraries. This is actually how Android's Jetifier works.

For the short term, I'm afraid I don't have a solution ready to go. However, I do think the Transform route is something we should pursue. So I will work with my team to define this work, and find a time to introduce it. With that in mind, I think it makes sense to keep this issue open for now.

Sorry I couldn't find a quick win for you.

mlilienberg commented 3 years ago

Thanks for all the energy you put into this SDK, even with this limitation it is already amazing. I was aiming for a quick win but if this is possible in the future, it would be awesome.

baolongnt commented 2 years ago

I opened a PR that may help here https://github.com/Ibotta/gradle-aspectj-pipeline-plugin/pull/18

My need is that I wrote a library that contains the AspectJ rules + UI to instrument code that is in our other apps.

mbhaskar98 commented 1 week ago

One way of achieving it (of-course with some limitations) https://github.com/mbhaskar98/okhttp-aop