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

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

[ddeptford, 2017-09-15 23:38:23]

The CQ bit was checked by ddeptford@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-09-15 23:38:27]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-09-15 23:44:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-09-15 23:44:58]

Dry run: Try jobs failed on following builders:
  Catapult Android Tryserver on master.tryserver.client.catapult (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20An...)

[ddeptford, 2017-09-16 00:04:09]

The CQ bit was checked by ddeptford@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-09-16 00:04:16]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Chris Craik, 2017-09-16 00:07:40]

ccraik@google.com changed reviewers:
+ ccraik@google.com, jreck@google.com, zhenw@chromium.org

[commit-bot: I haz the power, 2017-09-16 00:36:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-09-16 00:36:11]

Dry run: This issue passed the CQ dry run.

[ddeptford, 2017-09-18 17:47:51]

This change utilizes the BattOr visualization with data from the Monsoon power
monitor.  The Monsoon script used with data capture is from
/master/tools/utils/monsoon.py within Android (not the newer HVPM/LVPM toolset
from PyMonsoon).

Tracing in Monsoon mode disables USB while capture is in progress as not to
interfere with power measurement.  When tracing is complete, the monsoon tracer
is stopped first which enables USB connectivity for the rest of the tracers to
stop.

Data capture latency error is limited by the start time of Monsoon capture, the
inaccuracy of monsoon.py's reporting (integer second accuracy) and local USB
latency dependent upon system configuration and physical USB connections.  Since
the latter of these are locally dependent, an option is given to provide a
latency offset for monsoon data.

[Chris Craik, 2017-09-19 18:47:02]

Mostly looks good, but would like to hear from Zhen about usb stop/restart stuff

https://codereview.chromium.org/3018533002/diff/20001/systrace/systrace/traci...
File systrace/systrace/tracing_agents/monsoon_agent.py (right):

https://codereview.chromium.org/3018533002/diff/20001/systrace/systrace/traci...
systrace/systrace/tracing_agents/monsoon_agent.py:134: return True
can you do the actual stop here? we'd like to avoid stop happening in
getResults, if possible. it makes it so all the stops, and all of the getresults
happen simultaneously

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

https://codereview.chromium.org/3018533002/diff/20001/systrace/systrace/traci...
systrace/systrace/tracing_controller.py:125: # If we have a monsoon power
monitor controller in the group, make an
I'm not especially happy with this, but I don't have other ideas. Zhen,
thoughts? May be nice to abstract this to a "connectionOwnerAgent" or something
like that, instead of hard coding this as a monsoon agent, then have only
monsoon return 'true' from an isConnectionOwner method.

Zhen, do we know if there are other agents that expect to use USB while tracing?

Alternately, we could consider another 'lifecycle' callback that happens after
all the starts, and before all the stops, and monsoon could put its code there.

[Zhen Wang, 2017-09-19 23:53:50]

Notice that Catapult is switching to Gerrit Code Review instead of Rietveld on
9/27. So maybe migrate this CL to Gerrit.

See:
https://groups.google.com/a/google.com/forum/#!topic/chrome-speed-operations/...

https://codereview.chromium.org/3018533002/diff/20001/systrace/systrace/traci...
File systrace/systrace/tracing_agents/monsoon_agent.py (right):

https://codereview.chromium.org/3018533002/diff/20001/systrace/systrace/traci...
systrace/systrace/tracing_agents/monsoon_agent.py:134: return True
On 2017/09/19 18:47:02, Chris Craik wrote:
> can you do the actual stop here? we'd like to avoid stop happening in
> getResults, if possible. it makes it so all the stops, and all of the
getresults
> happen simultaneously

Seem that the actual stopping is done in StopCollectionWithClockSync, which is
specific to Monsoon only. And in current patch, it is explicitly called before
other agents are stopped.

This can be solved if we have a better way of controlling the USB connection.

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

