[Telemetry] Add MeasureMemory method to action_runner

[Telemetry] Add MeasureMemory method to action_runner

The method is intended to unify the way in which stories can request
memory dumps.

Design doc: https://docs.google.com/a/google.com/document/d/1mAXsOik83G5QSG8SKr0w-szkIzwH-6MQ-lZoIkPDzc4/edit?usp=sharing

BUG=chromium:625657
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/ff42a341b88b0b1c4c995d34160f259db16edde3

[perezju, 2016-07-07 11:14:09]

perezju@chromium.org changed reviewers:
+ eakuefner@chromium.org, nednguyen@google.com, petrcermak@chromium.org,
primiano@chromium.org

[perezju, 2016-07-07 11:14:10]

This is how the MeasureMemory method would look like. And see
https://codereview.chromium.org/2123713006 for an example of a client using it.

I'm sticking with "MeasureMemory" rather than just "Measure", to resit the
temptation of trying to generalize this too much for other metrics.

This first implementation is taken nearly verbatim from the current
memory_top_10_mobile.py, but I'm planning to iterate on it, making some
experiments to determine how different approaches would affect noise levels.

https://codereview.chromium.org/2123713005/diff/1/telemetry/telemetry/interna...
File telemetry/telemetry/internal/actions/action_runner.py (right):

https://codereview.chromium.org/2123713005/diff/1/telemetry/telemetry/interna...
telemetry/telemetry/internal/actions/action_runner.py:128:
self.Wait(DUMP_WAIT_TIME)
I'm really keen on removing these "waits". Started with them in place so we
don't diverge too much from the current behavior of memory.top_10_mobile.
Anyway, I'm doing some experiments to see if I can reduce or eliminate these
without affecting the measurements too much.

[petrcermak, 2016-07-07 13:01:32]

LGTM with 2 comments.

Thanks,
Petr

https://codereview.chromium.org/2123713005/diff/1/telemetry/telemetry/interna...
File telemetry/telemetry/internal/actions/action_runner.py (right):

https://codereview.chromium.org/2123713005/diff/1/telemetry/telemetry/interna...
telemetry/telemetry/internal/actions/action_runner.py:31: DUMP_WAIT_TIME = 3
I suppose this should be private (leading underscore)?

https://codereview.chromium.org/2123713005/diff/1/telemetry/telemetry/interna...
telemetry/telemetry/internal/actions/action_runner.py:120: control the
environment (force GCs, clear caches) before making the
nit: For consistency with the rest of this file, this and the next line should
be indented 4 spaces wrt line 119 (+2)

[nednguyen, 2016-07-07 20:55:36]

lgtm

Can you add test coverage?

[perezju, 2016-07-08 10:51:10]

Description was changed from

==========
[Telemetry] Add MeasureMemory method to action_runner

The method is intended to unify the way in which stories can request
memory dumps.

Design doc:
https://docs.google.com/a/google.com/document/d/1mAXsOik83G5QSG8SKr0w-szkIzwH...

BUG=chromium:#625657
==========

to

==========
[Telemetry] Add MeasureMemory method to action_runner

The method is intended to unify the way in which stories can request
memory dumps.

Design doc:
https://docs.google.com/a/google.com/document/d/1mAXsOik83G5QSG8SKr0w-szkIzwH...

BUG=chromium:625657
==========

[perezju, 2016-07-08 10:53:48]

I've added tests. PTAL.

https://codereview.chromium.org/2123713005/diff/1/telemetry/telemetry/interna...
File telemetry/telemetry/internal/actions/action_runner.py (right):

https://codereview.chromium.org/2123713005/diff/1/telemetry/telemetry/interna...
telemetry/telemetry/internal/actions/action_runner.py:31: DUMP_WAIT_TIME = 3
On 2016/07/07 13:01:32, petrcermak wrote:
> I suppose this should be private (leading underscore)?

Done.

https://codereview.chromium.org/2123713005/diff/1/telemetry/telemetry/interna...
telemetry/telemetry/internal/actions/action_runner.py:120: control the
environment (force GCs, clear caches) before making the
On 2016/07/07 13:01:32, petrcermak wrote:
> nit: For consistency with the rest of this file, this and the next line should
> be indented 4 spaces wrt line 119 (+2)

