Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
846663003:
Add UMA metrics to recovery component. (Closed)
|
Created:
5 years, 11 months ago by robertshield Modified:
5 years, 11 months ago CC:
chromium-reviews, asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd UMA metrics to recovery component.
BUG=447695
TEST=NONE
Committed: https://crrev.com/ecb81a7631aa81c10d6968f6c9896ebd229716d2
Cr-Commit-Position: refs/heads/master@{#311501}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Expand to include result codes. #
Total comments: 3
Patch Set 3 : Fix incorrect constants. #
Total comments: 9
Patch Set 4 : Dear Gab #
Total comments: 2
Patch Set 5 : Move UMA_ macros to helper function. #
Messages
Total messages: 21 (4 generated)
robertshield@chromium.org changed reviewers: + gab@chromium.org, xiaolingbao@chromium.org
Comments below, Cheers! Gab https://codereview.chromium.org/846663003/diff/1/chrome/browser/component_upd... File chrome/browser/component_updater/recovery_component_installer.cc (right): https://codereview.chromium.org/846663003/diff/1/chrome/browser/component_upd... chrome/browser/component_updater/recovery_component_installer.cc:63: enum RecoveryComponentEvent { If you make this an enum class (woot C++11!!) then you can explicitly use RecoveryComponentEvent::RUN_NON_ELEVATED, etc. everywhere rather than polluting this namespace and have to use RCE_ prefixes. I prefer this, more explicit and less namespace pollution. https://codereview.chromium.org/846663003/diff/1/chrome/browser/component_upd... chrome/browser/component_updater/recovery_component_installer.cc:68: RCE_MAX = 4 Don't explicitly label the max. (I also prefer to call it _COUNT or _END than _MAX -- as _MAX to me sounds like it's the value of the last item, not a past-the-end value). https://codereview.chromium.org/846663003/diff/1/chrome/browser/component_upd... chrome/browser/component_updater/recovery_component_installer.cc:121: RCE_MAX); This is slightly misleading, this event doesn't actually mean that it was *ran* elevated, only that the UAC was brought up, right? Perhaps we should log an attempt here and then another event when succeeding (i.e. we get a handle from base::LaunchElevatedProcess()?), this would also help quantify how bad the UAC really is?
Thanks, PTAL https://codereview.chromium.org/846663003/diff/1/chrome/browser/component_upd... File chrome/browser/component_updater/recovery_component_installer.cc (right): https://codereview.chromium.org/846663003/diff/1/chrome/browser/component_upd... chrome/browser/component_updater/recovery_component_installer.cc:63: enum RecoveryComponentEvent { On 2015/01/09 20:08:44, gab wrote: > If you make this an enum class (woot C++11!!) then you can explicitly use > RecoveryComponentEvent::RUN_NON_ELEVATED, etc. everywhere rather than polluting > this namespace and have to use RCE_ prefixes. > > I prefer this, more explicit and less namespace pollution. Sadly without the implicit conversion to int, that would require explicit casts to play nicely with the UMA_HISTOGRAM macros afaict. Will leave as is unless you know of a nice fix for this. https://codereview.chromium.org/846663003/diff/1/chrome/browser/component_upd... chrome/browser/component_updater/recovery_component_installer.cc:68: RCE_MAX = 4 On 2015/01/09 20:08:44, gab wrote: > Don't explicitly label the max. > > (I also prefer to call it _COUNT or _END than _MAX -- as _MAX to me sounds like > it's the value of the last item, not a past-the-end value). Done. https://codereview.chromium.org/846663003/diff/1/chrome/browser/component_upd... chrome/browser/component_updater/recovery_component_installer.cc:121: RCE_MAX); On 2015/01/09 20:08:44, gab wrote: > This is slightly misleading, this event doesn't actually mean that it was *ran* > elevated, only that the UAC was brought up, right? > > Perhaps we should log an attempt here and then another event when succeeding > (i.e. we get a handle from base::LaunchElevatedProcess()?), this would also help > quantify how bad the UAC really is? Right, the intent of this patch initially was to record what Chrome thought it was trying to do, not whether the recovery component actually worked. On further reflection, why not do both. Expanded CL accordingly.
Quick comments as I head off for the weekend. https://codereview.chromium.org/846663003/diff/1/chrome/browser/component_upd... File chrome/browser/component_updater/recovery_component_installer.cc (right): https://codereview.chromium.org/846663003/diff/1/chrome/browser/component_upd... chrome/browser/component_updater/recovery_component_installer.cc:63: enum RecoveryComponentEvent { On 2015/01/09 21:37:21, robertshield wrote: > On 2015/01/09 20:08:44, gab wrote: > > If you make this an enum class (woot C++11!!) then you can explicitly use > > RecoveryComponentEvent::RUN_NON_ELEVATED, etc. everywhere rather than > polluting > > this namespace and have to use RCE_ prefixes. > > > > I prefer this, more explicit and less namespace pollution. > > Sadly without the implicit conversion to int, that would require explicit casts > to play nicely with the UMA_HISTOGRAM macros afaict. Will leave as is unless you > know of a nice fix for this. Hmmm, really? I was sure I'd done this in the past, but maybe not... https://codereview.chromium.org/846663003/diff/20001/chrome/browser/component... File chrome/browser/component_updater/recovery_component_installer.cc (right): https://codereview.chromium.org/846663003/diff/20001/chrome/browser/component... chrome/browser/component_updater/recovery_component_installer.cc:260: UMA_HISTOGRAM_ENUMERATION("RecoveryComponent.Event", RCE_ELEVATION_NEEDED, Wrong event here and below?
Thanks https://codereview.chromium.org/846663003/diff/1/chrome/browser/component_upd... File chrome/browser/component_updater/recovery_component_installer.cc (right): https://codereview.chromium.org/846663003/diff/1/chrome/browser/component_upd... chrome/browser/component_updater/recovery_component_installer.cc:63: enum RecoveryComponentEvent { On 2015/01/09 22:41:22, gab wrote: > On 2015/01/09 21:37:21, robertshield wrote: > > On 2015/01/09 20:08:44, gab wrote: > > > If you make this an enum class (woot C++11!!) then you can explicitly use > > > RecoveryComponentEvent::RUN_NON_ELEVATED, etc. everywhere rather than > > polluting > > > this namespace and have to use RCE_ prefixes. > > > > > > I prefer this, more explicit and less namespace pollution. > > > > Sadly without the implicit conversion to int, that would require explicit > casts > > to play nicely with the UMA_HISTOGRAM macros afaict. Will leave as is unless > you > > know of a nice fix for this. > > Hmmm, really? I was sure I'd done this in the past, but maybe not... I tried it and it complained about requiring a cast to convert between RecoveryComponentEvent and the type required. Could do this, but that is uglier than the current solution. Also, http://stackoverflow.com/questions/8357240/how-to-automatically-convert-stron... https://codereview.chromium.org/846663003/diff/20001/chrome/browser/component... File chrome/browser/component_updater/recovery_component_installer.cc (right): https://codereview.chromium.org/846663003/diff/20001/chrome/browser/component... chrome/browser/component_updater/recovery_component_installer.cc:260: UMA_HISTOGRAM_ENUMERATION("RecoveryComponent.Event", RCE_ELEVATION_NEEDED, On 2015/01/09 22:41:22, gab wrote: > Wrong event here and below? Done.
Thanks Robert. Just a minor question about whether it is intended. https://codereview.chromium.org/846663003/diff/20001/chrome/browser/component... File chrome/browser/component_updater/recovery_component_installer.cc (right): https://codereview.chromium.org/846663003/diff/20001/chrome/browser/component... chrome/browser/component_updater/recovery_component_installer.cc:101: if (!process.WaitForExitWithTimeout(kMaxWaitTime, &installer_exit_code)) { Does this block handle both of the following cases: 1) User cancels the UAC prompt, and 2) This process fails to wait due to insufficient privilege to open the target process? They seem to lead to different install results, should we group them in the same category?
https://codereview.chromium.org/846663003/diff/1/chrome/browser/component_upd... File chrome/browser/component_updater/recovery_component_installer.cc (right): https://codereview.chromium.org/846663003/diff/1/chrome/browser/component_upd... chrome/browser/component_updater/recovery_component_installer.cc:63: enum RecoveryComponentEvent { On 2015/01/09 22:45:32, robertshield wrote: > On 2015/01/09 22:41:22, gab wrote: > > On 2015/01/09 21:37:21, robertshield wrote: > > > On 2015/01/09 20:08:44, gab wrote: > > > > If you make this an enum class (woot C++11!!) then you can explicitly use > > > > RecoveryComponentEvent::RUN_NON_ELEVATED, etc. everywhere rather than > > > polluting > > > > this namespace and have to use RCE_ prefixes. > > > > > > > > I prefer this, more explicit and less namespace pollution. > > > > > > Sadly without the implicit conversion to int, that would require explicit > > casts > > > to play nicely with the UMA_HISTOGRAM macros afaict. Will leave as is unless > > you > > > know of a nice fix for this. > > > > Hmmm, really? I was sure I'd done this in the past, but maybe not... > > I tried it and it complained about requiring a cast to convert between > RecoveryComponentEvent and the type required. Could do this, but that is uglier > than the current solution. > Also, > http://stackoverflow.com/questions/8357240/how-to-automatically-convert-stron... Ah ok, right, for some reason I thought I'd done this, but now that I think further what I have is a switch based on a enum class and then the histogram themselves don't report the enumeration value itself but a specific histogram (w/, e.g., a count) for each value. https://codereview.chromium.org/846663003/diff/40001/chrome/browser/component... File chrome/browser/component_updater/recovery_component_installer.cc (right): https://codereview.chromium.org/846663003/diff/40001/chrome/browser/component... chrome/browser/component_updater/recovery_component_installer.cc:110: RCE_ELEVATED_SKIPPED, RCE_COUNT); So if the exit code is not success it means "skip"? https://codereview.chromium.org/846663003/diff/40001/chrome/browser/component... chrome/browser/component_updater/recovery_component_installer.cc:150: base::WorkerPool::PostTask( Cool, didn't know of WorkerPool..! Are you sure that histograms are 100% safe to call at any point in the browser's lifetime (I think so but not 100% sure). https://codereview.chromium.org/846663003/diff/40001/chrome/browser/component... chrome/browser/component_updater/recovery_component_installer.cc:248: if (!process.WaitForExitWithTimeout(kMaxWaitTime, &installer_exit_code)) { Hmmm, the ! case should be the failure case in which you don't have an exit code, no? In general, I prefer having the basic case as the basic condition and the else{} handling the ! (here and in other changes in this file). Handling the ! first often makes for weird logic. https://codereview.chromium.org/846663003/diff/40001/chrome/browser/component... chrome/browser/component_updater/recovery_component_installer.cc:281: return false; Do we care about this failure?
PTAL https://codereview.chromium.org/846663003/diff/1/chrome/browser/component_upd... File chrome/browser/component_updater/recovery_component_installer.cc (right): https://codereview.chromium.org/846663003/diff/1/chrome/browser/component_upd... chrome/browser/component_updater/recovery_component_installer.cc:63: enum RecoveryComponentEvent { On 2015/01/12 19:12:20, gab wrote: > On 2015/01/09 22:45:32, robertshield wrote: > > On 2015/01/09 22:41:22, gab wrote: > > > On 2015/01/09 21:37:21, robertshield wrote: > > > > On 2015/01/09 20:08:44, gab wrote: > > > > > If you make this an enum class (woot C++11!!) then you can explicitly > use > > > > > RecoveryComponentEvent::RUN_NON_ELEVATED, etc. everywhere rather than > > > > polluting > > > > > this namespace and have to use RCE_ prefixes. > > > > > > > > > > I prefer this, more explicit and less namespace pollution. > > > > > > > > Sadly without the implicit conversion to int, that would require explicit > > > casts > > > > to play nicely with the UMA_HISTOGRAM macros afaict. Will leave as is > unless > > > you > > > > know of a nice fix for this. > > > > > > Hmmm, really? I was sure I'd done this in the past, but maybe not... > > > > I tried it and it complained about requiring a cast to convert between > > RecoveryComponentEvent and the type required. Could do this, but that is > uglier > > than the current solution. > > Also, > > > http://stackoverflow.com/questions/8357240/how-to-automatically-convert-stron... > > Ah ok, right, for some reason I thought I'd done this, but now that I think > further what I have is a switch based on a enum class and then the histogram > themselves don't report the enumeration value itself but a specific histogram > (w/, e.g., a count) for each value. Acknowledged. https://codereview.chromium.org/846663003/diff/40001/chrome/browser/component... File chrome/browser/component_updater/recovery_component_installer.cc (right): https://codereview.chromium.org/846663003/diff/40001/chrome/browser/component... chrome/browser/component_updater/recovery_component_installer.cc:110: RCE_ELEVATED_SKIPPED, RCE_COUNT); On 2015/01/12 19:12:20, gab wrote: > So if the exit code is not success it means "skip"? That's right. https://codereview.chromium.org/846663003/diff/40001/chrome/browser/component... chrome/browser/component_updater/recovery_component_installer.cc:150: base::WorkerPool::PostTask( On 2015/01/12 19:12:20, gab wrote: > Cool, didn't know of WorkerPool..! Are you sure that histograms are 100% safe to > call at any point in the browser's lifetime (I think so but not 100% sure). As sure as I am of anything. Alexei will do an OWNERS review on this, and I will ask him then. https://codereview.chromium.org/846663003/diff/40001/chrome/browser/component... chrome/browser/component_updater/recovery_component_installer.cc:248: if (!process.WaitForExitWithTimeout(kMaxWaitTime, &installer_exit_code)) { On 2015/01/12 19:12:20, gab wrote: > Hmmm, the ! case should be the failure case in which you don't have an exit > code, no? > > In general, I prefer having the basic case as the basic condition and the else{} > handling the ! (here and in other changes in this file). > > Handling the ! first often makes for weird logic. Hrmm, actually, the ! there looks like a typo. Thanks for catching, removing. https://codereview.chromium.org/846663003/diff/40001/chrome/browser/component... chrome/browser/component_updater/recovery_component_installer.cc:281: return false; On 2015/01/12 19:12:20, gab wrote: > Do we care about this failure? The existing code does, what motivates you to want to change the logic here?
robertshield@chromium.org changed reviewers: + asvitkine@chromium.org, sorin@chromium.org
Adding Sorin and Alexei for OWNERS. sorin@chromium.org: Please review the recovery_component_installer changes asvitkine@chromium.org: Please review UMA changes. There is also a question from Gab as to whether UMA macros are safe to use from threads in the WorkerPool. I'm working on the assumption that they are, please let me know if not.
Histograms are safe to use from any thread, so should be fine. Have a comment about the code. https://codereview.chromium.org/846663003/diff/60001/chrome/browser/component... File chrome/browser/component_updater/recovery_component_installer.cc (right): https://codereview.chromium.org/846663003/diff/60001/chrome/browser/component... chrome/browser/component_updater/recovery_component_installer.cc:103: UMA_HISTOGRAM_ENUMERATION("RecoveryComponent.Event", Nit: Please make a helper function for this histogram, so as not to duplicate the macro which expands to a lot of code.
lgtm w/ optional comment below https://codereview.chromium.org/846663003/diff/40001/chrome/browser/component... File chrome/browser/component_updater/recovery_component_installer.cc (right): https://codereview.chromium.org/846663003/diff/40001/chrome/browser/component... chrome/browser/component_updater/recovery_component_installer.cc:281: return false; On 2015/01/12 23:04:19, robertshield wrote: > On 2015/01/12 19:12:20, gab wrote: > > Do we care about this failure? > > The existing code does, what motivates you to want to change the logic here? Yes, I was asking whether this should also result in a histogram event?
lgtm thank you!
Thanks all! Alexei, PTAL https://codereview.chromium.org/846663003/diff/60001/chrome/browser/component... File chrome/browser/component_updater/recovery_component_installer.cc (right): https://codereview.chromium.org/846663003/diff/60001/chrome/browser/component... chrome/browser/component_updater/recovery_component_installer.cc:103: UMA_HISTOGRAM_ENUMERATION("RecoveryComponent.Event", On 2015/01/12 23:18:08, Alexei Svitkine wrote: > Nit: Please make a helper function for this histogram, so as not to duplicate > the macro which expands to a lot of code. Thanks, done.
Patchset #5 (id:80001) has been deleted
lgtm
The CQ bit was checked by robertshield@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/846663003/100001
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/ecb81a7631aa81c10d6968f6c9896ebd229716d2 Cr-Commit-Position: refs/heads/master@{#311501} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
