Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Bug 1609081 - Provide full server side search in Alerts view #5835

Merged
merged 2 commits into from Feb 17, 2020
Merged

Bug 1609081 - Provide full server side search in Alerts view #5835

merged 2 commits into from Feb 17, 2020

Conversation

ionutgoldan
Copy link
Contributor

No description provided.

@ionutgoldan ionutgoldan force-pushed the encapsulate-filter-text-search branch 2 times, most recently from 6e0946e to da3e62c Compare January 16, 2020 15:05
@codecov-io
Copy link

codecov-io commented Jan 16, 2020 •
edited

Codecov Report

Merging #5835 into master will increase coverage by 0.01%.
The diff coverage is 79.46%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5835      +/-   ##
==========================================
+ Coverage   72.68%   72.69%   +0.01%     
==========================================
  Files         456      456              
  Lines       18733    18796      +63     
  Branches     1511     1521      +10     
==========================================
+ Hits        13616    13664      +48     
- Misses       4790     4806      +16     
+ Partials      327      326       -1
Impacted Files Coverage Δ
ui/perfherder/Validation.jsx 48.38% <ø> (ø) ⬆️
ui/perfherder/App.jsx 0% <ø> (ø) ⬆️
ui/perfherder/FilterControls.jsx 93.75% <100%> (ø) ⬆️
ui/shared/InputFilter.jsx 68% <54.54%> (-14.36%) ⬇️
treeherder/webapp/api/performance_data.py 85.9% <68.96%> (-1.6%) ⬇️
ui/perfherder/alerts/AlertsView.jsx 84.55% <83.33%> (+0.34%) ⬆️
ui/perfherder/alerts/AlertsViewControls.jsx 90.9% <95.65%> (+3.4%) ⬆️
ui/models/optionCollection.js 100% <0%> (+11.11%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6e09f58...c4cc1b3. Read the comment docs.

@ionutgoldan ionutgoldan force-pushed the encapsulate-filter-text-search branch 5 times, most recently from 72d7529 to a7f0132 Compare January 29, 2020 12:47
@ionutgoldan ionutgoldan marked this pull request as ready for review January 30, 2020 12:39
@airimovici airimovici force-pushed the encapsulate-filter-text-search branch 2 times, most recently from 87c8063 to 9bd233e Compare February 3, 2020 07:57
@ionutgoldan ionutgoldan force-pushed the encapsulate-filter-text-search branch 2 times, most recently from 6a2f3da to 9306e2e Compare February 4, 2020 15:00
@airimovici airimovici force-pushed the encapsulate-filter-text-search branch from 75e2431 to 8ec7a79 Compare February 7, 2020 14:33
ionutgoldan
ionutgoldan commented Feb 10, 2020
View reviewed changes
ui/perfherder/alerts/AlertsView.jsx Outdated Show resolved Hide resolved
ui/perfherder/alerts/AlertsView.jsx Show resolved Hide resolved
ui/perfherder/alerts/AlertsView.jsx Outdated Show resolved Hide resolved
@ionutgoldan ionutgoldan force-pushed the encapsulate-filter-text-search branch 2 times, most recently from fdf7ca6 to 7a7bfee Compare February 10, 2020 11:32
octavian-negru
octavian-negru approved these changes Feb 14, 2020
View reviewed changes
Copy link
Contributor

@octavian-negru octavian-negru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

@ionutgoldan
Copy link
Contributor Author

@sarah-clements could you review this PR?

@ionutgoldan
Copy link
Contributor Author

ionutgoldan commented Feb 14, 2020 •
edited

@camd & @armenzg could you please review the backend code, which does the query building part?

@ionutgoldan
Copy link
Contributor Author

ionutgoldan commented Feb 14, 2020 •
edited

@armenzg I noticed you're using treeherder-prototype for testing bug 1608427's PR. We need to let the perf sheriffs test this PR on that environment as well, to have their feedback.

Could you lend us treeherder-prototype for next Monday (Feb 24)? Thanks!

@ionutgoldan
Copy link
Contributor Author

I've also added the perf sheriffs as reviewers, in advance, so they're more aware of this PR.

@ionutgoldan ionutgoldan changed the title Bug 1609081 - Encapsulate client side search for filter text Bug 1609081 - Provide full server side search in Alerts view Feb 14, 2020
@armenzg
Copy link
Collaborator

armenzg commented Feb 14, 2020

I've released prototype.

armenzg
armenzg reviewed Feb 14, 2020
View reviewed changes
Copy link
Collaborator

@armenzg armenzg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not as familiar with Django so I can't add a lot of value to the review of the backend code.
I don't see anything outrageous in the code.

sarah-clements
sarah-clements reviewed Feb 14, 2020
View reviewed changes
treeherder/webapp/api/performance_data.py
.values('id')
.distinct())

return queryset.filter(id__in=Subquery(filtered_summaries))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these results paginated?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, they are automatically paginated by our custom Django-based code. More precisely, here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't remember if I've mentioned this before, but you might find the CustomPaginator useful if you end up profiling this API for performance and see issues. In the past, the count method was performing a full table scan when returning paginated results.

sarah-clements
sarah-clements approved these changes Feb 14, 2020
View reviewed changes
Copy link
Contributor

@sarah-clements sarah-clements left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not familiar with django filters so I can't add anything of value regarding it's use. As far as the performance issues Ionut mentioned in the last Perfherder meeting, if that is still an issue I suggest pushing the branch to prototype to test the API and using new relic to give a more detailed analysis of the query than the django debug toolbar can give.

In the future, it'd be preferable to review changes of this size as two separate prs: one for API/backend changes and one for the UI front end changes (even if front end changes are rebased on the backend pr and start from a specific commit, just that commit could be reviewed). It makes it much easier to review, in my opinion.

@ionutgoldan ionutgoldan temporarily deployed to treeherder-prototype February 17, 2020 09:43 Inactive
@ionutgoldan ionutgoldan temporarily deployed to treeherder-prototype February 17, 2020 10:53 Inactive
marianrai
marianrai approved these changes Feb 17, 2020
View reviewed changes
Copy link

@marianrai marianrai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@ionutgoldan ionutgoldan merged commit eec3bfe into mozilla:master Feb 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@armenzg armenzg armenzg left review comments

@sarah-clements sarah-clements sarah-clements approved these changes

@airimovici airimovici airimovici left review comments

@marianrai marianrai marianrai approved these changes

@alexandru-io alexandru-io alexandru-io approved these changes

@octavian-negru octavian-negru octavian-negru approved these changes

@camd camd Awaiting requested review from camd

@bebef1987 bebef1987 Awaiting requested review from bebef1987

Assignees

@ionutgoldan ionutgoldan

@airimovici airimovici

Labels
javascript python
Projects
None yet
Milestone
No milestone
8 participants
@ionutgoldan @codecov-io @armenzg @sarah-clements @marianrai @alexandru-io @airimovici @octavian-negru