Closed Bug 1063710 Opened 10 years ago Closed 10 years ago

Make Reset Profile write the reset's timestamp to times.json

Categories

(Firefox :: General, defect)

defect
Not set
normal
Points:
3

Tracking

()

RESOLVED FIXED
Firefox 36
Iteration:
36.1
Tracking Status
firefox35 --- fixed
firefox36 --- fixed

People

(Reporter: adw, Assigned: markh)

References

Details

Attachments

(1 file, 3 obsolete files)

Broken down from bug 860238.

I'm not sure if we want to record "reset" times or "migration" (from another Firefox profile) times.  The two technically aren't the same since theoretically migration from another Firefox profile could happen without a reset, maybe... but probably not in practice?

If we really need reset times, then it looks like times.json could be updated either by the front end: http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/ResetProfile.jsm

Or the back end, somewhere around here: http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp?rev=5c930eeebcac#3959

If migration times are sufficient, then it should probably happen here: http://mxr.mozilla.org/mozilla-central/source/browser/components/migration/FirefoxProfileMigrator.js

But right now that only defines/overrides getResources(), which probably shouldn't have the side effect of changing a file.  So FirefoxProfileMigrator may need to also override migrate(), or something else.  MigratorPrototype in MigrationUtils.jsm should probably not be changed since it's used as the prototype for all types of migrators (i.e., migration from IE, Safari) and not only from other Firefox profiles.

But also note that it looks like the names of profiles that are created on reset are suffixed with a timestamp, so maybe writing to times.json isn't even necessary: http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/ProfileReset.cpp?rev=5a9badd6db00#34
Flags: qe-verify?
Flags: firefox-backlog+
Points: --- → 3
Flags: qe-verify? → qe-verify-
Assignee: nobody → mhammond
Status: NEW → ASSIGNED
Iteration: --- → 35.2
Drew,
  Looking into this, it seems reasonably difficult to record meaningful "reset times".  When a new profile is created, a new times.json is created with the creation time.  This means that (a) any previous "reset times" have been lost, and (b) if we then write the time the reset happened it will be within a few ms of the actual creation time.

