open-telemetry / opentelemetry-android

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

Extract Services out in their own modules so they can be plugged-in as needed #669

Open surbhiia opened 4 weeks ago

surbhiia commented 4 weeks ago

Currently, services are part of the bigger core module. It would be beneficial to extract each of them in their own separate modules so they can be plugged-in as needed.

Example use case - Some services like PeriodicWorkService are needed if Disk Buffering is used (to export spans stored in disk to backend). If users/ vendors implement their own disk buffering mechanism, they do not need this service.

There are mainly 4 services (AppLifecycle, VisibleScreenService, CurrentNetworkProvider, PeriodicWorkService) and 2 classes (CacheStorage and Preferences). Objects are created from the 2 classes and the objects can be obtained from the service interface but they're not really services.

Following bullets cover what instrumentations rely on what services for more clarity, so we know not all instrumentations require all services

surbhiia commented 2 weeks ago

Currently, services are housed under core -> internal -> services

Changes proposal:

NOTE - One major problem in the above approach is: All instrumentations need InstallationContext (new android-instrumentation module), that module needs service-manager module due to the 3rd argument in InstallationContext of ServiceManager. And service-manager needs dependence on all services due to the getXService() functions. So, in turn anyway all instrumentations depend on all services. So, I think the solution here could be to define another AutoService interface for these services and then we can load and get services without actual class names in the ServiceManager. Similar style to AndroidInstrumentationLoader - getInstrumentationByType() or getAllInstrumentation() methods.

Services as AutoService:

Some other classes from core that are used in these services and any other improvements:

This is an initial draft. Would be thinking through if it fits all our use cases and refining it more as needed. Do share your feedback on any of it. Also, missed proposal for periodic work service. Would add that as well.

LikeTheSalad commented 2 weeks ago

A lot of what's mentioned here makes a lot of sense, although before making any final decisions on the matter, I'd like to point out some aspects of "services" to clarify how they are supposed to help us in general, hoping that we can come up with a solution that would still allow us to rely on them for future use cases.

What are Services and why we need them

Potential issues we should avoid when doing the refactor

Difficult to reuse

Services are meant to be reused, for different reasons which can vary depending on the scope of each service, so I think we need to try and take a look at how we can easily reuse them for future cases.

For example, let's consider PeriodicWorkService. This service is helpful to manage a queue of background work that can be repeated, in a way that reduces the amount of resources that might be needed should we needed to do this from different parts of our agent. For now, it's only used to run periodic exports from the disk by the disk buffering tool, however, most likely in the future we will also require to run periodic network requests to fetch the real time from an NTP server so that distributed tracing works well. If we can reuse PeriodicWorkService for that purpose, not only we wouldn't have to come up with a custom mechanism each time we need to run these kinds of tasks in the background, but also we would be managing the agent's resources better as we can decide from a single place how many secondary threads we would like it to spawn at a time.

For the same use-case of the previous example, we might also need to use the Preferences service, to store the current NTP time and avoid making too many requests to fetch it if it's not needed.

Based on the above, one of my concerns with some of the proposed changes would be that we might move some of the services code, or full classes, to the places where they are needed right now, making it difficult to reuse those tools for future cases.

Easy to overlook

Having the available services in a single place is useful to get an idea of what's available at any time, kind of like having a toolbox when working on a DIY project. I think this can still be accomplished when splitting services into micro-modules, so long as those modules are placed in a single dir. Failing to somehow provide visibility to existing services might cause issues in the future where people start reinventing the wheel in the best case and consuming resources unnecessarily in the worst.

Difficult to scale and maintain

If we decide to place each service in its own micro-module, we might eventually run into a scenario where we get too many modules to maintain, maybe it won't be too much trouble since we use gradle file conventions, though I think it's worth mentioning it just to make sure there's no problem with that, or if maybe we can alternatively group some services if needed to reduce the amount of modules. Also, if it's needed to create a new module just to add a new service I'd be concerned about people deciding to quickly write the code they need in the module they're working on to avoid having to set up a full module, a process that requires creating a set of dirs and files that maybe some people aren't familiarized with. If that ends up happening, then we would lose the benefits of keeping our tools in a single place.

Final thoughts

I think there's a lot of room for improvement for services and I'm up to make changes to them as needed as long as they can still provide the benefits of sharing global tools easily. Probably one way to start could be by trying to see what future usages we might need for them and making sure that those will be easy to accomplish, and for those which we can't think of future usages for now, I think we should come up with a well-documented process of how to make them "globally accessible" if needed in the future.

surbhiia commented 2 weeks ago

Thanks @LikeTheSalad for your detailed review of the proposal and for explaining the points so clearly! :)

A few take-a-ways and some additional thoughts to carry our discussion forward:

Open questions:

LikeTheSalad commented 2 weeks ago

The idea I got from the latest SIG meeting was that, as a nice first step, we could extract all services into their own module. Also, we should ensure that services are lazily initialized, preventing unnecessary initialization for environments where they'll never be used. Plus, we should try and keep all the services internal to avoid having to maintain them as a public API.

surbhiia commented 1 week ago

Building on top of what we've discussed so far here and in the sig meeting:

Let me know what you all think!

bidetofevil commented 1 week ago

Per what we talked about during the SIG meeting, I summarized my thoughts on this in #702.

tl;dr, I think we should support using the instrumentation independently, but do it in such a way that we don't expose any public API or use the service locator pattern to allow apps to provide their own implementation of services.

LikeTheSalad commented 6 days ago

Thanks @surbhiia! I have a question regarding:

  • InstallationContext contains a SharedUtilsProvider object. All instrumentations call the relevant getXUtil()method on that object.

If I understand correctly, since InstallationContext is passed to AndroidInstrumentation.install, and SharedUtilsProvider is the same ServiceManagerImpl but renamed, then it's not clear to me how are we planning to decouple our internal services from the instrumentation API? Since both, AndroidInstrumentation and InstallationContext, are part of the public instrumentation API, any type that they reference is essentially a public API too, which in this case would make SharedUtilsProvider public, along with the "services/utils" it provides.