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

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

Open
surbhiia opened this issue Oct 23, 2024 · 5 comments

Comments

@surbhiia
Copy link
Contributor

surbhiia commented Oct 23, 2024

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

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

surbhiia commented Nov 4, 2024

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

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

surbhiia commented Nov 6, 2024

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

@surbhiia
Copy link
Contributor Author

surbhiia commented Nov 8, 2024

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:

  • We can remove the singleton object fetch from AndroidInstrumentationLoader. Keeping it just an interface with getInstrumentationByType() and getAllInstrumentations() method declarations. Instead of getInstrumentation method we'll directly call getInstrumentationByType() method on created AndroidInstrumentationLoader object.
  • AndroidInstrumentationLoaderImpl would be the implementation for it. Where we can load the instrumentation map on object creation. (I think we do not need to keep it in internal folder as this is also something others can use directly as an implementation of the interface if they want to not utilize the core and utilize the individual instrumentations independently)

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

Let me know what you all think! :)

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

No branches or pull requests

2 participants