open-telemetry / opentelemetry-android

OpenTelemetry Tooling for Android
Apache License 2.0
159 stars 41 forks source link

Extract AndroidInstrumentation Interface and related classes into their own module + update install method to take generic OpenTelemetry instance #658

Open surbhiia opened 1 month ago

surbhiia commented 1 month ago

Right now all the instrumentations implement the AndroidInstrumentation Interface which resides in the bigger core module. If suppose, some vendors just want to utilize the individual instrumentation modules like OkHttp3 / HttpURLConnection without all of the core SDK, they'd be dependent on the bigger core module without needing it. So, it would be helpful to have the AndroidInstrumentation Interface and related interface (AndroidInstrumentationLoader) and classes (AndroidInstrumentationLoaderImpl) reside in their own separate module.

Secondly, If a vendor does not want to use OpenTelemetryRum Interface and just the individual instrumentations, they'd not be able to do that because the AndroidInstrumentation install method requires an OpenTelemetryRum instance. We should change it to generic OpenTelemetry instance instead. For example: if I want to utilize the HttpURLConnection instrumentation, I'd have to do the following:

OpenTelemetryRum oTelRumInstance = OpenTelemetryRum.builder(application).build();

HttpUrlInstrumentation instrumentation = new HttpUrlInstrumentation(); 

// or HttpUrlInstrumentation instrumentation = AndroidInstrumentationLoader.getInstrumentation(HttpUrlInstrumentation.class);

instrumentation.install(application, oTelRumInstance);

Instead, I should be able to do the following:

  HttpUrlInstrumentation instrumentation = new HttpUrlInstrumentation();

    instrumentation.install(application, myOpenTelemetryInstance); 

For some of the other instrumentations that depend on some services, there'd still be dependence on the core. For example, ANR instrumentation depends on AppLifecycle Service. I plan to create another issue/proposal to extract services out in their own separate modules as well so vendors can plug-in the services they need only - based on the instrumentations they're utilizing.

I'm happy to make this code change once we have a consensus on this requirement. :)

surbhiia commented 1 month ago

Created issue #669 to capture extraction of 4 services in their own modules and extracting the two classes in the services folder to some other relevant modules.

surbhiia commented 3 weeks ago

Hi @LikeTheSalad, @bidetofevil, @marandaneto ! Reaching out to gather any opinions you guys might have on the first part of this issue - "it would be helpful to have the AndroidInstrumentation Interface and related interface (AndroidInstrumentationLoader) and classes (AndroidInstrumentationLoaderImpl) reside in their own separate module."

Due to a holiday tomorrow for me, won't be able to join Android Sig so collecting some asynchronous feedback here so we can make progress on this one. Thanks a lot! :)

LikeTheSalad commented 3 weeks ago

Hi @surbhiia

I think it makes sense since we're moving towards allowing users to use auto-instrumentations independently. It seems like this might not be the only responsibility that we should extract from core, as I've noticed some mentions of doing something similar for services too. I think it's doable, although I'd like to discuss the details in a SIG meeting.

surbhiia commented 3 weeks ago

Thanks @LikeTheSalad for your feedback! I'll bring it up for discussion in the next android SIG meeting. :)

surbhiia commented 3 weeks ago

Currently, AndroidInstrumentation classes are housed as follows: -> core -> instrumentation -> AndroidInstrumentation -> core -> instrumentation -> InstallationContext -> core -> instrumentation -> AndroidInstrumentationLoader -> core -> internal -> instrumentation -> AndroidInstrumentationLoaderImpl

Proposal to move them as follows as an independent module: -> instrumentation -> android-instrumentation (new module) -> All 4 classes

The test classes under core -> test -> instrumentation would also be moved to the new module and core will now depend on the new module.

Changes to classes: Taking inspiration from recent changes @breedx-splk has PRed (in above linked PR 671) for ServiceManager:

This is mostly a straightforward change. Will propose other modularization changes to core in the services issue.

Let me know what you all think! :)