Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
356893002:
Add UMA for --app-shim-error. (Closed)
|
Created:
6 years, 6 months ago by jackhou1 Modified:
6 years, 5 months ago CC:
chromium-reviews, chrome-apps-syd-reviews_chromium.org, asvitkine+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionAdd UMA for --app-shim-error.
This will give us some insight into how often this happens.
BUG=353047
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=281992
Patch Set 1 #
Total comments: 14
Patch Set 2 : Address comments #Patch Set 3 : Change to reading the plist. #
Total comments: 8
Patch Set 4 : Address comments #
Total comments: 8
Patch Set 5 : Address comments #
Total comments: 5
Patch Set 6 : Address comments #
Total comments: 4
Patch Set 7 : Address comments #
Messages
Total messages: 25 (0 generated)
What do you think of this? Is there a better way to pass the error details from the shim?
We should also have some timeout on launch -- if a shim was launched but doesn't connect within 1 minute (say), report an error. That can probably be a follow-up, but you might alter how to think about the histogram changes. https://codereview.chromium.org/356893002/diff/1/chrome/app/app_mode_loader_m... File chrome/app/app_mode_loader_mac.mm (right): https://codereview.chromium.org/356893002/diff/1/chrome/app/app_mode_loader_m... chrome/app/app_mode_loader_mac.mm:143: #if defined(ARCH_CPU_32_BITS) I have a hunch this will never be false again on mac.. But I could be wrong https://codereview.chromium.org/356893002/diff/1/chrome/browser/web_applicati... File chrome/browser/web_applications/web_app_mac.mm (right): https://codereview.chromium.org/356893002/diff/1/chrome/browser/web_applicati... chrome/browser/web_applications/web_app_mac.mm:513: void RecordAppShimErrorAndRebuild(const CommandLine& command_line) { maybe rename this to make clear the thread it runs on RecordAppShimErrorAndRebuild -> RecordAppShimErrorAndRebuildOnBlockingPool https://codereview.chromium.org/356893002/diff/1/chrome/browser/web_applicati... chrome/browser/web_applications/web_app_mac.mm:520: error_enum = APP_SHIM_ERROR_BITNESS_BOTH_32; So.... I think we can determine shim bitness from the plist. If we read CFBundleShortVersionString from the shim's plist before upgrading it, and grab Chrome's major version number, that's enough to determine the bitness for everything except canary/dev: beta and stable should never change bitness during the major version. And since we are adding this logic in the shim "now", canary/dev will likely only ever be able to report 64-bit anyway. WDYT? https://codereview.chromium.org/356893002/diff/1/chrome/browser/web_applicati... chrome/browser/web_applications/web_app_mac.mm:984: base::Bind(&RecordAppShimErrorAndRebuild, command_line)); this will do a deep-copy of the command line object into the closure, which isn't cheap -- probably cheaper to just call RecordAppShimError(command_line) on the UI thread before posting the task https://codereview.chromium.org/356893002/diff/1/tools/metrics/histograms/his... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/356893002/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:480: + <owner>rdsmith@chromium.org</owner> probably the wrong owner, and summary below :) https://codereview.chromium.org/356893002/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:34233: + <int value="0" label="BITNESS_SHIM_32_CHROME_64"/> I think it's preferable to have descriptive names -- these are just for the uma UI
https://codereview.chromium.org/356893002/diff/1/chrome/app/app_mode_loader_m... File chrome/app/app_mode_loader_mac.mm (right): https://codereview.chromium.org/356893002/diff/1/chrome/app/app_mode_loader_m... chrome/app/app_mode_loader_mac.mm:143: #if defined(ARCH_CPU_32_BITS) On 2014/06/26 06:47:05, tapted wrote: > I have a hunch this will never be false again on mac.. But I could be wrong I don't think it hurts to differentiate this. If it turns out to be useless we can remove it. https://codereview.chromium.org/356893002/diff/1/chrome/browser/web_applicati... File chrome/browser/web_applications/web_app_mac.mm (right): https://codereview.chromium.org/356893002/diff/1/chrome/browser/web_applicati... chrome/browser/web_applications/web_app_mac.mm:513: void RecordAppShimErrorAndRebuild(const CommandLine& command_line) { On 2014/06/26 06:47:05, tapted wrote: > maybe rename this to make clear the thread it runs on > RecordAppShimErrorAndRebuild -> RecordAppShimErrorAndRebuildOnBlockingPool Moved this to the UI thread. https://codereview.chromium.org/356893002/diff/1/chrome/browser/web_applicati... chrome/browser/web_applications/web_app_mac.mm:520: error_enum = APP_SHIM_ERROR_BITNESS_BOTH_32; On 2014/06/26 06:47:05, tapted wrote: > So.... I think we can determine shim bitness from the plist. > > If we read CFBundleShortVersionString from the shim's plist before upgrading it, > and grab Chrome's major version number, that's enough to determine the bitness > for everything except canary/dev: beta and stable should never change bitness > during the major version. > > And since we are adding this logic in the shim "now", canary/dev will likely > only ever be able to report 64-bit anyway. > > WDYT? I think the main benefit would be not having to update the shim binary again. Is there already a mapping from version to bitness? I wouldn't want to have to maintain that, unless we can just record the versions into the histogram. One benefit of passing a command line flag is that we can pass more error types in the future. Though I'm not sure how likely that is, since it only covers errors where it can start Chrome but can't dyload the framework. Ideally we'd inspect the shim binary from Chrome, but I think the only way to do that is to call "file" in a subprocess. https://codereview.chromium.org/356893002/diff/1/chrome/browser/web_applicati... chrome/browser/web_applications/web_app_mac.mm:984: base::Bind(&RecordAppShimErrorAndRebuild, command_line)); On 2014/06/26 06:47:05, tapted wrote: > this will do a deep-copy of the command line object into the closure, which > isn't cheap -- probably cheaper to just call RecordAppShimError(command_line) on > the UI thread before posting the task Done. https://codereview.chromium.org/356893002/diff/1/tools/metrics/histograms/his... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/356893002/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:480: + <owner>rdsmith@chromium.org</owner> On 2014/06/26 06:47:05, tapted wrote: > probably the wrong owner, and summary below :) Done. https://codereview.chromium.org/356893002/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:34233: + <int value="0" label="BITNESS_SHIM_32_CHROME_64"/> On 2014/06/26 06:47:05, tapted wrote: > I think it's preferable to have descriptive names -- these are just for the uma > UI Done.
don't forget to upload a new patch :) https://codereview.chromium.org/356893002/diff/1/chrome/browser/web_applicati... File chrome/browser/web_applications/web_app_mac.mm (right): https://codereview.chromium.org/356893002/diff/1/chrome/browser/web_applicati... chrome/browser/web_applications/web_app_mac.mm:520: error_enum = APP_SHIM_ERROR_BITNESS_BOTH_32; On 2014/06/27 04:55:57, jackhou1 wrote: > On 2014/06/26 06:47:05, tapted wrote: > > So.... I think we can determine shim bitness from the plist. > > > > If we read CFBundleShortVersionString from the shim's plist before upgrading > it, > > and grab Chrome's major version number, that's enough to determine the bitness > > for everything except canary/dev: beta and stable should never change bitness > > during the major version. > > > > And since we are adding this logic in the shim "now", canary/dev will likely > > only ever be able to report 64-bit anyway. > > > > WDYT? > > I think the main benefit would be not having to update the shim binary again. Is > there already a mapping from version to bitness? I wouldn't want to have to > maintain that, unless we can just record the versions into the histogram. The mapping should be pretty simple: <38: no 38: maybe >38: yes recording those 3 values for the shim version in the histogram is probably enough. > One benefit of passing a command line flag is that we can pass more error types > in the future. Though I'm not sure how likely that is, since it only covers > errors where it can start Chrome but can't dyload the framework. Yeah - let's get the UMA in first. If we see any "bitness matches but dyload failed" in m38 beta then we will need to investigate more. > Ideally we'd inspect the shim binary from Chrome, but I think the only way to do > that is to call "file" in a subprocess. Yeah realised the same, and figured the plist might have this info as well in some form (i.e. bitness), but Chrome version number was the closest it gets. It *might* be as simple as reading the first 2 bytes of app_mode_loader but raises a lot more questions than just reading the plist.
https://codereview.chromium.org/356893002/diff/1/chrome/browser/web_applicati... File chrome/browser/web_applications/web_app_mac.mm (right): https://codereview.chromium.org/356893002/diff/1/chrome/browser/web_applicati... chrome/browser/web_applications/web_app_mac.mm:520: error_enum = APP_SHIM_ERROR_BITNESS_BOTH_32; Okay, changed this to just read the plist. I also forgot previously that the UMA include Chrome's version, so we only need to track the shim version, which is pretty simple.
lgtm with some nite https://codereview.chromium.org/356893002/diff/40001/chrome/browser/web_appli... File chrome/browser/web_applications/web_app_mac.mm (right): https://codereview.chromium.org/356893002/diff/40001/chrome/browser/web_appli... chrome/browser/web_applications/web_app_mac.mm:62: // the app shim. nit: mention that buckets are chosen to track the 32->64 bit change on Mac https://codereview.chromium.org/356893002/diff/40001/chrome/browser/web_appli... chrome/browser/web_applications/web_app_mac.mm:514: componentsSeparatedByString:@"."] objectAtIndex:0] intValue]; since we're rebuilding a possibly corrupt shim.. perhaps this should be robust to that. I think objective C's you-can-call-methods-on-nil will mean this will be pretty safe (the only think I'm not certain about is whether calling componentsSeparatedByString on an empty string (as opposed to nil) returns an empty array or an array with a single empty string [documentation suggests the latter, but if it's an empty array, objectAtIndex[0] will crash]). However, then the error_enum should handle `0` -- so maybe add AP_SHIM_ERROR_UNKNOWN_VERSION; https://codereview.chromium.org/356893002/diff/40001/chrome/browser/web_appli... chrome/browser/web_applications/web_app_mac.mm:515: int error_enum; int -> AppShimError? https://codereview.chromium.org/356893002/diff/40001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/356893002/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:482: + Counts which versions of app_mode_loader are sending --app-shim-error. nit: Histograms folks usually ask for more details in this. Maybe something like ~"--app-shim-error is sent as a command line argument to Chrome when app_mode_loader was unable to dyload the Chrome Framework and call ChromeAppModeMain"
https://codereview.chromium.org/356893002/diff/40001/chrome/browser/web_appli... File chrome/browser/web_applications/web_app_mac.mm (right): https://codereview.chromium.org/356893002/diff/40001/chrome/browser/web_appli... chrome/browser/web_applications/web_app_mac.mm:62: // the app shim. On 2014/07/01 05:53:58, tapted wrote: > nit: mention that buckets are chosen to track the 32->64 bit change on Mac Done. https://codereview.chromium.org/356893002/diff/40001/chrome/browser/web_appli... chrome/browser/web_applications/web_app_mac.mm:514: componentsSeparatedByString:@"."] objectAtIndex:0] intValue]; On 2014/07/01 05:53:59, tapted wrote: > since we're rebuilding a possibly corrupt shim.. perhaps this should be robust > to that. I think objective C's you-can-call-methods-on-nil will mean this will > be pretty safe (the only think I'm not certain about is whether calling > componentsSeparatedByString on an empty string (as opposed to nil) returns an > empty array or an array with a single empty string [documentation suggests the > latter, but if it's an empty array, objectAtIndex[0] will crash]). > > However, then the error_enum should handle `0` -- so maybe add > AP_SHIM_ERROR_UNKNOWN_VERSION; Done. I tried breaking the field in various ways and it seems to handle it fine. https://codereview.chromium.org/356893002/diff/40001/chrome/browser/web_appli... chrome/browser/web_applications/web_app_mac.mm:515: int error_enum; On 2014/07/01 05:53:58, tapted wrote: > int -> AppShimError? Done. https://codereview.chromium.org/356893002/diff/40001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/356893002/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:482: + Counts which versions of app_mode_loader are sending --app-shim-error. On 2014/07/01 05:53:59, tapted wrote: > nit: Histograms folks usually ask for more details in this. Maybe something like > ~"--app-shim-error is sent as a command line argument to Chrome when > app_mode_loader was unable to dyload the Chrome Framework and call > ChromeAppModeMain" Done.
jar, please review for OWNERS in tools/metrics/ rsesek, please review for OWNERS in chrome/common/mac/
https://codereview.chromium.org/356893002/diff/60001/chrome/browser/web_appli... File chrome/browser/web_applications/web_app_mac.mm (right): https://codereview.chromium.org/356893002/diff/60001/chrome/browser/web_appli... chrome/browser/web_applications/web_app_mac.mm:517: if (version < 38) probably needs `version > 0 &&..` here so the unknown case can be caught https://codereview.chromium.org/356893002/diff/60001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/356893002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:34234: + <int value="0" label="Shim was version 37 or lower."/> this needs updating for the UNKNOWN case
https://codereview.chromium.org/356893002/diff/60001/chrome/browser/web_appli... File chrome/browser/web_applications/web_app_mac.mm (right): https://codereview.chromium.org/356893002/diff/60001/chrome/browser/web_appli... chrome/browser/web_applications/web_app_mac.mm:514: int version = [[[[plist valueForKey:app_mode::kCFBundleShortVersionStringKey] May want to use base/version.h. https://codereview.chromium.org/356893002/diff/60001/chrome/browser/web_appli... chrome/browser/web_applications/web_app_mac.mm:525: UMA_HISTOGRAM_ENUMERATION("Apps.AppShimError", Would it make more sense to just histogram the |version| with a linear scale?
https://codereview.chromium.org/356893002/diff/60001/chrome/browser/web_appli... File chrome/browser/web_applications/web_app_mac.mm (right): https://codereview.chromium.org/356893002/diff/60001/chrome/browser/web_appli... chrome/browser/web_applications/web_app_mac.mm:514: int version = [[[[plist valueForKey:app_mode::kCFBundleShortVersionStringKey] On 2014/07/01 14:10:39, rsesek wrote: > May want to use base/version.h. Done. https://codereview.chromium.org/356893002/diff/60001/chrome/browser/web_appli... chrome/browser/web_applications/web_app_mac.mm:517: if (version < 38) On 2014/07/01 12:48:11, tapted wrote: > probably needs `version > 0 &&..` here so the unknown case can be caught Done. https://codereview.chromium.org/356893002/diff/60001/chrome/browser/web_appli... chrome/browser/web_applications/web_app_mac.mm:525: UMA_HISTOGRAM_ENUMERATION("Apps.AppShimError", On 2014/07/01 14:10:39, rsesek wrote: > Would it make more sense to just histogram the |version| with a linear scale? Done. https://codereview.chromium.org/356893002/diff/60001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/356893002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:34234: + <int value="0" label="Shim was version 37 or lower."/> On 2014/07/01 12:48:11, tapted wrote: > this needs updating for the UNKNOWN case Done.
LGTM https://codereview.chromium.org/356893002/diff/80001/chrome/browser/web_appli... File chrome/browser/web_applications/web_app_mac.mm (right): https://codereview.chromium.org/356893002/diff/80001/chrome/browser/web_appli... chrome/browser/web_applications/web_app_mac.mm:65: const int kAppShimErrorVersionMax = 51; FYI Chrome 51 will ship in December 2015/January 2016, assuming we stay on the current six week schedule ;-).
asvitkine, could you please review for OWNERS in tools/metrics/ Not sure if jar does UMA reviews anymore.
I'm away next week, so adding Ilya, who can continue this review while I'm away. Meanwhile, some comments: https://codereview.chromium.org/356893002/diff/80001/chrome/browser/web_appli... File chrome/browser/web_applications/web_app_mac.mm (right): https://codereview.chromium.org/356893002/diff/80001/chrome/browser/web_appli... chrome/browser/web_applications/web_app_mac.mm:516: kAppShimErrorVersionMax); I suggest using UMA_HISTOGRAM_SPARSE_SLOWLY() instead, then you don't need to have kAppShimErrorVersionMax and your histogram wouldn't be using memory for 51 buckets that are mostly empty. https://codereview.chromium.org/356893002/diff/80001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/356893002/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:485: + ChromeAppModeMain. Please clarify this comment to mention what "version" means. It looks like it's the major milestone version - if so, please say so. Also, please update the comment to explain or give an example of when this is expected to be different from Chrome's version number that's logging this error. (If it can't be different, then why bucketize by version?)
https://codereview.chromium.org/356893002/diff/80001/chrome/browser/web_appli... File chrome/browser/web_applications/web_app_mac.mm (right): https://codereview.chromium.org/356893002/diff/80001/chrome/browser/web_appli... chrome/browser/web_applications/web_app_mac.mm:516: kAppShimErrorVersionMax); On 2014/07/04 12:49:24, asvitkine (OOO back July 14th) wrote: > I suggest using UMA_HISTOGRAM_SPARSE_SLOWLY() instead, then you don't need to > have kAppShimErrorVersionMax and your histogram wouldn't be using memory for 51 > buckets that are mostly empty. Done. https://codereview.chromium.org/356893002/diff/80001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/356893002/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:485: + ChromeAppModeMain. On 2014/07/04 12:49:24, asvitkine (OOO back July 14th) wrote: > Please clarify this comment to mention what "version" means. It looks like it's > the major milestone version - if so, please say so. > > Also, please update the comment to explain or give an example of when this is > expected to be different from Chrome's version number that's logging this error. > (If it can't be different, then why bucketize by version?) Done.
Histograms LGTM % nits, thanks. https://codereview.chromium.org/356893002/diff/100001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/356893002/diff/100001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:485: + call ChromeAppModeMain. E.g. when Chrome updates from 32-bit to 64-bit, a nit: "E.g." -> "For example," https://codereview.chromium.org/356893002/diff/100001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:485: + call ChromeAppModeMain. E.g. when Chrome updates from 32-bit to 64-bit, a nit: "a older" -> "an older"
https://codereview.chromium.org/356893002/diff/100001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/356893002/diff/100001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:485: + call ChromeAppModeMain. E.g. when Chrome updates from 32-bit to 64-bit, a On 2014/07/07 23:07:57, Ilya Sherman wrote: > nit: "a older" -> "an older" Done. https://codereview.chromium.org/356893002/diff/100001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:485: + call ChromeAppModeMain. E.g. when Chrome updates from 32-bit to 64-bit, a On 2014/07/07 23:07:57, Ilya Sherman wrote: > nit: "E.g." -> "For example," Done.
The CQ bit was checked by jackhou@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jackhou@chromium.org/356893002/120001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...)
The CQ bit was checked by jackhou@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jackhou@chromium.org/356893002/120001
Message was sent while issue was closed.
Change committed as 281992 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