Done.

[nednguyen, 2016-07-08 11:15:00]

https://codereview.chromium.org/2123713005/diff/20001/telemetry/telemetry/int...
File telemetry/telemetry/internal/actions/action_runner_unittest.py (right):

https://codereview.chromium.org/2123713005/diff/20001/telemetry/telemetry/int...
telemetry/telemetry/internal/actions/action_runner_unittest.py:67: with
mock.patch.object(self._tab.browser, 'DumpMemory') as mock_method:
Can we not use mock here but instead assert the expected tracing event to be in
the trace when MeasureMemory is called at the right time? Even better if you
assert that the event is inside an async event that is created by console.time +
console.timeEnd

From my experience, tests that exercise code path similar to production usually
catch most problems.

[perezju, 2016-07-08 11:25:44]

https://codereview.chromium.org/2123713005/diff/20001/telemetry/telemetry/int...
File telemetry/telemetry/internal/actions/action_runner_unittest.py (right):

https://codereview.chromium.org/2123713005/diff/20001/telemetry/telemetry/int...
telemetry/telemetry/internal/actions/action_runner_unittest.py:67: with
mock.patch.object(self._tab.browser, 'DumpMemory') as mock_method:
On 2016/07/08 11:15:00, nednguyen (ooo til 7-11) wrote:
> Can we not use mock here but instead assert the expected tracing event to be
in
> the trace when MeasureMemory is called at the right time? Even better if you
> assert that the event is inside an async event that is created by console.time
+
> console.timeEnd
> 
> From my experience, tests that exercise code path similar to production
usually
> catch most problems.

Note that I'm only using mock to assert that *something is not called*. Or to
fake a "failed dump" in testWithFailedDump.

_testWithTracing below does not mock the memory dumper and we do get an actual
trace with an actual dump. As you suggest, I'll try to assert that the dump is
actually there when we expect it.

[nednguyen, 2016-07-08 11:30:37]

lgtm

https://codereview.chromium.org/2123713005/diff/20001/telemetry/telemetry/int...
File telemetry/telemetry/internal/actions/action_runner_unittest.py (right):

https://codereview.chromium.org/2123713005/diff/20001/telemetry/telemetry/int...
telemetry/telemetry/internal/actions/action_runner_unittest.py:67: with
mock.patch.object(self._tab.browser, 'DumpMemory') as mock_method:
On 2016/07/08 11:25:44, perezju wrote:
> On 2016/07/08 11:15:00, nednguyen (ooo til 7-11) wrote:
> > Can we not use mock here but instead assert the expected tracing event to be
> in
> > the trace when MeasureMemory is called at the right time? Even better if you
> > assert that the event is inside an async event that is created by
console.time
> +
> > console.timeEnd
> > 
> > From my experience, tests that exercise code path similar to production
> usually
> > catch most problems.
> 
> Note that I'm only using mock to assert that *something is not called*. Or to
> fake a "failed dump" in testWithFailedDump.

Oh I see. Sorry for not looking at this carefully :P
> 
> _testWithTracing below does not mock the memory dumper and we do get an actual
> trace with an actual dump. As you suggest, I'll try to assert that the dump is
> actually there when we expect it.
sgtm

[perezju, 2016-07-08 13:15:38]

The CQ bit was checked by perezju@chromium.org

[perezju, 2016-07-08 13:15:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
petrcermak@chromium.org, nednguyen@google.com
Link to the patchset: https://codereview.chromium.org/2123713005/#ps40001
(title: "assert dumps in trace")

[commit-bot: I haz the power, 2016-07-08 13:15:53]

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, 2016-07-08 13:28:37]

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

[commit-bot: I haz the power, 2016-07-08 13:28:39]

Try jobs failed on following builders:
  Catapult Mac Tryserver on master.tryserver.client.catapult (JOB_FAILED, no
build URL)

[perezju, 2016-07-08 13:38:21]

On 2016/07/08 13:28:39, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   Catapult Mac Tryserver on master.tryserver.client.catapult (JOB_FAILED, no
> build URL)

Hmm, how can I check from the platform whether I have root/admin access? It's
failing e.g. on Mac with:

