Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
136403007:
Implement system tracing for cros (Closed)
|
Created:
6 years, 10 months ago by haraken Modified:
6 years, 10 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, dsinclair+watch_chromium.org, jam Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionImplement system tracing for cros
When we migrated from trace_controller to tracing_controller, we broke system tracing in cros. This CL fixes the system tracing.
The old implementation of the system tracing is this one: https://codereview.chromium.org/23556003/diff/6001/content/browser/tracing/tracing_ui.cc?context=&column_width=80
BUG=334017
TBR=dsinclair
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=249822
Patch Set 1 #
Total comments: 12
Patch Set 2 : #Patch Set 3 : #Patch Set 4 : fix json framing #
Messages
Total messages: 20 (0 generated)
https://codereview.chromium.org/136403007/diff/1/content/browser/tracing/trac... File content/browser/tracing/tracing_controller_impl.cc (right): https://codereview.chromium.org/136403007/diff/1/content/browser/tracing/trac... content/browser/tracing/tracing_controller_impl.cc:674: TracingUI* tracing_ui = 0; This is a problem. Since TracingController does not know which TracingUI called TracingController::DisableRecording, TracingController cannot dispatch OnSystemTraceDataCollected to a proper TracingUI. Since TracingController::DisableRecording is called by static methods of TracingUI, it is not trivial to teach TracingController which TracingUI called TracingController::DisableRecording. (Note: TracingController knows a list of existing TracingUIs. They are stored in TracingControllerImpl::tracing_uis_.) How can we address this issue?
https://codereview.chromium.org/136403007/diff/1/content/browser/tracing/trac... File content/browser/tracing/tracing_controller_impl.cc (right): https://codereview.chromium.org/136403007/diff/1/content/browser/tracing/trac... content/browser/tracing/tracing_controller_impl.cc:203: if (options & ENABLE_SYSTRACE) { Is SYSTRACE only for cros, or will this if () also be used for Android? If just CrOS, can we put the #define outside the if () so the branch gets pre-processed out? https://codereview.chromium.org/136403007/diff/1/content/browser/tracing/trac... content/browser/tracing/tracing_controller_impl.cc:207: StartSystemTracing(); nit: indenting. https://codereview.chromium.org/136403007/diff/1/content/browser/tracing/trac... content/browser/tracing/tracing_controller_impl.cc:650: is_system_tracing_ = false; This can only be set true inside a #if defined(OS_CHROMEOS) so can we move this whole block inside the #if define below (including the outer if (is_system_tracing_)? https://codereview.chromium.org/136403007/diff/1/content/browser/tracing/trac... content/browser/tracing/tracing_controller_impl.cc:653: RequestStopSystemTracing( nit: indenting https://codereview.chromium.org/136403007/diff/1/content/browser/tracing/trac... content/browser/tracing/tracing_controller_impl.cc:674: TracingUI* tracing_ui = 0; On 2014/01/30 13:39:46, haraken wrote: > > This is a problem. Since TracingController does not know which TracingUI called > TracingController::DisableRecording, TracingController cannot dispatch > OnSystemTraceDataCollected to a proper TracingUI. > > Since TracingController::DisableRecording is called by static methods of > TracingUI, it is not trivial to teach TracingController which TracingUI called > TracingController::DisableRecording. > > (Note: TracingController knows a list of existing TracingUIs. They are stored in > TracingControllerImpl::tracing_uis_.) > > How can we address this issue? So, does tracing_ui matter in this case? I though we wanted to dump the system trace data into the file the same as we're dumping in regular trace data? I guess the problem we have is that the JSON objects in the file are already complete and we have no way to inject the extra systrace data into them correct? https://codereview.chromium.org/136403007/diff/1/content/browser/tracing/trac... File content/browser/tracing/tracing_ui.cc (right): https://codereview.chromium.org/136403007/diff/1/content/browser/tracing/trac... content/browser/tracing/tracing_ui.cc:293: web_ui()->CallJavascriptFunction( Don't think we want to send this back as JS, but instead in the JSON file.
https://codereview.chromium.org/136403007/diff/1/content/browser/tracing/trac... File content/browser/tracing/tracing_controller_impl.cc (right): https://codereview.chromium.org/136403007/diff/1/content/browser/tracing/trac... content/browser/tracing/tracing_controller_impl.cc:674: TracingUI* tracing_ui = 0; On 2014/01/30 14:36:06, dsinclair wrote: > On 2014/01/30 13:39:46, haraken wrote: > > > > This is a problem. Since TracingController does not know which TracingUI > called > > TracingController::DisableRecording, TracingController cannot dispatch > > OnSystemTraceDataCollected to a proper TracingUI. > > > > Since TracingController::DisableRecording is called by static methods of > > TracingUI, it is not trivial to teach TracingController which TracingUI called > > TracingController::DisableRecording. > > > > (Note: TracingController knows a list of existing TracingUIs. They are stored > in > > TracingControllerImpl::tracing_uis_.) > > > > How can we address this issue? > > > So, does tracing_ui matter in this case? I though we wanted to dump the system > trace data into the file the same as we're dumping in regular trace data? I > guess the problem we have is that the JSON objects in the file are already > complete and we have no way to inject the extra systrace data into them correct? I'm not familiar with how the system tracing was working, so I want to understand :) If we can just append system tracing data to the same JSON file, I think I can implement that way. I thought we need to call "tracingController.onSystemTraceDataCollected" since the previous tracing_ui.cc was doing that (Line 553 of https://codereview.chromium.org/23556003/diff/6001/content/browser/tracing/tr...).
On 2014/01/30 15:25:02, haraken wrote: > https://codereview.chromium.org/136403007/diff/1/content/browser/tracing/trac... > File content/browser/tracing/tracing_controller_impl.cc (right): > > https://codereview.chromium.org/136403007/diff/1/content/browser/tracing/trac... > content/browser/tracing/tracing_controller_impl.cc:674: TracingUI* tracing_ui = > 0; > On 2014/01/30 14:36:06, dsinclair wrote: > > On 2014/01/30 13:39:46, haraken wrote: > > > > > > This is a problem. Since TracingController does not know which TracingUI > > called > > > TracingController::DisableRecording, TracingController cannot dispatch > > > OnSystemTraceDataCollected to a proper TracingUI. > > > > > > Since TracingController::DisableRecording is called by static methods of > > > TracingUI, it is not trivial to teach TracingController which TracingUI > called > > > TracingController::DisableRecording. > > > > > > (Note: TracingController knows a list of existing TracingUIs. They are > stored > > in > > > TracingControllerImpl::tracing_uis_.) > > > > > > How can we address this issue? > > > > > > So, does tracing_ui matter in this case? I though we wanted to dump the system > > trace data into the file the same as we're dumping in regular trace data? I > > guess the problem we have is that the JSON objects in the file are already > > complete and we have no way to inject the extra systrace data into them > correct? > > I'm not familiar with how the system tracing was working, so I want to > understand :) > > If we can just append system tracing data to the same JSON file, I think I can > implement that way. > > I thought we need to call "tracingController.onSystemTraceDataCollected" since > the previous tracing_ui.cc was doing that (Line 553 of > https://codereview.chromium.org/23556003/diff/6001/content/browser/tracing/tr...). It's quite possible I'm wrong on this, heh. So, the old code patch sent the data through JS because that was the only way we had to do it (the same as regular tracing dumped the data through JS). With this new file based path, I had originally though we were going to add the system trace data into the file? I'm just not exactly sure how that would work. (Although, maybe I'm getting the format wrong? Looking at the trace-viewer/test_data/chromeos_system_trace.json it looks like the trace events from CrOS are just added as more entries in the traceEvents array?)
> It's quite possible I'm wrong on this, heh. So, the old code patch sent the data > through JS because that was the only way we had to do it (the same as regular > tracing dumped the data through JS). > > With this new file based path, I had originally though we were going to add the > system trace data into the file? I'm just not exactly sure how that would work. > (Although, maybe I'm getting the format wrong? Looking at the > trace-viewer/test_data/chromeos_system_trace.json it looks like the trace events > from CrOS are just added as more entries in the traceEvents array?) That sounds reasonable to me :) Let me try that. Davemoore: I'm happy if you could correct us if our observation about the system tracing is wrong.
On 2014/01/30 15:48:55, haraken wrote: > > It's quite possible I'm wrong on this, heh. So, the old code patch sent the > data > > through JS because that was the only way we had to do it (the same as regular > > tracing dumped the data through JS). > > > > With this new file based path, I had originally though we were going to add > the > > system trace data into the file? I'm just not exactly sure how that would > work. > > (Although, maybe I'm getting the format wrong? Looking at the > > trace-viewer/test_data/chromeos_system_trace.json it looks like the trace > events > > from CrOS are just added as more entries in the traceEvents array?) > > That sounds reasonable to me :) Let me try that. > > Davemoore: I'm happy if you could correct us if our observation about the system > tracing is wrong. <Dave swaps>: I think that was what we intended to happen once the rest of the changes were made. But there were some differences in the way the system trace events were collected that made it hard with the old model. Perhaps that's easier now. If you run into problems, please contact me and we'll figure something out.
Thanks for review! PTAL. Also I don't have a development environment of Chromium OS, so I would be happy if someone could verify that the system tracing works as expected. https://codereview.chromium.org/136403007/diff/1/content/browser/tracing/trac... File content/browser/tracing/tracing_controller_impl.cc (right): https://codereview.chromium.org/136403007/diff/1/content/browser/tracing/trac... content/browser/tracing/tracing_controller_impl.cc:203: if (options & ENABLE_SYSTRACE) { On 2014/01/30 14:36:06, dsinclair wrote: > Is SYSTRACE only for cros, or will this if () also be used for Android? If just > CrOS, can we put the #define outside the if () so the branch gets pre-processed > out? Done. https://codereview.chromium.org/136403007/diff/1/content/browser/tracing/trac... content/browser/tracing/tracing_controller_impl.cc:207: StartSystemTracing(); On 2014/01/30 14:36:06, dsinclair wrote: > nit: indenting. I'm not sure but this is in the middle of an expression (not parameters of a function), so it should be 2-space indented. The old code also used 2-space indentation. https://codereview.chromium.org/136403007/diff/1/content/browser/tracing/trac... content/browser/tracing/tracing_controller_impl.cc:650: is_system_tracing_ = false; On 2014/01/30 14:36:06, dsinclair wrote: > This can only be set true inside a #if defined(OS_CHROMEOS) so can we move this > whole block inside the #if define below (including the outer if > (is_system_tracing_)? Done. https://codereview.chromium.org/136403007/diff/1/content/browser/tracing/trac... content/browser/tracing/tracing_controller_impl.cc:674: TracingUI* tracing_ui = 0; On 2014/01/30 15:25:02, haraken wrote: > On 2014/01/30 14:36:06, dsinclair wrote: > > On 2014/01/30 13:39:46, haraken wrote: > > > > > > This is a problem. Since TracingController does not know which TracingUI > > called > > > TracingController::DisableRecording, TracingController cannot dispatch > > > OnSystemTraceDataCollected to a proper TracingUI. > > > > > > Since TracingController::DisableRecording is called by static methods of > > > TracingUI, it is not trivial to teach TracingController which TracingUI > called > > > TracingController::DisableRecording. > > > > > > (Note: TracingController knows a list of existing TracingUIs. They are > stored > > in > > > TracingControllerImpl::tracing_uis_.) > > > > > > How can we address this issue? > > > > > > So, does tracing_ui matter in this case? I though we wanted to dump the system > > trace data into the file the same as we're dumping in regular trace data? I > > guess the problem we have is that the JSON objects in the file are already > > complete and we have no way to inject the extra systrace data into them > correct? > > I'm not familiar with how the system tracing was working, so I want to > understand :) > > If we can just append system tracing data to the same JSON file, I think I can > implement that way. > > I thought we need to call "tracingController.onSystemTraceDataCollected" since > the previous tracing_ui.cc was doing that (Line 553 of > https://codereview.chromium.org/23556003/diff/6001/content/browser/tracing/tr...). Done. I changed the code so that the system tracing data is appended to |result_file_|.
On 2014/01/31 05:58:26, haraken wrote: > Thanks for review! > > PTAL. > > Also I don't have a development environment of Chromium OS, so I would be happy > if someone could verify that the system tracing works as expected. > > https://codereview.chromium.org/136403007/diff/1/content/browser/tracing/trac... > File content/browser/tracing/tracing_controller_impl.cc (right): > > https://codereview.chromium.org/136403007/diff/1/content/browser/tracing/trac... > content/browser/tracing/tracing_controller_impl.cc:203: if (options & > ENABLE_SYSTRACE) { > On 2014/01/30 14:36:06, dsinclair wrote: > > Is SYSTRACE only for cros, or will this if () also be used for Android? If > just > > CrOS, can we put the #define outside the if () so the branch gets > pre-processed > > out? > > Done. > > https://codereview.chromium.org/136403007/diff/1/content/browser/tracing/trac... > content/browser/tracing/tracing_controller_impl.cc:207: StartSystemTracing(); > On 2014/01/30 14:36:06, dsinclair wrote: > > nit: indenting. > > I'm not sure but this is in the middle of an expression (not parameters of a > function), so it should be 2-space indented. The old code also used 2-space > indentation. > > https://codereview.chromium.org/136403007/diff/1/content/browser/tracing/trac... > content/browser/tracing/tracing_controller_impl.cc:650: is_system_tracing_ = > false; > On 2014/01/30 14:36:06, dsinclair wrote: > > This can only be set true inside a #if defined(OS_CHROMEOS) so can we move > this > > whole block inside the #if define below (including the outer if > > (is_system_tracing_)? > > Done. > > https://codereview.chromium.org/136403007/diff/1/content/browser/tracing/trac... > content/browser/tracing/tracing_controller_impl.cc:674: TracingUI* tracing_ui = > 0; > On 2014/01/30 15:25:02, haraken wrote: > > On 2014/01/30 14:36:06, dsinclair wrote: > > > On 2014/01/30 13:39:46, haraken wrote: > > > > > > > > This is a problem. Since TracingController does not know which TracingUI > > > called > > > > TracingController::DisableRecording, TracingController cannot dispatch > > > > OnSystemTraceDataCollected to a proper TracingUI. > > > > > > > > Since TracingController::DisableRecording is called by static methods of > > > > TracingUI, it is not trivial to teach TracingController which TracingUI > > called > > > > TracingController::DisableRecording. > > > > > > > > (Note: TracingController knows a list of existing TracingUIs. They are > > stored > > > in > > > > TracingControllerImpl::tracing_uis_.) > > > > > > > > How can we address this issue? > > > > > > > > > So, does tracing_ui matter in this case? I though we wanted to dump the > system > > > trace data into the file the same as we're dumping in regular trace data? I > > > guess the problem we have is that the JSON objects in the file are already > > > complete and we have no way to inject the extra systrace data into them > > correct? > > > > I'm not familiar with how the system tracing was working, so I want to > > understand :) > > > > If we can just append system tracing data to the same JSON file, I think I can > > implement that way. > > > > I thought we need to call "tracingController.onSystemTraceDataCollected" since > > the previous tracing_ui.cc was doing that (Line 553 of > > > https://codereview.chromium.org/23556003/diff/6001/content/browser/tracing/tr...). > > Done. I changed the code so that the system tracing data is appended to > |result_file_|. I've tweaked your change to frame the system trace correctly in json. It has to be escaped as a json string and put in the "systemTraceEvents" property. I've tested this and it's working.
> I've tweaked your change to frame the system trace correctly in json. It has to > be escaped as a json string and put in the "systemTraceEvents" property. > > I've tested this and it's working. Thanks!! nduca, dsinclair: Would you take a look at the CL from spang?
lgtm
The CQ bit was checked by haraken@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/haraken@chromium.org/136403007/70001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
On 2014/02/06 23:19:47, I haz the power (commit-bot) wrote: > Retried try job too often on chromium_presubmit for step(s) presubmit > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p... dsinclair: would you LG from your chromium account? :)
The CQ bit was checked by haraken@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/haraken@chromium.org/136403007/70001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/haraken@chromium.org/136403007/70001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/haraken@chromium.org/136403007/70001
Message was sent while issue was closed.
Change committed as 249822 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
