Closed Bug 1138560 Opened 7 years ago Closed 6 years ago

Add telemetry for web pages launched from home screen shortcuts.

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(firefox42 fixed)

RESOLVED DUPLICATE of bug 1023306
Firefox 42
Tracking Status
firefox42 --- fixed

People

(Reporter: wesj, Assigned: alex, Mentored)

Details

(Whiteboard: [lang=java][good next bug])

Attachments

(2 files, 2 obsolete files)

We should add something to the "Add to homescreen" links in Firefox so that we can track when they're clicked. We can just put an extra on the intent and read it out quickly when we're launched.
Mentor: wjohnston
Whiteboard: [lang=java][good next bug]
Attached patch bug-1138560-v1.patch (obsolete) — Splinter Review
As suggested by mcomella in my first bug, I'd like to work on this. I already created a patch and need some feedback.

The aim of this bug is to measure the time spans between the creation of a home screen shortcut and all times the user opens the shortcut. Is that right?

I created a new string constant and named it "EXTRA_HOMESCREEN_SHORTCUT_CREATION_TIME" to store the creation time. The name describes exactly what it's used for, but I don't know whether it's too long or not.

The time span is calculated by using the values out of System.currentTimeMillis(). I'm using this function because the SystemClock values are reset on every system boot. The problematic part about this is that the value can and will change sometimes.

The measured time span gets submit with Telemetry.sendUIEvent(TelemetryContract.Event.ACTION, TelemetryContract.Method.HOMESCREEN, timeSpan);
All of that line is guessed by comparing it to other places. I probably have to change things here ;).

Additional note: When reading the value of the intent extra, there is an check whether the value equals zero, the default value. This is used to skip old shortcuts that do not have this extra set.
Attachment #8605882 - Flags: feedback?(wjohnston)
Cool. I don't even think we need the timestamp code here TBH. I just wanted a count of how many times you launched from one of these. You'll need to add a key to the Telemetry.json file:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/Histograms.json#7775

and then store it like you are here.
Assignee: nobody → alex
Alex: I'd really like to see this happen.  I see that you have a patch and some feedback -- can I help get this moving again?
Flags: needinfo?(alex)
Mentor: nalexander
Sorry for not updating this for such a long time.