Exception: Telemetry needs to run /usr/sbin/purge with elevated privileges, but
the setuid bit is not set and there is no interactive terminal for a prompt.
Please ask an administrator to set the setuid bit on this executable and ensure
that it is owned by a user with the necessary privileges. Aborting.

when trying to platform.FlushEntireSystemCache()

[nednguyen, 2016-07-08 13:50:39]

nednguyen@google.com changed reviewers:
+ dtu@chromium.org

[nednguyen, 2016-07-08 13:51:55]

On 2016/07/08 13:38:21, perezju wrote:
> On 2016/07/08 13:28:39, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   Catapult Mac Tryserver on master.tryserver.client.catapult (JOB_FAILED, no
> > build URL)
> 
> Hmm, how can I check from the platform whether I have root/admin access? It's
> failing e.g. on Mac with:
> 
> Exception: Telemetry needs to run /usr/sbin/purge with elevated privileges,
but
> the setuid bit is not set and there is no interactive terminal for a prompt.
> Please ask an administrator to set the setuid bit on this executable and
ensure
> that it is owned by a user with the necessary privileges. Aborting.
> 
> when trying to platform.FlushEntireSystemCache()

+Dave do you remember if we have way to check root privilege?

[nednguyen, 2016-07-12 15:42:33]

On 2016/07/08 13:51:55, nednguyen wrote:
> On 2016/07/08 13:38:21, perezju wrote:
> > On 2016/07/08 13:28:39, commit-bot: I haz the power wrote:
> > > Try jobs failed on following builders:
> > >   Catapult Mac Tryserver on master.tryserver.client.catapult (JOB_FAILED,
no
> > > build URL)
> > 
> > Hmm, how can I check from the platform whether I have root/admin access?
It's
> > failing e.g. on Mac with:
> > 
> > Exception: Telemetry needs to run /usr/sbin/purge with elevated privileges,
> but
> > the setuid bit is not set and there is no interactive terminal for a prompt.
> > Please ask an administrator to set the setuid bit on this executable and
> ensure
> > that it is owned by a user with the necessary privileges. Aborting.
> > 
> > when trying to platform.FlushEntireSystemCache()
> 
> +Dave do you remember if we have way to check root privilege?

Let disable the test on Mac for now & file a bug about the privillege problem.

[perezju, 2016-07-28 14:58:44]

The CQ bit was checked by perezju@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-28 14:58:46]

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, 2016-07-28 15:11:07]

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

[commit-bot: I haz the power, 2016-07-28 15:11:09]

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

[perezju, 2016-07-28 15:39:10]

On 2016/07/12 15:42:33, nednguyen wrote:
> On 2016/07/08 13:51:55, nednguyen wrote:
> > On 2016/07/08 13:38:21, perezju wrote:
> > > On 2016/07/08 13:28:39, commit-bot: I haz the power wrote:
> > > > Try jobs failed on following builders:
> > > >   Catapult Mac Tryserver on master.tryserver.client.catapult
(JOB_FAILED,
> no
> > > > build URL)
> > > 
> > > Hmm, how can I check from the platform whether I have root/admin access?
> It's
> > > failing e.g. on Mac with:
> > > 
> > > Exception: Telemetry needs to run /usr/sbin/purge with elevated
privileges,
> > but
> > > the setuid bit is not set and there is no interactive terminal for a
prompt.
> > > Please ask an administrator to set the setuid bit on this executable and
> > ensure
> > > that it is owned by a user with the necessary privileges. Aborting.
> > > 
> > > when trying to platform.FlushEntireSystemCache()
> > 
> > +Dave do you remember if we have way to check root privilege?
> 
> Let disable the test on Mac for now & file a bug about the privillege problem.

Hmm, I tried to prevent the issue by not flushing cashes if we're not sure that
we have root privilege. The test still fails on all platforms when trying to
import the memory dumps failing the assert at:

    # Either all processes have mmaps or none of them do.
    have_mmaps = set(dump.has_mmaps for dump in self._process_dumps)
    assert len(have_mmaps) == 1
   
https://github.com/catapult-project/catapult/blob/a8a6fb7ea065c2df8ae129ad05a...

Ned or Primiano, have you seen this before? What could be causing dumps to have
mmaps on some processes but not others?

[nednguyen, 2016-07-28 21:45:52]

On 2016/07/28 15:39:10, perezju wrote:
> On 2016/07/12 15:42:33, nednguyen wrote:
> > On 2016/07/08 13:51:55, nednguyen wrote:
> > > On 2016/07/08 13:38:21, perezju wrote:
> > > > On 2016/07/08 13:28:39, commit-bot: I haz the power wrote:
> > > > > Try jobs failed on following builders:
> > > > >   Catapult Mac Tryserver on master.tryserver.client.catapult
> (JOB_FAILED,
> > no
> > > > > build URL)
> > > > 
> > > > Hmm, how can I check from the platform whether I have root/admin access?
> > It's
> > > > failing e.g. on Mac with:
> > > > 
> > > > Exception: Telemetry needs to run /usr/sbin/purge with elevated
> privileges,
> > > but
> > > > the setuid bit is not set and there is no interactive terminal for a
> prompt.
> > > > Please ask an administrator to set the setuid bit on this executable and
> > > ensure
> > > > that it is owned by a user with the necessary privileges. Aborting.
> > > > 
> > > > when trying to platform.FlushEntireSystemCache()
> > > 
> > > +Dave do you remember if we have way to check root privilege?
> > 
> > Let disable the test on Mac for now & file a bug about the privillege
problem.
> 
> Hmm, I tried to prevent the issue by not flushing cashes if we're not sure
that
> we have root privilege. The test still fails on all platforms when trying to
> import the memory dumps failing the assert at:
> 
>     # Either all processes have mmaps or none of them do.
>     have_mmaps = set(dump.has_mmaps for dump in self._process_dumps)
>     assert len(have_mmaps) == 1
>    
>
https://github.com/catapult-project/catapult/blob/a8a6fb7ea065c2df8ae129ad05a...
> 
> Ned or Primiano, have you seen this before? What could be causing dumps to
have
> mmaps on some processes but not others?

Sorry, I don't know much about this. Primiano probably would know :P

[Primiano Tucci (use gerrit), 2016-07-29 15:43:30]

On 2016/07/28 21:45:52, nednguyen wrote:
> Sorry, I don't know much about this. Primiano probably would know :P

Talked offline with Juan. Very weird issue. For sure we don't have mmaps on
Mac/Win (in fact the mac/win bots are failing for another reason).
Instead it should never happen that a trace has some processes with mmaps and
some processes without.
I suggested Juan to add a print to spit out the trace from the bot.

[perezju, 2016-08-03 13:08:39]

The CQ bit was checked by perezju@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-03 13:08:47]

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, 2016-08-03 13:21:56]

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

[commit-bot: I haz the power, 2016-08-03 13:21:57]

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

[perezju, 2016-08-03 13:29:45]

The CQ bit was checked by perezju@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-03 13:29:47]

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, 2016-08-03 13:41:48]

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

[commit-bot: I haz the power, 2016-08-03 13:41:49]

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

[perezju, 2016-08-03 13:47:54]

The CQ bit was checked by perezju@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-03 13:47:58]

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, 2016-08-03 14:15:54]

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

[commit-bot: I haz the power, 2016-08-03 14:15:55]

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...)

[perezju, 2016-08-03 16:48:07]

The CQ bit was checked by perezju@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-03 16:48:15]

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, 2016-08-03 17:12:05]

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

[commit-bot: I haz the power, 2016-08-03 17:12:07]

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...)

[perezju, 2016-08-03 17:16:05]

On 2016/07/29 15:43:30, Primiano Tucci wrote:
> On 2016/07/28 21:45:52, nednguyen wrote:
> > Sorry, I don't know much about this. Primiano probably would know :P
> 
> Talked offline with Juan. Very weird issue. For sure we don't have mmaps on
> Mac/Win (in fact the mac/win bots are failing for another reason).
> Instead it should never happen that a trace has some processes with mmaps and
> some processes without.
> I suggested Juan to add a print to spit out the trace from the bot.

Ok, I've fixed some issues, but the puzzling problem remains on the linux and
android platforms. An example of an inconsistent global dump is:

--- GLOBAL MEMORY DUMP ---
- ProcessMemoryDumpEvent[id=0xafb4254c868a5896, pid=20688, has_mmaps=False]
- ProcessMemoryDumpEvent[id=0xafb4254c868a5896, pid=20696, has_mmaps=True]
- ProcessMemoryDumpEvent[id=0xafb4254c868a5896, pid=20648, has_mmaps=True]

