1248066 - Use separate Adjust token for Firefox for Android release and beta builds

Status: VERIFIED FIXED

(Firefox Build System :: Android Studio and Gradle Integration, defect)

Description

[Nick Alexander :nalexander [he/him]]

In theory, this is simple: just point to different files in the release and beta mozconfig files.

Of course, nothing is simple -- there is no release-specific mozconfig file.  Instead, there's a script that migrates the beta mozconfig forward.  (See https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/configs/merge_day/beta_to_release.py#12.)  I can't explain this, but it means the obvious thing is unavailable.

The next obvious thing is to bake this setting into the per-channel branding, but it seems wrong to bake a Mozilla-automation *path* into mobile/android/branding/official.  In #releng, aki agreed.

So that leaves us executing shell in mobile/android/config/mozconfigs/common and testing the update channel.  There's precedent for this: https://dxr.mozilla.org/mozilla-central/source/browser/confvars.sh#11.

Comment 1

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1248066 - Use separate Adjust token for Firefox for Android release and beta builds. r?glandium

The token is only used for release and beta builds, so it's better not
to define it inadvertently for all builds.

Review commit: https://reviewboard.mozilla.org/r/34847/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/34847/

Comment 2

[Mike Hommey [:glandium]]

Comment on attachment 8719036 [details]
MozReview Request: Bug 1248066 - Use separate Adjust token for Firefox for Android release and beta builds. r?glandium

https://reviewboard.mozilla.org/r/34847/#review31579

::: mobile/android/config/mozconfigs/common:52
(Diff revision 1)
> +if test "$MOZ_UPDATE_CHANNEL" = "release" ; then
> -ac_add_options --with-adjust-sdk-keyfile=/builds/adjust-sdk.token
> +    ac_add_options --with-adjust-sdk-keyfile=/builds/adjust-sdk.token
> +elif test "$MOZ_UPDATE_CHANNEL" = "beta" ; then
> +    ac_add_options --with-adjust-sdk-keyfile=/builds/adjust-sdk-beta.token
> +fi

Do we really want one more way for things to differ between aurora/nightly and beta/release, and have problems only appear when reaching beta?

Comment 3

[Nick Alexander :nalexander [he/him]]

https://reviewboard.mozilla.org/r/34847/#review31579

> Do we really want one more way for things to differ between aurora/nightly and beta/release, and have problems only appear when reaching beta?

Things already differ between aurora/nightly and beta/release; this will make the build break when we get it wrong.  But in general, yes, we don't want to track Aurora and Nightly installs using Adjust, for I can't recall but vaguely remember have to do with the installs already being partially tracked via the Play Store.  It might be worth revisiting that decision.

As for the actual token, yes -- we really want things to differ between beta and release.

Comment 4

[Nick Alexander :nalexander [he/him]]