While it would be possible to do this if strictly necessary, my reading of bug 860238 is that simply recording a flag that the reset happened is OK and would meet the actual requirements.  Is it also your understanding that just storing a boolean in times.json is acceptable? (Or maybe to keep the filename correct, recording the same(-ish) timestamp as the creation time?)
Flags: needinfo?(adw)
ie, something like this.  I figured the migration code is a reasonable place to do this recording.
Attachment #8493553 - Flags: feedback?(adw)
(In reply to Mark Hammond [:markh] from comment #1)
> When a new profile is created, a new times.json is created with
> the creation time.  This means that (a) any previous "reset
> times" have been lost, and (b) if we then write the time the
> reset happened it will be within a few ms of the actual
> creation time.

I think the idea is to write the reset time in the old profile's times.json (as outlined in comment 0), and then as part of the files that are migrated, copy that times.json over to the new profile.  This bug does the first part, bug 1063714 does the second.  Maybe there would need to be more logic telling Firefox not to overwrite the copied-over times.json with a new one.

> While it would be possible to do this if strictly necessary, my reading of
> bug 860238 is that simply recording a flag that the reset happened is OK and
> would meet the actual requirements.

So a profile would only be marked as having been created as either part of a reset or not?  That would mean that your reset "history" is lost -- rather, it only goes back one reset in time.  I was thinking that we should store the entire history, although I'm not sure whether that's actually reflected in the requirements in these bugs.

I broke down these FHR bugs starting from the plan in Richard's bug 860238 comment 1, which seems to indicate that FHR should record and report multiple reset times -- all of them I would say because that seems like interesting info.

I'll look at the patch next.
Flags: needinfo?(adw)
Comment on attachment 8493553 [details] [diff] [review]
0001-Bug-1063710-record-a-profile-migration-time-in-ProfD.patch

Review of attachment 8493553 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, this doesn't capture full reset history, right?  I would do something like this bug and bug 1063714 in combination suggest.  If it's easier to modify the new times.json instead of the old one, after copying over the old one (or at least its contents), no problem.

Actually, since we should record history as I've said, it makes sense to me to make times.json a list of timestamped events.  But maybe that would be inefficient when it comes time to use that data, depending on how we end up using it.  Although if it is a list, the whole thing could just be sent in the FHR payload without looking for needles in it.

So you think migration times are sufficient, as opposed to true reset times?
Attachment #8493553 - Flags: feedback?(adw)
Thanks for the feedback.

(In reply to Drew Willcoxon :adw from comment #4)

> Yeah, this doesn't capture full reset history, right?  I would do something
> like this bug and bug 1063714 in combination suggest.  If it's easier to
> modify the new times.json instead of the old one, after copying over the old
> one (or at least its contents), no problem.

This patch now migrates times.json from the old profile to the new (so it is basically stealing a part of bug 1063714).  This way the creation time of the migrated profile is the same as the old profile.

> Actually, since we should record history as I've said, it makes sense to me
> to make times.json a list of timestamped events.  But maybe that would be
> inefficient when it comes time to use that data, depending on how we end up
> using it.  Although if it is a list, the whole thing could just be sent in
> the FHR payload without looking for needles in it.

This new patch records an (unbounded, sorted) array of migration times.  I'm not 100% sure that "(unbounded, sorted)" is actually desired, but I can't imagine it getting too large - maybe we should put an upper limit on the number we maintain?

> So you think migration times are sufficient, as opposed to true reset times?

Yeah, I think that's fine.  My understanding of the code is that a "reset" is really just a convenience wrapper for "make a brand new profile and swap it".  It is actually the migration code that makes the profile usable and thus is the *real* conceptual reset.

I've named the field in times.json |migrations| but given the above, I'd be comfortable with naming it |resets| - what do you think?

Benjamin: can you please confirm this behaviour is correct from your POV?  Specifically:

* A migrated profile now has the creation time of the original profile and not the time the new profile was technically created.
and:
* You (well, FHR) would like a list of migration/reset times, and clarify if you think we should put an upper limit on the number of times we record for a given profile?

FTR, in my testing I see times.json transition thusly:

original:
{"created":1411523590206}

first reset:
{"created":1411523590206,"migrations":[1411523806274]}

2nd reset:
{"created":1411523590206,"migrations":[1411523806274,1411523917307]}

3rd reset:
{"created":1411523590206,"migrations":[1411523806274,1411523917307,1411525192435]}
Attachment #8493553 - Attachment is obsolete: true
Attachment #8494203 - Flags: feedback?(benjamin)
Attachment #8494203 - Flags: feedback?(adw)
Comment on attachment 8494203 [details] [diff] [review]
0001-Bug-1063710-migrate-times.json-to-a-new-profile-and-.patch

Review of attachment 8494203 [details] [diff] [review]:
-----------------------------------------------------------------

This approach looks more like what I had in mind (not that that's right necessarily, but it seems right to me).  I can't imagine the list getting too large either, but IMO it's sound engineering to cap it anyway.  `migrations` is more accurate than `resets` IMO -- although migrations could also mean migrations from other browsers... so firefoxMigrations might be even better, but maybe that's pedantic.

I'd like to recap what I said above about keeping a single list of events.  If we did that, it'd be easy to add new types of events/times in the future.
Attachment #8494203 - Flags: feedback?(adw) → feedback+
(In reply to Drew Willcoxon :adw (Away 9/29–10/14) from comment #6)
> `migrations` is more accurate than `resets` IMO -- although migrations could
> also mean migrations from other browsers... so firefoxMigrations might be
> even better, but maybe that's pedantic.

I think that would be confusing in the opposite direction - we care to track resets, not profile migrations in general. That it's a "migration" under the hood is an unimportant implementation detail.
But Mark's patch doesn't technically track resets, it tracks migrations from other Firefox profiles.  That's what I was getting at in comment 0.
(In reply to Drew Willcoxon :adw (Away 9/29–10/14) from comment #8)
> But Mark's patch doesn't technically track resets, it tracks migrations from
> other Firefox profiles.  That's what I was getting at in comment 0.

There is no useful distinction between "migrations form other Firefox profiles" and "resets". As far as I know there is no way to migrate from another Firefox profile apart from using the reset feature.
Iteration: 35.2 → 35.3
Comment on attachment 8494203 [details] [diff] [review]
0001-Bug-1063710-migrate-times.json-to-a-new-profile-and-.patch

I think with the other bug I don't need to provide feedback on this one, right?
Attachment #8494203 - Flags: feedback?(benjamin)
Given the discussion in bug 1063704, we are not tracking each reset time, just the most recent.  This patch reflects that (and note that it is on-top of the one in bug 1063714.(

gps, I'd like your feedback on the FHR parts here, specifically:

* That I had to increment the Metrics.Measurement version field due to adding a new field.

* That when there is no profile reset time, I simply decline to provide data for the new field.  I'm not sure if this is correct, or whether I should *always* include a value for the new field using (presumably) 0 as the value in that case.

* Any other FHR-specific stuff you notice :)

MattN: feel free to ignore this feedback request - I'm just flagging you as I expect to be asking you for review once gps is happy with the FHR stuff given adw is on leave.
Attachment #8494203 - Attachment is obsolete: true
Attachment #8501536 - Flags: feedback?(gps)
Attachment #8501536 - Flags: feedback?(MattN+bmo)
Comment on attachment 8501536 [details] [diff] [review]
0006-Bug-1063710-Make-Reset-Profile-write-the-reset-s-tim.patch

Review of attachment 8501536 [details] [diff] [review]:
-----------------------------------------------------------------

I didn't follow the times discussion too much. As long as bsmedberg and/or Metrics is happy with the data we're getting (read: have someone f+ the measurement fields if you haven't already), I'll be happy to review the code.

When you create a new measurement version in FHR, you need to keep the old one around so that FHR will still report data from it. Otherwise, the data sits in the database and never makes it to the payload. It's totally fine to make the collection code not work for that version (just don't have the provider populate version <N. Just copy ProfileMetadataMeasurement to ProfileMetadataMeasurement2 and remove uses of the original and you'll be fine.

You'll need to update the in-tree docs about available measurements in services/healthreport/docs/dataformat.rst.

This looks mostly fine. So f+.

::: services/healthreport/tests/xpcshell/test_profile.js
@@ +129,5 @@
>    let again = yield acc.created
>    do_check_eq(expected, again);
>  });
>  
> +add_task(function test_time_reset() {

function*

@@ +249,5 @@
> +  // we've already tested truncate() works as expected, so here just check
> +  // we got values.
> +  do_check_true(created);
> +  do_check_true(reset);
> +  do_check_true(created <= reset);

It's OK to use Assert for new test code.
Attachment #8501536 - Flags: feedback?(gps) → feedback+
Comment on attachment 8501536 [details] [diff] [review]
0006-Bug-1063710-Make-Reset-Profile-write-the-reset-s-tim.patch

(In reply to Gregory Szorc [:gps] from comment #12)
> Comment on attachment 8501536 [details] [diff] [review]
> 0006-Bug-1063710-Make-Reset-Profile-write-the-reset-s-tim.patch
> 
> Review of attachment 8501536 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I didn't follow the times discussion too much. As long as bsmedberg and/or
> Metrics is happy with the data we're getting (read: have someone f+ the
> measurement fields if you haven't already), I'll be happy to review the code.

Thanks - I'll make the changes you requested, but I think it's possible to f+ those fields in the meantime.
Attachment #8501536 - Flags: feedback?(benjamin)
Comment on attachment 8501536 [details] [diff] [review]
0006-Bug-1063710-Make-Reset-Profile-write-the-reset-s-tim.patch

Review of attachment 8501536 [details] [diff] [review]:
-----------------------------------------------------------------

I only looked closely at FirefoxProfileMigrator.js since gps knows profile.jsm better.

::: browser/components/migration/FirefoxProfileMigrator.js
@@ +142,5 @@
> +        file.copyTo(currentProfileDir, "");
> +      }
> +      // And record the fact a migration (ie, a reset) happened.
> +      let timesAccessor = new ProfileTimesAccessor(currentProfileDir.path);
> +      timesAccessor.recordProfileMigration().then(

Um, the method is now called "recordProfileReset" (which is a better name IMO) so I guess this version wasn't tested ;) It would be good to add an automated migrator test for this like you did in bug 1063714.

::: services/healthreport/profile.jsm
@@ +28,5 @@
>  Cu.import("resource://gre/modules/Task.jsm");
>  Cu.import("resource://gre/modules/Log.jsm");
>  Cu.import("resource://services-common/utils.js");
>  
> +// Profile access to times.json (eg, creation/migration time).

Nit: s/migration/reset/ since it being a migrator is an implementation detail and I don't want it to be confused with importing data from other browsers.

@@ +191,5 @@
>      return promise.then(onSuccess, onFailure);
>    },
> +
> +  /**
> +   * Record (and persist) when a profile migration happened.  We just store a

s/migration/reset/
Attachment #8501536 - Flags: feedback?(MattN+bmo) → feedback+
Again, thanks for the quick feedback!

(In reply to Gregory Szorc [:gps] from comment #12)
> Just copy ProfileMetadataMeasurement to
> ProfileMetadataMeasurement2 and remove uses of the original and you'll be
> fine.

Done, thanks.  I'd just like to check that the change:

-  measurementTypes: [ProfileMetadataMeasurement],
+  measurementTypes: [ProfileMetadataMeasurement2],

is correct given the above.

> You'll need to update the in-tree docs about available measurements in
> services/healthreport/docs/dataformat.rst.

Done.

> function*
> It's OK to use Assert for new test code.

In both cases I struggled between doing things the "new" way vs keeping the code consistent, but I've made those changes.

(In reply to Matthew N. [:MattN] (behind on reviews - focused on Loop) from comment #14)

> Um, the method is now called "recordProfileReset" (which is a better name
> IMO) so I guess this version wasn't tested ;) It would be good to add an
> automated migrator test for this like you did in bug 1063714.

heh - yes, good spot.  I'd already noticed that and added the test before you mentioned it (truly ;)

> Nit: s/migration/reset/ since it being a migrator is an implementation
> detail and I don't want it to be confused with importing data from other
> browsers.

> s/migration/reset/

done.

Requesting feedback from bsmedberg for the measurements as requested by gps.
Attachment #8501536 - Attachment is obsolete: true
Attachment #8501536 - Flags: feedback?(benjamin)
Attachment #8502268 - Flags: feedback?(benjamin)
Attachment #8502268 - Flags: feedback?(benjamin) → feedback+
Attachment #8502268 - Flags: review?(gps)
Iteration: 35.3 → 36.1
Comment on attachment 8502268 [details] [diff] [review]
0006-Bug-1063710-Make-Reset-Profile-write-the-reset-s-tim.patch

Review of attachment 8502268 [details] [diff] [review]:
-----------------------------------------------------------------

I missed this before, but I think the storing of a single scalar "last reset day" field is limiting for data analysis. Wouldn't it be nice to know how often profile resets occur, for example? What about seeing if people reset Firefox multiple times because they are not happy with the result? Since we're copying the FHR data across resets, we can keep track of this in the database. All we need to do is write some daily data indicating that a profile reset occurred on that day. This can be either a last daily numeric field using "1" to signify a reset occurred, or it could be a daily counter that is incremented whenever a reset event occurs.

I think there is value in the current approach of recording the last reset day and we should do that regardless (data expires and having a persisted "this profile was reset at some time" helps us answer questions like "what percent of Firefox profiles have ever been reset." But I think recording every reset event is more powerful and answers much more questions to help us build a better Firefox.

If this were a normal patch, I'd say do a follow-up bug. But since this is FHR and we are incurring a version bump on the measurement and we try to limit preventable version bumps, I'm going to ask you to consider making the changes here and now.

Is that reasonable?

::: services/healthreport/profile.jsm
@@ +250,5 @@
> +  fields: {
> +    // Profile creation date. Number of days since Unix epoch.
> +    profileCreation: {type: Metrics.Storage.FIELD_LAST_NUMERIC},
> +    // Profile reset date. Number of days since Unix epoch.
> +    profileReset: {type: Metrics.Storage.FIELD_LAST_NUMERIC},

If you change this to FIELD_DAILY_LAST_NUMERIC or FIELD_DAILY_COUNTER and you ensure you are inserting a value on the proper day, you can magically now record *every* profile reset that ever occurs. This is *much* better than simply recording the last reset.

You can pass a date into setDailyLastNumeric() to set a value for a specific date, not the current date. https://hg.mozilla.org/mozilla-central/file/62f0b771583c/services/metrics/dataprovider.jsm#l299
Attachment #8502268 - Flags: review?(gps)
Comment on attachment 8502268 [details] [diff] [review]
0006-Bug-1063710-Make-Reset-Profile-write-the-reset-s-tim.patch

Review of attachment 8502268 [details] [diff] [review]:
-----------------------------------------------------------------

As discussed on IRC with gps, the has been an explicit decision to go with this somewhat minimalistic scheme and the patch has f+ from bsmedberg, so I'm re-requesting review.
Attachment #8502268 - Flags: review?(gps)
Comment on attachment 8502268 [details] [diff] [review]
0006-Bug-1063710-Make-Reset-Profile-write-the-reset-s-tim.patch

Review of attachment 8502268 [details] [diff] [review]:
-----------------------------------------------------------------

::: services/healthreport/profile.jsm
@@ +250,5 @@
> +  fields: {
> +    // Profile creation date. Number of days since Unix epoch.
> +    profileCreation: {type: Metrics.Storage.FIELD_LAST_NUMERIC},
> +    // Profile reset date. Number of days since Unix epoch.
> +    profileReset: {type: Metrics.Storage.FIELD_LAST_NUMERIC},

If you change this to FIELD_DAILY_LAST_NUMERIC or FIELD_DAILY_COUNTER and you ensure you are inserting a value on the proper day, you can magically now record *every* profile reset that ever occurs. This is *much* better than simply recording the last reset.

You can pass a date into setDailyLastNumeric() to set a value for a specific date, not the current date. https://hg.mozilla.org/mozilla-central/file/62f0b771583c/services/metrics/dataprovider.jsm#l299

@@ +300,2 @@
>  
> +    return Task.spawn(function collectConstants() {

function*
Attachment #8502268 - Flags: review?(gps) → review+
As discussed on IRC, gps said I should ignore the discussion about changing the field type.  I did fix the iterator decl.

https://hg.mozilla.org/integration/fx-team/rev/5b8bb7676793
https://hg.mozilla.org/mozilla-central/rev/5b8bb7676793
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Comment on attachment 8502268 [details] [diff] [review]
0006-Bug-1063710-Make-Reset-Profile-write-the-reset-s-tim.patch

Approval Request Comment
[Feature/regressing bug #]: N/A
[User impact if declined]: This allows the health report to get better statistics about usage and growth.
[Describe test coverage new/current, TBPL]: Reasonable test coverage is part of the patch.
[Risks and why]: Relatively low risk
[String/UUID change made/needed]: None

Note to sheriffs: assuming approval, the patch in bug 1063714 will need to land first.
Attachment #8502268 - Flags: approval-mozilla-aurora?
Attachment #8502268 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: