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

bidetofevil · 2023-11-16T08:28:31Z

Goal

There were two problems that were addressed in this PR:

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

Properly enforce network call per session limit and use the remote limit as a ceiling for any local limits set.
WHY:

Limiting the number of session calls recorded prevents session payloads from growing to an unhealthy size.

…ently

bidetofevil · 2023-11-16T08:28:42Z

Current dependencies on/for this PR:

master
- PR Use remote limit as ceiling for network call limits and apply consistently #78 👈

This stack of pull requests is managed by Graphite.

codecov · 2023-11-16T08:41:20Z

Codecov Report

Merging #78 (010968e) into master (4c5ff31) will increase coverage by 0.10%.
Report is 2 commits behind head on master.
The diff coverage is 85.71%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #78      +/-   ##
==========================================
+ Coverage   75.92%   76.03%   +0.10%     
==========================================
  Files         313      313              
  Lines       10114    10097      -17     
  Branches     1461     1455       -6     
==========================================
- Hits         7679     7677       -2     
+ Misses       1843     1832      -11     
+ Partials      592      588       -4

Files	Coverage Δ
...roid/embracesdk/config/behavior/NetworkBehavior.kt	`83.33% <83.33%> (+8.33%)`	⬆️
...dk/network/logging/EmbraceNetworkLoggingService.kt	`87.60% <86.66%> (+0.68%)`	⬆️

... and 1 file with indirect coverage changes

bidetofevil · 2023-11-16T16:31:22Z

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

-        for (domain in local?.networking?.domains ?: return null) {
-            if (domain.domain != null && domain.limit != null) {
-                mergedLimits[domain.domain] = domain.limit
+        val limitCeiling = getLimitCeiling()


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.

Consider leaving as a comment in the code?

bidetofevil · 2023-11-16T16:33:08Z

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

+
+    private val untrackedNetworkCallCount = AtomicInteger(0)
+
+    private var defaultPerDomainCallLimit = configService.networkBehavior.getNetworkCaptureLimit()


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.

bidetofevil · 2023-11-16T16:34:01Z

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

            }
            return
+        } else if (!domainSetting.containsKey(domain)) {
+            createLimitForDomain(domain)


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.

bidetofevil · 2023-11-16T16:35:56Z

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

-            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) {


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.

bidetofevil · 2023-11-16T16:38:22Z

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

                )
            }
        }
    }

-    private fun storeSettings(url: String) {
+    private fun createLimitForDomain(domain: String) {


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.

bidetofevil · 2023-11-16T16:40:01Z

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

@@ -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


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.

Periodic session caching strikes again 💥

fractalwrench

LGTM

fractalwrench · 2023-11-16T17:29:34Z

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

-        for (domain in local?.networking?.domains ?: return null) {
-            if (domain.domain != null && domain.limit != null) {
-                mergedLimits[domain.domain] = domain.limit
+        val limitCeiling = getLimitCeiling()


Consider leaving as a comment in the code?

fractalwrench · 2023-11-16T17:30:42Z

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

@@ -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


Periodic session caching strikes again 💥

…curate

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

1fd748c

…ently

bidetofevil marked this pull request as ready for review November 16, 2023 16:27

bidetofevil requested a review from a team as a code owner November 16, 2023 16:27

bidetofevil requested a review from fractalwrench November 16, 2023 16:27

bidetofevil commented Nov 16, 2023

View reviewed changes

$fractalwrench$

fractalwrench approved these changes Nov 16, 2023

View reviewed changes

Add more comments per review and tweaked variable names to be more ac…

010968e

…curate

bidetofevil merged commit 989dbf2 into master Nov 16, 2023

bidetofevil deleted the hho/network-logging-limits branch November 16, 2023 18:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

bidetofevil commented Nov 16, 2023 •

edited

Loading

bidetofevil commented Nov 16, 2023

codecov bot commented Nov 16, 2023 •

edited

Loading

bidetofevil Nov 16, 2023

$@fractalwrench$ fractalwrench Nov 16, 2023

bidetofevil Nov 16, 2023

bidetofevil Nov 16, 2023

bidetofevil Nov 16, 2023

bidetofevil Nov 16, 2023

bidetofevil Nov 16, 2023

bidetofevil Nov 16, 2023

$@fractalwrench$ fractalwrench Nov 16, 2023

$@fractalwrench$ fractalwrench left a comment

$@fractalwrench$ fractalwrench Nov 16, 2023

$@fractalwrench$ fractalwrench Nov 16, 2023


		private val untrackedNetworkCallCount = AtomicInteger(0)

		private var defaultPerDomainCallLimit = configService.networkBehavior.getNetworkCaptureLimit()

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

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

Conversation

bidetofevil commented Nov 16, 2023 • edited Loading

Goal

Testing

Release Notes

bidetofevil commented Nov 16, 2023

codecov bot commented Nov 16, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fractalwrench left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bidetofevil commented Nov 16, 2023 •

edited

Loading

codecov bot commented Nov 16, 2023 •

edited

Loading

$@fractalwrench$ fractalwrench left a comment