-
Notifications
You must be signed in to change notification settings - Fork 7
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.
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
Moved the logic for counting domain limits to a new class #749
Conversation
|
||
override fun logNetworkRequest(networkRequest: EmbraceNetworkRequest) { | ||
logNetworkCaptureData(networkRequest) | ||
processNetworkRequest(networkRequest) | ||
recordNetworkRequest(networkRequest) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably want to log the network request first before we send the data capture since that seems more important?
*/ | ||
private fun processNetworkRequest(networkRequest: EmbraceNetworkRequest) { | ||
private fun recordNetworkRequest(networkRequest: EmbraceNetworkRequest) { | ||
// TODO: Shouldn't we ignore domains that can't be parsed for networkCapturedData too? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Log a KTLO task and move on - not sure how these are related, so lets detangle this later
) : MemoryCleanerListener { | ||
|
||
private val domainSetting = ConcurrentHashMap<String, DomainSettings>() | ||
private val callsPerDomainSuffix = hashMapOf<String, NetworkSessionV2.DomainCount>() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be accessed concurrently so we need to use a threadsafe map
private var domainSuffixCallLimits = configService.networkBehavior.getNetworkCallLimitsPerDomainSuffix() | ||
|
||
fun canLogNetworkRequest(domain: String): Boolean { | ||
// TODO: Should we synchronize on another object now that we don't use callsStorageLastUpdate? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we don't synchronize, we might get a few network requests above the absolute max. That might be fine...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Want to see this at least be threadsafe before approving
5bdad96
to
21e7adf
Compare
e852564
to
c4e3c1e
Compare
Merge activity
|
21e7adf
to
31d4443
Compare
c4e3c1e
to
30a58da
Compare
Goal
Extracted the logic for counting domain limits to another class. This will allow us to test it more thoroughly in the future, as we detected some inconsistencies. It also keeps the NetworkLogging service cleaner.
Questions: