Support async_count for scopes #8394
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #8394 +/- ##
=======================================
Coverage 99.10% 99.11%
=======================================
Files 140 141 +1
Lines 4029 4055 +26
=======================================
+ Hits 3993 4019 +26
Misses 36 36 ☔ View full report in Codecov by Sentry. |
|
Hi @javierjulio. I was wondering if there was any interest in this PR from the ActiveAdmin maintainers. If not, I have no issue closing it. Sorry about the direct ping, I was looking for a maintainer to nudge. |
|
@jdlubrano thank you. Just have to be patient for us to find time. I agree this can be a helpful feature. I haven't tried the new async querying capability in Rails yet so I'll have to rely on others for input. Looking at the scopes DSL in our docs, it seems best that this would be an option per scope and that it should be an opt-in feature. I'm not sure about your last question though. What need do you see for having the ability to override the class? |
|
Thanks so much for the feedback! I will work on tweaking this to rely on a per-scope, opt-in action. Given the per-scope option, I don't see a need to leave the class open to overrides. |
c974cd1 to
7f0ab34
Compare
|
I have updated scopes to accept an option like scope(:my_scope, show_count: :async)as a way to opt-in to async queries. If someone uses The PR description has also been updated accordingly. |
7f0ab34 to
58c1c00
Compare
There was a problem hiding this comment.
@jdlubrano thanks for your work. I like the PR feature.
I wrote a few comments. I want to know your thoughts about them.
I hope I have more time tomorrow to keep reviewing the PR.
71e7125 to
f4205c3
Compare
f4205c3 to
32dfecf
Compare
There was a problem hiding this comment.
@mgrunberg I think this is ready and looks good to me. Would you be able to test it out?
There was a problem hiding this comment.
@javierjulio looks good to me.
I tested it on my machine and it works. I have a simple suggestion before merging.
Sorry for the oversight!
Co-authored-by: Javier Julio <jjfutbol@gmail.com>
Co-authored-by: Matias Grunberg <matias@yellowspot.dev>
8b044d9 to
415ffb0
Compare
Background
In ActiveAdmin applications, a surprising performance bottleneck can be the
COUNTqueries that run to provide counts for each scope made available on an resource's index page. In many cases, performance issues can be addressed by adding missing indices and/or otherwise speeding up the underlying query.There is another performance boost that could be low-hanging fruit, however. The
COUNTqueries for scopes currently run sequentially. In Rails 7.1, the framework provides theasync_countmethod that allows queries to be run in parallel. For index pages with many scopes (and many COUNTs) running queries in parallel could offer a significant time savings.This PR will use
async_countto query the collection size of scopes in versions of Rails whereasync_countis available. Rails apps not running on Rails 7.1 will continue to rely on sequential queries.Approach
This PR introduces an
ActiveAdmin::AsyncCountclass that quacks enough like an ActiveRecord Relation to play nicely with thecollection_sizehelper. TheActiveAdmin::Views::Scopesclass eagerly initializesAsyncCounts for each scope that is configured likewhich kicks off async queries. As a result, rendering the scopes component should only be as slow as its slowest
COUNT.Closed Questions
Should individual scopes have an(I chooseasync_count: trueoption?show_count: :asyncto leverage the existingshow_countattribute)Should the index options support a(It seems easy enough to configure many scopes to usescope_count: :asyncoption?async_countusing thewith_optionsRails/ActiveSupport helper. Example:Should consumers be able to configure their ownNot necessaryScopeCounterclass globally and/or per index?Credits
@tuffylock @aturar