Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2871793003:
Added histograms on process singleton create when remote process exists and we cannot notify it (Closed)
|
Created:
3 years, 7 months ago by Alexey Seren Modified:
3 years, 7 months ago CC:
asvitkine+watch_chromium.org, chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdded histograms on process singleton create when remote process exists and we cannot notify it
Currently we have no information about starting of browser process when other
remote processes exist. This CL adds some histograms that will track following:
- how we have interacted with remote process before start we cannot notify them
(i.e. unlocked profile or terminated)
- reason of remote process termination
- error codes for remote process termination
BUG=720277
Review-Url: https://codereview.chromium.org/2871793003
Cr-Commit-Position: refs/heads/master@{#472144}
Committed: https://chromium.googlesource.com/chromium/src/+/028ea15d88db7e9bfb4303698ae52fbf75fc55ca
Patch Set 1 #
Total comments: 16
Patch Set 2 : Some fixes basing on review comments #
Total comments: 4
Patch Set 3 : Split histogram enums by platforms #
Total comments: 6
Patch Set 4 : Updated NotifyOtherProcessNoSuicide test to send right histogram #
Total comments: 8
Patch Set 5 : Reset SkipIsChromeProcessCheck state prior to every test #
Total comments: 1
Patch Set 6 : Made histogram enums COUNT to be grown automatically #
Total comments: 2
Patch Set 7 : Updated histograms owners #
Messages
Total messages: 44 (20 generated)
Description was changed from ========== Added histograms on process singleton create when remote process exists Currently we have no information about starting of browser process when other remote processes exist. This CL adds some histograms that will track following: - how we have interacted with remote process before start (i.e. unlocked profile or terminated) - reason of remote process termination - error codes for remote process termination ========== to ========== Added histograms on process singleton create when remote process exists Currently we have no information about starting of browser process when other remote processes exist. This CL adds some histograms that will track following: - how we have interacted with remote process before start (i.e. unlocked profile or terminated) - reason of remote process termination - error codes for remote process termination ==========
aseren@yandex-team.ru changed reviewers: + holte@chromium.org, jochen@chromium.org
aseren@yandex-team.ru changed reviewers: + gab@chromium.org
https://codereview.chromium.org/2871793003/diff/1/chrome/browser/process_sing... File chrome/browser/process_singleton_posix.cc (right): https://codereview.chromium.org/2871793003/diff/1/chrome/browser/process_sing... chrome/browser/process_singleton_posix.cc:1092: UnlinkPath(lock_path_); We need to unlink lock file if user selects to unlock profile.
The CQ bit was checked by aseren@yandex-team.ru to run a CQ dry run
The CQ bit was unchecked by aseren@yandex-team.ru
can you file a tracking bug please and reference it from the CL description?
Description was changed from ========== Added histograms on process singleton create when remote process exists Currently we have no information about starting of browser process when other remote processes exist. This CL adds some histograms that will track following: - how we have interacted with remote process before start (i.e. unlocked profile or terminated) - reason of remote process termination - error codes for remote process termination ========== to ========== Added histograms on process singleton create when remote process exists Currently we have no information about starting of browser process when other remote processes exist. This CL adds some histograms that will track following: - how we have interacted with remote process before start (i.e. unlocked profile or terminated) - reason of remote process termination - error codes for remote process termination BUG=720277 ==========
On 2017/05/10 08:20:34, jochen wrote: > can you file a tracking bug please and reference it from the CL description? Done. https://bugs.chromium.org/p/chromium/issues/detail?id=720277
Are any of these actionable? What are you trying to diagnose from this data? https://codereview.chromium.org/2871793003/diff/1/chrome/browser/process_sing... File chrome/browser/process_singleton.h (right): https://codereview.chromium.org/2871793003/diff/1/chrome/browser/process_sing... chrome/browser/process_singleton.h:59: MAX_REMOTE_HUNG_PROCESS_TERMINATE_REASON This isn't a MAX, it's a COUNT or maybe END (since ends in C++'s are usually past-the-end). https://codereview.chromium.org/2871793003/diff/1/chrome/browser/process_sing... File chrome/browser/process_singleton_posix.cc (right): https://codereview.chromium.org/2871793003/diff/1/chrome/browser/process_sing... chrome/browser/process_singleton_posix.cc:781: INVALID_LOCK_FILE, MAX_HUNG_REMOTE_PROCESS_ACTION); Each UMA macro expands to a fair amount of code. Extract this to a helper method that takes the enum instead of inlining the macro multiple times for the same enum. https://codereview.chromium.org/2871793003/diff/1/chrome/browser/process_sing... chrome/browser/process_singleton_posix.cc:802: ORPHANED_LOCK_FILE, MAX_HUNG_REMOTE_PROCESS_ACTION); To be consistent I'd say have the UMA logging always on the line just before "return PROCESS_NONE;" https://codereview.chromium.org/2871793003/diff/1/chrome/browser/process_sing... chrome/browser/process_singleton_posix.cc:821: NOTIFY_ATTEMPTS_EXCEEDED, MAX_REMOTE_HUNG_PROCESS_TERMINATE_REASON); Same here, instead of if (kill_unresponsive) {}, move this to right before PROCESS_NONE (below as well) https://codereview.chromium.org/2871793003/diff/1/chrome/browser/process_sing... chrome/browser/process_singleton_posix.cc:1092: UnlinkPath(lock_path_); On 2017/05/10 04:27:07, aseren wrote: > We need to unlink lock file if user selects to unlock profile. Is this a bug fix? If so, should go in a separate CL. https://codereview.chromium.org/2871793003/diff/1/chrome/browser/process_sing... chrome/browser/process_singleton_posix.cc:1142: default: { action = TERMINATE_FAILED; } use same syntax as other cases for default, i.e. action = TERMINATE_FAILED; break; https://codereview.chromium.org/2871793003/diff/1/chrome/browser/process_sing... File chrome/browser/process_singleton_win.cc (right): https://codereview.chromium.org/2871793003/diff/1/chrome/browser/process_sing... chrome/browser/process_singleton_win.cc:222: "Chrome.ProcessSingleton.ProcessTerminateErrorCode.Windows", These histograms will be examined filtered per platform anyways, so I don't think we need different names. Just like say "Chrome.ProcessSingleton.RemoteProcessInteractionResult" doesn't mean the same thing on each platform.
aseren@yandex-team.ru changed reviewers: + manzagop@chromium.org
I think that all of this histograms indicates different issues that can happen with chromium instance so they are quite actionable. If we have a large amount of any histogram, we definitely have a problems in chromium shutdown. This should be investigated and fixed. https://codereview.chromium.org/2871793003/diff/1/chrome/browser/process_sing... File chrome/browser/process_singleton.h (right): https://codereview.chromium.org/2871793003/diff/1/chrome/browser/process_sing... chrome/browser/process_singleton.h:59: MAX_REMOTE_HUNG_PROCESS_TERMINATE_REASON On 2017/05/10 15:16:50, gab wrote: > This isn't a MAX, it's a COUNT or maybe END (since ends in C++'s are usually > past-the-end). Acknowledged. REMOTE_HUNG_PROCESS_TERMINATE_REASON_COUNT will be more appropriate. https://codereview.chromium.org/2871793003/diff/1/chrome/browser/process_sing... File chrome/browser/process_singleton_posix.cc (right): https://codereview.chromium.org/2871793003/diff/1/chrome/browser/process_sing... chrome/browser/process_singleton_posix.cc:781: INVALID_LOCK_FILE, MAX_HUNG_REMOTE_PROCESS_ACTION); On 2017/05/10 15:16:50, gab wrote: > Each UMA macro expands to a fair amount of code. Extract this to a helper method > that takes the enum instead of inlining the macro multiple times for the same > enum. Acknowledged. https://codereview.chromium.org/2871793003/diff/1/chrome/browser/process_sing... chrome/browser/process_singleton_posix.cc:802: ORPHANED_LOCK_FILE, MAX_HUNG_REMOTE_PROCESS_ACTION); On 2017/05/10 15:16:50, gab wrote: > To be consistent I'd say have the UMA logging always on the line just before > "return PROCESS_NONE;" Acknowledged. https://codereview.chromium.org/2871793003/diff/1/chrome/browser/process_sing... chrome/browser/process_singleton_posix.cc:821: NOTIFY_ATTEMPTS_EXCEEDED, MAX_REMOTE_HUNG_PROCESS_TERMINATE_REASON); On 2017/05/10 15:16:50, gab wrote: > Same here, instead of if (kill_unresponsive) {}, move this to right before > PROCESS_NONE (below as well) Acknowledged. https://codereview.chromium.org/2871793003/diff/1/chrome/browser/process_sing... chrome/browser/process_singleton_posix.cc:1092: UnlinkPath(lock_path_); On 2017/05/10 15:16:50, gab wrote: > On 2017/05/10 04:27:07, aseren wrote: > > We need to unlink lock file if user selects to unlock profile. > > Is this a bug fix? If so, should go in a separate CL. Yes it is a bug fix. I will move it to separate CL https://codereview.chromium.org/2871793003/diff/1/chrome/browser/process_sing... chrome/browser/process_singleton_posix.cc:1142: default: { action = TERMINATE_FAILED; } On 2017/05/10 15:16:50, gab wrote: > use same syntax as other cases for default, i.e. > action = TERMINATE_FAILED; > break; Acknowledged. https://codereview.chromium.org/2871793003/diff/1/chrome/browser/process_sing... File chrome/browser/process_singleton_win.cc (right): https://codereview.chromium.org/2871793003/diff/1/chrome/browser/process_sing... chrome/browser/process_singleton_win.cc:222: "Chrome.ProcessSingleton.ProcessTerminateErrorCode.Windows", On 2017/05/10 15:16:50, gab wrote: > These histograms will be examined filtered per platform anyways, so I don't > think we need different names. > > Just like say "Chrome.ProcessSingleton.RemoteProcessInteractionResult" doesn't > mean the same thing on each platform. Unfortunately we cannot do it because each platform specific histogram has it own enum (see histograms.xml): <histogram name="Chrome.ProcessSingleton.TerminateProcessErrorCode.Posix" enum="OSAgnosticErrno"> <owner>aseren@yandex-team.ru</owner> <summary> The error code of remote process termination on Posix in case when remote process hung. </summary> </histogram> <histogram name="Chrome.ProcessSingleton.TerminateProcessErrorCode.Windows" enum="WinGetLastError"> <owner>aseren@yandex-team.ru</owner> <summary> The error code of remote process termination on Windows in case when remote process hung. </summary> </histogram>
On 2017/05/10 15:16:50, gab wrote: > Are any of these actionable? What are you trying to diagnose from this data? > > https://codereview.chromium.org/2871793003/diff/1/chrome/browser/process_sing... > File chrome/browser/process_singleton.h (right): > > https://codereview.chromium.org/2871793003/diff/1/chrome/browser/process_sing... > chrome/browser/process_singleton.h:59: MAX_REMOTE_HUNG_PROCESS_TERMINATE_REASON > This isn't a MAX, it's a COUNT or maybe END (since ends in C++'s are usually > past-the-end). > > https://codereview.chromium.org/2871793003/diff/1/chrome/browser/process_sing... > File chrome/browser/process_singleton_posix.cc (right): > > https://codereview.chromium.org/2871793003/diff/1/chrome/browser/process_sing... > chrome/browser/process_singleton_posix.cc:781: INVALID_LOCK_FILE, > MAX_HUNG_REMOTE_PROCESS_ACTION); > Each UMA macro expands to a fair amount of code. Extract this to a helper method > that takes the enum instead of inlining the macro multiple times for the same > enum. > > https://codereview.chromium.org/2871793003/diff/1/chrome/browser/process_sing... > chrome/browser/process_singleton_posix.cc:802: ORPHANED_LOCK_FILE, > MAX_HUNG_REMOTE_PROCESS_ACTION); > To be consistent I'd say have the UMA logging always on the line just before > "return PROCESS_NONE;" > > https://codereview.chromium.org/2871793003/diff/1/chrome/browser/process_sing... > chrome/browser/process_singleton_posix.cc:821: NOTIFY_ATTEMPTS_EXCEEDED, > MAX_REMOTE_HUNG_PROCESS_TERMINATE_REASON); > Same here, instead of if (kill_unresponsive) {}, move this to right before > PROCESS_NONE (below as well) > > https://codereview.chromium.org/2871793003/diff/1/chrome/browser/process_sing... > chrome/browser/process_singleton_posix.cc:1092: UnlinkPath(lock_path_); > On 2017/05/10 04:27:07, aseren wrote: > > We need to unlink lock file if user selects to unlock profile. > > Is this a bug fix? If so, should go in a separate CL. > > https://codereview.chromium.org/2871793003/diff/1/chrome/browser/process_sing... > chrome/browser/process_singleton_posix.cc:1142: default: { action = > TERMINATE_FAILED; } > use same syntax as other cases for default, i.e. > action = TERMINATE_FAILED; > break; > > https://codereview.chromium.org/2871793003/diff/1/chrome/browser/process_sing... > File chrome/browser/process_singleton_win.cc (right): > > https://codereview.chromium.org/2871793003/diff/1/chrome/browser/process_sing... > chrome/browser/process_singleton_win.cc:222: > "Chrome.ProcessSingleton.ProcessTerminateErrorCode.Windows", > These histograms will be examined filtered per platform anyways, so I don't > think we need different names. > > Just like say "Chrome.ProcessSingleton.RemoteProcessInteractionResult" doesn't > mean the same thing on each platform. Thank you for reviewing this CL and PTAL at the code.
The CQ bit was checked by aseren@yandex-team.ru 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: No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
There are some issues with launching a second browser when one is already running, and they have some implications on our ability to record metrics. I have a doc somewhere that talks about this. I'll try to find it.
Description was changed from ========== Added histograms on process singleton create when remote process exists Currently we have no information about starting of browser process when other remote processes exist. This CL adds some histograms that will track following: - how we have interacted with remote process before start (i.e. unlocked profile or terminated) - reason of remote process termination - error codes for remote process termination BUG=720277 ========== to ========== Added histograms on process singleton create when remote process exists and we cannot notify it Currently we have no information about starting of browser process when other remote processes exist. This CL adds some histograms that will track following: - how we have interacted with remote process before start we cannot notify them (i.e. unlocked profile or terminated) - reason of remote process termination - error codes for remote process termination BUG=720277 ==========
Great, thank you! Please note that all metrics are sent from STARTED browser when we have failed to notify another browser instance. https://codereview.chromium.org/2871793003/diff/20001/chrome/browser/process_... File chrome/browser/process_singleton_win.cc (left): https://codereview.chromium.org/2871793003/diff/20001/chrome/browser/process_... chrome/browser/process_singleton_win.cc:262: // TODO(manzagop): capture a hang report. Seems like this has been done in current CL.
https://codereview.chromium.org/2871793003/diff/1/chrome/browser/process_sing... File chrome/browser/process_singleton_win.cc (right): https://codereview.chromium.org/2871793003/diff/1/chrome/browser/process_sing... chrome/browser/process_singleton_win.cc:222: "Chrome.ProcessSingleton.ProcessTerminateErrorCode.Windows", On 2017/05/11 13:42:05, aseren wrote: > On 2017/05/10 15:16:50, gab wrote: > > These histograms will be examined filtered per platform anyways, so I don't > > think we need different names. > > > > Just like say "Chrome.ProcessSingleton.RemoteProcessInteractionResult" doesn't > > mean the same thing on each platform. > > Unfortunately we cannot do it because each platform specific histogram has it > own enum (see histograms.xml): > > > <histogram name="Chrome.ProcessSingleton.TerminateProcessErrorCode.Posix" > enum="OSAgnosticErrno"> s/OSAgnosticErrno/PosixErrno/ (maybe there's already such a suffix in histograms.xml? I didn't look...) > <https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=owner>aseren@yandex-team.r...> > <summary> > The error code of remote process termination on Posix in case when remote > process hung. > </summary> > </histogram> > > <histogram name="Chrome.ProcessSingleton.TerminateProcessErrorCode.Windows" > enum="WinGetLastError"> > <https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=owner>aseren@yandex-team.r...> > <summary> > The error code of remote process termination on Windows in case when remote > process hung. > </summary> > </histogram> https://codereview.chromium.org/2871793003/diff/20001/chrome/browser/process_... File chrome/browser/process_singleton.h (right): https://codereview.chromium.org/2871793003/diff/20001/chrome/browser/process_... chrome/browser/process_singleton.h:78: }; Split both of these into enum MyEnum { #if defined(OS_WIN) ... #elif defined(OS_POSIX) ... #endif }; since the implementations are mostly orthogonal and don't share many of these?
> Great, thank you! Please note that all metrics are sent from STARTED browser > when we have failed to notify another browser instance. Oh good! Btw, great to see you do this. I can't seem to find that doc, but will send it if I ever do.
https://codereview.chromium.org/2871793003/diff/20001/chrome/browser/process_... File chrome/browser/process_singleton_win.cc (left): https://codereview.chromium.org/2871793003/diff/20001/chrome/browser/process_... chrome/browser/process_singleton_win.cc:262: // TODO(manzagop): capture a hang report. On 2017/05/11 14:48:46, aseren wrote: > Seems like this has been done in current CL. Acknowledged.
PTAL, > s/OSAgnosticErrno/PosixErrno/ (maybe there's already such a suffix in > histograms.xml? I didn't look...) I have looked through histograms.xml and enums.xml file and haven't found such enum. Also I have found existing histogram that is split by platforms: <histogram name="PlatformFile.UnknownErrors.Posix" enum="OSAgnosticErrno"> ... <histogram name="PlatformFile.UnknownErrors.Windows" enum="WinGetLastError"> ... https://codereview.chromium.org/2871793003/diff/20001/chrome/browser/process_... File chrome/browser/process_singleton.h (right): https://codereview.chromium.org/2871793003/diff/20001/chrome/browser/process_... chrome/browser/process_singleton.h:78: }; On 2017/05/11 15:10:03, gab wrote: > Split both of these into > > enum MyEnum { > #if defined(OS_WIN) > ... > #elif defined(OS_POSIX) > ... > #endif > }; > > since the implementations are mostly orthogonal and don't share many of these? Acknowledged.
https://codereview.chromium.org/2871793003/diff/40001/chrome/browser/process_... File chrome/browser/process_singleton.h (right): https://codereview.chromium.org/2871793003/diff/40001/chrome/browser/process_... chrome/browser/process_singleton.h:75: TERMINATE_NOT_ENOUGH_PERMISSIONS = 5, Why are some terminate histograms here instead of under RemoteHungProcessTerminateReason? https://codereview.chromium.org/2871793003/diff/40001/chrome/browser/process_... File chrome/browser/process_singleton_posix.cc (right): https://codereview.chromium.org/2871793003/diff/40001/chrome/browser/process_... chrome/browser/process_singleton_posix.cc:1084: SendRemoteProcessInteractionResultHistogram(PROFILE_UNLOCKED); Feels wrong to share histogram for two different code paths (here and above)? https://codereview.chromium.org/2871793003/diff/40001/chrome/browser/process_... chrome/browser/process_singleton_posix.cc:1090: SendRemoteProcessInteractionResultHistogram(SAME_BROWSER_INSTANCE); ditto
PTAL, Also I have fixed ProcessSingletonPosixTest.NotifyOtherProcessNoSuicide test to do right check. https://codereview.chromium.org/2871793003/diff/40001/chrome/browser/process_... File chrome/browser/process_singleton.h (right): https://codereview.chromium.org/2871793003/diff/40001/chrome/browser/process_... chrome/browser/process_singleton.h:75: TERMINATE_NOT_ENOUGH_PERMISSIONS = 5, On 2017/05/12 18:40:59, gab wrote: > Why are some terminate histograms here instead of under > RemoteHungProcessTerminateReason? "RemoteProcessInteractionResult" histogram tracks what happened if process starts in presence of existing browsers. "RemoteHungProcessTerminateReason" histogram track the reason why we have tried to terminate remote process. TERMINATE_NOT_ENOUGH_PERMISSIONS indicated what happened. It means that we have tried to terminate remote process but have no permissions for it. https://codereview.chromium.org/2871793003/diff/40001/chrome/browser/process_... File chrome/browser/process_singleton_posix.cc (right): https://codereview.chromium.org/2871793003/diff/40001/chrome/browser/process_... chrome/browser/process_singleton_posix.cc:1084: SendRemoteProcessInteractionResultHistogram(PROFILE_UNLOCKED); On 2017/05/12 18:40:59, gab wrote: > Feels wrong to share histogram for two different code paths (here and above)? Acknowledged. https://codereview.chromium.org/2871793003/diff/40001/chrome/browser/process_... chrome/browser/process_singleton_posix.cc:1090: SendRemoteProcessInteractionResultHistogram(SAME_BROWSER_INSTANCE); On 2017/05/12 18:40:59, gab wrote: > ditto Acknowledged. https://codereview.chromium.org/2871793003/diff/60001/chrome/browser/process_... File chrome/browser/process_singleton_posix.cc (right): https://codereview.chromium.org/2871793003/diff/60001/chrome/browser/process_... chrome/browser/process_singleton_posix.cc:342: (g_skip_is_chrome_process_check || Do not check remote process name if g_skip_is_chrome_process_check is true https://codereview.chromium.org/2871793003/diff/60001/chrome/browser/process_... File chrome/browser/process_singleton_posix_unittest.cc (right): https://codereview.chromium.org/2871793003/diff/60001/chrome/browser/process_... chrome/browser/process_singleton_posix_unittest.cc:320: ProcessSingleton::SkipIsChromeProcessCheckForTesting(); Previously ProcessSingletonPosixTest.NotifyOtherProcessNoSuicide test checked the wrong thing. It checked that no suicide if current process is not chrome process. Process IDs weren't compared actually. I have added ProcessSingleton::SkipIsChromeProcessCheckForTesting() method to fix this.
lgtm w/ nits, thanks https://codereview.chromium.org/2871793003/diff/60001/chrome/browser/process_... File chrome/browser/process_singleton.h (right): https://codereview.chromium.org/2871793003/diff/60001/chrome/browser/process_... chrome/browser/process_singleton.h:85: REMOTE_PROCESS_INTERACTION_RESULT_COUNT = 14 Do not give an explicit number to the count, that way it will grow automatically if more items are added (and it will also be the appropriate size per platform) https://codereview.chromium.org/2871793003/diff/60001/chrome/browser/process_... File chrome/browser/process_singleton_posix.cc (right): https://codereview.chromium.org/2871793003/diff/60001/chrome/browser/process_... chrome/browser/process_singleton_posix.cc:115: static bool g_skip_is_chrome_process_check = false; static and namespace{} are redundant, remove static here and above.
Thank you for reviewing this CL. Please find some comment and code fixes below. https://codereview.chromium.org/2871793003/diff/60001/chrome/browser/process_... File chrome/browser/process_singleton.h (right): https://codereview.chromium.org/2871793003/diff/60001/chrome/browser/process_... chrome/browser/process_singleton.h:85: REMOTE_PROCESS_INTERACTION_RESULT_COUNT = 14 On 2017/05/15 16:58:39, gab wrote: > Do not give an explicit number to the count, that way it will grow automatically > if more items are added (and it will also be the appropriate size per platform) I think that COUNT should have one value on each platform because Bucket count for histogram is calculated as COUNT + 1 One value of Bucket count for different platforms can be significant for server that collects and processes this histogram for all platforms. https://codereview.chromium.org/2871793003/diff/60001/chrome/browser/process_... File chrome/browser/process_singleton_posix.cc (right): https://codereview.chromium.org/2871793003/diff/60001/chrome/browser/process_... chrome/browser/process_singleton_posix.cc:115: static bool g_skip_is_chrome_process_check = false; On 2017/05/15 16:58:39, gab wrote: > static and namespace{} are redundant, remove static here and above. Acknowledged. https://codereview.chromium.org/2871793003/diff/80001/chrome/browser/process_... File chrome/browser/process_singleton_posix_unittest.cc (right): https://codereview.chromium.org/2871793003/diff/80001/chrome/browser/process_... chrome/browser/process_singleton_posix_unittest.cc:85: ProcessSingleton::SkipIsChromeProcessCheckForTesting(false); Need to reset SkipIsChromeProcessCheckForTesting before each test run to eliminate influence of one test to another.
The CQ bit was checked by aseren@yandex-team.ru 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...
https://codereview.chromium.org/2871793003/diff/60001/chrome/browser/process_... File chrome/browser/process_singleton.h (right): https://codereview.chromium.org/2871793003/diff/60001/chrome/browser/process_... chrome/browser/process_singleton.h:85: REMOTE_PROCESS_INTERACTION_RESULT_COUNT = 14 On 2017/05/15 19:20:25, aseren wrote: > On 2017/05/15 16:58:39, gab wrote: > > Do not give an explicit number to the count, that way it will grow > automatically > > if more items are added (and it will also be the appropriate size per > platform) > > I think that COUNT should have one value on each platform because Bucket count > for histogram is calculated as COUNT + 1 > One value of Bucket count for different platforms can be significant for server > that collects and processes this histogram for all platforms. AFAIK, UMA doesn't process "count", it's only used client-side to know how many buckets are required when aggregating the stats. If this wasn't the case the server would choke when the count changes later and different versions of chrome report different counts anyways. In that light, I still insist that we remove the count as it reinforces what you mentioned which isn't correct in practice.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
histograms lgtm
Small code changes basing on gab comments https://codereview.chromium.org/2871793003/diff/60001/chrome/browser/process_... File chrome/browser/process_singleton.h (right): https://codereview.chromium.org/2871793003/diff/60001/chrome/browser/process_... chrome/browser/process_singleton.h:85: REMOTE_PROCESS_INTERACTION_RESULT_COUNT = 14 On 2017/05/15 20:13:05, gab wrote: > On 2017/05/15 19:20:25, aseren wrote: > > On 2017/05/15 16:58:39, gab wrote: > > > Do not give an explicit number to the count, that way it will grow > > automatically > > > if more items are added (and it will also be the appropriate size per > > platform) > > > > I think that COUNT should have one value on each platform because Bucket count > > for histogram is calculated as COUNT + 1 > > One value of Bucket count for different platforms can be significant for > server > > that collects and processes this histogram for all platforms. > > AFAIK, UMA doesn't process "count", it's only used client-side to know how many > buckets are required when aggregating the stats. > > If this wasn't the case the server would choke when the count changes later and > different versions of chrome report different counts anyways. > > In that light, I still insist that we remove the count as it reinforces what you > mentioned which isn't correct in practice. Acknowledged. Ok, you are right. Thank you for the explanation! https://codereview.chromium.org/2871793003/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2871793003/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:7068: + <owner>aseren@yandex-team.ru</owner> Should I change owner of new histograms to someone from chromium team? If yes, I think that gab@chromium.org will be most appropriate here.
lgtm
https://codereview.chromium.org/2871793003/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2871793003/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:7068: + <owner>aseren@yandex-team.ru</owner> On 2017/05/16 05:30:38, aseren wrote: > Should I change owner of new histograms to someone from chromium team? If yes, I > think that https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=gab@chromium.org will be most appropriate here. Multiple owners are allowed. You can put both of us.
Done. Thank you.
The CQ bit was checked by aseren@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org, holte@chromium.org, jochen@chromium.org Link to the patchset: https://codereview.chromium.org/2871793003/#ps120001 (title: "Updated histograms owners")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1494948954215010,
"parent_rev": "64b5e1fad912daf7c311c7f984db673bcae7713c", "commit_rev":
"028ea15d88db7e9bfb4303698ae52fbf75fc55ca"}
Message was sent while issue was closed.
Description was changed from ========== Added histograms on process singleton create when remote process exists and we cannot notify it Currently we have no information about starting of browser process when other remote processes exist. This CL adds some histograms that will track following: - how we have interacted with remote process before start we cannot notify them (i.e. unlocked profile or terminated) - reason of remote process termination - error codes for remote process termination BUG=720277 ========== to ========== Added histograms on process singleton create when remote process exists and we cannot notify it Currently we have no information about starting of browser process when other remote processes exist. This CL adds some histograms that will track following: - how we have interacted with remote process before start we cannot notify them (i.e. unlocked profile or terminated) - reason of remote process termination - error codes for remote process termination BUG=720277 Review-Url: https://codereview.chromium.org/2871793003 Cr-Commit-Position: refs/heads/master@{#472144} Committed: https://chromium.googlesource.com/chromium/src/+/028ea15d88db7e9bfb4303698ae5... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/028ea15d88db7e9bfb4303698ae5... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
