Relax the ServiceManager singleton nature #671

breedx-splk · 2024-10-30T04:07:23Z

As noted elsewhere and discussed in the Android SIG today, the static singleton nature of the ServiceManager both causes modularity challenges and makes testing in vendor distros that wrap the OpenTelmetryRumBuilder difficult if not impossible. So this makes a first attempt at the following:

Eliminate the static singleton nature of the ServiceManager. This primarily means that any class that depends on this must have it injected (via construction). There is still only one instance used at runtime, but no longer can classes reach out statically to randomly call ServiceManager.get() in an ad-hoc manner.
Allows a custom ServiceManager instance to be injected into the OpenTelemetryRumBuilder in case the (power) user needs to override some defaults or provide their own implementation of a service.
Several instrumentations need to get access to services during installation time, so the instrumentation API needed to change to accommodate this. As a result, we now have an InstallationContext data class in the instrumentation package that bundles up the app, the otel instance, and service manager instance.
Unfortunately, some of the disk buffering configuration ended up hitting the static ServiceManager.get() singleton to get the PeriodicWorkService when creating the default implementations of the (from disk) export scheduler. I decided that having functional components as "configuration" was probably stupid, so I just pulled them out all the way up to the builder.
The list of services that are implied/default is no longer coupled inside the ServiceManager interface, but instead fixed in the sole implementation: ServiceManagerImpl.

Two things that this PR does not yet do that will need to be addressed in follow-up efforts:

ServiceManager interface is still marked as internal and lives in an internal package. This is problematic because the OTRB now has a public interface method that can take a ServiceManager instance. Maybe we just make the interface no longer internal?
As @surbhiia has pointed out elsewhere, the list of created and started services still doesn't map well or appropriately to the requirements of the actual instrumentation. In other words, there could still be services created and started that are not used by any instrumentation or exporters or anything.

In any case, I think the testability is greatly improved and the lifecycle is less complex.

… time.

… setter with same sane defaults.

…face and into impl

surbhiia · 2024-10-30T21:21:34Z

core/src/main/java/io/opentelemetry/android/instrumentation/InstallationContext.kt

+data class InstallationContext(
+    val application: Application,
+    val openTelemetry: OpenTelemetry,
+    val serviceManager: ServiceManager,


Can we make the application and serviceManager nullable as they're not needed in all instrumentations?

Possibly, but I think we mostly want to use the same context when installing all instrumentations at startup -- in which case some of the instrumentations will need some of these, so they won't be null.

I think it's simpler if instrumentations just ignore (don't use) the parts they don't need.

That makes sense! We can perhaps provide a getter for a no-op ServiceManager instance to support the use cases when a user is utilizing one/some of the many instrumentations like the okhttp3 instrumentation that does not require a ServiceManager?

Or we can skip that too assuming such users can bypass the AndroidInstrumentation interface and directly call the underlying code under install method but providing a no-op could be beneficial for users who want to leverage the
AndroidInstrumentation interface too for wrapping in their own discover instrumentations feature. :)

Instead of dealing with nulls, why not make them interfaces that we can provide Fakes to during a test?

If it's all Kotlin, then nulls are fine if it's handled downstream in only a few key places, but because there's still so much Java, I'd go with Fake or no-op implementations so we never ever get NPEs, but also will be able to bypass this during tests. Another advantage of that is there's no need to mock any of these during tests - just instantiate a the fake and you're good to go.

Based on yesterday's SIG meeting, we should try to find a way to allow instrumentations to reuse services/utilities without having to expose them as public apis. There's more details about it all on this issue.

LikeTheSalad · 2024-11-05T15:01:38Z

core/src/main/java/io/opentelemetry/android/instrumentation/AndroidInstrumentation.kt

-        application: Application,
-        openTelemetry: OpenTelemetry,
-    )
+    fun install(ctx: InstallationContext)


I like this idea. I still need to take a deeper look at the whole PR so I can provide a better review overall, though this part reminded me of this other PR which makes it a bit confusing to me because there it seemed like we wanted to avoid passing core-specific types to instrumentations? I'm ok with doing so btw, I just want to make sure what's the overall consensus of the kind of use cases we want to support for instrumentations (whether we need them to be fully independent or not).

And when you say "core-specific types" in this case you mean both the InstallationContext itself but also the ServiceManager contained within?

In any case, the api needs to have something to pass to install(), and instrumentations might want to leverage the consistent way we've started setting up to get at services. I'm open to other ways that don't involve global/static/singletons. :)

And when you say "core-specific types" in this case you mean both the InstallationContext itself but also the ServiceManager contained within?

Yes, that's what I was thinking.

In any case, the api needs to have something to pass to install(), and instrumentations might want to leverage the consistent way we've started setting up to get at services. I'm open to other ways that don't involve global/static/singletons. :)

Makes sense, we still need to pass something to instrumentations, and I like this idea of passing a single object of ours because it means that we could add more tools in the future if needed without having to change the signature of install().