(In reply to Nick Alexander :nalexander from comment #3)
> https://reviewboard.mozilla.org/r/34847/#review31579
> 
> > Do we really want one more way for things to differ between aurora/nightly and beta/release, and have problems only appear when reaching beta?
> 
> Things already differ between aurora/nightly and beta/release; this will
> make the build break when we get it wrong.  But in general, yes, we don't
> want to track Aurora and Nightly installs using Adjust, for I can't recall
> but vaguely remember have to do with the installs already being partially
> tracked via the Play Store.  It might be worth revisiting that decision.
> 
> As for the actual token, yes -- we really want things to differ between beta
> and release.

glandium: what do you want to see here?

Comment 5

[Mike Hommey [:glandium]]

I don't want you to realize that the code handling those tokens is broken by something else once the changes have reached beta at any time in the future. But if you're fine with that, then meh, I guess.

Comment 6

[Nick Alexander :nalexander [he/him]]

(In reply to Mike Hommey [:glandium] from comment #5)
> I don't want you to realize that the code handling those tokens is broken by
> something else once the changes have reached beta at any time in the future.
> But if you're fine with that, then meh, I guess.

Nah, not fine, but this is on a different timeline.  I've filed Bug 1248764 to unify the situation in terms of build and release, but we want to land and uplift this ASAP.

Comment 7

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Bug 1245304 depends on this bug and we're tracking it for 45.  Does something need to happen here before the 45 release next week? Thanks.

Comment 8

[Nick Alexander :nalexander [he/him]]

margaret: I think this is valuable enough to have you re-assign it.  Patch should still be the same.

Comment 9

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Bug 1245304 is now fixed in 47. So whatever happens here should be aimed at 47.

Comment 10

[:Margaret Leibovic]

Mike, can you pick this up?

Comment 11

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Assigned and on my radar so clearing NI.

Comment 12

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Based on talking with mcomella this may not be fixed in time for 47. 
Nick, will that mess anything up? I am not clear whether we need this to ship along with bug 1245304. If not, we can wontfix this bug for 47 and try for 48.

Comment 13

[Kevin Brosnan [Ex-Mozilla]]

The risk here is that we won't be able to detect adjust breakage during the Beta cycle. Currently the Beta and Release populations report the same ID. We may get into a situation like bug 1244930 / bug 1233238 again. The beta population install rate is drowned out by the release information.

Reading the comments here is there anything to do besides land the patch? I don't see anything major called out in the Nick/Glandium discussion.

Comment 14

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Looking into this, it is my understanding that bug 1245304 uploaded the adjust-sdk-beta.token file to the build slaves and set up the scaffolding to access it while the patch in this bug actually puts that into production.

(In reply to Kevin Brosnan [:kbrosnan] from comment #13)
> Reading the comments here is there anything to do besides land the patch? I
> don't see anything major called out in the Nick/Glandium discussion.

I think we'd need a proper r+ but otherwise, no – it seems like we just need to land the patch.

To be honest, I'm not familiar enough with the adjust configs to know exactly how this all works but if this patch breaks anything, I'd expect it to lose the Beta data because the sdk token is improperly configured. Since we don't want Beta in the release data and, as Kevin notes in comment 13, it gets drowned out by the release data, this wouldn't be a terrible loss if we screwed up.

On the other hand, release uses the same token and should be unaffected, assuming the "if" statement is correct (and I assume glandium can attest that it is).

So I'd say let's get an r+ and land/uplift this sooner, rather than later, to test how it'd work.

Comment 15

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

(In reply to Michael Comella (:mcomella) from comment #14)
> this wouldn't be a terrible loss if we screwed up.

And we could always back this out of Beta if it breaks so the patch never hits release (even though I think it'd be fine).

Comment 16

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Comment on attachment 8719036 [details]
MozReview Request: Bug 1248066 - Use separate Adjust token for Firefox for Android release and beta builds. r?glandium

Glandium, from the previous discussion, it seems that we do want the divided release/beta configs – we don't expect this code to change often and Adjust doesn't give us much flexibility so it should be okay (and probably necessary) to have that divide.

Please review ASAP so we can get this uplifted!

Comment 17

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #12)
> Nick, will that mess anything up? I am not clear whether we need this to
> ship along with bug 1245304. If not, we can wontfix this bug for 47 and try
> for 48.

Looking at bug 1245304, we duplicate all the code for the adjust token for the adjust beta token (i.e. don't remove what's already existing) so I think it should be fine to ship bug 1245304 without this bug.

Comment 18

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8719036 [details]
MozReview Request: Bug 1248066 - Use separate Adjust token for Firefox for Android release and beta builds. r?glandium

https://reviewboard.mozilla.org/r/34847/#review49467

I'm fine with this.  I'm pretty sure it was my patch originally :)

Comment 19

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

https://hg.mozilla.org/integration/fx-team/rev/07f2b11057670f34f67b35c9c436cc9acbee676c
Bug 1248066 - Use separate Adjust token for Firefox for Android release and beta builds. r=nalexander

Comment 20

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Comment on attachment 8719036 [details]
MozReview Request: Bug 1248066 - Use separate Adjust token for Firefox for Android release and beta builds. r?glandium

Approval Request Comment
[Feature/regressing bug #]: Inspired by bug 1244930.
[User impact if declined]: The Beta & Release Adjust numbers are in the same pool and release overshadows the beta numbers so if something breaks in beta, we don't see it until it hits release. This patch should set us up to have two separate pools for our beta and release users.
[Describe test coverage new/current, TreeHerder]: None – perhaps nalexander has done more testing (if it's even possible)
[Risks and why]: Medium – I don't have full context on these changes but see comment 14 for my thoughts
[String/UUID change made/needed]: None

Comment 21

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/07f2b1105767

Comment 22

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8719036 [details]
MozReview Request: Bug 1248066 - Use separate Adjust token for Firefox for Android release and beta builds. r?glandium

Let's uplift so we can test on Beta47 soon, RC week is still 2 weeks away.

Comment 23

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Mike, could you mark this bug as verified once we know that Adjust data is coming in on Beta47 as expected and this fix is doing what it's supposed to do? This fix if it lands by tomm noon PST will be included in 47.0b6.

Comment 24

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Slightly passed noon :\ but...

https://hg.mozilla.org/releases/mozilla-beta/rev/2c182e75d6cc

& Aurora is currently closed.

I'll verify once this gets into a beta build.

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/54854c4cb6fb

Comment 26

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

(In reply to Michael Comella (:mcomella) from comment #24)
> I'll verify once this gets into a beta build.

This made it into beta 47.0b6 [1]. Once this gains traction in the Play Store, we can do a proper verification but that being said, I already see some installs/sessions in the adjust dashboard.

[1]: I verified via https://archive.mozilla.org/pub/mobile/releases/47.0b6/source/

Comment 27

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

I have started to see a decrease in our release statistics and large number of installs into our Beta statistics, as expected. The numbers don't exactly correlate yet (but I imagine that's because the data, or the beta release on Google Play, hasn't propagated yet). This sounds good to me.

Alex, can you verify that the numbers on Adjust correlate with the numbers on Google Play and look correct to you?

Note: I can't actually verify this on 48 & 49 until those releases merge into Beta, but I trust that it'll work (because they're the same patch)

Comment 28

[Alex Davis [:adavis]]

The numbers don't line up yet but this is normal. All our existing Beta users will also show up as new users when the sdk tracks them for the first time. It might take a week or so for the numbers to stabilize.

In summary, what I'm observing is what I'd expect from this change. It's just hard to confirm with exact numbers for now.

Comment 29

[Joel Maher ( :jmaher ) (UTC -8)]

sort of odd (in a good way), we have a bunch of autophone improvements that appears to be related to this change- on beta only:
https://treeherder.mozilla.org/perf.html#/alerts?id=1285

these improvements do not show up on aurora/central/inbound/fx-team where we have data on autophone.  Possibly there is some other root cause for these improvements- but either way it is nice to see them.

Comment 30

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

(In reply to Joel Maher (:jmaher) from comment #29)
> sort of odd (in a good way), we have a bunch of autophone improvements that
> appears to be related to this change- on beta only:

This is very unexpected – I don't know why that would be!

It's possible the Adjust SDK is affecting our performance in some way. Perhaps the new end-point is faster than the old one (e.g. it doesn't get as much traffic so it's faster to upload, relieving a background thread which gives us perf improvements).

Comment 31

[:Margaret Leibovic]

I hope we're not making Adjust pings from automation...

Comment 32

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

(In reply to :Margaret Leibovic from comment #31)
> I hope we're not making Adjust pings from automation...

Filed bug 1275669.