Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
mlilienberg opened this issue Jul 13, 2021 · 11 comments
Labels
enhancement New feature or request

Comments

@mlilienberg
Copy link

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
Copy link
Contributor

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
Copy link
Contributor

jdvp commented Jul 14, 2021

I think likely related to #2

@bsautner
Copy link

bsautner commented Jul 14, 2021

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
Copy link
Author

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
Copy link
Contributor

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

@eschlenz
Copy link
Contributor

eschlenz commented Jul 14, 2021

@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
Copy link
Author

@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
Copy link
Contributor

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
Copy link
Author

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.

@eschlenz eschlenz added the enhancement New feature or request label Aug 10, 2021
@baolongnt
Copy link

I opened a PR that may help here
#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
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

6 participants