Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1776013005:
[DO NOT COMMIT] Refactor systrace to support new clock sync design (Closed)
|
Created:
4 years, 9 months ago by alexandermont Modified:
4 years, 8 months ago CC:
catapult-reviews_chromium.org Base URL:
git@github.com:catapult-project/catapult@master Target Ref:
refs/heads/master Project:
catapult Visibility:
Public. |
DescriptionRefactor systrace to support new clock sync design.
Do not commit this CL. This CL will be broken into several smaller CLs. I am keeping the CL visible so that I can use these changes as a reference when creating the smaller CLs.
Patch Set 1 #Patch Set 2 : split up files and fix imports #Patch Set 3 : use callbacks for clock sync #
Total comments: 30
Patch Set 4 : changes from code review #
Total comments: 28
Patch Set 5 : more changes from code review #Patch Set 6 : use json #Patch Set 7 : fix similarity #
Total comments: 15
Patch Set 8 : fix html/json output #Patch Set 9 : change to stop with enter instead of control-C #Patch Set 10 : set the pythonpath in init #Patch Set 11 : add controllerTraceDataKey #
Total comments: 4
Patch Set 12 : more changes from code reviews #
Total comments: 12
Patch Set 13 : fix some bugs and changes from code review #
Total comments: 14
Patch Set 14 : add timeouts and fix tests #
Total comments: 29
Patch Set 15 : changes from code review #
Total comments: 17
Patch Set 16 : update documentation and python paths #
Total comments: 9
Patch Set 17 : more changes from code review #
Total comments: 1
Patch Set 18 : fix categories issue #Messages
Total messages: 48 (9 generated)
alexandermont@chromium.org changed reviewers: + ccraik@google.com, jbudorick@chromium.org, nednguyen@google.com
This change is based on the code review comments in the other CL (now closed). systrace/systrace/systrace.py:42: class TracingAgent(object): Put the interface to a separate file and document the interface. Or use a similar way as Telemetry: Done. systrace/systrace/systrace.py:61: class TracingController(TracingAgent): Put the TracingController to a separate file. Done. systrace/systrace/systrace.py:131: self.StopAgentTracing() This should be done after looping all agents. Done. Put SystraceTracingController to a separate file. Done. After the refactor, I [zhenw@] think systrace.py should just contain logics to: 1. handle input arguments; 2. call SystraceTracingController to run tracing and get the trace data. Done.
alexandermont@chromium.org changed reviewers: + zhenw@chromium.org
zhenw@chromium.org changed reviewers: + charliea@chromium.org
Thanks for doing this, Alex! charliea, please take a look at the comment at systrace/systrace/systrace_tracing_controller.py:60 https://codereview.chromium.org/1776013005/diff/40001/systrace/systrace/agent... File systrace/systrace/agents/atrace_agent.py (right): https://codereview.chromium.org/1776013005/diff/40001/systrace/systrace/agent... systrace/systrace/agents/atrace_agent.py:107: trace_data = self._collect_trace_data() StopAgentTracing needs to return immediately and does not wait for the data. I double this is actually a blocking call, which should be changed. Also see comments in _dump_trace(). https://codereview.chromium.org/1776013005/diff/40001/systrace/systrace/agent... systrace/systrace/agents/atrace_agent.py:189: return self._device_utils.RunShellCommand(dump_cmd, split_lines=False) How long will this take? Is this a blocking call? https://codereview.chromium.org/1776013005/diff/40001/systrace/systrace/run_s... File systrace/systrace/run_systrace.py (right): https://codereview.chromium.org/1776013005/diff/40001/systrace/systrace/run_s... systrace/systrace/run_systrace.py:142: controller.OutputSystraceResults() Call stop tracing here before OutputSystraceResults() https://codereview.chromium.org/1776013005/diff/40001/systrace/systrace/systr... File systrace/systrace/systrace_tracing_controller.py (right): https://codereview.chromium.org/1776013005/diff/40001/systrace/systrace/systr... systrace/systrace/systrace_tracing_controller.py:60: def OutputSystraceResults(self): This can take results from StopTracing(). Then you can 1. either output the HTML file as usual, 2. or output a JSON formatted trace file containing the data only (need to add a flag for this). I think you will need JSON format version to parse BattOr data in trace viewer. Double check with charliea. https://codereview.chromium.org/1776013005/diff/40001/systrace/systrace/systr... systrace/systrace/systrace_tracing_controller.py:63: results = self.StopTracing() This should be decoupled from OutputSystraceResults. Call StopTracing() in run_systrace.py. https://codereview.chromium.org/1776013005/diff/40001/systrace/systrace/traci... File systrace/systrace/tracing_agent.py (right): https://codereview.chromium.org/1776013005/diff/40001/systrace/systrace/traci... systrace/systrace/tracing_agent.py:9: This class represents an agent that captures traces from a particular nit: Please fill the blank space at the end of this line. There are also many other similar occasions in this CL. Please also fix those. https://codereview.chromium.org/1776013005/diff/40001/systrace/systrace/traci... systrace/systrace/tracing_agent.py:23: def StartAgentTracing(self): StartAgentTracing() should return whether it succeed or not. Based on the return value, the tracing controller can decide what to do based on this information, e.g., maybe tracing controller can directly raise errors. By the way, you also need to document the arguments and return values for all the APIs. https://codereview.chromium.org/1776013005/diff/40001/systrace/systrace/traci... systrace/systrace/tracing_agent.py:29: def StopAgentTracing(self): Need to mention this will return immediately and does not wait for the data. https://codereview.chromium.org/1776013005/diff/40001/systrace/systrace/traci... systrace/systrace/tracing_agent.py:47: def GetResults(self): Need to mention this can be a blocking call and can wait for the data. https://codereview.chromium.org/1776013005/diff/40001/systrace/systrace/traci... File systrace/systrace/tracing_controller.py (right): https://codereview.chromium.org/1776013005/diff/40001/systrace/systrace/traci... systrace/systrace/tracing_controller.py:42: self._trace_in_progress = True This should be set in StartTracing(). https://codereview.chromium.org/1776013005/diff/40001/systrace/systrace/traci... systrace/systrace/tracing_controller.py:43: clock_sync_file = tempfile.NamedTemporaryFile(delete=False) s/clock_sync_file/controller_log_file https://codereview.chromium.org/1776013005/diff/40001/systrace/systrace/traci... systrace/systrace/tracing_controller.py:56: self._trace_in_progress = False This should be set in StopTracing(). https://codereview.chromium.org/1776013005/diff/40001/systrace/systrace/traci... systrace/systrace/tracing_controller.py:69: return tracing_agent.TraceResults('trace-data', result, True) I think it is better to use specific class names here. Each tracing agent should use its own class name. In OutputSystraceResults(), if it is outputting HTML file, you can then use 'trace-data' for all agents. But if you want to output JSON format, you will need different class names for different agent. Also see comments in OutputSystraceResults(). For the class names, see https://docs.google.com/document/d/1CvAClvFfyA5R-PhYUmn5OOQtYMH4h6I0nSsKchNAy...: - atrace: systemTraceEvents - ftrace: systemTraceEvents - battor: powerTraceAsString (not battorLogAsString, it will be changed) - controller: traceEvents And I think class_name is not easy to understand. Maybe rename it to data_name or something similar. https://codereview.chromium.org/1776013005/diff/40001/systrace/systrace/traci... systrace/systrace/tracing_controller.py:106: self._current_sync_id += 1 Use uuid to generate the IDs.
https://codereview.chromium.org/1776013005/diff/40001/systrace/systrace/agent... File systrace/systrace/agents/atrace_agent.py (right): https://codereview.chromium.org/1776013005/diff/40001/systrace/systrace/agent... systrace/systrace/agents/atrace_agent.py:107: trace_data = self._collect_trace_data() On 2016/03/11 at 17:27:17, Zhen Wang wrote: > StopAgentTracing needs to return immediately and does not wait for the data. I double this is actually a blocking call, which should be changed. Also see comments in _dump_trace(). See comment below. https://codereview.chromium.org/1776013005/diff/40001/systrace/systrace/agent... systrace/systrace/agents/atrace_agent.py:189: return self._device_utils.RunShellCommand(dump_cmd, split_lines=False) On 2016/03/11 at 17:27:17, Zhen Wang wrote: > How long will this take? Is this a blocking call? It looks like running the dump command takes about a second, even if there's nothing in the trace buffer to dump. Yes, this is blocking. I changed this to use a separate process. https://codereview.chromium.org/1776013005/diff/40001/systrace/systrace/run_s... File systrace/systrace/run_systrace.py (right): https://codereview.chromium.org/1776013005/diff/40001/systrace/systrace/run_s... systrace/systrace/run_systrace.py:142: controller.OutputSystraceResults() On 2016/03/11 at 17:27:17, Zhen Wang wrote: > Call stop tracing here before OutputSystraceResults() Done. Note that this required modifying StopTracing() to store the results in the TracingController. https://codereview.chromium.org/1776013005/diff/40001/systrace/systrace/systr... File systrace/systrace/systrace_tracing_controller.py (right): https://codereview.chromium.org/1776013005/diff/40001/systrace/systrace/systr... systrace/systrace/systrace_tracing_controller.py:60: def OutputSystraceResults(self): On 2016/03/11 at 17:27:17, Zhen Wang wrote: > This can take results from StopTracing(). Then you can > 1. either output the HTML file as usual, > 2. or output a JSON formatted trace file containing the data only (need to add a flag for this). > > I think you will need JSON format version to parse BattOr data in trace viewer. Double check with charliea. Option added, but not yet implemented. https://codereview.chromium.org/1776013005/diff/40001/systrace/systrace/systr... systrace/systrace/systrace_tracing_controller.py:63: results = self.StopTracing() On 2016/03/11 at 17:27:17, Zhen Wang wrote: > This should be decoupled from OutputSystraceResults. Call StopTracing() in run_systrace.py. Done. https://codereview.chromium.org/1776013005/diff/40001/systrace/systrace/traci... File systrace/systrace/tracing_agent.py (right): https://codereview.chromium.org/1776013005/diff/40001/systrace/systrace/traci... systrace/systrace/tracing_agent.py:9: This class represents an agent that captures traces from a particular On 2016/03/11 at 17:27:17, Zhen Wang wrote: > nit: Please fill the blank space at the end of this line. There are also many other similar occasions in this CL. Please also fix those. What do you mean by "blank space at the end of the line"? Do you just mean that I am line-wrapping without using the full 80 characters? https://codereview.chromium.org/1776013005/diff/40001/systrace/systrace/traci... systrace/systrace/tracing_agent.py:23: def StartAgentTracing(self): On 2016/03/11 at 17:27:18, Zhen Wang wrote: > StartAgentTracing() should return whether it succeed or not. Based on the return value, the tracing controller can decide what to do based on this information, e.g., maybe tracing controller can directly raise errors. > > By the way, you also need to document the arguments and return values for all the APIs. Done https://codereview.chromium.org/1776013005/diff/40001/systrace/systrace/traci... systrace/systrace/tracing_agent.py:29: def StopAgentTracing(self): On 2016/03/11 at 17:27:18, Zhen Wang wrote: > Need to mention this will return immediately and does not wait for the data. Done https://codereview.chromium.org/1776013005/diff/40001/systrace/systrace/traci... systrace/systrace/tracing_agent.py:47: def GetResults(self): On 2016/03/11 at 17:27:18, Zhen Wang wrote: > Need to mention this can be a blocking call and can wait for the data. Done https://codereview.chromium.org/1776013005/diff/40001/systrace/systrace/traci... systrace/systrace/tracing_agent.py:47: def GetResults(self): On 2016/03/11 at 17:27:18, Zhen Wang wrote: > Need to mention this can be a blocking call and can wait for the data. Done https://codereview.chromium.org/1776013005/diff/40001/systrace/systrace/traci... File systrace/systrace/tracing_controller.py (right): https://codereview.chromium.org/1776013005/diff/40001/systrace/systrace/traci... systrace/systrace/tracing_controller.py:42: self._trace_in_progress = True On 2016/03/11 at 17:27:18, Zhen Wang wrote: > This should be set in StartTracing(). Done https://codereview.chromium.org/1776013005/diff/40001/systrace/systrace/traci... systrace/systrace/tracing_controller.py:43: clock_sync_file = tempfile.NamedTemporaryFile(delete=False) On 2016/03/11 at 17:27:18, Zhen Wang wrote: > s/clock_sync_file/controller_log_file Done https://codereview.chromium.org/1776013005/diff/40001/systrace/systrace/traci... systrace/systrace/tracing_controller.py:56: self._trace_in_progress = False On 2016/03/11 at 17:27:18, Zhen Wang wrote: > This should be set in StopTracing(). Done https://codereview.chromium.org/1776013005/diff/40001/systrace/systrace/traci... systrace/systrace/tracing_controller.py:69: return tracing_agent.TraceResults('trace-data', result, True) On 2016/03/11 at 17:27:18, Zhen Wang wrote: > I think it is better to use specific class names here. Each tracing agent should use its own class name. In OutputSystraceResults(), if it is outputting HTML file, you can then use 'trace-data' for all agents. But if you want to output JSON format, you will need different class names for different agent. Also see comments in OutputSystraceResults(). > > For the class names, see https://docs.google.com/document/d/1CvAClvFfyA5R-PhYUmn5OOQtYMH4h6I0nSsKchNAy...: > - atrace: systemTraceEvents > - ftrace: systemTraceEvents > - battor: powerTraceAsString (not battorLogAsString, it will be changed) > - controller: traceEvents > > And I think class_name is not easy to understand. Maybe rename it to data_name or something similar. Done https://codereview.chromium.org/1776013005/diff/40001/systrace/systrace/traci... systrace/systrace/tracing_controller.py:106: self._current_sync_id += 1 On 2016/03/11 at 17:27:18, Zhen Wang wrote: > Use uuid to generate the IDs. Done
https://codereview.chromium.org/1776013005/diff/60001/systrace/systrace/agent... File systrace/systrace/agents/atrace_agent.py (right): https://codereview.chromium.org/1776013005/diff/60001/systrace/systrace/agent... systrace/systrace/agents/atrace_agent.py:218: self._stop_trace() does stopping make sense if there's no trace? https://codereview.chromium.org/1776013005/diff/60001/systrace/systrace/agent... File systrace/systrace/agents/atrace_agent_unittest.py (right): https://codereview.chromium.org/1776013005/diff/60001/systrace/systrace/agent... systrace/systrace/agents/atrace_agent_unittest.py:29: #STOP_FIX_UPS = ['atrace', '--no-fix-threads', '--no-fix-tgids'] delete? https://codereview.chromium.org/1776013005/diff/60001/systrace/systrace/run_s... File systrace/systrace/run_systrace.py (right): https://codereview.chromium.org/1776013005/diff/60001/systrace/systrace/run_s... systrace/systrace/run_systrace.py:44: parser.add_option('-i', '--interrupt', dest='interrupt', default=False, How would folks feel about making this the default? Then you specify -t/--time to switch to a timer based mode. (suggesting now, since it would mean we wouldn't need this option)
The overall structure looks good. Alex, can you make all agents' SupportsExplicitClockSync() return false for now (and add TODOs)? So we do not depend on the adb persistent shell CL and we can test run this refactor CL by itself. We can support the clock sync functionality in follow-up CLs. https://codereview.chromium.org/1776013005/diff/40001/systrace/systrace/traci... File systrace/systrace/tracing_agent.py (right): https://codereview.chromium.org/1776013005/diff/40001/systrace/systrace/traci... systrace/systrace/tracing_agent.py:9: This class represents an agent that captures traces from a particular On 2016/03/11 23:01:09, alexandermont wrote: > On 2016/03/11 at 17:27:17, Zhen Wang wrote: > > nit: Please fill the blank space at the end of this line. There are also many > other similar occasions in this CL. Please also fix those. > > What do you mean by "blank space at the end of the line"? Do you just mean that > I am line-wrapping without using the full 80 characters? Right. Please use the full 80 characters. https://codereview.chromium.org/1776013005/diff/60001/systrace/systrace/run_s... File systrace/systrace/run_systrace.py (right): https://codereview.chromium.org/1776013005/diff/60001/systrace/systrace/run_s... systrace/systrace/run_systrace.py:1: #!/usr/bin/env python +ccraik, do we need to keep the old file name (systrace.py)? https://codereview.chromium.org/1776013005/diff/60001/systrace/systrace/run_s... systrace/systrace/run_systrace.py:44: parser.add_option('-i', '--interrupt', dest='interrupt', default=False, On 2016/03/12 02:02:34, Chris Craik wrote: > How would folks feel about making this the default? Then you specify -t/--time > to switch to a timer based mode. > > (suggesting now, since it would mean we wouldn't need this option) +1 https://codereview.chromium.org/1776013005/diff/60001/systrace/systrace/systr... File systrace/systrace/systrace_tracing_controller.py (right): https://codereview.chromium.org/1776013005/diff/60001/systrace/systrace/systr... systrace/systrace/systrace_tracing_controller.py:32: self._all_results = None I think you don't want to override self._all_results in parent class (tracing_controller.TracingController)? https://codereview.chromium.org/1776013005/diff/60001/systrace/systrace/traci... File systrace/systrace/tracing_agent.py (right): https://codereview.chromium.org/1776013005/diff/60001/systrace/systrace/traci... systrace/systrace/tracing_agent.py:1: #!/usr/bin/env python 1. Rename agents/ to tracing_agents/ 2. Move this file to tracing_agents/__init__.py https://codereview.chromium.org/1776013005/diff/60001/systrace/systrace/traci... systrace/systrace/tracing_agent.py:43: ''' nit: Add a sentence to explain what the function do here. https://codereview.chromium.org/1776013005/diff/60001/systrace/systrace/traci... File systrace/systrace/tracing_controller.py (right): https://codereview.chromium.org/1776013005/diff/60001/systrace/systrace/traci... systrace/systrace/tracing_controller.py:71: return tracing_agent.TraceResults('trace-data', result, True) s/trace-data/systrace This will need some small trace viewer change after Charlie finish the clock sync work there. https://codereview.chromium.org/1776013005/diff/60001/systrace/systrace/traci... systrace/systrace/tracing_controller.py:110: return False Conceptually the controller supports explicit clock sync. I understand that it needs to return false here because it is not an agent controlled by other controllers. Can you add some comments here to explain why this returns false?
https://codereview.chromium.org/1776013005/diff/60001/systrace/systrace/traci... File systrace/systrace/tracing_agent.py (right): https://codereview.chromium.org/1776013005/diff/60001/systrace/systrace/traci... systrace/systrace/tracing_agent.py:50: def RecordClockSyncMarker(self, sync_id, callback): s/callback/did_record_sync_marker_callback https://codereview.chromium.org/1776013005/diff/60001/systrace/systrace/traci... File systrace/systrace/tracing_controller.py (right): https://codereview.chromium.org/1776013005/diff/60001/systrace/systrace/traci... systrace/systrace/tracing_controller.py:18: TRACE_ERR = 'Cannot enable trace_event; ensure catapult_base is in PYTHONPATH' It's strange that you put this as a constant. I would just inline it directly below. https://codereview.chromium.org/1776013005/diff/60001/systrace/systrace/traci... systrace/systrace/tracing_controller.py:106: self._all_results = [agent.GetResults() for agent in self._agents + [self]] this has to be a dictionary that conforms to https://docs.google.com/document/d/1CvAClvFfyA5R-PhYUmn5OOQtYMH4h6I0nSsKchNAy... for trace viewer to consume the data
https://codereview.chromium.org/1776013005/diff/60001/systrace/systrace/traci... File systrace/systrace/tracing_controller.py (right): https://codereview.chromium.org/1776013005/diff/60001/systrace/systrace/traci... systrace/systrace/tracing_controller.py:106: self._all_results = [agent.GetResults() for agent in self._agents + [self]] On 2016/03/14 17:48:11, nednguyen wrote: > this has to be a dictionary that conforms to > https://docs.google.com/document/d/1CvAClvFfyA5R-PhYUmn5OOQtYMH4h6I0nSsKchNAy... > for trace viewer to consume the data GetResults() here returns a tuple. The old systrace way uses it as a list: SystraceTracingController._WriteTraceHTML(). The new way (for clock sync) will need to use it as a dict: SystraceTracingController._WriteTraceJSON(). Though maybe we should create a TraceResult class, so it will be clearer.
Description was changed from ========== refactor systrace to support new clock sync design NOTE: This CL depends on https://codereview.chromium.org/1748173002 (persistent ADB shell) ========== to ========== refactor systrace to support new clock sync design ==========
Changes made from the code review. Note that the change to device_utils (to add the split_lines parameter) is required for the new method of reading from the ADB shell to work. Thus that change has been moved from the persistent shell CL to this CL. (that change is just to device_utils, all it does is add that parameter) https://codereview.chromium.org/1776013005/diff/60001/systrace/systrace/agent... File systrace/systrace/agents/atrace_agent.py (right): https://codereview.chromium.org/1776013005/diff/60001/systrace/systrace/agent... systrace/systrace/agents/atrace_agent.py:218: self._stop_trace() On 2016/03/12 at 02:02:34, Chris Craik wrote: > does stopping make sense if there's no trace? Fixed https://codereview.chromium.org/1776013005/diff/60001/systrace/systrace/agent... File systrace/systrace/agents/atrace_agent_unittest.py (right): https://codereview.chromium.org/1776013005/diff/60001/systrace/systrace/agent... systrace/systrace/agents/atrace_agent_unittest.py:29: #STOP_FIX_UPS = ['atrace', '--no-fix-threads', '--no-fix-tgids'] On 2016/03/12 at 02:02:34, Chris Craik wrote: > delete? Done https://codereview.chromium.org/1776013005/diff/60001/systrace/systrace/run_s... File systrace/systrace/run_systrace.py (right): https://codereview.chromium.org/1776013005/diff/60001/systrace/systrace/run_s... systrace/systrace/run_systrace.py:1: #!/usr/bin/env python On 2016/03/14 at 17:11:27, Zhen Wang wrote: > +ccraik, do we need to keep the old file name (systrace.py)? FYI, the reason that I changed the name systrace.py to run_systrace.py was because the import seemed to be getting confused with the name "systrace" for the whole module and the same name "systrace" for this file. https://codereview.chromium.org/1776013005/diff/60001/systrace/systrace/run_s... systrace/systrace/run_systrace.py:44: parser.add_option('-i', '--interrupt', dest='interrupt', default=False, On 2016/03/14 at 17:11:27, Zhen Wang wrote: > On 2016/03/12 02:02:34, Chris Craik wrote: > > How would folks feel about making this the default? Then you specify -t/--time > > to switch to a timer based mode. > > > > (suggesting now, since it would mean we wouldn't need this option) > > +1 Done https://codereview.chromium.org/1776013005/diff/60001/systrace/systrace/systr... File systrace/systrace/systrace_tracing_controller.py (right): https://codereview.chromium.org/1776013005/diff/60001/systrace/systrace/systr... systrace/systrace/systrace_tracing_controller.py:32: self._all_results = None On 2016/03/14 at 17:11:27, Zhen Wang wrote: > I think you don't want to override self._all_results in parent class (tracing_controller.TracingController)? Fixed https://codereview.chromium.org/1776013005/diff/60001/systrace/systrace/traci... File systrace/systrace/tracing_agent.py (right): https://codereview.chromium.org/1776013005/diff/60001/systrace/systrace/traci... systrace/systrace/tracing_agent.py:1: #!/usr/bin/env python On 2016/03/14 at 17:11:27, Zhen Wang wrote: > 1. Rename agents/ to tracing_agents/ > 2. Move this file to tracing_agents/__init__.py Done https://codereview.chromium.org/1776013005/diff/60001/systrace/systrace/traci... systrace/systrace/tracing_agent.py:43: ''' On 2016/03/14 at 17:11:27, Zhen Wang wrote: > nit: Add a sentence to explain what the function do here. I'm confused: what's wrong with the current documentation "returns Boolean value indicating whether this agent supports recording of clock sync markers?" https://codereview.chromium.org/1776013005/diff/60001/systrace/systrace/traci... systrace/systrace/tracing_agent.py:50: def RecordClockSyncMarker(self, sync_id, callback): On 2016/03/14 at 17:48:11, nednguyen wrote: > s/callback/did_record_sync_marker_callback Done https://codereview.chromium.org/1776013005/diff/60001/systrace/systrace/traci... File systrace/systrace/tracing_controller.py (right): https://codereview.chromium.org/1776013005/diff/60001/systrace/systrace/traci... systrace/systrace/tracing_controller.py:18: TRACE_ERR = 'Cannot enable trace_event; ensure catapult_base is in PYTHONPATH' On 2016/03/14 at 17:48:11, nednguyen wrote: > It's strange that you put this as a constant. I would just inline it directly below. Done https://codereview.chromium.org/1776013005/diff/60001/systrace/systrace/traci... systrace/systrace/tracing_controller.py:71: return tracing_agent.TraceResults('trace-data', result, True) On 2016/03/14 at 17:11:27, Zhen Wang wrote: > s/trace-data/systrace > > This will need some small trace viewer change after Charlie finish the clock sync work there. Done https://codereview.chromium.org/1776013005/diff/60001/systrace/systrace/traci... systrace/systrace/tracing_controller.py:106: self._all_results = [agent.GetResults() for agent in self._agents + [self]] On 2016/03/14 at 17:58:23, Zhen Wang wrote: > On 2016/03/14 17:48:11, nednguyen wrote: > > this has to be a dictionary that conforms to > > https://docs.google.com/document/d/1CvAClvFfyA5R-PhYUmn5OOQtYMH4h6I0nSsKchNAy... > > for trace viewer to consume the data > > GetResults() here returns a tuple. > > The old systrace way uses it as a list: SystraceTracingController._WriteTraceHTML(). > > The new way (for clock sync) will need to use it as a dict: SystraceTracingController._WriteTraceJSON(). > > Though maybe we should create a TraceResult class, so it will be clearer. Changed it to use a dict. Note that I still have not written _WriteTraceJSON yet. I will need to look at documentation on the format before writing this function. https://codereview.chromium.org/1776013005/diff/60001/systrace/systrace/traci... systrace/systrace/tracing_controller.py:110: return False On 2016/03/14 at 17:11:27, Zhen Wang wrote: > Conceptually the controller supports explicit clock sync. I understand that it needs to return false here because it is not an agent controlled by other controllers. Can you add some comments here to explain why this returns false? Done
Hmm, it seems the code diff is confused by the directory change. For example, atrace_agent.py is now a complete delete + complete new file. Is it possible to maintain just the code diff and ignore the changes due to directory change? Alex, did you use 'git mv' (or something similar)?
On 2016/03/14 at 22:30:31, zhenw wrote: > Hmm, it seems the code diff is confused by the directory change. For example, atrace_agent.py is now a complete delete + complete new file. > > Is it possible to maintain just the code diff and ignore the changes due to directory change? Alex, did you use 'git mv' (or something similar)? I used "git mv ./agents ./tracing_agents"
On 2016/03/14 22:31:53, alexandermont wrote: > On 2016/03/14 at 22:30:31, zhenw wrote: > > Hmm, it seems the code diff is confused by the directory change. For example, > atrace_agent.py is now a complete delete + complete new file. > > > > Is it possible to maintain just the code diff and ignore the changes due to > directory change? Alex, did you use 'git mv' (or something similar)? > > I used "git mv ./agents ./tracing_agents" Actually, can you use 'mv' instead of 'git mv'? Using just 'mv' will do the trick. See https://codereview.chromium.org/1797013003/ and check the files, you will see only atrace_agent.py has the actual diff. All changes due to directory change will be ignored.
Updated to use the json output format. Note that the trace viewer will not view even the original HTML format unless I manually remove the trace data output from the tracing controller (i.e. "controller side" of the clock sync markers. Maybe I am using an older version of the trace viewer?
I think there are several importing issues. We should be able to run both of the following - //catapult/systrace/bin/systrace - //catapult/systrace/systrace/run_systrace.py You will need to add the devil path in //catapult/systrace/systrace/__init__.py and __main__ function of run_systrace.py How did you test run your patch with importing errors? I saw you could test run the patch on your machine yesterday, right? https://codereview.chromium.org/1776013005/diff/60001/systrace/systrace/run_s... File systrace/systrace/run_systrace.py (right): https://codereview.chromium.org/1776013005/diff/60001/systrace/systrace/run_s... systrace/systrace/run_systrace.py:1: #!/usr/bin/env python On 2016/03/14 22:19:28, alexandermont wrote: > On 2016/03/14 at 17:11:27, Zhen Wang wrote: > > +ccraik, do we need to keep the old file name (systrace.py)? > > FYI, the reason that I changed the name systrace.py to run_systrace.py was > because the import seemed to be getting confused with the name "systrace" for > the whole module and the same name "systrace" for this file. If you change it to run_systrace.py, you will also need to update //catapult/systrace/bin/systrace to call this file https://codereview.chromium.org/1776013005/diff/60001/systrace/systrace/traci... File systrace/systrace/tracing_agent.py (right): https://codereview.chromium.org/1776013005/diff/60001/systrace/systrace/traci... systrace/systrace/tracing_agent.py:43: ''' On 2016/03/14 22:19:28, alexandermont wrote: > On 2016/03/14 at 17:11:27, Zhen Wang wrote: > > nit: Add a sentence to explain what the function do here. > > I'm confused: what's wrong with the current documentation "returns Boolean value > indicating whether this agent supports recording of clock sync markers?" There is nothing wrong with the Returns part. I mean just add a one line summary. See examples here: https://google.github.io/styleguide/pyguide.html?showone=Comments#Comments https://codereview.chromium.org/1776013005/diff/120001/systrace/systrace/syst... File systrace/systrace/systrace_tracing_controller.py (right): https://codereview.chromium.org/1776013005/diff/120001/systrace/systrace/syst... systrace/systrace/systrace_tracing_controller.py:103: if data: Do you actually also need to check should_output_trace? By the way, when should we not output trace? https://codereview.chromium.org/1776013005/diff/120001/systrace/systrace/syst... systrace/systrace/systrace_tracing_controller.py:107: html_file.write(str(data)) As I mentioned in the email, you will need some trace viewer change to be able to show the py_trace_event data (update systrace_config.html in trace viewer.) To unblock from that change, you can check the data_name here and exclude systrace log for now (add a TODO). After the trace viewer change is landed, you can include systrace log back. https://codereview.chromium.org/1776013005/diff/120001/systrace/systrace/trac... File systrace/systrace/tracing_agents/__init__.py (right): https://codereview.chromium.org/1776013005/diff/120001/systrace/systrace/trac... systrace/systrace/tracing_agents/__init__.py:25: Starts running the trace for this agent. Please follow the style guide for all the APIs here: https://google.github.io/styleguide/pyguide.html?showone=Comments#Comments For example, the summary is usually just one small sentence that can fit in the same line with '''. And then you can have some detailed description, followed by Args and Returns. Please also check the comments in other files and use the same style. https://codereview.chromium.org/1776013005/diff/120001/systrace/systrace/trac... File systrace/systrace/tracing_agents/atrace_agent.py (right): https://codereview.chromium.org/1776013005/diff/120001/systrace/systrace/trac... systrace/systrace/tracing_agents/atrace_agent.py:17: #TODO: add import below after persistent shell CL lands When adding TODO, also include your ID, e.g., TODO(alexandermont). https://codereview.chromium.org/1776013005/diff/120001/systrace/systrace/trac... systrace/systrace/tracing_agents/atrace_agent.py:135: #return True Remove this "return True" line here. Just describe in the TODO. In general, do not include the commented code in the patch. Please also update other places in the patch. https://codereview.chromium.org/1776013005/diff/120001/systrace/systrace/trac... systrace/systrace/tracing_agents/atrace_agent.py:151: # did_record_sync_marker_callback(t1, sync_id) ditto https://codereview.chromium.org/1776013005/diff/120001/systrace/systrace/trac... File systrace/systrace/tracing_controller.py (right): https://codereview.chromium.org/1776013005/diff/120001/systrace/systrace/trac... systrace/systrace/tracing_controller.py:16: from systrace.tracing_agents import TracingAgent, TraceResults 1. Add one empty line between system library and our own library. 2. Put TracingAgent and TraceResults on 2 lines. Apply the above to all files. https://codereview.chromium.org/1776013005/diff/120001/systrace/systrace/trac... systrace/systrace/tracing_controller.py:35: #override I think you do not need to comment override here (and also other places in the patch).
Changed it to stop the trace using enter instead of control-C if no length of time is selected. This is necessary because control-C will also stop the battor agent binary, which will prevent us from getting the battor agent binary trace out.
Some more comments about import issues in addition to https://codereview.chromium.org/1776013005/#msg22 You also need to update //catapult/systrace/bin/systrace line 13-16: from systrace import run_systrace if __name__ == '__main__': sys.exit(run_systrace.main()) https://codereview.chromium.org/1776013005/diff/200001/systrace/systrace/run_... File systrace/systrace/run_systrace.py (right): https://codereview.chromium.org/1776013005/diff/200001/systrace/systrace/run_... systrace/systrace/run_systrace.py:16: import systrace #pylint: disable=unused-import If someones runs this script directly, there will be import error to import systrace, too. You need to add this dir to the search path first. Add the following lines: _SYSTRACE_DIR = os.path.abspath( os.path.join(os.path.dirname(__file__), os.path.pardir)) sys.path.insert(0, _SYSTRACE_DIR) https://codereview.chromium.org/1776013005/diff/200001/systrace/systrace/run_... systrace/systrace/run_systrace.py:151: line 146-151 can be removed now. https://codereview.chromium.org/1776013005/diff/200001/systrace/systrace/trac... File systrace/systrace/tracing_agents/atrace_agent.py (right): https://codereview.chromium.org/1776013005/diff/200001/systrace/systrace/trac... systrace/systrace/tracing_agents/atrace_agent.py:12: import util Use "from systrace import util", otherwise, you will get import error.
https://codereview.chromium.org/1776013005/diff/120001/systrace/systrace/syst... File systrace/systrace/systrace_tracing_controller.py (right): https://codereview.chromium.org/1776013005/diff/120001/systrace/systrace/syst... systrace/systrace/systrace_tracing_controller.py:103: if data: On 2016/03/16 at 16:54:29, Zhen Wang wrote: > Do you actually also need to check should_output_trace? > > By the way, when should we not output trace? We don't output the trace if --list-categories is set to True. In this case we just list the categories, don't do the whole trace. https://codereview.chromium.org/1776013005/diff/120001/systrace/systrace/syst... systrace/systrace/systrace_tracing_controller.py:107: html_file.write(str(data)) On 2016/03/16 at 16:54:29, Zhen Wang wrote: > As I mentioned in the email, you will need some trace viewer change to be able to show the py_trace_event data (update systrace_config.html in trace viewer.) > > To unblock from that change, you can check the data_name here and exclude systrace log for now (add a TODO). After the trace viewer change is landed, you can include systrace log back. @charliea should be done with the changes to the trace viewer very soon. https://codereview.chromium.org/1776013005/diff/120001/systrace/systrace/trac... File systrace/systrace/tracing_agents/__init__.py (right): https://codereview.chromium.org/1776013005/diff/120001/systrace/systrace/trac... systrace/systrace/tracing_agents/__init__.py:25: Starts running the trace for this agent. On 2016/03/16 at 16:54:29, Zhen Wang wrote: > Please follow the style guide for all the APIs here: > https://google.github.io/styleguide/pyguide.html?showone=Comments#Comments > > For example, the summary is usually just one small sentence that can fit in the same line with '''. And then you can have some detailed description, followed by Args and Returns. > > Please also check the comments in other files and use the same style. Done https://codereview.chromium.org/1776013005/diff/120001/systrace/systrace/trac... File systrace/systrace/tracing_agents/atrace_agent.py (right): https://codereview.chromium.org/1776013005/diff/120001/systrace/systrace/trac... systrace/systrace/tracing_agents/atrace_agent.py:17: #TODO: add import below after persistent shell CL lands On 2016/03/16 at 16:54:30, Zhen Wang wrote: > When adding TODO, also include your ID, e.g., TODO(alexandermont). Done https://codereview.chromium.org/1776013005/diff/120001/systrace/systrace/trac... systrace/systrace/tracing_agents/atrace_agent.py:151: # did_record_sync_marker_callback(t1, sync_id) On 2016/03/16 at 16:54:29, Zhen Wang wrote: > ditto Done https://codereview.chromium.org/1776013005/diff/120001/systrace/systrace/trac... File systrace/systrace/tracing_controller.py (right): https://codereview.chromium.org/1776013005/diff/120001/systrace/systrace/trac... systrace/systrace/tracing_controller.py:16: from systrace.tracing_agents import TracingAgent, TraceResults On 2016/03/16 at 16:54:30, Zhen Wang wrote: > 1. Add one empty line between system library and our own library. > 2. Put TracingAgent and TraceResults on 2 lines. > > Apply the above to all files. Done https://codereview.chromium.org/1776013005/diff/120001/systrace/systrace/trac... systrace/systrace/tracing_controller.py:35: #override On 2016/03/16 at 16:54:30, Zhen Wang wrote: > I think you do not need to comment override here (and also other places in the patch). Done https://codereview.chromium.org/1776013005/diff/200001/systrace/systrace/trac... File systrace/systrace/tracing_agents/atrace_agent.py (right): https://codereview.chromium.org/1776013005/diff/200001/systrace/systrace/trac... systrace/systrace/tracing_agents/atrace_agent.py:12: import util On 2016/03/18 at 22:33:18, Zhen Wang wrote: > Use "from systrace import util", otherwise, you will get import error. Done
https://codereview.chromium.org/1776013005/diff/220001/systrace/systrace/__in... File systrace/systrace/__init__.py (right): https://codereview.chromium.org/1776013005/diff/220001/systrace/systrace/__in... systrace/systrace/__init__.py:20: os.path.dirname(os.path.abspath(__file__)), '..', '..') nit: s/'..'/os.path.pardir https://codereview.chromium.org/1776013005/diff/220001/systrace/systrace/__in... systrace/systrace/__init__.py:23: _AddDirToPythonPath(_CATAPULT_DIR, 'devil') Put this in alphabetic order. https://codereview.chromium.org/1776013005/diff/220001/systrace/systrace/run_... File systrace/systrace/run_systrace.py (right): https://codereview.chromium.org/1776013005/diff/220001/systrace/systrace/run_... systrace/systrace/run_systrace.py:25: from systrace import systrace_tracing_controller There are still import errors for devil. And you do not need to import systrace because you added it to the search path. Replace line 18-25 with the following: _SYSTRACE_DIR = os.path.abspath( os.path.join(os.path.dirname(__file__), os.path.pardir)) _CATAPULT_DIR = os.path.join( os.path.dirname(os.path.abspath(__file__)), os.path.pardir, os.path.pardir) _DEVIL_DIR = os.path.join(_CATAPULT_DIR, 'devil') if _DEVIL_DIR not in sys.path: sys.path.insert(0, _DEVIL_DIR) if _SYSTRACE_DIR not in sys.path: sys.path.insert(0, _SYSTRACE_DIR) from devil.utils import cmd_helper from systrace import systrace_tracing_controller https://codereview.chromium.org/1776013005/diff/220001/systrace/systrace/run_... systrace/systrace/run_systrace.py:57: parser.add_option('-l', '--list-categories', dest='list_categories', This is not working now. Can you fix it? https://codereview.chromium.org/1776013005/diff/220001/systrace/systrace/run_... systrace/systrace/run_systrace.py:101: help='chose tracing target (android or linux)') Have you tested --target=linux (ftrace agent)? It seems not working. Can you fix it? https://codereview.chromium.org/1776013005/diff/220001/systrace/systrace/trac... File systrace/systrace/tracing_agents/ftrace_agent.py (right): https://codereview.chromium.org/1776013005/diff/220001/systrace/systrace/trac... systrace/systrace/tracing_agents/ftrace_agent.py:9: from systrace.tracing_agents import TracingAgent, TraceResults Separate this to 2 lines
https://codereview.chromium.org/1776013005/diff/220001/systrace/systrace/__in... File systrace/systrace/__init__.py (right): https://codereview.chromium.org/1776013005/diff/220001/systrace/systrace/__in... systrace/systrace/__init__.py:20: os.path.dirname(os.path.abspath(__file__)), '..', '..') On 2016/03/24 at 22:13:05, Zhen Wang wrote: > nit: s/'..'/os.path.pardir Done https://codereview.chromium.org/1776013005/diff/220001/systrace/systrace/__in... systrace/systrace/__init__.py:23: _AddDirToPythonPath(_CATAPULT_DIR, 'devil') On 2016/03/24 at 22:13:05, Zhen Wang wrote: > Put this in alphabetic order. Done https://codereview.chromium.org/1776013005/diff/220001/systrace/systrace/run_... File systrace/systrace/run_systrace.py (right): https://codereview.chromium.org/1776013005/diff/220001/systrace/systrace/run_... systrace/systrace/run_systrace.py:25: from systrace import systrace_tracing_controller On 2016/03/24 at 22:13:05, Zhen Wang wrote: > There are still import errors for devil. And you do not need to import systrace because you added it to the search path. > > Replace line 18-25 with the following: > > > _SYSTRACE_DIR = os.path.abspath( > os.path.join(os.path.dirname(__file__), os.path.pardir)) > _CATAPULT_DIR = os.path.join( > os.path.dirname(os.path.abspath(__file__)), os.path.pardir, os.path.pardir) > _DEVIL_DIR = os.path.join(_CATAPULT_DIR, 'devil') > if _DEVIL_DIR not in sys.path: > sys.path.insert(0, _DEVIL_DIR) > if _SYSTRACE_DIR not in sys.path: > sys.path.insert(0, _SYSTRACE_DIR) > > from devil.utils import cmd_helper > from systrace import systrace_tracing_controller Done https://codereview.chromium.org/1776013005/diff/220001/systrace/systrace/run_... systrace/systrace/run_systrace.py:57: parser.add_option('-l', '--list-categories', dest='list_categories', On 2016/03/24 at 22:13:05, Zhen Wang wrote: > This is not working now. Can you fix it? Done. Also note that since the list_categories command is being handled a different way now (through a check before you get into the tracing controller, since you don't need to do all the tracing controller stuff if you just want to list the categories) I have removed the parts inside of list_categories that handle the tracing agents. Note that all the should_output_trace stuff was only being used for list_categories, so it might no longer be necessary. @ccraik, is there anything else that should_output_trace was needed for, or can it be removed now? https://codereview.chromium.org/1776013005/diff/220001/systrace/systrace/run_... systrace/systrace/run_systrace.py:101: help='chose tracing target (android or linux)') On 2016/03/24 at 22:13:05, Zhen Wang wrote: > Have you tested --target=linux (ftrace agent)? It seems not working. Can you fix it? Done. The problem was the following. In order to run systrace with --target=linux, it's necessary to run systrace with sudo. However, using sudo makes the "adb devices" command which is run to get the serial number of the android device not work. But you don't need the android device serial number when you're using target=linux, so you can just not run "adb devices" in this case. https://codereview.chromium.org/1776013005/diff/220001/systrace/systrace/trac... File systrace/systrace/tracing_agents/ftrace_agent.py (right): https://codereview.chromium.org/1776013005/diff/220001/systrace/systrace/trac... systrace/systrace/tracing_agents/ftrace_agent.py:9: from systrace.tracing_agents import TracingAgent, TraceResults On 2016/03/24 at 22:13:05, Zhen Wang wrote: > Separate this to 2 lines Done
Alex, when I try systrace, I get error message "Couldn't create an importer for the provided eventData." Is that something expected because it depends on some trace viewer patches? Can you also fill up the CL description with which CLs this CL is depending on? (If the dependent CL is not uploaded yet, describe what is needed.) https://codereview.chromium.org/1776013005/diff/240001/systrace/systrace/run_... File systrace/systrace/run_systrace.py (right): https://codereview.chromium.org/1776013005/diff/240001/systrace/systrace/run_... systrace/systrace/run_systrace.py:143: ftrace_agent.list_categories(options) return after this line. The else block below can be move outside. That is: if xyz: [statement 1] [statement 2] return [statement 3] [statement 4] ... https://codereview.chromium.org/1776013005/diff/240001/systrace/systrace/trac... File systrace/systrace/tracing_agents/__init__.py (right): https://codereview.chromium.org/1776013005/diff/240001/systrace/systrace/trac... systrace/systrace/tracing_agents/__init__.py:23: def StartAgentTracing(self): Also add a timeout here. In terms of atrace and ftrace, the timeout seems to be overkill. But it is in general good to make sure the program is not hanging for too long. Later, when we want to make this work with Chrome tracing, we will also need to have timeout. Let's starting from 10 sec timeout here. The agent should throw timeout exception when timeout. https://codereview.chromium.org/1776013005/diff/240001/systrace/systrace/trac... systrace/systrace/tracing_agents/__init__.py:31: def StopAgentTracing(self): Also add a timeout here. Let's starting from 10 sec timeout. https://codereview.chromium.org/1776013005/diff/240001/systrace/systrace/trac... systrace/systrace/tracing_agents/__init__.py:60: def GetResults(self): Also add a timeout here. Given this function is a blocking call, we need to limit how long we should wait. Let's starting from 30 sec timeout (giving more time to get results). https://codereview.chromium.org/1776013005/diff/240001/systrace/systrace/trac... File systrace/systrace/tracing_controller.py (right): https://codereview.chromium.org/1776013005/diff/240001/systrace/systrace/trac... systrace/systrace/tracing_controller.py:92: all_succ = agent.StartAgentTracing() and all_succ What happens if some agent returns false? Can we also add some error handling logic here? https://codereview.chromium.org/1776013005/diff/240001/systrace/systrace/trac... systrace/systrace/tracing_controller.py:111: all_succ = self.StopAgentTracing() and all_succ What happens if some agent returns false? Can we also add some error handling logic here? https://codereview.chromium.org/1776013005/diff/240001/systrace/systrace/trac... systrace/systrace/tracing_controller.py:115: (key, value, show_output) = agent.GetResults() Add some error handling logic here.
Description was changed from ========== refactor systrace to support new clock sync design ========== to ========== Refactor systrace to support new clock sync design. This CL adds a new "traceEvents" record to the list of traces output by systrace (even though clock sync markers themselves are turned off in this CL). Thus, to view this in the trace viewer, the trace viewer will need to be changed to accept this new output format. ==========
Now with changes based on issues from previous code review. Note that this will still require charliea@'s changes to the trace viewer, in order to process the new format with the clock sync marker. https://codereview.chromium.org/1776013005/diff/240001/systrace/systrace/run_... File systrace/systrace/run_systrace.py (right): https://codereview.chromium.org/1776013005/diff/240001/systrace/systrace/run_... systrace/systrace/run_systrace.py:143: ftrace_agent.list_categories(options) On 2016/03/25 at 22:12:26, Zhen Wang wrote: > return after this line. The else block below can be move outside. That is: > > if xyz: > [statement 1] > [statement 2] > return > > [statement 3] > [statement 4] > ... Done https://codereview.chromium.org/1776013005/diff/240001/systrace/systrace/trac... File systrace/systrace/tracing_agents/__init__.py (right): https://codereview.chromium.org/1776013005/diff/240001/systrace/systrace/trac... systrace/systrace/tracing_agents/__init__.py:23: def StartAgentTracing(self): On 2016/03/25 at 22:12:26, Zhen Wang wrote: > Also add a timeout here. > > In terms of atrace and ftrace, the timeout seems to be overkill. But it is in general good to make sure the program is not hanging for too long. Later, when we want to make this work with Chrome tracing, we will also need to have timeout. Let's starting from 10 sec timeout here. > > The agent should throw timeout exception when timeout. Done https://codereview.chromium.org/1776013005/diff/240001/systrace/systrace/trac... systrace/systrace/tracing_agents/__init__.py:31: def StopAgentTracing(self): On 2016/03/25 at 22:12:26, Zhen Wang wrote: > Also add a timeout here. > > Let's starting from 10 sec timeout. Done https://codereview.chromium.org/1776013005/diff/240001/systrace/systrace/trac... systrace/systrace/tracing_agents/__init__.py:60: def GetResults(self): On 2016/03/25 at 22:12:26, Zhen Wang wrote: > Also add a timeout here. Given this function is a blocking call, we need to limit how long we should wait. > > Let's starting from 30 sec timeout (giving more time to get results). Done https://codereview.chromium.org/1776013005/diff/240001/systrace/systrace/trac... File systrace/systrace/tracing_controller.py (right): https://codereview.chromium.org/1776013005/diff/240001/systrace/systrace/trac... systrace/systrace/tracing_controller.py:92: all_succ = agent.StartAgentTracing() and all_succ On 2016/03/25 at 22:12:26, Zhen Wang wrote: > What happens if some agent returns false? Can we also add some error handling logic here? Logic added to continue the trace with only the tracing agents that returned true. https://codereview.chromium.org/1776013005/diff/240001/systrace/systrace/trac... systrace/systrace/tracing_controller.py:111: all_succ = self.StopAgentTracing() and all_succ On 2016/03/25 at 22:12:26, Zhen Wang wrote: > What happens if some agent returns false? Can we also add some error handling logic here? Done https://codereview.chromium.org/1776013005/diff/240001/systrace/systrace/trac... systrace/systrace/tracing_controller.py:115: (key, value, show_output) = agent.GetResults() On 2016/03/25 at 22:12:26, Zhen Wang wrote: > Add some error handling logic here. Done
I just tried your CL with both HTML and JSON formats for --target=android. It actually works. You do not need to be blocked on charliea@' work to commit this CL. The problem is that you need to use the most updated systrace_trace_viewer.html file for HTML output. See my comments in run_systrace.py and systrace_tracing_controller.py on how to fix that. But for --target=linux, the trace viewer does not show anything. Can you fix that? https://codereview.chromium.org/1776013005/diff/260001/systrace/systrace/run_... File systrace/systrace/run_systrace.py (right): https://codereview.chromium.org/1776013005/diff/260001/systrace/systrace/run_... systrace/systrace/run_systrace.py:149: script_dir = os.path.dirname(os.path.abspath(sys.argv[0])) s/sys.argv[0]/__file__ Change script_dir to be relative to run_systrace.py itself. See https://github.com/catapult-project/catapult/issues/2085 https://codereview.chromium.org/1776013005/diff/260001/systrace/systrace/syst... File systrace/systrace/systrace_tracing_controller.py (right): https://codereview.chromium.org/1776013005/diff/260001/systrace/systrace/syst... systrace/systrace/systrace_tracing_controller.py:50: update_systrace_trace_viewer.update() Pass script_dir to update_systrace_trace_viewer and save the generated systrace_trace_viewer.html to the script_dir. In update_systrace_trace_viewer.py, the html file path is relative to where you run the command. It should be relative to the script_dir. https://codereview.chromium.org/1776013005/diff/260001/systrace/systrace/trac... File systrace/systrace/tracing_agents/__init__.py (right): https://codereview.chromium.org/1776013005/diff/260001/systrace/systrace/trac... systrace/systrace/tracing_agents/__init__.py:22: self._categories = categories In general, the interface class should not contain any states. Do not pass in options and categories here. We should just have a bunch of public functions in this class. In addition, we should limit the access to the states as much as possible. This means that concrete agents should not store options and categories as their member variables, either. For example, StartAgentTracing needs options and categories. But StopAgentTracing and GetResults only needs timeout value. https://codereview.chromium.org/1776013005/diff/260001/systrace/systrace/trac... systrace/systrace/tracing_agents/__init__.py:24: def _StartAgentTracing_func(self): Is this function "private" or "protected"? I guess you mean "protected" here, because subclasses need to override it. We do not have a way to distinguish between "protected" and "private" functions in python. People will be confused and think this is "private" function. (prepending single underscore means "private".) TracingAgent is the interface for all agents. It should just contain public interfaces. This applies to StopAgentTracing and GetResults too. https://codereview.chromium.org/1776013005/diff/260001/systrace/systrace/trac... systrace/systrace/tracing_agents/__init__.py:44: def StartAgentTracing(self): def StartAgentTracing(self, options, categories) https://codereview.chromium.org/1776013005/diff/260001/systrace/systrace/trac... systrace/systrace/tracing_agents/__init__.py:52: self._options.timeout, 1) How to handle timeout should be dedicated to each concrete agent. Not all agents can use timeout_retry. For example, later we will add Chrome tracing agent (merging with Telemetry tracing facilities). The timeout value will be passed to the websocket to DevTools and be handled there. (WebSocketTimeoutException will be thrown.) So this interface should just raise NotImplementedError. Not doing timeout_retry here also solve the problem above in for _StartAgentTracing_func(). You will then just have public interfaces. This applies to StopAgentTracing and GetResults too. https://codereview.chromium.org/1776013005/diff/260001/systrace/systrace/trac... systrace/systrace/tracing_agents/__init__.py:54: def StopAgentTracing(self): def StopAgentTracing(self, timeout) https://codereview.chromium.org/1776013005/diff/260001/systrace/systrace/trac... systrace/systrace/tracing_agents/__init__.py:95: def GetResults(self): def GetResults(self, timeout) https://codereview.chromium.org/1776013005/diff/260001/systrace/systrace/trac... File systrace/systrace/tracing_agents/atrace_agent.py (right): https://codereview.chromium.org/1776013005/diff/260001/systrace/systrace/trac... systrace/systrace/tracing_agents/atrace_agent.py:89: def __init__(self, options, categories): Do not pass in options and categories. Only device_serial is needed in the constructor. https://codereview.chromium.org/1776013005/diff/260001/systrace/systrace/trac... File systrace/systrace/tracing_agents/ftrace_agent.py (right): https://codereview.chromium.org/1776013005/diff/260001/systrace/systrace/trac... systrace/systrace/tracing_agents/ftrace_agent.py:98: def __init__(self, options, categories, fio=FtraceAgentIo): Do not pass in options and categories. Pass them in StartAgentTracing() https://codereview.chromium.org/1776013005/diff/260001/systrace/systrace/trac... systrace/systrace/tracing_agents/ftrace_agent.py:129: """ Run self._fio.haveWritePermissions before actually starting tracing. If the user forgets sudo, we have a way to print some nice error message out early (and return early). For example: "No write permission to ..., perhaps you need sudo/root?" https://codereview.chromium.org/1776013005/diff/260001/systrace/systrace/trac... File systrace/systrace/tracing_controller.py (right): https://codereview.chromium.org/1776013005/diff/260001/systrace/systrace/trac... systrace/systrace/tracing_controller.py:99: print 'Warning: Only %d of %d tracing agents started.' % (na, ns) Can we also print which agent is not started? https://codereview.chromium.org/1776013005/diff/260001/systrace/systrace/trac... systrace/systrace/tracing_controller.py:126: print 'Warning: Only %d of %d tracing agents stopped.' % (na, ns) Can we also print which agent is not started? https://codereview.chromium.org/1776013005/diff/260001/systrace/systrace/trac... systrace/systrace/tracing_controller.py:135: except reraiser_thread.TimeoutError: Since not every agent will use timeout_retry, you will then need to add one more catch for Exception.
Some more changes from the code review. I was not able to reproduce the problem you were seeing with --target=android vs. --target=linux, nor was I able to get it to view the traces with the new controller tracing agent record. For me, regardless of which target was chosen, the trace viewer (at least with the HTML) was blank if I included the controller tracing agent record, and worked correctly if I did not include that record. For now, I added a variable (in tracing_controller.py) that says whether to include the controller tracing agent record. https://codereview.chromium.org/1776013005/diff/260001/systrace/systrace/run_... File systrace/systrace/run_systrace.py (right): https://codereview.chromium.org/1776013005/diff/260001/systrace/systrace/run_... systrace/systrace/run_systrace.py:149: script_dir = os.path.dirname(os.path.abspath(sys.argv[0])) On 2016/03/29 at 18:53:39, Zhen Wang wrote: > s/sys.argv[0]/__file__ > > Change script_dir to be relative to run_systrace.py itself. > See https://github.com/catapult-project/catapult/issues/2085 Done. https://codereview.chromium.org/1776013005/diff/260001/systrace/systrace/syst... File systrace/systrace/systrace_tracing_controller.py (right): https://codereview.chromium.org/1776013005/diff/260001/systrace/systrace/syst... systrace/systrace/systrace_tracing_controller.py:50: update_systrace_trace_viewer.update() On 2016/03/29 at 18:53:39, Zhen Wang wrote: > Pass script_dir to update_systrace_trace_viewer and save the generated systrace_trace_viewer.html to the script_dir. > > In update_systrace_trace_viewer.py, the html file path is relative to where you run the command. It should be relative to the script_dir. Done https://codereview.chromium.org/1776013005/diff/260001/systrace/systrace/trac... File systrace/systrace/tracing_agents/__init__.py (right): https://codereview.chromium.org/1776013005/diff/260001/systrace/systrace/trac... systrace/systrace/tracing_agents/__init__.py:22: self._categories = categories On 2016/03/29 at 18:53:40, Zhen Wang wrote: > In general, the interface class should not contain any states. Do not pass in options and categories here. We should just have a bunch of public functions in this class. > > In addition, we should limit the access to the states as much as possible. This means that concrete agents should not store options and categories as their member variables, either. > > For example, StartAgentTracing needs options and categories. But StopAgentTracing and GetResults only needs timeout value. Done https://codereview.chromium.org/1776013005/diff/260001/systrace/systrace/trac... systrace/systrace/tracing_agents/__init__.py:24: def _StartAgentTracing_func(self): On 2016/03/29 at 18:53:40, Zhen Wang wrote: > Is this function "private" or "protected"? I guess you mean "protected" here, because subclasses need to override it. We do not have a way to distinguish between "protected" and "private" functions in python. People will be confused and think this is "private" function. (prepending single underscore means "private".) > > TracingAgent is the interface for all agents. It should just contain public interfaces. > > This applies to StopAgentTracing and GetResults too. Done https://codereview.chromium.org/1776013005/diff/260001/systrace/systrace/trac... systrace/systrace/tracing_agents/__init__.py:44: def StartAgentTracing(self): On 2016/03/29 at 18:53:39, Zhen Wang wrote: > def StartAgentTracing(self, options, categories) Done https://codereview.chromium.org/1776013005/diff/260001/systrace/systrace/trac... systrace/systrace/tracing_agents/__init__.py:52: self._options.timeout, 1) On 2016/03/29 at 18:53:40, Zhen Wang wrote: > How to handle timeout should be dedicated to each concrete agent. Not all agents can use timeout_retry. > > For example, later we will add Chrome tracing agent (merging with Telemetry tracing facilities). The timeout value will be passed to the websocket to DevTools and be handled there. (WebSocketTimeoutException will be thrown.) > > So this interface should just raise NotImplementedError. Not doing timeout_retry here also solve the problem above in for _StartAgentTracing_func(). You will then just have public interfaces. > > This applies to StopAgentTracing and GetResults too. Done https://codereview.chromium.org/1776013005/diff/260001/systrace/systrace/trac... systrace/systrace/tracing_agents/__init__.py:54: def StopAgentTracing(self): On 2016/03/29 at 18:53:40, Zhen Wang wrote: > def StopAgentTracing(self, timeout) Done https://codereview.chromium.org/1776013005/diff/260001/systrace/systrace/trac... systrace/systrace/tracing_agents/__init__.py:54: def StopAgentTracing(self): On 2016/03/29 at 18:53:40, Zhen Wang wrote: > def StopAgentTracing(self, timeout) Done https://codereview.chromium.org/1776013005/diff/260001/systrace/systrace/trac... systrace/systrace/tracing_agents/__init__.py:95: def GetResults(self): On 2016/03/29 at 18:53:40, Zhen Wang wrote: > def GetResults(self, timeout) Done https://codereview.chromium.org/1776013005/diff/260001/systrace/systrace/trac... File systrace/systrace/tracing_agents/atrace_agent.py (right): https://codereview.chromium.org/1776013005/diff/260001/systrace/systrace/trac... systrace/systrace/tracing_agents/atrace_agent.py:89: def __init__(self, options, categories): On 2016/03/29 at 18:53:40, Zhen Wang wrote: > Do not pass in options and categories. Only device_serial is needed in the constructor. Done https://codereview.chromium.org/1776013005/diff/260001/systrace/systrace/trac... File systrace/systrace/tracing_agents/ftrace_agent.py (right): https://codereview.chromium.org/1776013005/diff/260001/systrace/systrace/trac... systrace/systrace/tracing_agents/ftrace_agent.py:98: def __init__(self, options, categories, fio=FtraceAgentIo): On 2016/03/29 at 18:53:40, Zhen Wang wrote: > Do not pass in options and categories. Pass them in StartAgentTracing() Done https://codereview.chromium.org/1776013005/diff/260001/systrace/systrace/trac... systrace/systrace/tracing_agents/ftrace_agent.py:129: """ On 2016/03/29 at 18:53:40, Zhen Wang wrote: > Run self._fio.haveWritePermissions before actually starting tracing. If the user forgets sudo, we have a way to print some nice error message out early (and return early). For example: "No write permission to ..., perhaps you need sudo/root?" Done https://codereview.chromium.org/1776013005/diff/260001/systrace/systrace/trac... File systrace/systrace/tracing_controller.py (right): https://codereview.chromium.org/1776013005/diff/260001/systrace/systrace/trac... systrace/systrace/tracing_controller.py:99: print 'Warning: Only %d of %d tracing agents started.' % (na, ns) On 2016/03/29 at 18:53:40, Zhen Wang wrote: > Can we also print which agent is not started? Done https://codereview.chromium.org/1776013005/diff/260001/systrace/systrace/trac... systrace/systrace/tracing_controller.py:126: print 'Warning: Only %d of %d tracing agents stopped.' % (na, ns) On 2016/03/29 at 18:53:40, Zhen Wang wrote: > Can we also print which agent is not started? Done https://codereview.chromium.org/1776013005/diff/260001/systrace/systrace/trac... systrace/systrace/tracing_controller.py:135: except reraiser_thread.TimeoutError: On 2016/03/29 at 18:53:40, Zhen Wang wrote: > Since not every agent will use timeout_retry, you will then need to add one more catch for Exception. Done. Note that it re-raises the exception if it's not a TimeoutError, since it doesn't know whether the other exception represents a timeout or something more serious.
If you use Catapult ToT, atrace json and html formats both work. For ftrace, I think it is related to the importer logic in Trace Viewer. I will ask charliea. https://codereview.chromium.org/1776013005/diff/280001/systrace/systrace/syst... File systrace/systrace/systrace_tracing_controller.py (right): https://codereview.chromium.org/1776013005/diff/280001/systrace/systrace/syst... systrace/systrace/systrace_tracing_controller.py:46: update_systrace_trace_viewer = __import__('update_systrace_trace_viewer') Update this line to the following: from systrace import update_systrace_trace_viewer https://codereview.chromium.org/1776013005/diff/280001/systrace/systrace/trac... File systrace/systrace/tracing_agents/__init__.py (right): https://codereview.chromium.org/1776013005/diff/280001/systrace/systrace/trac... systrace/systrace/tracing_agents/__init__.py:25: nit: document the parameters. https://codereview.chromium.org/1776013005/diff/280001/systrace/systrace/trac... systrace/systrace/tracing_agents/__init__.py:34: nit: document the parameters. https://codereview.chromium.org/1776013005/diff/280001/systrace/systrace/trac... systrace/systrace/tracing_agents/__init__.py:66: nit: document the parameters. https://codereview.chromium.org/1776013005/diff/280001/systrace/systrace/trac... File systrace/systrace/tracing_agents/ftrace_agent.py (right): https://codereview.chromium.org/1776013005/diff/280001/systrace/systrace/trac... systrace/systrace/tracing_agents/ftrace_agent.py:34: def checkedWriteFile(path, data): nit: s/checkedWriteFile/checkAndWriteFile https://codereview.chromium.org/1776013005/diff/280001/systrace/systrace/trac... File systrace/systrace/tracing_agents/ftrace_agent_unittest.py (right): https://codereview.chromium.org/1776013005/diff/280001/systrace/systrace/trac... systrace/systrace/tracing_agents/ftrace_agent_unittest.py:39: def checkedWriteFile(path, data): nit: checkAndWriteFile https://codereview.chromium.org/1776013005/diff/280001/systrace/systrace/trac... File systrace/systrace/tracing_controller.py (right): https://codereview.chromium.org/1776013005/diff/280001/systrace/systrace/trac... systrace/systrace/tracing_controller.py:50: def _StartAgentTracing_func(self): nit: is this name following the style? Why not just _StartAgentTracing? Same question for other places. https://codereview.chromium.org/1776013005/diff/280001/systrace/systrace/upda... File systrace/systrace/update_systrace_trace_viewer.py (right): https://codereview.chromium.org/1776013005/diff/280001/systrace/systrace/upda... systrace/systrace/update_systrace_trace_viewer.py:28: def get_catapult_rev_in_file_(): need to pass in dest dir or use full path name. https://codereview.chromium.org/1776013005/diff/280001/systrace/systrace/upda... systrace/systrace/update_systrace_trace_viewer.py:72: if os.path.exists(SYSTRACE_TRACE_VIEWER_HTML_FILE_): need to use full path name with dest dir.
https://codereview.chromium.org/1776013005/diff/280001/systrace/systrace/trac... File systrace/systrace/tracing_controller.py (right): https://codereview.chromium.org/1776013005/diff/280001/systrace/systrace/trac... systrace/systrace/tracing_controller.py:27: output_controller_tracing_agent = False s/output_controller_tracing_agent/OUTPUT_CONTROLLER_TRACING_AGENT_ I just chatted with Charlie, I think we do need to wait for his CL to make sure the output with controller log is working correctly. We can keep this for now and remove this once his work is done. Later, when adding clock sync support, you also need to add some support to ftrace to make sure it is working correctly. That should not be very hard. See updates on https://github.com/catapult-project/catapult/issues/2193 for more background.
https://codereview.chromium.org/1776013005/diff/280001/systrace/systrace/syst... File systrace/systrace/systrace_tracing_controller.py (right): https://codereview.chromium.org/1776013005/diff/280001/systrace/systrace/syst... systrace/systrace/systrace_tracing_controller.py:46: update_systrace_trace_viewer = __import__('update_systrace_trace_viewer') On 2016/03/30 at 16:14:08, Zhen Wang wrote: > Update this line to the following: > > from systrace import update_systrace_trace_viewer Done https://codereview.chromium.org/1776013005/diff/280001/systrace/systrace/trac... File systrace/systrace/tracing_agents/__init__.py (right): https://codereview.chromium.org/1776013005/diff/280001/systrace/systrace/trac... systrace/systrace/tracing_agents/__init__.py:25: On 2016/03/30 at 16:14:08, Zhen Wang wrote: > nit: document the parameters. Done https://codereview.chromium.org/1776013005/diff/280001/systrace/systrace/trac... systrace/systrace/tracing_agents/__init__.py:34: On 2016/03/30 at 16:14:08, Zhen Wang wrote: > nit: document the parameters. Done https://codereview.chromium.org/1776013005/diff/280001/systrace/systrace/trac... systrace/systrace/tracing_agents/__init__.py:66: On 2016/03/30 at 16:14:08, Zhen Wang wrote: > nit: document the parameters. Done https://codereview.chromium.org/1776013005/diff/280001/systrace/systrace/trac... File systrace/systrace/tracing_agents/ftrace_agent.py (right): https://codereview.chromium.org/1776013005/diff/280001/systrace/systrace/trac... systrace/systrace/tracing_agents/ftrace_agent.py:34: def checkedWriteFile(path, data): On 2016/03/30 at 16:14:08, Zhen Wang wrote: > nit: s/checkedWriteFile/checkAndWriteFile Done https://codereview.chromium.org/1776013005/diff/280001/systrace/systrace/trac... File systrace/systrace/tracing_agents/ftrace_agent_unittest.py (right): https://codereview.chromium.org/1776013005/diff/280001/systrace/systrace/trac... systrace/systrace/tracing_agents/ftrace_agent_unittest.py:39: def checkedWriteFile(path, data): On 2016/03/30 at 16:14:08, Zhen Wang wrote: > nit: checkAndWriteFile Done https://codereview.chromium.org/1776013005/diff/280001/systrace/systrace/trac... File systrace/systrace/tracing_controller.py (right): https://codereview.chromium.org/1776013005/diff/280001/systrace/systrace/trac... systrace/systrace/tracing_controller.py:50: def _StartAgentTracing_func(self): On 2016/03/30 at 16:14:08, Zhen Wang wrote: > nit: is this name following the style? Why not just _StartAgentTracing? > > Same question for other places. Done. Changed to _StartAgentTracingImpl. (Don't want to change it to _StartAgentTracing, because that's too similar to the name of the public API function, StartAgentTracing.)
https://codereview.chromium.org/1776013005/diff/300001/systrace/systrace/syst... File systrace/systrace/systrace_tracing_controller.py (right): https://codereview.chromium.org/1776013005/diff/300001/systrace/systrace/syst... systrace/systrace/systrace_tracing_controller.py:85: results['controllerTraceDataKey'] = 'traceEvents' Check if we need to output controller data in this class. Also see comments in tracing_controller.py. Check the results here. If not outputting controller data and if result's key is "traceEvents", dump "traceEvents": [], Equivalently, I think it means: results['traceEvents'] = '[]' (if not outputting controller data) That means we dump empty data. Notice that we need this empty data to make JSON format work in trace viewer. Quote from charliea: "The gist of it is that the trace event importer uses that as a differentiator between two flavors of the trace event format, and "systemTraceEvents" won't get picked up as a subtrace unless a "traceEvents" field is also present." https://codereview.chromium.org/1776013005/diff/300001/systrace/systrace/syst... systrace/systrace/systrace_tracing_controller.py:117: for (_, data) in results.iteritems(): s/_/name if not outputing controller data and if name is "traceEvents", continue. https://codereview.chromium.org/1776013005/diff/300001/systrace/systrace/trac... File systrace/systrace/tracing_controller.py (right): https://codereview.chromium.org/1776013005/diff/300001/systrace/systrace/trac... systrace/systrace/tracing_controller.py:27: output_controller_tracing_agent = False Do this in systrace_tracing_controller.py. We capture and pass all the trace results and let systrace_tracing_controller decide which data to dump. Also see comments there. By the way, when you do it in systrace_tracing_controller.py, s/output_controller_tracing_agent/OUTPUT_CONTROLLER_TRACING_AGENT_ https://codereview.chromium.org/1776013005/diff/300001/systrace/systrace/upda... File systrace/systrace/update_systrace_trace_viewer.py (right): https://codereview.chromium.org/1776013005/diff/300001/systrace/systrace/upda... systrace/systrace/update_systrace_trace_viewer.py:77: """ Define output_html_file here. https://codereview.chromium.org/1776013005/diff/300001/systrace/systrace/upda... systrace/systrace/update_systrace_trace_viewer.py:93: output_html_file = os.path.join(dest, SYSTRACE_TRACE_VIEWER_HTML_FILE_) Why not just move this line to the beginning of update() and use it throughout, e.g., pass into get_catapult_rev_in_file_() etc.
https://codereview.chromium.org/1776013005/diff/300001/systrace/systrace/syst... File systrace/systrace/systrace_tracing_controller.py (right): https://codereview.chromium.org/1776013005/diff/300001/systrace/systrace/syst... systrace/systrace/systrace_tracing_controller.py:85: results['controllerTraceDataKey'] = 'traceEvents' On 2016/03/30 at 21:36:44, Zhen Wang wrote: > Check if we need to output controller data in this class. Also see comments in tracing_controller.py. > > > Check the results here. If not outputting controller data and if result's key is "traceEvents", dump > > "traceEvents": [], > > Equivalently, I think it means: > > results['traceEvents'] = '[]' (if not outputting controller data) > > That means we dump empty data. Notice that we need this empty data to make JSON format work in trace viewer. > > Quote from charliea: > "The gist of it is that the trace event importer uses that as a differentiator between two flavors of the trace event format, and "systemTraceEvents" won't get picked up as a subtrace unless a "traceEvents" field is also present." Done https://codereview.chromium.org/1776013005/diff/300001/systrace/systrace/syst... systrace/systrace/systrace_tracing_controller.py:117: for (_, data) in results.iteritems(): On 2016/03/30 at 21:36:44, Zhen Wang wrote: > s/_/name > > if not outputing controller data and if name is "traceEvents", continue. Done https://codereview.chromium.org/1776013005/diff/300001/systrace/systrace/trac... File systrace/systrace/tracing_controller.py (right): https://codereview.chromium.org/1776013005/diff/300001/systrace/systrace/trac... systrace/systrace/tracing_controller.py:27: output_controller_tracing_agent = False On 2016/03/30 at 21:36:44, Zhen Wang wrote: > Do this in systrace_tracing_controller.py. We capture and pass all the trace results and let systrace_tracing_controller decide which data to dump. Also see comments there. > > By the way, when you do it in systrace_tracing_controller.py, s/output_controller_tracing_agent/OUTPUT_CONTROLLER_TRACING_AGENT_ Done https://codereview.chromium.org/1776013005/diff/300001/systrace/systrace/upda... File systrace/systrace/update_systrace_trace_viewer.py (right): https://codereview.chromium.org/1776013005/diff/300001/systrace/systrace/upda... systrace/systrace/update_systrace_trace_viewer.py:93: output_html_file = os.path.join(dest, SYSTRACE_TRACE_VIEWER_HTML_FILE_) On 2016/03/30 at 21:36:44, Zhen Wang wrote: > Why not just move this line to the beginning of update() and use it throughout, e.g., pass into get_catapult_rev_in_file_() etc. Done
I took a quick look. Overall, looks good. But I think the JSON format for ftrace is still not working. Please check with charliea to see if we can have a quick fix. I don't have time to look into the details as my wife is in the baby delivery room of the hospital now. And I will take paternity leave right after. So for future code reviews, please contact Ned and Chris. Thanks! https://codereview.chromium.org/1776013005/diff/320001/systrace/systrace/syst... File systrace/systrace/systrace_tracing_controller.py (right): https://codereview.chromium.org/1776013005/diff/320001/systrace/systrace/syst... systrace/systrace/systrace_tracing_controller.py:93: results['traceEvents'] = [] With this, the JSON format for atrace is working. But the JSON format for ftrace is still not working. Please check with Charlie to see if this is the correct way.
On 2016/03/31 16:34:35, Zhen Wang (OOO until late May) wrote: > I took a quick look. Overall, looks good. But I think the JSON format for ftrace > is still not working. Please check with charliea to see if we can have a quick > fix. > > I don't have time to look into the details as my wife is in the baby delivery > room of the hospital now. And I will take paternity leave right after. So for > future code reviews, please contact Ned and Chris. Thanks! > > https://codereview.chromium.org/1776013005/diff/320001/systrace/systrace/syst... > File systrace/systrace/systrace_tracing_controller.py (right): > > https://codereview.chromium.org/1776013005/diff/320001/systrace/systrace/syst... > systrace/systrace/systrace_tracing_controller.py:93: results['traceEvents'] = [] > With this, the JSON format for atrace is working. But the JSON format for ftrace > is still not working. Please check with Charlie to see if this is the correct > way. Looks like there is the bug which the trace categories used in 'atrace' have changed after this CL. I am pretty sure that you want to keep those categories the same, at least for this refactoring patch.
Description was changed from ========== Refactor systrace to support new clock sync design. This CL adds a new "traceEvents" record to the list of traces output by systrace (even though clock sync markers themselves are turned off in this CL). Thus, to view this in the trace viewer, the trace viewer will need to be changed to accept this new output format. ========== to ========== Refactor systrace to support new clock sync design. This CL adds a new "traceEvents" record to the list of traces output by systrace (even though clock sync markers themselves are turned off in this CL). Thus, to view this in the trace viewer, the trace viewer will need to be changed to accept this new output format. ==========
charliea@chromium.org changed reviewers: - charliea@chromium.org
Moving myself to cc (I don't really have enough experience with systrace to do a real review)
Description was changed from ========== Refactor systrace to support new clock sync design. This CL adds a new "traceEvents" record to the list of traces output by systrace (even though clock sync markers themselves are turned off in this CL). Thus, to view this in the trace viewer, the trace viewer will need to be changed to accept this new output format. ========== to ========== Refactor systrace to support new clock sync design. This CL adds a new "traceEvents" record to the list of traces output by systrace (even though clock sync markers themselves are turned off in this CL). Thus, to view this in the trace viewer, the trace viewer will need to be changed to accept this new output format. ==========
We have a possible new plan to split this CL up into multiple smaller CLs: 1. Rename systrace/systrace/agents to systrace/systrace/tracing_agents 2. Change device_utils to add the new split_lines parameter 3. Rename systrace to run_systrace 4. Add TracingAgent class and modify atrace and ftrace to subclass from it. 5. Add in TracingController and SystraceTracingController (Possibly look at whether we need to have two separate classes for these)
Description was changed from ========== Refactor systrace to support new clock sync design. This CL adds a new "traceEvents" record to the list of traces output by systrace (even though clock sync markers themselves are turned off in this CL). Thus, to view this in the trace viewer, the trace viewer will need to be changed to accept this new output format. ========== to ========== Refactor systrace to support new clock sync design. Do not commit this CL. This CL will be broken into several smaller CLs. I am keeping the CL visible so that I can use these changes as a reference when creating the smaller CLs. ========== |
