Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1356573002:
[Smart Lock, Settings Reconciliation] Histograms for tracking initial pref values. (Closed)
|
Created:
5 years, 3 months ago by melandory Modified:
5 years, 2 months ago CC:
chromium-reviews, gcasto+watchlist_chromium.org, vabr+watchlist_chromium.org, asvitkine+watch_chromium.org, mkwst+watchlist-passwords_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@reconcile Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Smart Lock, Settings Reconciliation] Histograms for tracking initial pref values.
In order to make sure that migration algorithm works correctly we want
to observe initial values for the preferences before the migration.
BUG=517087
Committed: https://crrev.com/c0f41c7ebd8a402b3c17f2d273a9dd9061164d7b
Cr-Commit-Position: refs/heads/master@{#351291}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 16
Patch Set 4 : #
Total comments: 4
Patch Set 5 : #
Total comments: 2
Patch Set 6 : #
Messages
Total messages: 18 (8 generated)
Patchset #2 (id:20001) has been deleted
melandory@chromium.org changed reviewers: + engedy@chromium.org
Hi Balazs, please take a look. Thanks!
LGTM % comments. https://codereview.chromium.org/1356573002/diff/60001/chrome/browser/password... File chrome/browser/password_manager/password_manager_setting_migrator_service.cc (right): https://codereview.chromium.org/1356573002/diff/60001/chrome/browser/password... chrome/browser/password_manager/password_manager_setting_migrator_service.cc:33: const char kSettingsReconciliationInitialValuesMetrics[] = nit: Inline this string literal. It's customary for histograms, and allows the PRESUBMIT script to check that there is a corresponding definition in histograms.xml (to prevent typos). https://codereview.chromium.org/1356573002/diff/60001/chrome/browser/password... chrome/browser/password_manager/password_manager_setting_migrator_service.cc:250: // TODO(melandory) Add histogram which will log initial and final values for nit: I'm not sure I understand this comment correctly. Initial values are already logged. Do you mean a histogram that would capture the combination of 4 values here? https://codereview.chromium.org/1356573002/diff/60001/chrome/browser/password... File chrome/browser/password_manager/password_manager_setting_migrator_service.h (right): https://codereview.chromium.org/1356573002/diff/60001/chrome/browser/password... chrome/browser/password_manager/password_manager_setting_migrator_service.h:26: enum PasswordManagerPreferencesInitialValues { What do you think about not defining this enum here at all, and instead simply recording (N << 1 | L)? At the moment, I cannot really think of any benefits of an explicit enum -- except for readability in the unit test, but I guess we could just define the enum in the unittest.cc in an anonymous namespace. https://codereview.chromium.org/1356573002/diff/60001/chrome/browser/password... File chrome/browser/password_manager/password_manager_setting_migrator_service_unittest.cc (right): https://codereview.chromium.org/1356573002/diff/60001/chrome/browser/password... chrome/browser/password_manager/password_manager_setting_migrator_service_unittest.cc:36: const char kSettingsReconciliationInitialValuesMetrics[] = nit: How about |kInitialValuesHistogramName|? I think it is already clear that this belongs to settings reconciliation, as it is in the corresponding unittest. https://codereview.chromium.org/1356573002/diff/60001/chrome/browser/password... chrome/browser/password_manager/password_manager_setting_migrator_service_unittest.cc:198: EXPECT_THAT( Wait a second, is this the right place to verify this? My impressions is that these tests verify propagation, not reconciliation. Shouldn't this be in ReconcileWhenSyncIsNotExpectedPasswordManager* tests? https://codereview.chromium.org/1356573002/diff/60001/chrome/browser/password... chrome/browser/password_manager/password_manager_setting_migrator_service_unittest.cc:370: TestThatMetricsAreNotReportedIfInitializationWasFinished) { nit: s/.../NoInitialValuesAreReportedAfterInitializationHasFinished/ But perhaps this could be checked in TestMigrationOnLocalChanges? https://codereview.chromium.org/1356573002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1356573002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:30252: + Tracks the initial values for both password manager settings. Also explain when / how many samples are recorded. nit: I'd move the summary from the enum to here. I'm not really sure where the summary for the enum is displayed on the histograms dashboard, if at all. https://codereview.chromium.org/1356573002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:66893: + <int value="0" label="N0L0"/> nit: I'd suggest to be a bit more verbose here with the labels, so it will be obvious even to non-technical people what each value means on the dashboard.
Patchset #4 (id:80001) has been deleted
Patchset #4 (id:100001) has been deleted
Patchset #4 (id:120001) has been deleted
melandory@chromium.org changed reviewers: + rkaplow@chromium.org
rkaplow@chromium.org: Please review changes in tools/metrics/histograms/histograms.xml https://codereview.chromium.org/1356573002/diff/60001/chrome/browser/password... File chrome/browser/password_manager/password_manager_setting_migrator_service.cc (right): https://codereview.chromium.org/1356573002/diff/60001/chrome/browser/password... chrome/browser/password_manager/password_manager_setting_migrator_service.cc:33: const char kSettingsReconciliationInitialValuesMetrics[] = On 2015/09/24 14:26:18, engedy wrote: > nit: Inline this string literal. > > It's customary for histograms, and allows the PRESUBMIT script to check that > there is a corresponding definition in histograms.xml (to prevent typos). Done. https://codereview.chromium.org/1356573002/diff/60001/chrome/browser/password... chrome/browser/password_manager/password_manager_setting_migrator_service.cc:250: // TODO(melandory) Add histogram which will log initial and final values for On 2015/09/24 14:26:18, engedy wrote: > nit: I'm not sure I understand this comment correctly. Initial values are > already logged. Do you mean a histogram that would capture the combination of 4 > values here? Yep. https://codereview.chromium.org/1356573002/diff/60001/chrome/browser/password... File chrome/browser/password_manager/password_manager_setting_migrator_service.h (right): https://codereview.chromium.org/1356573002/diff/60001/chrome/browser/password... chrome/browser/password_manager/password_manager_setting_migrator_service.h:26: enum PasswordManagerPreferencesInitialValues { On 2015/09/24 14:26:18, engedy wrote: > What do you think about not defining this enum here at all, and instead simply > recording (N << 1 | L)? At the moment, I cannot really think of any benefits of > an explicit enum -- except for readability in the unit test, but I guess we > could just define the enum in the unittest.cc in an anonymous namespace. Sure, I thought that enum is clearer, but it sounds good to have it just in unit tests. https://codereview.chromium.org/1356573002/diff/60001/chrome/browser/password... File chrome/browser/password_manager/password_manager_setting_migrator_service_unittest.cc (right): https://codereview.chromium.org/1356573002/diff/60001/chrome/browser/password... chrome/browser/password_manager/password_manager_setting_migrator_service_unittest.cc:36: const char kSettingsReconciliationInitialValuesMetrics[] = On 2015/09/24 14:26:18, engedy wrote: > nit: How about |kInitialValuesHistogramName|? I think it is already clear that > this belongs to settings reconciliation, as it is in the corresponding unittest. Done. https://codereview.chromium.org/1356573002/diff/60001/chrome/browser/password... chrome/browser/password_manager/password_manager_setting_migrator_service_unittest.cc:198: EXPECT_THAT( On 2015/09/24 14:26:18, engedy wrote: > Wait a second, is this the right place to verify this? My impressions is that > these tests verify propagation, not reconciliation. > > Shouldn't this be in ReconcileWhenSyncIsNotExpectedPasswordManager* tests? But service is started here also, so metrics will be recorded. As you suggested in TestThatMetricsAreNotReportedIfInitializationWasFinished, I can move declaration of base::HistogramTester tester to the line after NotifyProfileAdded() and test that no metrics were recorded. https://codereview.chromium.org/1356573002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1356573002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:30252: + Tracks the initial values for both password manager settings. On 2015/09/24 14:26:18, engedy wrote: > Also explain when / how many samples are recorded. > > nit: I'd move the summary from the enum to here. I'm not really sure where the > summary for the enum is displayed on the histograms dashboard, if at all. Done. https://codereview.chromium.org/1356573002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:66893: + <int value="0" label="N0L0"/> On 2015/09/24 14:26:18, engedy wrote: > nit: I'd suggest to be a bit more verbose here with the labels, so it will be > obvious even to non-technical people what each value means on the dashboard. Done.
Still LGTM % nits. https://codereview.chromium.org/1356573002/diff/60001/chrome/browser/password... File chrome/browser/password_manager/password_manager_setting_migrator_service_unittest.cc (right): https://codereview.chromium.org/1356573002/diff/60001/chrome/browser/password... chrome/browser/password_manager/password_manager_setting_migrator_service_unittest.cc:198: EXPECT_THAT( On 2015/09/28 07:10:12, melandory wrote: > On 2015/09/24 14:26:18, engedy wrote: > > Wait a second, is this the right place to verify this? My impressions is that > > these tests verify propagation, not reconciliation. > > > > Shouldn't this be in ReconcileWhenSyncIsNotExpectedPasswordManager* tests? > > But service is started here also, so metrics will be recorded. As you suggested > in TestThatMetricsAreNotReportedIfInitializationWasFinished, I can move > declaration of base::HistogramTester tester to the line after > NotifyProfileAdded() and test that no metrics were recorded. SGTM. https://codereview.chromium.org/1356573002/diff/140001/chrome/browser/passwor... File chrome/browser/password_manager/password_manager_setting_migrator_service.cc (right): https://codereview.chromium.org/1356573002/diff/140001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrator_service.cc:124: const int max_init_val = 4; nit: kMaxInitialValues In general, avoid abbreviations in variable names unless they are really widely used (e.g., URL). https://codereview.chromium.org/1356573002/diff/140001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1356573002/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:30254: + Smart Lock on Android. Sample is recorded on every profile initialization, nit: mention that this is recorded before running the reconciliation logic. https://codereview.chromium.org/1356573002/diff/160001/chrome/browser/passwor... File chrome/browser/password_manager/password_manager_setting_migrator_service.cc (right): https://codereview.chromium.org/1356573002/diff/160001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrator_service.cc:124: const int max_init_val = 5; Hold on for a second, shouldn't this be 4? The maximum value we are recording is 3.
lgtm
https://codereview.chromium.org/1356573002/diff/140001/chrome/browser/passwor... File chrome/browser/password_manager/password_manager_setting_migrator_service.cc (right): https://codereview.chromium.org/1356573002/diff/140001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrator_service.cc:124: const int max_init_val = 4; On 2015/09/28 10:03:48, engedy wrote: > nit: kMaxInitialValues > > In general, avoid abbreviations in variable names unless they are really widely > used (e.g., URL). Done. https://codereview.chromium.org/1356573002/diff/140001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1356573002/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:30254: + Smart Lock on Android. Sample is recorded on every profile initialization, On 2015/09/28 10:03:48, engedy wrote: > nit: mention that this is recorded before running the reconciliation logic. Done. https://codereview.chromium.org/1356573002/diff/160001/chrome/browser/passwor... File chrome/browser/password_manager/password_manager_setting_migrator_service.cc (right): https://codereview.chromium.org/1356573002/diff/160001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrator_service.cc:124: const int max_init_val = 5; On 2015/09/28 10:03:48, engedy wrote: > Hold on for a second, shouldn't this be 4? The maximum value we are recording is > 3. You are right.
The CQ bit was checked by melandory@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from engedy@chromium.org Link to the patchset: https://codereview.chromium.org/1356573002/#ps180001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1356573002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1356573002/180001
Message was sent while issue was closed.
Committed patchset #6 (id:180001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/c0f41c7ebd8a402b3c17f2d273a9dd9061164d7b Cr-Commit-Position: refs/heads/master@{#351291} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
