Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1227363002:
[Background Sync] Gather UMA data for Background Sync (Closed)
|
Created:
5 years, 5 months ago by iclelland Modified:
5 years, 5 months ago CC:
chromium-reviews, tim+watch_chromium.org, zea+watch_chromium.org, maxbogue+watch_chromium.org, jam, pvalenzuela+watch_chromium.org, plaree+watch_chromium.org, jkarlin+watch_chromium.org, asvitkine+watch_chromium.org, darin-cc_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. |
Description[Background Sync] Gather UMA data for Background Sync
This patch adds UMA histograms for
* Sync registrations and unregistrations
* Sync event success and failure
* Sync event total run time
It adds logging to the BackgroundSyncManager to collect the data for these histograms.
BUG=490482
Committed: https://crrev.com/a84c0e6e9875a605f5ef9a1f78f59c4cadb43836
Cr-Commit-Position: refs/heads/master@{#339072}
Patch Set 1 #Patch Set 2 : Record whether a registration could fire immediately or not #
Total comments: 9
Patch Set 3 : Addressing review comments #Patch Set 4 : Remove uncollected metrics #
Total comments: 23
Patch Set 5 : Addressing review comments #Patch Set 6 : Properly account for total sync running time #Patch Set 7 : Rename WouldFireRegistrationWithOptions to AreOptionConditionsMet #
Total comments: 2
Patch Set 8 : Explicit conversion to boolean in UMA recording #
Total comments: 14
Patch Set 9 : Addressing latest comments #Patch Set 10 : Remove BackgroundSync.Registration.Periodic.CouldFire #Patch Set 11 : Rebase #Patch Set 12 : Fix bad rebase #
Messages
Total messages: 26 (6 generated)
iclelland@chromium.org changed reviewers: + asvitkine@chromium.org, jkarlin@chromium.org
asvitkine -- can you PTAL at the UMA code, and the updates to histograms.xml: Have I missed anything important? Defined the historgrams correctly? Am I actually allowed to be listed as <owner> in them :) Thanks! jkarlin -- PTAL at everything.
I don't see all of the new static functions being called - did you omit some code perhaps? https://codereview.chromium.org/1227363002/diff/20001/content/browser/backgro... File content/browser/background_sync/background_sync_manager.cc (right): https://codereview.chromium.org/1227363002/diff/20001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:922: startTime); Ditto https://codereview.chromium.org/1227363002/diff/20001/content/browser/backgro... File content/browser/background_sync/background_sync_manager.h (right): https://codereview.chromium.org/1227363002/diff/20001/content/browser/backgro... content/browser/background_sync/background_sync_manager.h:268: static void OnAllSyncEventsCompleted(const base::TimeTicks& startTime, Nit: hacker_style not camelCase for param names https://codereview.chromium.org/1227363002/diff/20001/content/browser/backgro... File content/browser/background_sync/background_sync_metrics.h (right): https://codereview.chromium.org/1227363002/diff/20001/content/browser/backgro... content/browser/background_sync/background_sync_metrics.h:14: class BackgroundSyncMetrics { Add a comment explaining the purpose of this class. https://codereview.chromium.org/1227363002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1227363002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:2516: + order to fire a registered periodic sync event. This description doesn't tell me much about what a false or true event is. Is a false event that Chrome was started for some other purpose than the Background Sync Manager to fire the event... Please clarify.
Not all of the metrics are called yet. The registration, unregistration, and event time and result tracking have been implemented, but not the browser startup metrics. I added them because we have listed them as desired metrics in the bug (crbug.com/490482), but the code for them isn't written yet. Should I leave the methods out until the calling code is also written, or is it okay to have them there, ready to use in the next CL? https://codereview.chromium.org/1227363002/diff/20001/content/browser/backgro... File content/browser/background_sync/background_sync_manager.cc (right): https://codereview.chromium.org/1227363002/diff/20001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:922: startTime); On 2015/07/09 21:02:57, Alexei Svitkine wrote: > Ditto Done. https://codereview.chromium.org/1227363002/diff/20001/content/browser/backgro... File content/browser/background_sync/background_sync_manager.h (right): https://codereview.chromium.org/1227363002/diff/20001/content/browser/backgro... content/browser/background_sync/background_sync_manager.h:268: static void OnAllSyncEventsCompleted(const base::TimeTicks& startTime, On 2015/07/09 21:02:57, Alexei Svitkine wrote: > Nit: hacker_style not camelCase for param names Done. https://codereview.chromium.org/1227363002/diff/20001/content/browser/backgro... File content/browser/background_sync/background_sync_metrics.h (right): https://codereview.chromium.org/1227363002/diff/20001/content/browser/backgro... content/browser/background_sync/background_sync_metrics.h:14: class BackgroundSyncMetrics { On 2015/07/09 21:02:57, Alexei Svitkine wrote: > Add a comment explaining the purpose of this class. Done. https://codereview.chromium.org/1227363002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1227363002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:2516: + order to fire a registered periodic sync event. On 2015/07/09 21:02:57, Alexei Svitkine wrote: > This description doesn't tell me much about what a false or true event is. > > Is a false event that Chrome was started for some other purpose than the > Background Sync Manager to fire the event... Please clarify. False here would indicate that the wakeup was wasted -- that is, the sync event that we woke the browser up for didn't actually exist, or we tried to run it and it failed. It means that Chrome wasn't ablt to do any useful work. Would a better enum be in order here, or just a different description?
I think it's better to land the metrics in the same CL where they get called. https://codereview.chromium.org/1227363002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1227363002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:2516: + order to fire a registered periodic sync event. On 2015/07/10 14:57:26, iclelland wrote: > On 2015/07/09 21:02:57, Alexei Svitkine wrote: > > This description doesn't tell me much about what a false or true event is. > > > > Is a false event that Chrome was started for some other purpose than the > > Background Sync Manager to fire the event... Please clarify. > > False here would indicate that the wakeup was wasted -- that is, the sync event > that we woke the browser up for didn't actually exist, or we tried to run it and > it failed. It means that Chrome wasn't ablt to do any useful work. > > Would a better enum be in order here, or just a different description? It sounds like an enum would be a better fit here indeed.
On 2015/07/10 15:12:50, Alexei Svitkine wrote: > I think it's better to land the metrics in the same CL where they get called. > > https://codereview.chromium.org/1227363002/diff/20001/tools/metrics/histogram... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/1227363002/diff/20001/tools/metrics/histogram... > tools/metrics/histograms/histograms.xml:2516: + order to fire a registered > periodic sync event. > On 2015/07/10 14:57:26, iclelland wrote: > > On 2015/07/09 21:02:57, Alexei Svitkine wrote: > > > This description doesn't tell me much about what a false or true event is. > > > > > > Is a false event that Chrome was started for some other purpose than the > > > Background Sync Manager to fire the event... Please clarify. > > > > False here would indicate that the wakeup was wasted -- that is, the sync > event > > that we woke the browser up for didn't actually exist, or we tried to run it > and > > it failed. It means that Chrome wasn't ablt to do any useful work. > > > > Would a better enum be in order here, or just a different description? > > It sounds like an enum would be a better fit here indeed. Two birds; same stone -- I've remove the uncollected metrics from this CL. Once I get the code in place to gather the wake-up stats, I'll start a new CL for that (with a new enum :) )
lgtm
https://codereview.chromium.org/1227363002/diff/60001/content/browser/backgro... File content/browser/background_sync/background_sync_manager.cc (right): https://codereview.chromium.org/1227363002/diff/60001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:81: options.periodicity, false, Prefer an enum to a bool. https://codereview.chromium.org/1227363002/diff/60001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:82: BackgroundSyncMetrics::REGISTRATION_RESULT_FAILED); RESULT_DISABLED instead of RESULT_FAILED. https://codereview.chromium.org/1227363002/diff/60001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:104: BackgroundSyncMetrics::CountUnregistration(periodicity, false); Why a bool? Why not use the same enum as registration? That would tell us more about what's going on. https://codereview.chromium.org/1227363002/diff/60001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:310: bool registrationCouldFire = WouldFireRegistrationWithOptions(options); registration_could_fire https://codereview.chromium.org/1227363002/diff/60001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:475: bool registrationCouldFire = registration_could_fire https://codereview.chromium.org/1227363002/diff/60001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:683: bool BackgroundSyncManager::WouldFireRegistrationWithOptions( Why this function? Why not just call IsRegistrationReadyToFire for the UMA check? https://codereview.chromium.org/1227363002/diff/60001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:782: base::Bind(&OnAllSyncEventsCompleted, start_time, callback)); OnAllSyncEventsCompleted doesn't get called when you think it does. This barrier is complete once all of the service worker versions have called DispatchSync. It doesn't wait for them to complete. https://codereview.chromium.org/1227363002/diff/60001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:816: base::Bind(&BackgroundSyncManager::EventComplete, To record the time of all of the syncs, you'll want to create a barrier in this function and pass it to EventComplete and then to EventCompleteImpl to run. https://codereview.chromium.org/1227363002/diff/60001/content/browser/backgro... File content/browser/background_sync/background_sync_metrics.cc (right): https://codereview.chromium.org/1227363002/diff/60001/content/browser/backgro... content/browser/background_sync/background_sync_metrics.cc:27: UMA_HISTOGRAM_MEDIUM_TIMES("BackgroundSync.Event.Time", time); MEDIUM_TIMES goes from 10ms to 3 minutes. We should range from 10ms to 6 minutes (with 5 being the max we should be able to reach). https://codereview.chromium.org/1227363002/diff/60001/content/browser/backgro... File content/browser/background_sync/background_sync_metrics.h (right): https://codereview.chromium.org/1227363002/diff/60001/content/browser/backgro... content/browser/background_sync/background_sync_metrics.h:25: }; Why not use BackgroundSyncManager::ErrorType?
Thanks for the review, Josh -- I've addressed most of the comments now. I think the only outstanding one is about recording the total event time correctly. https://codereview.chromium.org/1227363002/diff/60001/content/browser/backgro... File content/browser/background_sync/background_sync_manager.cc (right): https://codereview.chromium.org/1227363002/diff/60001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:81: options.periodicity, false, On 2015/07/13 16:51:12, jkarlin wrote: > Prefer an enum to a bool. Yes, generally. I hate to have to define the enum here, though, and import it in background_sync_metrics.h. But it's worse to define it there, when I'd really want it to be the return value for WouldFireRegistrationWithOptions. https://codereview.chromium.org/1227363002/diff/60001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:82: BackgroundSyncMetrics::REGISTRATION_RESULT_FAILED); On 2015/07/13 16:51:13, jkarlin wrote: > RESULT_DISABLED instead of RESULT_FAILED. I'll switch to ERROR_TYPE_STORAGE, to mirror ErrorType, as discussed in a later comment. https://codereview.chromium.org/1227363002/diff/60001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:104: BackgroundSyncMetrics::CountUnregistration(periodicity, false); On 2015/07/13 16:51:12, jkarlin wrote: > Why a bool? Why not use the same enum as registration? That would tell us more > about what's going on. They both started as bool, but registration has different possibilities (duplicate registration, e.g.), so I switched it. I'll switch unregister as well, if that makes sense. https://codereview.chromium.org/1227363002/diff/60001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:310: bool registrationCouldFire = WouldFireRegistrationWithOptions(options); On 2015/07/13 16:51:13, jkarlin wrote: > registration_could_fire Done. https://codereview.chromium.org/1227363002/diff/60001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:475: bool registrationCouldFire = On 2015/07/13 16:51:13, jkarlin wrote: > registration_could_fire Done. https://codereview.chromium.org/1227363002/diff/60001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:683: bool BackgroundSyncManager::WouldFireRegistrationWithOptions( On 2015/07/13 16:51:12, jkarlin wrote: > Why this function? Why not just call IsRegistrationReadyToFire for the UMA > check? IsRegistrationReadyToFire takes a Registration object, and I wanted to call this early in the Register() method, when all we have is an options object. That way, even if we bail early, we can still report whether the registration *could* have fired, as far as the developer was concerned. I thought it made sense to factor out the "are these options satisfied right now" check, and leave the sync-lifecycle (PENDING vs FIRING, etc) checks to the method that is actually checking registered syncs. https://codereview.chromium.org/1227363002/diff/60001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:816: base::Bind(&BackgroundSyncManager::EventComplete, On 2015/07/13 16:51:12, jkarlin wrote: > To record the time of all of the syncs, you'll want to create a barrier in this > function and pass it to EventComplete and then to EventCompleteImpl to run. Thanks -- You were right, above, about being mistaken about when the first barrier would fire. I'll move it here, and gather stats at that point. https://codereview.chromium.org/1227363002/diff/60001/content/browser/backgro... File content/browser/background_sync/background_sync_metrics.cc (right): https://codereview.chromium.org/1227363002/diff/60001/content/browser/backgro... content/browser/background_sync/background_sync_metrics.cc:27: UMA_HISTOGRAM_MEDIUM_TIMES("BackgroundSync.Event.Time", time); On 2015/07/13 16:51:13, jkarlin wrote: > MEDIUM_TIMES goes from 10ms to 3 minutes. We should range from 10ms to 6 minutes > (with 5 being the max we should be able to reach). Thanks -- I meant go back to check that. Done. https://codereview.chromium.org/1227363002/diff/60001/content/browser/backgro... File content/browser/background_sync/background_sync_metrics.h (right): https://codereview.chromium.org/1227363002/diff/60001/content/browser/backgro... content/browser/background_sync/background_sync_metrics.h:25: }; On 2015/07/13 16:51:13, jkarlin wrote: > Why not use BackgroundSyncManager::ErrorType? We need to mirror the enum in histograms.xml, which records duplicate successes distinctly from new-registration successes. (I want to track the number of times that an identical sync is registered.) Also, I was hoping to not have to depend on background_sync_manager.h here (trying to avoid circular dependencies) but if that really is the best enum to use -- and there's no better place to move ErrorType -- then I'll do that. Maybe the better thing for recording duplicates is to tack on a boolean param to CountRegistration for is_duplicate_registration, and leave the enums to the implementation. I'll switch that.
https://codereview.chromium.org/1227363002/diff/60001/content/browser/backgro... File content/browser/background_sync/background_sync_manager.cc (right): https://codereview.chromium.org/1227363002/diff/60001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:683: bool BackgroundSyncManager::WouldFireRegistrationWithOptions( On 2015/07/13 19:34:31, iclelland wrote: > On 2015/07/13 16:51:12, jkarlin wrote: > > Why this function? Why not just call IsRegistrationReadyToFire for the UMA > > check? > > IsRegistrationReadyToFire takes a Registration object, and I wanted to call this > early in the Register() method, when all we have is an options object. That way, > even if we bail early, we can still report whether the registration *could* have > fired, as far as the developer was concerned. > I thought it made sense to factor out the "are these options satisfied right > now" check, and leave the sync-lifecycle (PENDING vs FIRING, etc) checks to the > method that is actually checking registered syncs. Okay. Can you rename to AreOptionConditionsMet? https://codereview.chromium.org/1227363002/diff/60001/content/browser/backgro... File content/browser/background_sync/background_sync_metrics.h (right): https://codereview.chromium.org/1227363002/diff/60001/content/browser/backgro... content/browser/background_sync/background_sync_metrics.h:25: }; On 2015/07/13 19:34:31, iclelland wrote: > On 2015/07/13 16:51:13, jkarlin wrote: > > Why not use BackgroundSyncManager::ErrorType? > > We need to mirror the enum in histograms.xml, which records duplicate successes > distinctly from new-registration successes. (I want to track the number of times > that an identical sync is registered.) > > Also, I was hoping to not have to depend on background_sync_manager.h here > (trying to avoid circular dependencies) but if that really is the best enum to > use -- and there's no better place to move ErrorType -- then I'll do that. > > Maybe the better thing for recording duplicates is to tack on a boolean param to > CountRegistration for is_duplicate_registration, and leave the enums to the > implementation. I'll switch that. Thanks for explaining, I hadn't noticed the "duplicate" distinction before and mistakenly thought ErrorType was the same as the new enum. ErrorType can be moved into its own file, that's no problem. If you prefer the original approach you took, with the separate enum, that also works for me.
https://codereview.chromium.org/1227363002/diff/120001/content/browser/backgr... File content/browser/background_sync/background_sync_metrics.cc (right): https://codereview.chromium.org/1227363002/diff/120001/content/browser/backgr... content/browser/background_sync/background_sync_metrics.cc:51: UMA_HISTOGRAM_BOOLEAN("BackgroundSync.Registration.Periodic.CouldFire", These should either be made proper booleans or use UMA_HISTOGRAM_ENUMERATION.
https://codereview.chromium.org/1227363002/diff/60001/content/browser/backgro... File content/browser/background_sync/background_sync_manager.cc (right): https://codereview.chromium.org/1227363002/diff/60001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:683: bool BackgroundSyncManager::WouldFireRegistrationWithOptions( On 2015/07/14 15:54:10, jkarlin wrote: > On 2015/07/13 19:34:31, iclelland wrote: > > On 2015/07/13 16:51:12, jkarlin wrote: > > > Why this function? Why not just call IsRegistrationReadyToFire for the UMA > > > check? > > > > IsRegistrationReadyToFire takes a Registration object, and I wanted to call > this > > early in the Register() method, when all we have is an options object. That > way, > > even if we bail early, we can still report whether the registration *could* > have > > fired, as far as the developer was concerned. > > I thought it made sense to factor out the "are these options satisfied right > > now" check, and leave the sync-lifecycle (PENDING vs FIRING, etc) checks to > the > > method that is actually checking registered syncs. > > Okay. Can you rename to AreOptionConditionsMet? Done. https://codereview.chromium.org/1227363002/diff/60001/content/browser/backgro... content/browser/background_sync/background_sync_manager.cc:782: base::Bind(&OnAllSyncEventsCompleted, start_time, callback)); On 2015/07/13 16:51:13, jkarlin wrote: > OnAllSyncEventsCompleted doesn't get called when you think it does. This barrier > is complete once all of the service worker versions have called DispatchSync. It > doesn't wait for them to complete. Thanks for catching that -- I've added a second barrier now, so one is called when all events have fired, and the other when all events have completed. That second one now calls OnAllSyncEventsCompleted, as intended. https://codereview.chromium.org/1227363002/diff/120001/content/browser/backgr... File content/browser/background_sync/background_sync_metrics.cc (right): https://codereview.chromium.org/1227363002/diff/120001/content/browser/backgr... content/browser/background_sync/background_sync_metrics.cc:51: UMA_HISTOGRAM_BOOLEAN("BackgroundSync.Registration.Periodic.CouldFire", On 2015/07/14 21:56:22, jkarlin wrote: > These should either be made proper booleans or use UMA_HISTOGRAM_ENUMERATION. Done.
Looking really good. Just a few more things. https://codereview.chromium.org/1227363002/diff/140001/content/browser/backgr... File content/browser/background_sync/background_sync_metrics.cc (right): https://codereview.chromium.org/1227363002/diff/140001/content/browser/backgr... content/browser/background_sync/background_sync_metrics.cc:52: UMA_HISTOGRAM_BOOLEAN("BackgroundSync.Registration.Periodic.CouldFire", Not sure what periodic could fire means. https://codereview.chromium.org/1227363002/diff/140001/content/browser/backgr... content/browser/background_sync/background_sync_metrics.cc:62: void BackgroundSyncMetrics::CountUnregistration( Is unregistration a word? Perhaps CountUnregister and CountRegister? https://codereview.chromium.org/1227363002/diff/140001/content/browser/backgr... File content/browser/background_sync/background_sync_metrics.h (right): https://codereview.chromium.org/1227363002/diff/140001/content/browser/backgr... content/browser/background_sync/background_sync_metrics.h:34: // Records the total time spent running all sync events. This makes it sound like it's counting up all sync event time forever. Perhaps, "records the time spent running a batch of sync events". https://codereview.chromium.org/1227363002/diff/140001/content/browser/backgr... content/browser/background_sync/background_sync_metrics.h:35: static void RecordSyncEventHandlingTime(const base::TimeDelta& time); Perhaps RecordBatchSyncEventHandlingTime to make it clear that it's not recording a single sync event. https://codereview.chromium.org/1227363002/diff/140001/content/browser/backgr... content/browser/background_sync/background_sync_metrics.h:38: // whether the network/power was sufficient for the sync to fire immediately network/power is specific and the conditions might change over time. Suggest, "indicates whether the conditions were sufficient for..." https://codereview.chromium.org/1227363002/diff/140001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1227363002/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:2453: + <summary>Time taken to execute all sync events to completion.</summary> Suggest: "Time taken to execute a batch of sync events. A batch is defined as the set of sync events dispatched at the same time by the BackgroundSyncManager. Periodic syncs often run in a batch. One-shots usually run individually (a batch of one), unless the device was offline and multiple are waiting for the device to go back online."
https://codereview.chromium.org/1227363002/diff/140001/content/browser/backgr... File content/browser/background_sync/background_sync_metrics.cc (right): https://codereview.chromium.org/1227363002/diff/140001/content/browser/backgr... content/browser/background_sync/background_sync_metrics.cc:52: UMA_HISTOGRAM_BOOLEAN("BackgroundSync.Registration.Periodic.CouldFire", On 2015/07/15 17:17:06, jkarlin wrote: > Not sure what periodic could fire means. I suppose it would have to mean "the power and network were sufficient for this task to happen immediately, and also, the app chose to register a periodic task to do it in the future." If it's not a useful metric (and I'm by no means convinced that it is) then I can remove it. https://codereview.chromium.org/1227363002/diff/140001/content/browser/backgr... content/browser/background_sync/background_sync_metrics.cc:62: void BackgroundSyncMetrics::CountUnregistration( On 2015/07/15 17:17:06, jkarlin wrote: > Is unregistration a word? Perhaps CountUnregister and CountRegister? Deregisration? Disregistration? :) Switched to CountUnregister and CountRegister. Done. https://codereview.chromium.org/1227363002/diff/140001/content/browser/backgr... File content/browser/background_sync/background_sync_metrics.h (right): https://codereview.chromium.org/1227363002/diff/140001/content/browser/backgr... content/browser/background_sync/background_sync_metrics.h:34: // Records the total time spent running all sync events. On 2015/07/15 17:17:06, jkarlin wrote: > This makes it sound like it's counting up all sync event time forever. Perhaps, > "records the time spent running a batch of sync events". Done. https://codereview.chromium.org/1227363002/diff/140001/content/browser/backgr... content/browser/background_sync/background_sync_metrics.h:35: static void RecordSyncEventHandlingTime(const base::TimeDelta& time); On 2015/07/15 17:17:06, jkarlin wrote: > Perhaps RecordBatchSyncEventHandlingTime to make it clear that it's not > recording a single sync event. Done. https://codereview.chromium.org/1227363002/diff/140001/content/browser/backgr... content/browser/background_sync/background_sync_metrics.h:38: // whether the network/power was sufficient for the sync to fire immediately On 2015/07/15 17:17:06, jkarlin wrote: > network/power is specific and the conditions might change over time. Suggest, > "indicates whether the conditions were sufficient for..." Done. https://codereview.chromium.org/1227363002/diff/140001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1227363002/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:2453: + <summary>Time taken to execute all sync events to completion.</summary> On 2015/07/15 17:17:06, jkarlin wrote: > Suggest: "Time taken to execute a batch of sync events. A batch is defined as > the set of sync events dispatched at the same time by the BackgroundSyncManager. > Periodic syncs often run in a batch. One-shots usually run individually (a batch > of one), unless the device was offline and multiple are waiting for the device > to go back online." Done.
lgtm with one change https://codereview.chromium.org/1227363002/diff/140001/content/browser/backgr... File content/browser/background_sync/background_sync_metrics.cc (right): https://codereview.chromium.org/1227363002/diff/140001/content/browser/backgr... content/browser/background_sync/background_sync_metrics.cc:52: UMA_HISTOGRAM_BOOLEAN("BackgroundSync.Registration.Periodic.CouldFire", On 2015/07/15 18:01:35, iclelland wrote: > On 2015/07/15 17:17:06, jkarlin wrote: > > Not sure what periodic could fire means. > > I suppose it would have to mean "the power and network were sufficient for this > task to happen immediately, and also, the app chose to register a periodic task > to do it in the future." > > If it's not a useful metric (and I'm by no means convinced that it is) then I > can remove it. Let's remove this one for now. Easy to add back later if necessary.
https://codereview.chromium.org/1227363002/diff/140001/content/browser/backgr... File content/browser/background_sync/background_sync_metrics.cc (right): https://codereview.chromium.org/1227363002/diff/140001/content/browser/backgr... content/browser/background_sync/background_sync_metrics.cc:52: UMA_HISTOGRAM_BOOLEAN("BackgroundSync.Registration.Periodic.CouldFire", On 2015/07/15 19:27:33, jkarlin_OOO wrote: > On 2015/07/15 18:01:35, iclelland wrote: > > On 2015/07/15 17:17:06, jkarlin wrote: > > > Not sure what periodic could fire means. > > > > I suppose it would have to mean "the power and network were sufficient for > this > > task to happen immediately, and also, the app chose to register a periodic > task > > to do it in the future." > > > > If it's not a useful metric (and I'm by no means convinced that it is) then I > > can remove it. > > Let's remove this one for now. Easy to add back later if necessary. Done.
The CQ bit was checked by iclelland@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, jkarlin@chromium.org Link to the patchset: https://codereview.chromium.org/1227363002/#ps180001 (title: "Remove BackgroundSync.Registration.Periodic.CouldFire")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1227363002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) (exceeded global retry quota)
The CQ bit was checked by iclelland@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jkarlin@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/1227363002/#ps220001 (title: "Fix bad rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1227363002/220001
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/a84c0e6e9875a605f5ef9a1f78f59c4cadb43836 Cr-Commit-Position: refs/heads/master@{#339072} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
