Skip to content

Remove min cache size and cache size allocation #828

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Feb 21, 2025
Merged

Remove min cache size and cache size allocation #828

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7329cb0
Remove min cache size and available space allocation from DiskManager
deividasstr Feb 18, 2025
02085a4
Remove unused available space and allocation methods
deividasstr Feb 18, 2025
23ad1c1
Modify DiskManager tests
deividasstr Feb 18, 2025
3122bd8
Modify otel rum builder test
deividasstr Feb 18, 2025
1acb086
Revert import conversion to wildcard grouping
deividasstr Feb 18, 2025
f548c70
Remove maxFolderSize calculation caching
deividasstr Feb 19, 2025
506f07b
Fix lint issue
deividasstr Feb 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions core/src/main/java/io/opentelemetry/android/OpenTelemetryRumBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@
import io.opentelemetry.android.internal.processors.ScreenAttributesLogRecordProcessor;
import io.opentelemetry.android.internal.processors.SessionIdLogRecordAppender;
import io.opentelemetry.android.internal.services.CacheStorage;
import io.opentelemetry.android.internal.services.Preferences;
import io.opentelemetry.android.internal.services.Services;
import io.opentelemetry.android.internal.services.periodicwork.PeriodicWork;
import io.opentelemetry.android.internal.session.SessionIdTimeoutHandler;
Expand Down Expand Up @@ -446,10 +445,9 @@ public OpenTelemetryRumBuilder setExportScheduleHandler(
}