https://codereview.chromium.org/3018533002/diff/20001/systrace/systrace/traci...
systrace/systrace/tracing_controller.py:125: # If we have a monsoon power
monitor controller in the group, make an
On 2017/09/19 18:47:02, Chris Craik wrote:
> I'm not especially happy with this, but I don't have other ideas. Zhen,
> thoughts? May be nice to abstract this to a "connectionOwnerAgent" or
something
> like that, instead of hard coding this as a monsoon agent, then have only
> monsoon return 'true' from an isConnectionOwner method.
> 
> Zhen, do we know if there are other agents that expect to use USB while
tracing?
> 
> Alternately, we could consider another 'lifecycle' callback that happens after
> all the starts, and before all the stops, and monsoon could put its code
there.

I see --usbpassthrough is set to off for monsoon. Looks like it also affects
other agents?

For systrace and adb_profile_chrome, I think agents do not need to use USB while
tracing. For Telemetry, it will need USB, but that has separate controlling
logic. So it should be fine.

Another lifecycle callback may not work well. It seems that the sequence is:
1. start tracing for other agents
2. disable usb
3. start tracing for monsoon
So it seems that monsoon's start cannot be grouped into step 1. 

ConnectionOwnerAgent is probably the only way, so it will always start last and
stop first.

[Chris Craik, 2017-09-19 23:58:07]

On 2017/09/19 at 23:53:50, zhenw wrote:
> Notice that Catapult is switching to Gerrit Code Review instead of Rietveld on
9/27. So maybe migrate this CL to Gerrit.
> 
> See:
https://groups.google.com/a/google.com/forum/#!topic/chrome-speed-operations/...
> 
>
https://codereview.chromium.org/3018533002/diff/20001/systrace/systrace/traci...
> File systrace/systrace/tracing_agents/monsoon_agent.py (right):
> 
>
https://codereview.chromium.org/3018533002/diff/20001/systrace/systrace/traci...
> systrace/systrace/tracing_agents/monsoon_agent.py:134: return True
> On 2017/09/19 18:47:02, Chris Craik wrote:
> > can you do the actual stop here? we'd like to avoid stop happening in
> > getResults, if possible. it makes it so all the stops, and all of the
getresults
> > happen simultaneously
> 
> Seem that the actual stopping is done in StopCollectionWithClockSync, which is
specific to Monsoon only. And in current patch, it is explicitly called before
other agents are stopped.
> 
> This can be solved if we have a better way of controlling the USB connection.
> 
>
https://codereview.chromium.org/3018533002/diff/20001/systrace/systrace/traci...
> File systrace/systrace/tracing_controller.py (right):
> 
>
https://codereview.chromium.org/3018533002/diff/20001/systrace/systrace/traci...
> systrace/systrace/tracing_controller.py:125: # If we have a monsoon power
monitor controller in the group, make an
> On 2017/09/19 18:47:02, Chris Craik wrote:
> > I'm not especially happy with this, but I don't have other ideas. Zhen,
> > thoughts? May be nice to abstract this to a "connectionOwnerAgent" or
something
> > like that, instead of hard coding this as a monsoon agent, then have only
> > monsoon return 'true' from an isConnectionOwner method.
> > 
> > Zhen, do we know if there are other agents that expect to use USB while
tracing?
> > 
> > Alternately, we could consider another 'lifecycle' callback that happens
after
> > all the starts, and before all the stops, and monsoon could put its code
there.
> 
> I see --usbpassthrough is set to off for monsoon. Looks like it also affects
other agents?
> 
> For systrace and adb_profile_chrome, I think agents do not need to use USB
while tracing. For Telemetry, it will need USB, but that has separate
controlling logic. So it should be fine.
> 
> Another lifecycle callback may not work well. It seems that the sequence is:
> 1. start tracing for other agents
> 2. disable usb
> 3. start tracing for monsoon
> So it seems that monsoon's start cannot be grouped into step 1. 
> 
> ConnectionOwnerAgent is probably the only way, so it will always start last
and stop first.

The other lifecycle methods I was imagining would be:

1 start tracing for all agents (monsoon ignores)
2 monsoon disables usb, and starts tracing (maybe call this 'startInnerTracing'
or something)
3 ...
4 monsoon ends tracing, reenables USB (maybe 'stopInnerTracing')
5 stop tracing callback for all agents (monsoon ignores)

[Zhen Wang, 2017-09-20 00:04:42]

