Telemetry: Using traffic or cache settings adds a new grouping key |
|||||||
Issue descriptionUsually, tests are reported to the dashboard in the form: ChromiumPerf/<device_name>/<benchmark_name>/<metric_name>/<page_name> However, if you add traffic settings, it automatically adds a new grouping_key ( https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry/page/__init__.py?sq=package:chromium&dr&l=55 ): if traffic_setting != traffic_setting_module.NONE: self.grouping_keys['traffic_setting'] = traffic_setting ChromiumPerf/<device_name>/<benchmark_name>/<metric_name>/<traffic_setting>/<page_name> . For example, ChromiumPerf/chromium-rel-mac12/media.desktop/time_to_video_play_avg/Regular-3G/video.html?src_tulip2.vp9.webm_Regular-3G This means that for a benchmark for which some of the pages are constrained network and some are not constained network we have to have strange complicated alerting configs. Instead of just having ChromiumPerf/*/media.*/time_to_*_play_avg/*, we have to also have ChromiumPerf/*/media.*/time_to_*_play_avg/*/* Additionally, presumably the ChromiumPerf/*/media.*/time_to_*_play_avg/* pattern will also hit a summary metric that averages mutliple pages. My team does not want this. Also, if you try to pick a graph in the dashboard page it looks like the attached file. It seems like a bad idea to break the normal ChromiumPerf/<device_name>/<benchmark_name>/<metric_name>/<page_name> format for constrained networks, since the page_name could just contain the networking traffic information since you need to make a new page for a new networking traffic setting. There are two parts to this bug: 1. How to make the situation with media_cases ( https://cs.chromium.org/chromium/src/tools/perf/page_sets/media_cases.py?q=media_cases.py&sq=package:chromium&l=1 ) better for media (videostack) team. 2. Whether this exception to the regular format should be rolled back for every team. The exception was made in https://codereview.chromium.org/2494713004 by ksakamoto@ for https://bugs.chromium.org/p/chromium/issues/detail?id=664420 Note that instead loading story set could edit the grouping_keys in its own code rather than changing it in Page for everyone.
,
Nov 6 2017
,
Nov 6 2017
That sounds reasonable. In order to accomplish 1 and set the stage for 2, I can remove the following code from Page and move it into page_cycler_story.py:
if cache_temperature != cache_temperature_module.ANY:
self.grouping_keys['cache_temperature'] = cache_temperature
if traffic_setting != traffic_setting_module.NONE:
self.grouping_keys['traffic_setting'] = traffic_setting
It looks like the only pages that use traffic_setting and cache_temperature are loading benchmark pages that are of class PageCyclerStory. This change will prevent additional teams from relying on traffic_settings and cache_temperatures setting the grouping_keys while you're trying to deprecate grouping keys.
I'll assign myself to accomplish this. Does this plan seem reasonable, Ned?
,
Nov 6 2017
#3 your plan SGTM.
,
Nov 7 2017
,
Nov 7 2017
,
Nov 8 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5daa21ce99f8f70abe1202689a510d9f03e5a3e1 commit 5daa21ce99f8f70abe1202689a510d9f03e5a3e1 Author: Caleb Rouleau <crouleau@chromium.org> Date: Wed Nov 08 23:25:07 2017 Have pages edit grouping keys themselves. This allows us to move that logic out of Page so that it does not affect other Pages that don't want to use it. See related Catapult CL https: //chromium-review.googlesource.com/c/catapult/+/757504. Bug: 781899 Change-Id: I27e6b34a594ee983ead460828d170020bfa8ac15 Reviewed-on: https://chromium-review.googlesource.com/757239 Reviewed-by: Ned Nguyen <nednguyen@google.com> Reviewed-by: Kouhei Ueno <kouhei@chromium.org> Reviewed-by: Kunihiko Sakamoto <ksakamoto@chromium.org> Cr-Commit-Position: refs/heads/master@{#514999} [modify] https://crrev.com/5daa21ce99f8f70abe1202689a510d9f03e5a3e1/tools/perf/page_sets/page_cycler_story.py [modify] https://crrev.com/5daa21ce99f8f70abe1202689a510d9f03e5a3e1/tools/perf/page_sets/typical_25.py
,
Nov 9 2017
Marking this fixed. Ned can file a new bug for fully deprecating grouping keys.
,
Jun 4 2018
|
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by nedngu...@google.com
, Nov 6 2017