Upgrade to Pro — share decks privately, control downloads, hide ads and more …

Refactoring Systems with Confidence

Refactoring Systems with Confidence

Talk by Jesse Toth and Nathan Witmer, given at OSCON 2015.

At GitHub, we recently replaced a large subsystem of our application – the permissions code – with a faster and more flexible version. In our talk, we’ll share our approach to this large-scale rewrite of a critical piece of our Rails application, and how we accomplished this feat while both preserving the performance of our app and proving the new technology over the course of the project.

We’ll go over these general themes:

* An explanation of the challenge, including a brief overview of the permissions models, both old and new
* Our approach to testing, validating, and then changing our codebase to use a new system for authorization, using a Ruby library we created to help us with this task
* How we used production traffic and data to verify the correctness of our changes and reveal data quality issues
* How we used metrics and gradual rollouts to track performance and errors as we integrated the new permissions system
* Examples of the user interface we built to control and visualize our experiments
* Some stories about what went wrong (and right!) as we made these modifications.

After this session, we hope the attendees will gain a new perspective on ways to tackle a large-scale refactoring project with confidence.

While code examples will be in Ruby, no Ruby knowledge is required.

Nathan Witmer

July 23, 2015
Tweet

More Decks by Nathan Witmer

Other Decks in Programming

Transcript

  1. SELECT  r.id   FROM  r,  (      SELECT  r.id

     as  r_ids,  2  as  perms  from  r      WHERE  r.owner_id  =  99999      AND  (r.x  =  0)      UNION  ALL      SELECT  r.id  as  r_ids,  1  as  perms  from  r      INNER  JOIN  p  ON  r.id  =  p.r_id      WHERE  p.u_id  =  99999      AND  (r.x  =  0)      UNION  ALL      SELECT  r.id  as  r_ids,  2  as  perms  from  r      INNER  JOIN                t  ON  r.o_id  =  t.o_id      INNER  JOIN  t_m  ON  t.id  =  t_m.t_id      WHERE  t.name  =  'X'      AND  t_m.u_id  =  99999      AND  (r.x  =  0)      UNION  ALL      SELECT  r.id  as  r_ids,  GROUP_CONCAT(distinct  t.p)  as  perms  from  r      INNER  JOIN  t_m  r_t  ON  r.id  =  r_t.r_id      INNER  JOIN                                    t  ON  r_t.t_id  =  t.id      INNER  JOIN  t_m  u_t  ON  t.id  =  u_t.t_id      WHERE  u_t.u_id  =  99999      AND  t.name  !=  'X'      AND  t.p  in  (2,  1,  0)      AND  (r.x  =  0)      GROUP  BY  r.id      UNION  ALL      SELECT  r.id  as  r_ids,  0  as  perms  from  r      JOIN  u  ON  r.plan_owner_id  =  u.id      JOIN  t  ON  t.o_id  =  u.id      JOIN  t_m  ON  t.id  =  t_m.t_id      WHERE  u.type  =  'XX'      AND  t.name  =  'X'      AND  t_m.u_id  =  99999      AND  (r.x  =  0)      AND  r.parent_id  IS    NOT  NULL  )  AS  unioned   WHERE  r.id  =  r_ids; user.accessible_repositories
  2. User read Team Team write Repository User write Repository User

    Team Repository action Actor Subject read write write
  3. user.can?  :read,  repository SELECT  1  FROM  abilities    WHERE  actor_id

             =  user_id        AND  actor_type      =  'User'        AND  subject_id      =  repository_id        AND  subject_type  =  'Repository'
  4. user.accessible_repositories SELECT  subject_id      FROM  abilities    WHERE  actor_id

             =  user_id        AND  actor_type      =  'User'        AND  subject_type  =  'Repository'
  5. If test coverage is thin, you can’t be sure that

    the tests you add fully cover all behavior — especially if the indended behavior is not 100% clear in the collective knowlege of the team
  6. class  Repository      def  pullable_by?(user)        

     #  old  code...      end   end  
  7. class  Repository      def  pullable_by?(user)        

     pullable_by_legacy?(user)      end      def  pullable_by_legacy?(user)          #  old  code      end   end  
  8. class  Repository      def  pullable_by?(user)        

     pullable_by_legacy?(user)      end      def  pullable_by_legacy?(user)          #  old  code      end      def  pullable_by_refactored?(user)          #  new  code      end   end  
  9. class  Repository      include  Scientist      def  pullable_by?(user)

             pullable_by_legacy?(user)      end      def  pullable_by_legacy?(user)          #  old  code      end      def  pullable_by_refactored?(user)          #  new  code      end   end  
  10. class  Repository      include  Scientist      def  pullable_by?(user)

             #  Let's  do  an  experiment          science  "repository.pullable_by"  do  |experiment|          end      end   end  
  11. class  Repository      include  Scientist      def  pullable_by?(user)

             #  Let's  do  an  experiment          science  "repository.pullable_by"  do  |experiment|              #  Return  this  value  no  matter  what              experiment.use  {  pullable_by_legacy?(user)  }          end      end   end  
  12. class  Repository      include  Scientist      def  pullable_by?(user)

             #  Let's  do  an  experiment          science  "repository.pullable_by"  do  |experiment|              #  Return  this  value  no  matter  what              experiment.use  {  pullable_by_legacy?(user)  }              #  Run  the  new  code  too,  and  compare  the  results              experiment.try  {  pullable_by_refactored?(user)  }          end      end   end  
  13. class  Repository      include  Scientist      def  pullable_by?(user)

             #  Let's  do  an  experiment          science  "repository.pullable_by"  do  |experiment|              #  Return  this  value  no  matter  what              experiment.use  {  pullable_by_legacy?(user)  }              #  Run  the  new  code  too,  and  compare  the  results              experiment.try  {  pullable_by_refactored?(user)  }              #  Some  context  for  published  results              experiment.context  :user  =>  user,  :repo  =>  self          end      end   end
  14. class  Repository      include  Scientist      def  pullable_by?(user)

             #  Let's  do  an  experiment          science  "repository.pullable_by"  do  |experiment|              #  Return  this  value  no  matter  what              experiment.use  {  pullable_by_legacy?(user)  }              #  Run  the  new  code  too,  and  compare  the  results              experiment.try  {  pullable_by_refactored?(user)  }              #  Some  context  for  published  results              experiment.context  :user  =>  user,  :repo  =>  self          end      end   end
  15. Metrics • num times the experiment has run • num

    times the use and try blocks’ return values differed • timings of each block’s execution Redis • return values of use and try blocks
 for experiments that mismatched
  16. User Team Repository read write write 1 Team with 10,000

    Users × 5,000 Repositories = 50,000,000 rows
  17. User Team Repository read write write User read Team Team

    write Repository User write Repository action Actor Subject