[system-health] Add support for disabling individual stories on individual platforms

[system-health] Add support for disabling individual stories on individual platforms

This patch adds support for enabling/disabling individual stories on individual
platforms using the same approaches as for benchmarks:

  1. Disabled/Enabled decorator:

     @decorators.Disabled('win')
     class Story(system_health_story.SystemHealthStory):
        ...

  2. ShouldDisable method:

     class Story(system_health_story.SystemHealthStory):
        ...

        @classmethod
        def ShouldDisable(cls, possible_browser):
          return possible_browser.platform.GetOSName() == 'win'

Note that this patch also enables search:portal:google on all desktop
platforms except for Windows.

BUG=634331
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq

Committed: https://crrev.com/0d3d85c5f3a772dd65b1eae53c890d7251dee0ab
Cr-Commit-Position: refs/heads/master@{#417552}

[petrcermak, 2016-08-09 17:51:12]

Description was changed from

==========
[system-health] Add support for disabling individual stories on individual
platforms

BUG=634331
==========

to

==========
[system-health] Add support for disabling individual stories on individual
platforms

This patch adds support for enabling/disabling individual stories on individual
platforms using the same approaches as for benchmarks:

  1. Disabled/Enabled decorator:

     @decorators.Disabled('win')
     class Story(system_health_story.SystemHealthStory):
        ...

  2. ShouldDisable method:

     class Story(system_health_story.SystemHealthStory):
        ...

        @classmethod
        def ShouldDisable(cls, possible_browser):
          return possible_browser.platform.GetOSName() == 'win'

Note that this patch also enables search:portal:google on all desktop
platforms except for Windows.

BUG=634331
==========

[petrcermak, 2016-08-09 17:51:19]

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

[petrcermak, 2016-08-09 17:51:32]

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

[petrcermak, 2016-08-09 17:51:33]

PTAL.

Thanks,
Petr

[commit-bot: I haz the power, 2016-08-09 17:51: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-09 20:18:38]

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

[commit-bot: I haz the power, 2016-08-09 20:18:38]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[petrcermak, 2016-08-10 09:33:25]

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

[commit-bot: I haz the power, 2016-08-10 09:33:42]

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-10 10:52:06]

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

[commit-bot: I haz the power, 2016-08-10 10:52:07]

Dry run: This issue passed the CQ dry run.

[petrcermak, 2016-08-10 11:06:12]

Description was changed from

==========
[system-health] Add support for disabling individual stories on individual
platforms

This patch adds support for enabling/disabling individual stories on individual
platforms using the same approaches as for benchmarks:

  1. Disabled/Enabled decorator:

     @decorators.Disabled('win')
     class Story(system_health_story.SystemHealthStory):
        ...

  2. ShouldDisable method:

     class Story(system_health_story.SystemHealthStory):
        ...

        @classmethod
        def ShouldDisable(cls, possible_browser):
          return possible_browser.platform.GetOSName() == 'win'

Note that this patch also enables search:portal:google on all desktop
platforms except for Windows.

BUG=634331
==========

to

==========
[system-health] Add support for disabling individual stories on individual
platforms

This patch adds support for enabling/disabling individual stories on individual
platforms using the same approaches as for benchmarks:

  1. Disabled/Enabled decorator:

     @decorators.Disabled('win')
     class Story(system_health_story.SystemHealthStory):
        ...

  2. ShouldDisable method:

     class Story(system_health_story.SystemHealthStory):
        ...

        @classmethod
        def ShouldDisable(cls, possible_browser):
          return possible_browser.platform.GetOSName() == 'win'

Note that this patch also enables search:portal:google on all desktop
platforms except for Windows.

BUG=634331
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq
==========

[petrcermak, 2016-08-10 11:06:20]

The CQ bit was checked by petrcermak@chromium.org

[petrcermak, 2016-08-10 11:06:22]

The CQ bit was unchecked by petrcermak@chromium.org

[petrcermak, 2016-08-10 11:06:32]

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

[commit-bot: I haz the power, 2016-08-10 11:06: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-10 13:11:31]

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

[commit-bot: I haz the power, 2016-08-10 13:11:32]

Dry run: This issue passed the CQ dry run.

[nednguyen, 2016-08-10 13:15:00]

On 2016/08/09 17:51:33, petrcermak wrote:
> PTAL.
> 
> Thanks,
> Petr