And here the json of that trace:
https://x20web.corp.google.com/~perezju/stuff/testRealisticMode.json

I also see the following on the browser stdout.

  *************** BROWSER STANDARD OUTPUT ***************
  Xlib:  extension "RANDR" missing on display ":99".
  Xlib:  extension "RANDR" missing on display ":99".
  [20648:20648:0803/100429:ERROR:logging.h(813)] Failed to call method:
org.freedesktop.DBus.ObjectManager.GetManagedObjects: object_path= /:
org.freedesktop.DBus.Error.ServiceUnknown: The name org.bluez was not provided
by any .service files
  [20648:20648:0803/100429:ERROR:logging.h(813)] Failed to call method:
org.freedesktop.DBus.ObjectManager.GetManagedObjects: object_path= /:
org.freedesktop.DBus.Error.ServiceUnknown: The name org.bluez was not provided
by any .service files
  [20696:20696:0803/100429:ERROR:sandbox_linux.cc(334)] InitializeSandbox()
called with multiple threads in process gpu-process
  [20648:20671:0803/100429:ERROR:browser_gpu_channel_host_factory.cc(145)]
Failed to create channel.
  [20648:20648:0803/100429:ERROR:navigation_entry_screenshot_manager.cc(141)]
Invalid entry with unique id: 1

Petr, Primiano, any clues?

[Primiano Tucci (use gerrit), 2016-08-04 09:09:17]

Thanks a lot Juan for the Trace.
It seems an actual bug in the chromium codebase when it generates the dump. The
metric seems to behave correctly.
I can totally see a dump for pid 20688 that was supposed to be detailed but does
not contain mmaps!
-----
      {
        "name": "explicitly_triggered",
        "tts": 7870,
        "args": {
          "dumps": {
            "level_of_detail": "detailed",
          }
        },
        "pid": 20688,
        "ts": 889217228,
        "cat": "disabled-by-default-memory-infra",
        "tid": 0,
        "ph": "v",
        "id": "0xafb4254c868a5896"
      },
-----

The thing that is weird, is that the trace seems to be generated by a very old
memory-infra codebase. Is this a ref build by the chance?
I see  specifically things like:
"dump_provider.name": "ProcessMemoryTotals"
which IIRC ssid changed long time ago in
https://codereview.chromium.org/1629393003

[Primiano Tucci (use gerrit), 2016-08-04 09:11:17]

Yeah I think this is a problem with ref builds.
Any chance we can disable them or memory-infra while stable rolls to a better
version?
What's happening here is that the old code was somewhat racy, and you are
hitting that race here.

[perezju, 2016-08-04 09:50:03]

The CQ bit was checked by perezju@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-04 09:50:09]

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, 2016-08-04 10:14:03]

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

[commit-bot: I haz the power, 2016-08-04 10:14:03]

Dry run: This issue passed the CQ dry run.

[perezju, 2016-08-04 10:20:55]

The CQ bit was checked by perezju@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-04 10:21:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[perezju, 2016-08-04 10:24:46]

yay! finally all trybots are green.

Had a chat offline with petr and primiano; indeed the inconsistent mmaps are due
to some race conditions on the older chrome versions used for testing in the
catapult CQ. I've relaxed the condition for now, and filed a bug to make it
strict again when we have a recent enough version of Chrome as ref.

So, please take a look again, I would recommend comparing the latest against
patch set 4 which was the one which most of you have reviewed already.

[nednguyen, 2016-08-04 10:37:11]

On 2016/08/04 10:24:46, perezju wrote:
> yay! finally all trybots are green.
> 
> Had a chat offline with petr and primiano; indeed the inconsistent mmaps are
due
> to some race conditions on the older chrome versions used for testing in the
> catapult CQ. I've relaxed the condition for now, and filed a bug to make it
> strict again when we have a recent enough version of Chrome as ref.
> 
> So, please take a look again, I would recommend comparing the latest against
> patch set 4 which was the one which most of you have reviewed already.

Why not keep the condition the same and as @Disabled('reference') decorator?
That way this make sures TOT chrome must be correct.

