Kiril Mitov blog

Tagged: cancancan Toggle Comment Threads | Keyboard Shortcuts

• 

  kmitov 6:34 am on April 2, 2021 Permalink | Log in to leave a Comment
  Tags: cancancan, databases, fllcasts ( 5 ), new relic, performance, rails ( 40 ), rails 6 ( 14 )   

  Resolving a performance issue in a Rails platform. cancancan, New relic, N+1 

  (Everyday Code – instead of keeping our knowledge in a README.md let’s share it with the internet)

  Today’s article is about a performance issue that we had in one of our Rails platforms and how we resolved it. I enter into details of the specific root cause and how New relic was helpful in identifying the issue. This article is mainly targeted for our team, but I hope it would be useful for the community to see a practical example of cancancan and N+1.

  The root cause

  Every time we show a lesson on the FLLCasts platform we show the tutorials and materials of that lesson. But if the user does not have access to a tutorial, because it is not published or it requires a teachers subscription we do not show it in the lesson. We filter it from the lesson.

  We are using cancancan and we’ve added code for checking if every tutorial in the course could be accessed – whether you have the right subscription. This was not a problem on its own. The major problem was that we were showing a navigation of the course on the right that contains links to all the lessons with all the tutorials:

  

  To show the navigation we looped through all the tutorials in the whole course and checked if the user has access to them. It was an N+1. This seems to be working when a course contains < 100 tutorials. But once the courses grew and contained more tutorials the logic got slow.

  People started noticing. We had to increase the number of Heroku dynos from time to time. Especially when a lot of students join and use the platform.

  There were two problems

  1. We don’t need to show all the tutorials and building instructions in the navigation. We just need to show them for the current lesson.
  2. We need to make a singe query to the DB that would get only the tutorials in that lesson and only the tutorials the user has access to.

  What was the performance impact

  During load when a lot of students were using the platform the navigation/_course_section partial was taking 41% of the show lesson request average of 5 seconds. There were about 100 find queries for a single lesson.

  

  It was taking about 10-15 seconds for the server to respond and sometimes requests were failing.

  

  On the chart below we see how the moment people start opening courses the time for showing lessons (course_sections) starts growing.
  

  

  Practically the platform was unusable in this case. Everything stopped.

  Solutions

  Change the navigation to include only the current lesson

  We decided that we don’t need to show all the tutorials from the whole course in the navigation, only the tutorials for the current lessons. This reduced the load because instead of returning 140 tutorials we were returning 20.

  Improve the call to cancancan

  One thing that we’ve missed is a call to cancancan that is can? :show for each record. It has slipped in the years and we haven’t noticed.

  records.each do |record|
     if can? :show, record
       ...
     end
  end

  I refactored it to use cancancan accessible_by method.

  @content_refs = @course_section
          .content_refs
          .accessible_by(current_ability)
          .order(:position)

  and added two new :index rules in the ContentRefs ability. The benefit is that in this way cancancan makes a single query to the db.

  module Abilities
    class ContentRefsAbility
      include CanCan::Ability
  
      def initialize user
        can :index, ContentRef, { archived: [nil,false], hidden: false}
        can :index, ContentRef, { archived: true, hidden: [true,false], course_section: { authors: { user_id: user.id }}}
        ...
        can :read, ContentRef, { archived: [nil,false], hidden: true ,course_section: {course: {required_access_for_hidden_refs: {weight: 0..user_max_plan_weight}}}}
        ...
      end
    end
  end

  The third rule is the deal breaker. With it we check if the user has a subscription plan that would allow them to access the content ref.
  

  With these three rules we allow everybody to see non archive and non hidden content_refs, we also allow authors of the course_section to see archived and hidden content_refs where they are the author of the course_section and we allow allow everybody to see hidden content_refs if they have the proper subscription plan.

  Results

  Number of timeouts reduced to from 180 to 6

  jenkins@elvis:/var/log/fllcasts$ grep "Rack::Timeout::RequestTimeoutException" access.log | wc -l
  6
  jenkins@elvis:/var/log/fllcasts$ zgrep "Rack::Timeout::RequestTimeoutException" access.log-20210320.gz | wc -l
  183
  jenkins@elvis:/var/log/fllcasts$ zgrep "Rack::Timeout::RequestTimeoutException" access.log-20210319.gz | wc -l
  164

  Increase in throughout put does not increase the required time

  It does not matter whether we have 1 or 40 rpm for this controller. The time stays the same. Next we can focus on how to make it even faster, but this will do until the platform grows x10

  

   

  Reply Cancel reply

  You must be logged in to post a comment.