Currently there is a bug with the decorator
(https://bugs.chromium.org/p/chromium/issues/detail?id=568333)

If you have B subclass from A & B has the decorator on it, A is also affected
:-(

The overall change lgtm though, but we should wait for issue 568333 to be fixed?

[petrcermak, 2016-08-10 13:26:19]

On 2016/08/10 13:15:00, nednguyen wrote:
> On 2016/08/09 17:51:33, petrcermak wrote:
> > PTAL.
> > 
> > Thanks,
> > Petr
> 
> Currently there is a bug with the decorator
> (https://bugs.chromium.org/p/chromium/issues/detail?id=568333)
> 
> If you have B subclass from A & B has the decorator on it, A is also affected
> :-(
> 
> The overall change lgtm though, but we should wait for issue 568333 to be
fixed?

Sure, but is there an ETA?

[nednguyen, 2016-08-10 13:28:03]

On 2016/08/10 13:26:19, petrcermak wrote:
> On 2016/08/10 13:15:00, nednguyen wrote:
> > On 2016/08/09 17:51:33, petrcermak wrote:
> > > PTAL.
> > > 
> > > Thanks,
> > > Petr
> > 
> > Currently there is a bug with the decorator
> > (https://bugs.chromium.org/p/chromium/issues/detail?id=568333)
> > 
> > If you have B subclass from A & B has the decorator on it, A is also
affected
> > :-(
> > 
> > The overall change lgtm though, but we should wait for issue 568333 to be
> fixed?
> 
> Sure, but is there an ETA?

Kari was on hook for it, I need to check whether she can still take it.

[petrcermak, 2016-09-07 10:21:00]

Patchset #3 (id:40001) has been deleted

[petrcermak, 2016-09-07 10:25:45]

Patchset #3 (id:60001) has been deleted

[petrcermak, 2016-09-07 10:26:47]

I think we can now land this since
https://bugs.chromium.org/p/chromium/issues/detail?id=568333 has been fixed.

Ned: Could you please do a final check of the patch (it's been a while since you
approved it)?

Thanks,
Petr

[nednguyen, 2016-09-07 18:23:51]

https://codereview.chromium.org/2228103002/diff/80001/tools/perf/page_sets/sy...
File tools/perf/page_sets/system_health/system_health_story.py (right):

https://codereview.chromium.org/2228103002/diff/80001/tools/perf/page_sets/sy...
tools/perf/page_sets/system_health/system_health_story.py:41: return
story.CanRun(self.browser)
Hmhh, the api is CanRun(possible_browser), and not browser?

[petrcermak, 2016-09-08 10:20:44]

https://codereview.chromium.org/2228103002/diff/80001/tools/perf/page_sets/sy...
File tools/perf/page_sets/system_health/system_health_story.py (right):

https://codereview.chromium.org/2228103002/diff/80001/tools/perf/page_sets/sy...
tools/perf/page_sets/system_health/system_health_story.py:41: return
story.CanRun(self.browser)
On 2016/09/07 18:23:51, nednguyen wrote:
> Hmhh, the api is CanRun(possible_browser), and not browser?

Yeah. The reason for this is that we are re-using decorators.ShouldSkip
(https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry...),
which expects a *possible* browser. Also, we want to mirror the ShouldDisable
method of benchmarks, which also expects a *possible* browser. As far as I can
tell, there's no way to get a PossibleBrowser from a Browser. However, since
Browser has almost all fields exposed by PossibleBrowser, I don't think this is
a major problem. If you disagree, let me know how you think I should proceed ;-)

[nednguyen, 2016-09-08 12:56:22]

https://codereview.chromium.org/2228103002/diff/80001/tools/perf/page_sets/sy...
File tools/perf/page_sets/system_health/system_health_story.py (right):

https://codereview.chromium.org/2228103002/diff/80001/tools/perf/page_sets/sy...
tools/perf/page_sets/system_health/system_health_story.py:41: return
story.CanRun(self.browser)
On 2016/09/08 10:20:44, petrcermak wrote:
> On 2016/09/07 18:23:51, nednguyen wrote:
> > Hmhh, the api is CanRun(possible_browser), and not browser?
> 
> Yeah. The reason for this is that we are re-using decorators.ShouldSkip
>
(https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry...),
> which expects a *possible* browser. Also, we want to mirror the ShouldDisable
> method of benchmarks, which also expects a *possible* browser. As far as I can
> tell, there's no way to get a PossibleBrowser from a Browser. However, since
> Browser has almost all fields exposed by PossibleBrowser, I don't think this
is
> a major problem. If you disagree, let me know how you think I should proceed
;-)

We do keep the possible_browser in
https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry...


Why not use it?

[petrcermak, 2016-09-08 18:45:10]

PTAL.

Thanks,
Petr

https://codereview.chromium.org/2228103002/diff/80001/tools/perf/page_sets/sy...
File tools/perf/page_sets/system_health/system_health_story.py (right):

https://codereview.chromium.org/2228103002/diff/80001/tools/perf/page_sets/sy...
tools/perf/page_sets/system_health/system_health_story.py:41: return
story.CanRun(self.browser)
On 2016/09/08 12:56:22, nednguyen wrote:
> On 2016/09/08 10:20:44, petrcermak wrote:
> > On 2016/09/07 18:23:51, nednguyen wrote:
> > > Hmhh, the api is CanRun(possible_browser), and not browser?
> > 
> > Yeah. The reason for this is that we are re-using decorators.ShouldSkip
> >
>
(https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry...),
> > which expects a *possible* browser. Also, we want to mirror the
ShouldDisable
> > method of benchmarks, which also expects a *possible* browser. As far as I
can
> > tell, there's no way to get a PossibleBrowser from a Browser. However, since
> > Browser has almost all fields exposed by PossibleBrowser, I don't think this
> is
> > a major problem. If you disagree, let me know how you think I should proceed
> ;-)
> 
> We do keep the possible_browser in
>
https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry...
> 
> 
> Why not use it?

Done.

[nednguyen, 2016-09-08 19:31:16]

lgtm

https://codereview.chromium.org/2228103002/diff/120001/tools/perf/page_sets/s...
File tools/perf/page_sets/system_health/platforms.py (right):

https://codereview.chromium.org/2228103002/diff/120001/tools/perf/page_sets/s...
tools/perf/page_sets/system_health/platforms.py:15: NO_PLATFORMS = frozenset()
Why do you add this back? I thought your decision was removing this is right :-)

[petrcermak, 2016-09-09 09:34:09]

https://codereview.chromium.org/2228103002/diff/120001/tools/perf/page_sets/s...
File tools/perf/page_sets/system_health/platforms.py (right):

https://codereview.chromium.org/2228103002/diff/120001/tools/perf/page_sets/s...
tools/perf/page_sets/system_health/platforms.py:15: NO_PLATFORMS = frozenset()
On 2016/09/08 19:31:16, nednguyen wrote:
> Why do you add this back? I thought your decision was removing this is right
:-)

Good question. The reason is that it needs to be used for stories which don't
have a WPR recording (e.g. FacebookPhotosDesktopStory in browsing_stories.py).

If I just used @decorators.Disabled('all') on the story, the benchmark would
fail because of "missing WPR archive" (even though the story would never
actually run).

Maybe the story_runner logic could be updated to handle such cases properly, but
it might might be difficult in the general case (shared_state.CanRun() can
depend on the current state of things, so it might not be possible to tell
upfront if a story will run; downloading the WPR archives on demand could, on
the other hand, add noise to benchmarks which don't tear down the browser
between individual stories) and is probably not be worth the effort.

[petrcermak, 2016-09-09 09:34:16]

The CQ bit was checked by petrcermak@chromium.org

[commit-bot: I haz the power, 2016-09-09 09:34:30]

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-09-09 10:42:08]

