Closed Bug 1273689 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 49
Tracking Status
firefox49 --- fixed

People

(Reporter: mcomella, Assigned: mcomella)

References

(Blocks 1 open bug)

Details

Attachments

(6 files)

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.
Margaret, do you know how important this data is? Or if not, who I should talk to to find out?
Flags: needinfo?(margaret.leibovic)
I think this is a good question for Barbara.
Flags: needinfo?(margaret.leibovic) → needinfo?(bbermes)
This is considered _very important data_ for product, so please yes!
Flags: needinfo?(bbermes)
(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.
Flags: needinfo?(bbermes)
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/
Attachment #8758472 - Flags: review?(s.kaspari)
Attachment #8758473 - Flags: review?(s.kaspari)
Attachment #8758474 - Flags: review?(s.kaspari)
Attachment #8758475 - Flags: review?(s.kaspari)
Attachment #8758476 - Flags: review?(s.kaspari)
Attachment #8758477 - Flags: review?(s.kaspari)
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/
The pushed implementation only gets the users' last session info for the very first session (see comment 4 for more).
That's fine as per comment 11.
Flags: needinfo?(bbermes)
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?
Attachment #8758472 - Flags: review?(s.kaspari) → review+
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
Attachment #8758473 - Flags: review?(s.kaspari) → review+
Attachment #8758474 - Flags: review?(s.kaspari) → review+
Comment on attachment 8758474 [details]
MozReview Request: Bug 1273689 - Factor out getSharedPreferences call. r=sebastian

https://reviewboard.mozilla.org/r/56716/#review53612
Attachment #8758475 - Flags: review?(s.kaspari) → review+
Comment on attachment 8758475 [details]
MozReview Request: Bug 1273689 - Move session measurement calls to CorePingDelegate. r=sebastian

https://reviewboard.mozilla.org/r/56718/#review53614
Attachment #8758476 - Flags: review?(s.kaspari) → review+
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 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?
Attachment #8758477 - Flags: review?(s.kaspari) → review+
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.
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.
You need to log in before you can comment on or make changes to this bug.