search

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

Add support for URL tuples to generic-set-span #4397

Closed eakuefner closed 5 years ago

eakuefner commented 6 years ago

Basically, we should add BUILDS to the add_reserved_diagnostics call in the recipe, and then plumb BUILDS in /add_histograms.

eakuefner commented 6 years ago

Okay, so this is actually appearing in the data set, just some corner case in graph_json.py is preventing this from working completely properly. We should just modify graph_json to make this work in the sane case.

eakuefner commented 6 years ago

This turned out to be a little more complicated than originally anticipated. As it happens, the a_ columns are always Markdown-formatted links. The easiest way of making this work is to allow tuples of [name, url] on the GenericSet side that we then squash to Markdown in /add_histograms. So to make this work, we need to:

  1. Modify the recipe and the src-side OBBS script to add these tuples instead of plain URLs
  2. Modify /add_histograms to munge these tuples into Markdown links for storage as annotations (this will fix this bug)
  3. Add support to generic_set_span for rendering link tuples.
eakuefner commented 6 years ago

Having completed quite a lot of traveling and meetings, I expect to finally crack this today. I'm going to do step 2 (Catapult-side) first so that the Dashboard knows what to do with these tuple URLs once we start producing them, then do step 1 (which necessarily has to be done as two separate patches to the recipe and to the merge script, which lives in src/), then do step 3 as a followup.

eakuefner commented 6 years ago

After the src- and recipe-side changes cycle through to the bots, we should also add an assert that makes sure that LOG_URLS diagnostics only contain URL tuples. It might also be a good idea to add such functionality when addressing #3770.

eakuefner commented 6 years ago

Thinking about this, another problem here is that add_reserved_diagnostics doesn't really know how to accept list arguments right now. Ideally we'd autogenerate _k and _v arguments for reserved names that have link tuple entry type, but I think for now, let's just special case it for LOG_URLS.

eakuefner commented 6 years ago

This is fixed.

eakuefner commented 6 years ago

Reopening, forgot to add support to generic-set-span for this. Will queue that up.

eakuefner commented 5 years ago

@benshayden Assigning this over to you; all that's left to do is add support for parsing URL tuples to generic-set-span.

benshayden commented 5 years ago

https://chromium-review.googlesource.com/c/catapult/+/1392448