Skip to content

Diagnostics: Sync diagnostics when configuring SDK if needed#791

Merged
tonidero merged 2 commits into
diagnosticsfrom
diagnostics-syncing-configure-if-needed
Feb 16, 2023
Merged

Diagnostics: Sync diagnostics when configuring SDK if needed#791
tonidero merged 2 commits into
diagnosticsfrom
diagnostics-syncing-configure-if-needed

Conversation

@tonidero

@tonidero tonidero commented Feb 10, 2023

Copy link
Copy Markdown
Contributor

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 tonidero added the pr:feat A new feature label Feb 10, 2023
@tonidero tonidero changed the title Sync diagnostics when configuring SDK if needed Diagnostics: Sync diagnostics when configuring SDK if needed Feb 10, 2023

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.

This will cause a conflict when merging with #782 but we can easily solve it then.

@tonidero tonidero marked this pull request as ready for review February 10, 2023 16:28
@tonidero tonidero requested a review from a team February 10, 2023 16:28
@codecov

codecov Bot commented Feb 10, 2023

Copy link
Copy Markdown

Codecov Report

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

❗ Current head 1f21ef7 differs from pull request most recent head 7f6f079. Consider uploading reports for the commit 7f6f079 to get more accurate results

@@              Coverage Diff               @@
##             diagnostics     #791   +/-   ##
==============================================
  Coverage               ?   81.95%           
==============================================
  Files                  ?      127           
  Lines                  ?     4200           
  Branches               ?      529           
==============================================
  Hits                   ?     3442           
  Misses                 ?      548           
  Partials               ?      210           

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

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 like this API

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.

+1, really clean

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.

If we have it as false, and since this is for our own benefit, will anyone ever enable it?

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.

We recently decided that we would do better for our customers by leaving it disabled by default. It would certainly cause us to gather very little to no data at the beginning, but our plan is to earn people enabling this by providing extra value to people that enable this. CC @aboedo

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.

The fact that we are instantiating here is going to make it hard to mock. Do you think we need to mock this?

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.

Well, PurchasesFactory is a hard class to mock, since it's where all our real dependencies actually get instantiated. We could try to refactor this class to better separate all these dependencies and allow us to test the dependency instantiation process we do in this class, but that seems out of the scope of this PR.

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.

+1, really clean

@tonidero tonidero force-pushed the diagnostics-backend branch 2 times, most recently from 6516607 to 5131dea Compare February 16, 2023 09:09
Base automatically changed from diagnostics-backend to diagnostics February 16, 2023 09:30
@tonidero tonidero force-pushed the diagnostics-syncing-configure-if-needed branch from 1f21ef7 to a6d94a9 Compare February 16, 2023 09:33
@tonidero tonidero merged commit b8e819a into diagnostics Feb 16, 2023
@tonidero tonidero deleted the diagnostics-syncing-configure-if-needed branch February 16, 2023 09:49
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
### 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
### 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

pr:feat A new feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants