browserless story failing with "Cannot find browser of type system. To list out all available browsers, rerun your command with --browser=list" |
||||
Issue descriptionA 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
,
May 7 2018
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).
,
May 7 2018
+Ethan/Ben for the (1) in comment #2
,
May 8 2018
Still a hack, but would it make more sense to implement a --browser=none for the browserless case?
,
May 8 2018
#4: I think that is a good workaround as well, but what is your opposition to (2) in #2?
,
May 8 2018
,
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
,
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
,
May 9 2018
Marking as fixed since this is now working. Thanks for the fix! |
||||
►
Sign in to add a comment |
||||
Comment 1 by nednguyen@chromium.org
, May 7 2018