Skip to content

Commit

Permalink
Synchronize network call logging (#99)
Browse files Browse the repository at this point in the history 
## Goal

Properly synchronize the update and fetching of network calls. The previous strategy only accounted for the modification of the network call storage, but left limits tracking and limit overage reporting open to race conditions. This change makes sure that while a network call is being logged, we can't fetch a current snapshot for the session payload. 

Also, we were trying to be clever about when when to lock when we take the snapshot, but it just didn't work (i.e. the things can change once the calls were fetched but before the payload is returned). As much as we can, we put work outside the synchronization block if we could (i.e. when we validate the staleness of the cache, which should not be a problem now), but in most cases, the locking isn't too bad (i.e. cache read and limits overage data construction), and all we're blocking are background threads. So this should be OK? *sweat_smile

## Testing

Existing tests pass. The stateless internal error was happening once every ~650 session or so, and this change should bring it down to 0
  • Loading branch information
@bidetofevil
bidetofevil authored Nov 21, 2023
2 parents 69e2b53 + 3e516aa commit 57c3406
Showing 1 changed file with 69 additions and 65 deletions.
134 changes: 69 additions & 65 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 @@ -53,28 +53,33 @@ internal class EmbraceNetworkLoggingService(
private var domainSuffixCallLimits = configService.networkBehavior.getNetworkCallLimitsPerDomainSuffix()

override fun getNetworkCallsForSession(): NetworkSessionV2 {
val calls = networkCallCache.value {
var storedCallsSize: Int? = null
var cachedCallsSize: Int? = null

try {
synchronized(callsStorageLastUpdate) {
sessionNetworkCalls.values.toList()
}
}
val calls = networkCallCache.value {
sessionNetworkCalls.values.toList()
}

val storedCallsSize = sessionNetworkCalls.size
val cachedCallsSize = calls.size
storedCallsSize = sessionNetworkCalls.size
cachedCallsSize = calls.size

val overLimit = hashMapOf<String, DomainCount>()
for ((key, value) in callsPerDomainSuffix) {
if (value.requestCount > value.captureLimit) {
overLimit[key] = value
}
}
val overLimit = hashMapOf<String, DomainCount>()
for ((key, value) in callsPerDomainSuffix) {
if (value.requestCount > value.captureLimit) {
overLimit[key] = value
}
}

if (cachedCallsSize != storedCallsSize) {
val msg = "Cached network call count different than expected: $cachedCallsSize instead of $storedCallsSize"
logger.logError(msg, IllegalStateException(msg), true)
return NetworkSessionV2(calls, overLimit)
}
} finally {
if (cachedCallsSize != storedCallsSize) {
val msg = "Cached network call count different than expected: $cachedCallsSize instead of $storedCallsSize"
logger.logError(msg, IllegalStateException(msg), true)
}
}

return NetworkSessionV2(calls, overLimit)
}

override fun logNetworkCall(
Expand Down Expand Up @@ -161,6 +166,13 @@ internal class EmbraceNetworkLoggingService(
processNetworkCall(callId, networkCall)
}

override fun cleanCollections() {
clearNetworkCalls()
// re-fetch limits in case they changed since they last time they were fetched
defaultPerDomainSuffixCallLimit = configService.networkBehavior.getNetworkCaptureLimit()
domainSuffixCallLimits = configService.networkBehavior.getNetworkCallLimitsPerDomainSuffix()
}

/**
* Process network calls to be ready when the session requests them.
*
Expand All @@ -173,44 +185,45 @@ internal class EmbraceNetworkLoggingService(
getDomain(it)
} ?: return

if (isIpAddress(domain)) {
if (ipAddressNetworkCallCount.getAndIncrement() < defaultPerDomainSuffixCallLimit) {
storeNetworkCall(callId, networkCall)
synchronized(callsStorageLastUpdate) {
if (isIpAddress(domain)) {
if (ipAddressNetworkCallCount.getAndIncrement() < defaultPerDomainSuffixCallLimit) {
storeNetworkCall(callId, networkCall)
}
return
} else if (!domainSetting.containsKey(domain)) {
createLimitForDomain(domain)
}
return
} else if (!domainSetting.containsKey(domain)) {
createLimitForDomain(domain)
}

val settings = domainSetting[domain]
if (settings == null) {
// 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 countPerSuffix = callsPerDomainSuffix[suffix]
val settings = domainSetting[domain]
if (settings == null) {
// 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)
}
return
} else {
val suffix = settings.suffix
val limit = settings.limit
var countPerSuffix = callsPerDomainSuffix[suffix]

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

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

// Track the number of calls for each domain (or configured suffix)
suffix?.let {
callsPerDomainSuffix[it] = DomainCount(countPerSuffix.requestCount + 1, limit)
logger.logDeveloper(
"EmbraceNetworkLoggingService",
"Call per domain $domain ${countPerSuffix.requestCount + 1}"
)
// Track the number of calls for each domain (or configured suffix)
suffix?.let {
callsPerDomainSuffix[it] = DomainCount(countPerSuffix.requestCount + 1, limit)
logger.logDeveloper(
"EmbraceNetworkLoggingService",
"Call per domain $domain ${countPerSuffix.requestCount + 1}"
)
}
}
}
}
Expand All @@ -232,27 +245,18 @@ internal class EmbraceNetworkLoggingService(
}

private fun storeNetworkCall(callId: String, networkCall: NetworkCallV2) {
synchronized(callsStorageLastUpdate) {
callsStorageLastUpdate.incrementAndGet()
sessionNetworkCalls[callId] = networkCall
}
callsStorageLastUpdate.incrementAndGet()
sessionNetworkCalls[callId] = networkCall
}

private fun clearNetworkCalls() {
synchronized(callsStorageLastUpdate) {
domainSetting.clear()
callsPerDomainSuffix.clear()
ipAddressNetworkCallCount.set(0)
untrackedNetworkCallCount.set(0)
callsStorageLastUpdate.set(0)
sessionNetworkCalls.clear()
}
}

override fun cleanCollections() {
domainSetting.clear()
callsPerDomainSuffix.clear()
ipAddressNetworkCallCount.set(0)
untrackedNetworkCallCount.set(0)
clearNetworkCalls()
// re-fetch limits in case they changed since they last time they were fetched
defaultPerDomainSuffixCallLimit = configService.networkBehavior.getNetworkCaptureLimit()
domainSuffixCallLimits = configService.networkBehavior.getNetworkCallLimitsPerDomainSuffix()
}
}

0 comments on commit 57c3406

Please sign in to comment.