I think the solution to this will become clearer once we fully understand the problem, if it's a matter of having "zero dependency" on anything that's outside of the upstream java repo, then things might get quite complicated, however, if it's just a matter of package size concerns and having a lot of code from a dependency that might not be fully used, such as the Android core, then it's probably a matter of extracting the instrumentation-related files out of core, as @surbhiia mentioned here, where we can also add a new type like InstallationContext. If I understood correctly, I think the problem is the latter, so maybe we can get away with just moving this type into the separate "instrumentation API" module. Now, what to do about the service manager is another issue though, I hope we can discuss the details properly in a SIG meeting.

While I agree that there are probably several layers of functionality we can split this out into that will allow the interfaces, singletons, and config objects be more decoupled, it may still be useful to maintain this uber object for init that references all the layers for convenience - the alternative is to have N different objects and have them passed in separately. At that point, it's more of an aesthetic choice, and I don't particular care too much :-)

bidetofevil

LGTM. The change moves the project ahead, so I'm good with it overall (see specific comments for non-blocking suggestions)

bidetofevil · 2024-11-07T23:15:09Z

core/src/main/java/io/opentelemetry/android/OpenTelemetryRumBuilder.java

-        CacheStorage storage = serviceManager.getCacheStorage();
+    @NonNull
+    private ServiceManager getServiceManager() {
+        if (serviceManager == null) {


In theory this should be called only from the main thread, but probably doesn't hurt to synchronize the init/set?

bidetofevil · 2024-11-07T23:22:07Z

core/src/main/java/io/opentelemetry/android/instrumentation/AndroidInstrumentation.kt

-        application: Application,
-        openTelemetry: OpenTelemetry,
-    )
+    fun install(ctx: InstallationContext)


While I agree that there are probably several layers of functionality we can split this out into that will allow the interfaces, singletons, and config objects be more decoupled, it may still be useful to maintain this uber object for init that references all the layers for convenience - the alternative is to have N different objects and have them passed in separately. At that point, it's more of an aesthetic choice, and I don't particular care too much :-)

bidetofevil · 2024-11-07T23:24:21Z

core/src/main/java/io/opentelemetry/android/instrumentation/InstallationContext.kt

+data class InstallationContext(
+    val application: Application,
+    val openTelemetry: OpenTelemetry,
+    val serviceManager: ServiceManager,


Instead of dealing with nulls, why not make them interfaces that we can provide Fakes to during a test?

If it's all Kotlin, then nulls are fine if it's handled downstream in only a few key places, but because there's still so much Java, I'd go with Fake or no-op implementations so we never ever get NPEs, but also will be able to bypass this during tests. Another advantage of that is there's no need to mock any of these during tests - just instantiate a the fake and you're good to go.

breedx-splk · 2024-11-18T23:09:20Z

There's still a lot of open discussion in this PR, but it's been approved for 2 weeks without any blocking reviews, so I'm going to merge to try and keep momentum. Like always, happy to continue revising incrementally as we see fit.

breedx-splk added 14 commits October 29, 2024 15:48

allow setting the ServiceManager and only create if needed at build()…

ef96126

… time.

create InstallationContext

6510fbc

update AndroidInstrumentation.install() to take the InstallationContext.

4028251

decouple instrumentations from ServiceManager singleton

53e2b10

pull static usage up

e4865e6

remove static create method

6c1f307

remove static create() in favor of constructor

530d16f

pull operational object out of disk buffer config and promote to OTRB…

927af23

… setter with same sane defaults.

move opinionated default list of services out of ServiceManager inter…

5571131

…face and into impl

remove static call and just test the impl

d906f89

cleanup

2bcd1e8

eliminate test dep on ServiceManager static usage

486840f

finally remove singleton nature from ServiceManager.

71b56dc

add period

3e388ed

breedx-splk requested a review from a team as a code owner October 30, 2024 04:07

surbhiia reviewed Oct 30, 2024

View reviewed changes

LikeTheSalad reviewed Nov 5, 2024

View reviewed changes

bidetofevil mentioned this pull request Nov 5, 2024

[Do not merge] Agent api prototype #624

Draft

bidetofevil approved these changes Nov 7, 2024

View reviewed changes

breedx-splk merged commit 85e48b8 into open-telemetry:main Nov 18, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Relax the ServiceManager singleton nature #671

Relax the ServiceManager singleton nature #671

breedx-splk commented Oct 30, 2024

surbhiia Oct 30, 2024

breedx-splk Oct 30, 2024

surbhiia Oct 31, 2024 •

edited

Loading

bidetofevil Nov 7, 2024

LikeTheSalad Nov 13, 2024

LikeTheSalad Nov 5, 2024

breedx-splk Nov 6, 2024

LikeTheSalad Nov 7, 2024

bidetofevil Nov 7, 2024

bidetofevil left a comment

bidetofevil Nov 7, 2024

bidetofevil Nov 7, 2024

bidetofevil Nov 7, 2024

breedx-splk commented Nov 18, 2024

Relax the ServiceManager singleton nature #671

Relax the ServiceManager singleton nature #671

Conversation

breedx-splk commented Oct 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

surbhiia Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bidetofevil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

breedx-splk commented Nov 18, 2024

surbhiia Oct 31, 2024 •

edited

Loading