New issue
Advanced search Search tips

Issue 921616 link

Starred by 1 user

Issue metadata

Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 921000



Sign in to add a comment

Trim down unused methods from Telemetry's value system

Project Member Reported by perezju@chromium.org, Jan 14

Issue description

We probably want to keep this value system as small as possible (enough to output json-test-results), while all other numeric values are computed by metrics (or directly as histograms).

To start, it looks like the following methods were only there to support a no longer existing output format:

- GetBuildbotDataType
- GetBuildbotValue
- GetRepresentativeNumber
- GetRepresentativeString

Nothing calls these methods other than unit tests. Let's get rid of them.
 
I've found a bunch of unittests that appear to use GetRepresentativeNumber in order to get the mean of a list_of_scalar_values. Here:
https://cs.chromium.org/search/?q=GetRepresentative(Number%7CString)+-file:catapult/telemetry/telemetry/value&type=cs

I'm thinking to add instead a list_of_scalar_values.mean (it already has .std and .variance), so we can remove all of the above methods.

Ben, does that sound fine to you?
You now know more about the legacy value system than I ever have. If it moves us towards replacing values with histograms, I'm for it.
Project Member

Comment 3 by bugdroid1@chromium.org, Jan 15

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

commit fb89f75dc9eaa05e9c4752bb8178b39ca306e8e2
Author: Juan Antonio Navarro Perez <perezju@chromium.org>
Date: Tue Jan 15 09:29:11 2019

[Telemetry] Add list_of_scalar_values.mean

Will be used by a few clients instead of the soon to be removed
GetRepresentativeNumber.

Bug: chromium:921616
Change-Id: I312fa2506f31730f2f3b06d7a2e3a9056e0c112a
Reviewed-on: https://chromium-review.googlesource.com/c/1409563
Reviewed-by: Ben Hayden <benjhayden@chromium.org>
Commit-Queue: Juan Antonio Navarro Pérez <perezju@chromium.org>

[modify] https://crrev.com/fb89f75dc9eaa05e9c4752bb8178b39ca306e8e2/telemetry/telemetry/value/list_of_scalar_values.py
[modify] https://crrev.com/fb89f75dc9eaa05e9c4752bb8178b39ca306e8e2/telemetry/telemetry/value/list_of_scalar_values_unittest.py

Project Member

Comment 4 by bugdroid1@chromium.org, Jan 15

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

commit b0552857138392bd13415d9a1d42cecf143ac70f
Author: Juan Antonio Navarro Perez <perezju@chromium.org>
Date: Tue Jan 15 15:19:28 2019

[tools/perf] Remove references to GetBuildbotValue

The value.GetBuildbotValue() method is being removed. For a
list_of_scalar_values, like all metrics here, one can directly
access metric.values instead.

