966485 - Remove telemetry bucket for FENNEC_STARTUP_TIME_ABOUTHOME

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

We dropped the code to stop the timer in Fx26. We still create the timer (mAboutHomeTimer) and we cancel() the timer when needed. But we don't stop() the timer.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Looks like the code was removed here:
http://hg.mozilla.org/mozilla-central/rev/0807f3d71b68

Comment 2

[Lucas Rocha (:lucasr)]

I don't think it makes sense to restore FENNEC_STARTUP_TIME_ABOUTHOME as it used to be. about:home is now configurable, which means that we wouldn't necessarily be comparing the same about:home setups i.e. some users might have the top sites as their default panel but others might hide all about:home panels, set a different default panel and so on. We wouldn't be benchmarking the same setup.

Not sure how to proceed here. Any thoughts?

Comment 3

[:Margaret Leibovic]

Maybe we can only take this start-up measurement if the user has the default set-up, and add a separate telemetry probe to measure how many users have customized their set-up.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

I think we need to pick a time point in the startup process and use it. If we decide to remove FENNEC_STARTUP_TIME_ABOUTHOME and create FENNEC_STARTUP_TIME_UI, so be it. But we need some telemetry on startup time.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

At this point, I'd be happy to WONTFIX this, but instead let's remove the unused telemetry bucket.

Comment 6

[Michael Holloway [:mdholloway]]

I can take this on.  Looks like it will involve removing FENNEC_STARTUP_TIME_ABOUTHOME from /toolkit/components/telemetry/Histograms.json and removing the references to it (via mAboutHomeStartupTimer) from /mobile/android/base/BrowserApp.java.  As for testing, I don't yet have Robocop set up -- should I do that first?

Comment 7

[:Margaret Leibovic]

(In reply to Michael Holloway from comment #6)
> I can take this on.  Looks like it will involve removing
> FENNEC_STARTUP_TIME_ABOUTHOME from
> /toolkit/components/telemetry/Histograms.json and removing the references to
> it (via mAboutHomeStartupTimer) from /mobile/android/base/BrowserApp.java.

That sounds like the right approach.

> As for testing, I don't yet have Robocop set up -- should I do that first?

I don't think we have any robocop tests specifically for this, so you can just make a build locally and play around with it to make sure nothing is broken, and then we'll push to try server for good measure before landing (running all of the robocop tests locally takes a very long time, try server is much better for this use-case :).

Comment 8

[Michael Holloway [:mdholloway]]

Attachment: FENNEC_STARTUP_TIME_ABOUTHOME.diff

Submitted for your review.  I played around a bit with a build with these changes, including with the WebIDE debugger attached, and it appears to work without issue.

Comment 9

[Richard Newman [:rnewman]]

Comment on attachment 8563824 [details] [diff] [review]
FENNEC_STARTUP_TIME_ABOUTHOME.diff

Review of attachment 8563824 [details] [diff] [review]:
-----------------------------------------------------------------

Nit: our commit messages are of the form:

  Bug 966485 - Remove telemetry bucket for FENNEC_STARTUP_TIME_ABOUTHOME. r=rnewman

Margaret might stop by with other comments, but this looks good to me!

Comment 10

[:Margaret Leibovic]

Comment on attachment 8563824 [details] [diff] [review]
FENNEC_STARTUP_TIME_ABOUTHOME.diff

Review of attachment 8563824 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for helping with the review, rnewman!

Michael, you should attach a new patch with the updated commit message, as rnewman suggested.

Do you have access to try server yet? If not, you should fill out the committer agreement here and request level 1 commit access:
https://www.mozilla.org/en-US/about/governance/policies/commit/

We can help teach you how to use try server, and once you have that, if you link to a passing try run in the bug and add the "checkin-needed" keyword, a sheriff will happily land your patch for you. But in the meantime, if you don't have access to try server yet, we can always land an updated version of the patch for you :)

Comment 11

[Michael Holloway [:mdholloway]]

Attachment: FENNEC_STARTUP_TIME_ABOUTHOME_revised.diff

Updated patch attached.  

I don't have try server access yet, but will request it -- Margaret, would you be willing to vouch for me?

Comment 12

[:Margaret Leibovic]

(In reply to Michael Holloway from comment #11)
> Created attachment 8564159 [details] [diff] [review]
> FENNEC_STARTUP_TIME_ABOUTHOME_revised.diff
> 
> Updated patch attached.  
> 
> I don't have try server access yet, but will request it -- Margaret, would
> you be willing to vouch for me?

Yes, indeed! You can just cc me to that bug and I will vouch.

We've been having some issues with test automation, so the trees are closed now, but I can help land this for you when they re-open.

Comment 13

[Michael Holloway [:mdholloway]]

So, I took a stab at pushing to the try server.  I selected the tests on TryChooser that seemed relevant to Android (no performance tests, since this change presumably wouldn't impact performance) to build my query, so hopefully I was on the right track there.  

This was my try query:

> try: -b do -p android-api-9,android-api-11,android-x86,emulator,emulator-jb,emulator-kk -u

This was the result:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=07df3b7ec40d

It looks like there are a handful of failing tests but I'm not sure whether/how they relate to my patch.

Comment 14

[:Margaret Leibovic]

(In reply to Michael Holloway from comment #13)
> So, I took a stab at pushing to the try server.  I selected the tests on
> TryChooser that seemed relevant to Android (no performance tests, since this
> change presumably wouldn't impact performance) to build my query, so
> hopefully I was on the right track there.  
> 
> This was my try query:
> 
> > try: -b do -p android-api-9,android-api-11,android-x86,emulator,emulator-jb,emulator-kk -u
> 
> This was the result:
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=07df3b7ec40d
> 
> It looks like there are a handful of failing tests but I'm not sure
> whether/how they relate to my patch.

Sorry for being so slow to respond! This looks good, these just look like intermittent issues that are not related to your patch.

So this is good to check in :)

Comment 15

[Michael Holloway [:mdholloway]]

Awesome!  Thanks again for your help.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/22cebd0c92a6

Comment 17

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

https://hg.mozilla.org/mozilla-central/rev/22cebd0c92a6