Skip to content
This repository has been archived by the owner on Jul 5, 2021. It is now read-only.

Glean Integration #4953

Closed
wants to merge 3 commits into from
Closed

Glean Integration #4953

wants to merge 3 commits into from

Conversation

cnevinc
Copy link
Contributor

@cnevinc cnevinc commented May 5, 2020

Notes:

  1. The APK size impact is 600~700KB (with AAB and enabling ABI split). After confirming with @shsu571, this PR is ready for review.

  2. I'll add the extra 2861397 bytes that Glean SDK took when checking the APK size.

  3. Glean will only be enabled in debug, firebase, and nightly builds.

@cnevinc cnevinc force-pushed the nevin/glean branch 6 times, most recently from 96019bb to 2615724 Compare May 5, 2020 09:59
@cnevinc cnevinc changed the title Add Glean [DO NOT MERGE] Glean Integration POC May 5, 2020
app/metrics.yaml Outdated Show resolved Hide resolved
app/build.gradle Show resolved Hide resolved
build.gradle Show resolved Hide resolved
@cnevinc cnevinc force-pushed the nevin/glean branch 3 times, most recently from c603e1c to 3367d38 Compare May 6, 2020 08:53
Copy link

@Dexterp37 Dexterp37 left a comment

Choose a reason for hiding this comment

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

This looks good to me. Please consider adding a note to your telemetry documentation stating that your product also uses Glean for sending telemetry and link to the data review response. You can also link to the pings glean sends out of the box for user documentation, here: https://mozilla.github.io/glean/book/user/pings/index.html

@cnevinc
Copy link
Contributor Author

cnevinc commented May 6, 2020

This looks good to me. Please consider adding a note to your [telemetry documentation]

@Dexterp37 Thank you for the review!
Last time I checked the Glean SDK only adds 6 KB to the APK size. Now it's 2MB. Do you have any idea? Or should I use an older version?

@Dexterp37
Copy link

This looks good to me. Please consider adding a note to your [telemetry documentation]

@Dexterp37 Thank you for the review!
Last time I checked the Glean SDK only adds 6 KB to the APK size. Now it's 2MB. Do you have any idea? Or should I use an older version?

We discussed this over Slack, but to recap:

  • yes, we are aware that this is a 2MB library;
  • no, the old SDK cannot be used;

@cnevinc is investigating other options to reduce the impact. If not successful, we'll try to explore ways on our end to attempt reducing the size.

@cnevinc cnevinc force-pushed the nevin/glean branch 3 times, most recently from 5960675 to dfda45d Compare May 7, 2020 03:15
@cnevinc
Copy link
Contributor Author

cnevinc commented May 7, 2020

Even if we replace the legacy Telemetry library with Glean, we'll still 2MB over our budget.
(8396272 > 6291456.0)

I first think we can use Dynamic Feature.
But we have 10-20% of users that download our app outside of the Play Store. (concern 1)

Proposal 2:
Using ABI-split might not help much cause the size is still around 1.2 to 1.9 MB.
I think the product team only accept at most 600 to 700K (around 10%) growth in APK size. (@shsu571 please help confirm) (concern 2)

I can't think of a better way right now.

@cnevinc
Copy link
Contributor Author

cnevinc commented May 7, 2020

I tried ABI split and build AAB, then build APKs for Pixel, Pixel 3a, Emulator. Here's the result. I think the APK size is a problem even we can get the artifact separately.

image

@cnevinc
Copy link
Contributor Author

cnevinc commented May 7, 2020

After looking at the APK content, maybe I should look at the download size, not raw size. If that's the case, it should be good to go
image

@cnevinc
Copy link
Contributor Author

cnevinc commented May 7, 2020

looked at the code @Dexterp37 wrote in his PR using [multiple APKS]
(https://developer.android.com/studio/build/configure-apk-splits)
And that gives us the APK size we want
image

We will not use multiple APKs, but I think per my discovery in above step, our APK size should not be affected.

I'd like to put this PR to Play Store and see the displayed APK size. Or I can build an internal release and see how it looks.

@cnevinc
Copy link
Contributor Author

cnevinc commented May 9, 2020

I've tested the current build and deploy to Play Store with my personal account [see].
With AAB and ABI split enabled, the APK just 6.3M.
image
So I think this could be landed after we fix the logic apk_size.py.
There are two approaches:

  1. Still check the APK size but raise the ceiling by 2.6M(the total of all ABI target .so files)
  2. Check the APKs generated from AAB and set the limit there.

The first approach can help us track size growth more precisely. Since we exclude the size of the Glean library. But the number is not the real number on Play Store.

The second approach is to

  • build app bundle
  • use bundletool to generate APK
  • check the total size of the APKs per device/split
    The second approach is more close to the file size on Play Store.

But since we have multiple target ABI, breaking one AIB's size limit won't necessarily break others. Thus make the APK size tracking hard.

@cnevinc cnevinc changed the title [DO NOT MERGE] Glean Integration POC Glean Integration May 14, 2020
@st3fan st3fan added the archived Archived Issue (Feel free to re-open if needed) label Jan 22, 2021
@st3fan st3fan closed this Jan 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived Archived Issue (Feel free to re-open if needed)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants