New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 840428 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: May 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue skia:7872



Sign in to add a comment

browserless story failing with "Cannot find browser of type system. To list out all available browsers, rerun your command with --browser=list"

Project Member Reported by rmis...@google.com, May 7 2018

Issue description


A browserless story was added in https://chromium.googlesource.com/chromium/src/+/master/tools/perf/contrib/cluster_telemetry/shared_browserless_story.py for the issue https://bugs.chromium.org/p/skia/issues/detail?id=7796 ('Create ct_metrics_analysis benchmark that allows computing metrics on Cluster Telemetry').

However, when a browser is not available on the host machine, it fails with:
"""
Cannot find browser of type system. To list out all available browsers, rerun your command with --browser=list
"""

The problem is this check: https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry/internal/story_runner.py?q=story_runner.py&sq=package:chromium&l=302
 
Cc: perezju@chromium.org charliea@chromium.org
Cc: eakuefner@chromium.org benjhayden@chromium.org
Status: Started (was: Assigned)
There are two angles from which this bug can be fixed:
1) We should haven't use "browserless benchmark" model. Instead, this should just be a script that fetch traces from cloud storage, then invoke the TBMv2 metrics trace computation, and use the csv_output_formatter to output the results in csv format.

Attacking the bug this way likely gonna take quarter to un-entangle all the assumptions baked in Telemetry results system to deal with page, results summary. This is also a bad time to do this as the service team is also working on this area to launch the histogram set format.

2) The code in story_runner should not be specific to the browser. The line inhttps://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry/internal/story_runner.py?rcl=b9ff578f32e76574afd53543708a694d08b841de&l=302 is hacky in the way that it makes the assumption that Telemetry core benchmark state machine always deal with a benchmark that involes a browser (thus the step of invoking "possible_browser = browser_finder.FindBrowser(finder_options)").

All that step needs is a platform object to determine the test expectation. To fix this, we can just initiate the shared_state object earlier (used to be in https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry/internal/story_runner.py?rcl=b9ff578f32e76574afd53543708a694d08b841de&l=205), and pull the platform out of that shared_state class.

Another side benefit of this is it also help making sure that typical Telemetry benchmark doesn't try to find the browser twice (first time for identifying the expectation, then the second time for starting the benchmark story loop).
+Ethan/Ben for the (1) in comment #2
Still a hack, but would it make more sense to implement a --browser=none for the browserless case?
#4: I think that is a good workaround as well, but what is your opposition to (2) in #2?

Comment 6 by rmis...@google.com, May 8 2018

Blocking: skia:7872
Project Member

Comment 7 by bugdroid1@chromium.org, May 8 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/catapult/+/f25391b703a2c40d84198be20d8bc9753c5b630d

commit f25391b703a2c40d84198be20d8bc9753c5b630d
Author: nednguyen <nednguyen@google.com>
Date: Tue May 08 19:32:41 2018

Making browserless benchmark runnable on story_runner

Bug:  chromium:840428 
Change-Id: Iaad2a1da998f3eebafb4f68904c9d439b57dd695
Reviewed-on: https://chromium-review.googlesource.com/1050367
Reviewed-by: Charlie Andrews <charliea@chromium.org>
Reviewed-by: Juan Antonio Navarro Pérez <perezju@chromium.org>
Commit-Queue: Ned Nguyen <nednguyen@google.com>

[modify] https://crrev.com/f25391b703a2c40d84198be20d8bc9753c5b630d/telemetry/telemetry/internal/story_runner.py
[modify] https://crrev.com/f25391b703a2c40d84198be20d8bc9753c5b630d/telemetry/telemetry/page/shared_page_state.py

Project Member

Comment 8 by bugdroid1@chromium.org, May 8 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0ddf8b2449001c40871fe1f6255a17e31148e1d3

commit 0ddf8b2449001c40871fe1f6255a17e31148e1d3
Author: catapult-chromium-autoroll <catapult-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Date: Tue May 08 23:56:02 2018

Roll src/third_party/catapult/ a6dce8334..f25391b70 (2 commits)

https://chromium.googlesource.com/catapult.git/+log/a6dce8334230..f25391b703a2

$ git log a6dce8334..f25391b70 --date=short --no-merges --format='%ad %ae %s'
2018-05-08 nednguyen Making browserless benchmark runnable on story_runner
2018-05-08 chiniforooshan Telemetry: set duration hist label only when given

Created with:
  roll-dep src/third_party/catapult
BUG= chromium:840428 


The AutoRoll server is located here: https://catapult-roll.skia.org

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC'd on the roll, and stop the roller if necessary.


TBR=sullivan@chromium.org

Change-Id: I0a63708b574dc41520c38f8204e4d0c41b58403b
Reviewed-on: https://chromium-review.googlesource.com/1050811
Commit-Queue: catapult-chromium-autoroll <catapult-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Reviewed-by: catapult-chromium-autoroll <catapult-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#557013}
[modify] https://crrev.com/0ddf8b2449001c40871fe1f6255a17e31148e1d3/DEPS

Comment 9 by rmis...@google.com, May 9 2018

Status: Fixed (was: Started)
Marking as fixed since this is now working. Thanks for the fix!

Sign in to add a comment