From 2e283c5d667f0c9324b06a78454b651eca481e4f Mon Sep 17 00:00:00 2001 From: Cyril Mougel Date: Wed, 15 May 2013 09:45:46 +0200 Subject: [PATCH] Refactor the problem merge system --- app/interactors/problem_merge.rb | 26 ++++++++++++++++++++++++++ app/interactors/problem_updater_cache.rb | 90 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ app/models/notice.rb | 8 ++------ app/models/problem.rb | 43 ++----------------------------------------- lib/tasks/errbit/database.rake | 4 +++- spec/fabricators/problem_fabricator.rb | 10 ++++++++++ spec/interactors/problem_merge_spec.rb | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ spec/interactors/problem_updater_cache_spec.rb | 153 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ spec/models/backtrace_line.rb | 12 ------------ spec/models/backtrace_line_spec.rb | 12 ++++++++++++ spec/models/problem_spec.rb | 43 +++++++++++++++++++------------------------ 11 files changed, 365 insertions(+), 84 deletions(-) create mode 100644 app/interactors/problem_merge.rb create mode 100644 app/interactors/problem_updater_cache.rb create mode 100644 spec/interactors/problem_merge_spec.rb create mode 100644 spec/interactors/problem_updater_cache_spec.rb delete mode 100644 spec/models/backtrace_line.rb create mode 100644 spec/models/backtrace_line_spec.rb diff --git a/app/interactors/problem_merge.rb b/app/interactors/problem_merge.rb new file mode 100644 index 0000000..4c7492a --- /dev/null +++ b/app/interactors/problem_merge.rb @@ -0,0 +1,26 @@ +require 'problem_destroy' + +class ProblemMerge + def initialize(*problems) + problems = problems.flatten.uniq + @merged_problem = problems[0] + @child_problems = problems[1..-1] + raise ArgumentError.new("need almost 2 uniq different problems") if @child_problems.empty? + end + attr_reader :merged_problem, :child_problems + + def merge + child_problems.each do |problem| + merged_problem.errs.concat Err.where(:problem_id => problem.id) + ProblemDestroy.execute(problem) + end + reset_cached_attributes + merged_problem + end + + private + + def reset_cached_attributes + ProblemUpdaterCache.new(merged_problem).update + end +end diff --git a/app/interactors/problem_updater_cache.rb b/app/interactors/problem_updater_cache.rb new file mode 100644 index 0000000..7a53106 --- /dev/null +++ b/app/interactors/problem_updater_cache.rb @@ -0,0 +1,90 @@ +class ProblemUpdaterCache + def initialize(problem, notice=nil) + @problem = problem + @notice = notice + end + attr_reader :problem + + ## + # Update cache information about child associate to this problem + # + # update the notices count, and some notice informations + # + # @return [ Problem ] the problem with this update + # + def update + update_notices_count + update_notices_cache + problem + end + + private + + def update_notices_count + if @notice + problem.inc(:notices_count, 1) + else + problem.update_attribute( + :notices_count, problem.notices.count + ) + end + end + + ## + # Update problem statistique from some notice information + # + def update_notices_cache + first_notice = notices.first + last_notice = notices.last + notice ||= @notice || first_notice + + attrs = {} + attrs[:first_notice_at] = first_notice.created_at if first_notice + attrs[:last_notice_at] = last_notice.created_at if last_notice + attrs.merge!( + :message => notice.message, + :environment => notice.environment_name, + :error_class => notice.error_class, + :where => notice.where, + :messages => attribute_count(:message, messages), + :hosts => attribute_count(:host, hosts), + :user_agents => attribute_count(:user_agent_string, user_agents) + ) if notice + problem.update_attributes!(attrs) + end + + def notices + @notices ||= @notice ? [@notice].sort(&:created_at) : problem.notices.order_by([:created_at, :asc]) + end + + def messages + @notice ? problem.messages : {} + end + + def hosts + @notice ? problem.hosts : {} + end + + def user_agents + @notice ? problem.user_agents : {} + end + + private + + def attribute_count(value, init) + init.tap do |counts| + notices.each do |notice| + counts[attribute_index(notice.send(value))] ||= { + 'value' => notice.send(value), + 'count' => 0 + } + counts[attribute_index(notice.send(value))]['count'] += 1 + end + end + end + + def attribute_index(value) + @attributes_index ||= {} + @attributes_index[value.to_s] ||= Digest::MD5.hexdigest(value.to_s) + end +end diff --git a/app/models/notice.rb b/app/models/notice.rb index adc6e57..f428af6 100644 --- a/app/models/notice.rb +++ b/app/models/notice.rb @@ -26,7 +26,7 @@ class Notice ] ) - after_create :increase_counter_cache, :cache_attributes_on_problem, :unresolve_problem + after_create :cache_attributes_on_problem, :unresolve_problem before_save :sanitize before_destroy :decrease_counter_cache, :remove_cached_attributes_from_problem @@ -116,10 +116,6 @@ class Notice protected - def increase_counter_cache - problem.inc(:notices_count, 1) - end - def decrease_counter_cache problem.inc(:notices_count, -1) if err end @@ -133,7 +129,7 @@ class Notice end def cache_attributes_on_problem - problem.cache_notice_attributes(self) + ProblemUpdaterCache.new(problem, self).update end def sanitize diff --git a/app/models/problem.rb b/app/models/problem.rb index 6002901..29fc693 100644 --- a/app/models/problem.rb +++ b/app/models/problem.rb @@ -83,15 +83,7 @@ class Problem def self.merge!(*problems) - problems = problems.flatten.uniq - merged_problem = problems.shift - problems.each do |problem| - merged_problem.errs.concat Err.where(:problem_id => problem.id) - problem.errs(true) # reload problem.errs (should be empty) before problem.destroy - problem.destroy - end - merged_problem.reset_cached_attributes - merged_problem + ProblemMerge.new(problems).merge end def merged? @@ -128,9 +120,7 @@ class Problem def reset_cached_attributes - update_attribute(:notices_count, notices.count) - cache_app_attributes - cache_notice_attributes + ProblemUpdaterCache.new(self).update end def cache_app_attributes @@ -145,26 +135,6 @@ class Problem end end - def cache_notice_attributes(notice=nil) - first_notice = notices.order_by([:created_at, :asc]).first - last_notice = notices.order_by([:created_at, :asc]).last - notice ||= first_notice - - attrs = {} - attrs[:first_notice_at] = first_notice.created_at if first_notice - attrs[:last_notice_at] = last_notice.created_at if last_notice - attrs.merge!( - :message => notice.message, - :environment => notice.environment_name, - :error_class => notice.error_class, - :where => notice.where, - :messages => attribute_count_increase(:messages, notice.message), - :hosts => attribute_count_increase(:hosts, notice.host), - :user_agents => attribute_count_increase(:user_agents, notice.user_agent_string) - ) if notice - update_attributes!(attrs) - end - def remove_cached_notice_attributes(notice) update_attributes!( :messages => attribute_count_descrease(:messages, notice.message), @@ -184,15 +154,6 @@ class Problem end private - def attribute_count_increase(name, value) - counter, index = send(name), attribute_index(value) - if counter[index].nil? - counter[index] = {'value' => value, 'count' => 1} - else - counter[index]['count'] += 1 - end - counter - end def attribute_count_descrease(name, value) counter, index = send(name), attribute_index(value) diff --git a/lib/tasks/errbit/database.rake b/lib/tasks/errbit/database.rake index 151e40d..e23d84d 100644 --- a/lib/tasks/errbit/database.rake +++ b/lib/tasks/errbit/database.rake @@ -6,7 +6,9 @@ namespace :errbit do desc "Updates cached attributes on Problem" task :update_problem_attrs => :environment do puts "Updating problems" - Problem.all.each(&:cache_notice_attributes) + Problem.all.each{|problem| + ProblemUpdaterCache.new(self).update + } end desc "Updates Problem#notices_count" diff --git a/spec/fabricators/problem_fabricator.rb b/spec/fabricators/problem_fabricator.rb index 80d8fa0..2536f6d 100644 --- a/spec/fabricators/problem_fabricator.rb +++ b/spec/fabricators/problem_fabricator.rb @@ -10,3 +10,13 @@ Fabricator(:problem_with_comments, :from => :problem) do end } end + +Fabricator(:problem_with_errs, :from => :problem) do + after_create { |parent| + 3.times do + Fabricate(:err, :problem => parent) + end + } +end + + diff --git a/spec/interactors/problem_merge_spec.rb b/spec/interactors/problem_merge_spec.rb new file mode 100644 index 0000000..b9afefb --- /dev/null +++ b/spec/interactors/problem_merge_spec.rb @@ -0,0 +1,48 @@ +require 'spec_helper' + +describe ProblemMerge do + let(:problem) { Fabricate(:problem_with_errs) } + let(:problem_1) { Fabricate(:problem_with_errs) } + + describe "#initialize" do + it 'failed if less than 2 uniq problem pass in args' do + expect { + ProblemMerge.new(problem) + }.to raise_error(ArgumentError) + end + + it 'extract first problem like merged_problem' do + problem_merge = ProblemMerge.new(problem, problem, problem_1) + expect(problem_merge.merged_problem).to eql problem + end + it 'extract other problem like child_problems' do + problem_merge = ProblemMerge.new(problem, problem, problem_1) + expect(problem_merge.child_problems).to eql [problem_1] + end + end + + describe "#merge" do + let!(:problem_merge) { + ProblemMerge.new(problem, problem_1) + } + let(:first_errs) { problem.errs } + let(:merged_errs) { problem_1.errs } + let!(:notice) { Fabricate(:notice, :err => first_errs.first) } + let!(:notice_1) { Fabricate(:notice, :err => merged_errs.first) } + it 'delete one of problem' do + expect { + problem_merge.merge + }.to change(Problem, :count).by(-1) + end + + it 'move all err in one problem' do + problem_merge.merge + problem.reload.errs.should eq (first_errs | merged_errs) + end + + it 'update problem cache' do + ProblemUpdaterCache.should_receive(:new).with(problem).and_return(mock(:update => true)) + problem_merge.merge + end + end +end diff --git a/spec/interactors/problem_updater_cache_spec.rb b/spec/interactors/problem_updater_cache_spec.rb new file mode 100644 index 0000000..ff6f743 --- /dev/null +++ b/spec/interactors/problem_updater_cache_spec.rb @@ -0,0 +1,153 @@ +require 'spec_helper' + +describe ProblemUpdaterCache do + let(:problem) { Fabricate(:problem_with_errs) } + let(:first_errs) { problem.errs } + let!(:notice) { Fabricate(:notice, :err => first_errs.first) } + + describe "#update" do + context "without notice pass args" do + before do + problem.update_attribute(:notices_count, 0) + end + + it 'update the notice_count' do + expect { + ProblemUpdaterCache.new(problem).update + }.to change{ + problem.notices_count + }.from(0).to(1) + end + + context "with only one notice" do + before do + problem.update_attributes!(:messages => {}) + ProblemUpdaterCache.new(problem).update + end + + it 'update information about this notice' do + expect(problem.message).to eq notice.message + expect(problem.environment).to eq notice.environment_name + expect(problem.error_class).to eq notice.error_class + expect(problem.where).to eq notice.where + end + + it 'update first_notice_at' do + expect(problem.first_notice_at).to eq notice.created_at + end + + it 'update last_notice_at' do + expect(problem.last_notice_at).to eq notice.created_at + end + + it 'update stats messages' do + expect(problem.messages).to eq({ + Digest::MD5.hexdigest(notice.message) => {'value' => notice.message, 'count' => 1} + }) + end + + it 'update stats hosts' do + expect(problem.hosts).to eq({ + Digest::MD5.hexdigest(notice.host) => {'value' => notice.host, 'count' => 1} + }) + end + + it 'update stats user_agents' do + expect(problem.user_agents).to eq({ + Digest::MD5.hexdigest(notice.user_agent_string) => {'value' => notice.user_agent_string, 'count' => 1} + }) + end + end + + context "with several notices" do + let!(:notice_2) { Fabricate(:notice, :err => first_errs.first) } + let!(:notice_3) { Fabricate(:notice, :err => first_errs.first) } + before do + problem.update_attributes!(:messages => {}) + ProblemUpdaterCache.new(problem).update + end + it 'update information about this notice' do + expect(problem.message).to eq notice.message + expect(problem.environment).to eq notice.environment_name + expect(problem.error_class).to eq notice.error_class + expect(problem.where).to eq notice.where + end + + it 'update first_notice_at' do + expect(problem.first_notice_at.to_time).to eq notice.created_at.to_time + end + + it 'update last_notice_at' do + expect(problem.last_notice_at.to_i).to be_within(1).of(notice.created_at.to_i) + end + + it 'update stats messages' do + expect(problem.messages).to eq({ + Digest::MD5.hexdigest(notice.message) => {'value' => notice.message, 'count' => 3} + }) + end + + it 'update stats hosts' do + expect(problem.hosts).to eq({ + Digest::MD5.hexdigest(notice.host) => {'value' => notice.host, 'count' => 3} + }) + end + + it 'update stats user_agents' do + expect(problem.user_agents).to eq({ + Digest::MD5.hexdigest(notice.user_agent_string) => {'value' => notice.user_agent_string, 'count' => 3} + }) + end + + end + end + + context "with notice pass in args" do + + before do + ProblemUpdaterCache.new(problem, notice).update + end + + it 'increase notices_count by 1' do + expect { + ProblemUpdaterCache.new(problem, notice).update + }.to change{ + problem.notices_count + }.by(1) + end + + it 'update information about this notice' do + expect(problem.message).to eq notice.message + expect(problem.environment).to eq notice.environment_name + expect(problem.error_class).to eq notice.error_class + expect(problem.where).to eq notice.where + end + + it 'update first_notice_at' do + expect(problem.first_notice_at).to eq notice.created_at + end + + it 'update last_notice_at' do + expect(problem.last_notice_at).to eq notice.created_at + end + + it 'inc stats messages' do + expect(problem.messages).to eq({ + Digest::MD5.hexdigest(notice.message) => {'value' => notice.message, 'count' => 2} + }) + end + + it 'inc stats hosts' do + expect(problem.hosts).to eq({ + Digest::MD5.hexdigest(notice.host) => {'value' => notice.host, 'count' => 2} + }) + end + + it 'inc stats user_agents' do + expect(problem.user_agents).to eq({ + Digest::MD5.hexdigest(notice.user_agent_string) => {'value' => notice.user_agent_string, 'count' => 2} + }) + end + end + end +end diff --git a/spec/models/backtrace_line.rb b/spec/models/backtrace_line.rb deleted file mode 100644 index 43f4d36..0000000 --- a/spec/models/backtrace_line.rb +++ /dev/null @@ -1,12 +0,0 @@ -require 'spec_helper' - -describe BacktraceLine do - subject { described_class.new(raw_line) } - - describe "root at the start of decorated filename" do - let(:raw_line) { { 'number' => rand(999), 'file' => '[PROJECT_ROOT]/app/controllers/pages_controller.rb', 'method' => ActiveSupport.methods.shuffle.first.to_s } } - it "should leave leading root symbol in filepath" do - subject.decorated_path.should == '/app/controllers/' - end - end -end diff --git a/spec/models/backtrace_line_spec.rb b/spec/models/backtrace_line_spec.rb new file mode 100644 index 0000000..9a3a043 --- /dev/null +++ b/spec/models/backtrace_line_spec.rb @@ -0,0 +1,12 @@ +require 'spec_helper' + +describe BacktraceLine do + subject { described_class.new(raw_line) } + + describe "root at the start of decorated filename" do + let(:raw_line) { { 'number' => rand(999), 'file' => '[PROJECT_ROOT]/app/controllers/pages_controller.rb', 'method' => ActiveSupport.methods.shuffle.first.to_s } } + it "should leave leading root symbol in filepath" do + subject.decorated_path.should == 'app/controllers/' + end + end +end diff --git a/spec/models/problem_spec.rb b/spec/models/problem_spec.rb index 7ce9777..a39a64f 100644 --- a/spec/models/problem_spec.rb +++ b/spec/models/problem_spec.rb @@ -23,6 +23,17 @@ describe Problem do end.should change(Comment, :count).by(3) end end + + context "Fabricate(:problem_with_errs)" do + it 'should be valid' do + Fabricate.build(:problem_with_errs).should be_valid + end + it 'should have 3 errs' do + lambda do + Fabricate(:problem_with_errs) + end.should change(Err, :count).by(3) + end + end end context '#last_notice_at' do @@ -49,7 +60,7 @@ describe Problem do problem.first_notice_at.should == notice1.created_at notice2 = Fabricate(:notice, :err => err) - problem.first_notice_at.should == notice1.created_at + problem.first_notice_at.to_time.should eq notice1.created_at end end @@ -134,22 +145,6 @@ describe Problem do end end - - context ".merge!" do - it "collects the Errs from several problems into one and deletes the other problems" do - problem1 = Fabricate(:err).problem - problem2 = Fabricate(:err).problem - problem1.errs.length.should == 1 - problem2.errs.length.should == 1 - - lambda { - merged_problem = Problem.merge!(problem1, problem2) - merged_problem.reload.errs.length.should == 2 - }.should change(Problem, :count).by(-1) - end - end - - context "#unmerge!" do it "creates a separate problem for each err" do problem1 = Fabricate(:notice).problem @@ -188,17 +183,17 @@ describe Problem do context "searching" do it 'finds the correct record' do - find = Fabricate(:problem, :resolved => false, :error_class => 'theErrorclass::other', + find = Fabricate(:problem, :resolved => false, :error_class => 'theErrorclass::other', :message => "other", :where => 'errorclass', :environment => 'development', :app_name => 'other') - dont_find = Fabricate(:problem, :resolved => false, :error_class => "Batman", + dont_find = Fabricate(:problem, :resolved => false, :error_class => "Batman", :message => 'todo', :where => 'classerror', :environment => 'development', :app_name => 'other') Problem.search("theErrorClass").unresolved.should include(find) Problem.search("theErrorClass").unresolved.should_not include(dont_find) end end end - - + + context "notice counter cache" do before do @app = Fabricate(:app) @@ -213,15 +208,15 @@ describe Problem do it "adding a notice increases #notices_count by 1" do lambda { Fabricate(:notice, :err => @err, :message => 'ERR 1') - }.should change(@problem, :notices_count).from(0).to(1) + }.should change(@problem.reload, :notices_count).from(0).to(1) end it "removing a notice decreases #notices_count by 1" do notice1 = Fabricate(:notice, :err => @err, :message => 'ERR 1') - lambda { + expect { @err.notices.first.destroy @problem.reload - }.should change(@problem, :notices_count).from(1).to(0) + }.to change(@problem, :notices_count).from(1).to(0) end end -- libgit2 0.21.2