Closed Bug 1246209 Opened 4 years ago Closed 4 years ago

(telemetry v2.5) Add profile creation date to telemetry pings

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 47
Tracking Status
firefox45 --- fixed
firefox46 --- fixed
firefox47 --- fixed
fennec 45+ ---

People

(Reporter: mcomella, Assigned: mcomella)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

Apparently it's all we need now for retention data and we don't need to implement v3 for that (bug 1243585).

Let's land with bug 1205835, the initial implementation.
No longer blocks: 1205835
Depends on: 1205835
Margaret pointed me at ProfileAge.jsm to find out how to get this data, specifically `get created()` [1] and `getOldestProfileTimestamp()` [2].

Since we don't want to wait on Gecko, we'll have to replicate their techniques.

[1]: https://mxr.mozilla.org/mozilla-central/source/toolkit/modules/ProfileAge.jsm#29
[2]: https://mxr.mozilla.org/mozilla-central/source/toolkit/modules/ProfileAge.jsm#122
Also a reminder that GeckoProfile should be writing the "times.json" file out [1] since Fx24 (bug 874253)

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/GeckoProfile.java#898
Retrieving the profile creation date from the filesystem is not strictly
necessary to upload this data and returns -1 until it is implemented. If the
decision is r+'d here, it will be implemented in bug 1246816.

Review commit: https://reviewboard.mozilla.org/r/34091/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/34091/
Attachment #8717258 - Flags: review?(mark.finkle)
This patch adds 2 workarounds for the fact that getProfileCreationDate
returns -1 when it can't find a creation date. Returning -1 turned
out to be not particularly robust but I did it this way to avoid
adding too many additional versions of methods in order to have
optional parameters such as profileCreationDate. The workarounds
are added as TODOs w/ bug #'s in the code and mentioned in the
comments of bug 1246816 itself.

A future implementation should probably add a Builder to pass a
single Object as the argument to TelemetryPingGenerator.createCorePing
to prevent the argument list from growing unreasonably large and
to properly operate on optional parameters. I didn't do this in
this patch in order to simplify the uplifted code.

Review commit: https://reviewboard.mozilla.org/r/34093/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/34093/
Attachment #8717259 - Flags: review?(mark.finkle)
Comment on attachment 8717257 [details]
MozReview Request: Bug 1246209 - Extract readJSONObjectFromFile out of getClientId. r=mfinkle

https://reviewboard.mozilla.org/r/34089/#review30817
Attachment #8717257 - Flags: review?(mark.finkle) → review+
Comment on attachment 8717259 [details]
MozReview Request: Bug 1246209 - Add profile creation date to core ping. r=mfinkle

https://reviewboard.mozilla.org/r/34093/#review30819

Consider using | floor | here, but we'll finish this up in bug 1246816

::: mobile/android/base/java/org/mozilla/gecko/telemetry/TelemetryPingGenerator.java:65
(Diff revision 1)
> -            final String serverURLSchemeHostPort, final int seq) {
> +            final String serverURLSchemeHostPort, final int seq, final long profileCreationDateDays) {

This is borderline E_TOO_DAMN_LONG. I know what you're trying to, but the JavaDoc comment tells the story well enough. Please consider "profileCreationDate".

::: mobile/android/base/java/org/mozilla/gecko/telemetry/TelemetryPingGenerator.java:92
(Diff revision 1)
> +        // TODO (bug 1246816): Remove this "optional" parameter work-around when

At some point, we'llneed this to be non-optional and send back some form of data. We'll discuss that in bug 1246816. Just a note: I see 1970-01-01 showing up as a profile.createDate in a rare handful of Telemetry environment pings.

::: mobile/android/base/java/org/mozilla/gecko/telemetry/TelemetryUploadService.java:220
(Diff revision 1)
> +        return profileMillis / MILLIS_IN_DAY; // Intentional integer division to truncate value.

FTR, Telemetry uses Math.floor(profileMillis / MILLIS_PER_DAY) in this function:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/TelemetryUtils.jsm#49
Attachment #8717259 - Flags: review?(mark.finkle) → review+
https://reviewboard.mozilla.org/r/34093/#review30819

Used floor.

> This is borderline E_TOO_DAMN_LONG. I know what you're trying to, but the JavaDoc comment tells the story well enough. Please consider "profileCreationDate".

I'm more likely to skimp on reading the javadoc than I am on the variable name so I'm going to leave it (though it's worth noting that I'm unjustified in skimping on reading the javadoc).

> At some point, we'llneed this to be non-optional and send back some form of data. We'll discuss that in bug 1246816. Just a note: I see 1970-01-01 showing up as a profile.createDate in a rare handful of Telemetry environment pings.

> I see 1970-01-01 showing up as a profile.createDate in a rare handful of Telemetry environment pings.

This is referring to desktop telemetry?
(In reply to Michael Comella (:mcomella) from comment #8)

> > I see 1970-01-01 showing up as a profile.createDate in a rare handful of Telemetry environment pings.
> 
> This is referring to desktop telemetry?

Unknown. I am referring to mobile telemetry, but the profile.createDate could have been created by FHR (Java) or Toolkit (JS) code.
Comment on attachment 8717258 [details]
MozReview Request: Bug 1246209 - Add getProfileCreationDate, implement from filestystem, & add stencil code. r=mfinkle

https://reviewboard.mozilla.org/r/34091/#review30881
Attachment #8717258 - Flags: review?(mark.finkle) → review+
Comment on attachment 8717257 [details]
MozReview Request: Bug 1246209 - Extract readJSONObjectFromFile out of getClientId. r=mfinkle

Finkle, can you verify this data is being correctly received on the server?

Approval Request Comment
[Feature/regressing bug #]: Landing of unified telemetry (bug 1205835). This is the last of the data-oriented changesets we hope to land with bug 1205835.
[User impact if declined]: We will have to wait another release to track user retention.

[Describe test coverage new/current, TreeHerder]: Tested locally with gzip server & ensured successful uploads to remote, Finkle to verify data
[Risks and why]: Between low & medium – we have to read the profile creation date from a file we haven't read before and the error handling for file handling is a bit tricky. Note that there is precedent on how to do this, though I added a slight refactor. That being said, we catch the Exceptions that are expected to be thrown by this code and return an invalid value (-1) if there's an error. This return value gets passed through several functions before being added to an existing JSONObject (which is already set to be uploaded to the server). Since we're not dealing with Objects, we don't need to handle null pointers and there are very few (if any) places for typical gotcha problems.

[String/UUID change made/needed]: None
Flags: needinfo?(mark.finkle)
Attachment #8717257 - Flags: approval-mozilla-beta?
Attachment #8717257 - Flags: approval-mozilla-aurora?
I verified that "profileDate" is showing up in the Core ping data. Looks good!
Flags: needinfo?(mark.finkle)
Comment on attachment 8717257 [details]
MozReview Request: Bug 1246209 - Extract readJSONObjectFromFile out of getClientId. r=mfinkle

As part of the telemetry effort, taking it.
Should be in 45 beta 6.
Attachment #8717257 - Flags: approval-mozilla-beta?
Attachment #8717257 - Flags: approval-mozilla-beta+
Attachment #8717257 - Flags: approval-mozilla-aurora?
Attachment #8717257 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.