Skip to content
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

Jump to bottom

Use remote limit as ceiling for network call limits and apply consistently #78

Merged
merged 2 commits into from
Nov 16, 2023
Merged

Use remote limit as ceiling for network call limits and apply consistently #78

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1fd748c
Use remote limit as ceiling for network call limits and apply consist…
bidetofevil Nov 16, 2023
010968e
Add more comments per review and tweaked variable names to be more ac…
bidetofevil Nov 16, 2023
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
Next Next commit
Use remote limit as ceiling for network call limits and apply consist… 
…ently
  • Loading branch information
@bidetofevil
bidetofevil committed Nov 16, 2023
commit 1fd748cf30bc98534d1a2cb415ab75880dbd9cd8
35 changes: 20 additions & 15 deletions ...ndroid-sdk/src/main/java/io/embrace/android/embracesdk/config/behavior/NetworkBehavior.kt
View file
Open in desktop
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 @@ -68,28 +69,27 @@ internal class NetworkBehavior(
* List of domains to be limited for tracking.
*/
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
val limitCeiling = getLimitCeiling()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The algo is:

  • Use the remote settings as a base
  • For domains where there is both a local and remote entry, apply the local setting per domain if the local limit is smaller than the remote one
  • For domains with only a local entry, apply the local setting or the ceiling as defined by the default limit set on the remote, which ever is smaller.

Copy link
Contributor

Choose a reason for hiding this comment

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

Consider leaving as a comment in the code?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea

val domainLimits: MutableMap<String, Int> = remote?.networkConfig?.domainLimits?.toMutableMap() ?: mutableMapOf()

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

return domainLimits
}

/**
* 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 +129,9 @@ internal class NetworkBehavior(
* Gets the rules for capturing network call bodies
*/
fun getNetworkCaptureRules(): Set<NetworkCaptureRuleRemoteConfig> = remote?.networkCaptureRules ?: emptySet()

/**
* Cap the limit at the default limit set on the remote config
*/
private fun getLimitCeiling(): Int = remote?.networkConfig?.defaultCaptureLimit ?: DEFAULT_NETWORK_CALL_LIMIT
}
92 changes: 37 additions & 55 deletions ...c/main/java/io/embrace/android/embracesdk/network/logging/EmbraceNetworkLoggingService.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,11 +40,17 @@

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 ipAddressCount = AtomicInteger(0)
private val ipAddressNetworkCallCount = AtomicInteger(0)

private val untrackedNetworkCallCount = AtomicInteger(0)

private var defaultPerDomainCallLimit = configService.networkBehavior.getNetworkCaptureLimit()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We want to maintain the same limits for each session, so we get it at init time, and update when we reset the service when the session ends. Previously, this is pulled every time at run time, and we can get into inconsistent states in the middle of a session if new remote limits are fetched.


private var domainSuffixCallLimits = configService.networkBehavior.getNetworkCallLimitsPerDomain()

override fun getNetworkCallsForSession(): NetworkSessionV2 {
val calls = networkCallCache.value {
Expand All @@ -68,8 +74,6 @@
logger.logError(msg, IllegalStateException(msg), true)
}

// clear calls per domain and session network calls lists before be used by the next session
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Lol is one line is probably the most important change in the PR. We were resetting this every time a session payload is built, which effectively nerfs the limit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Periodic session caching strikes again 💥

callsPerDomain.clear()
return NetworkSessionV2(calls, overLimit)
}

Expand Down Expand Up @@ -114,7 +118,6 @@
}

processNetworkCall(callId, networkCall)
storeSettings(url)
}

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

/**
Expand All @@ -166,88 +168,66 @@
* @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() < defaultPerDomainCallLimit) {
storeNetworkCall(callId, networkCall)
} else {
logger.logDeveloper("EmbraceNetworkLoggingService", "capture limit exceeded")
}
return
} else if (!domainSetting.containsKey(domain)) {
createLimitForDomain(domain)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added the fetching of the domain limits here to run before we record a network request. It was doing it after and I have no idea why.

}

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() < defaultPerDomainCallLimit) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This should never happen so I don't know how to test it, but I've added code to catch and limit this in case it happens - it wasn't limiting that before.

Another option is just to ditch it like if we can't find a domain (which probably prevents this block from every running tbh), but this seems safer and who knows what funny refactoring in the future will bring. Safety first I guess.

storeNetworkCall(callId, networkCall)

Check warning on line 189 in embrace-android-sdk/src/main/java/io/embrace/android/embracesdk/network/logging/EmbraceNetworkLoggingService.kt

View check run for this annotation

Codecov / codecov/patch

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

Added line #L189 was not covered by tests
}
} else {
val suffix = settings.suffix
val limit = settings.limit
var count = callsPerDomain[suffix]
var countPerSuffix = callsPerDomain[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)
callsPerDomain[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) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The old implementation is really wasteful in that it rebuild the domain limits map at every call, often unnecessarily. So we build and cache it for reuse at init time and redo it when the session ends instead.

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(defaultPerDomainCallLimit, domain)
}
} catch (ex: Exception) {
logger.logDebug("Failed to determine limits for URL: $url", ex)
logger.logDebug("Failed to determine limits for domain: $domain", ex)

Check warning on line 230 in embrace-android-sdk/src/main/java/io/embrace/android/embracesdk/network/logging/EmbraceNetworkLoggingService.kt

View check run for this annotation

Codecov / codecov/patch

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

Added line #L230 was not covered by tests
}
}

Expand All @@ -266,11 +246,13 @@
}

override fun cleanCollections() {
domainSettings.clear()
domainSetting.clear()
callsPerDomain.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
defaultPerDomainCallLimit = configService.networkBehavior.getNetworkCaptureLimit()
domainSuffixCallLimits = configService.networkBehavior.getNetworkCallLimitsPerDomain()
}
}
23 changes: 21 additions & 2 deletions ...id-sdk/src/test/java/io/embrace/android/embracesdk/config/behavior/NetworkBehaviorTest.kt
View file
Open in desktop
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 @@ -86,13 +86,32 @@ internal class NetworkBehaviorTest {
assertTrue(isRequestContentLengthCaptureEnabled())
assertFalse(isNativeNetworkingMonitoringEnabled())
assertEquals(mapOf("google.com" to 100), getNetworkCallLimitsPerDomain())
assertEquals(220, getNetworkCaptureLimit())
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), getNetworkCallLimitsPerDomain())
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 })) {
Expand Down
Loading
Loading