Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2073303002:
[Telemetry] Allow users to select TBMv1 metrics to run alongside TBMv2 (Closed)
|
Created:
4 years, 6 months ago by eakuefner Modified:
3 years, 10 months ago Reviewers:
nednguyen CC:
catapult-reviews_chromium.org, petrcermak, telemetry-reviews_chromium.org, ulan Base URL:
git@github.com:catapult-project/catapult.git@master Target Ref:
refs/heads/master Project:
catapult Visibility:
Public. |
Description[Telemetry] Allow users to select TBMv1 metrics to run alongside TBMv2
This CL relaxes the restriction that users must choose either a list of TBMv1
metrics, or a TBMv2 metric, to be run by a benchmark. It reconciles the legacy
functionality that selects all TBMv1 metrics by default in the following way:
1. If a TBMv2 metric is specified, any explicitly specified (using
SetLegacyTimelineBasedMetrics) v1 metrics will also be computed.
2. If a TBMv2 metric is not specified, but TBMv1 metrics are specified, only
the selected TBMv1 metrics will be computed.
3. If neither a TBMv2 metric nor any TBMv1 metrics are specified, all TBMv1
metrics will be computed and no TBMv2 metric will be computed.
BUG=chromium:621035
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/054fe195c7bcf92dc42d2567492eb1e04281c0db
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/d8f0134fc047765b23230cc639e1565ca41aa900
Patch Set 1 #
Total comments: 5
Patch Set 2 : Address Ned's comments #Patch Set 3 : Fix indentation bug #
Total comments: 4
Messages
Total messages: 20 (7 generated)
eakuefner@chromium.org changed reviewers: + nednguyen@google.com
PTAL
lgtm https://codereview.chromium.org/2073303002/diff/1/telemetry/telemetry/web_per... File telemetry/telemetry/web_perf/timeline_based_measurement.py (right): https://codereview.chromium.org/2073303002/diff/1/telemetry/telemetry/web_per... telemetry/telemetry/web_perf/timeline_based_measurement.py:44: def _GetAllLegacyTimelineBasedMetrics(): I think we can just remove this to simplify the whole situation further. But in another CL, of course :-) https://codereview.chromium.org/2073303002/diff/1/telemetry/telemetry/web_per... telemetry/telemetry/web_perf/timeline_based_measurement.py:290: _GetAllLegacyTimelineBasedMetrics) _GetAllLegacyTimelineBasedMetrics() https://codereview.chromium.org/2073303002/diff/1/telemetry/telemetry/web_per... telemetry/telemetry/web_perf/timeline_based_measurement.py:290: _GetAllLegacyTimelineBasedMetrics) Can you also add warning.warns here about we will not support GetAllLegacyTimelineBasedMetrics() by default & will deprecate it by 1 month form now?
https://codereview.chromium.org/2073303002/diff/1/telemetry/telemetry/web_per... File telemetry/telemetry/web_perf/timeline_based_measurement.py (right): https://codereview.chromium.org/2073303002/diff/1/telemetry/telemetry/web_per... telemetry/telemetry/web_perf/timeline_based_measurement.py:44: def _GetAllLegacyTimelineBasedMetrics(): On 2016/06/17 at 21:36:11, nednguyen wrote: > I think we can just remove this to simplify the whole situation further. But in another CL, of course :-) We can either just update all such benchmarks (e.g. blob_storage) to compute all metrics explicitly, or we can consult with CL authors to see which metrics they're actually interested in. Probably I'd prefer the latter solution even though it's a bit more work, because in general I'm interested in uploading as little garbage as possible to the perf dashboard. https://codereview.chromium.org/2073303002/diff/1/telemetry/telemetry/web_per... telemetry/telemetry/web_perf/timeline_based_measurement.py:290: _GetAllLegacyTimelineBasedMetrics) On 2016/06/17 at 21:36:11, nednguyen wrote: > Can you also add warning.warns here about we will not support GetAllLegacyTimelineBasedMetrics() by default & will deprecate it by 1 month form now? Done.
The CQ bit was checked by eakuefner@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com Link to the patchset: https://codereview.chromium.org/2073303002/#ps20001 (title: "Address Ned's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2073303002/20001
Message was sent while issue was closed.
Description was changed from ========== [Telemetry] Allow users to select TBMv1 metrics to run alongside TBMv2 This CL relaxes the restriction that users must choose either a list of TBMv1 metrics, or a TBMv2 metric, to be run by a benchmark. It reconciles the legacy functionality that selects all TBMv1 metrics by default in the following way: 1. If a TBMv2 metric is specified, any explicitly specified (using SetLegacyTimelineBasedMetrics) v1 metrics will also be computed. 2. If a TBMv2 metric is not specified, but TBMv1 metrics are specified, only the selected TBMv1 metrics will be computed. 3. If neither a TBMv2 metric nor any TBMv1 metrics are specified, all TBMv1 metrics will be computed and no TBMv2 metric will be computed. BUG=chromium:621035 ========== to ========== [Telemetry] Allow users to select TBMv1 metrics to run alongside TBMv2 This CL relaxes the restriction that users must choose either a list of TBMv1 metrics, or a TBMv2 metric, to be run by a benchmark. It reconciles the legacy functionality that selects all TBMv1 metrics by default in the following way: 1. If a TBMv2 metric is specified, any explicitly specified (using SetLegacyTimelineBasedMetrics) v1 metrics will also be computed. 2. If a TBMv2 metric is not specified, but TBMv1 metrics are specified, only the selected TBMv1 metrics will be computed. 3. If neither a TBMv2 metric nor any TBMv1 metrics are specified, all TBMv1 metrics will be computed and no TBMv2 metric will be computed. BUG=chromium:621035 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/2073363002/ by nednguyen@google.com. The reason for reverting is: Suspect making smoothness benchmarks failing, blocking catapult roll. See https://github.com/catapult-project/catapult/issues/2418.
The CQ bit was checked by eakuefner@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com Link to the patchset: https://codereview.chromium.org/2073303002/#ps40001 (title: "Fix indentation bug")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2073303002/40001
Message was sent while issue was closed.
Description was changed from ========== [Telemetry] Allow users to select TBMv1 metrics to run alongside TBMv2 This CL relaxes the restriction that users must choose either a list of TBMv1 metrics, or a TBMv2 metric, to be run by a benchmark. It reconciles the legacy functionality that selects all TBMv1 metrics by default in the following way: 1. If a TBMv2 metric is specified, any explicitly specified (using SetLegacyTimelineBasedMetrics) v1 metrics will also be computed. 2. If a TBMv2 metric is not specified, but TBMv1 metrics are specified, only the selected TBMv1 metrics will be computed. 3. If neither a TBMv2 metric nor any TBMv1 metrics are specified, all TBMv1 metrics will be computed and no TBMv2 metric will be computed. BUG=chromium:621035 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ========== to ========== [Telemetry] Allow users to select TBMv1 metrics to run alongside TBMv2 This CL relaxes the restriction that users must choose either a list of TBMv1 metrics, or a TBMv2 metric, to be run by a benchmark. It reconciles the legacy functionality that selects all TBMv1 metrics by default in the following way: 1. If a TBMv2 metric is specified, any explicitly specified (using SetLegacyTimelineBasedMetrics) v1 metrics will also be computed. 2. If a TBMv2 metric is not specified, but TBMv1 metrics are specified, only the selected TBMv1 metrics will be computed. 3. If neither a TBMv2 metric nor any TBMv1 metrics are specified, all TBMv1 metrics will be computed and no TBMv2 metric will be computed. BUG=chromium:621035 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
Message was sent while issue was closed.
https://codereview.chromium.org/2073303002/diff/40001/telemetry/telemetry/web... File telemetry/telemetry/web_perf/timeline_based_measurement.py (right): https://codereview.chromium.org/2073303002/diff/40001/telemetry/telemetry/web... telemetry/telemetry/web_perf/timeline_based_measurement.py:294: self._ComputeLegacyTimelineBasedMetrics(results, trace_result) Probably should modify existing test in timeline_based_page_test_unittest to cover this code path rather than the code path of AllLegacyTimelineBasedMetric
Message was sent while issue was closed.
https://codereview.chromium.org/2073303002/diff/40001/telemetry/telemetry/web... File telemetry/telemetry/web_perf/timeline_based_measurement.py (right): https://codereview.chromium.org/2073303002/diff/40001/telemetry/telemetry/web... telemetry/telemetry/web_perf/timeline_based_measurement.py:294: self._ComputeLegacyTimelineBasedMetrics(results, trace_result) On 2016/06/20 at 23:17:05, nednguyen wrote: > Probably should modify existing test in timeline_based_page_test_unittest to cover this code path rather than the code path of AllLegacyTimelineBasedMetric Done in https://codereview.chromium.org/2084943003
Message was sent while issue was closed.
https://codereview.chromium.org/2073303002/diff/40001/telemetry/telemetry/web... File telemetry/telemetry/web_perf/timeline_based_measurement.py (right): https://codereview.chromium.org/2073303002/diff/40001/telemetry/telemetry/web... telemetry/telemetry/web_perf/timeline_based_measurement.py:291: 'on July 17, 2016.') Ethan, is this code path ready to be killed now?
Message was sent while issue was closed.
https://codereview.chromium.org/2073303002/diff/40001/telemetry/telemetry/web... File telemetry/telemetry/web_perf/timeline_based_measurement.py (right): https://codereview.chromium.org/2073303002/diff/40001/telemetry/telemetry/web... telemetry/telemetry/web_perf/timeline_based_measurement.py:291: 'on July 17, 2016.') On 2017/02/02 at 13:59:57, nednguyen wrote: > Ethan, is this code path ready to be killed now? Just about. Looking at this is on my radar for this week. There are two behaviors to consider here: 1. Allowing users to select both TBMv1 and TBMv2 metrics to be computed: pretty sure this functionality can be killed right now. I'm inclined to just do that first and send an email to telemetry-announce. 2. Selecting all TBMv1 metrics implicitly if TimelineBasedMeasurement is specified but SetTimelineBasedMetrics or SetLegacyTimelineBasedMetrics are not called. This functionality is still used by smoothness but probably nothing else, and could probably be fixed by just inserting SetLegacyTimelineBasedMetrics into smoothness. I just need to finish checking the rest of the TBMv1 tests to make sure that this call is present.
Message was sent while issue was closed.
On 2017/02/07 20:13:09, eakuefner wrote: > https://codereview.chromium.org/2073303002/diff/40001/telemetry/telemetry/web... > File telemetry/telemetry/web_perf/timeline_based_measurement.py (right): > > https://codereview.chromium.org/2073303002/diff/40001/telemetry/telemetry/web... > telemetry/telemetry/web_perf/timeline_based_measurement.py:291: 'on July 17, > 2016.') > On 2017/02/02 at 13:59:57, nednguyen wrote: > > Ethan, is this code path ready to be killed now? > > Just about. Looking at this is on my radar for this week. There are two > behaviors to consider here: > > 1. Allowing users to select both TBMv1 and TBMv2 metrics to be computed: pretty > sure this functionality can be killed right now. I'm inclined to just do that > first and send an email to telemetry-announce. > 2. Selecting all TBMv1 metrics implicitly if TimelineBasedMeasurement is > specified but SetTimelineBasedMetrics or SetLegacyTimelineBasedMetrics are not > called. This functionality is still used by smoothness but probably nothing > else, and could probably be fixed by just inserting > SetLegacyTimelineBasedMetrics into smoothness. I just need to finish checking > the rest of the TBMv1 tests to make sure that this call is present. The plan SGTM. Thanks! |
