Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(7)

Issue 3018533002: Implementing a Monsoon power monitor trace agent, utilizing the UI infrastructure that the BattOr a…

Created:
1 year, 2 months ago by ddeptford
Modified:
1 year, 2 months ago
CC:
catapult-reviews_chromium.org
Target Ref:
refs/heads/master
Project:
catapult
Visibility:
Public.

Description

Implementing a Monsoon power monitor trace agent, utilizing the UI infrastructure that the BattOr agent provides.

Patch Set 1 #

Patch Set 2 : Fixing test failures. #

Total comments: 5

Patch Set 3 : Added "IsConnectionOwner" method to determine connection controllers instead of explicitly looking … #

Patch Set 4 : Updating static methods and fixing test fakes. #

Total comments: 6
Unified diffs Side-by-side diffs Delta from patch set Stats (+435 lines, -10 lines) Patch
A + common/monsoon/monsoon/__init__.py View 1 chunk +4 lines, -8 lines 0 comments Download
A common/monsoon/monsoon/monsoon_utils.py View 1 1 chunk +62 lines, -0 lines 0 comments Download
A common/monsoon/monsoon/monsoon_wrapper.py View 1 chunk +124 lines, -0 lines 0 comments Download
M systrace/profile_chrome/fake_agent_1.py View 1 2 3 1 chunk +4 lines, -0 lines 2 comments Download
M systrace/profile_chrome/fake_agent_2.py View 1 2 3 1 chunk +4 lines, -0 lines 0 comments Download
M systrace/systrace/__init__.py View 1 chunk +1 line, -0 lines 0 comments Download
M systrace/systrace/run_systrace.py View 1 chunk +2 lines, -1 line 0 comments Download
M systrace/systrace/systrace_runner.py View 1 chunk +3 lines, -1 line 0 comments Download
M systrace/systrace/tracing_agents/__init__.py View 1 2 3 1 chunk +15 lines, -0 lines 3 comments Download
A systrace/systrace/tracing_agents/monsoon_agent.py View 1 2 3 1 chunk +185 lines, -0 lines 0 comments Download
M systrace/systrace/tracing_controller.py View 1 2 3 chunks +31 lines, -0 lines 1 comment Download

Messages

Total messages: 34 (21 generated)
ddeptford
This change utilizes the BattOr visualization with data from the Monsoon power monitor. The Monsoon ...
1 year, 2 months ago (2017-09-18 17:47:51 UTC) #10
Chris Craik
Mostly looks good, but would like to hear from Zhen about usb stop/restart stuff https://codereview.chromium.org/3018533002/diff/20001/systrace/systrace/tracing_agents/monsoon_agent.py ...
1 year, 2 months ago (2017-09-19 18:47:02 UTC) #11
Zhen Wang
Notice that Catapult is switching to Gerrit Code Review instead of Rietveld on 9/27. So ...
1 year, 2 months ago (2017-09-19 23:53:50 UTC) #12
Chris Craik
On 2017/09/19 at 23:53:50, zhenw wrote: > Notice that Catapult is switching to Gerrit Code ...
1 year, 2 months ago (2017-09-19 23:58:07 UTC) #13
Zhen Wang
> The other lifecycle methods I was imagining would be: > > 1 start tracing ...
1 year, 2 months ago (2017-09-20 00:04:42 UTC) #14
Chris Craik
Sounds good, let's just go with the 'isConnectionOwnerAgent' bool method that monsoon agent can override, ...
1 year, 2 months ago (2017-09-20 22:00:10 UTC) #15
ddeptford
https://codereview.chromium.org/3018533002/diff/20001/systrace/systrace/tracing_agents/monsoon_agent.py File systrace/systrace/tracing_agents/monsoon_agent.py (right): https://codereview.chromium.org/3018533002/diff/20001/systrace/systrace/tracing_agents/monsoon_agent.py#newcode134 systrace/systrace/tracing_agents/monsoon_agent.py:134: return True On 2017/09/19 18:47:02, Chris Craik wrote: > ...
1 year, 2 months ago (2017-09-20 23:45:58 UTC) #16
Chris Craik
LGTM, assuming there's a reason for the agent change, but please wait for LGTM from ...
1 year, 2 months ago (2017-09-26 17:57:35 UTC) #25
ddeptford
https://codereview.chromium.org/3018533002/diff/60001/systrace/profile_chrome/fake_agent_1.py File systrace/profile_chrome/fake_agent_1.py (right): https://codereview.chromium.org/3018533002/diff/60001/systrace/profile_chrome/fake_agent_1.py#newcode41 systrace/profile_chrome/fake_agent_1.py:41: # pylint: disable=no-self-use On 2017/09/26 17:57:35, Chris Craik wrote: ...
1 year, 2 months ago (2017-09-26 18:05:49 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/3018533002/60001
1 year, 2 months ago (2017-09-26 20:01:46 UTC) #28
commit-bot: I haz the power
Try jobs failed on following builders: Catapult Presubmit on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Presubmit/builds/9535)
1 year, 2 months ago (2017-09-26 20:06:18 UTC) #30
Zhen Wang
+simonhatch for common/ +charliea FYI Daniel, feel free to commit after Simon has taken a ...
1 year, 2 months ago (2017-09-26 21:32:09 UTC) #33
Zhen Wang
1 year, 2 months ago (2017-09-28 01:58:49 UTC) #34
https://codereview.chromium.org/3018533002/diff/60001/systrace/systrace/traci...
File systrace/systrace/tracing_agents/__init__.py (right):

https://codereview.chromium.org/3018533002/diff/60001/systrace/systrace/traci...
systrace/systrace/tracing_agents/__init__.py:93: # pylint: disable=no-self-use
Why do we need this pylint disable?

https://codereview.chromium.org/3018533002/diff/60001/systrace/systrace/traci...
systrace/systrace/tracing_agents/__init__.py:99: For example, a power monitoring
tracing agent may disable the USB port to
Please name Monsoon explicitly here. There is another power monitoring agent for
BattOr.

https://codereview.chromium.org/3018533002/diff/60001/systrace/systrace/traci...
systrace/systrace/tracing_agents/__init__.py:101: 
Can you also add that the connection owner will start last and stop earliest,
and USB disable/enable is done in StartAgentTracing/StopAgentTracing.

Also see comments in tracing_controller.py.

https://codereview.chromium.org/3018533002/diff/60001/systrace/systrace/traci...
File systrace/systrace/tracing_controller.py (right):

https://codereview.chromium.org/3018533002/diff/60001/systrace/systrace/traci...
systrace/systrace/tracing_controller.py:209: if not
self._connection_owner.StopCollectionWithClockSync(sync_id,
Do we still need this |StopCollectionWithClockSync| function. Can we just call
StopAgentTracing here for connection owner?

It seems to me that StartAgentTracing takes care of USB disable. Similarly,
StopAgentTracing should take care of USB enable. You can make a comment in
__init__.py.

Powered by Google App Engine
This is Rietveld 408576698