Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2358223002:
Add log date to the metrics log (Closed)
|
Created:
4 years, 3 months ago by gayane -on leave until 09-2017 Modified:
4 years, 2 months ago Reviewers:
Alexei Svitkine (slow) CC:
chromium-reviews, asvitkine+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd log date to the metrics log
BUG=649440
Committed: https://crrev.com/8b523b876294fd17bb4abed0408486562c4dab27
Cr-Commit-Position: refs/heads/master@{#422229}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Adding log date in persisted logs #
Total comments: 21
Patch Set 3 : No explicit migration #
Total comments: 3
Patch Set 4 : fix unittests #
Total comments: 4
Patch Set 5 : init with certain timestamp #
Total comments: 2
Patch Set 6 : add deprectaed comments #
Messages
Total messages: 41 (26 generated)
The CQ bit was checked by gayane@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by gayane@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Description was changed from ========== Add log date to the metrics log BUG= ========== to ========== Add log date to the metrics log BUG=649440 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
gayane@chromium.org changed reviewers: + asvitkine@chromium.org
Please have a look.
https://codereview.chromium.org/2358223002/diff/40001/components/metrics/prot... File components/metrics/proto/system_profile.proto (right): https://codereview.chromium.org/2358223002/diff/40001/components/metrics/prot... components/metrics/proto/system_profile.proto:865: optional int64 log_date = 24; So I wasn't thinking we include it in the actual log proto - but in the local state storage of old logs that's managed by components/metrics/persisted_logs.cc. (The reason for that is that I think we never actually de-serialize the old logs when loading them and sending them - we just read the data into list_[i].compressed_log_data.) So... to do it that way, probably the solution is to store this in prefs. Looks like currently it stores it as a list and uses a strange convention of alternating between storing hash and the data. We can either change this structure to have 3 entries or switch to a new pref and use a dictionary as the array element type. The latter seems more elegant to me, but we'd need the old code for compatibility as well.
On 2016/09/22 20:51:30, Alexei Svitkine (very slow) wrote: > https://codereview.chromium.org/2358223002/diff/40001/components/metrics/prot... > File components/metrics/proto/system_profile.proto (right): > > https://codereview.chromium.org/2358223002/diff/40001/components/metrics/prot... > components/metrics/proto/system_profile.proto:865: optional int64 log_date = 24; > So I wasn't thinking we include it in the actual log proto - but in the local > state storage of old logs that's managed by > components/metrics/persisted_logs.cc. > > (The reason for that is that I think we never actually de-serialize the old logs > when loading them and sending them - we just read the data into > list_[i].compressed_log_data.) > > So... to do it that way, probably the solution is to store this in prefs. Looks > like currently it stores it as a list and uses a strange convention of > alternating between storing hash and the data. We can either change this > structure to have 3 entries or switch to a new pref and use a dictionary as the > array element type. The latter seems more elegant to me, but we'd need the old > code for compatibility as well. Just to clarify. Here is my take on this. - we will keep a list of dictionaries, where each dict corresponds to one log and contains 3 key-values (hash, date, data). - we will need a new pref name for string list of dictionaries - when we serialize the logs we write it down in the new format - when we read persisted logs back we can read back from old pref in old format and from new pref in new format. But we could also have a migration code that will read and rewrite from old format to new format with 'some' date. What do you think ?
Your summary matches my mental model. :) It might be easiest to do the migration part - because that means you just remove the old codepath for writing the data and data is always written back in the new format. Maybe for the date, we can just omit that key for entries that were from the old format? On Mon, Sep 26, 2016 at 4:20 PM, <gayane@chromium.org> wrote: > On 2016/09/22 20:51:30, Alexei Svitkine (very slow) wrote: > > > https://codereview.chromium.org/2358223002/diff/40001/ > components/metrics/proto/system_profile.proto > > File components/metrics/proto/system_profile.proto (right): > > > > > https://codereview.chromium.org/2358223002/diff/40001/ > components/metrics/proto/system_profile.proto#newcode865 > > components/metrics/proto/system_profile.proto:865: optional int64 > log_date = > 24; > > So I wasn't thinking we include it in the actual log proto - but in the > local > > state storage of old logs that's managed by > > components/metrics/persisted_logs.cc. > > > > (The reason for that is that I think we never actually de-serialize the > old > logs > > when loading them and sending them - we just read the data into > > list_[i].compressed_log_data.) > > > > So... to do it that way, probably the solution is to store this in prefs. > Looks > > like currently it stores it as a list and uses a strange convention of > > alternating between storing hash and the data. We can either change this > > structure to have 3 entries or switch to a new pref and use a dictionary > as > the > > array element type. The latter seems more elegant to me, but we'd need > the old > > code for compatibility as well. > > Just to clarify. Here is my take on this. > - we will keep a list of dictionaries, where each dict corresponds to one > log > and contains 3 key-values (hash, date, data). > - we will need a new pref name for string list of dictionaries > - when we serialize the logs we write it down in the new format > - when we read persisted logs back we can read back from old pref in old > format > and from new pref in new format. But we could also have a migration code > that > will read and rewrite from old format to new format with 'some' date. > What do you think ? > > https://codereview.chromium.org/2358223002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Please have a look
https://codereview.chromium.org/2358223002/diff/60001/components/metrics/metr... File components/metrics/metrics_log_manager.cc (right): https://codereview.chromium.org/2358223002/diff/60001/components/metrics/metr... components/metrics/metrics_log_manager.cc:53: initial_log_queue_.MigrateFromOldFormat(prefs::kDeprecatedMetricsInitialLogs); I'm not sure we need an explicit migrate function. Just have the write function always write in new format and things will migrate themselves whenever it's updated. https://codereview.chromium.org/2358223002/diff/60001/components/metrics/metr... File components/metrics/metrics_pref_names.cc (right): https://codereview.chromium.org/2358223002/diff/60001/components/metrics/metr... components/metrics/metrics_pref_names.cc:41: // info, etc. Update comment here and below. https://codereview.chromium.org/2358223002/diff/60001/components/metrics/metr... components/metrics/metrics_pref_names.cc:43: "user_experience_metrics.initial_logs_dict_list"; I would just name it "initial_logs2" and "ongoing_logs2". Keep it simple and short, since the name is part of the file content and so don't need the extra bloat. https://codereview.chromium.org/2358223002/diff/60001/components/metrics/pers... File components/metrics/persisted_logs.cc (right): https://codereview.chromium.org/2358223002/diff/60001/components/metrics/pers... components/metrics/persisted_logs.cc:20: namespace { Nit: add line after https://codereview.chromium.org/2358223002/diff/60001/components/metrics/pers... components/metrics/persisted_logs.cc:70: timestamp = (base::Time::Now() - base::Time::UnixEpoch()).InSeconds(); Isn't return value int64_t but your field is an int? https://codereview.chromium.org/2358223002/diff/60001/components/metrics/pers... components/metrics/persisted_logs.cc:124: WriteLogsToPrefList(update.Get()); I don't see where WriteLogsToPrefList is implemented? https://codereview.chromium.org/2358223002/diff/60001/components/metrics/pers... components/metrics/persisted_logs.cc:125: } I'm not sure we need an explicit migrate function. Just have the write function always write in new format and things will migrate themselves whenever it's updated. https://codereview.chromium.org/2358223002/diff/60001/components/metrics/pers... components/metrics/persisted_logs.cc:159: void PersistedLogs::WriteLogsToPrefList_OldFormat( Do we need this anymore? https://codereview.chromium.org/2358223002/diff/60001/components/metrics/pers... components/metrics/persisted_logs.cc:198: PersistedLogs::LogReadStatus PersistedLogs::ReadLogsFromPrefList_OldFormat( Nit: ReadLogsFromOldFormatPrefList https://codereview.chromium.org/2358223002/diff/60001/components/metrics/pers... File components/metrics/persisted_logs.h (right): https://codereview.chromium.org/2358223002/diff/60001/components/metrics/pers... components/metrics/persisted_logs.h:143: int timestamp; Add a ctor so that this value is initialized.
https://codereview.chromium.org/2358223002/diff/60001/components/metrics/metr... File components/metrics/metrics_pref_names.cc (right): https://codereview.chromium.org/2358223002/diff/60001/components/metrics/metr... components/metrics/metrics_pref_names.cc:41: // info, etc. On 2016/09/28 18:50:30, Alexei Svitkine (slow) wrote: > Update comment here and below. Done. https://codereview.chromium.org/2358223002/diff/60001/components/metrics/metr... components/metrics/metrics_pref_names.cc:43: "user_experience_metrics.initial_logs_dict_list"; On 2016/09/28 18:50:30, Alexei Svitkine (slow) wrote: > I would just name it "initial_logs2" and "ongoing_logs2". Keep it simple and > short, since the name is part of the file content and so don't need the extra > bloat. Done. https://codereview.chromium.org/2358223002/diff/60001/components/metrics/pers... File components/metrics/persisted_logs.cc (right): https://codereview.chromium.org/2358223002/diff/60001/components/metrics/pers... components/metrics/persisted_logs.cc:20: namespace { On 2016/09/28 18:50:30, Alexei Svitkine (slow) wrote: > Nit: add line after Done. https://codereview.chromium.org/2358223002/diff/60001/components/metrics/pers... components/metrics/persisted_logs.cc:70: timestamp = (base::Time::Now() - base::Time::UnixEpoch()).InSeconds(); On 2016/09/28 18:50:30, Alexei Svitkine (slow) wrote: > Isn't return value int64_t but your field is an int? hmm right. I had a look and dictionaries have only SetInteger and SetDouble and no long. i calculated that 100 years in seconds will fit in 32bits https://codereview.chromium.org/2358223002/diff/60001/components/metrics/pers... components/metrics/persisted_logs.cc:124: WriteLogsToPrefList(update.Get()); On 2016/09/28 18:50:30, Alexei Svitkine (slow) wrote: > I don't see where WriteLogsToPrefList is implemented? opps deleted that instead of WriteLogsToPrefList_OldFormat https://codereview.chromium.org/2358223002/diff/60001/components/metrics/pers... components/metrics/persisted_logs.cc:125: } On 2016/09/28 18:50:30, Alexei Svitkine (slow) wrote: > I'm not sure we need an explicit migrate function. Just have the write function > always write in new format and things will migrate themselves whenever it's > updated. But then when we read we need to read from two different prefs in two different ways. is it preferable ? https://codereview.chromium.org/2358223002/diff/60001/components/metrics/pers... components/metrics/persisted_logs.cc:159: void PersistedLogs::WriteLogsToPrefList_OldFormat( On 2016/09/28 18:50:30, Alexei Svitkine (slow) wrote: > Do we need this anymore? Nope https://codereview.chromium.org/2358223002/diff/60001/components/metrics/pers... components/metrics/persisted_logs.cc:198: PersistedLogs::LogReadStatus PersistedLogs::ReadLogsFromPrefList_OldFormat( On 2016/09/28 18:50:30, Alexei Svitkine (slow) wrote: > Nit: ReadLogsFromOldFormatPrefList Done. https://codereview.chromium.org/2358223002/diff/60001/components/metrics/pers... File components/metrics/persisted_logs.h (right): https://codereview.chromium.org/2358223002/diff/60001/components/metrics/pers... components/metrics/persisted_logs.h:143: int timestamp; On 2016/09/28 18:50:30, Alexei Svitkine (slow) wrote: > Add a ctor so that this value is initialized. Done.
https://codereview.chromium.org/2358223002/diff/60001/components/metrics/pers... File components/metrics/persisted_logs.cc (right): https://codereview.chromium.org/2358223002/diff/60001/components/metrics/pers... components/metrics/persisted_logs.cc:70: timestamp = (base::Time::Now() - base::Time::UnixEpoch()).InSeconds(); On 2016/09/28 20:50:49, gayane wrote: > On 2016/09/28 18:50:30, Alexei Svitkine (slow) wrote: > > Isn't return value int64_t but your field is an int? > > hmm right. I had a look and dictionaries have only SetInteger and SetDouble and > no long. > i calculated that 100 years in seconds will fit in 32bits That's a 100 years since unix epoch? So 56 years at this point? I looked at how we store other timestamps and looks like we just use strings and store the TimeT of the value. e.g. local_state_->SetInt64(prefs::kInstallDate, client_info_backup->installation_date != 0 ? client_info_backup->installation_date : now.ToTimeT()); Which just ends up being: SetUserPrefValue(path, new base::StringValue(base::Int64ToString(value))); So let's just register this as a string pref and use base::Int64ToString(). https://codereview.chromium.org/2358223002/diff/60001/components/metrics/pers... components/metrics/persisted_logs.cc:125: } On 2016/09/28 20:50:49, gayane wrote: > On 2016/09/28 18:50:30, Alexei Svitkine (slow) wrote: > > I'm not sure we need an explicit migrate function. Just have the write > function > > always write in new format and things will migrate themselves whenever it's > > updated. > > But then when we read we need to read from two different prefs in two different > ways. is it preferable ? Well, you're already reading from both prefs - just one of them is done in the migrate function... so I don't really see the difference. I guess we need a tiny bit of extra logic (if statement) about which pref to read from, but that should be minimal. We won't ever have data in both at the same time - since I think the full data is written at once when writing (and so when writing in the new way, we should clear the old pref with a todo). So then the read function just needs to have an "if has old data, return readold();" which can have a todo to be removed when we migrate.
The CQ bit was checked by gayane@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #3 (id:80001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by gayane@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #3 (id:100001) has been deleted
Thanks Alexei for the help. Have one more look https://codereview.chromium.org/2358223002/diff/120001/components/metrics/per... File components/metrics/persisted_logs.cc (right): https://codereview.chromium.org/2358223002/diff/120001/components/metrics/per... components/metrics/persisted_logs.cc:75: (base::Time::Now() - base::Time::UnixEpoch()).InSeconds()); don't use ToTimeT() because it seems to be deprecated https://codereview.chromium.org/2358223002/diff/120001/components/metrics/per... components/metrics/persisted_logs.cc:202: new base::DictionaryValue); this changed because the overloaded version of list_value->Append got deprecated https://codereview.chromium.org/2358223002/diff/120001/components/metrics/per... File components/metrics/persisted_logs.h (right): https://codereview.chromium.org/2358223002/diff/120001/components/metrics/per... components/metrics/persisted_logs.h:134: struct LogInfo { no constructor here because we turned the timestamp to be a string
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
https://codereview.chromium.org/2358223002/diff/140001/components/metrics/per... File components/metrics/persisted_logs.cc (right): https://codereview.chromium.org/2358223002/diff/140001/components/metrics/per... components/metrics/persisted_logs.cc:75: (base::Time::Now() - base::Time::UnixEpoch()).InSeconds()); I don't think we should do it here - because that means if with Init() it with log data it will now set the time to today. Instead, I'd move this to be outside the caller and be a param of init. So cases where we read an old log that doesn't have a timestamp won't get reset to "today". I'd also still use TimeT. It's been deprecated forever but is not really actively being migrated. And we'll need to update all the rest of our code when we actually want to move away from it - so I think keeping it consistent now is the best path here - and using TimeT. https://codereview.chromium.org/2358223002/diff/140001/components/metrics/per... components/metrics/persisted_logs.cc:111: if (local_state_->HasPrefPath(outdated_pref_name_)) Nit: {}'s
Thanks. Please have one more look https://codereview.chromium.org/2358223002/diff/140001/components/metrics/per... File components/metrics/persisted_logs.cc (right): https://codereview.chromium.org/2358223002/diff/140001/components/metrics/per... components/metrics/persisted_logs.cc:75: (base::Time::Now() - base::Time::UnixEpoch()).InSeconds()); On 2016/09/30 19:08:28, Alexei Svitkine (slow) wrote: > I don't think we should do it here - because that means if with Init() it with > log data it will now set the time to today. Instead, I'd move this to be outside > the caller and be a param of init. > > So cases where we read an old log that doesn't have a timestamp won't get reset > to "today". > > I'd also still use TimeT. It's been deprecated forever but is not really > actively being migrated. And we'll need to update all the rest of our code when > we actually want to move away from it - so I think keeping it consistent now is > the best path here - and using TimeT. Done. https://codereview.chromium.org/2358223002/diff/140001/components/metrics/per... components/metrics/persisted_logs.cc:111: if (local_state_->HasPrefPath(outdated_pref_name_)) On 2016/09/30 19:08:28, Alexei Svitkine (slow) wrote: > Nit: {}'s Done.
LGTM, thanks! https://codereview.chromium.org/2358223002/diff/160001/components/metrics/met... File components/metrics/metrics_pref_names.cc (right): https://codereview.chromium.org/2358223002/diff/160001/components/metrics/met... components/metrics/metrics_pref_names.cc:12: // info, etc. Nit: add "Deprecated by kMetricsInitialLogs." https://codereview.chromium.org/2358223002/diff/160001/components/metrics/met... components/metrics/metrics_pref_names.cc:19: // user activities. Nit: add "Deprecated by kMetricsOngoingLogs."
The CQ bit was checked by gayane@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2358223002/#ps180001 (title: "add deprectaed comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add log date to the metrics log BUG=649440 ========== to ========== Add log date to the metrics log BUG=649440 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Add log date to the metrics log BUG=649440 ========== to ========== Add log date to the metrics log BUG=649440 Committed: https://crrev.com/8b523b876294fd17bb4abed0408486562c4dab27 Cr-Commit-Position: refs/heads/master@{#422229} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/8b523b876294fd17bb4abed0408486562c4dab27 Cr-Commit-Position: refs/heads/master@{#422229} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
