Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
660563004:
Added UMA statistics for Chrome Cleaner. (Closed)
|
Created:
6 years, 2 months ago by Georges Khalil Modified:
6 years, 1 month ago CC:
chromium-reviews, asvitkine+watch_chromium.org, cpu_(ooo_6.6-7.5) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionAdded UMA statistics for Chrome Cleaner.
BUG=425601
Committed: https://crrev.com/d09466deb3c47ed947469f03395ecd30fca6915e
Cr-Commit-Position: refs/heads/master@{#302124}
Patch Set 1 : Check if we have information from Cleaner and record UMA statistics. #
Total comments: 10
Patch Set 2 : Responded to comments. #Patch Set 3 : Fixed histograms. #
Total comments: 8
Patch Set 4 : Responded to comments. #
Total comments: 4
Patch Set 5 : Responded to comments. #
Messages
Total messages: 40 (17 generated)
georgesak@chromium.org changed reviewers: + mad@chromium.org
Très cool, some comments... Thanks! BYE MAD https://codereview.chromium.org/660563004/diff/1/chrome/browser/component_upd... File chrome/browser/component_updater/sw_reporter_installer_win.cc (right): https://codereview.chromium.org/660563004/diff/1/chrome/browser/component_upd... chrome/browser/component_updater/sw_reporter_installer_win.cc:89: L"Software\\Google\\Software Removal Tool\Cleaner"; I don't think we should duplicate the initial portion of the path, we should reuse kSoftwareRemovalRegistryKey. https://codereview.chromium.org/660563004/diff/1/chrome/browser/component_upd... chrome/browser/component_updater/sw_reporter_installer_win.cc:204: // Check if we have information from Cleaner and send a UMA report. I wouldn't wait until we are ready to launch the reporter before we report the cleaner state from the registry, I would do it way below in RegisterSwReporterComponent, but only after validating that UMA is enabled. https://codereview.chromium.org/660563004/diff/1/chrome/browser/component_upd... chrome/browser/component_updater/sw_reporter_installer_win.cc:214: UMA_HISTOGRAM_SPARSE_SLOWLY("Cleaner.Version", version); This won't store the version string, it will store the pointer value. You should double check the value saved in UMA by navigating to the chrome://histograms page. You need to build a base::Version object, and then work with the components of the version in a similar way that is used for the reporter version below. Note that you can now assume that the cleaner will have only 3 components, that the first one is a byte, the second one a word and the third one another byte, so you can build a DWORD to be uploaded to UMA using a sparse histogram as you are doing above. Now I wonder if this computation should be done by the cleaner instead when it saves the value to the registry... ??? https://codereview.chromium.org/660563004/diff/1/chrome/browser/component_upd... chrome/browser/component_updater/sw_reporter_installer_win.cc:226: UMA_HISTOGRAM_LONG_TIMES("Cleaner.RunTime", Base::TimeDelta::Max()); You're missing the .InMilliseconds() after Max().
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Some comments. Almost there. Thanks! BYE MAD https://codereview.chromium.org/660563004/diff/40001/chrome/browser/component... File chrome/browser/component_updater/sw_reporter_installer_win.cc (right): https://codereview.chromium.org/660563004/diff/40001/chrome/browser/component... chrome/browser/component_updater/sw_reporter_installer_win.cc:86: const wchar_t kCleanerSuffixRegistryKey[] = L"%ls\\Cleaner"; Since the other string is simply at the beginning, you should use simpler concatenation instead. https://codereview.chromium.org/660563004/diff/40001/chrome/browser/component... chrome/browser/component_updater/sw_reporter_installer_win.cc:347: cleaner_key.ReadInt64(kStartTimeRegistryValueName, &start_time_value); Since we only use the start_time when we have a value for end time, we should only read and convert it below. https://codereview.chromium.org/660563004/diff/40001/chrome/browser/component... chrome/browser/component_updater/sw_reporter_installer_win.cc:348: base::Time start_time = base::Time::FromInternalValue(start_time_value); We usually prefer using the constructor instead of the = operator. But since we use the variable only once, we could not have a local variable for this and simply put the FromInternalValue call within the operation to compute run_time. https://codereview.chromium.org/660563004/diff/40001/chrome/browser/component... chrome/browser/component_updater/sw_reporter_installer_win.cc:349: cleaner_key.DeleteValue(kStartTimeRegistryValueName); Then this would need to move lower below of course... https://codereview.chromium.org/660563004/diff/40001/chrome/browser/component... chrome/browser/component_updater/sw_reporter_installer_win.cc:361: base::Time end_time = base::Time::FromInternalValue(end_time_value); Use constructor instead, or use the return value directly in the computation of run_time... which should also be in a constructor.
Responded to comments. https://codereview.chromium.org/660563004/diff/40001/chrome/browser/component... File chrome/browser/component_updater/sw_reporter_installer_win.cc (right): https://codereview.chromium.org/660563004/diff/40001/chrome/browser/component... chrome/browser/component_updater/sw_reporter_installer_win.cc:86: const wchar_t kCleanerSuffixRegistryKey[] = L"%ls\\Cleaner"; On 2014/10/20 01:38:33, MAD wrote: > Since the other string is simply at the beginning, you should use simpler > concatenation instead. Done. https://codereview.chromium.org/660563004/diff/40001/chrome/browser/component... chrome/browser/component_updater/sw_reporter_installer_win.cc:347: cleaner_key.ReadInt64(kStartTimeRegistryValueName, &start_time_value); On 2014/10/20 01:38:33, MAD wrote: > Since we only use the start_time when we have a value for end time, we should > only read and convert it below. Done. https://codereview.chromium.org/660563004/diff/40001/chrome/browser/component... chrome/browser/component_updater/sw_reporter_installer_win.cc:348: base::Time start_time = base::Time::FromInternalValue(start_time_value); On 2014/10/20 01:38:33, MAD wrote: > We usually prefer using the constructor instead of the = operator. But since we > use the variable only once, we could not have a local variable for this and > simply put the FromInternalValue call within the operation to compute run_time. Done. https://codereview.chromium.org/660563004/diff/40001/chrome/browser/component... chrome/browser/component_updater/sw_reporter_installer_win.cc:349: cleaner_key.DeleteValue(kStartTimeRegistryValueName); On 2014/10/20 01:38:33, MAD wrote: > Then this would need to move lower below of course... Done. https://codereview.chromium.org/660563004/diff/40001/chrome/browser/component... chrome/browser/component_updater/sw_reporter_installer_win.cc:361: base::Time end_time = base::Time::FromInternalValue(end_time_value); On 2014/10/20 01:38:33, MAD wrote: > Use constructor instead, or use the return value directly in the computation of > run_time... which should also be in a constructor. Done.
LGTM! Thanks BYE MAD
The CQ bit was checked by georgesak@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/660563004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
asvitkine@chromium.org changed reviewers: + asvitkine@chromium.org
not lgtm Can you document the added histograms in histograms.xml?
ping, what's the status of this?
Patchset #3 (id:80001) has been deleted
Patchset #3 (id:100001) has been deleted
https://codereview.chromium.org/660563004/diff/120001/chrome/browser/componen... File chrome/browser/component_updater/sw_reporter_installer_win.cc (right): https://codereview.chromium.org/660563004/diff/120001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:352: if (cleaner_key.HasValue(kEndTimeRegistryValueName)) { Could it happen that the kEndTime was from a previous run of the software, or does the cleaner tool make sure to clear the end time when it writes the start time? https://codereview.chromium.org/660563004/diff/120001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:365: base::TimeDelta::Max()); Are you sure you want to do this? This will make things like getting average from the dashboard really tough. I would use a separate histogram for it, to be honest - to not conflate things. https://codereview.chromium.org/660563004/diff/120001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:378: Nit: Remove blank line. https://codereview.chromium.org/660563004/diff/120001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/660563004/diff/120001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:32894: + <summary>The runtime of the he software reporter cleaner tool.</summary> Nit: "the he" -> "the" I think "runtime" is ambiguous - as it could mean different things. I would explicitly spell out "running time" or "How long it took to run the software reporter cleaner." Also, please mention in this description that you're logging crashes as the maximum runtime, if you still want to do this.
Patchset #4 (id:140001) has been deleted
PTAL. https://codereview.chromium.org/660563004/diff/120001/chrome/browser/componen... File chrome/browser/component_updater/sw_reporter_installer_win.cc (right): https://codereview.chromium.org/660563004/diff/120001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:352: if (cleaner_key.HasValue(kEndTimeRegistryValueName)) { On 2014/10/29 21:47:51, Alexei Svitkine wrote: > Could it happen that the kEndTime was from a previous run of the software, or > does the cleaner tool make sure to clear the end time when it writes the start > time? The registry is cleared every time the cleaner starts. https://codereview.chromium.org/660563004/diff/120001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:365: base::TimeDelta::Max()); On 2014/10/29 21:47:51, Alexei Svitkine wrote: > Are you sure you want to do this? > > This will make things like getting average from the dashboard really tough. I > would use a separate histogram for it, to be honest - to not conflate things. Added a histogram for whether the cleaner has completed execution or not. https://codereview.chromium.org/660563004/diff/120001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:378: On 2014/10/29 21:47:51, Alexei Svitkine wrote: > Nit: Remove blank line. Done. https://codereview.chromium.org/660563004/diff/120001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/660563004/diff/120001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:32894: + <summary>The runtime of the he software reporter cleaner tool.</summary> On 2014/10/29 21:47:51, Alexei Svitkine wrote: > Nit: "the he" -> "the" > > I think "runtime" is ambiguous - as it could mean different things. I would > explicitly spell out "running time" or "How long it took to run the software > reporter cleaner." > > Also, please mention in this description that you're logging crashes as the > maximum runtime, if you still want to do this. Done.
Couple more comments, then should be good to go. Thanks. https://codereview.chromium.org/660563004/diff/160001/chrome/browser/componen... File chrome/browser/component_updater/sw_reporter_installer_win.cc (right): https://codereview.chromium.org/660563004/diff/160001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:365: UMA_HISTOGRAM_BOOLEAN("SoftwareReporter.Cleaner.HasCompleted", false); Nit: Each histogram macro actually expands to a non-trivial block of code that bloats the binary. So I suggest not repeating the macro for the same histogram. How about the following structure? // If we don't have an end time, we can assume the cleaner crashed. bool completed = cleaner_key.HasValue(kEndTimeRegistryValueName); UMA_HISTOGRAM_BOOLEAN("SoftwareReporter.Cleaner.HasCompleted", completed); if (completed) { ... } https://codereview.chromium.org/660563004/diff/160001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/660563004/diff/160001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:32892: +<histogram name="SoftwareReporter.Cleaner.HasCompleted" enum="BooleanEnabled"> Please add a BooleanCompleted enum entry to the file and reference it, instead of BooleanEnabled.
Patchset #5 (id:180001) has been deleted
Patchset #5 (id:200001) has been deleted
PTAL. https://codereview.chromium.org/660563004/diff/160001/chrome/browser/componen... File chrome/browser/component_updater/sw_reporter_installer_win.cc (right): https://codereview.chromium.org/660563004/diff/160001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:365: UMA_HISTOGRAM_BOOLEAN("SoftwareReporter.Cleaner.HasCompleted", false); On 2014/10/30 14:02:24, Alexei Svitkine wrote: > Nit: Each histogram macro actually expands to a non-trivial block of code that > bloats the binary. So I suggest not repeating the macro for the same histogram. > How about the following structure? > > // If we don't have an end time, we can assume the cleaner crashed. > bool completed = cleaner_key.HasValue(kEndTimeRegistryValueName); > UMA_HISTOGRAM_BOOLEAN("SoftwareReporter.Cleaner.HasCompleted", completed); > if (completed) { > ... > } Done. https://codereview.chromium.org/660563004/diff/160001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/660563004/diff/160001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:32892: +<histogram name="SoftwareReporter.Cleaner.HasCompleted" enum="BooleanEnabled"> On 2014/10/30 14:02:24, Alexei Svitkine wrote: > Please add a BooleanCompleted enum entry to the file and reference it, instead > of BooleanEnabled. Done.
lgtm, thanks!
The CQ bit was checked by georgesak@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/660563004/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
georgesak@chromium.org changed reviewers: + sky@chromium.org
Sky, please review changes to sw_reporter_installer_win.cc for owner approval. Thank you.
sky@chromium.org changed reviewers: + cpu@chromium.org - sky@chromium.org
How about a more local owner, like cpu? sky->cpu
mad@chromium.org changed reviewers: + waffles@chromium.org - cpu@chromium.org
cpu->waffles who did the approvals for this code before.
lgtm
The CQ bit was checked by georgesak@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/660563004/220001
Message was sent while issue was closed.
Committed patchset #5 (id:220001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/d09466deb3c47ed947469f03395ecd30fca6915e Cr-Commit-Position: refs/heads/master@{#302124} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
