4117 - Clean up ahoy records after roll up #4168
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WillNigel23
commented
Jun 19, 2024
| 'static#landing_page', | ||
| 'transcribe#display_page', | ||
| 'transcribe#save_transcription' | ||
| ] |
There was a problem hiding this comment.
I moved some relevant constants here.
WillNigel23
commented
Jun 19, 2024
lib/ahoy_activity_utils.rb
Outdated
| exclude_actions = AhoyActivitySummary::WEEKLY_TRIAL_COHORT_TARGET_ACTIONS + | ||
| AhoyActivitySummary::WEEKLY_TRANSCRIBER_COHORT_TARGET_ACTIONS | ||
|
|
||
| Ahoy::Event.where('date < ?', keep_after_date).where.not(name: exclude_actions).destroy_all |
There was a problem hiding this comment.
Can you let me know if this looks right to you @benwbrum ?
In ticket it was mentioned we delete Ahoy::Events and Visits, but it seems there is no 'action' field for Visits. Do I delete only based on started_at for Visits then?
There was a problem hiding this comment.
This looks correct to me.
On events versus visits, one Visit may have many Ahoy::Event child records. Generally speaking, a Visit corresponds to a user session, while an Event corresponds to a click or HTTP request.
You can see this in action if you go to the Admin dashboard and click on the Users tab, then click Visits for a user, and drill into a few Visits.
I'm trying to think about the ways we might clean up Visit records. Since one is created for each session--including anonymous sessions or bots/spiders--we do want to clean these as well. We certainly want to delete Visit records that have no child Ahoy::Event records after the Event clean-up.
Any ideas on how to do this in a performant manner?
WillNigel23
commented
Jun 19, 2024
| @@ -2,7 +2,7 @@ namespace :fromthepage do | |||
| desc "weekly transcriber cohort" | |||
| task :weekly_transcriber_cohort => :environment do | |||
| # generate a csv file of users who signed up in the last week and write it out to a temporary file | |||
| TRANSCRIBER_TARGET_ACTIONS = ['static#landing_page', 'registrations#new', 'registrations#create', 'transcribe#display_page', 'transcribe#save_transcription'] | |||
| target_actions = AhoyActivitySummary::WEEKLY_TRANSCRIBER_COHORT_TARGET_ACTIONS | |||
There was a problem hiding this comment.
Moved these to constants
WillNigel23
commented
Jun 20, 2024
lib/ahoy_activity_utils.rb
Outdated
| AhoyActivitySummary::WEEKLY_TRANSCRIBER_COHORT_TARGET_ACTIONS | ||
|
|
||
| Ahoy::Event.where('date < ?', keep_after_date).where.not(name: exclude_actions).destroy_all | ||
| Visit.left_joins(:ahoy_events).where(ahoy_events: { id: nil }).destroy_all |
There was a problem hiding this comment.
@benwbrum this should delete all Visits without ahoy_events. Let me know if this is good for you
There was a problem hiding this comment.
Let's add a constraint on visit deletion to only delete visits older than 2 weeks (or whatever the constant is). We have used the Visit data to track down problems with scrapers and bots.
WillNigel23
force-pushed
the
4117-clean-up-ahoy-records-after-roll-up
branch
4 times, most recently
from
5f2a0be to
6f3e6c2
Compare
|
I've tested this and run into an exception: |
WillNigel23
force-pushed
the
4117-clean-up-ahoy-records-after-roll-up
branch
from
2863313 to
23ab536
Compare
benwbrum
reviewed
Jun 30, 2024
lib/ahoy_activity_utils.rb
Outdated
| exclude_actions = AhoyActivitySummary::WEEKLY_TRIAL_COHORT_TARGET_ACTIONS + | ||
| AhoyActivitySummary::WEEKLY_TRANSCRIBER_COHORT_TARGET_ACTIONS | ||
|
|
||
| Ahoy::Event.where('time < ?', keep_after_date).where.not(name: exclude_actions).destroy_all |
There was a problem hiding this comment.
I tried running this locally in my development environment, and the process ran for an hour and got up to 15GB of memory before I killed it.
Can we switch these two lines to call delete_all instead of destroy_all?
There was a problem hiding this comment.
Done. Although, without proper callbacks we are producing orphaned records this way (I believe deeds will be affected). Are we sure that is okay?
There was a problem hiding this comment.
That's a good point. Ahoy events should be leaf nodes in the data model, and Ahoy Visits without any events should also be leaf nodes. Let me take a look at the deed data model and think harder about this.
There was a problem hiding this comment.
Okay, Deed has a foreign key to Ahoy::Visit, which we use when showing which deeds were created during a particular session on the Admin->Users->Visits listing. We never travel in the other direction in the application, so we shouldn't run into a nil visit when displaying a deed.
That said, it would be nice to clean these up. Perhaps we could get the list of visit_ids which we are planning to delete, call delete_all on them, then call Deed.where(visit_id: visit_ids_to_delete).update_all(visit_id: nil)?
There was a problem hiding this comment.
Can you please try again and see how is the performance with this one.
We do destroy_all in batches. Maybe that would help.
There was a problem hiding this comment.
Please git pull --rebase or reset to head if there are any conflicts
WillNigel23
force-pushed
the
4117-clean-up-ahoy-records-after-roll-up
branch
from
ebbea3b to
b4fdb17
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.