Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2110683010:
Allow benchmarks to specify running multiple metrics. (Closed)
|
Created:
4 years, 5 months ago by alexandermont Modified:
4 years, 5 months ago CC:
catapult-reviews_chromium.org, telemetry-reviews_chromium.org, tracing-review_chromium.org Base URL:
git@github.com:catapult-project/catapult@master Target Ref:
refs/heads/master Project:
catapult Visibility:
Public. |
DescriptionAllow benchmarks to specify running multiple metrics.
BUG=catapult:#2430
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/7efefa6bb999510b9b98772df23d9ad24cb05dcc
Patch Set 1 #
Total comments: 6
Patch Set 2 : code review changes #
Total comments: 6
Patch Set 3 : fix style #
Total comments: 4
Patch Set 4 : fix tests #Patch Set 5 : fix style #Patch Set 6 : fix discover test #
Total comments: 12
Patch Set 7 : fix style some more #
Messages
Total messages: 33 (14 generated)
alexandermont@chromium.org changed reviewers: + charliea@chromium.org, eakuefner@chromium.org, nednguyen@google.com
https://codereview.chromium.org/2110683010/diff/1/telemetry/telemetry/web_per... File telemetry/telemetry/web_perf/timeline_based_measurement.py (right): https://codereview.chromium.org/2110683010/diff/1/telemetry/telemetry/web_per... telemetry/telemetry/web_perf/timeline_based_measurement.py:215: """Sets the new-style (TBMv2) metric to run. Can you update the docstring to take into account that this is plural now? https://codereview.chromium.org/2110683010/diff/1/tracing/bin/run_metric File tracing/bin/run_metric (right): https://codereview.chromium.org/2110683010/diff/1/tracing/bin/run_metric#newc... tracing/bin/run_metric:20: parser.add_argument('metric_names', Instead of doing this, can you make it possible to pass multiple --metric arguments, like we do with --output-format in Telemetry [0] and implicitly take the argument before the trace_file_or_dir as a --metric if none is specified, like how sed will treat the string you pass as the expression to evaluate if you don't pass -e explicitly? [0] https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/...
https://codereview.chromium.org/2110683010/diff/1/telemetry/telemetry/web_per... File telemetry/telemetry/web_perf/timeline_based_measurement.py (right): https://codereview.chromium.org/2110683010/diff/1/telemetry/telemetry/web_per... telemetry/telemetry/web_perf/timeline_based_measurement.py:215: """Sets the new-style (TBMv2) metric to run. On 2016/06/30 at 18:45:03, eakuefner wrote: > Can you update the docstring to take into account that this is plural now? Done https://codereview.chromium.org/2110683010/diff/1/tracing/bin/run_metric File tracing/bin/run_metric (right): https://codereview.chromium.org/2110683010/diff/1/tracing/bin/run_metric#newc... tracing/bin/run_metric:20: parser.add_argument('metric_names', On 2016/06/30 at 18:45:03, eakuefner wrote: > Instead of doing this, can you make it possible to pass multiple --metric arguments, like we do with --output-format in Telemetry [0] and implicitly take the argument before the trace_file_or_dir as a --metric if none is specified, like how sed will treat the string you pass as the expression to evaluate if you don't pass -e explicitly? > > [0] https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/... Done
petrcermak@chromium.org changed reviewers: + petrcermak@chromium.org
FYI, this patch is blocking: https://codereview.chromium.org/2106133002/ (Port oortonline_tbm to TBMv2) → https://bugs.chromium.org/p/chromium/issues/detail?id=621035 (Switch oortonline_tbm to the TBMv2 memory metric) → https://bugs.chromium.org/p/chromium/issues/detail?id=621037 (Deprecate & remove TBMv1 memory metric) which prevents us from bulk renaming memory values used by other teams (who recently switched from TBMv1 to TBMv2) to track memory. Hence it would be great if this patch could land as soon as possible :-) Thanks, Petr
non-owner LGTM with a couple of code style nits. Petr https://codereview.chromium.org/2110683010/diff/20001/telemetry/telemetry/web... File telemetry/telemetry/web_perf/timeline_based_measurement.py (right): https://codereview.chromium.org/2110683010/diff/20001/telemetry/telemetry/web... telemetry/telemetry/web_perf/timeline_based_measurement.py:211: assert isinstance(metric, basestring) Perhaps it would be better to call SetTimelineBasedMetrics here to avoid code duplication: def SetTimelineBasedMetric(self, metric): return self.SetTimelineBasedMetrics([metric]) https://codereview.chromium.org/2110683010/diff/20001/tracing/bin/run_metric File tracing/bin/run_metric (right): https://codereview.chromium.org/2110683010/diff/20001/tracing/bin/run_metric#... tracing/bin/run_metric:23: 'of registered metrics (not filenames.) ' nit: Lines 23-25 should be vertically aligned with "'Comma...", i.e.: parser.add_argument('metric', action='append', dest='metric_names', default=[], help=('Comma separated list of function names ' 'of registered metrics (not filenames.) ' [...] https://codereview.chromium.org/2110683010/diff/20001/tracing/bin/run_metric#... tracing/bin/run_metric:40: for k, v in metric_runner.RunMetricOnTraces(traces, metrics).iteritems()} nit: I think that "for" should be vertically aligned with "k:" according to the Chromium Python style guide. Since that will make the secind line too long, I suggest you break before "in" instead: results = {k: v.AsDict() for k, v in metric_runner...} https://codereview.chromium.org/2110683010/diff/20001/tracing/tracing/metrics... File tracing/tracing/metrics/metric_map_function.html (right): https://codereview.chromium.org/2110683010/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/metric_map_function.html:28: throw new Error('A metric name should be specified.'); nit: s/A metric name/Metric names/ https://codereview.chromium.org/2110683010/diff/20001/tracing/tracing/metrics... File tracing/tracing/metrics/metric_runner.py (right): https://codereview.chromium.org/2110683010/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/metric_runner.py:38: extra_import_options=None): [I know that this is unrelated to the patch] I think that this will fit on the previous line ;-)
https://codereview.chromium.org/2110683010/diff/1/tracing/bin/run_metric File tracing/bin/run_metric (right): https://codereview.chromium.org/2110683010/diff/1/tracing/bin/run_metric#newc... tracing/bin/run_metric:20: parser.add_argument('metric_names', On 2016/07/01 at 21:46:00, alexandermont wrote: > On 2016/06/30 at 18:45:03, eakuefner wrote: > > Instead of doing this, can you make it possible to pass multiple --metric arguments, like we do with --output-format in Telemetry [0] and implicitly take the argument before the trace_file_or_dir as a --metric if none is specified, like how sed will treat the string you pass as the expression to evaluate if you don't pass -e explicitly? > > > > [0] https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/... > > Done It doesn't look like this is done in the latest patch set -- did you forget to upload these changes? To be clear, rather than using commas, I would like to be able to write: bin/run_metric --metric=memoryMetric --metric=executionMetric mytrace.json, or as a shorthand if I only happen to be specifying one metric: bin/run_metric memoryMetric mytrace.json https://codereview.chromium.org/2110683010/diff/20001/telemetry/telemetry/web... File telemetry/telemetry/web_perf/timeline_based_measurement.py (right): https://codereview.chromium.org/2110683010/diff/20001/telemetry/telemetry/web... telemetry/telemetry/web_perf/timeline_based_measurement.py:211: assert isinstance(metric, basestring) On 2016/07/06 at 08:19:47, petrcermak wrote: > Perhaps it would be better to call SetTimelineBasedMetrics here to avoid code duplication: > > def SetTimelineBasedMetric(self, metric): > return self.SetTimelineBasedMetrics([metric]) Good idea.
https://codereview.chromium.org/2110683010/diff/1/tracing/bin/run_metric File tracing/bin/run_metric (right): https://codereview.chromium.org/2110683010/diff/1/tracing/bin/run_metric#newc... tracing/bin/run_metric:20: parser.add_argument('metric_names', On 2016/07/06 at 15:52:54, eakuefner wrote: > On 2016/07/01 at 21:46:00, alexandermont wrote: > > On 2016/06/30 at 18:45:03, eakuefner wrote: > > > Instead of doing this, can you make it possible to pass multiple --metric arguments, like we do with --output-format in Telemetry [0] and implicitly take the argument before the trace_file_or_dir as a --metric if none is specified, like how sed will treat the string you pass as the expression to evaluate if you don't pass -e explicitly? > > > > > > [0] https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/... > > > > Done > > It doesn't look like this is done in the latest patch set -- did you forget to upload these changes? To be clear, rather than using commas, I would like to be able to write: > > bin/run_metric --metric=memoryMetric --metric=executionMetric mytrace.json, or as a shorthand if I only happen to be specifying one metric: > > bin/run_metric memoryMetric mytrace.json Talked to Alex about this offline and read a bit more about argparse -- it turns out that normally, arguments with - are "optional", and in this case, we don't want it to be optional, we want it to be mandatory and variadic. However, the nargs= functionality that argparse provides assumes such a variadic argument will be the last one provided. Since trace_file_or_dir is always going to be a singleton, this works for us, as long as we switch the order of the arguments, which this CL will now do.
lgtm when petr's comments are addressed.
The CQ bit was checked by alexandermont@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eakuefner@chromium.org, petrcermak@chromium.org Link to the patchset: https://codereview.chromium.org/2110683010/#ps40001 (title: "fix style")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2110683010/diff/40001/telemetry/telemetry/web... File telemetry/telemetry/web_perf/timeline_based_measurement.py (right): https://codereview.chromium.org/2110683010/diff/40001/telemetry/telemetry/web... telemetry/telemetry/web_perf/timeline_based_measurement.py:211: self.SetTimelineBasedMetrics([metric]) nit: Why is there a comment for SetTimelineBasedMetrics, but not SetTimelineBasedMetric? (My personal preference would be for neither - the method names are sufficient. However, seems like we should at least be consistent.) https://codereview.chromium.org/2110683010/diff/40001/tracing/bin/run_metric File tracing/bin/run_metric (right): https://codereview.chromium.org/2110683010/diff/40001/tracing/bin/run_metric#... tracing/bin/run_metric:23: help=('Function names ' Seems like weird line splitting for the help string https://codereview.chromium.org/2110683010/diff/40001/tracing/tracing/metrics... File tracing/tracing/metrics/metric_map_function.html (right): https://codereview.chromium.org/2110683010/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/metric_map_function.html:1: <!DOCTYPE html> Can you write a test that uses multiple metrics? https://codereview.chromium.org/2110683010/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/metric_map_function.html:27: if (metricNames === undefined) Maybe add a check for if metricNames.length === 0? (Alternatively, you could just check: if (!metricNames) which would check for both)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Catapult Mac Tryserver on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Ma...)
The CQ bit was checked by alexandermont@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eakuefner@chromium.org, petrcermak@chromium.org Link to the patchset: https://codereview.chromium.org/2110683010/#ps80001 (title: "fix style")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Catapult Mac Tryserver on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Ma...)
The CQ bit was checked by alexandermont@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eakuefner@chromium.org, petrcermak@chromium.org Link to the patchset: https://codereview.chromium.org/2110683010/#ps100001 (title: "fix discover test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2110683010/diff/100001/telemetry/telemetry/we... File telemetry/telemetry/web_perf/timeline_based_measurement.py (right): https://codereview.chromium.org/2110683010/diff/100001/telemetry/telemetry/we... telemetry/telemetry/web_perf/timeline_based_measurement.py:210: def SetTimelineBasedMetric(self, metric): nit: are you going to delete this now that you're replacing all SetTimelineBasedMetric calls with SetTimelineBasedMetrics([metric])? (If so, please add a TODO to delete this method once that's done.) (Also, FWIW, my vote is to delete this method. It doesn't seem like saving two characters is enough of a convenience to warrant having an extra method) https://codereview.chromium.org/2110683010/diff/100001/telemetry/telemetry/we... telemetry/telemetry/web_perf/timeline_based_measurement.py:213: def SetTimelineBasedMetrics(self, metrics): Gah - sorry, this is totally my fault. I'd leave the comment in: I didn't see the additional context about where metrics are assumed to live and such, which is probably useful for callers to know. (I just saw "Sets the new-style (TBMv2) metric to run.") Sorry for the noise. https://codereview.chromium.org/2110683010/diff/100001/tracing/tracing/metric... File tracing/tracing/metrics/discover_unittest.py (right): https://codereview.chromium.org/2110683010/diff/100001/tracing/tracing/metric... tracing/tracing/metrics/discover_unittest.py:15: ['sampleMetric', 'sampleMetricTwo'], nit: usually we don't spell out numbers in names, which would make this 'sampleMetric2', not 'sampleMetricTwo' https://codereview.chromium.org/2110683010/diff/100001/tracing/tracing/metric... File tracing/tracing/metrics/metric_map_function_test.html (right): https://codereview.chromium.org/2110683010/diff/100001/tracing/tracing/metric... tracing/tracing/metrics/metric_map_function_test.html:61: var resTwo = new pi.mre.MreResult(); nit1: We usually don't spell out numbers in variables names, which would make this "res2", not "resTwo" nit2: We should be consistent with our naming: if we use "result" above, we should use "result2" here, not "res2". https://codereview.chromium.org/2110683010/diff/100001/tracing/tracing/metric... tracing/tracing/metrics/metric_map_function_test.html:62: tr.metrics.metricMapFunction(resTwo, m, {'metrics': ['sampleMetric', nit: In javascript, we don't line up array values when the split lines. nit2: Split at the highest syntactic level. I think that'd make it: tr.metrics.metricMapFunction( resTwo, m, {'metrics': ['sampleMetric', 'sampleMetricTwo']}); or, if that won't fit: tr.metrics.metricMapFunction(resTwo, m, {'metrics': ['sampleMetric', 'sampleMetricTwo']}); https://codereview.chromium.org/2110683010/diff/100001/tracing/tracing/metric... File tracing/tracing/metrics/sample_metric.html (right): https://codereview.chromium.org/2110683010/diff/100001/tracing/tracing/metric... tracing/tracing/metrics/sample_metric.html:33: var n1 = new tr.v.ScalarNumeric(sizeInBytes_smallerIsBetter, 1); nit: these are really bad variable names
The CQ bit was unchecked by alexandermont@chromium.org
The CQ bit was checked by alexandermont@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eakuefner@chromium.org, petrcermak@chromium.org Link to the patchset: https://codereview.chromium.org/2110683010/#ps120001 (title: "fix style some more")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2110683010/diff/100001/telemetry/telemetry/we... File telemetry/telemetry/web_perf/timeline_based_measurement.py (right): https://codereview.chromium.org/2110683010/diff/100001/telemetry/telemetry/we... telemetry/telemetry/web_perf/timeline_based_measurement.py:210: def SetTimelineBasedMetric(self, metric): On 2016/07/06 at 19:52:41, charliea wrote: > nit: are you going to delete this now that you're replacing all SetTimelineBasedMetric calls with SetTimelineBasedMetrics([metric])? > > (If so, please add a TODO to delete this method once that's done.) > > (Also, FWIW, my vote is to delete this method. It doesn't seem like saving two characters is enough of a convenience to warrant having an extra method) Done https://codereview.chromium.org/2110683010/diff/100001/telemetry/telemetry/we... telemetry/telemetry/web_perf/timeline_based_measurement.py:213: def SetTimelineBasedMetrics(self, metrics): On 2016/07/06 at 19:52:41, charliea wrote: > Gah - sorry, this is totally my fault. I'd leave the comment in: I didn't see the additional context about where metrics are assumed to live and such, which is probably useful for callers to know. > > (I just saw "Sets the new-style (TBMv2) metric to run.") > > Sorry for the noise. Done https://codereview.chromium.org/2110683010/diff/100001/tracing/tracing/metric... File tracing/tracing/metrics/discover_unittest.py (right): https://codereview.chromium.org/2110683010/diff/100001/tracing/tracing/metric... tracing/tracing/metrics/discover_unittest.py:15: ['sampleMetric', 'sampleMetricTwo'], On 2016/07/06 at 19:52:41, charliea wrote: > nit: usually we don't spell out numbers in names, which would make this 'sampleMetric2', not 'sampleMetricTwo' Done https://codereview.chromium.org/2110683010/diff/100001/tracing/tracing/metric... File tracing/tracing/metrics/metric_map_function_test.html (right): https://codereview.chromium.org/2110683010/diff/100001/tracing/tracing/metric... tracing/tracing/metrics/metric_map_function_test.html:61: var resTwo = new pi.mre.MreResult(); On 2016/07/06 at 19:52:41, charliea wrote: > nit1: We usually don't spell out numbers in variables names, which would make this "res2", not "resTwo" > > nit2: We should be consistent with our naming: if we use "result" above, we should use "result2" here, not "res2". Done https://codereview.chromium.org/2110683010/diff/100001/tracing/tracing/metric... tracing/tracing/metrics/metric_map_function_test.html:62: tr.metrics.metricMapFunction(resTwo, m, {'metrics': ['sampleMetric', On 2016/07/06 at 19:52:41, charliea wrote: > nit: In javascript, we don't line up array values when the split lines. > > nit2: Split at the highest syntactic level. I think that'd make it: > > tr.metrics.metricMapFunction( > resTwo, m, {'metrics': ['sampleMetric', 'sampleMetricTwo']}); > > or, if that won't fit: > > tr.metrics.metricMapFunction(resTwo, m, > {'metrics': ['sampleMetric', 'sampleMetricTwo']}); Done https://codereview.chromium.org/2110683010/diff/100001/tracing/tracing/metric... File tracing/tracing/metrics/sample_metric.html (right): https://codereview.chromium.org/2110683010/diff/100001/tracing/tracing/metric... tracing/tracing/metrics/sample_metric.html:33: var n1 = new tr.v.ScalarNumeric(sizeInBytes_smallerIsBetter, 1); On 2016/07/06 at 19:52:41, charliea wrote: > nit: these are really bad variable names These are the same names that were there earlier in sampleMetric. This is just a sample metric that gets used for tests, and I just made a "sampleMetric2" to be able to run a test (in metric_map_function_test) with two metrics.
Message was sent while issue was closed.
Description was changed from ========== Allow benchmarks to specify running multiple metrics. BUG=catapult:#2430 ========== to ========== Allow benchmarks to specify running multiple metrics. BUG=catapult:#2430 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
