Closed Bug 1289540 Opened 5 years ago Closed 2 years ago
alert pagination make it difficult to find alerts
47 bytes, text/x-github-pull-request
|Details | Review|
I am looking for windows 8 compositor_video alerts: https://treeherder.mozilla.org/perf.html#/alerts?status=4&framework=1&filter=video&page=1 I want to see a larger range of alerts, I suspect some are from the same root cause and can be combined- the problem is that I have to click page by page and the ones I see on the previous page disappear. I would love a way to search for an alert by branch, platform, test and then all results are paginated, vs now where all possible alerts are paginated and any search is done on the single page.
Yeah, I think we should change the filter fields to change the query we make against the server, and paginate that (instead of filtering through the paginated results). Not trivial, but probably worth doing.
Hey Will, Joel! I'd like to take on this bug! What's a good place to start reading the code?
We talked about this a bit on irc. Most of the changes will need to be in how we handle filtering on the server. In particular we'll need to add the ability to filter by test, etc. based on an input filter string. The django rest framework model that will need to be modified is here: https://github.com/mozilla/treeherder/blob/ef46fa0/treeherder/webapp/api/performance_data.py#L209 I recommend testing this as you go by writing a unit test, you can see some existing performance alert summary api tests here: https://github.com/mozilla/treeherder/blob/ef46fa0/tests/webapp/api/test_performance_alertsummary_api.py Once that's working, the client side logic to do the filtering is in this model method: https://github.com/mozilla/treeherder/blob/ef46fa0/ui/js/models/perf/alerts.js#L246 You'll also need to modify the controller to call into the model correctly (instead of doing client-side filtering): https://github.com/mozilla/treeherder/blob/ef46fa0/ui/js/controllers/perf/alerts.js
Hey Will, I went through the JS code and I think I understand it well now. So if I understand correctly, there are a total of five filters - status, framework, hide_improvements, filter (the string filter) and page. Currently, status, framework and page have been implemented in the Django API. I need to integrate the string filter and the hide_improvements filter as well in order to prevent the pagination problem right?
I'm thinking of using http://www.django-rest-framework.org/api-guide/filtering/#searchfilter to perform the text filtering on the "title" field of the alert. How does it sound? For the hideImprovements filter, I think I will pass "is_regression" in the API request. Copying from the Django docs, """ When using boolean fields, you should use the values True and False in the URL query parameters, rather than 0, 1, true or false. (The allowed boolean values are currently hardwired in Django's NullBooleanSelect implementation.) """ under "Hints and Tips" for http://www.django-rest-framework.org/api-guide/filtering/#specifying-a-filterset Hence I will pass a is_regression=True / is_regression=False and add it to API request. It will be added in this tuple of the API https://github.com/mozilla/treeherder/blob/ef46fa0/treeherder/webapp/api/performance_data.py#L221 How does this workflow sound?
(In reply to Kalpesh Krishna [:martianwars] from comment #5) > I'm thinking of using > http://www.django-rest-framework.org/api-guide/filtering/#searchfilter to > perform the text filtering on the "title" field of the alert. How does it > sound? > > For the hideImprovements filter, I think I will pass "is_regression" in the > API request. Copying from the Django docs, > """ > When using boolean fields, you should use the values True and False in the > URL query parameters, rather than 0, 1, true or false. (The allowed boolean > values are currently hardwired in Django's NullBooleanSelect implementation.) > """ under "Hints and Tips" for > http://www.django-rest-framework.org/api-guide/filtering/#specifying-a- > filterset > > Hence I will pass a is_regression=True / is_regression=False and add it to > API request. It will be added in this tuple of the API > https://github.com/mozilla/treeherder/blob/ef46fa0/treeherder/webapp/api/ > performance_data.py#L221 > > How does this workflow sound? So in general this sounds right, but there are a few details which make this more complicated than you'd thank: 1. These properties which you want to filter on are properties of *alerts*, not "alert summaries" which are what we are fetching. So you'll need to create a search which only return alert summary objects which have alerts with the properties we care about (title or "is_regression"). I believe Django rest framework should support this, but you'll have to do some experimentation on how to do it. 2. We currently calculate the title of the alert on the client side (https://github.com/mozilla/treeherder/blob/ef46fa0/ui/js/models/perf/alerts.js#L11), so the Django search filter won't work. I think it would be ok to give each alert a title text property though (and search for that). This would involve modifying the Django model to add a charfield/textfield to represent this information, writing the code to determine what it should be, and use that on the creation of new alerts. You'll also need to write a small migration script to add that property to old alerts, which were created before this existed (there are lots of examples of this type of script in `treeherder/model/management/commands`.
Comment on attachment 8786782 [details] [review] [treeherder] martiansideofthemoon:afresh > mozilla:master Will, I've started off by applying the migration. I've tried to emulate a **very** basic version of https://github.com/mozilla/treeherder/blob/master/ui/js/models/perf/series.js#L25-L35 while constructing the title, but I'm not sure how feasible this is and what are the correct options to have.
Attachment #8786782 - Flags: feedback?(wlachance)
Comment on attachment 8786782 [details] [review] [treeherder] martiansideofthemoon:afresh > mozilla:master This looks like it's on the right track, I left some suggestions in the PR.
Attachment #8786782 - Flags: feedback?(wlachance) → feedback+
Hey :torroid, I don't think I will be able to get back to this anytime soon. This is a little involved, but I think you will do a good job of it. I'd done the first part in the PR if I remember correctly. Would you be interested?
Hey! Ya sure I'd like to give it a try.
Hey! Ya sure I'd like to give it a try.
Sorry about the double comment.
That's great! You should read all the comments leading up to this very carefully and try to understand the PR I've made a month ago. I should remember most of it, and Will should be able to tell you more about the bug. To begin with, I recommend you to go through the "Perfherder enhancement QoC" and "Getting Started" section here https://wiki.mozilla.org/Auto-tools/New_Contributor/Quarter_of_Contribution/Perfherder. This will tell you what Perfherder is. You luckily have it all setup. Just a matter to run it in the correct way!
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1564833
You need to log in before you can comment on or make changes to this bug.