• 

  kmitov 9:32 pm on January 11, 2021 Permalink | Log in to leave a Comment
  Tags: acts_as_paranoid, cancancan, globalize-rails, i18n ( 2 ), rails ( 40 ), rails 6 ( 14 )   

  The problems with acts_as_paranoid and generally with Soft Delete of records in Rails 

  I am about to refactor and completely remove acts_as_paranoid from one of the models in our platform. There are certain issues that were piling up in the last few years and it is now already difficult to support it. As I am about to tell this to colleagues tomorrow I thought to first structure the summary in an article and directly send the article to the whole team.

  If you are thinking about using acts_as_paranoid and generally soft delete your records then this article could give you a good understanding of what to expect.

  Disclaimer: I am not trying to roast acts_as_paranoid here. I think it is a great gem that I’ve used successfully for years and it has helped me save people’s work where they accidentally delete something. It could be exactly what you need. For us it got too entangled and dependent on workarounds with other gems and I am planning to remove it.

  acts_as_paranoid VS Globalize – problem 1

  We have a model called Material. Material has a title. The title is managed by globalize as we are delivering the content in a number of languages.

  class Material < ApplicationRecord
    acts_as_paranoid
    translates :title
  end

  The problem is that Globalize knows nothing about acts_as_paranoid. You can delete a Material, and it should delete the translations, but when you try to recover the Material then there is an error because of how the translations are implemented and the order in which the translations and the Material are recovered. Which record should be recovered first? Details are at https://github.com/ActsAsParanoid/acts_as_paranoid/issues/166#issuecomment-650974373, but as a quote here

  Ok, then I think I see what happens: ActsAsParanoid tries to recover your translations but their validation fails because they are recovered before the main object. When you just call recover this means the records are just not saved and the code proceeds to save the main record. However, when you call recover!, the validation failure leads to the exception you see.

  In this case, it seems, the main record needs to be recovered first. I wonder if that should be made the default.

  I have work around this and I used it like this for an year

  module TranslatableSoftdeletable
    extend ActiveSupport::Concern
  
    included do
  
      translation_class.class_eval do
        acts_as_paranoid
      end
  
      before_recover :restore_translations
    
    end
  
    def restore_translations
      translations.with_deleted.each do |translation|
        translation.deleted_at = nil
        translation.save(validate: false)
      end
      self.translations.reload
    end
  
  end

  acts_as_paranoid VS Globalize – problem 2

  Let’s say that you delete a Material. What is the title of this deleted material?

  material = Material.find(1)
  material.destroy
  material.title == ?

  Remember that the Material has all of its translations for the title in a table that just got soft deleted. So the correct answer is “nil”. The title of the delete material is nil.

  material = Material.find(1)
  material.destroy
  material.title == nil # is true

  You can workaround this with

  material.translations.with_deleted.where(locale: I18n.locale).first.title

  But this is quite ugly.

  acts_as_paranoid VS cancancan

  The material could have authors. An author is the connection between the User and the Material. We are using cancancan for the controllers.

  In the controller for a specific author we show only the models that are for this author. We have the following rule:

  can [:access, :read, :update, :destroy], clazz, authors: { user_id: user.id }
  

  Here the problem is that you can only return non deleted records. If you would like to implement a Trash/Recycle Bin/Bin functionality for the Materials that you can not reuse the rule. The reason is the cancancan can not have can? and cannot? with an sql statement and the “authors: {user_id: user.id}” is an sql statement.

  What we could do is to use scopes

  # in the ability
  can [:access, :read, :update, :destroy], Material, Material.with_authors(user.id)
  
  # In the Material
  scope :with_authors, -> (user_ids) {
      joins(:authors).where(authors: {user_id: user_ids})
  }
  

  We move the logic for authorization from the ability to the model. I can live with that. It is not that outrageous. But it does not work, because the association with authors will return empty authors for the Material as the materials are also soft deleted.

  acts_as_paranoid vs cancancan – case 2

  When declaring load_and_authorize in the controller it will not fetch records that are already deleted. So you can not use cancancan to load in the controller a record that is deleted and you must load it yourself.

  class MaterialsTrashController < ApplicationController
  
    before_action only: [:show, :edit, :update, :destroy] do
      @content_picture = Material.only_deleted.find(params[:id])
    end
  
  
    load_and_authorize_resource :content_picture, class: "Material", parent: false
  
  end

  This was ugly, but as we had one trash controller it was kind of acceptable. But with a second one it got more difficult and deviated from the other controllers logic.

  acts_as_paranoid vs ContentPictures

  Every Material has many ContentPictures on our platform. There is a ContentPictureRef model. One of the pictures could be the thumbnail. Once you delete a material what is the thumbnail that we can show for this material?

  As the content_picture_refs relation as also soft deleted we should change the logic for returning the thumbnail. We change it from

  def thumbnail
      thumbnail_ref = self.content_picture_refs.where(content_picture_type:  :thumbnail).first
  end

  to

  def thumbnail
      thumbnail_ref = self.content_picture_refs.with_deleted.where(content_picture_type:  :thumbnail).first
  end

  I can live with this. Even relation that we have for the deleted record we must call with “with_deleted”. ContentPictures, Authors and other relations all should be changed for us to be able to see the deleted materials in a table that represents the Trash.

  acts_as_paranoid vs Active Record Callbacks

  There is another issue right at https://github.com/ActsAsParanoid/acts_as_paranoid/issues/168

  It took me hours to debug it a few months back. The order of before_destroy should not really matter.

  class Episode < ApplicationRecord
     acts_as_paranoid
     before_destroy do 
        puts self.authors.count
     end
     has_many :authors, dependent: :destroy
  
  class Episode < ApplicationRecord
     before_destroy do 
        puts self.authors.count
     end
     acts_as_paranoid 
     has_many :authors, dependent: :destroy

  acts_as_paranoid vs belongs_to

  By using acts_as_paranoid belongs_to should be:

  class Author < ApplicationRecord
     belongs_to :material, with_deleted: true
  end

  I could also live with this. At a certain stage it was easier to just add with_deleted: true to the belongs_to relations.

  acts_as_paranoid vs Shrine

  So the Material has a Shrine attachment stored on Amazon S3. What should happen when you delete the Material. Should you delete the file from S3? Of course not. If you delete the file you will not be able to recover the file.

  The solution was to modify the Shrine Uploader

  class Attacher < Shrine::Attacher
  
    def activerecord_after_destroy 
      # Just dont call super and keep the file
      # Sine objects at BuildIn3D & FLLCasts are softdeleted we want to 
      # handle the deletion logic in another way.
      true
    end
  
  end
  

  I was living with this for months and did not had many issues.

  What Pain and Problem are we addressing?

  The current problem that I can not find a workaround for is the MaterialsTrashController that is using CanCanCan. All the solutions would require for this controller to be different than the rest of the controllers and my biggest concern is that this will later result in issues. I would like to have a single place were we check if a User has access to the Material and whether they could read or update it or recover it. If we split the logic of the MaterialsController and the MaterialsTrashController we would end up with a hidden and difficult to maintain duplication.

  But was is the real problem that we want to solve?

  On our platform we have authors for the instructions and we work close with them. I imagine one particular author that I will call TM (Taekwondon Master). So TM uploads a material and from time to time he incidentally could delete a material. That’s it. When he deletes a Material it should not be deleted, but rather put in a trash. Then when he deletes it from the trash he must confirm with a blood sample. That’s it. I just want to stop TM from losing any Material by accident.

  The solution is pretty simple.

  In the MaterialsController just show all the materials that do not have a :deleted_at column set.

  In the MaterialsTrashController just show only the Materials with :delete_at controller.

  I can solve the whole problem with one simple filter that would take me like 1 minute to implement. We don’t need any of the problems above. They simply will not exist.

  That’s it. I will start with the Material and I will move through the other models as we are implementing the additional Trash controllers for the other models.