1261830 - Content notifications: Start telemetry sessions

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: MozReview Request: Bug 1261830 - Content notifications: Stamp telemetry events with experiment session. r?margaret

Review commit: https://reviewboard.mozilla.org/r/44319/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/44319/

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

The very first action is when we subscribe to a feed. That's where I start the session now. Is this the best place? Or should the session start later when we show a notification or the user clicks on it? I guess I should start to look at the data and do some evaluations to understand what we are looking for.

Comment 3

[:Margaret Leibovic]

(In reply to Sebastian Kaspari (:sebastian) from comment #2)
> The very first action is when we subscribe to a feed. That's where I start
> the session now. Is this the best place? Or should the session start later
> when we show a notification or the user clicks on it? I guess I should start
> to look at the data and do some evaluations to understand what we are
> looking for.

We should understand when sessions end. I thought they ended automatically when gecko is killed, so this may not work as expected, but maybe I'm wrong.

I wonder how well this even works if it's called from an Android background service. The implementation leads me to here:
http://mxr.mozilla.org/mozilla-central/source/widget/android/nsAppShell.cpp#714

And it looks like this ends up calling into UITelemetry.startSession:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/UITelemetry.jsm#129

Looking in this file, I don't actually see logic where we automatically end sessions, so maybe this works as expected?

NI to mfinkle to see if he has more context around the behavior of these UI telemetry sessions.

Comment 4

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

Sessions can be a PITA, especially when we don't really have a "session", like this situation. Typically, sessions are used for things like "showing History panel" which has a start and end. 

--
Some history: We thought Sessions would be important to track in their own right, but turns out, they are not that useful. We also stamped all Events with the active Sessions. That turned out to be more useful, because it adds a different kind of context to an Event.

A good example of this is "firstrun.1" Sessions. Started, but never stopped because we never know when Android stops the application. So "firstrun.1" never appears as a Session itself (must be stopped for that), but it is stamped on every Event that fires in the user's initial launch of Fennec. Yes, Sessions are cleared on subsequent app launches.

We don't even save the standalone Sessions in Redash. Just the Session stamped into Events.
--

In this case, we are looking to stamp Sessions (Experiments) onto Events to help with tracking A/B experiments. Given that we are stretching the definition of a Session used in a background service and we only really care about the stamped Event, why not use this simple minded pattern:

startTelemetrySession();
Telemetry.sendUIEvent(TelemetryContract.Event.SAVE, TelemetryContract.Method.SERVICE, "content_update");
stopTelemetrySession();

That will give us the Event stamping we want. We will be able to tell that the content_update was saved using a specific heuristic.

Wait, as I write this it occurs to me that this Event is no the one we care about is it? We want to stamp the (SHOW, NOTIFICATION, "content_update") event, right? That's the one that is controlled by the experiment, right?

Comment 5

[:Margaret Leibovic]

Comment on attachment 8738103 [details]
MozReview Request: Bug 1261830 - Content notifications: Stamp telemetry events with experiment session. r?margaret

https://reviewboard.mozilla.org/r/44319/#review41289

Given mfinkle's last comment, let's just surround the events we care about with sessions, so that we can tag them appropriately.

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8738103 [details]
MozReview Request: Bug 1261830 - Content notifications: Stamp telemetry events with experiment session. r?margaret

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44319/diff/1-2/

Comment 7

[:Margaret Leibovic]

Comment on attachment 8738103 [details]
MozReview Request: Bug 1261830 - Content notifications: Stamp telemetry events with experiment session. r?margaret

https://reviewboard.mozilla.org/r/44319/#review42007

Clunky, but gets the job done. We should think about if there are better patterns for this type of analysis that might emerge in the future.

::: mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferences.java:391
(Diff revision 2)
>          }
>  
>          // Launched from "Notifications settings" action button in a notification.
>          if (intentExtras != null && intentExtras.containsKey(CheckForUpdatesAction.EXTRA_CONTENT_NOTIFICATION)) {
> +            Telemetry.startUISession(TelemetryContract.Session.EXPERIMENT, FeedService.getEnabledExperiment(this));
>              Telemetry.sendUIEvent(TelemetryContract.Event.ACTION, Method.BUTTON, "notification-settings");

This is the only notification with this button right now, right?

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to :Margaret Leibovic from comment #7)
> This is the only notification with this button right now, right?

Yeah, correct!

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

https://hg.mozilla.org/integration/fx-team/rev/adbe62e4a9748eb9570e9bf72eaf7bd19af4da2b
Bug 1261830 - Content notifications: Stamp telemetry events with experiment session. r=margaret

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/adbe62e4a974