Optimize diagnostics file management by tonidero · Pull Request #1194 · RevenueCat/purchases-android

tonidero · 2023-08-08T14:05:52Z

Description

This PR is the first part of a possible approach to avoid loading huge files in memory in diagnostics at once. Basically, in here we change to reading the file per lines instead of reading everything in one go.

Behavior changes

Diagnostics will be Android 24+ only.
An important change in behavior is that, instead of using the number of events in the diagnostics file to set a limit, we use the file size. So if on SDK configuration we detect that it's reached X size, we would clear the whole file and start over.
When we reach the file size limit, we remove the whole file, instead of just the oldest events.
We remove the properties from the MAX_EVENTS_STORED_LIMIT_REACHED event since they don't make sense with the new approach. (This will require backend changes)
Now we will have a limit to the max number of diagnostics events in memory and per request.

tonidero · 2023-08-08T14:09:35Z

 ) {
    companion object {
        const val DIAGNOSTICS_FILE_PATH = "RevenueCat/diagnostics/diagnostic_entries.jsonl"
+        const val DIAGNOSTICS_FILE_LIMIT_IN_KB = 500


This is TBD, this is a pretty big file already, but not sure how big these files might get... From my estimations, this might be ~1k events.

tonidero · 2023-08-08T14:11:11Z

            name = DiagnosticsEventName.MAX_EVENTS_STORED_LIMIT_REACHED,
-            properties = mapOf(
-                "total_number_events_stored" to totalEventsStored,
-                "events_removed" to eventsRemoved,


I thought these properties don't make sense anymore. We could send the file size, but in general, these files shouldn't get much bigger than the limit (currently of 500kb) so I thought it wouldn't be that useful.

tonidero · 2023-08-08T14:15:17Z

@@ -0,0 +1,6 @@
+package com.revenuecat.purchases.utils
+
+interface DataListener<T> {


Since I'm using this in the FileHelper, it should have a pretty generic name... Still not completely in love with it... Lmk if you have any other thoughts.

tonidero · 2023-08-08T14:20:02Z

+        var currentEventsToSync = 0
+        diagnosticsFileHelper.readDiagnosticsFile(object : DataListener<JSONObject> {
+            override fun onData(data: JSONObject) {
+                eventsToSync.add(data)


Note how we currently still read the whole file and keep it in memory. I'm thinking about splitting things in a future PR, so we post diagnostics in batches. That's slightly tricky since we don't have much control over the listener (we can't stop it midway, nor ask for new events on demand).

codecov · 2023-08-08T14:37:08Z

Codecov Report

Attention: Patch coverage is 86.53846% with 7 lines in your changes missing coverage. Please review.

Project coverage is 85.88%. Comparing base (0853414) to head (7202adb).
Report is 528 commits behind head on main.

Files with missing lines	Patch %	Lines
...otlin/com/revenuecat/purchases/PurchasesFactory.kt	0.00%	1 Missing and 3 partials ⚠️
.../com/revenuecat/purchases/PurchasesOrchestrator.kt	66.66%	0 Missing and 1 partial ⚠️
...purchases/common/diagnostics/DiagnosticsTracker.kt	88.88%	0 Missing and 1 partial ⚠️
.../revenuecat/purchases/utils/AndroidVersionUtils.kt	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1194      +/-   ##
==========================================
- Coverage   85.92%   85.88%   -0.04%     
==========================================
  Files         181      183       +2     
  Lines        6217     6229      +12     
  Branches      900      905       +5     
==========================================
+ Hits         5342     5350       +8     
- Misses        533      534       +1     
- Partials      342      345       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

tonidero · 2023-08-10T07:28:09Z

This is something that I don't fully like... But I didn't want to leak the responsibility of closing the readers.

It makes sense tho

tonidero · 2023-08-10T07:44:47Z

Adding a limit to the events in memory/per request was done previously in a separate PR, but I decided to move it to this PR for simplicity

NachoSoto

Sorry for the delay!

NachoSoto · 2023-08-16T14:37:52Z

What do you think about a more "functional" test that adds say 500 events and verifies that it's not considered too big?
As a way to validate that the size limit allows for some number of events that we're happy with.

Good idea, done in 2914d17

NachoSoto · 2023-08-16T14:38:08Z

Do we need to put this in the docs too?

We haven't added diagnostics to the docs yet since we were planning to add extra functionality before that, so nothing to update

…chronizer

@tonidero

**This is an automatic release.** ### Performance Improvements * Optimize SDK initialization when requests executed before any activity starts (#1204) via Toni Rico (@tonidero) * Optimize diagnostics file management (#1194) via Toni Rico (@tonidero) ### Other Changes * Use real debug view dependencies in magic weather compose (#1203) via Toni Rico (@tonidero) --------- Co-authored-by: revenuecat-ops <ops@revenuecat.com> Co-authored-by: Toni Rico <antonio.rico.diez@revenuecat.com> Co-authored-by: Toni Rico <toni.rico.diez@revenuecat.com>

tonidero added perf labels Aug 8, 2023

tonidero commented Aug 8, 2023

View reviewed changes

tonidero mentioned this pull request Aug 9, 2023

Add limit to how many diagnostics events to sync per request (part 2) #1195

Closed

tonidero marked this pull request as ready for review August 9, 2023 09:51

tonidero requested a review from a team August 9, 2023 09:51

tonidero marked this pull request as draft August 9, 2023 15:39

tonidero commented Aug 10, 2023

View reviewed changes

tonidero changed the title ~~Optimize diagnostics file management (Part 1)~~ Optimize diagnostics file management Aug 10, 2023

tonidero marked this pull request as ready for review August 10, 2023 07:45

tonidero requested a review from NachoSoto August 10, 2023 07:45

NachoSoto approved these changes Aug 16, 2023

View reviewed changes

tonidero added 4 commits August 17, 2023 11:59

Optimize diagnostics file management

277e944

Remove commented out tests

bc6661f

Fix test

4a455d8

Change to use Stream and limit diagnostics to Android N+

24bd04c

tonidero force-pushed the toniricodiez/sdk-3194-ensure-that-sdk-cant-accidentally-end-up-loading-a-very branch 2 times, most recently from 265d1f6 to 2914d17 Compare August 17, 2023 10:23

Add some extra tests using real diagnostics files for diagnostics syn…

7202adb

…chronizer

tonidero force-pushed the toniricodiez/sdk-3194-ensure-that-sdk-cant-accidentally-end-up-loading-a-very branch from 2914d17 to 7202adb Compare August 17, 2023 10:39

tonidero merged commit 7feda65 into main Aug 17, 2023

tonidero deleted the toniricodiez/sdk-3194-ensure-that-sdk-cant-accidentally-end-up-loading-a-very branch August 17, 2023 10:53

RCGitBot mentioned this pull request Aug 23, 2023

[AUTOMATIC] Release/6.9.3 #1207

Merged

vegaro added pr:other and removed pr:perf labels Sep 17, 2024

		@@ -0,0 +1,6 @@
		package com.revenuecat.purchases.utils

		interface DataListener<T> {

Uh oh!

Conversation

tonidero commented Aug 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Behavior changes

Uh oh!

Uh oh!

tonidero Aug 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Aug 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NachoSoto left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonidero Aug 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tonidero commented Aug 8, 2023 •

edited

Loading

tonidero Aug 8, 2023 •

edited

Loading

codecov Bot commented Aug 8, 2023 •

edited

Loading

tonidero Aug 17, 2023 •

edited

Loading