Adding disk buffering, part 3

[LikeTheSalad]

It should take just one more PR after this one!

This PR is for setting the scaffolding that will allow to periodically read and export previously stored data from the disk. This is needed because the previous PRs related to disk buffering have taken care of the writing part only, so now we're going to set up the reading part which has to happen periodically (and in a background thread) to make sure new data is exported at some point.

Goals for these changes

• Providing an API for scheduling periodic reads/exports of previously stored data.
• The provided API allows for custom implementations so that projects can decide the mechanism they want to use to trigger a read/export action periodically.
• A default implementation for the API is provided with a very basic functionality that ensures the following:
  • Polls and exports the data in disk periodically in the background while the app is running.
  • Doesn't rely on extra dependencies to ensure it won't negatively affect the lib size.
  • Doesn't create permanent running secondary threads if it's not needed to avoid consuming the device's resources unnecessarily.

For the next part

Which seems like it will be the last one, we should take care of:

• Making the default scheduler to read and export the available data on disk.
• Use the disk buffering configuration, when the agent is initialized, to start the scheduling process with the provided scheduler or the default one if no custom scheduler is provided in the configs.

[marandaneto]

Not for this PR:
If using the Kotlin explicitApi mode, all those interfaces have to be made public, then you can use the org.jetbrains.kotlinx.binary-compatibility-validator plugin to generate a public API file its quite nice.

[LikeTheSalad]

That sounds really nice 👍 I think we should create an issue to address it later.

[breedx-splk]

@marandaneto do you mind creating an issue for this? I'm not familiar with it and would love to see an example in action. Thanks!

[marandaneto]

Sure, #217
It's quite simple, the plugin does all the magic, all you need to do is install the plugin and make the commands part of the CI pipeline, so the CI generates the API dump in every CI run if any, the check is already done automatically part of the builtin gradle check.

[marandaneto]

noice!

[breedx-splk]

There do appear to be a lot of moving parts here...There's a PeriodicWorkService that delegates to the WorkDelegator that, when run, delegates all of its enqueued items to be run later in its ExecutorService and also tells a Handler to post a callback run on the MainLooper, which will repeat the process (every 10 seconds).

There's also this PeriodicRunnable which is a Runnable that when run()s checks to see if enough time has passed and yes then it actually runs via the onRun() template method, but it always reenqueues itself back into the PeriodicWorkService global service instance, which it finds though the global ServiceManager instance.

Phew. That's a lot to keep track of for something that we just want to run every 10 seconds.

Oh yeah, and I haven't even mentioned yet that there's the DefaultExportScheduler which is an implementation of PeriodicRunnable.

There's something that's eating at me about the period in the PeriodicRunnable and the scheduled time in the PeriodicWorkService and how they might be at odds with each other. If you have a PeriodicRunnable that wants to be run every second, but the PeriodicWorkService only can schedule work every 10 seconds, then doesn't that effectively make the PeriodicRunnable period 10 seconds as well? In other words, maybe it can only run as fast as its service?

In any case, there is probably something in the merits of using the Handler/Looper mechanism that I need to read up on...but I wonder why we can't just use a plain old ScheduledExecutorService for all of this?

[LikeTheSalad]

@breedx-splk regarding:

There do appear to be a lot of moving parts here...

Yeah it's a lot 😅 - I'll try to go over them one by one in here.

There's a PeriodicWorkService that delegates to the WorkDelegator that, when run, delegates all of its enqueued items to be run later in its ExecutorService

Correct, the PeriodicWorkService tool is supposed to be used for any kind of periodic background work needed by the RUM agent, I think that, by having the background work centralized there, we get more control over how many threads the OTel Android spawns and how many processes are running at a given time, which is important for an Android app because resources are limited, the amount of threads spawned means more memory usage, and the amount of work being done at a time consumes both memory and battery. I know in the near future of other features that might need to run periodically in the background, such as NTP requests and ANR detection, so I thought of adding the scaffolding right away instead of piling up some tech debt and refactorings for later.

The WorkDelegator is just an inner class that implements Runnable and is passed to the Handler. It's not really needed to make PeriodicWorkService well though, it can all be part of PeriodicWorkService's class, is just that I was worried about accidental calls to PeriodicWorkService.run that could bypass the timer.

also tells a Handler to post a callback run on the MainLooper, which will repeat the process (every 10 seconds).

This is related to not abusing of Android app's resources, I believe that whatever work is done by this lib will always be secondary in terms of the host app's priorities, so from a user's perspective, having a lot of secondary background work happening at any time, probably continuously running doesn't seem like a behavior I'd like to get in my app from one of its libs, so that's why I believe there should be a small window of time where it does all it needs to do in the background and then waits for another window. It's common to have some kind of minimum delay between background jobs in Android due to proper resource consumption, like in WorkManager for example.

10 seconds doesn't mean anything special to me though, if we think we need shorter delays I think we can reduce that number.

There's also this PeriodicRunnable

This one's a utility for background tasks that need to happen more than once. But if a task only needs to happen once it can just implement Runnable directly and get enqueued once in the PeriodicWorkService object.

there's the DefaultExportScheduler which is an implementation of PeriodicRunnable.

Yeah the DefaultExportScheduler is an intentionally very simple implementation of ExportScheduler that runs periodically and triggers the disk buffering exporting from the disk while the app is running. Users should be able to create their own ExportScheduler implementation to cover more scenarios if they like, such as triggering periodic exports even when their app isn't running, but I don't think we should add that behavior in here since it would mean adding more dependencies and also we couldn't guarantee that it will always work as expected on every flavor of Android.

If you have a PeriodicRunnable that wants to be run every second, but the PeriodicWorkService only can schedule work every 10 seconds, then doesn't that effectively make the PeriodicRunnable period 10 seconds as well?

Yes, we can lower that number, although I think we should also be cautious about how often we need to do work in the background for the reasons I mentioned earlier.

In any case, there is probably something in the merits of using the Handler/Looper mechanism that I need to read up on...but I wonder why we can't just use a plain old ScheduledExecutorService for all of this?

I think we could also use ScheduledExecutorService, the reason I went with a Handler is to take advantage of an existing thread to do the scheduling to avoid spawning unnecessary ones. It also seems to me that ScheduledExecutorServices don't have a limit on how many threads they can spawn, based on the code inside Executors.newScheduledThreadPool, which is concerning to me due to the same device's resources handling issue, and threads don't seem to be cheap when it comes to memory. Although I could have missed something about ScheduledExecutorService that won't cause more than one thread to be spawned at a time, or at least limit the max amount of parallel ones?

I think it all can be summarized to trying to make this lib a good android lib citizen 😅 - there might be cases where we need to do more work or take more resources but I'd like to keep those as exceptions and try to do most of this lib's work by using as little as possible.

[breedx-splk]

Thanks for the depth of response and taking the time to write it.

Oh that's a good point about the ScheduledExecutorService, it does indeed look like the Executors.newScheduledThreadPool() can't leverage an upper limit. What about Executors.newSingleThreadScheduledExecutor() here then? We probably don't need a lot of concurrency, especially if one of the goals is to reduce resource usage. I dunno.

I do wonder, when it comes to reducing resource usage, if having a recurring task firing when there may not be work to be done is a good idea.

In any case, I don't think any of this is a showstopper. Thanks for putting in all this work. I'll do another pass through and see if I have anything specific, but I remember it looks good overall.

[breedx-splk]

Progress! Looking forward to seeing it all come together in the final episode. :)

[breedx-splk]

Can we make the DefaultExportScheduler an instance field passed thru constructor? Both good DI practice and saves object creation.

[LikeTheSalad]

You're right, that sounds way better! I'll add the changes.

[LikeTheSalad]

Thanks, it's updated now.

[breedx-splk]

@marandaneto do you mind creating an issue for this? I'm not familiar with it and would love to see an example in action. Thanks!

[LikeTheSalad]

I do wonder, when it comes to reducing resource usage, if having a recurring task firing when there may not be work to be done is a good idea.

Good point 😅 it sounds like a good topic for the SIG 👍

[LikeTheSalad]

What about Executors.newSingleThreadScheduledExecutor() here then?

I think any tool that prevents creating more than one thread for now should work yeah, I'm not sure if that's the case even for newSingleThreadScheduledExecutor but I think we can discuss about it further in the SIG, I know it's quite a lot of code for the current approach so I'm happy to replace it with something that requires less code if we find something that's easy on memory consumption.