Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1136763004:
Add UMA data for memory pressure warning received for sync (Closed)
|
Created:
5 years, 7 months ago by Gang Wu Modified:
5 years, 5 months ago CC:
chromium-reviews, tim+watch_chromium.org, zea+watch_chromium.org, maxbogue+watch_chromium.org, pvalenzuela+watch_chromium.org, plaree+watch_chromium.org, asvitkine+watch_chromium.org, maniscalco+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdding UMA for sync service, to see how many MEMORY_PRESSURE_LEVEL_CRITICAL received.
BUG=488290
Committed: https://crrev.com/68208a4cb9def359c3054f0769853f31602968bf
Cr-Commit-Position: refs/heads/master@{#330253}
Patch Set 1 #Patch Set 2 : #
Total comments: 25
Patch Set 3 : #
Total comments: 18
Patch Set 4 : #
Total comments: 1
Patch Set 5 : #
Total comments: 27
Patch Set 6 : #
Total comments: 20
Patch Set 7 : #Patch Set 8 : #
Messages
Total messages: 31 (8 generated)
gangwu@chromium.org changed reviewers: + asvitkine@chromium.org, zea@chromium.org
please take a look.
gangwu@chromium.org changed reviewers: + isherman@chromium.org, jar@chromium.org
gangwu@chromium.org changed reviewers: - isherman@chromium.org, jar@chromium.org
gangwu@chromium.org changed reviewers: + mpearson@chromium.org - asvitkine@chromium.org
Add Mark for UMA review
https://codereview.chromium.org/1136763004/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1136763004/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:41555: +<histogram name="Sync.MemoryPressureWarningBeforeCrash"> units=? or enum=? ditto below https://codereview.chromium.org/1136763004/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:41558: + The MEMORY_PRESSURE_LEVEL_CRITICAL sync service received before crash. All histograms should clearly state when they are emitted. Is this emitted on a later startup after a crash? Or later opening of a syncing profile that was open during a crash? ditto below https://codereview.chromium.org/1136763004/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:41558: + The MEMORY_PRESSURE_LEVEL_CRITICAL sync service received before crash. Are you talking about a full browser crash here or something narrower, like the sync service failing? Also, depending on your answers to the other questions, if these are stability-related histograms, you may want to consider marking them as such in the code (UMA_STABILITY_...). This has certain implications.
zea@chromium.org changed reviewers: + stanisc@chromium.org
Handing this off to Stan for review of the sync logic
updated base on comment https://codereview.chromium.org/1136763004/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1136763004/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:41555: +<histogram name="Sync.MemoryPressureWarningBeforeCrash"> On 2015/05/15 17:46:09, Mark P wrote: > units=? or enum=? > ditto below Done. https://codereview.chromium.org/1136763004/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:41558: + The MEMORY_PRESSURE_LEVEL_CRITICAL sync service received before crash. On 2015/05/15 17:46:09, Mark P wrote: > All histograms should clearly state when they are emitted. Is this emitted on a > later startup after a crash? Or later opening of a syncing profile that was > open during a crash? > ditto below Done.
https://codereview.chromium.org/1136763004/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1136763004/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:41555: +<histogram name="Sync.MemoryPressureWarningBeforeCrash" units="Times"> units="count" I think would be clearer. "Times" has some association with time. https://codereview.chromium.org/1136763004/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:41558: + Counts the number of times sync service received nit:times sync service received -> times the sync service received a (i.e., add "the" and "a") https://codereview.chromium.org/1136763004/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:41559: + MEMORY_PRESSURE_LEVEL_CRITICAL warning before sync service crashed. The sync How does the sync service crash? How is this different than a normal browser crash? If the browser crashes, does that count as a "sync service crash"? https://codereview.chromium.org/1136763004/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:41560: + service will emit this number during next startup. If this number is 0, it'll be emitted, right? Please say so explicitly. https://codereview.chromium.org/1136763004/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:41560: + service will emit this number during next startup. I notice you're storing this number in a preference store. Is this per-user or global? (I'm not familiar with prefs to be able to tell.) i.e., if I start up chrome using a different profile after a crash, do we emit something? Or is this only emitted on the next open of the profile that was part of the sync server crash, not necessarily the next startup? https://codereview.chromium.org/1136763004/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:41564: +<histogram name="Sync.MemoryPressureWarningwithoutCrash" units="Times"> Many of the above comments apply to this histogram description too. https://codereview.chromium.org/1136763004/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:41568: + MEMORY_PRESSURE_LEVEL_CRITICAL warning before sync service shutdown clearly. nit: clearly -> cleanly
updated. https://codereview.chromium.org/1136763004/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1136763004/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:41555: +<histogram name="Sync.MemoryPressureWarningBeforeCrash" units="Times"> On 2015/05/15 19:18:23, Mark P wrote: > units="count" > I think would be clearer. "Times" has some association with time. Done. https://codereview.chromium.org/1136763004/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:41558: + Counts the number of times sync service received On 2015/05/15 19:18:22, Mark P wrote: > nit:times sync service received -> times the sync service received a > (i.e., add "the" and "a") Done. https://codereview.chromium.org/1136763004/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:41559: + MEMORY_PRESSURE_LEVEL_CRITICAL warning before sync service crashed. The sync On 2015/05/15 19:18:23, Mark P wrote: > How does the sync service crash? How is this different than a normal browser > crash? If the browser crashes, does that count as a "sync service crash"? Sync could crash for several reasons. Browser crash will cause sync to crash. and also, sync could crash(or stopped unnormally, like freezing) without browser crashes. that is one of the reason we want to this UMA, we want to know how many crashes from memory pressure. So browser crashes will count into sync service crashes. https://codereview.chromium.org/1136763004/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:41560: + service will emit this number during next startup. On 2015/05/15 19:18:22, Mark P wrote: > I notice you're storing this number in a preference store. Is this per-user or > global? (I'm not familiar with prefs to be able to tell.) i.e., if I start up > chrome using a different profile after a crash, do we emit something? Or is > this only emitted on the next open of the profile that was part of the sync > server crash, not necessarily the next startup? the Preference file, which used here, is per-user. There is a global preference file, but did not use here. So every time user start up, sync will check this user's preference. https://codereview.chromium.org/1136763004/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:41560: + service will emit this number during next startup. On 2015/05/15 19:18:23, Mark P wrote: > If this number is 0, it'll be emitted, right? Please say so explicitly. Yes, even it is 0, the number will be emitted as well. added into comment. https://codereview.chromium.org/1136763004/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:41564: +<histogram name="Sync.MemoryPressureWarningwithoutCrash" units="Times"> On 2015/05/15 19:18:23, Mark P wrote: > Many of the above comments apply to this histogram description too. Done. https://codereview.chromium.org/1136763004/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:41568: + MEMORY_PRESSURE_LEVEL_CRITICAL warning before sync service shutdown clearly. On 2015/05/15 19:18:23, Mark P wrote: > nit: clearly -> cleanly Done.
https://codereview.chromium.org/1136763004/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1136763004/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:41558: + Counts the number of times sync service received On 2015/05/15 20:05:56, Gang Wu wrote: > On 2015/05/15 19:18:22, Mark P wrote: > > nit:times sync service received -> times the sync service received a > > (i.e., add "the" and "a") > > Done. nit: Not done; you forgot to add the "the". sync service -> the sync service https://codereview.chromium.org/1136763004/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:41559: + MEMORY_PRESSURE_LEVEL_CRITICAL warning before sync service crashed. The sync On 2015/05/15 20:05:56, Gang Wu wrote: > On 2015/05/15 19:18:23, Mark P wrote: > > How does the sync service crash? How is this different than a normal browser > > crash? If the browser crashes, does that count as a "sync service crash"? > > Sync could crash for several reasons. > Browser crash will cause sync to crash. and also, sync could crash(or stopped > unnormally, like freezing) without browser crashes. that is one of the reason we > want to this UMA, we want to know how many crashes from memory pressure. > > So browser crashes will count into sync service crashes. Please revise the histogram descriptions to explain this. Explaining to me in the code review is not as useful for future people who may read it this description. https://codereview.chromium.org/1136763004/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:41560: + service will emit this number during next startup. On 2015/05/15 20:05:56, Gang Wu wrote: > On 2015/05/15 19:18:22, Mark P wrote: > > I notice you're storing this number in a preference store. Is this per-user > or > > global? (I'm not familiar with prefs to be able to tell.) i.e., if I start > up > > chrome using a different profile after a crash, do we emit something? Or is > > this only emitted on the next open of the profile that was part of the sync > > server crash, not necessarily the next startup? > > the Preference file, which used here, is per-user. There is a global preference > file, but did not use here. So every time user start up, sync will check this > user's preference. Then when this emission happens depends on what user starts up chrome next. Please revise the description; saying "The sync service will emit this number during next startup" is wrong. Also explain what happens if multiple profiles are open when the sync service crash. Are sync service crashes per profile or global? Would this count be emitted for each profile on the next time it starts or only one profile on the next time it starts? What about if the browser crashes? Is the answer about what gets emitted the same? https://codereview.chromium.org/1136763004/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1136763004/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:41560: + service will emit this number(even is 0) during next startup. nit: bad spacing (here an below) nit: is -> if it is (that's what you meant, right? I don't know what "even is 0" means.)
Looks mostly good but a few names should be changed to improve code readability. https://codereview.chromium.org/1136763004/diff/20001/chrome/browser/sync/pro... File chrome/browser/sync/profile_sync_service.cc (right): https://codereview.chromium.org/1136763004/diff/20001/chrome/browser/sync/pro... chrome/browser/sync/profile_sync_service.cc:920: sync_prefs_.SetCurrentShutdown(false); Please add comment explaining this. https://codereview.chromium.org/1136763004/diff/20001/chrome/browser/sync/pro... chrome/browser/sync/profile_sync_service.cc:2734: sync_prefs_.SetMemoryPressure(sync_prefs_.GetMemoryPressure() + 1); Looking at function names, GetMemoryPressure and SetMemoryPressure, I expected them to return some sort of pressure level. From this code it looks like these are counters of received warnings. Consider renaming these functions to reflect that. How about GetMemoryPressureWarningCount and SetMemoryPressureWarningCount? https://codereview.chromium.org/1136763004/diff/20001/chrome/browser/sync/pro... chrome/browser/sync/profile_sync_service.cc:2742: // -1 means it is new client Indent this comment and add "." at the end. https://codereview.chromium.org/1136763004/diff/20001/chrome/browser/sync/pro... chrome/browser/sync/profile_sync_service.cc:2747: UMA_HISTOGRAM_COUNTS("Sync.MemoryPressureWarningwithoutCrash", without should start with capital W https://codereview.chromium.org/1136763004/diff/20001/chrome/browser/sync/pro... File chrome/browser/sync/profile_sync_service.h (right): https://codereview.chromium.org/1136763004/diff/20001/chrome/browser/sync/pro... chrome/browser/sync/profile_sync_service.h:899: // Check if previous shutdown is crash or not. If there was a crash that means there wasn't a clean shutdown. So perhaps rephrase this a bit to make it clearer. Also explain result of the function. It returns void. Does it have any side effect? https://codereview.chromium.org/1136763004/diff/20001/components/sync_driver/... File components/sync_driver/pref_names.cc (right): https://codereview.chromium.org/1136763004/diff/20001/components/sync_driver/... components/sync_driver/pref_names.cc:105: const char kSyncMemoryPressureReceivings[] = "sync.memory_warning_receivings"; Consider naming it kSyncMemoryPressureWarningCount instead. Also "sync.memory_warning_receivings" -> "sync.memory_warning_count" https://codereview.chromium.org/1136763004/diff/20001/components/sync_driver/... components/sync_driver/pref_names.cc:108: const char kSyncShutdownUnnormal[] = "sync.shutdown_forced"; So why it is abnormal if it didn't crash. Please clarify. https://codereview.chromium.org/1136763004/diff/20001/components/sync_driver/... File components/sync_driver/pref_names.h (right): https://codereview.chromium.org/1136763004/diff/20001/components/sync_driver/... components/sync_driver/pref_names.h:69: extern const char kSyncShutdownUnnormal[]; Unnormal -> Abnormal https://codereview.chromium.org/1136763004/diff/20001/components/sync_driver/... File components/sync_driver/sync_prefs.h (right): https://codereview.chromium.org/1136763004/diff/20001/components/sync_driver/... components/sync_driver/sync_prefs.h:143: int GetMemoryPressure() const; Need comments for all 4 functions https://codereview.chromium.org/1136763004/diff/20001/components/sync_driver/... components/sync_driver/sync_prefs.h:145: bool IsPreviousCrashed() const; These two function names are confusing. Basically they are supposed to tell whether the last shutdown was successful. The names should reflect that clearly. https://codereview.chromium.org/1136763004/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1136763004/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:41562: +<histogram name="Sync.MemoryPressureWarningwithoutCrash"> without should start with capital W
gangwu@chromium.org changed reviewers: - zea@chromium.org
https://codereview.chromium.org/1136763004/diff/20001/chrome/browser/sync/pro... File chrome/browser/sync/profile_sync_service.cc (right): https://codereview.chromium.org/1136763004/diff/20001/chrome/browser/sync/pro... chrome/browser/sync/profile_sync_service.cc:920: sync_prefs_.SetCurrentShutdown(false); On 2015/05/15 20:31:04, stanisc wrote: > Please add comment explaining this. Done. https://codereview.chromium.org/1136763004/diff/20001/chrome/browser/sync/pro... chrome/browser/sync/profile_sync_service.cc:2734: sync_prefs_.SetMemoryPressure(sync_prefs_.GetMemoryPressure() + 1); On 2015/05/15 20:31:04, stanisc wrote: > Looking at function names, GetMemoryPressure and SetMemoryPressure, I expected > them to return some sort of pressure level. From this code it looks like these > are counters of received warnings. Consider renaming these functions to reflect > that. > How about GetMemoryPressureWarningCount and SetMemoryPressureWarningCount? Done. https://codereview.chromium.org/1136763004/diff/20001/chrome/browser/sync/pro... chrome/browser/sync/profile_sync_service.cc:2742: // -1 means it is new client On 2015/05/15 20:31:04, stanisc wrote: > Indent this comment and add "." at the end. Done. https://codereview.chromium.org/1136763004/diff/20001/chrome/browser/sync/pro... chrome/browser/sync/profile_sync_service.cc:2747: UMA_HISTOGRAM_COUNTS("Sync.MemoryPressureWarningwithoutCrash", On 2015/05/15 20:31:04, stanisc wrote: > without should start with capital W Done. https://codereview.chromium.org/1136763004/diff/20001/chrome/browser/sync/pro... File chrome/browser/sync/profile_sync_service.h (right): https://codereview.chromium.org/1136763004/diff/20001/chrome/browser/sync/pro... chrome/browser/sync/profile_sync_service.h:899: // Check if previous shutdown is crash or not. On 2015/05/15 20:31:04, stanisc wrote: > If there was a crash that means there wasn't a clean shutdown. > So perhaps rephrase this a bit to make it clearer. > Also explain result of the function. It returns void. Does it have any side > effect? Done. https://codereview.chromium.org/1136763004/diff/20001/components/sync_driver/... File components/sync_driver/pref_names.cc (right): https://codereview.chromium.org/1136763004/diff/20001/components/sync_driver/... components/sync_driver/pref_names.cc:105: const char kSyncMemoryPressureReceivings[] = "sync.memory_warning_receivings"; On 2015/05/15 20:31:04, stanisc wrote: > Consider naming it kSyncMemoryPressureWarningCount instead. > Also "sync.memory_warning_receivings" -> "sync.memory_warning_count" Done. https://codereview.chromium.org/1136763004/diff/20001/components/sync_driver/... components/sync_driver/pref_names.cc:108: const char kSyncShutdownUnnormal[] = "sync.shutdown_forced"; On 2015/05/15 20:31:04, stanisc wrote: > So why it is abnormal if it didn't crash. Please clarify. Done. https://codereview.chromium.org/1136763004/diff/20001/components/sync_driver/... File components/sync_driver/pref_names.h (right): https://codereview.chromium.org/1136763004/diff/20001/components/sync_driver/... components/sync_driver/pref_names.h:69: extern const char kSyncShutdownUnnormal[]; On 2015/05/15 20:31:04, stanisc wrote: > Unnormal -> Abnormal Done. https://codereview.chromium.org/1136763004/diff/20001/components/sync_driver/... File components/sync_driver/sync_prefs.h (right): https://codereview.chromium.org/1136763004/diff/20001/components/sync_driver/... components/sync_driver/sync_prefs.h:143: int GetMemoryPressure() const; On 2015/05/15 20:31:04, stanisc wrote: > Need comments for all 4 functions Done. https://codereview.chromium.org/1136763004/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1136763004/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:41558: + Counts the number of times sync service received On 2015/05/15 20:15:39, Mark P wrote: > On 2015/05/15 20:05:56, Gang Wu wrote: > > On 2015/05/15 19:18:22, Mark P wrote: > > > nit:times sync service received -> times the sync service received a > > > (i.e., add "the" and "a") > > > > Done. > > nit: Not done; you forgot to add the "the". sync service -> the sync service This is really Done :-).
In the future, please use the code review tool to reply to comments. I sent 4 comments in my last message; you replied to one. You did make some changes in response to the others. If you had hit reply on those comments in the code review tool, it would've been easier for me to track down what I said so I can see if/how you did it. thanks, mark https://codereview.chromium.org/1136763004/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1136763004/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:41558: + Counts the number of times each user's sync service received a nit: each -> a ditto in other histogram https://codereview.chromium.org/1136763004/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:41559: + MEMORY_PRESSURE_LEVEL_CRITICAL warning before sync service crashed. The sync nit: missing "the" before sync https://codereview.chromium.org/1136763004/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:41560: + service emits this number on user's next startup. This counts are per user. nit: on user's next startup -> the next time the user's profile is opened ditto in other histogram https://codereview.chromium.org/1136763004/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:41560: + service emits this number on user's next startup. This counts are per user. nit (plural agreement): This counts are -> This count is ditto in other histogram https://codereview.chromium.org/1136763004/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:41560: + service emits this number on user's next startup. This counts are per user. You never answered this paragraph of my previous comment: > Also explain what happens if multiple profiles are open when the sync service > crashes. Are sync service crashes per profile or global?* Would this count be > emitted for each profile on the next time it starts or only one profile on the > next time it starts? What about if the browser crashes? Is the answer about > what gets emitted the same? *In other words, is it possible that the sync service could crash for one user but not another? https://codereview.chromium.org/1136763004/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:41561: + The sync service's crashes may cause by browser crashes, sync freezing or nit: or -> , https://codereview.chromium.org/1136763004/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:41561: + The sync service's crashes may cause by browser crashes, sync freezing or How is a crash a freeze? Freeze makes me think of an infinite loop... Or by crash do you simply mean unclean shutdown? If that's what you mean, please revise the histogram name and the description. https://codereview.chromium.org/1136763004/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:41569: + Counts the number of times each user's the sync service received a nit: omit "the" before "sync" https://codereview.chromium.org/1136763004/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:41570: + MEMORY_PRESSURE_LEVEL_CRITICAL warning before sync service shutdown cleanly. nit: add "the" before "sync" https://codereview.chromium.org/1136763004/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:41573: + freezing or etc. This last sentence is not necessary or useful here.
https://codereview.chromium.org/1136763004/diff/80001/chrome/browser/sync/pro... File chrome/browser/sync/profile_sync_service.cc (right): https://codereview.chromium.org/1136763004/diff/80001/chrome/browser/sync/pro... chrome/browser/sync/profile_sync_service.cc:920: // Mark this dhutdown cleanly(without crash). Typo: dhutdown https://codereview.chromium.org/1136763004/diff/80001/chrome/browser/sync/pro... chrome/browser/sync/profile_sync_service.cc:2740: void ProfileSyncService::RecordPreviousShutdownMemoryWarning() { How about this name: ReportPreviousSessionMemoryWarningCount? https://codereview.chromium.org/1136763004/diff/80001/chrome/browser/sync/pro... chrome/browser/sync/profile_sync_service.cc:2754: sync_prefs_.SetCurrentShutdownCleanly(false); Add a comment explaining that this would be set to true on a clean shutdown and remain false if the process crashes. https://codereview.chromium.org/1136763004/diff/80001/components/sync_driver/... File components/sync_driver/pref_names.cc (right): https://codereview.chromium.org/1136763004/diff/80001/components/sync_driver/... components/sync_driver/pref_names.cc:107: // Stores if sync shutdown uncleanly. This should be in sync with the field name. https://codereview.chromium.org/1136763004/diff/80001/components/sync_driver/... File components/sync_driver/sync_prefs.h (right): https://codereview.chromium.org/1136763004/diff/80001/components/sync_driver/... components/sync_driver/sync_prefs.h:148: bool IsPreviousShutdownCleanly() const; The function name could be a bit clearer.
https://codereview.chromium.org/1136763004/diff/80001/chrome/browser/sync/pro... File chrome/browser/sync/profile_sync_service.cc (right): https://codereview.chromium.org/1136763004/diff/80001/chrome/browser/sync/pro... chrome/browser/sync/profile_sync_service.cc:920: // Mark this dhutdown cleanly(without crash). On 2015/05/15 22:06:48, stanisc wrote: > Typo: dhutdown Done. https://codereview.chromium.org/1136763004/diff/80001/chrome/browser/sync/pro... chrome/browser/sync/profile_sync_service.cc:2740: void ProfileSyncService::RecordPreviousShutdownMemoryWarning() { On 2015/05/15 22:06:48, stanisc wrote: > How about this name: ReportPreviousSessionMemoryWarningCount? Done. https://codereview.chromium.org/1136763004/diff/80001/chrome/browser/sync/pro... chrome/browser/sync/profile_sync_service.cc:2754: sync_prefs_.SetCurrentShutdownCleanly(false); On 2015/05/15 22:06:48, stanisc wrote: > Add a comment explaining that this would be set to true on a clean shutdown > and remain false if the process crashes. Done. https://codereview.chromium.org/1136763004/diff/80001/components/sync_driver/... File components/sync_driver/pref_names.cc (right): https://codereview.chromium.org/1136763004/diff/80001/components/sync_driver/... components/sync_driver/pref_names.cc:107: // Stores if sync shutdown uncleanly. On 2015/05/15 22:06:48, stanisc wrote: > This should be in sync with the field name. Done. https://codereview.chromium.org/1136763004/diff/80001/components/sync_driver/... File components/sync_driver/sync_prefs.h (right): https://codereview.chromium.org/1136763004/diff/80001/components/sync_driver/... components/sync_driver/sync_prefs.h:148: bool IsPreviousShutdownCleanly() const; On 2015/05/15 22:06:48, stanisc wrote: > The function name could be a bit clearer. Done. https://codereview.chromium.org/1136763004/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1136763004/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:41558: + Counts the number of times each user's sync service received a On 2015/05/15 21:37:39, Mark P wrote: > nit: each -> a > ditto in other histogram Done. https://codereview.chromium.org/1136763004/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:41559: + MEMORY_PRESSURE_LEVEL_CRITICAL warning before sync service crashed. The sync On 2015/05/15 21:37:38, Mark P wrote: > nit: missing "the" before sync Done. https://codereview.chromium.org/1136763004/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:41560: + service emits this number on user's next startup. This counts are per user. On 2015/05/15 21:37:39, Mark P wrote: > You never answered this paragraph of my previous comment: > > Also explain what happens if multiple profiles are open when the sync service > > crashes. Are sync service crashes per profile or global?* Would this count > be > > emitted for each profile on the next time it starts or only one profile on the > > next time it starts? What about if the browser crashes? Is the answer about > > what gets emitted the same? > > *In other words, is it possible that the sync service could crash for one user > but not another? yes, it is possible. https://codereview.chromium.org/1136763004/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:41561: + The sync service's crashes may cause by browser crashes, sync freezing or On 2015/05/15 21:37:39, Mark P wrote: > nit: or -> , removed this sentence. https://codereview.chromium.org/1136763004/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:41569: + Counts the number of times each user's the sync service received a On 2015/05/15 21:37:39, Mark P wrote: > nit: omit "the" before "sync" Done. https://codereview.chromium.org/1136763004/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:41570: + MEMORY_PRESSURE_LEVEL_CRITICAL warning before sync service shutdown cleanly. On 2015/05/15 21:37:39, Mark P wrote: > nit: add "the" before "sync" Done. https://codereview.chromium.org/1136763004/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:41573: + freezing or etc. On 2015/05/15 21:37:38, Mark P wrote: > This last sentence is not necessary or useful here. Done.
sorry, forgot to upload the changes, please take a look now.
https://codereview.chromium.org/1136763004/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1136763004/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:41559: + MEMORY_PRESSURE_LEVEL_CRITICAL warning bbefore the sync service shut down nit: typo (bbefore) https://codereview.chromium.org/1136763004/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:41560: + unleanly. The sync service emits this number when the next time the user's nit: omit "when" ditto below https://codereview.chromium.org/1136763004/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:41560: + unleanly. The sync service emits this number when the next time the user's nit: typo (unleanly) ditto below Please proofread what you write. https://codereview.chromium.org/1136763004/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:41561: + sync service is started. This count is per user. How about the following? Replace is started. This count is per user. with is started, which will likely happen the next time the user's profile is opened after a Chrome restart. This count is emitted once per user/profile. Things like browser crashes that implicitly bring down all users' sync services will cause unclean shutdown tags to appear on all open profiles, meaning that there will be multiple emissions to this histogram as those profiles are re-opened. (Is that all correct?) ditto below
Thanks for recommendation! https://codereview.chromium.org/1136763004/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1136763004/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:41559: + MEMORY_PRESSURE_LEVEL_CRITICAL warning bbefore the sync service shut down On 2015/05/15 22:44:44, Mark P wrote: > nit: typo (bbefore) Done. https://codereview.chromium.org/1136763004/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:41560: + unleanly. The sync service emits this number when the next time the user's On 2015/05/15 22:44:44, Mark P wrote: > nit: typo (unleanly) > ditto below > > Please proofread what you write. Done. https://codereview.chromium.org/1136763004/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:41560: + unleanly. The sync service emits this number when the next time the user's On 2015/05/15 22:44:44, Mark P wrote: > nit: omit "when" > ditto below Done. https://codereview.chromium.org/1136763004/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:41561: + sync service is started. This count is per user. On 2015/05/15 22:44:44, Mark P wrote: > How about the following? Replace > is started. This count is per user. > with > is started, which will likely happen the next time the user's profile is opened > after a Chrome restart. This count is emitted once per user/profile. Things > like browser crashes that implicitly bring down all users' sync services will > cause unclean shutdown tags to appear on all open profiles, meaning that there > will be multiple emissions to this histogram as those profiles are re-opened. > (Is that all correct?) > > ditto below Done.
histograms.xml lgtm
lgtm with a few nits. https://codereview.chromium.org/1136763004/diff/100001/chrome/browser/sync/pr... File chrome/browser/sync/profile_sync_service.cc (right): https://codereview.chromium.org/1136763004/diff/100001/chrome/browser/sync/pr... chrome/browser/sync/profile_sync_service.cc:920: // Mark this shutdown cleanly(without crash). How about this: mark this as a clean shutdown. https://codereview.chromium.org/1136763004/diff/100001/chrome/browser/sync/pr... chrome/browser/sync/profile_sync_service.cc:2754: // Will to true during a clean shutdown, so crash or something elase will There are still a couple of typos here. Please review and fix. https://codereview.chromium.org/1136763004/diff/100001/components/sync_driver... File components/sync_driver/pref_names.cc (right): https://codereview.chromium.org/1136763004/diff/100001/components/sync_driver... components/sync_driver/pref_names.cc:105: const char kSyncMemoryPressureWarningCount[] = "sync.memory_warning_counts"; counts -> count. https://codereview.chromium.org/1136763004/diff/100001/components/sync_driver... File components/sync_driver/sync_prefs.h (right): https://codereview.chromium.org/1136763004/diff/100001/components/sync_driver... components/sync_driver/sync_prefs.h:143: // Get/Set number of memory warning received. warning -> warnings. https://codereview.chromium.org/1136763004/diff/100001/components/sync_driver... components/sync_driver/sync_prefs.h:147: // Check if previous shutdown cleanly. Check if the previous shutdown was clean. https://codereview.chromium.org/1136763004/diff/100001/components/sync_driver... components/sync_driver/sync_prefs.h:150: // Set the sync service shutdown cleanly. Set whether the last shutdown was clean.
Thanks for the review! https://codereview.chromium.org/1136763004/diff/100001/chrome/browser/sync/pr... File chrome/browser/sync/profile_sync_service.cc (right): https://codereview.chromium.org/1136763004/diff/100001/chrome/browser/sync/pr... chrome/browser/sync/profile_sync_service.cc:920: // Mark this shutdown cleanly(without crash). On 2015/05/15 23:11:04, stanisc wrote: > How about this: mark this as a clean shutdown. Done. https://codereview.chromium.org/1136763004/diff/100001/chrome/browser/sync/pr... chrome/browser/sync/profile_sync_service.cc:2754: // Will to true during a clean shutdown, so crash or something elase will On 2015/05/15 23:11:04, stanisc wrote: > There are still a couple of typos here. Please review and fix. Done. https://codereview.chromium.org/1136763004/diff/100001/components/sync_driver... File components/sync_driver/pref_names.cc (right): https://codereview.chromium.org/1136763004/diff/100001/components/sync_driver... components/sync_driver/pref_names.cc:105: const char kSyncMemoryPressureWarningCount[] = "sync.memory_warning_counts"; On 2015/05/15 23:11:04, stanisc wrote: > counts -> count. Done. https://codereview.chromium.org/1136763004/diff/100001/components/sync_driver... File components/sync_driver/sync_prefs.h (right): https://codereview.chromium.org/1136763004/diff/100001/components/sync_driver... components/sync_driver/sync_prefs.h:143: // Get/Set number of memory warning received. On 2015/05/15 23:11:04, stanisc wrote: > warning -> warnings. Done. https://codereview.chromium.org/1136763004/diff/100001/components/sync_driver... components/sync_driver/sync_prefs.h:147: // Check if previous shutdown cleanly. On 2015/05/15 23:11:04, stanisc wrote: > Check if the previous shutdown was clean. Done. https://codereview.chromium.org/1136763004/diff/100001/components/sync_driver... components/sync_driver/sync_prefs.h:150: // Set the sync service shutdown cleanly. On 2015/05/15 23:11:04, stanisc wrote: > Set whether the last shutdown was clean. Done.
The CQ bit was checked by gangwu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stanisc@chromium.org, mpearson@chromium.org Link to the patchset: https://codereview.chromium.org/1136763004/#ps140001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1136763004/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/68208a4cb9def359c3054f0769853f31602968bf Cr-Commit-Position: refs/heads/master@{#330253}
Message was sent while issue was closed.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