private StorageConfiguration createStorageConfiguration(Services services) throws IOException {
Preferences preferences = services.getPreferences();
CacheStorage storage = services.getCacheStorage();
DiskBufferingConfig config = this.config.getDiskBufferingConfig();
DiskManager diskManager = new DiskManager(storage, preferences, config);
DiskManager diskManager = new DiskManager(storage, config);
return StorageConfiguration.builder()
.setRootDir(diskManager.getSignalsBufferDir())
.setMaxFileSize(diskManager.getMaxCacheFileSize())
Expand Down
45 changes: 7 additions & 38 deletions core/src/main/java/io/opentelemetry/android/internal/features/persistence/DiskManager.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,11 @@ import android.util.Log
import io.opentelemetry.android.common.RumConstants
import io.opentelemetry.android.features.diskbuffering.DiskBufferingConfig
import io.opentelemetry.android.internal.services.CacheStorage
import io.opentelemetry.android.internal.services.Preferences
import java.io.File
import java.io.IOException

internal class DiskManager(
private val cacheStorage: CacheStorage,
private val preferences: Preferences,
private val diskBufferingConfig: DiskBufferingConfig,
) {
@get:Throws(IOException::class)
Expand All @@ -37,48 +35,21 @@ internal class DiskManager(

val maxFolderSize: Int
/**
* It checks for the available cache space in disk, then it attempts to divide by 3 in order to
* get each signal's folder max size. The resulting value is subtracted the max file size value
* in order to account for temp files used during the reading process.
* It divides the requested cache size by 3 in order to
* get each signal's folder max size.
*
* @return If the calculated size is < the max file size value, it returns 0. The calculated
* size is stored in the preferences and returned otherwise.
* @return The calculated size is stored in the preferences and returned.
*/
get() {
val storedSize = preferences.retrieveInt(MAX_FOLDER_SIZE_KEY, -1)
if (storedSize > 0) {
Log.d(
RumConstants.OTEL_RUM_LOG_TAG,
String.format("Returning max folder size from preferences: %s", storedSize),
)
return storedSize
}
val requestedSize = diskBufferingConfig.maxCacheSize
val availableCacheSize =
cacheStorage.ensureCacheSpaceAvailable(requestedSize.toLong()).toInt()
// Divides the available cache size by 3 (for each signal's folder) and then subtracts the
// size of a single file to be aware of a temp file used when reading data back from the
// disk.
val maxCacheFileSize = maxCacheFileSize
val calculatedSize = availableCacheSize / 3 - maxCacheFileSize
if (calculatedSize < maxCacheFileSize) {
Log.w(
RumConstants.OTEL_RUM_LOG_TAG,
String.format(
"Insufficient folder cache size: %s, it must be at least: %s",
calculatedSize,
maxCacheFileSize,
),
)
return 0
}
preferences.store(MAX_FOLDER_SIZE_KEY, calculatedSize)

// Divides the available cache size by 3 (for each signal's folder)
val calculatedSize = requestedSize / 3
Copy link
Contributor

@LikeTheSalad LikeTheSalad Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably we would no longer need to store this calculation since it's not a heavy one anymore.

Copy link
Contributor

@breedx-splk breedx-splk Feb 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@deividasstr want to pick this up in this PR or the next?

Copy link
Contributor Author

@deividasstr deividasstr Feb 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! I will do it in this one.
Just in case, I noticed that this value here is used by another sdk, disk buffering. There shouldn't be no negative backwards compatibility consequence to change cache size on existing files, right?

Copy link
Contributor

@LikeTheSalad LikeTheSalad Feb 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be fine. The size config number is read before writing to ensure that we won't take more space than needed, and when the used space plus the one that is about to be written goes over said number, disk buffering clears the oldest data before doing the writing. So to your question on what would happen if the cache size changes, if the new value is higher, then the existing oldest data will remain there longer, and if it's shorter, then more older data will be cleared in the next write (unless it gets exported first).

Log.d(
RumConstants.OTEL_RUM_LOG_TAG,
String.format(
"Requested cache size: %s, available cache size: %s, folder size: %s",
"Requested cache size: %s, folder size: %s",
requestedSize,
availableCacheSize,
calculatedSize,
),
)
Expand All @@ -89,8 +60,6 @@ internal class DiskManager(
get() = diskBufferingConfig.maxCacheFileSize

companion object {
private const val MAX_FOLDER_SIZE_KEY = "max_signal_folder_size"

private fun deleteFiles(dir: File) {
val files = dir.listFiles()
if (files != null) {
Expand Down
7 changes: 1 addition & 6 deletions core/src/test/java/io/opentelemetry/android/OpenTelemetryRumBuilderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,9 @@
import static io.opentelemetry.semconv.incubating.SessionIncubatingAttributes.SESSION_ID;
import static org.awaitility.Awaitility.await;
import static org.mockito.ArgumentMatchers.anyCollection;
import static org.mockito.ArgumentMatchers.anyLong;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.ArgumentMatchers.isA;
import static org.mockito.Mockito.doAnswer;
import static org.mockito.Mockito.doReturn;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.verify;
Expand Down Expand Up @@ -417,9 +415,7 @@ public void setLogRecordExporterCustomizer() {

@Test
public void diskBufferingEnabled() {
Services services = createAndSetServiceManager();
CacheStorage cacheStorage = services.getCacheStorage();
doReturn(60 * 1024 * 1024L).when(cacheStorage).ensureCacheSpaceAvailable(anyLong());
createAndSetServiceManager();
OtelRumConfig config = buildConfig();
ExportScheduleHandler scheduleHandler = mock();
config.setDiskBufferingConfig(new DiskBufferingConfig(true));
Expand Down Expand Up @@ -447,7 +443,6 @@ public void diskBufferingEnabled_when_exception_thrown() {
Services services = createAndSetServiceManager();
CacheStorage cacheStorage = services.getCacheStorage();
ExportScheduleHandler scheduleHandler = mock();
doReturn(60 * 1024 * 1024L).when(cacheStorage).ensureCacheSpaceAvailable(anyLong());
doAnswer(
invocation -> {
throw new IOException();
Expand Down
51 changes: 3 additions & 48 deletions core/src/test/java/io/opentelemetry/android/internal/features/persistence/DiskManagerTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,11 @@ package io.opentelemetry.android.internal.features.persistence

import io.mockk.Called
import io.mockk.MockKAnnotations
import io.mockk.Runs
import io.mockk.clearMocks
import io.mockk.confirmVerified
import io.mockk.every
import io.mockk.impl.annotations.MockK
import io.mockk.just
import io.mockk.verify
import io.opentelemetry.android.features.diskbuffering.DiskBufferingConfig
import io.opentelemetry.android.internal.services.CacheStorage
import io.opentelemetry.android.internal.services.Preferences
import io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.assertThat
import org.junit.jupiter.api.Assertions.assertTrue
import org.junit.jupiter.api.BeforeEach
Expand All @@ -28,9 +23,6 @@ internal class DiskManagerTest {
@MockK
lateinit var cacheStorage: CacheStorage

@MockK
lateinit var preferences: Preferences

@MockK
lateinit var diskBufferingConfig: DiskBufferingConfig

Expand All @@ -43,7 +35,7 @@ internal class DiskManagerTest {
MockKAnnotations.init(this)
every { cacheStorage.cacheDir }.returns(cacheDir)
diskManager =
DiskManager(cacheStorage, preferences, diskBufferingConfig)
DiskManager(cacheStorage, diskBufferingConfig)
}

@Test
Expand Down Expand Up @@ -90,48 +82,11 @@ internal class DiskManagerTest {
val maxCacheFileSize = 1024 * 1024 // 1 MB
every { diskBufferingConfig.maxCacheSize }.returns(maxCacheSize.toInt())
every { diskBufferingConfig.maxCacheFileSize }.returns(maxCacheFileSize)
every { cacheStorage.ensureCacheSpaceAvailable(maxCacheSize) }.returns(maxCacheSize)
every { preferences.retrieveInt(MAX_FOLDER_SIZE_KEY, -1) }.returns(-1)
every { preferences.store(any(), any()) } just Runs

// Expects the size of a single signal type folder minus the size of a cache file, to use as
// temporary space for reading.
val expected = 2446677
// Expects the size of a single signal type folder, to use as temporary space for reading.
val expected = (maxCacheSize / 3).toInt()
assertThat(diskManager.maxFolderSize).isEqualTo(expected)
verify {
preferences.store(MAX_FOLDER_SIZE_KEY, expected)
}

// On a second call, should get the value from the preferences.
clearMocks(cacheStorage, diskBufferingConfig, preferences)
every { preferences.retrieveInt(MAX_FOLDER_SIZE_KEY, -1) }.returns(expected)
assertThat(diskManager.maxFolderSize).isEqualTo(expected)

verify {
preferences.retrieveInt(MAX_FOLDER_SIZE_KEY, -1)
}
confirmVerified(preferences)
verify { cacheStorage wasNot Called }
verify { diskBufferingConfig wasNot Called }
}

@Test
fun `max folder size is used when calculated size is invalid`() {
val maxCacheSize = (1024 * 1024).toLong() // 1 MB
val maxCacheFileSize = 1024 * 1024 // 1 MB
every { diskBufferingConfig.maxCacheSize }.returns(maxCacheSize.toInt())
every { diskBufferingConfig.maxCacheFileSize }.returns(maxCacheFileSize)
every { cacheStorage.ensureCacheSpaceAvailable(maxCacheSize) }.returns(maxCacheSize)
every { preferences.retrieveInt(MAX_FOLDER_SIZE_KEY, -1) }.returns(-1)
// Expects the size of a single signal type folder minus the size of a cache file, to use as
// temporary space for reading.
assertThat(diskManager.maxFolderSize).isEqualTo(0)
verify(inverse = true) {
preferences.store(any(), any())
}
}

companion object {
private const val MAX_FOLDER_SIZE_KEY = "max_signal_folder_size"
}
}
58 changes: 0 additions & 58 deletions services/src/main/java/io/opentelemetry/android/internal/services/CacheStorage.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,7 @@
package io.opentelemetry.android.internal.services;

import android.content.Context;
import android.os.Build;
import android.os.storage.StorageManager;
import android.util.Log;
import androidx.annotation.RequiresApi;
import androidx.annotation.WorkerThread;
import io.opentelemetry.android.common.RumConstants;
import java.io.File;
import java.io.IOException;
import java.util.UUID;

/**
* Utility to get information about the host app.
Expand All @@ -32,54 +24,4 @@ public CacheStorage(Context appContext) {
public File getCacheDir() {
return appContext.getCacheDir();
}

/**
* Checks for available cache space in the device and compares it to the max amount needed, if
* the available space is lower than the provided max value, the available space is returned,
* otherwise, the provided amount is returned.
*
* <p>On Android OS with API level 26 and above, it will also ask the OS to clear stale cache
* from the device in order to make room for the provided max value if needed.
*/
@WorkerThread
public long ensureCacheSpaceAvailable(long maxSpaceNeeded) {
File cacheDir = getCacheDir();
if (android.os.Build.VERSION.SDK_INT < android.os.Build.VERSION_CODES.O) {
return getLegacyAvailableSpace(cacheDir, maxSpaceNeeded);
}
return getAvailableSpace(cacheDir, maxSpaceNeeded);
}

@RequiresApi(api = Build.VERSION_CODES.O)
private long getAvailableSpace(File directory, long maxSpaceNeeded) {
Log.d(
RumConstants.OTEL_RUM_LOG_TAG,
String.format(
"Getting available space for %s, max needed is: %s",
directory, maxSpaceNeeded));
try {
StorageManager storageManager = appContext.getSystemService(StorageManager.class);
UUID appSpecificInternalDirUuid = storageManager.getUuidForPath(directory);
// Get the minimum amount of allocatable space.
long spaceToAllocate =
Math.min(
storageManager.getAllocatableBytes(appSpecificInternalDirUuid),
maxSpaceNeeded);
// Ensure the space is available by asking the OS to clear stale cache if needed.
storageManager.allocateBytes(appSpecificInternalDirUuid, spaceToAllocate);
return spaceToAllocate;
} catch (IOException e) {
Log.w(RumConstants.OTEL_RUM_LOG_TAG, "Failed to get available space", e);
return getLegacyAvailableSpace(directory, maxSpaceNeeded);
}
}

private long getLegacyAvailableSpace(File directory, long maxSpaceNeeded) {
Log.d(
RumConstants.OTEL_RUM_LOG_TAG,
String.format(
"Getting legacy available space for %s max needed is: %s",
directory, maxSpaceNeeded));
return Math.min(directory.getUsableSpace(), maxSpaceNeeded);
}
}
Loading