Skip to content

Crashlytics Add Internal Keys feature for Unity Metadata #2671

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

Merged
merged 9 commits into from
May 25, 2021

Conversation

samedson
Copy link
Contributor

  • Allow us to pull internal Unity Metadata without impacting customers' Custom Keys limits.

@googlebot googlebot added the cla: yes Override cla label May 17, 2021
@google-oss-bot
Copy link
Contributor

google-oss-bot commented May 17, 2021

Coverage Report

Affected SDKs

  • firebase-crashlytics

    SDK overall coverage did not change between base commit (5a0ed84) and head commit (28bc1000). However there are changes in individual files.

    Filename Base (5a0ed84) Head (28bc1000) Diff
    InternalKeys.java ? 0.00% ?

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 (28bc1000) is created by Prow via merging commits: 5a0ed84 b03a676.

@google-oss-bot
Copy link
Contributor

google-oss-bot commented May 17, 2021

Binary Size Report

Affected SDKs

  • firebase-crashlytics

    Type Base (b5d06cd) Head (c0edee6a) Diff
    aar 318 kB 320 kB +1.97 kB (+0.6%)
    apk (aggressive) 193 kB 193 kB +296 B (+0.2%)
    apk (release) 821 kB 821 kB +836 B (+0.1%)

Test Logs

Notes

Head commit (c0edee6a) is created by Prow via merging commits: b5d06cd 17d2d9d.

@samedson samedson requested a review from mrichards May 19, 2021 20:51
@samedson
Copy link
Contributor Author

Getting the following failure:

Task:copyRootGoogleServices -> sourceGoogleServicesFile {path: '/Users/samedson/firebase/firebase-android-sdk/google-services.json', exists: false}, dest {path: '/Users/samedson/firebase/firebase-android-sdk/appcheck/firebase-appcheck/test-app'}, targetPackage {name: 'com.googletest.firebase.appcheck', foundInSource: false}
[test] applying google-services plugin
Claiming file firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/FirebaseCrashlytics.java for project project ':firebase-crashlytics'

FAILURE: Build failed with an exception.

@firebase firebase deleted a comment from google-oss-bot May 21, 2021
import java.util.Map;

/** Handles custom attributes internal to the SDK, eg. for custom Unity metadata. */
public class InternalKeys {
Copy link
Contributor

Choose a reason for hiding this comment

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

The implementations of InternalKeys and UserMetadata are almost identical. Did you consider making a KeyValueStore class and making UserMetadata extend from it? Then the internalKeys object is an instance of KeyValueStore. Usually I'm not too worried about DRY but I think in this case it makes more sense to use inheritance, since the two stores are so conceptually similar. Thoughts?

@google-oss-bot
Copy link
Contributor

google-oss-bot commented May 25, 2021

Macrobenchmark Report

Affected SDKs

Measurements are for head commit (17d2d9d). Diffing against base commit (b5d06cd) is working in progress.

  • baseline

    NameDeviceMinMaxP50P90P99
    BenchmarkTest.startup[mode=COLD]flame-30-en-portrait118.0 ms152.0 ms131.0 ms142.1 ms152.0 ms
    BenchmarkTest.startup[mode=HOT]flame-30-en-portrait39.0 ms63.0 ms47.5 ms62.0 ms62.8 ms
    BenchmarkTest.startup[mode=WARM]flame-30-en-portrait47.0 ms81.0 ms66.0 ms77.3 ms80.8 ms
  • firebase-common

    NameDeviceMinMaxP50P90P99
    BenchmarkTest.startup[mode=COLD]flame-30-en-portrait117.0 ms158.0 ms138.5 ms150.6 ms157.6 ms
    BenchmarkTest.startup[mode=HOT]flame-30-en-portrait43.0 ms62.0 ms55.5 ms60.2 ms62.0 ms
    BenchmarkTest.startup[mode=WARM]flame-30-en-portrait49.0 ms79.0 ms74.0 ms77.1 ms78.8 ms
  • firebase-config

    NameDeviceMinMaxP50P90P99
    BenchmarkTest.startup[mode=COLD]flame-30-en-portrait132.0 ms164.0 ms149.5 ms159.2 ms163.4 ms
    BenchmarkTest.startup[mode=HOT]flame-30-en-portrait39.0 ms64.0 ms50.0 ms61.0 ms63.4 ms
    BenchmarkTest.startup[mode=WARM]flame-30-en-portrait50.0 ms83.0 ms68.5 ms82.0 ms82.8 ms
  • firebase-crashlytics

    NameDeviceMinMaxP50P90P99
    BenchmarkTest.startup[mode=COLD]flame-30-en-portrait135.0 ms181.0 ms151.0 ms171.7 ms180.4 ms
    BenchmarkTest.startup[mode=HOT]flame-30-en-portrait40.0 ms65.0 ms56.0 ms63.0 ms64.6 ms
    BenchmarkTest.startup[mode=WARM]flame-30-en-portrait52.0 ms83.0 ms69.0 ms80.1 ms82.6 ms
  • firebase-database

