Skip to content

Remote maxReplayBufferSize #487

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

Open
lmeier wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Remote maxReplayBufferSize #487

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 12 additions & 0 deletions sdk/src/main/java/io/radar/sdk/model/RadarSdkConfiguration.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ internal data class RadarSdkConfiguration(
val useForegroundLocationUpdatedAtMsDiff: Boolean = false,
val locationManagerTimeout: Int = 0,
val syncAfterSetUser: Boolean = false,
val maxReplayBufferSize: Int = DEFAULT_MAX_REPLAY_BUFFER_SIZE,
) {
companion object {
private const val MAX_CONCURRENT_JOBS = "maxConcurrentJobs"
Expand All @@ -39,11 +40,20 @@ internal data class RadarSdkConfiguration(
private const val USE_FOREGROUND_LOCATION_UPDATED_AT_MS_DIFF = "useForegroundLocationUpdatedAtMsDiff"
private const val LOCATION_MANAGER_TIMEOUT = "locationManagerTimeout"
private const val SYNC_AFTER_SET_USER = "syncAfterSetUser"
private const val MAX_REPLAY_BUFFER_SIZE = "maxReplayBufferSize"
const val DEFAULT_MAX_REPLAY_BUFFER_SIZE = 120

fun fromJson(json: JSONObject?): RadarSdkConfiguration {
// set json as empty object if json is null, which uses fallback values
val config = json ?: JSONObject();

val maxReplayBufferSize = config.optInt(MAX_REPLAY_BUFFER_SIZE, DEFAULT_MAX_REPLAY_BUFFER_SIZE)
val validatedMaxReplayBufferSize = if (maxReplayBufferSize > 0 && maxReplayBufferSize <= DEFAULT_MAX_REPLAY_BUFFER_SIZE) {
maxReplayBufferSize
} else {
DEFAULT_MAX_REPLAY_BUFFER_SIZE
}
Comment on lines +51 to +55
Copy link

Copilot AI Jan 25, 2026

Choose a reason for hiding this comment

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

The validation logic restricts maxReplayBufferSize to values less than or equal to DEFAULT_MAX_REPLAY_BUFFER_SIZE (120). This defeats the purpose of having a remotely configurable buffer size, as the server cannot increase the buffer size beyond 120. If the intention is to allow remote configuration to adjust the buffer size, consider either removing the upper bound check or documenting why 120 is a hard limit. If 120 is indeed a hard limit due to the LinkedBlockingDeque initialization in RadarSimpleReplayBuffer, that should be addressed as well.

Copilot uses AI. Check for mistakes.

return RadarSdkConfiguration(
config.optInt(MAX_CONCURRENT_JOBS, DEFAULT_MAX_CONCURRENT_JOBS),
config.optBoolean(SCHEDULER_REQUIRES_NETWORK, false),
Expand All @@ -58,6 +68,7 @@ internal data class RadarSdkConfiguration(
config.optBoolean(USE_FOREGROUND_LOCATION_UPDATED_AT_MS_DIFF, false),
config.optInt(LOCATION_MANAGER_TIMEOUT, 0),
config.optBoolean(SYNC_AFTER_SET_USER, false),
validatedMaxReplayBufferSize,
)
}

Expand Down Expand Up @@ -89,6 +100,7 @@ internal data class RadarSdkConfiguration(
putOpt(USE_FOREGROUND_LOCATION_UPDATED_AT_MS_DIFF, useForegroundLocationUpdatedAtMsDiff)
putOpt(LOCATION_MANAGER_TIMEOUT, locationManagerTimeout)
putOpt(SYNC_AFTER_SET_USER, syncAfterSetUser)
putOpt(MAX_REPLAY_BUFFER_SIZE, maxReplayBufferSize)
}
}
}
12 changes: 10 additions & 2 deletions sdk/src/main/java/io/radar/sdk/util/RadarSimpleReplayBuffer.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package io.radar.sdk.util

import io.radar.sdk.RadarSettings
import io.radar.sdk.model.RadarReplay
import io.radar.sdk.model.RadarSdkConfiguration
import android.content.Context
import android.content.SharedPreferences
import androidx.core.content.edit
Expand All @@ -24,11 +25,18 @@ internal class RadarSimpleReplayBuffer(private val context: Context) : RadarRepl
}

override fun write(replayParams: JSONObject) {
if (buffer.size >= MAXIMUM_CAPACITY) {
val sdkConfiguration = RadarSettings.getSdkConfiguration(context)
val maxBufferSize = if (sdkConfiguration.maxReplayBufferSize > 0) {
sdkConfiguration.maxReplayBufferSize
} else {
RadarSdkConfiguration.DEFAULT_MAX_REPLAY_BUFFER_SIZE
}
Comment on lines +29 to +33
Copy link

Copilot AI Jan 25, 2026

Choose a reason for hiding this comment

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

The validation logic here checks if maxReplayBufferSize > 0, but this check is redundant. The fromJson method in RadarSdkConfiguration already ensures that maxReplayBufferSize is always greater than 0 and falls back to DEFAULT_MAX_REPLAY_BUFFER_SIZE if invalid. Consider removing this conditional and using sdkConfiguration.maxReplayBufferSize directly.

Suggested change
val maxBufferSize = if (sdkConfiguration.maxReplayBufferSize > 0) {
sdkConfiguration.maxReplayBufferSize
} else {
RadarSdkConfiguration.DEFAULT_MAX_REPLAY_BUFFER_SIZE
}
val maxBufferSize = sdkConfiguration.maxReplayBufferSize

Copilot uses AI. Check for mistakes.

Copy link

Copilot AI Jan 25, 2026

Choose a reason for hiding this comment

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

The change from if to while is correct for ensuring the buffer size constraint is satisfied when maxBufferSize is reduced. However, when maxBufferSize is significantly smaller than the current buffer.size, this could result in removing many items in a tight loop. Consider adding a comment explaining why a while loop is necessary here, or consider clearing multiple items in a single operation if the difference is large for better performance.

Suggested change
// Use a while-loop (instead of a single if) to ensure the buffer is trimmed
// even when maxReplayBufferSize is reduced to a value much smaller than the
// current buffer size. The loop is bounded by MAXIMUM_CAPACITY, so this
// remains inexpensive in practice.

Copilot uses AI. Check for mistakes.
while (buffer.size >= maxBufferSize) {
buffer.removeFirst()
}

buffer.offer(RadarReplay(replayParams))
val sdkConfiguration = RadarSettings.getSdkConfiguration(context)
if (sdkConfiguration.usePersistence) {
// if buffer length is above 50, remove every fifth replay from the persisted buffer
if (buffer.size > 50) {
Expand Down
5 changes: 5 additions & 0 deletions sdk/src/test/java/io/radar/sdk/model/RadarSdkConfigurationTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ class RadarSdkConfigurationTest {
private var useForegroundLocationUpdatedAtMsDiff = false
private var locationManagerTimeout = 123456
private var syncAfterSetUser = false
private var maxReplayBufferSize = 120

@Before
fun setUp() {
Expand All @@ -51,6 +52,7 @@ class RadarSdkConfigurationTest {
"useForegroundLocationUpdatedAtMsDiff":$useForegroundLocationUpdatedAtMsDiff,
"locationManagerTimeout":$locationManagerTimeout,
"syncAfterSetUser":$syncAfterSetUser,
"maxReplayBufferSize":$maxReplayBufferSize,
}""".trimIndent()
}

Expand All @@ -72,6 +74,7 @@ class RadarSdkConfigurationTest {
useForegroundLocationUpdatedAtMsDiff,
locationManagerTimeout,
syncAfterSetUser,
maxReplayBufferSize,
).toJson().toString()
)
}
Expand All @@ -92,6 +95,7 @@ class RadarSdkConfigurationTest {
assertEquals(useForegroundLocationUpdatedAtMsDiff, settings.useForegroundLocationUpdatedAtMsDiff)
assertEquals(locationManagerTimeout, settings.locationManagerTimeout)
assertEquals(syncAfterSetUser, settings.syncAfterSetUser)
assertEquals(maxReplayBufferSize, settings.maxReplayBufferSize)
}

@Test
Expand All @@ -109,6 +113,7 @@ class RadarSdkConfigurationTest {
assertTrue(settings.useOpenedAppConversion)
assertFalse(settings.useForegroundLocationUpdatedAtMsDiff)
assertEquals(0, settings.locationManagerTimeout)
assertEquals(120, settings.maxReplayBufferSize)
Copy link

Copilot AI Jan 25, 2026

Choose a reason for hiding this comment

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

Test coverage is missing for the validation logic of maxReplayBufferSize. The fromJson method in RadarSdkConfiguration includes validation that clamps values to be between 1 and DEFAULT_MAX_REPLAY_BUFFER_SIZE (120), but there are no tests verifying this behavior. Consider adding tests for edge cases such as: negative values, zero, values greater than 120, and values between 1 and 120 to ensure the validation logic works as intended.

Copilot uses AI. Check for mistakes.
}

private fun String.removeWhitespace(): String = replace("\\s".toRegex(), "")
Expand Down