813677 - about:home telemetry shouldn't be measured if we don't show about:home

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Comment 1

[Wesley Johnston (:wesj)]

Attachment: Patch

I think this is better. This only sets the timer when we decide (in initializeChrome) to show about:home, and only kills it if we created it. We're missing a little bit of startup time, but I think this is the best we can do.

Comment 2

[Wesley Johnston (:wesj)]

Note this only measures if we have about:home passed to us from the outside. Maybe we'd rather just measure all about:home starts by starting the logging in the AboutHomeRunnable itself.

Comment 3

[Wesley Johnston (:wesj)]

Attachment: Patch v2

Updated based on an irc conversation with mfinkle.

Comment 4

[Wesley Johnston (:wesj)]

Comment on attachment 683704 [details] [diff] [review]
Patch v2

Want to make sure gcp is ok with this too.

Comment 5

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

Comment on attachment 683704 [details] [diff] [review]
Patch v2

>diff --git a/mobile/android/base/Telemetry.java b/mobile/android/base/Telemetry.java

>             GeckoAppShell.sendEventToGecko(event);
> 
>-            Log.v(LOGTAG, "Sending telemetry: " + jsonData.toString());

Might as well remove the trailing blank line too


The double Timer.cancel calls are my favorite thing, but I'd rather have clean data.

Comment 6

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 683704 [details] [diff] [review]
Patch v2

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

Negative from me - The code appears not to be doing what is described in the bug, and it's very arguable whether this is a good tradeoff if it did.

Cancelling the timer if we're not going to show about:home is fine and a good idea, that might avoid strange Telemetry if the user manually hits about:home later or something tike that. f+ on that idea.

::: mobile/android/base/BrowserApp.java
@@ +203,5 @@
>  
>      @Override
>      public void onCreate(Bundle savedInstanceState) {
> +        mAboutHomeStartupTimer = new Telemetry.Timer("FENNEC_STARTUP_TIME_ABOUTHOME");
> +

I don't see the point of this? The GeckoApp timer will be accessible here (it's protected, not private), because GeckoApp extend BrowserApp and that would avoid moving it around and potentially affecting the existing telemetry (which lucasr is already monitoring). Your change also makes the meaning of the data harder to interpret.

Lastly, your reasoning here was "This only sets the timer when we decide (in initializeChrome) to show about:home". But your code always initializes it as far as I can tell.

The advantage of not having the allocate a Java object, fetch the time, and keep a Long around in one startup path vs. having accurate Telemetry in a very important one is also quite arguable. If you'd move the code to where it would have to be to achieve your goal, you're going to end up doing quite some things of unknown speed inbetween.

@@ +676,5 @@
>                      });
>                      mAboutHomeContent.setLoadCompleteCallback(new AboutHomeContent.VoidCallback() {
> +                        public void callback() {
> +                            mAboutHomeStartupTimer.stop();
> +                        }

Whitespace noise?

Comment 7

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

(In reply to Gian-Carlo Pascutto (:gcp) from comment #6)

> ::: mobile/android/base/BrowserApp.java
> @@ +203,5 @@
> >  
> >      @Override
> >      public void onCreate(Bundle savedInstanceState) {
> > +        mAboutHomeStartupTimer = new Telemetry.Timer("FENNEC_STARTUP_TIME_ABOUTHOME");
> > +
> 
> I don't see the point of this? The GeckoApp timer will be accessible here
> (it's protected, not private), because GeckoApp extend BrowserApp and that
> would avoid moving it around and potentially affecting the existing
> telemetry (which lucasr is already monitoring). Your change also makes the
> meaning of the data harder to interpret.

I think it make more sense to have the timer in BrowserApp, where AboutHome and the Timer are actually used. Leaving it in GeckoApp is the wrong encapsulation.

The existing Telemetry should not be affected that much since the timer is still created in onCreate, but in the child not the super. In any case, the existing Telemetry is incredibly skewed right now, and is not helping Lucas.

I also don't follow the "harder to interpret" argument. The data will only be sent when about:home is actually shown. The existing data seems to show that most people open Firefox without seeing about:home, likely from external URLs. The telemetry is sent long after Firefox starts up, when user finally gets around to seeing the about:home page.

> Lastly, your reasoning here was "This only sets the timer when we decide (in
> initializeChrome) to show about:home". But your code always initializes it
> as far as I can tell.

s/sets timer/sends data

> The advantage of not having the allocate a Java object, fetch the time, and
> keep a Long around in one startup path vs. having accurate Telemetry in a
> very important one is also quite arguable. If you'd move the code to where
> it would have to be to achieve your goal, you're going to end up doing quite
> some things of unknown speed inbetween.

As written in the patch, we should have nearly the exact same start point as the current code, in onCreate.

> @@ +676,5 @@
> >                      });
> >                      mAboutHomeContent.setLoadCompleteCallback(new AboutHomeContent.VoidCallback() {
> > +                        public void callback() {
> > +                            mAboutHomeStartupTimer.stop();
> > +                        }
> 
> Whitespace noise?

But worth fixing IMO

Comment 8

[Gian-Carlo Pascutto [:gcp]]

Bugzilla ate my comments :-/

1) You're right that the start of the timer is really in exactly the same place, this voids most of my comments. Your code only cancels the timer (stops Telemetry from sending) which is fine, instead of moving the start around to when we know what to do (which wouldn't have been more problematic), so I'm OK with this!

2) "The existing data seems to show that most people open Firefox without seeing about:home, likely from external URLs." This isn't true:
FENNEC_STARTUP_GECKOAPP_ACTION shows 68% arriving at about:home, the rest for opening via URLs.

Comment 9

[Gian-Carlo Pascutto [:gcp]]

s/which wouldn't have been more problematic/which would have been more problematic/

>>Whitespace noise?
>But worth fixing IMO

I think we're generally only supposed to fix it when actually editing the line. It makes hg blame quite a bit more cumbersome if you fix unrelated whitespace.

Comment 10

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2b300216e791

Comment 11

[Wesley Johnston (:wesj)]

Comment on attachment 683704 [details] [diff] [review]
Patch v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Since about:home telemetry was landed
User impact if declined: None. Our telemetry is just off
Testing completed (on m-c, etc.): Landed on mc today
Risk to taking this patch (and alternatives if risky): Low risk. Moves some code to be encapsulated away from webapps, and basically does nothing in the case where the ping should not happen.
String or UUID changes made by this patch: None.

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2b300216e791

Comment 13

[Alex Keybl [:akeybl]]

Comment on attachment 683704 [details] [diff] [review]
Patch v2

Low risk fix in support of better Telemetry data. Approving for Aurora 19.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/78fbf1d13fc8