I've updated the patch now:
- removed the timestamp code
- removed the intent extra (I'm now using the existing intent action ACTION_HOMESCREEN_SHORTCUT only)
- added a new histogram FENNEC_HOMESCREEN_SHORTCUT_LAUNCHES to Histograms.json

Is the histogram entry and its kind "count" okay?
(I also found FENNEC_STARTUP_GECKOAPP_ACTION, should those two histograms be combined?)



Now I'm searching for a way to record those launches within the Java code.

If I use the following line, there is still a connection to the histogram needed, but I don't know where to do this.
> Telemetry.sendUIEvent(TelemetryContract.Event.ACTION, TelemetryContract.Method.HOMESCREEN);

(There is also the method addToHistogram() where I can specify the histogram name directly, but this method is probably not suitable for this, because I have to increase the value.)
> Telemetry.addToHistogram("FENNEC_HOMESCREEN_SHORTCUT_LAUNCHES", 1);

I've searched at places like the Mozilla Wiki and in related Android bugs but I wasn't successful in finding a solution for that. Can someone point me to an Java example or give me some help in here?
Flags: needinfo?(alex) → needinfo?(nalexander)
I'm not sure who "wants" the needinfo flag ;).
Flags: needinfo?(wjohnston)
Comment on attachment 8632552 [details] [diff] [review]
Updated patch recording to feedback.

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

Alexander: thanks for pushing this forward!

For UI Telemetry, I think everything is set up: there's no need to add to Histograms.json.  Flagging liuche to confirm.

::: mobile/android/base/GeckoApp.java
@@ +1837,5 @@
>          } else if (ACTION_HOMESCREEN_SHORTCUT.equals(action)) {
>              String uri = getURIFromIntent(intent);
>              GeckoAppShell.sendEventToGecko(GeckoEvent.createBookmarkLoadEvent(uri));
> +
> +            Telemetry.sendUIEvent(TelemetryContract.Event.ACTION, TelemetryContract.Method.HOMESCREEN);

I think this is right: we want Event.LOAD_URL and Method.HOMESCREEN here.  There's no need to reference a particular histogram, IIUC.
Attachment #8632552 - Flags: review?(liuche)
Attachment #8632552 - Flags: feedback+
Flags: needinfo?(wjohnston)
Flags: needinfo?(nalexander)
(In reply to Nick Alexander :nalexander from comment #6)
> For UI Telemetry, I think everything is set up: there's no need to add to
> Histograms.json.  Flagging liuche to confirm.
Oooh, okay, that might be the reason why I haven't found an example with Telemetry.sendUIEvent() and Histograms.json together :D. Now I probably understand the difference between those two ;).


> I think this is right: we want Event.LOAD_URL and Method.HOMESCREEN here.
Yep, now you are saying it, it totally makes sense to me.


I've updated the patch so that it's immediately ready if liuche says that everything is right.
Attachment #8605882 - Attachment is obsolete: true
Attachment #8605882 - Flags: feedback?(wjohnston)
Attachment #8632552 - Attachment is obsolete: true
Attachment #8632552 - Flags: review?(liuche)
Comment on attachment 8632870 [details] [diff] [review]
bug-1138560-v3.patch

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

Yep, that's correct! You don't need a histogram for UI Telemetry in Fennec.
Attachment #8632870 - Flags: review+
Ok, now that the patch seems okay, how should I go on? :)

Last time (bug 1148549) someone else pushed my patch to the try servers. (That's my second bug.)
Flags: needinfo?(nalexander)
(In reply to Alexander Ploner from comment #9)
> Ok, now that the patch seems okay, how should I go on? :)
> 
> Last time (bug 1148549) someone else pushed my patch to the try servers.
> (That's my second bug.)

I don't think we'll need a try push here.  liuche gave the r+, so we just need to add [checkin-needed] to the keywords.  (Which I'll do now.)  Then the sheriffs will come and land the patch.
Status: NEW → ASSIGNED
Flags: needinfo?(nalexander)
Keywords: checkin-needed
OS: Mac OS X → Android
Hardware: x86 → All
Perfect, thank you!
(In reply to Alexander Ploner from comment #11)
> Perfect, thank you!

Excellent.  Alex, I'd like to work with you on some additional things...  but I don't know much about what you want to work on, and I'm going to be on holiday until the end of July.  So I'd like to try to connect when I get back, and in the meantime, perhaps liuche has a good next ticket for you.

liuche: can you chime in?
Flags: needinfo?(liuche)
Flags: needinfo?(liuche)
Hey Alexander! What sort of bugs would you be interested in working on? If you want to do a few frontend bugs, here are a few that you could take a look at: bug 1179020, bug 1183149, bug 1175375 ?
https://hg.mozilla.org/mozilla-central/rev/2d12d2f2f450
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
If I look into the stats (see screenshot from [1]) there are already values for all release channels. Is this intended? Where does this data come from? I'm wondering if this patch mixes up two different things?


(In reply to Chenxia Liu [:liuche] from comment #14)
> Hey Alexander! What sort of bugs would you be interested in working on? If
> you want to do a few frontend bugs, here are a few that you could take a
> look at: bug 1179020, bug 1183149, bug 1175375 ?

Thanks for your suggestions. Seems like all bugs are taken already :D. I had to learn for exams so I didn't have time to have a look into them. I'll write you an email as soon as I feel able to take new bugs ;).

[1] https://people.mozilla.org/~mfinkle/uitelemetry/
I see four uses of HOMESCREEN in the tree:
http://mxr.mozilla.org/mozilla-central/search?string=Method.HOMESCREEN

Two are related to launching from homescreen icons:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#3664
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp.java#1877

These are both fired in onNewIntent, and I think we are now double counting. The existing BrowserApp version should have been good enough. Looks like LucasR added the probe in bug 1023306.

I think we need to back this patch out. Sorry.
Ah, sorry about that, I should have caught that. Good catch, mfinkle.

I'm duping this, and I've backed it out: https://hg.mozilla.org/integration/fx-team/rev/4fe45a08aeba
Resolution: FIXED → DUPLICATE
Duplicate of bug: 1023306
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.