$30 off During Our Annual Pro Sale. View Details »

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

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.

Jesse Toth

June 05, 2014
Tweet

More Decks by Jesse Toth

Other Decks in Programming

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

  25. None
  26. None
  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

  45. None
  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!