    NameDeviceMinMaxP50P90P99
    BenchmarkTest.startup[mode=COLD]flame-30-en-portrait118.0 ms166.0 ms143.0 ms152.3 ms163.9 ms
    BenchmarkTest.startup[mode=HOT]flame-30-en-portrait31.0 ms60.0 ms53.0 ms57.1 ms59.6 ms
    BenchmarkTest.startup[mode=WARM]flame-30-en-portrait51.0 ms93.0 ms70.5 ms77.3 ms90.5 ms
  • firebase-dynamic-links

    NameDeviceMinMaxP50P90P99
    BenchmarkTest.startup[mode=COLD]flame-30-en-portrait129.0 ms169.0 ms136.5 ms163.1 ms168.1 ms
    BenchmarkTest.startup[mode=HOT]flame-30-en-portrait36.0 ms70.0 ms53.5 ms62.2 ms68.9 ms
    BenchmarkTest.startup[mode=WARM]flame-30-en-portrait52.0 ms93.0 ms71.5 ms90.2 ms92.8 ms
  • firebase-firestore

    NameDeviceMinMaxP50P90P99
    BenchmarkTest.startup[mode=COLD]flame-30-en-portrait127.0 ms168.0 ms146.5 ms161.5 ms167.6 ms
    BenchmarkTest.startup[mode=HOT]flame-30-en-portrait35.0 ms66.0 ms48.0 ms58.8 ms66.0 ms
    BenchmarkTest.startup[mode=WARM]flame-30-en-portrait54.0 ms84.0 ms69.5 ms76.4 ms83.2 ms
  • firebase-functions

    NameDeviceMinMaxP50P90P99
    BenchmarkTest.startup[mode=COLD]flame-30-en-portrait132.0 ms176.0 ms153.5 ms163.8 ms175.1 ms
    BenchmarkTest.startup[mode=HOT]flame-30-en-portrait40.0 ms66.0 ms55.0 ms61.0 ms65.1 ms
    BenchmarkTest.startup[mode=WARM]flame-30-en-portrait46.0 ms92.0 ms73.5 ms84.0 ms90.5 ms
  • firebase-inappmessaging-display

    NameDeviceMinMaxP50P90P99
    BenchmarkTest.startup[mode=COLD]flame-30-en-portrait156.0 ms214.0 ms172.0 ms196.2 ms212.7 ms
    BenchmarkTest.startup[mode=HOT]flame-30-en-portrait46.0 ms70.0 ms57.0 ms66.3 ms69.8 ms
    BenchmarkTest.startup[mode=WARM]flame-30-en-portrait58.0 ms91.0 ms73.0 ms83.2 ms89.9 ms
  • firebase-messaging

    NameDeviceMinMaxP50P90P99
    BenchmarkTest.startup[mode=COLD]flame-30-en-portrait136.0 ms170.0 ms153.5 ms163.1 ms168.9 ms
    BenchmarkTest.startup[mode=HOT]flame-30-en-portrait37.0 ms67.0 ms57.0 ms63.3 ms66.8 ms
    BenchmarkTest.startup[mode=WARM]flame-30-en-portrait53.0 ms91.0 ms77.5 ms82.0 ms89.3 ms
  • firebase-perf

    NameDeviceMinMaxP50P90P99
    BenchmarkTest.startup[mode=COLD]flame-30-en-portrait133.0 ms172.0 ms155.5 ms165.2 ms171.0 ms
    BenchmarkTest.startup[mode=HOT]flame-30-en-portrait42.0 ms69.0 ms50.5 ms65.0 ms68.2 ms
    BenchmarkTest.startup[mode=WARM]flame-30-en-portrait53.0 ms85.0 ms70.0 ms79.2 ms84.2 ms
  • firebase-storage

    NameDeviceMinMaxP50P90P99
    BenchmarkTest.startup[mode=COLD]flame-30-en-portrait124.0 ms161.0 ms139.0 ms158.2 ms160.8 ms
    BenchmarkTest.startup[mode=HOT]flame-30-en-portrait40.0 ms69.0 ms52.5 ms64.3 ms68.6 ms
    BenchmarkTest.startup[mode=WARM]flame-30-en-portrait51.0 ms91.0 ms68.5 ms87.0 ms90.2 ms

@@ -461,6 +465,7 @@ private void mockEventInteractions() {
when(mockEvent.getApp()).thenReturn(mockEventApp);
when(mockEventApp.toBuilder()).thenReturn(mockEventAppBuilder);
when(mockEventAppBuilder.setCustomAttributes(any())).thenReturn(mockEventAppBuilder);
when(mockEventAppBuilder.setInternalKeys(any())).thenReturn(mockEventAppBuilder);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was the necessary line to fix tests

Copy link
Contributor

@mrichards mrichards left a comment

Choose a reason for hiding this comment

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

:shipit: Good choice on the KeysMap!

@samedson samedson merged commit 79c1305 into master May 25, 2021
@samedson samedson deleted the internal-keys branch May 25, 2021 21:07
samedson added a commit that referenced this pull request May 26, 2021
samedson added a commit that referenced this pull request May 26, 2021
@firebase firebase locked and limited conversation to collaborators Jun 25, 2021
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.

4 participants