Diagnostics Add diagnostics manager by tonidero · Pull Request #785 · RevenueCat/purchases-android

tonidero · 2023-02-09T15:28:11Z

Description

This PR is based on #784. Here we are adding the DiagnosticsManager which is in charge of coordinating the diagnostics logic like calling the backend and handling the jsonl files. There are some WIPs to be addressed in future PRs.

tonidero · 2023-02-09T15:28:54Z

All these WIPs will be addressed in future PRs.

tonidero · 2023-02-09T15:30:41Z

The plan is to retry if there is a network/500 error but we didn't want to retry forever. Since an immediate retry would not solve all cases, we use shared preferences to store the number of retries through app restarts. Lmk what you think of that logic!

Currently I'm creating a new shared preferences file for this. Once we migrate to use a separate shared preferences file for the whole SDK, we could unify and have a single one, but for now, I thought it best to not use the app's shared preferences.

codecov · 2023-02-09T15:45:20Z

Codecov Report

❗ No coverage uploaded for pull request base (diagnostics@74d6c21). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head a6873d9 differs from pull request most recent head 51d8760. Consider uploading reports for the commit 51d8760 to get more accurate results

@@              Coverage Diff               @@
##             diagnostics     #785   +/-   ##
==============================================
  Coverage               ?   81.85%           
==============================================
  Files                  ?      127           
  Lines                  ?     4133           
  Branches               ?      520           
==============================================
  Hits                   ?     3383           
  Misses                 ?      544           
  Partials               ?      206

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

NachoSoto · 2023-02-14T19:52:34Z

We need diagnostics for diagnostics...
In all seriousness, I'm curious why all these are verbose?

I guess it's to prevent noise for folks actually debugging, since this isn't that much of a developer-facing feature (other than opting in)

like, if we had error level for these, folks might get concerned but it won't affect their apps in any way

Yeah, that's my thought on this. Since this is a non-critical feature, I thought we don't want to polute the logs with it. As mentioned before, lmk if you prefer to send errors/warnings and I can change it, not opposed to that approach either.

Yeah that makes sense, wanted to make sure I understood.

NachoSoto · 2023-02-14T19:53:58Z

Not sure what other types in the Android codebase do, but it would help to have a quick docstring here summarizing what this type does.

We don't usually add docstrings from what I've seen. But yeah, I can add this here 👍

NachoSoto · 2023-02-14T19:54:22Z

Maybe

Suggested change

const val MAX_NUMBER_RETRIES = 3

const val MAX_NUMBER_POST_RETRIES = 3

NachoSoto · 2023-02-14T19:55:24Z

I wonder if this would be simpler like:

Suggested change

if (shouldRetry) {

val currentRetryCount = increaseConsecutiveNumberOfErrors()

if (currentRetryCount >= MAX_NUMBER_RETRIES) {

resetDiagnosticsStatus()

}

} else {

resetDiagnosticsStatus()

}

if (shouldRetry || increaseConsecutiveNumberOfErrors() >= MAX_NUMBER_RETRIES) {

resetDiagnosticsStatus()

}

In the end, I added the logs Andy suggested below, so didn't do this change

aboedo · 2023-02-14T20:23:04Z

Let's make the behavior clearer:

Suggested change

verboseLog("Error syncing diagnostics file: $error. Should retry: $shouldRetry")

if (shouldRetry) {

verboseLog("Error syncing diagnostics file: $error. Will retry the next time the SDK is initialized")

} else {

verboseLog("Error syncing diagnostics file: $error. This was the final attempt ($MAX_NUMBER_RETRIES). Deleting diagnostics file without posting.")

aboedo · 2023-02-14T20:23:53Z

aboedo · 2023-02-14T20:24:58Z

I guess it's to prevent noise for folks actually debugging, since this isn't that much of a developer-facing feature (other than opting in)

aboedo · 2023-02-14T20:25:32Z

like, if we had error level for these, folks might get concerned but it won't affect their apps in any way

…795) ### Description Based on #793 This PR refactors previous code to split the `DiagnosticsManager` into `DiagnosticsTracker` and `DiagnosticsSynchronizer`. This is a prerequisite to avoid circular dependencies when tracking request data. - Before: `DiagnosticsManager`->`Backend`->`HttpClient`->`DiagnosticsManager`. - After: `DiagnosticsSynchronizer`->`Backend` for syncing and `HttpClient`->`DiagnosticsTracker` for tracking requests I could have tried to extract this logic from the HttpClient, but that would be a bigger refactor. This was the simplest solution I thought for this problem. Also, sorry I didn't see that issue before. I didn't modify the #785 PR directly to try to avoid some conflicts that would happen if I did it there directly.

tonidero added the refactor label Feb 9, 2023

tonidero commented Feb 9, 2023

View reviewed changes

This was referenced Feb 9, 2023

Diagnostics: Anonymize events #782

Merged

Diagnostics support #766

Closed

Diagnostics: Backend integration #771

Closed

tonidero force-pushed the diagnostics-jsonl-file-management branch from 313ca01 to b5e7f30 Compare February 10, 2023 11:12

tonidero force-pushed the diagnostics-manager branch from 2c34b06 to a6873d9 Compare February 10, 2023 11:25

tonidero marked this pull request as ready for review February 10, 2023 11:26

tonidero requested a review from a team February 10, 2023 11:26

This was referenced Feb 10, 2023

Diagnostics: Backend support #787

Merged

Diagnostics: limit events sent #793

Merged

Diagnostics: Diagnostics split manager into tracker and synchronizer #795

Merged

NachoSoto approved these changes Feb 14, 2023

View reviewed changes

aboedo approved these changes Feb 14, 2023

View reviewed changes

Base automatically changed from diagnostics-jsonl-file-management to diagnostics February 15, 2023 08:19

tonidero added 2 commits February 15, 2023 09:25

Add diagnostics manager with coordination logic

b186c7d

Fixes from review

51d8760

tonidero force-pushed the diagnostics-manager branch from a6873d9 to 51d8760 Compare February 15, 2023 08:39

tonidero merged commit 366b0c8 into diagnostics Feb 16, 2023

tonidero deleted the diagnostics-manager branch February 16, 2023 08:58

tonidero added a commit that referenced this pull request Feb 28, 2023

Diagnostics: Add diagnostics manager (#785)

863c0d4

tonidero added a commit that referenced this pull request Feb 28, 2023

Diagnostics: Add diagnostics manager (#785)

9627e87

	const val MAX_NUMBER_RETRIES = 3
	const val MAX_NUMBER_POST_RETRIES = 3

-                        if (shouldRetry) {
-                            val currentRetryCount = increaseConsecutiveNumberOfErrors()
-                            if (currentRetryCount >= MAX_NUMBER_RETRIES) {
-                                resetDiagnosticsStatus()
-                            }
-                        } else {
-                            resetDiagnosticsStatus()
-                        }
+                        if (shouldRetry || increaseConsecutiveNumberOfErrors() >= MAX_NUMBER_RETRIES) {
+                            resetDiagnosticsStatus()
+                        }

-                        verboseLog("Error syncing diagnostics file: $error. Should retry: $shouldRetry")
+                        if (shouldRetry) {
+                            verboseLog("Error syncing diagnostics file: $error. Will retry the next time the SDK is initialized")
+                        } else {
+                            verboseLog("Error syncing diagnostics file: $error. This was the final attempt ($MAX_NUMBER_RETRIES). Deleting diagnostics file without posting.")

Uh oh!

Conversation

tonidero commented Feb 9, 2023

Description

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Feb 9, 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!

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!

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!

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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Feb 9, 2023 •

edited

Loading