1246816 - Get ProfileCreationDate from system if times.json could not be read

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

In bug 1246209, we try to read times.json, which should be created by the GeckoProfile init code to get the profile creation date. However, in the case of error, we should try to implement the algo in ProfileAge.jsm - getOldestProfileTimestamp [1] which walks the file system to find the oldest file age and uses that instead. Then we should persist the data to times.json.

It's not strictly necessary as times.json should be written eventually by the Gecko side of things if it fails to write on profile creation, but it's more accurate than the solution we have now.

[1]: https://mxr.mozilla.org/mozilla-central/source/toolkit/modules/ProfileAge.jsm#122

Comment 1

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

Remove the "optional" argument work-around in TelemetryPingGenerator.createCorePingPayload too.

Comment 2

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

And remove the work-around in TelemetryUploadService.getProfileCreationDate.

Comment 3

[Richard Newman [:rnewman]]

Oh hey, I wrote that code!

http://hg.mozilla.org/mozilla-central/annotate/94b360f64761/services/healthreport/profile.jsm

But maybe don't over-think this…

http://developer.android.com/reference/android/content/pm/PackageInfo.html#firstInstallTime

Comment 4

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

(In reply to Richard Newman [:rnewman] from comment #3)

> But maybe don't over-think this…
> 
> http://developer.android.com/reference/android/content/pm/PackageInfo.
> html#firstInstallTime

Awesome simplification

Comment 5

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

Georg, there is still the (unlikely) possibility of throwing errors and being unable to read the profile creation date. In this case, should we return -1 or null? I'd think -1 to keep the types consistent.

Comment 6

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

Georg, our simplified algorithm uses the Android system's package install time for our application, rather than walking the filesystem like JS. I persist this value to times.json so we always return a consistent times.json value.

Could using this time, as opposed to the actual profile creation time, break any existing systems you are aware of? Should I not persist to disk and let Gecko eventually write to times.json instead?

Comment 7

[Georg Fritzsche [:gfritzsche]]

On desktop we used |null| to signal failure/missing/etc. for fields, i think it's good to be able to tell this apart.

I think null instead of -1 is a clearer signal, both would affect the semantics (negative vs. positive number or null vs. positive number).

Comment 8

[Georg Fritzsche [:gfritzsche]]

(In reply to Michael Comella (:mcomella) from comment #6)
> Georg, our simplified algorithm uses the Android system's package install
> time for our application, rather than walking the filesystem like JS. I
> persist this value to times.json so we always return a consistent times.json
> value.
> 
> Could using this time, as opposed to the actual profile creation time, break
> any existing systems you are aware of? Should I not persist to disk and let
> Gecko eventually write to times.json instead?

I don't think this should break anything, but i'm not sure if this can affect semantics.
Could we have different profile creation vs. installation times?
And would we care?

Comment 9

[Richard Newman [:rnewman]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #8)

> Could we have different profile creation vs. installation times?

Yes: the profile is created on first launch, which will always be later (seconds, hours, months…) than installation time.

> And would we care?

It would make a user appear to have been retained for longer.

Comment 10

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

Finkle, in comment 9, Newman brought up some good points about the ways using "install time" could affect retention & usage numbers – do we care?

We could also simplify this by writing to times.json with System.currentTimeMillis if we suspect that this isn't a really old profile (e.g. use the timestamp from 2016-03-25), otherwise fall-back to installTime in the worst case.

Comment 11

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

(In reply to Michael Comella (:mcomella) from comment #10)
> Finkle, in comment 9, Newman brought up some good points about the ways
> using "install time" could affect retention & usage numbers – do we care?

Yeah. I think we should care. We have enough noise in the system, adding another potential source (installing but not using) should be avoided, if possible.

That said, this bug is about situations where we have an existing profile, not a new profile, without a times.json file. This should not be a problem for new installs, just old installs.

Therefore:
I could live using the "install time" as a fallback for "profile creation" since it's better than NULL and should give a ballpark. This problem will work itself out in the long run.
 
> We could also simplify this by writing to times.json with
> System.currentTimeMillis if we suspect that this isn't a really old profile
> (e.g. use the timestamp from 2016-03-25), otherwise fall-back to installTime
> in the worst case.

Let's not do that.

Comment 12

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

Attachment: MozReview Request: Bug 1246816 - Add ContextUtils.getPackageInstallTime. r=sebastian

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

Comment 13

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

Attachment: MozReview Request: Bug 1246816 - Get core ping profile creation date from application install time. r=sebastian

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

Comment 14

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

Attachment: MozReview Request: Bug 1246816 - Add unit tests for profile creation date. r=sebastian

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

Comment 15

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

Attachment: MozReview Request: roll

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

Comment 16

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

Attachment: MozReview Request: Bug 1246816 - Add docs for profileDate. r=gfritzsche

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

Comment 17

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

Comment on attachment 8736977 [details]
MozReview Request: Bug 1246816 - Get core ping profile creation date from application install time. r=sebastian

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

Comment 18

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

Comment on attachment 8736978 [details]
MozReview Request: Bug 1246816 - Add unit tests for profile creation date. r=sebastian

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

Comment 19

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

Comment on attachment 8736980 [details]
MozReview Request: Bug 1246816 - Add docs for profileDate. r=gfritzsche

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

Comment 20

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

Georg, I should increment the ping version number for each change to the ping format, right? So I should increment again here?

Comment 21

[Georg Fritzsche [:gfritzsche]]

This is not strictly a new addition, but it is a format change for validation/schema purposes.
Let's bump the version so we can actually tell those pings apart.

Comment 22

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8736980 [details]
MozReview Request: Bug 1246816 - Add docs for profileDate. r=gfritzsche

https://reviewboard.mozilla.org/r/43619/#review40395

Let's bump the version and add it to the version history, so we can clearly tell this apart from previous pings.
Let's also merge this with the doc changes from bug 1257595 to preserve the history.

Comment 23

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8736976 [details]
MozReview Request: Bug 1246816 - Add ContextUtils.getPackageInstallTime. r=sebastian

https://reviewboard.mozilla.org/r/43611/#review40623

::: mobile/android/base/java/org/mozilla/gecko/util/ContextUtils.java:21
(Diff revision 1)
> +     * @return {@link android.content.pm.PackageInfo#firstInstallTime} for the context's package.
> +     * @throws PackageManager.NameNotFoundException Unexpected - we get the package name from the context so
> +     *         it's expected to be found.
> +     */
> +    public static long getPackageInstallTime(final Context context) throws PackageManager.NameNotFoundException {
> +        return context.getPackageManager().getPackageInfo(context.getPackageName(), 0).firstInstallTime;

Should we handle NameNotFoundException inside this method and return -1 from here?

My assumption is that this should never fail for the current package. I throw an AssertionError in this case here:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/dlc/StudyAction.java#73-75

Comment 24

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8736977 [details]
MozReview Request: Bug 1246816 - Get core ping profile creation date from application install time. r=sebastian

https://reviewboard.mozilla.org/r/43613/#review40627

Comment 25

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8736978 [details]
MozReview Request: Bug 1246816 - Add unit tests for profile creation date. r=sebastian

https://reviewboard.mozilla.org/r/43615/#review40629

Comment 26

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

https://reviewboard.mozilla.org/r/43611/#review40623

> Should we handle NameNotFoundException inside this method and return -1 from here?
> 
> My assumption is that this should never fail for the current package. I throw an AssertionError in this case here:
> https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/dlc/StudyAction.java#73-75

I like this – I decided to throw `AssertionError` instead (because yes, it is unexpected).

Comment 27

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

Attachment: MozReview Request: Bug 1246816 - Get core ping profile creation date from application install time. r=sebastian

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

Comment 28

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

Comment on attachment 8736976 [details]
MozReview Request: Bug 1246816 - Add ContextUtils.getPackageInstallTime. r=sebastian

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

Comment 29

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

Comment on attachment 8736977 [details]
MozReview Request: Bug 1246816 - Get core ping profile creation date from application install time. r=sebastian

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

Comment 30

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

Comment on attachment 8736978 [details]
MozReview Request: Bug 1246816 - Add unit tests for profile creation date. r=sebastian

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

Comment 31

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

Comment on attachment 8736980 [details]
MozReview Request: Bug 1246816 - Add docs for profileDate. r=gfritzsche

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

Comment 32

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

Comment on attachment 8736977 [details]
MozReview Request: Bug 1246816 - Get core ping profile creation date from application install time. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43613/diff/3-4/

Comment 33

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

Comment on attachment 8736978 [details]
MozReview Request: Bug 1246816 - Add unit tests for profile creation date. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43615/diff/3-4/

Comment 34

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

Comment on attachment 8736980 [details]
MozReview Request: Bug 1246816 - Add docs for profileDate. r=gfritzsche

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43619/diff/3-4/

Comment 35

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8736980 [details]
MozReview Request: Bug 1246816 - Add docs for profileDate. r=gfritzsche

https://reviewboard.mozilla.org/r/43619/#review41335

This looks good.

Comment 36

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

https://hg.mozilla.org/integration/fx-team/rev/7fc39b90861d0a2d0fcc9dc7ad46b47c6204514f
Bug 1246816 - Add ContextUtils.getPackageInstallTime. r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/6653113a8093393a68ce21899a0ab698e89a8d1c
Bug 1246816 - Get core ping profile creation date from application install time. r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/0bdc00213180d671c58bd10e2e7e79853d481532
Bug 1246816 - Add unit tests for profile creation date. r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/163ec1ec0f25e482a9c5156581887744f1f3fbb7
Bug 1246816 - Add docs for profileDate. r=gfritzsche

Comment 37

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7fc39b90861d
https://hg.mozilla.org/mozilla-central/rev/6653113a8093
https://hg.mozilla.org/mozilla-central/rev/0bdc00213180
https://hg.mozilla.org/mozilla-central/rev/163ec1ec0f25

Comment 38

[Georg Fritzsche [:gfritzsche]]

This looks good on Nightly so far, i'm not seeing any missing profileDate values:
https://gist.github.com/georgf/59af52c55cea0e88d6a2e39895da296b

We should check this more once its on beta/release (probably just together with other validations).

Comment 39

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

Comment on attachment 8736976 [details]
MozReview Request: Bug 1246816 - Add ContextUtils.getPackageInstallTime. r=sebastian

While not strictly necessary, bug 1249288 should get uplifted first. It bumps a version number which will conflict if this patch is uplifted first.

This request applies to all commits in this patch. However, the last commit is a doc update. If it fails to apply, it's okay to leave it out.

Approval Request Comment
[Feature/regressing bug #]: Profile creation date added to core ping (bug 1246209)
[User impact if declined]: We'll have less useful (i.e. many null) values for the profile creation date

[Describe test coverage new/current, TreeHerder]: Tested locally, on Nightly for a week, server-side validated, TH unit tests landed with patch
[Risks and why]: Low. We do some file IO, which is verbose and error prone in Java, however, the unit tests cover this. Otherwise, the patch is pretty simple – just calling out to existing APIs.

[String/UUID change made/needed]: None

Comment 40

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8736976 [details]
MozReview Request: Bug 1246816 - Add ContextUtils.getPackageInstallTime. r=sebastian

Improves telemetry data quality, baked and validated on Nightly, Aurora47+

Comment 41

[Carsten Book [:Tomcat]]

has problems to apply:

grafting 338188:7fc39b90861d "Bug 1246816 - Add ContextUtils.getPackageInstallTime. r=sebastian"
merging mobile/android/base/moz.build
warning: conflicts while merging mobile/android/base/moz.build! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue

Comment 42

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

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/940fa79188fc
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/6eb56cc6d62f
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/f96dd0b45c03

I dropped the docs commit because it doesn't need to be up-to-date on aurora.