[commit-bot: I haz the power, 2016-08-04 10:44:49]

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

[commit-bot: I haz the power, 2016-08-04 10:44:50]

Dry run: This issue passed the CQ dry run.

[perezju, 2016-08-04 11:06:43]

On 2016/08/04 10:37:11, nednguyen wrote:
> On 2016/08/04 10:24:46, perezju wrote:
> > yay! finally all trybots are green.
> > 
> > Had a chat offline with petr and primiano; indeed the inconsistent mmaps are
> due
> > to some race conditions on the older chrome versions used for testing in the
> > catapult CQ. I've relaxed the condition for now, and filed a bug to make it
> > strict again when we have a recent enough version of Chrome as ref.
> > 
> > So, please take a look again, I would recommend comparing the latest against
> > patch set 4 which was the one which most of you have reviewed already.
> 
> Why not keep the condition the same and as @Disabled('reference') decorator?
> That way this make sures TOT chrome must be correct.

Maybe I'm wrong, but I thought that this would effectively disable it on the
catapult CQ making the test close to pointless?

Also feels a bit wrong to me for action_runner.MeasureMemory to be testing the
integrity of the dumping mechanism; I think here we should be just testing that
its interface works as advertised. Maybe we need some other tests for the mmaps
consistency in global dumps closer to where those promises are made?

[nednguyen, 2016-08-04 12:12:40]

On 2016/08/04 11:06:43, perezju wrote:
> On 2016/08/04 10:37:11, nednguyen wrote:
> > On 2016/08/04 10:24:46, perezju wrote:
> > > yay! finally all trybots are green.
> > > 
> > > Had a chat offline with petr and primiano; indeed the inconsistent mmaps
are
> > due
> > > to some race conditions on the older chrome versions used for testing in
the
> > > catapult CQ. I've relaxed the condition for now, and filed a bug to make
it
> > > strict again when we have a recent enough version of Chrome as ref.
> > > 
> > > So, please take a look again, I would recommend comparing the latest
against
> > > patch set 4 which was the one which most of you have reviewed already.
> > 
> > Why not keep the condition the same and as @Disabled('reference') decorator?
> > That way this make sures TOT chrome must be correct.
> 
> Maybe I'm wrong, but I thought that this would effectively disable it on the
> catapult CQ making the test close to pointless?

We run telemetry tests on chromium CQ against TOT browser through
tools/perf/run_telemetry_tests script.
> 
> Also feels a bit wrong to me for action_runner.MeasureMemory to be testing the
> integrity of the dumping mechanism; I think here we should be just testing
that
> its interface works as advertised. Maybe we need some other tests for the
mmaps
> consistency in global dumps closer to where those promises are made?

Splitting the unittest so it's more "unit" sound good to me :-)

[perezju, 2016-08-04 12:49:09]

On 2016/08/04 12:12:40, nednguyen wrote:
> On 2016/08/04 11:06:43, perezju wrote:
> > Maybe I'm wrong, but I thought that this would effectively disable it on the
> > catapult CQ making the test close to pointless?
> 
> We run telemetry tests on chromium CQ against TOT browser through
> tools/perf/run_telemetry_tests script.

OK. I was confused about that. I've uploaded a patch then to leave the original
check and disable on reference. We still loose a bit of coverage of catapult
bugs that could fail on chromium CQ; but I guess we had to give in somewhere.

> Splitting the unittest so it's more "unit" sound good to me :-)

Will think about that for follow up CLs.

[perezju, 2016-08-04 12:49:13]

The CQ bit was checked by perezju@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-04 12:49:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[nednguyen, 2016-08-04 12:56:36]

lgtm

[petrcermak, 2016-08-04 13:13:38]

Still LGTM with a few more comments.

Thanks,
Petr

https://codereview.chromium.org/2123713005/diff/240001/telemetry/telemetry/in...
File telemetry/telemetry/internal/actions/action_runner_unittest.py (right):

https://codereview.chromium.org/2123713005/diff/240001/telemetry/telemetry/in...
telemetry/telemetry/internal/actions/action_runner_unittest.py:71: def
_testWithTracing(self, deterministic_mode=False):
nit: I think that this should actually be _TestWithTracing now (since it's not a
proper test method)

