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.

Transcript

1. Refactoring Systems with Confidence Jesse Toth and Nathan Witmer OSCON 2015
2. ! # $ # %
3. Domain Model &
4. Permissions
5. access control search result filtering event feeds
6. Repository User
7. Repository User Collaborator
8. Repository User Collaborator Organization
9. Repository User Collaborator Team Repository Team Organization Team Member
10. Repository User Collaborator Team Repository Team Admin Team Organization Team Member
11. repository.pullable_by?(user)
12. Organic growth and complexity
13. Performance problems
14. 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
15. Edge cases and transitional states
16. Difficult to build on
17. A New Permissions System '
18. Goals
19. Simple, flexible interface
20. Fast lookups
21. Easy to integrate and operate
22. Abilities
23. action Actor Subject
24. action Actor Subject User Team
25. action Actor Subject User Team read write admin
26. action Actor Subject User Team read write admin Team Repository
27. User Team Repository action Actor Subject
28. User Team Repository action Actor Subject read User read Team
29. User Team Repository action Actor Subject read write User read Team Team write Repository
30. User read Team Team write Repository User write Repository User Team Repository action Actor Subject read write write
31. 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'
32. user.accessible_repositories SELECT  subject_id      FROM  abilities    WHERE  actor_id          =  user_id        AND  actor_type      =  'User'        AND  subject_type  =  'Repository'
33. Replacing the permissions system
34. Refactoring with Scientist $
35. You can’t be confident that test cases fully cover the complexity of real-world data
36. 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
37. Current behavior may have unintended bugs and side-effects that users are relying on!
38. Test suites don’t cover production performance
39. Production data is the real test
40. Compare return values Legacy Code Refactored Code repository.pullable_by?(user) execute execute return result Metrics publish
41. Scientist A Ruby library for carefully refactoring critical paths
42. class  Repository      def  pullable_by?(user)          #  old  code...      end   end  
43. class  Repository      def  pullable_by?(user)          pullable_by_legacy?(user)      end      def  pullable_by_legacy?(user)          #  old  code      end   end  
44. 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  
45. 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  
46. class  Repository      include  Scientist      def  pullable_by?(user)          #  Let's  do  an  experiment          science  "repository.pullable_by"  do  |experiment|          end      end   end  
47. 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  
48. 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  
49. 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
50. 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
51. 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
58. Backfill and Validation (
59. Legacy Permissions Abilities read & verify
60. Legacy Permissions Abilities write read & verify
61. Legacy Permissions Abilities write read & verify backfill
62. Legacy Permissions Abilities write read & verify backfill
63. Legacy Permissions Abilities write read & verify backfill repair
64. Legacy Permissions Abilities write read & verify backfill repair
65. Legacy Permissions Abilities write read & verify backfill & validation repair
67. Legacy Permissions Abilities write read & verify backfill & validation repair
68. The Results %
72. Lessons Learned )
73. Production Data is the Real Test
74. Data Quality is Paramount
75. Math is Important
76. User Team Repository read write write 1 Team with 10,000 Users × 5,000 Repositories
77. User Team Repository read write write 1 Team with 10,000 Users × 5,000 Repositories = 50,000,000 rows
78. User Team Repository read write write User read Team Team write Repository User write Repository action Actor Subject
81. Double-Check your Queries
83. More Uses of Scientist *
84. Pipeline to render HTML, Markdown, and Blobs
85. Load-testing a new search cluster
86. Testing performance and correctness of query changes
87. +/github/scientist
88. Thanks! ♥