Bug: 921616
Change-Id: I67255e11dd2f37eb836ccc3b952087c0c065a87e
Reviewed-on: https://chromium-review.googlesource.com/c/1409504
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Juan Antonio Navarro Pérez <perezju@chromium.org>
Cr-Commit-Position: refs/heads/master@{#622861}
[modify] https://crrev.com/b0552857138392bd13415d9a1d42cecf143ac70f/tools/perf/contrib/oilpan/oilpan_gc_times_unittest.py

Project Member

Comment 5 by bugdroid1@chromium.org, Jan 16

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

commit c9d39cb7402e371e19d1485f07270953c266d4cd
Author: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Date: Wed Jan 16 02:43:37 2019

Roll src/third_party/catapult 96320b515106..78448d90081e (17 commits)

https://chromium.googlesource.com/catapult.git/+log/96320b515106..78448d90081e


git log 96320b515106..78448d90081e --date=short --no-merges --format='%ad %ae %s'
2019-01-15 vollick@chromium.org Plumb the trace buffer size for atrace
2019-01-15 perezju@chromium.org Revert "[py_utils] Add modules_util.RequireVersion"
2019-01-15 taylori@google.com Fix handling of perfetto protobuf on mac
2019-01-15 perezju@chromium.org [dashboard] Remove dead code in start_try_job.py
2019-01-15 perezju@chromium.org Revert "Remove TagMap."
2019-01-15 perezju@chromium.org [Telemetry] Add list_of_scalar_values.mean
2019-01-15 dtu@chromium.org [pinpoint] Add Tags field to try job dialog.
2019-01-15 dtu@chromium.org [pinpoint] Ignore cached isolate hashes over 8 weeks old.
2019-01-14 eroman@chromium.org Fix import of new NetExport generated logs.
2019-01-14 bsheedy@chromium.org Fix gtest conversion multiplier
2019-01-14 eyaich@google.com Adding timeToFirstViewportReady metric
2019-01-14 perezju@chromium.org [py_utils] Add modules_util.RequireVersion
2019-01-14 perezju@chromium.org Remove GetNetworkData methods
2019-01-14 perezju@chromium.org [cli services] Add buildbucket_service
2019-01-14 perezju@chromium.org [Telemetry] Remove TBMv1 metrics.SmoothnessMetric
2019-01-11 benjhayden@chromium.org Remove TagMap.
2019-01-11 benjhayden@chromium.org Truncate serialized floats in HistogramSet JSON.


Created with:
  gclient setdep -r src/third_party/catapult@78448d90081e

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

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.

CQ_INCLUDE_TRYBOTS=luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel

BUG=chromium:776709,chromium:777865,chromium:918218,chromium:921616,chromium:874940,chromium:916877, chromium:917273 ,chromium:921342,chromium:904879,chromium:776709,chromium:777865,chromium:480512, chromium:919093 , chromium:691581 ,chromium:921000,chromium:918218,chromium:918208
TBR=sullivan@chromium.org

Change-Id: I75c07d8719f5668f9b4a95a9757cf6c25412b547
Reviewed-on: https://chromium-review.googlesource.com/c/1413252
Reviewed-by: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Commit-Queue: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#623062}
[modify] https://crrev.com/c9d39cb7402e371e19d1485f07270953c266d4cd/DEPS

Comment 6 by benhenry@google.com, Jan 16 (6 days ago)

Components: Test>Telemetry

Comment 7 by benhenry@google.com, Jan 16 (6 days ago)

Components: -Tests>Telemetry
Project Member

Comment 8 by bugdroid1@chromium.org, Jan 18 (4 days ago)

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

commit 478ab9d54c747c6bb1254fabe172623d7db9a93c
Author: Juan Antonio Navarro Perez <perezju@chromium.org>
Date: Fri Jan 18 11:03:06 2019

[tools/perf] Remove references to deprecated GetRepresentativeNumber

Clients should now use instead:
- metric.value for scalars, and
- metric.mean for lists of scalar values

Bug: 921616
Change-Id: Idab3504059d2d91db17eefde6ad2d43f4b7391e7
Reviewed-on: https://chromium-review.googlesource.com/c/1411920
Reviewed-by: Caleb Rouleau <crouleau@chromium.org>
Commit-Queue: Juan Antonio Navarro Pérez <perezju@chromium.org>
Cr-Commit-Position: refs/heads/master@{#624068}
[modify] https://crrev.com/478ab9d54c747c6bb1254fabe172623d7db9a93c/tools/perf/benchmarks/blink_perf_unittest.py
[modify] https://crrev.com/478ab9d54c747c6bb1254fabe172623d7db9a93c/tools/perf/measurements/rasterize_and_record_micro_unittest.py
[modify] https://crrev.com/478ab9d54c747c6bb1254fabe172623d7db9a93c/tools/perf/measurements/skpicture_printer_unittest.py

Project Member

Comment 9 by bugdroid1@chromium.org, Jan 18 (4 days ago)

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

commit 1f939034d069a62cbc0c22a8cee76507a08649b9
Author: Juan Antonio Navarro Perez <perezju@chromium.org>
Date: Fri Jan 18 13:23:36 2019

[Telemetry] Remove unused methods from telemetry.values

Removing the following methods:
- AsDictWithoutBaseClassEntries
- GetBuildbotDataType
- GetBuildbotValue
- GetRepresentativeNumber
- GetRepresentativeString

And a few other pieces of now dead code. These were used in the past
to support an output format that no longer exists.

Bug: chromium:921616
Change-Id: I14921872f96a5852b744b93a03af6aa05638abfa
Reviewed-on: https://chromium-review.googlesource.com/c/1409562
Reviewed-by: Caleb Rouleau <crouleau@chromium.org>
Commit-Queue: Juan Antonio Navarro Pérez <perezju@chromium.org>

[modify] https://crrev.com/1f939034d069a62cbc0c22a8cee76507a08649b9/telemetry/telemetry/value/histogram.py
[modify] https://crrev.com/1f939034d069a62cbc0c22a8cee76507a08649b9/telemetry/telemetry/value/summarizable.py
[modify] https://crrev.com/1f939034d069a62cbc0c22a8cee76507a08649b9/telemetry/telemetry/value/value_unittest.py
[modify] https://crrev.com/1f939034d069a62cbc0c22a8cee76507a08649b9/telemetry/telemetry/value/__init__.py
[modify] https://crrev.com/1f939034d069a62cbc0c22a8cee76507a08649b9/telemetry/telemetry/value/histogram_unittest.py
[modify] https://crrev.com/1f939034d069a62cbc0c22a8cee76507a08649b9/telemetry/telemetry/value/list_of_scalar_values_unittest.py
[modify] https://crrev.com/1f939034d069a62cbc0c22a8cee76507a08649b9/telemetry/telemetry/value/scalar.py
[modify] https://crrev.com/1f939034d069a62cbc0c22a8cee76507a08649b9/telemetry/telemetry/value/skip.py
[modify] https://crrev.com/1f939034d069a62cbc0c22a8cee76507a08649b9/telemetry/telemetry/value/list_of_scalar_values.py
[modify] https://crrev.com/1f939034d069a62cbc0c22a8cee76507a08649b9/telemetry/telemetry/value/skip_unittest.py
[modify] https://crrev.com/1f939034d069a62cbc0c22a8cee76507a08649b9/telemetry/telemetry/value/trace.py
[modify] https://crrev.com/1f939034d069a62cbc0c22a8cee76507a08649b9/telemetry/telemetry/value/summarizable_unittest.py
[modify] https://crrev.com/1f939034d069a62cbc0c22a8cee76507a08649b9/telemetry/telemetry/value/scalar_unittest.py

Project Member

Comment 10 by bugdroid1@chromium.org, Jan 18 (4 days ago)

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

commit ac02b504ecad81887c47611f5b62dd2b9a603664
Author: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Date: Fri Jan 18 15:34:41 2019

Roll src/third_party/catapult 6ca0a8ed6790..1f939034d069 (1 commits)

https://chromium.googlesource.com/catapult.git/+log/6ca0a8ed6790..1f939034d069


git log 6ca0a8ed6790..1f939034d069 --date=short --no-merges --format='%ad %ae %s'
2019-01-18 perezju@chromium.org [Telemetry] Remove unused methods from telemetry.values


Created with:
  gclient setdep -r src/third_party/catapult@1f939034d069

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

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.

CQ_INCLUDE_TRYBOTS=luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel

BUG=chromium:921616
TBR=sullivan@chromium.org

Change-Id: I5fd61f4fadcb762d1c1b5c6d8231f47a18249207
Reviewed-on: https://chromium-review.googlesource.com/c/1421263
Reviewed-by: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Commit-Queue: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#624134}
[modify] https://crrev.com/ac02b504ecad81887c47611f5b62dd2b9a603664/DEPS

Comment 11 by perezju@chromium.org, Jan 18 (4 days ago)

Next, I think we can remove the tir_label and grouping_keys associated to *values*.

The concepts still exist on pages, and we can't remove them there because, despite being legacy concepts, they are still used to compute story names uploaded to the dashboard. But I think nothing adds these to the values any more, so it should be safe to remove from there.

Sign in to add a comment