catapult-project / catapult

Deprecated Catapult GitHub. Please instead use http://crbug.com "Speed>Benchmarks" component for bugs and https://chromium.googlesource.com/catapult for downloading and editing source code..
https://chromium.googlesource.com/catapult
BSD 3-Clause "New" or "Revised" License
1.93k stars 564 forks source link

Dashboard - Commit range not showing with histograms #4398

Closed simonhatch closed 6 years ago

simonhatch commented 6 years ago

What's happening is there are multiple ways to specify the exact same thing, and different uploaders have used different names (ie. the most recent r_webrtc_git vs r_webrtc_rev). Switching from one to another mid-timeseries results in a single instance of the UI not properly showing the range, since it looks at the current and previous point, looks up the annotation info for each, and then displays the range.

Ie. point A has r_chromium_commit_pos and point B has r_commit_pos, so it won't be able to find r_chromium_commit_pos and will just show the single end commit. Subsequent points will all have r_chromium_commit_pos and so the range should in theory show just fine again.

We could introduce some lookup code in chart-container:getRevisions that looks for an equivalent annotation if the current one isn't available. Or we could just ignore this, should be a once off.

The upside is that the histogram code path enforces a specific name, so we won't run into this again in the future.

@eakuefner @anniesullie

anniesullie commented 6 years ago

Any reason the histogram code enforces different names than the existing ones?

simonhatch commented 6 years ago

Not really, I kinda figured if they just needed to be valid names and tried to be consistent across them. We have the annotation name is generated in the endpoint before the row is saved out, vs being supplied by the uploader.

ie. webrtc_git, v8_git, chromium_commit_pos

I could go look at chromium.perf uploads, it's

r_commit_pos, r_chromium, r_v8_rev, r_webrtc_git

v8 uploads seem to use r_v8_git though

simonhatch commented 6 years ago

We ended up choosing the chromium.perf names as the standard.