Closed Bug 1248066 Opened 4 years ago Closed 3 years ago

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

Categories

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

defect
Not set

Tracking

(firefox47+ verified, firefox48+ fixed, firefox49 fixed)

VERIFIED FIXED
mozilla49
Tracking Status
firefox47 + verified
firefox48 + fixed
firefox49 --- fixed

People

(Reporter: nalexander, Assigned: mcomella)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
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/
Attachment #8719036 - Flags: review?(mh+mozilla)
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?
Attachment #8719036 - Flags: review?(mh+mozilla)
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.
(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?
Flags: needinfo?(mh+mozilla)
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.
Flags: needinfo?(mh+mozilla)
Blocks: 1248764
(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.
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.
Flags: needinfo?(nalexander)
margaret: I think this is valuable enough to have you re-assign it.  Patch should still be the same.
Flags: needinfo?(nalexander) → needinfo?(margaret.leibovic)
Bug 1245304 is now fixed in 47. So whatever happens here should be aimed at 47.
Mike, can you pick this up?
Assignee: nalexander → michael.l.comella
Flags: needinfo?(margaret.leibovic) → needinfo?(michael.l.comella)
Assigned and on my radar so clearing NI.
Flags: needinfo?(michael.l.comella)
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.
Flags: needinfo?(nalexander)
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.
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.
(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 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!
Attachment #8719036 - Flags: review?(mh+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.
Attachment #8719036 - Flags: review?(mh+mozilla) → review?(nalexander)
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 :)
Attachment #8719036 - Flags: review?(nalexander) → review+
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 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
Attachment #8719036 - Flags: approval-mozilla-beta?
Attachment #8719036 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/07f2b1105767
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
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.
Attachment #8719036 - Flags: approval-mozilla-beta?
Attachment #8719036 - Flags: approval-mozilla-beta+
Attachment #8719036 - Flags: approval-mozilla-aurora?
Attachment #8719036 - Flags: approval-mozilla-aurora+
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.
Flags: needinfo?(michael.l.comella)
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.
(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/
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)
Status: RESOLVED → VERIFIED
Flags: needinfo?(nalexander)
Flags: needinfo?(michael.l.comella)
Flags: needinfo?(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.
Flags: needinfo?(adavis)
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.
(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).
I hope we're not making Adjust pings from automation...
(In reply to :Margaret Leibovic from comment #31)
> I hope we're not making Adjust pings from automation...

Filed bug 1275669.
Product: Firefox for Android → Firefox Build System
Target Milestone: Firefox 49 → mozilla49
You need to log in before you can comment on or make changes to this bug.