Using deferred Analytics with Crashlytics. #2555
Conversation
Coverage ReportAffected SDKs
Test Logs
NotesHTML coverage reports can be produced locally with Head commit (ebdf28f3) is created by Prow via merging commits: 129d7e5 2c0c76c. |
Binary Size ReportAffected SDKs
Test Logs
NotesHead commit (ebdf28f3) is created by Prow via merging commits: 129d7e5 2c0c76c. |
...e-crashlytics/src/main/java/com/google/firebase/crashlytics/AnalyticsDeferredComponents.java
Outdated
Show resolved
Hide resolved
| import java.util.concurrent.TimeUnit; | ||
|
|
||
| /** @hide */ | ||
| public class AnalyticsDeferredComponents { |
There was a problem hiding this comment.
Unless I am missing something, it looks like we have 2 classes that depend on the connector:
- BreadcrumbSource
- AnalyticsEventLogger
I wonder if it would be cleaner to have separate implementations for both of them that depend on Deferred<AnalyticsConnector> as opposed to having them exposed through getters of this class. This would achieve 2 things:
- reduce coupling since all dependent classes won't have to depend on this "components" class with getters[1]
- Would make it easier to achieve thread safety in both classes, which your current implementation does not seem to be.
There was a problem hiding this comment.
Since both objects that depend on the deferred objects are interfaces in Crashlytics , I've addressed your comment as follows:
- Created new "proxy" implementations of the interfaces, these implementations are inner classes to the
AnalyticsDeferredComponentobject. - These proxy objects simply channel any calls to the actual implementing object in the
AnalyticsDeferredComponentobject. - The
AnalyticsDeferredComponentby default creates 2 objects which are the default implementations Crashlytics use when there's noAnalyticsConnectorpresent. - When the
AnalyticsConnectorbecomes available, we switch the default implementation objects with new ones that use Analytics.
This is transparent to any other objects using BreadcrumbSource and AnalyticsEventLogger since the proxy implementations will switch to the right objects cleanly.
| // Set the breadcrumb event receiver as the breadcrumb source for Crashlytics. | ||
| breadcrumbSource = breadcrumbReceiver; | ||
| // Set the blocking analytics event logger for Crashlytics. | ||
| analyticsEventLogger = blockingAnalyticsEventLogger; |
There was a problem hiding this comment.
This does not look thread-safe, i.e. other threads may never see these writes
...e-crashlytics/src/main/java/com/google/firebase/crashlytics/AnalyticsDeferredComponents.java
Outdated
Show resolved
Hide resolved
|
/retest |
|
/test device-check-changed |
No description provided.