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.

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

Synchronize network call logging #99

Merged
merged 1 commit into from
Nov 21, 2023
Merged

Conversation

bidetofevil
Copy link
Collaborator

@bidetofevil bidetofevil commented Nov 20, 2023

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

Copy link
Collaborator Author

bidetofevil commented Nov 20, 2023

Copy link

codecov bot commented Nov 20, 2023

Codecov Report

Merging #99 (3e516aa) into master (52b6edc) will increase coverage by 0.00%.
The diff coverage is 81.63%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #99   +/-   ##
=======================================
  Coverage   75.73%   75.73%           
=======================================
  Files         314      314           
  Lines       10068    10070    +2     
  Branches     1459     1459           
=======================================
+ Hits         7625     7627    +2     
  Misses       1850     1850           
  Partials      593      593           
Files Coverage Δ
...dk/network/logging/EmbraceNetworkLoggingService.kt 87.80% <81.63%> (+0.20%) ⬆️

Copy link
Contributor

@fractalwrench fractalwrench left a comment

Choose a reason for hiding this comment

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

LGTM

@bidetofevil bidetofevil merged commit 57c3406 into master Nov 21, 2023
4 checks passed
@bidetofevil bidetofevil deleted the hho/network-logging-synch branch November 21, 2023 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants