Historical Trend Speed up #3665

seanmarcia · 2023-06-17T16:11:00Z

This is a somewhat substantial PR that resolves #3592

It attempts to resolve some of the current problems with the historical data pages as well as speed them up through caching.

Things this PR does:

Makes the chart go back a full year and make it relative to the current month rather than to just the start of the year for all banks.
It only shows the items that are active on a bank rather than all their items regardless if they use them or not.
Updates the query to only look at the last year rather than all time -- this should speed up when an ad hoc query needs to be run.
Creates a rake task cache_historical_data that will cache the historical data for all banks. This should be run once a day.
Created a background job that the rake task will run so that it can run in the background.
Updated the buttons on the historical data page to be more consistent with the rest of the site.
Made the charts all deselected by default.
Adds an is_active scope to the organization model to determine if an organization is active. It does this by querying if any of the org users have logged on in the past 4 months.
Most importantly it makes the loading of the charts really, really fast. 👍

Things to consider:

The rake task will need to be scheduled.
There wasn't a caching strategy defined in the production.rb so I used :redis_cache_store which is the most common and we already have redis installed.
I saw we use url: ENV['REDIS_URL'] elsewhere on the site (for coverband) so I assumed that was the correct environment variable to use for redis.

Screenshots

Before:

After:

…ke it more usable.

cielf · 2023-06-22T22:03:39Z

"Updates the query to only look at the last year rather than all time -- this should speed up when an ad hoc query needs to be run." -- Ultimately I'd like to see this going further back in time, so they can see trends. But I think we need some more thought, and some people actually using it at all before that's an issue.

dorner

The overall technical approach looks good to me! I had some suggestions to firm up the safety and performance around this.

dorner · 2023-06-23T19:30:08Z

lib/tasks/cache_historical_data.rake

@@ -0,0 +1,16 @@
+desc "This task is run by a scheduling tool nightly to cache the processor intensive queries"
+task :cache_historical_data => :environment do
+  puts "Caching historical data"


Should we be using Rails.logger here instead of puts?

Switched to logger with info

dorner · 2023-06-23T19:30:52Z

lib/tasks/cache_historical_data.rake

+  puts "Caching historical data"
+  DATA_TYPES = ['Distribution', 'Purchase', 'Donation']
+
+  orgs = Organization.all


Is there some way for us to filter out old / inactive organizations from this list?

Have we got a definition of what an inactive organization is? (Have we marked them as such somehow?)

Nope, but we could define one as (say) one where no one has logged in for over 3 months.

I'd say 4, just in case there's someone working on a quarterly basis. I doubt it, but I haven't ruled it out.

Done. Created an is_active scope that looks to see if the organization has any users that have logged in in the past 4 months.

dorner · 2023-06-23T19:31:36Z

spec/jobs/historical_data_cache_job_spec.rb

+  end
+
+  it "caches the historical data" do
+    expect(Rails.cache).to receive(:write).with("#{organization.short_name}-historical-#{type}-data", anything)


Can we get a more thorough test that shows the actual data being written? Even if it means stubbing out the HistoricalTrendService.

Fixed. Stubbed out the response.

dorner · 2023-06-23T19:32:14Z

spec/services/historical_trend_service_spec.rb

+        {name: "Item 2", data: [0, 0, 0, 0, 0, 0, 0, 0, 30, 0, 0, 0], visible: false}
+      ]
+      expect(service.series).to eq(expected_result)
+    end


Can we add a couple more tests here where we time travel to different months? Just to make sure nothing weird happens around the year end.

Done. On the first item I created a donation line item for each of the 12 months in the series with the line item 12 months ago having 120, 11 having 110, etc.

…gned in in the past 4 months. Fleshed out tests, updated rake task to use logger.

dorner

Looks good to me!

@seanmarcia @cielf is there anything we need to do from a migration perspective here? Or are we good to merge it as is?

cielf · 2023-06-30T19:46:52Z

@dorner Well, we have to set up the job to run once a day, right? How do we make that happen a) on staging and then, of course, remembering that it has to also be set up in prod.

dorner · 2023-06-30T19:52:22Z

We can do something super dumb with e.g. https://github.com/jmettraux/rufus-scheduler. Or we can use https://github.com/sidekiq-cron/sidekiq-cron .

cielf · 2023-06-30T21:09:42Z

We have other periodic jobs, right (like the backups?) Makes sense to me to use the same method on all of them.

cielf · 2023-07-22T18:59:43Z

@dorner Let's talk about the actual procedure for getting this in with the periodic job too on Sunday

scooter-dangle · 2023-07-23T14:47:47Z

@seanmarcia, per Sunday planning, would this be better to use a delayed job or the cron capability in Cloud66? (And can you do this plumbing? 🪠)

seanmarcia · 2023-07-23T14:52:45Z

Happy to help with whatever. I was thinking of using the delay_job_recurring gem to handle this (and other similar) tasks.

Went with Rufus-scheduler since it is active an maintained.

awwaiid · 2023-07-28T19:58:54Z

Thank you @seanmarcia !

cielf · 2023-07-28T20:00:01Z

Cool. Thanks, @seanmarcia !

awwaiid · 2023-07-30T16:57:18Z

Tagged as RFG only because we pushed it the last bit while being together!

cielf · 2023-07-30T17:04:01Z

I'm certainly for counting it!

seanmarcia force-pushed the historical_trends_caching branch 2 times, most recently from 43ebc9c to 9616a9a Compare June 17, 2023 16:22

First pass at the clean up and caching of the historical trends to ma…

bd30366

…ke it more usable.

seanmarcia force-pushed the historical_trends_caching branch from 9616a9a to bd30366 Compare June 20, 2023 02:57

dorner requested changes Jun 23, 2023

View reviewed changes

Add an is_active scope on organizations to check if any users have si…

2a4dd1f

…gned in in the past 4 months. Fleshed out tests, updated rake task to use logger.

dorner approved these changes Jun 30, 2023

View reviewed changes

seanmarcia added 3 commits July 24, 2023 00:16

Add in the rufus-scheduler (a well maintained gem) to run the rake task

277eac1

Merge branch 'main' into historical_trends_caching

5892c79

Fix for broken test.

8fa8b31

awwaiid merged commit 0971ee0 into main Jul 28, 2023
12 checks passed

awwaiid deleted the historical_trends_caching branch July 28, 2023 19:58

awwaiid added the Ruby for Good 2023 DC Created for Ruby for Good 2023 DC label Jul 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Historical Trend Speed up #3665

Historical Trend Speed up #3665

seanmarcia commented Jun 17, 2023 •

edited

Loading

cielf commented Jun 22, 2023 •

edited

Loading

dorner left a comment

dorner Jun 23, 2023

seanmarcia Jun 24, 2023

dorner Jun 23, 2023

cielf Jun 23, 2023

dorner Jun 23, 2023

cielf Jun 23, 2023

seanmarcia Jun 24, 2023

dorner Jun 23, 2023

seanmarcia Jun 24, 2023

dorner Jun 23, 2023

seanmarcia Jun 24, 2023

dorner left a comment

cielf commented Jun 30, 2023

dorner commented Jun 30, 2023

cielf commented Jun 30, 2023

cielf commented Jul 22, 2023

scooter-dangle commented Jul 23, 2023 •

edited

Loading

seanmarcia commented Jul 23, 2023 •

edited

Loading

awwaiid commented Jul 28, 2023

cielf commented Jul 28, 2023

awwaiid commented Jul 30, 2023

cielf commented Jul 30, 2023

Historical Trend Speed up #3665

Historical Trend Speed up #3665

Conversation

seanmarcia commented Jun 17, 2023 • edited Loading

Things this PR does:

Things to consider:

Screenshots

cielf commented Jun 22, 2023 • edited Loading

dorner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dorner left a comment

Choose a reason for hiding this comment

cielf commented Jun 30, 2023

dorner commented Jun 30, 2023

cielf commented Jun 30, 2023

cielf commented Jul 22, 2023

scooter-dangle commented Jul 23, 2023 • edited Loading

seanmarcia commented Jul 23, 2023 • edited Loading

awwaiid commented Jul 28, 2023

cielf commented Jul 28, 2023

awwaiid commented Jul 30, 2023

cielf commented Jul 30, 2023

seanmarcia commented Jun 17, 2023 •

edited

Loading

cielf commented Jun 22, 2023 •

edited

Loading

scooter-dangle commented Jul 23, 2023 •

edited

Loading

seanmarcia commented Jul 23, 2023 •

edited

Loading