Closed Bug 966485 Opened 6 years ago Closed 5 years ago

Remove telemetry bucket for FENNEC_STARTUP_TIME_ABOUTHOME

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 38
Tracking Status
firefox38 --- fixed

People

(Reporter: mfinkle, Assigned: mdholloway, Mentored)

Details

(Whiteboard: [lang=js][good first bug])

Attachments

(1 file, 1 obsolete file)

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.
Looks like the code was removed here:
http://hg.mozilla.org/mozilla-central/rev/0807f3d71b68
Assignee: nobody → lucasr.at.mozilla
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?
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.
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.
At this point, I'd be happy to WONTFIX this, but instead let's remove the unused telemetry bucket.
Mentor: margaret.leibovic
Summary: Add back support for FENNEC_STARTUP_TIME_ABOUTHOME → Remove telemetry bucket for FENNEC_STARTUP_TIME_ABOUTHOME
Whiteboard: [lang=js]
Assignee: lucasr.at.mozilla → nobody
Whiteboard: [lang=js] → [lang=js][good first bug]
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?
Flags: needinfo?(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 :).
Flags: needinfo?(margaret.leibovic)
Assignee: nobody → michaeldavidholloway
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.
Attachment #8563824 - Flags: review?(margaret.leibovic)
Status: NEW → ASSIGNED
OS: Mac OS X → Android
Hardware: x86 → All
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!
Attachment #8563824 - Flags: review?(margaret.leibovic)
Attachment #8563824 - Flags: review+
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 :)
Attachment #8563824 - Flags: review?(margaret.leibovic) → review+
Updated patch attached.  

I don't have try server access yet, but will request it -- Margaret, would you be willing to vouch for me?
Attachment #8563824 - Attachment is obsolete: true
Attachment #8564159 - Flags: review?(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.
Attachment #8564159 - Flags: review?(margaret.leibovic) → review+
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.
Flags: needinfo?(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 :)
Flags: needinfo?(margaret.leibovic)
Keywords: checkin-needed
Awesome!  Thanks again for your help.
https://hg.mozilla.org/integration/fx-team/rev/22cebd0c92a6
Keywords: checkin-needed
Whiteboard: [lang=js][good first bug] → [lang=js][good first bug][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/22cebd0c92a6
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [lang=js][good first bug][fixed-in-fx-team] → [lang=js][good first bug]
Target Milestone: --- → Firefox 38
You need to log in before you can comment on or make changes to this bug.