1273689 - Consider adding core telemetry ping upload for end of first session to capture session usage data

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

We upload in onStart because:
 1) we want to ping the server ASAP (e.g. before a possible crash)
 2) An alternative, onStop, is not guaranteed to be called
 3) We don't want to upload in onPause/onResume because it occurs too frequently (e.g. when fennec is obscured by dialogs).

However, if a user uses fennec for the first time and decides never to open it, we won't receive their session data – do we care? Is it worth the complexity to capture this data?

I think it depends on how we plan to use this data in the future and if that first run is important.

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Margaret, do you know how important this data is? Or if not, who I should talk to to find out?

Comment 2

[:Margaret Leibovic]

I think this is a good question for Barbara.

Comment 3

[Barbara Bermes [:barbara] - NI please!]

This is considered _very important data_ for product, so please yes!

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Barbara Bermes [:barbara] from comment #3)
> This is considered _very important data_ for product, so please yes!

Barbara, I was going to implement this so we only get this information for the very first session but I realized it's possible a user may stop using Firefox on the second or third time. Is there a particular number of sessions after which I should stop trying to upload their last session information?

The downside of trying to upload all of the time (both when the browser is opened & closed) is that we'll be using more user data/battery.

fwiw, I came up with a more comprehensive, but less robust implementation idea in bug 1277091. It's simple to implement but I'm afraid of the long tail of debugging needs so I don't think I'll have time to implement it.

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1273689 - Move core ping upload to BrowserAppDelegate. r=sebastian

This lets us put the two paths of upload code all in the same place.

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

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1273689 - Elaborate on why we upload when we do. r=sebastian

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

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1273689 - Factor out getSharedPreferences call. r=sebastian

The same preferences will be used by the new code & the old code.

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

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1273689 - Move session measurement calls to CorePingDelegate. r=sebastian

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

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1273689 - Upload in onStop for first run. r=sebastian

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

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1273689 - Rename CorePingDelegate. r=sebastian

We are doing more than just uploading in the delegate now.

I didn't fix this in the previous commits because version control makes this
non-trivial.

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

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

The pushed implementation only gets the users' last session info for the very first session (see comment 4 for more).

Comment 12

[Barbara Bermes [:barbara] - NI please!]

That's fine as per comment 11.

Comment 13

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8758472 [details]
MozReview Request: Bug 1273689 - Move core ping upload to BrowserAppDelegate. r=sebastian

https://reviewboard.mozilla.org/r/56712/#review53578

Nice!

::: mobile/android/base/java/org/mozilla/gecko/telemetry/TelemetryCorePingUploadDelegate.java:52
(Diff revision 1)
> +        final SearchEngineManager searchEngineManager = browserApp.getSearchEngineManager();
> +        searchEngineManager.getEngine(new UploadTelemetryCorePingCallback(browserApp));
> +    }
> +
> +    @WorkerThread // via constructor
> +    private TelemetryDispatcher getTelemetryDispatcher(final BrowserApp browserApp) {

I wonder why this method is part of the outer class if it's only used by the inner class.

::: mobile/android/base/java/org/mozilla/gecko/telemetry/TelemetryCorePingUploadDelegate.java:61
(Diff revision 1)
> +        }
> +        return telemetryDispatcher;
> +    }
> +
> +    private class UploadTelemetryCorePingCallback implements SearchEngineManager.SearchEngineCallback {
> +        private final WeakReference<BrowserApp> activityWeakReference;

You can use BrowserAppDelegateWithReference if you do not want to handle the reference yourself.

However if you move it inside UploadTelemetryCorePingCallback there's not much left in the outer delegate class. Maybe the delegate could implement SearchEngineManager.SearchEngineCallback directly and then there wouldn't be any inner class?

Comment 14

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8758473 [details]
MozReview Request: Bug 1273689 - Elaborate on why we upload when we do. r=sebastian

https://reviewboard.mozilla.org/r/56714/#review53586

Comment 15

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8758474 [details]
MozReview Request: Bug 1273689 - Factor out getSharedPreferences call. r=sebastian

https://reviewboard.mozilla.org/r/56716/#review53612

Comment 16

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8758475 [details]
MozReview Request: Bug 1273689 - Move session measurement calls to CorePingDelegate. r=sebastian

https://reviewboard.mozilla.org/r/56718/#review53614

Comment 17

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8758476 [details]
MozReview Request: Bug 1273689 - Upload in onStop for first run. r=sebastian

https://reviewboard.mozilla.org/r/56720/#review53616

Comment 18

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8758477 [details]
MozReview Request: Bug 1273689 - Rename CorePingDelegate. r=sebastian

https://reviewboard.mozilla.org/r/56722/#review53618

::: mobile/android/base/java/org/mozilla/gecko/telemetry/TelemetryCorePingDelegate.java:1
(Diff revision 1)
> +/*

Is it just reviewboard that gets confused and shows this as a delete and an add?

Comment 19

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://reviewboard.mozilla.org/r/56722/#review53618

> Is it just reviewboard that gets confused and shows this as a delete and an add?

Yeah, I think so. Then again, hg seems to model renames as delete/adds in some contexts as well.

Comment 20

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://reviewboard.mozilla.org/r/56712/#review53578

> You can use BrowserAppDelegateWithReference if you do not want to handle the reference yourself.
> 
> However if you move it inside UploadTelemetryCorePingCallback there's not much left in the outer delegate class. Maybe the delegate could implement SearchEngineManager.SearchEngineCallback directly and then there wouldn't be any inner class?

I like it! Done.

Comment 21

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://hg.mozilla.org/integration/fx-team/rev/93325fc7131cb36584c85641132090ca8ba40cdd
Bug 1273689 - Move core ping upload to BrowserAppDelegate. r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/6d3576194df89669978874205caacd4ef5196451
Bug 1273689 - Elaborate on why we upload when we do. r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/ab5501b1b7d5ebdae96199d1bc6ceaf3defe35f9
Bug 1273689 - Factor out getSharedPreferences call. r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/e28024927ff6727dd04f58244fe663f9a0290043
Bug 1273689 - Move session measurement calls to CorePingDelegate. r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/11a6d5a59addedc2f282e9c4d33cc2884dad00cf
Bug 1273689 - Upload in onStop for first run. r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/8644fcfe29da58befd2c3ea28e230570570af925
Bug 1273689 - Rename CorePingDelegate. r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/7c0dce574b3f2d5e8f6e4e60caab5532f8e13fd6
Bug 1273689 - review - remove inner class & move methods to containing class; use BrowserAppDelegateWithReference. r=me

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/93325fc7131c
https://hg.mozilla.org/mozilla-central/rev/6d3576194df8
https://hg.mozilla.org/mozilla-central/rev/ab5501b1b7d5
https://hg.mozilla.org/mozilla-central/rev/e28024927ff6
https://hg.mozilla.org/mozilla-central/rev/11a6d5a59add
https://hg.mozilla.org/mozilla-central/rev/8644fcfe29da
https://hg.mozilla.org/mozilla-central/rev/7c0dce574b3f