Skip to content

Diagnostics: Backend support#787

Merged
tonidero merged 11 commits into
diagnosticsfrom
diagnostics-backend
Feb 16, 2023
Merged

Diagnostics: Backend support#787
tonidero merged 11 commits into
diagnosticsfrom
diagnostics-backend

Conversation

@tonidero

Copy link
Copy Markdown
Contributor

Description

Based on #785
This PR adds the logic to send diagnostics data to our backend.

@codecov

codecov Bot commented Feb 10, 2023

Copy link
Copy Markdown

Codecov Report

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

❗ Current head 6516607 differs from pull request most recent head 66e86ee. Consider uploading reports for the commit 66e86ee to get more accurate results

@@              Coverage Diff               @@
##             diagnostics     #787   +/-   ##
==============================================
  Coverage               ?   81.96%           
==============================================
  Files                  ?      127           
  Lines                  ?     4187           
  Branches               ?      527           
==============================================
  Hits                   ?     3432           
  Misses                 ?      546           
  Partials               ?      209           

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

Comment thread common/src/main/java/com/revenuecat/purchases/common/AppConfig.kt Outdated

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Note that both the backend request and the file operations are executed in the same dispatcher. I believe that's ok but lmk if you have any concerns.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah that makes sense to isolate all of diagnostics in the same dispatcher.

@tonidero tonidero marked this pull request as ready for review February 10, 2023 15:47
@tonidero tonidero requested a review from a team February 10, 2023 15:47

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah that makes sense to isolate all of diagnostics in the same dispatcher.

Comment thread common/src/main/java/com/revenuecat/purchases/common/Backend.kt Outdated
Comment thread common/src/main/java/com/revenuecat/purchases/common/Backend.kt Outdated
Comment thread common/src/test/java/com/revenuecat/purchases/common/BackendTest.kt Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Awesome

Comment thread common/src/test/java/com/revenuecat/purchases/common/BackendTest.kt Outdated
Comment thread common/src/test/java/com/revenuecat/purchases/common/BackendTest.kt Outdated
Comment thread common/src/main/java/com/revenuecat/purchases/common/AppConfig.kt Outdated
Comment on lines 154 to 158

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this reminds me - we should probably add some jitter to the diagnostics call too. That way, if many apps get opened at the same time (say, from a remote push notification), we lessen the impact on our servers. We can probably have a more aggressive jitter too, compared to what we have for the rest of the calls.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good call 👍

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added af32f2a

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I imagine that this is to play it safe, but in practice we should never have more than one postDiagnostics request happening in the same session, let alone at the same time

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You're completely right. But yeah, this is just in case.

@tonidero tonidero force-pushed the diagnostics-manager branch from a6873d9 to 51d8760 Compare February 15, 2023 08:39
@tonidero tonidero force-pushed the diagnostics-backend branch from 62f8d66 to 0ffca29 Compare February 15, 2023 09:26
Base automatically changed from diagnostics-manager to diagnostics February 16, 2023 08:58
@tonidero tonidero force-pushed the diagnostics-backend branch from 6516607 to 5131dea Compare February 16, 2023 09:09
@tonidero tonidero merged commit 0f9a64e into diagnostics Feb 16, 2023
@tonidero tonidero deleted the diagnostics-backend branch February 16, 2023 09:30
tonidero added a commit that referenced this pull request Feb 16, 2023
### Description
Based on #787 

This PR adds support to enable/disable diagnostics (disabled by default)
and adds all the plumbing to sync diagnostics when configuring the SDK.
tonidero added a commit that referenced this pull request Feb 22, 2023
### Description
Based on #787 

This PR adds support to enable/disable diagnostics (disabled by default)
and adds all the plumbing to sync diagnostics when configuring the SDK.
tonidero added a commit that referenced this pull request Feb 28, 2023
tonidero added a commit that referenced this pull request Feb 28, 2023
### Description
Based on #787 

This PR adds support to enable/disable diagnostics (disabled by default)
and adds all the plumbing to sync diagnostics when configuring the SDK.
tonidero added a commit that referenced this pull request Feb 28, 2023
tonidero added a commit that referenced this pull request Feb 28, 2023
### Description
Based on #787 

This PR adds support to enable/disable diagnostics (disabled by default)
and adds all the plumbing to sync diagnostics when configuring the SDK.
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.

3 participants