Rewriting Critical Parts of Your Application Using... Science!

The best advice I ever received about code was to never get attached to it, because eventually it will need to change. Things always change; and a robust system must be able to respond to these changes to survive and grow. You might be able to manage the small changes along the way, but eventually as your application gets older, there will come a day where you're going to have to make a big change -- a change so big that you're not just changing a piece of the system anymore, you're rewriting it.
The scariest thing about a rewrite is that usually the old system is very complicated and its behavior isn't completely documented, so it's hard to be sure that any new system you create replaces the functionality of the old system completely. This might be good enough for certain rewrites -- it might even be a good opportunity to rethink some behavior -- but there are other cases where you can't afford to be unsure about the correctness of the system you're replacing because it's too important.
At GitHub, we found ourselves at a point where we needed to rewrite the system that models our permissions in order to be able to build new features and improve performance. This talk will cover the strategies we developed to rewrite this critical piece of the app side-by-side with the existing code, specifically how we used our open-source science and analysis libraries to rewrite small pieces at a time and ensure that everything worked when we finally flipped the switch.

Transcript

1. Rewriting Critical Parts of Your Application Using... @jesseplusplus - GitHub Science!
2. !
3. a rewrite ? ! ? !
4. history
5. ! First, there was collaboration
6. ! Then, there was organization
7. ! ! separate ways of granting permissions living side-by-side " tons of bugs and edge cases around transitional states # performance degradation
8. !
9. !
10. rewrite !
11. ! Interface
12. ! user.can? :read, repository repository.grant user, :write repository.revoke user
13. ! | :read, :write, :admin |
14. ! +------+ | User |-------------- +------+ +------+ | Team |-------------- +------+ +------------+ | Repository |-------- +------------+
15. ! +------+ | User |---------------- +------+ +------+ 1 | Team |--------+------- +------+ | | +------------+ v | Repository |---------- +------------+
16. ! +------+ 2 | User |-----------+---- +------+ | | +------+ 1 v | Team |--------+------- +------+ | | +------------+ v | Repository |---------- +------------+
17. ! +------+ 2 3 | User |-----------+--+- +------+ | . | . +------+ 1 v . | Team |--------+-----.- +------+ | . | . +------------+ v v | Repository |---------- +------------+
18. ! performance #
19. ! existing API easy to compare class Repository # Is the given user allowed # to pull this repository? def pullable_by?(user) # is the user a collaborator # or a member of a team with access # or part of some other edge case? . . . end end
20. ! lists of repositories, pull requests, teams, etc. # Find all repositories which this organization # controls the access to. ! def organization.controlled_repositories . . . end
21. ! lists of repositories, pull requests, teams, etc. # Find all pull requests to which this user # has access. Access to PRs follow the user’s # access rights to the PR’s repository. ! def user.accessible_pull_requests . . . end
22. ! 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;
23. ! test coverage : less than ideal
24. rewrite …and refactor
27. ! class Repository def pullable_by?(user) science "ability.repository.pullable-by" do |e| e.context :user => (user.id if user), :repo => id ! e.control { pullable_by_legacy?(user) } e.candidate { pullable_by_abs?(user) } end end ! def pullable_by_abs?(user) user.can? :read, self end ! def pullable_by_legacy?(user) # original code for pullable_by? # collaborators || team || special case end end
28. ! class Repository def pullable_by?(user) science "ability.repository.pullable-by" do |e| e.context :user => (user.id if user), :repo => id ! e.control { pullable_by_legacy?(user) } e.candidate { pullable_by_abs?(user) } end end ! def pullable_by_abs?(user) user.can? :read, self end ! def pullable_by_legacy?(user) # original code for pullable_by? # collaborators || team || special case end end
29. ! class Repository def pullable_by?(user) science "ability.repository.pullable-by" do |e| e.context :user => (user.id if user), :repo => id ! e.control { pullable_by_legacy?(user) } e.candidate { pullable_by_abs?(user) } end end ! def pullable_by_abs?(user) user.can? :read, self end ! def pullable_by_legacy?(user) # original code for pullable_by? # collaborators || team || special case end end
30. ! class Repository def pullable_by?(user) science "ability.repository.pullable-by" do |e| e.context :user => (user.id if user), :repo => id ! e.control { pullable_by_legacy?(user) } e.candidate { pullable_by_abs?(user) } end end ! def pullable_by_abs?(user) user.can? :read, self end ! def pullable_by_legacy?(user) # original code for pullable_by? # collaborators || team || special case end end
31. ! class Repository def pullable_by?(user) science "ability.repository.pullable-by" do |e| e.context :user => (user.id if user), :repo => id ! e.control { pullable_by_legacy?(user) } e.candidate { pullable_by_abs?(user) } end end ! def pullable_by_abs?(user) user.can? :read, self end ! def pullable_by_legacy?(user) # original code for pullable_by? # collaborators || team || special case end end
32. ! class Repository def pullable_by?(user) science "ability.repository.pullable-by" do |e| e.context :user => (user.id if user), :repo => id ! e.control { pullable_by_legacy?(user) } e.candidate { pullable_by_abs?(user) } end end ! def pullable_by_abs?(user) user.can? :read, self end ! def pullable_by_legacy?(user) # original code for pullable_by? # collaborators || team || special case end end
33. ! class Science::Experiment def publish(event, payload) instrument "science.#{event}", payload end end
34. subscribe /^science\./ do |name, . . ., payload| experiment = "science.#{payload[:experiment]}" ! increment "#{experiment}.total" timing "#{experiment}.control", payload[:control][:duration] timing "#{experiment}.candidate", payload[:candidate][:duration] end ! subscribe /^science\.mismatch/ do |name, . . ., payload| experiment = "science.#{payload[:experiment]}" ! increment "#{experiment}.wrong" Redis.lpush "#{experiment}.mismatch", payload.to_json end
35. subscribe /^science\./ do |name, . . ., payload| experiment = "science.#{payload[:experiment]}" ! increment "#{experiment}.total" timing "#{experiment}.control", payload[:control][:duration] timing "#{experiment}.candidate", payload[:candidate][:duration] end ! subscribe /^science\.mismatch/ do |name, . . ., payload| experiment = "science.#{payload[:experiment]}" ! increment "#{experiment}.wrong" Redis.lpush "#{experiment}.mismatch", payload.to_json end
36. subscribe /^science\./ do |name, . . ., payload| experiment = "science.#{payload[:experiment]}" ! increment "#{experiment}.total" timing "#{experiment}.control", payload[:control][:duration] timing "#{experiment}.candidate", payload[:candidate][:duration] end ! subscribe /^science\.mismatch/ do |name, . . ., payload| name = payload[:experiment] experiment = "science.#{name}" ! increment "#{experiment}.wrong" Redis.lpush "#{experiment}.mismatch", payload.to_json end
37. subscribe /^science\./ do |name, . . ., payload| experiment = "science.#{payload[:experiment]}" ! increment "#{experiment}.total" timing "#{experiment}.control", payload[:control][:duration] timing "#{experiment}.candidate", payload[:candidate][:duration] end ! subscribe /^science\.mismatch/ do |name, . . ., payload| experiment = "science.#{payload[:experiment]}" ! increment "#{experiment}.wrong" Redis.lpush "#{experiment}.mismatch", payload.to_json end
38. subscribe /^science\./ do |name, . . ., payload| experiment = "science.#{payload[:experiment]}" ! increment "#{experiment}.total" timing "#{experiment}.control", payload[:control][:duration] timing "#{experiment}.candidate", payload[:candidate][:duration] end ! subscribe /^science\.mismatch/ do |name, . . ., payload| experiment = "science.#{payload[:experiment]}" ! increment "#{experiment}.wrong" Redis.lpush "#{experiment}.mismatch", payload.to_json end
39. subscribe /^science\./ do |name, . . ., payload| experiment = "science.#{payload[:experiment]}" ! increment "#{experiment}.total" timing "#{experiment}.control", payload[:control][:duration] timing "#{experiment}.candidate", payload[:candidate][:duration] end ! subscribe /^science\.mismatch/ do |name, . . ., payload| experiment = "science.#{payload[:experiment]}" ! increment "#{experiment}.wrong" Redis.lpush "#{experiment}.mismatch", payload.to_json end
40. ! class Experiment < Science::Experiment def enabled? self.class.enabled? && rand(100) < percentage end end
41. ! class Experiment < Science::Experiment def enabled? self.class.enabled? && rand(100) < percentage end end
42. ! class Experiment < Science::Experiment def enabled? self.class.enabled? && rand(100) < percentage end end
43. ! science all the things
44. ! $ examining the data
46. !
47. !
48. ! analyzing mismatches
49. ! class Analysis def read Redis.rpop "science.#{experiment_name}.mismatch" end def count Redis.llen "science.#{experiment_name}.mismatch" end end
50. ! irb(main):001> analysis = Analysis.new('ability.repository.pullable-by') => #<Analysis:0x007fd4c7f5c4d8 @experiment_name="ability.repository.pullable-by", @wrappers=[], @registry=#<Analysis::Registry:0x007fd4c7f72b98 @known_classes=[]>> ! irb(main):002> analysis.count => 3283 ! irb(main):003> result = analysis.fetch => {"from"=>"GitHub::Jobs::ProcessEvent", "experiment"=>"ability.repository.pullable-by", "user"=>XXXXXXX, “repo"=>XXXXXXX, "timestamp"=>"2014-05-28T18:53:16-07:00", "candidate"=>{"duration"=>0.41368900000000003, "exception"=>nil, "value"=>false}, "control"=>{"duration"=>1.141963, "exception"=>nil, "value"=>true}, "first"=>"control"} !
51. ! irb(main):001> analysis = Analysis.new('ability.repository.pullable-by') => #<Analysis:0x007fd4c7f5c4d8 @experiment_name="ability.repository.pullable-by", @wrappers=[], @registry=#<Analysis::Registry:0x007fd4c7f72b98 @known_classes=[]>> ! irb(main):002> analysis.count => 3283 ! irb(main):003> result = analysis.fetch => {"from"=>"GitHub::Jobs::ProcessEvent", "experiment"=>"ability.repository.pullable-by", "user"=>XXXXXXX, “repo"=>XXXXXXX, "timestamp"=>"2014-05-28T18:53:16-07:00", "candidate"=>{"duration"=>0.41368900000000003, "exception"=>nil, "value"=>false}, "control"=>{"duration"=>1.141963, "exception"=>nil, "value"=>true}, "first"=>"control"} !
52. ! irb(main):001> analysis = Analysis.new('ability.repository.pullable-by') => #<Analysis:0x007fd4c7f5c4d8 @experiment_name="ability.repository.pullable-by", @wrappers=[], @registry=#<Analysis::Registry:0x007fd4c7f72b98 @known_classes=[]>> ! irb(main):002> analysis.count => 3283 ! irb(main):003> result = analysis.fetch => {"from"=>"GitHub::Jobs::ProcessEvent", "experiment"=>"ability.repository.pullable-by", "user"=>XXXXXXX, “repo"=>XXXXXXX, "timestamp"=>"2014-05-28T18:53:16-07:00", "candidate"=>{"duration"=>0.41368900000000003, "exception"=>nil, "value"=>false}, "control"=>{"duration"=>1.141963, "exception"=>nil, "value"=>true}, "first"=>"control"} !
53. ! irb(main):001> analysis = Analysis.new('ability.repository.pullable-by') => #<Analysis:0x007fd4c7f5c4d8 @experiment_name="ability.repository.pullable-by", @wrappers=[], @registry=#<Analysis::Registry:0x007fd4c7f72b98 @known_classes=[]>> ! irb(main):002> analysis.count => 3283 ! irb(main):003> result = analysis.fetch => {"from"=>"GitHub::Jobs::ProcessEvent", "experiment"=>"ability.repository.pullable-by", "user"=>XXXXXXX, “repo"=>XXXXXXX, "timestamp"=>"2014-05-28T18:53:16-07:00", "candidate"=>{"duration"=>0.41368900000000003, "exception"=>nil, "value"=>false}, "control"=>{"duration"=>1.141963, "exception"=>nil, "value"=>true}, "first"=>"control"} !
54. ! irb(main):001> analysis = Analysis.new('ability.repository.pullable-by') => #<Analysis:0x007fd4c7f5c4d8 @experiment_name="ability.repository.pullable-by", @wrappers=[], @registry=#<Analysis::Registry:0x007fd4c7f72b98 @known_classes=[]>> ! irb(main):002> analysis.count => 3283 ! irb(main):003> result = analysis.fetch => {"from"=>"GitHub::Jobs::ProcessEvent", "experiment"=>"ability.repository.pullable-by", "user"=>XXXXXXX, “repo"=>XXXXXXX, "timestamp"=>"2014-05-28T18:53:16-07:00", "candidate"=>{"duration"=>0.41368900000000003, "exception"=>nil, "value"=>false}, "control"=>{"duration"=>1.141963, "exception"=>nil, "value"=>true}, "first"=>"control"} !
55. ! " % &'
56. ! % data quality
57. ! &'
58. … and repair rewrite …and refactor …and $ $ … and #
59. !
60. soon . . .
61. ! http://github.com/ github/dat-science ( dat-science ( dat-analysis http://github.com/github/dat-analysis
62. ! Muchas Gracias!