Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update crashlytics global data collection #1434

Merged
merged 10 commits into from
Jul 31, 2020
Merged

Conversation

VinayGuthal
Copy link
Contributor

No description provided.

@googlebot googlebot added the cla: yes Override cla label Apr 7, 2020
@google-oss-bot
Copy link
Contributor

google-oss-bot commented Apr 7, 2020

Coverage Report

Affected SDKs

No changes between base commit (f1e32c1) and head commit (462d9124).

Test Logs

Notes

HTML coverage reports can be produced locally with ./gradlew <product>:checkCoverage.
Report files are located at <product-build-dir>/reports/jacoco/.

Head commit (462d9124) is created by Prow via merging commits: f1e32c1 e29abd6.

@google-oss-bot
Copy link
Contributor

The public api surface has changed for the subproject firebase-crashlytics:
error: Added method com.google.firebase.crashlytics.FirebaseCrashlytics.setCrashlyticsCollectionEnabled(Boolean) [AddedMethod]

Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly.

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Apr 7, 2020

Binary Size Report

Affected SDKs

  • firebase-crashlytics

    Type Base (f1e32c1) Head (462d9124) Diff
    aar 409 kB 410 kB +428 B (+0.1%)
    apk (aggressive) 324 kB 324 kB +8 B (+0.0%)
    apk (release) 1.04 MB 1.04 MB +112 B (+0.0%)
  • firebase-installations

    Type Base (f1e32c1) Head (462d9124) Diff
    aar 59.2 kB 59.2 kB +3 B (+0.0%)

Test Logs

Notes

Head commit (462d9124) is created by Prow via merging commits: f1e32c1 e29abd6.

@google-oss-bot
Copy link
Contributor

The public api surface has changed for the subproject firebase-crashlytics:
error: Added method com.google.firebase.crashlytics.FirebaseCrashlytics.setCrashlyticsCollectionEnabled(Boolean) [AddedMethod]

Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly.

@google-oss-bot
Copy link
Contributor

The public api surface has changed for the subproject firebase-crashlytics:
error: Added method com.google.firebase.crashlytics.FirebaseCrashlytics.setCrashlyticsCollectionEnabled(Boolean) [AddedMethod]

Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly.

Copy link
Member

@vkryachko vkryachko left a comment

Choose a reason for hiding this comment

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

LGTM, pls get an lgtm from the crashlytics team

crashlyticsDataCollectionEnabled = enabled;
crashlyticsDataCollectionExplicitlySet = true;
sharedPreferences.edit().putBoolean(FIREBASE_CRASHLYTICS_COLLECTION_ENABLED, enabled).commit();
public synchronized void setCrashlyticsDataCollectionEnabled(Boolean enabled) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We can help our future selves by adding some comments here :)

arbiter.setCrashlyticsDataCollectionEnabled(false);
assertFalse(arbiter.isAutomaticDataCollectionEnabled());

when(mockPrefs.contains(PREFS_KEY)).thenReturn(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you split this up into separate tests that express the conditions they check?

@mrwillis21
Copy link
Contributor

/test device-check-changed

@VinayGuthal VinayGuthal merged commit ed3ff88 into master Jul 31, 2020
@VinayGuthal VinayGuthal deleted the update_crash_api branch July 31, 2020 17:31
@@ -463,4 +463,23 @@ public boolean didCrashOnPreviousExecution() {
public void setCrashlyticsCollectionEnabled(boolean enabled) {
core.setCrashlyticsCollectionEnabled(enabled);
}

/**
* Enables/disables/clears automatic data collection config by Crashlytics.
Copy link

@cbonnie cbonnie Aug 3, 2020

Choose a reason for hiding this comment

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

Suggest replacing line 468 with:

"* Allows you to enable or disable the automatic data collection configuration in Crashlytics."

/**
* Enables/disables/clears automatic data collection config by Crashlytics.
*
* <p>If this is set, it overrides the data collection settings provided by the Android Manifest,
Copy link

Choose a reason for hiding this comment

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

Suggest replacing lines 470 and 471 with:

"* If not set, defaults to True. When set to False, disables automatic collection by overriding the data collections settings in the AndroidManifest.xml, as well as any Firebase-wide automatic data collection settings. When set to null, clears the Crashlytics data collection configuration (in this case, enabling data collection becomes dependent on other settings)."

* disabled. Use {@link #deleteUnsentReports()} to delete any reports stored on the device without
* sending them to Crashlytics.
*
* @param enabled whether to enable automatic data collection. The value of null would clear the
Copy link

Choose a reason for hiding this comment

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

Suggest replacing line 479 with:

"@param Determines whether to enable or disable automatic data collection. If set to null, clears the Crashlytics data collection configuration."

@firebase firebase locked and limited conversation to collaborators Aug 31, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes Override cla size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants