Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10451076:
Adding UMA for sync IPCs (Closed)
|
Created:
8 years, 6 months ago by kinuko Modified:
8 years, 6 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jochen+watch-content_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdding UMA for sync IPCs
BUG=none
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=140017
Patch Set 1 #
Total comments: 1
Patch Set 2 : #
Total comments: 2
Patch Set 3 : #Patch Set 4 : histogram #
Total comments: 3
Patch Set 5 : #Patch Set 6 : #
Messages
Total messages: 19 (0 generated)
http://codereview.chromium.org/10451076/diff/1/content/worker/worker_webkitpl... File content/worker/worker_webkitplatformsupport_impl.cc (right): http://codereview.chromium.org/10451076/diff/1/content/worker/worker_webkitpl... content/worker/worker_webkitplatformsupport_impl.cc:17: #include "content/public/browser/user_metrics.h" you can't include browser headers in worker or renderer. Instead, you need to send a ViewHostMsg_UserMetrics_RecordAction IPC to the browser. For the worker, you'll need to add a WorkerHostMsg Alternatively, and maybe better, can you record the metric in the browser when you receive a sync ipc?
On 2012/05/30 08:28:09, jochen wrote: > http://codereview.chromium.org/10451076/diff/1/content/worker/worker_webkitpl... > File content/worker/worker_webkitplatformsupport_impl.cc (right): > > http://codereview.chromium.org/10451076/diff/1/content/worker/worker_webkitpl... > content/worker/worker_webkitplatformsupport_impl.cc:17: #include > "content/public/browser/user_metrics.h" > you can't include browser headers in worker or renderer. Aha, I should have guessed it. > Instead, you need to send a ViewHostMsg_UserMetrics_RecordAction IPC to the > browser. For the worker, you'll need to add a WorkerHostMsg > > Alternatively, and maybe better, can you record the metric in the browser when > you receive a sync ipc? Sounds good, I'm going to update.
Updated the patch. PTAL thanks!
It looks like the browsertest breakage might be related to your CL? http://codereview.chromium.org/10451076/diff/2003/content/browser/renderer_ho... File content/browser/renderer_host/file_utilities_message_filter.cc (right): http://codereview.chromium.org/10451076/diff/2003/content/browser/renderer_ho... content/browser/renderer_host/file_utilities_message_filter.cc:40: content::RecordAction(content::UserMetricsAction("FileSyncIPC_Received")); what about if (message.is_sync()) so this is less likely to break if somebody adds an async message in the future?
http://codereview.chromium.org/10451076/diff/2003/content/browser/renderer_ho... File content/browser/renderer_host/file_utilities_message_filter.cc (right): http://codereview.chromium.org/10451076/diff/2003/content/browser/renderer_ho... content/browser/renderer_host/file_utilities_message_filter.cc:40: content::RecordAction(content::UserMetricsAction("FileSyncIPC_Received")); On 2012/05/30 13:47:51, jochen wrote: > what about if (message.is_sync()) so this is less likely to break if somebody > adds an async message in the future? Done. Also added if (handled) so that we are only incrementing if it's for file utility. Browser tests on my local box looks ok now.
unofficial content lg
lgtm
On 2012/05/30 15:19:18, John Abd-El-Malek wrote: > lgtm although, IMO it would be more useful to log this in the renderer, and to histogram the time it takes for each sync IPC
On 2012/05/30 15:19:52, John Abd-El-Malek wrote: > On 2012/05/30 15:19:18, John Abd-El-Malek wrote: > > lgtm > > although, IMO it would be more useful to log this in the renderer, and to > histogram the time it takes for each sync IPC Hmm that sounds nice. I added the code for recording the histogram. Can you take another look?
http://codereview.chromium.org/10451076/diff/3004/content/browser/renderer_ho... File content/browser/renderer_host/file_utilities_message_filter.cc (right): http://codereview.chromium.org/10451076/diff/3004/content/browser/renderer_ho... content/browser/renderer_host/file_utilities_message_filter.cc:38: content::RecordAction(content::UserMetricsAction("FileSyncIPC_Handled")); don't need to change this file now http://codereview.chromium.org/10451076/diff/3004/content/renderer/renderer_w... File content/renderer/renderer_webkitplatformsupport_impl.cc (right): http://codereview.chromium.org/10451076/diff/3004/content/renderer/renderer_w... content/renderer/renderer_webkitplatformsupport_impl.cc:191: ChildThread::current()->Send(new ViewHostMsg_SyncIPCElapsedTime(delta)); you can histogram from the renderer, just call HISTOGRAM_TIMES here
http://codereview.chromium.org/10451076/diff/3004/content/renderer/renderer_w... File content/renderer/renderer_webkitplatformsupport_impl.cc (right): http://codereview.chromium.org/10451076/diff/3004/content/renderer/renderer_w... content/renderer/renderer_webkitplatformsupport_impl.cc:191: ChildThread::current()->Send(new ViewHostMsg_SyncIPCElapsedTime(delta)); On 2012/05/31 15:20:16, John Abd-El-Malek wrote: > you can histogram from the renderer, just call HISTOGRAM_TIMES here oooops... (ashamed) done.
On 2012/05/31 17:10:35, kinuko wrote: > http://codereview.chromium.org/10451076/diff/3004/content/renderer/renderer_w... > File content/renderer/renderer_webkitplatformsupport_impl.cc (right): > > http://codereview.chromium.org/10451076/diff/3004/content/renderer/renderer_w... > content/renderer/renderer_webkitplatformsupport_impl.cc:191: > ChildThread::current()->Send(new ViewHostMsg_SyncIPCElapsedTime(delta)); > On 2012/05/31 15:20:16, John Abd-El-Malek wrote: > > you can histogram from the renderer, just call HISTOGRAM_TIMES here > > oooops... (ashamed) done. Jam, can you take another look?
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kinuko@chromium.org/10451076/6005
Try job failure for 10451076-6005 (retry) on win for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kinuko@chromium.org/10451076/9005
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kinuko@chromium.org/10451076/3012
Change committed as 140017 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
