Skip to content

Commit

Permalink
Use remote limit as ceiling for network call limits and apply consist…
Browse files Browse the repository at this point in the history 
…ently (#78)

## Goal

There were two problems that were addressed in this PR: 

1. The initial issue was that if a remote config is not defined for network behaviour, the local settings on network call limits will be used as is, which means customers can set the limit to whatever they want, which sort of defeats the purpose of having a limit to prevent runaway payload size.
2. The second that I found while working on this is that we are clearing the limits prematurely, every time we cache a session. This means the window in which the limit will be applied before resetting was impossibly short.

This PR addresses both issues by using the default limits in the remote config (or defaults implied by the absence of a config) to act as the ceiling to whatever is set locally. This make the local setting a way of further limit what is set on the remote, and won't allow the overriding of it. I also removed the erroneous limit clearing, which ironically is probably the source of the original customer issue anyway, just obscured by what I found with the local limits

## Testing

Added more test cases to verify the fixed behaviour as well as corner cases not covered previously

## Release Notes

**WHAT**:<br>
Properly enforce network call per session limit and use the remote limit as a ceiling for any local limits set.
**WHY**:<br>
Limiting the number of session calls recorded prevents session payloads from growing to an unhealthy size.
  • Loading branch information
@bidetofevil
bidetofevil authored Nov 16, 2023
2 parents 901d327 + 010968e commit 989dbf2
Show file tree
Hide file tree
Showing 4 changed files with 382 additions and 177 deletions.
45 changes: 28 additions & 17 deletions ...ndroid-sdk/src/main/java/io/embrace/android/embracesdk/config/behavior/NetworkBehavior.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import io.embrace.android.embracesdk.config.local.SdkLocalConfig
import io.embrace.android.embracesdk.config.remote.NetworkCaptureRuleRemoteConfig
import io.embrace.android.embracesdk.config.remote.RemoteConfig
import java.util.regex.Pattern
import kotlin.math.min

/**
* Provides the behavior that functionality relating to network call capture should follow.
Expand Down Expand Up @@ -65,31 +66,36 @@ internal class NetworkBehavior(
local?.networking?.enableNativeMonitoring ?: ENABLE_NATIVE_MONITORING_DEFAULT

/**
* List of domains to be limited for tracking.
* Map of limits being enforced for each domain suffix for the maximum number of requests that are logged given that suffix. The
* algorithm to generate the limits for each domain suffix is as follows:
*
* - Use the domain-suffix-specific settings defined in the remote config as a base.
* - For suffixes where there is both local and remote entries, use the local limit if it is smaller than the remote one
* - For suffixes with only a local entry, apply the local limit or the ceiling defined by the default limit on the remote,
* which ever is smaller.
*/
fun getNetworkCallLimitsPerDomain(): Map<String, Int> {
return remote?.networkConfig?.domainLimits
?: transformLocalDomainCfg()
?: HashMap()
}

private fun transformLocalDomainCfg(): Map<String, Int>? {
val mergedLimits: MutableMap<String, Int> = HashMap()
for (domain in local?.networking?.domains ?: return null) {
if (domain.domain != null && domain.limit != null) {
mergedLimits[domain.domain] = domain.limit
fun getNetworkCallLimitsPerDomainSuffix(): Map<String, Int> {
val limitCeiling = getLimitCeiling()
val domainSuffixLimits: MutableMap<String, Int> = remote?.networkConfig?.domainLimits?.toMutableMap() ?: mutableMapOf()

local?.networking?.domains?.forEach { localLimit ->
if (localLimit.domain != null && localLimit.limit != null) {
domainSuffixLimits[localLimit.domain] =
domainSuffixLimits[localLimit.domain]?.let { remoteLimit ->
min(remoteLimit, localLimit.limit)
} ?: min(limitCeiling, localLimit.limit)
}
}
return mergedLimits

return domainSuffixLimits
}

/**
* Gets the capture limit for network calls.
* Gets the default limit for network calls for all domains where the limit is not specified.
*/
fun getNetworkCaptureLimit(): Int {
return remote?.networkConfig?.defaultCaptureLimit
?: local?.networking?.defaultCaptureLimit
?: DEFAULT_NETWORK_CALL_LIMIT
val remoteDefault = getLimitCeiling()
return min(remoteDefault, local?.networking?.defaultCaptureLimit ?: remoteDefault)
}

/**
Expand Down Expand Up @@ -129,4 +135,9 @@ internal class NetworkBehavior(
* Gets the rules for capturing network call bodies
*/
fun getNetworkCaptureRules(): Set<NetworkCaptureRuleRemoteConfig> = remote?.networkCaptureRules ?: emptySet()

/**
* Cap the default limit at whatever the default limit is that is set or implied by the remote config
*/
private fun getLimitCeiling(): Int = remote?.networkConfig?.defaultCaptureLimit ?: DEFAULT_NETWORK_CALL_LIMIT
}
98 changes: 40 additions & 58 deletions ...c/main/java/io/embrace/android/embracesdk/network/logging/EmbraceNetworkLoggingService.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,11 +40,17 @@ internal class EmbraceNetworkLoggingService(

private val networkCallCache = CacheableValue<List<NetworkCallV2>> { callsStorageLastUpdate.get() }

private val domainSettings = ConcurrentHashMap<String, DomainSettings>()
private val domainSetting = ConcurrentHashMap<String, DomainSettings>()

private val callsPerDomain = hashMapOf<String, DomainCount>()
private val callsPerDomainSuffix = hashMapOf<String, DomainCount>()

private val ipAddressCount = AtomicInteger(0)
private val ipAddressNetworkCallCount = AtomicInteger(0)

private val untrackedNetworkCallCount = AtomicInteger(0)

private var defaultPerDomainSuffixCallLimit = configService.networkBehavior.getNetworkCaptureLimit()

private var domainSuffixCallLimits = configService.networkBehavior.getNetworkCallLimitsPerDomainSuffix()

override fun getNetworkCallsForSession(): NetworkSessionV2 {
val calls = networkCallCache.value {
Expand All @@ -57,7 +63,7 @@ internal class EmbraceNetworkLoggingService(
val cachedCallsSize = calls.size

val overLimit = hashMapOf<String, DomainCount>()
for ((key, value) in callsPerDomain) {
for ((key, value) in callsPerDomainSuffix) {
if (value.requestCount > value.captureLimit) {
overLimit[key] = value
}
Expand All @@ -68,8 +74,6 @@ internal class EmbraceNetworkLoggingService(
logger.logError(msg, IllegalStateException(msg), true)
}

// clear calls per domain and session network calls lists before be used by the next session
callsPerDomain.clear()
return NetworkSessionV2(calls, overLimit)
}

Expand Down Expand Up @@ -114,7 +118,6 @@ internal class EmbraceNetworkLoggingService(
}

processNetworkCall(callId, networkCall)
storeSettings(url)
}

override fun logNetworkError(
Expand Down Expand Up @@ -156,7 +159,6 @@ internal class EmbraceNetworkLoggingService(
)
}
processNetworkCall(callId, networkCall)
storeSettings(url)
}

/**
Expand All @@ -166,88 +168,66 @@ internal class EmbraceNetworkLoggingService(
* @param networkCall that is going to be captured
*/
private fun processNetworkCall(callId: String, networkCall: NetworkCallV2) {
// Get the domain, if it can be successfully parsed
// Get the domain, if it can be successfully parsed. If not, don't log this call.
val domain = networkCall.url?.let {
getDomain(it)
}

if (domain == null) {
logger.logDeveloper("EmbraceNetworkLoggingService", "Domain is not present")
return
}

logger.logDeveloper("EmbraceNetworkLoggingService", "Domain: $domain")
} ?: return

if (isIpAddress(domain)) {
logger.logDeveloper("EmbraceNetworkLoggingService", "Domain is an ip address")
val captureLimit = configService.networkBehavior.getNetworkCaptureLimit()

if (ipAddressCount.getAndIncrement() < captureLimit) {
// only capture if the ipAddressCount has not exceeded defaultLimit
logger.logDeveloper("EmbraceNetworkLoggingService", "capturing network call")
if (ipAddressNetworkCallCount.getAndIncrement() < defaultPerDomainSuffixCallLimit) {
storeNetworkCall(callId, networkCall)
} else {
logger.logDeveloper("EmbraceNetworkLoggingService", "capture limit exceeded")
}
return
} else if (!domainSetting.containsKey(domain)) {
createLimitForDomain(domain)
}

val settings = domainSettings[domain]
val settings = domainSetting[domain]
if (settings == null) {
logger.logDeveloper("EmbraceNetworkLoggingService", "no domain settings")
storeNetworkCall(callId, networkCall)
// Not sure how this is possible, but in case it is, limit logged logs where we can't figure out the settings to apply
if (untrackedNetworkCallCount.getAndIncrement() < defaultPerDomainSuffixCallLimit) {
storeNetworkCall(callId, networkCall)
}
} else {
val suffix = settings.suffix
val limit = settings.limit
var count = callsPerDomain[suffix]
var countPerSuffix = callsPerDomainSuffix[suffix]

if (count == null) {
count = DomainCount(1, limit)
if (countPerSuffix == null) {
countPerSuffix = DomainCount(0, limit)
}

// Exclude if the network call exceeds the limit
if (count.requestCount < limit) {
if (countPerSuffix.requestCount < limit) {
storeNetworkCall(callId, networkCall)
} else {
logger.logDeveloper("EmbraceNetworkLoggingService", "capture limit exceeded")
}

// Track the number of calls for each domain (or configured suffix)
suffix?.let {
callsPerDomain[it] = DomainCount(count.requestCount + 1, limit)
callsPerDomainSuffix[it] = DomainCount(countPerSuffix.requestCount + 1, limit)
logger.logDeveloper(
"EmbraceNetworkLoggingService",
"Call per domain $domain ${count.requestCount + 1}"
"Call per domain $domain ${countPerSuffix.requestCount + 1}"
)
}
}
}

private fun storeSettings(url: String) {
private fun createLimitForDomain(domain: String) {
try {
val mergedLimits = configService.networkBehavior.getNetworkCallLimitsPerDomain()

val domain = getDomain(url)
if (domain == null) {
logger.logDeveloper("EmbraceNetworkLoggingService", "Domain not present")
return
}
if (domainSettings.containsKey(domain)) {
logger.logDeveloper("EmbraceNetworkLoggingService", "No settings for $domain")
return
}

for ((key, value) in mergedLimits) {
for ((key, value) in domainSuffixCallLimits) {
if (domain.endsWith(key)) {
domainSettings[domain] = DomainSettings(value, key)
return
domainSetting[domain] = DomainSettings(value, key)
}
}

val defaultLimit = configService.networkBehavior.getNetworkCaptureLimit()
domainSettings[domain] = DomainSettings(defaultLimit, domain)
if (!domainSetting.containsKey(domain)) {
domainSetting[domain] = DomainSettings(defaultPerDomainSuffixCallLimit, domain)
}
} catch (ex: Exception) {
logger.logDebug("Failed to determine limits for URL: $url", ex)
logger.logDebug("Failed to determine limits for domain: $domain", ex)
}
}

Expand All @@ -266,11 +246,13 @@ internal class EmbraceNetworkLoggingService(
}

override fun cleanCollections() {
domainSettings.clear()
callsPerDomain.clear()
domainSetting.clear()
callsPerDomainSuffix.clear()
ipAddressNetworkCallCount.set(0)
untrackedNetworkCallCount.set(0)
clearNetworkCalls()
// reset counters
ipAddressCount.set(0)
logger.logDeveloper("EmbraceNetworkLoggingService", "Collections cleaned")
// re-fetch limits in case they changed since they last time they were fetched
defaultPerDomainSuffixCallLimit = configService.networkBehavior.getNetworkCaptureLimit()
domainSuffixCallLimits = configService.networkBehavior.getNetworkCallLimitsPerDomainSuffix()
}
}
29 changes: 24 additions & 5 deletions ...id-sdk/src/test/java/io/embrace/android/embracesdk/config/behavior/NetworkBehaviorTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ internal class NetworkBehaviorTest {
)
),
disabledUrlPatterns = listOf("google.com"),
defaultCaptureLimit = 220,
defaultCaptureLimit = 720,
),
capturePublicKey = "test"
)
Expand Down Expand Up @@ -71,7 +71,7 @@ internal class NetworkBehaviorTest {
assertFalse(isRequestContentLengthCaptureEnabled())
assertTrue(isNativeNetworkingMonitoringEnabled())
assertEquals(1000, getNetworkCaptureLimit())
assertEquals(emptyMap<String, Int>(), getNetworkCallLimitsPerDomain())
assertEquals(emptyMap<String, Int>(), getNetworkCallLimitsPerDomainSuffix())
assertTrue(isUrlEnabled("google.com"))
assertFalse(isCaptureBodyEncryptionEnabled())
assertNull(getCapturePublicKey())
Expand All @@ -85,19 +85,38 @@ internal class NetworkBehaviorTest {
assertEquals("x-custom-trace", getTraceIdHeader())
assertTrue(isRequestContentLengthCaptureEnabled())
assertFalse(isNativeNetworkingMonitoringEnabled())
assertEquals(mapOf("google.com" to 100), getNetworkCallLimitsPerDomain())
assertEquals(220, getNetworkCaptureLimit())
assertEquals(mapOf("google.com" to 100), getNetworkCallLimitsPerDomainSuffix())
assertEquals(720, getNetworkCaptureLimit())
assertFalse(isUrlEnabled("google.com"))
assertTrue(isCaptureBodyEncryptionEnabled())
assertEquals("test", getCapturePublicKey())
}
}

@Test
fun testRemoteOnly() {
with(fakeNetworkBehavior(localCfg = { null }, remoteCfg = { remote })) {
assertEquals(409, getNetworkCaptureLimit())
assertEquals(mapOf("google.com" to 50), getNetworkCallLimitsPerDomainSuffix())
assertTrue(isUrlEnabled("google.com"))
assertFalse(isUrlEnabled("example.com"))
assertEquals(
NetworkCaptureRuleRemoteConfig(
"test",
5000,
"GET",
"google.com",
),
getNetworkCaptureRules().single()
)
}
}

@Test
fun testRemoteAndLocal() {
with(fakeNetworkBehavior(localCfg = { local }, remoteCfg = { remote })) {
assertEquals(409, getNetworkCaptureLimit())
assertEquals(mapOf("google.com" to 50), getNetworkCallLimitsPerDomain())
assertEquals(mapOf("google.com" to 50), getNetworkCallLimitsPerDomainSuffix())
assertTrue(isUrlEnabled("google.com"))
assertFalse(isUrlEnabled("example.com"))
assertEquals(
Expand Down
Loading

0 comments on commit 989dbf2

Please sign in to comment.