https://codereview.chromium.org/2123713005/diff/240001/telemetry/telemetry/in...
telemetry/telemetry/internal/actions/action_runner_unittest.py:83: # If
successful, check that we can find our dump in the trace.
How can this be "not-successful" when you have the assertion on line 84? The
current wording suggests that "not successful" is fine (from the point of view
of the test).

https://codereview.chromium.org/2123713005/diff/240001/telemetry/telemetry/in...
telemetry/telemetry/internal/actions/action_runner_unittest.py:93:
self._testWithTracing(deterministic_mode=True)
Check that GC was called?

https://codereview.chromium.org/2123713005/diff/240001/telemetry/telemetry/in...
File telemetry/telemetry/internal/platform/android_platform_backend.py (right):

https://codereview.chromium.org/2123713005/diff/240001/telemetry/telemetry/in...
telemetry/telemetry/internal/platform/android_platform_backend.py:335: def
CanElevatePrivilege(self):
Why don't you expose this as a @property (since it's just a bool)?

[commit-bot: I haz the power, 2016-08-04 13:14:34]

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

[commit-bot: I haz the power, 2016-08-04 13:14:35]

Dry run: This issue passed the CQ dry run.

[perezju, 2016-08-04 14:07:35]

https://codereview.chromium.org/2123713005/diff/240001/telemetry/telemetry/in...
File telemetry/telemetry/internal/actions/action_runner_unittest.py (right):

https://codereview.chromium.org/2123713005/diff/240001/telemetry/telemetry/in...
telemetry/telemetry/internal/actions/action_runner_unittest.py:83: # If
successful, check that we can find our dump in the trace.
On 2016/08/04 13:13:38, petrcermak wrote:
> How can this be "not-successful" when you have the assertion on line 84? The
> current wording suggests that "not successful" is fine (from the point of view
> of the test).

If not successful, then testWithFailedDump checks that an exception would have
been raised. And we never reach this point.

I'll try to reword this.

https://codereview.chromium.org/2123713005/diff/240001/telemetry/telemetry/in...
telemetry/telemetry/internal/actions/action_runner_unittest.py:93:
self._testWithTracing(deterministic_mode=True)
On 2016/08/04 13:13:38, petrcermak wrote:
> Check that GC was called?

But then I would need to mock the ForceGarbageCollection call, and Ned was keen
on having this "exercise real code paths". I could make the mock *also* call the
actual thing being mocked, but that starts feeling a bit messy.

https://codereview.chromium.org/2123713005/diff/240001/telemetry/telemetry/in...
File telemetry/telemetry/internal/platform/android_platform_backend.py (right):

https://codereview.chromium.org/2123713005/diff/240001/telemetry/telemetry/in...
telemetry/telemetry/internal/platform/android_platform_backend.py:335: def
CanElevatePrivilege(self):
On 2016/08/04 13:13:38, petrcermak wrote:
> Why don't you expose this as a @property (since it's just a bool)?

Just for consistency, e.g. there already are CanMonitorPower,
CanMonitorNetworkData, etc.

[perezju, 2016-08-04 14:10:54]

The CQ bit was checked by perezju@chromium.org

[perezju, 2016-08-04 14:10:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
nednguyen@google.com, petrcermak@chromium.org
Link to the patchset: https://codereview.chromium.org/2123713005/#ps260001
(title: "rephrase comment")

[commit-bot: I haz the power, 2016-08-04 14:11:04]

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, 2016-08-04 15:13:22]

Description was changed from

==========
[Telemetry] Add MeasureMemory method to action_runner

The method is intended to unify the way in which stories can request
memory dumps.

Design doc:
https://docs.google.com/a/google.com/document/d/1mAXsOik83G5QSG8SKr0w-szkIzwH...

BUG=chromium:625657
==========

to

==========
[Telemetry] Add MeasureMemory method to action_runner

The method is intended to unify the way in which stories can request
memory dumps.

Design doc:
https://docs.google.com/a/google.com/document/d/1mAXsOik83G5QSG8SKr0w-szkIzwH...

BUG=chromium:625657

Committed:
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
==========

[commit-bot: I haz the power, 2016-08-04 15:13:24]

Committed patchset #14 (id:260001) as
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...