Description was changed from

==========
[system-health] Add support for disabling individual stories on individual
platforms

This patch adds support for enabling/disabling individual stories on individual
platforms using the same approaches as for benchmarks:

  1. Disabled/Enabled decorator:

     @decorators.Disabled('win')
     class Story(system_health_story.SystemHealthStory):
        ...

  2. ShouldDisable method:

     class Story(system_health_story.SystemHealthStory):
        ...

        @classmethod
        def ShouldDisable(cls, possible_browser):
          return possible_browser.platform.GetOSName() == 'win'

Note that this patch also enables search:portal:google on all desktop
platforms except for Windows.

BUG=634331
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq
==========

to

==========
[system-health] Add support for disabling individual stories on individual
platforms

This patch adds support for enabling/disabling individual stories on individual
platforms using the same approaches as for benchmarks:

  1. Disabled/Enabled decorator:

     @decorators.Disabled('win')
     class Story(system_health_story.SystemHealthStory):
        ...

  2. ShouldDisable method:

     class Story(system_health_story.SystemHealthStory):
        ...

        @classmethod
        def ShouldDisable(cls, possible_browser):
          return possible_browser.platform.GetOSName() == 'win'

Note that this patch also enables search:portal:google on all desktop
platforms except for Windows.

BUG=634331
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq
==========

[commit-bot: I haz the power, 2016-09-09 10:42:09]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2016-09-09 10:44:15]

Description was changed from

==========
[system-health] Add support for disabling individual stories on individual
platforms

This patch adds support for enabling/disabling individual stories on individual
platforms using the same approaches as for benchmarks:

  1. Disabled/Enabled decorator:

     @decorators.Disabled('win')
     class Story(system_health_story.SystemHealthStory):
        ...

  2. ShouldDisable method:

     class Story(system_health_story.SystemHealthStory):
        ...

        @classmethod
        def ShouldDisable(cls, possible_browser):
          return possible_browser.platform.GetOSName() == 'win'

Note that this patch also enables search:portal:google on all desktop
platforms except for Windows.

BUG=634331
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq
==========

to

==========
[system-health] Add support for disabling individual stories on individual
platforms

This patch adds support for enabling/disabling individual stories on individual
platforms using the same approaches as for benchmarks:

  1. Disabled/Enabled decorator:

     @decorators.Disabled('win')
     class Story(system_health_story.SystemHealthStory):
        ...

  2. ShouldDisable method:

     class Story(system_health_story.SystemHealthStory):
        ...

        @classmethod
        def ShouldDisable(cls, possible_browser):
          return possible_browser.platform.GetOSName() == 'win'

Note that this patch also enables search:portal:google on all desktop
platforms except for Windows.

BUG=634331
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq

Committed: https://crrev.com/0d3d85c5f3a772dd65b1eae53c890d7251dee0ab
Cr-Commit-Position: refs/heads/master@{#417552}
==========

[commit-bot: I haz the power, 2016-09-09 10:44:16]

Patchset 5 (id:??) landed as
https://crrev.com/0d3d85c5f3a772dd65b1eae53c890d7251dee0ab
Cr-Commit-Position: refs/heads/master@{#417552}