> The other lifecycle methods I was imagining would be:
> 
> 1 start tracing for all agents (monsoon ignores)
> 2 monsoon disables usb, and starts tracing (maybe call this
'startInnerTracing'
> or something)
> 3 ...
> 4 monsoon ends tracing, reenables USB (maybe 'stopInnerTracing')
> 5 stop tracing callback for all agents (monsoon ignores)

I see. Probably more general as following:

1. StartTracingBeforeDisconnection
2. Disconnect
3. StartTracingAfterDisconnection
4 [tracing is running]
5. StopTracingBeforeReconnection
6. Reconnect
7. StopTracingAfterReconnection

Do we expect to have more than 1 agent that wants to disconnect before starting
tracing in the future? If not, it is probably simpler to just have something
like ConnectionOwnerAgent, because there is no API change.

[Chris Craik, 2017-09-20 22:00:10]

Sounds good, let's just go with the 'isConnectionOwnerAgent' bool method that
monsoon agent can override, and then have the tracing controller just operate on
that.

Daniel, if you want to go ahead and make that update to the CL here, we can
probably get it in before the gerrit switchover.

Or feel free to move to gerrit, whichever's fine.

[ddeptford, 2017-09-20 23:45:58]

https://codereview.chromium.org/3018533002/diff/20001/systrace/systrace/traci...
File systrace/systrace/tracing_agents/monsoon_agent.py (right):

https://codereview.chromium.org/3018533002/diff/20001/systrace/systrace/traci...
systrace/systrace/tracing_agents/monsoon_agent.py:134: return True
On 2017/09/19 18:47:02, Chris Craik wrote:
> can you do the actual stop here? we'd like to avoid stop happening in
> getResults, if possible. it makes it so all the stops, and all of the
getresults
> happen simultaneously

Unfortunately this is a limitation of the monsoon script being in a subprocess -
it cannot re-enable USB while collecting power data from within the same
subprocess as sampling, and the sampling subprocess retains a lock on the serial
device.  USB needs to be re-enabled prior to clock syncs being issued, which
happens prior to this method being called in the controller.

Hopefully this could change in the future if we integrate PyMonsoon (the
official Monsoon python library which also supports newer hardware).

[ddeptford, 2017-09-21 01:22:07]

The CQ bit was checked by ddeptford@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-09-21 01:22:10]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-09-21 01:39:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-09-21 01:39:21]

Dry run: Try jobs failed on following builders:
  Catapult Android Tryserver on master.tryserver.client.catapult (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20An...)

[ddeptford, 2017-09-22 19:22:47]

The CQ bit was checked by ddeptford@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-09-22 19:22:51]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-09-22 20:55:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-09-22 20:55:43]

Dry run: This issue passed the CQ dry run.

[Chris Craik, 2017-09-26 17:57:35]

LGTM, assuming there's a reason for the agent change, but please wait for LGTM
from Zhen

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

https://codereview.chromium.org/3018533002/diff/60001/systrace/profile_chrome...
systrace/profile_chrome/fake_agent_1.py:41: # pylint: disable=no-self-use
why override in the fake agents?

[ddeptford, 2017-09-26 18:05:49]

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

https://codereview.chromium.org/3018533002/diff/60001/systrace/profile_chrome...
systrace/profile_chrome/fake_agent_1.py:41: # pylint: disable=no-self-use
On 2017/09/26 17:57:35, Chris Craik wrote:
> why override in the fake agents?

The fake agents don't inherit from the common base class.

[ddeptford, 2017-09-26 20:01:40]

The CQ bit was checked by ddeptford@google.com

[commit-bot: I haz the power, 2017-09-26 20:01:46]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-09-26 20:06:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-09-26 20:06:18]

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%20Pr...)

[ddeptford, 2017-09-26 20:08:04]

ddeptford@google.com changed reviewers:
+ skyostil@chromium.org

[Zhen Wang, 2017-09-26 21:32:08]

zhenw@chromium.org changed reviewers:
+ charliea@chromium.org, simonhatch@chromium.org

[Zhen Wang, 2017-09-26 21:32:09]

+simonhatch for common/
+charliea FYI

Daniel, feel free to commit after Simon has taken a look for common/. I will
take a look later once I get some time.

[Zhen Wang, 2017-09-28 01:58:49]

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.