Use remote limit as ceiling for network call limits and apply consist…

…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.

embrace-android-sdk/src/main/java/io/embrace/android/embracesdk/config/behavior/NetworkBehavior.kt

@@ -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.
@@ -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)
  }
 
  /**
@@ -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
 }

embrace-android-sdk/src/main/java/io/embrace/android/embracesdk/network/logging/EmbraceNetworkLoggingService.kt

@@ -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 {
@@ -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
  }
@@ -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)
  }
 
@@ -114,7 +118,6 @@ internal class EmbraceNetworkLoggingService(
  }
 
  processNetworkCall(callId, networkCall)
- storeSettings(url)
  }
 
  override fun logNetworkError(
@@ -156,7 +159,6 @@ internal class EmbraceNetworkLoggingService(
  )
  }
  processNetworkCall(callId, networkCall)
- storeSettings(url)
  }
 
  /**
@@ -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)
  }
  }
 
@@ -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()
  }
 }

embrace-android-sdk/src/test/java/io/embrace/android/embracesdk/config/behavior/NetworkBehaviorTest.kt

@@ -41,7 +41,7 @@ internal class NetworkBehaviorTest {
  )
  ),
  disabledUrlPatterns = listOf("google.com"),
- defaultCaptureLimit = 220,
+ defaultCaptureLimit = 720,
  ),
  capturePublicKey = "test"
  )
@@ -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())
@@ -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(