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 563 forks source link

Add ability to retrieve alerts through dashboard API regardless of sheriffing rotation #4479

Closed zeptonaut closed 6 years ago

zeptonaut commented 6 years ago

This is basically this CL but for the alerts API instead of the timeseries API. This is more intuitive for most folks trying to do data analysis on their benchmark.

Basically, we want to add an "all" keyword that the API recognizes as eliminating the alert filter based on the sheriff rotation.

benshayden commented 6 years ago

V2SPA also needs to do this, and also needs to be able to filter by bug_id, improvements, and triaged and eventually revisions and report name. We'll need a new index. I was hoping to mail out that change in a week or so, but both @simonhatch and @perezju will be OOO, so I'd probably wait for one of them to return unless @anniesullie was comfortable. Do you want to add those features at the same time as you add your feature? Or do you want to focus on your feature first and let me add mine later? I'm happy to follow your lead. :-)

anniesullie commented 6 years ago

I'd be happy to review any CLs for this. Maybe it'd be best to start with a small CL to add the index? I'd be happy to approve it and walk someone through turning on the index.

zeptonaut commented 6 years ago

I don't really know what's involved in doing this, so I'm happy to relinquish control of this bug. The fact that you know that this might involve a new index makes it sound like you already know a whole lot more about how to access this data than I do

benshayden commented 6 years ago

The new index is being generated (see pantheon (can I link it here?)). When that process finishes (should be about a day?), I'll test it and mail out that CL. Another CL can wire up the api alerts handler to call the new Anomaly.GetAll() classmethod, and further followups can migrate other queries to that method. Eventually, we should be able to remove the other indexes and vacuum them to save a few GB storage. @eakuefner mentioned that @simonhatch vacuumed recently, so that shouldn't cause any problems, but I'll wait for everybody to come back from OOO just in case.

benshayden commented 6 years ago

Hm, the new index doesn't seem to be working how I expected. Currently, index.yaml specifies several indexes over 3, 4, or 5 properties to support individual queries. The API and V2SPA need several new queries that aren't supported by the existing indexes. I had hoped that a single index over all the properties could support a large variety of queries, but that might not be how it works. While reading ndb docs, I found this article. Basically, the datastore can use the zigzag merge join algorithm to support a large variety of different queries by joining multiple indexes over pairs of properties. So it could join one index sheriff, -timestamp with another is_improvement, -timestamp to support a query that specifies both a sheriff and is_improvement and orders by -timestamp. We could remove the existing index is_improvement, sheriff, -timestamp without changing any code. The datastore would be able to join the new pair indexes with other new pair indexes to replace the other 3-, 4-, and 5-property indexes and support new queries.

I'll try a few more things to see if I can make the single index work before creating a few smaller, joinable indexes. Let me know if I'm missing something!

benshayden commented 6 years ago

I believe the dashboard side of this is done. Did you want to use this bug to track a change to soundwave or close this?

perezju commented 6 years ago

@zeptonaut you wanted to look at adding the option to query for all sheriff rotations?

perezju commented 6 years ago

I've also just found that the API call from soundwave was returning bogus results:

I'll take this to make the needed soundwave changes, also adding @benshayden to fix warning/fall-back on chromeperf api.

perezju commented 6 years ago

I've also just noted. If we get a next_cursor on soundwave, what do we do with it? which chromeperf API do I call to follow it?

Update: Sorry, I've just read the API docs again. Ignore this question.