Download as
Browse Code »

Commit 8839611e583fa823e25f8660e8ae23d365e76518

Authored by Cyril Mougel
2 parents aa777b6f 2e283c5d
Exists in master and in 1 other branch production

Merge pull request #482 from shingara/fix/issue_447 

Refactor Problem.merge!
Showing 11 changed files with 365 additions and 84 deletions   Show diff stats
app/interactors/problem_merge.rb 0 → 100644
  Diff comments   View file @8839611
... ... @@ -0,0 +1,26 @@
  1 +require 'problem_destroy'
  2 +
  3 +class ProblemMerge
  4 + def initialize(*problems)
  5 + problems = problems.flatten.uniq
  6 + @merged_problem = problems[0]
  7 + @child_problems = problems[1..-1]
  8 + raise ArgumentError.new("need almost 2 uniq different problems") if @child_problems.empty?
  9 + end
  10 + attr_reader :merged_problem, :child_problems
  11 +
  12 + def merge
  13 + child_problems.each do |problem|
  14 + merged_problem.errs.concat Err.where(:problem_id => problem.id)
  15 + ProblemDestroy.execute(problem)
  16 + end
  17 + reset_cached_attributes
  18 + merged_problem
  19 + end
  20 +
  21 + private
  22 +
  23 + def reset_cached_attributes
  24 + ProblemUpdaterCache.new(merged_problem).update
  25 + end
  26 +end
... ...
app/interactors/problem_updater_cache.rb 0 → 100644
  Diff comments   View file @8839611
... ... @@ -0,0 +1,90 @@
  1 +class ProblemUpdaterCache
  2 + def initialize(problem, notice=nil)
  3 + @problem = problem
  4 + @notice = notice
  5 + end
  6 + attr_reader :problem
  7 +
  8 + ##
  9 + # Update cache information about child associate to this problem
  10 + #
  11 + # update the notices count, and some notice informations
  12 + #
  13 + # @return [ Problem ] the problem with this update
  14 + #
  15 + def update
  16 + update_notices_count
  17 + update_notices_cache
  18 + problem
  19 + end
  20 +
  21 + private
  22 +
  23 + def update_notices_count
  24 + if @notice
  25 + problem.inc(:notices_count, 1)
  26 + else
  27 + problem.update_attribute(
  28 + :notices_count, problem.notices.count
  29 + )
  30 + end
  31 + end
  32 +
  33 + ##
  34 + # Update problem statistique from some notice information
  35 + #
  36 + def update_notices_cache
  37 + first_notice = notices.first
  38 + last_notice = notices.last
  39 + notice ||= @notice || first_notice
  40 +
  41 + attrs = {}
  42 + attrs[:first_notice_at] = first_notice.created_at if first_notice
  43 + attrs[:last_notice_at] = last_notice.created_at if last_notice
  44 + attrs.merge!(
  45 + :message => notice.message,
  46 + :environment => notice.environment_name,
  47 + :error_class => notice.error_class,
  48 + :where => notice.where,
  49 + :messages => attribute_count(:message, messages),
  50 + :hosts => attribute_count(:host, hosts),
  51 + :user_agents => attribute_count(:user_agent_string, user_agents)
  52 + ) if notice
  53 + problem.update_attributes!(attrs)
  54 + end
  55 +
  56 + def notices
  57 + @notices ||= @notice ? [@notice].sort(&:created_at) : problem.notices.order_by([:created_at, :asc])
  58 + end
  59 +
  60 + def messages
  61 + @notice ? problem.messages : {}
  62 + end
  63 +
  64 + def hosts
  65 + @notice ? problem.hosts : {}
  66 + end
  67 +
  68 + def user_agents
  69 + @notice ? problem.user_agents : {}
  70 + end
  71 +
  72 + private
  73 +
  74 + def attribute_count(value, init)
  75 + init.tap do |counts|
  76 + notices.each do |notice|
  77 + counts[attribute_index(notice.send(value))] ||= {
  78 + 'value' => notice.send(value),
  79 + 'count' => 0
  80 + }
  81 + counts[attribute_index(notice.send(value))]['count'] += 1
  82 + end
  83 + end
  84 + end
  85 +
  86 + def attribute_index(value)
  87 + @attributes_index ||= {}
  88 + @attributes_index[value.to_s] ||= Digest::MD5.hexdigest(value.to_s)
  89 + end
  90 +end
... ...
app/models/notice.rb
  Diff comments   View file @8839611
... ... @@ -26,7 +26,7 @@ class Notice
26 26 ]
27 27 )
28 28  
29   - after_create :increase_counter_cache, :cache_attributes_on_problem, :unresolve_problem
  29 + after_create :cache_attributes_on_problem, :unresolve_problem
30 30 before_save :sanitize
31 31 before_destroy :decrease_counter_cache, :remove_cached_attributes_from_problem
32 32  
... ... @@ -116,10 +116,6 @@ class Notice
116 116  
117 117 protected
118 118  
119   - def increase_counter_cache
120   - problem.inc(:notices_count, 1)
121   - end
122   -
123 119 def decrease_counter_cache
124 120 problem.inc(:notices_count, -1) if err
125 121 end
... ... @@ -133,7 +129,7 @@ class Notice
133 129 end
134 130  
135 131 def cache_attributes_on_problem
136   - problem.cache_notice_attributes(self)
  132 + ProblemUpdaterCache.new(problem, self).update
137 133 end
138 134  
139 135 def sanitize
... ...
app/models/problem.rb
  Diff comments   View file @8839611
... ... @@ -83,15 +83,7 @@ class Problem
83 83  
84 84  
85 85 def self.merge!(*problems)
86   - problems = problems.flatten.uniq
87   - merged_problem = problems.shift
88   - problems.each do |problem|
89   - merged_problem.errs.concat Err.where(:problem_id => problem.id)
90   - problem.errs(true) # reload problem.errs (should be empty) before problem.destroy
91   - problem.destroy
92   - end
93   - merged_problem.reset_cached_attributes
94   - merged_problem
  86 + ProblemMerge.new(problems).merge
95 87 end
96 88  
97 89 def merged?
... ... @@ -128,9 +120,7 @@ class Problem
128 120  
129 121  
130 122 def reset_cached_attributes
131   - update_attribute(:notices_count, notices.count)
132   - cache_app_attributes
133   - cache_notice_attributes
  123 + ProblemUpdaterCache.new(self).update
134 124 end
135 125  
136 126 def cache_app_attributes
... ... @@ -145,26 +135,6 @@ class Problem
145 135 end
146 136 end
147 137  
148   - def cache_notice_attributes(notice=nil)
149   - first_notice = notices.order_by([:created_at, :asc]).first
150   - last_notice = notices.order_by([:created_at, :asc]).last
151   - notice ||= first_notice
152   -
153   - attrs = {}
154   - attrs[:first_notice_at] = first_notice.created_at if first_notice
155   - attrs[:last_notice_at] = last_notice.created_at if last_notice
156   - attrs.merge!(
157   - :message => notice.message,
158   - :environment => notice.environment_name,
159   - :error_class => notice.error_class,
160   - :where => notice.where,
161   - :messages => attribute_count_increase(:messages, notice.message),
162   - :hosts => attribute_count_increase(:hosts, notice.host),
163   - :user_agents => attribute_count_increase(:user_agents, notice.user_agent_string)
164   - ) if notice
165   - update_attributes!(attrs)
166   - end
167   -
168 138 def remove_cached_notice_attributes(notice)
169 139 update_attributes!(
170 140 :messages => attribute_count_descrease(:messages, notice.message),
... ... @@ -184,15 +154,6 @@ class Problem
184 154 end
185 155  
186 156 private
187   - def attribute_count_increase(name, value)
188   - counter, index = send(name), attribute_index(value)
189   - if counter[index].nil?
190   - counter[index] = {'value' => value, 'count' => 1}
191   - else
192   - counter[index]['count'] += 1
193   - end
194   - counter
195   - end
196 157  
197 158 def attribute_count_descrease(name, value)
198 159 counter, index = send(name), attribute_index(value)
... ...
lib/tasks/errbit/database.rake
  Diff comments   View file @8839611
... ... @@ -6,7 +6,9 @@ namespace :errbit do
6 6 desc "Updates cached attributes on Problem"
7 7 task :update_problem_attrs => :environment do
8 8 puts "Updating problems"
9   - Problem.all.each(&:cache_notice_attributes)
  9 + Problem.all.each{|problem|
  10 + ProblemUpdaterCache.new(self).update
  11 + }
10 12 end
11 13  
12 14 desc "Updates Problem#notices_count"
... ...
spec/fabricators/problem_fabricator.rb
  Diff comments   View file @8839611
... ... @@ -10,3 +10,13 @@ Fabricator(:problem_with_comments, :from => :problem) do
10 10 end
11 11 }
12 12 end
  13 +
  14 +Fabricator(:problem_with_errs, :from => :problem) do
  15 + after_create { |parent|
  16 + 3.times do
  17 + Fabricate(:err, :problem => parent)
  18 + end
  19 + }
  20 +end
  21 +
  22 +
... ...
spec/interactors/problem_merge_spec.rb 0 → 100644
  Diff comments   View file @8839611
... ... @@ -0,0 +1,48 @@
  1 +require 'spec_helper'
  2 +
  3 +describe ProblemMerge do
  4 + let(:problem) { Fabricate(:problem_with_errs) }
  5 + let(:problem_1) { Fabricate(:problem_with_errs) }
  6 +
  7 + describe "#initialize" do
  8 + it 'failed if less than 2 uniq problem pass in args' do
  9 + expect {
  10 + ProblemMerge.new(problem)
  11 + }.to raise_error(ArgumentError)
  12 + end
  13 +
  14 + it 'extract first problem like merged_problem' do
  15 + problem_merge = ProblemMerge.new(problem, problem, problem_1)
  16 + expect(problem_merge.merged_problem).to eql problem
  17 + end
  18 + it 'extract other problem like child_problems' do
  19 + problem_merge = ProblemMerge.new(problem, problem, problem_1)
  20 + expect(problem_merge.child_problems).to eql [problem_1]
  21 + end
  22 + end
  23 +
  24 + describe "#merge" do
  25 + let!(:problem_merge) {
  26 + ProblemMerge.new(problem, problem_1)
  27 + }
  28 + let(:first_errs) { problem.errs }
  29 + let(:merged_errs) { problem_1.errs }
  30 + let!(:notice) { Fabricate(:notice, :err => first_errs.first) }
  31 + let!(:notice_1) { Fabricate(:notice, :err => merged_errs.first) }
  32 + it 'delete one of problem' do
  33 + expect {
  34 + problem_merge.merge
  35 + }.to change(Problem, :count).by(-1)
  36 + end
  37 +
  38 + it 'move all err in one problem' do
  39 + problem_merge.merge
  40 + problem.reload.errs.should eq (first_errs | merged_errs)
  41 + end
  42 +
  43 + it 'update problem cache' do
  44 + ProblemUpdaterCache.should_receive(:new).with(problem).and_return(mock(:update => true))
  45 + problem_merge.merge
  46 + end
  47 + end
  48 +end
... ...
spec/interactors/problem_updater_cache_spec.rb 0 → 100644
  Diff comments   View file @8839611
... ... @@ -0,0 +1,153 @@
  1 +require 'spec_helper'
  2 +
  3 +describe ProblemUpdaterCache do
  4 + let(:problem) { Fabricate(:problem_with_errs) }
  5 + let(:first_errs) { problem.errs }
  6 + let!(:notice) { Fabricate(:notice, :err => first_errs.first) }
  7 +
  8 + describe "#update" do
  9 + context "without notice pass args" do
  10 + before do
  11 + problem.update_attribute(:notices_count, 0)
  12 + end
  13 +
  14 + it 'update the notice_count' do
  15 + expect {
  16 + ProblemUpdaterCache.new(problem).update
  17 + }.to change{
  18 + problem.notices_count
  19 + }.from(0).to(1)
  20 + end
  21 +
  22 + context "with only one notice" do
  23 + before do
  24 + problem.update_attributes!(:messages => {})
  25 + ProblemUpdaterCache.new(problem).update
  26 + end
  27 +
  28 + it 'update information about this notice' do
  29 + expect(problem.message).to eq notice.message
  30 + expect(problem.environment).to eq notice.environment_name
  31 + expect(problem.error_class).to eq notice.error_class
  32 + expect(problem.where).to eq notice.where
  33 + end
  34 +
  35 + it 'update first_notice_at' do
  36 + expect(problem.first_notice_at).to eq notice.created_at
  37 + end
  38 +
  39 + it 'update last_notice_at' do
  40 + expect(problem.last_notice_at).to eq notice.created_at
  41 + end
  42 +
  43 + it 'update stats messages' do
  44 + expect(problem.messages).to eq({
  45 + Digest::MD5.hexdigest(notice.message) => {'value' => notice.message, 'count' => 1}
  46 + })
  47 + end
  48 +
  49 + it 'update stats hosts' do
  50 + expect(problem.hosts).to eq({
  51 + Digest::MD5.hexdigest(notice.host) => {'value' => notice.host, 'count' => 1}
  52 + })
  53 + end
  54 +
  55 + it 'update stats user_agents' do
  56 + expect(problem.user_agents).to eq({
  57 + Digest::MD5.hexdigest(notice.user_agent_string) => {'value' => notice.user_agent_string, 'count' => 1}
  58 + })
  59 + end
  60 + end
  61 +
  62 + context "with several notices" do
  63 + let!(:notice_2) { Fabricate(:notice, :err => first_errs.first) }
  64 + let!(:notice_3) { Fabricate(:notice, :err => first_errs.first) }
  65 + before do
  66 + problem.update_attributes!(:messages => {})
  67 + ProblemUpdaterCache.new(problem).update
  68 + end
  69 + it 'update information about this notice' do
  70 + expect(problem.message).to eq notice.message
  71 + expect(problem.environment).to eq notice.environment_name
  72 + expect(problem.error_class).to eq notice.error_class
  73 + expect(problem.where).to eq notice.where
  74 + end
  75 +
  76 + it 'update first_notice_at' do
  77 + expect(problem.first_notice_at.to_time).to eq notice.created_at.to_time
  78 + end
  79 +
  80 + it 'update last_notice_at' do
  81 + expect(problem.last_notice_at.to_i).to be_within(1).of(notice.created_at.to_i)
  82 + end
  83 +
  84 + it 'update stats messages' do
  85 + expect(problem.messages).to eq({
  86 + Digest::MD5.hexdigest(notice.message) => {'value' => notice.message, 'count' => 3}
  87 + })
  88 + end
  89 +
  90 + it 'update stats hosts' do
  91 + expect(problem.hosts).to eq({
  92 + Digest::MD5.hexdigest(notice.host) => {'value' => notice.host, 'count' => 3}
  93 + })
  94 + end
  95 +
  96 + it 'update stats user_agents' do
  97 + expect(problem.user_agents).to eq({
  98 + Digest::MD5.hexdigest(notice.user_agent_string) => {'value' => notice.user_agent_string, 'count' => 3}
  99 + })
  100 + end
  101 +
  102 + end
  103 + end
  104 +
  105 + context "with notice pass in args" do
  106 +
  107 + before do
  108 + ProblemUpdaterCache.new(problem, notice).update
  109 + end
  110 +
  111 + it 'increase notices_count by 1' do
  112 + expect {
  113 + ProblemUpdaterCache.new(problem, notice).update
  114 + }.to change{
  115 + problem.notices_count
  116 + }.by(1)
  117 + end
  118 +
  119 + it 'update information about this notice' do
  120 + expect(problem.message).to eq notice.message
  121 + expect(problem.environment).to eq notice.environment_name
  122 + expect(problem.error_class).to eq notice.error_class
  123 + expect(problem.where).to eq notice.where
  124 + end
  125 +
  126 + it 'update first_notice_at' do
  127 + expect(problem.first_notice_at).to eq notice.created_at
  128 + end
  129 +
  130 + it 'update last_notice_at' do
  131 + expect(problem.last_notice_at).to eq notice.created_at
  132 + end
  133 +
  134 + it 'inc stats messages' do
  135 + expect(problem.messages).to eq({
  136 + Digest::MD5.hexdigest(notice.message) => {'value' => notice.message, 'count' => 2}
  137 + })
  138 + end
  139 +
  140 + it 'inc stats hosts' do
  141 + expect(problem.hosts).to eq({
  142 + Digest::MD5.hexdigest(notice.host) => {'value' => notice.host, 'count' => 2}
  143 + })
  144 + end
  145 +
  146 + it 'inc stats user_agents' do
  147 + expect(problem.user_agents).to eq({
  148 + Digest::MD5.hexdigest(notice.user_agent_string) => {'value' => notice.user_agent_string, 'count' => 2}
  149 + })
  150 + end
  151 + end
  152 + end
  153 +end
... ...
spec/models/backtrace_line.rb
View file @aa777b6
... ... @@ -1,12 +0,0 @@
1   -require 'spec_helper'
2   -
3   -describe BacktraceLine do
4   - subject { described_class.new(raw_line) }
5   -
6   - describe "root at the start of decorated filename" do
7   - let(:raw_line) { { 'number' => rand(999), 'file' => '[PROJECT_ROOT]/app/controllers/pages_controller.rb', 'method' => ActiveSupport.methods.shuffle.first.to_s } }
8   - it "should leave leading root symbol in filepath" do
9   - subject.decorated_path.should == '/app/controllers/'
10   - end
11   - end
12   -end
spec/models/backtrace_line_spec.rb 0 → 100644
  Diff comments   View file @8839611
... ... @@ -0,0 +1,12 @@
  1 +require 'spec_helper'
  2 +
  3 +describe BacktraceLine do
  4 + subject { described_class.new(raw_line) }
  5 +
  6 + describe "root at the start of decorated filename" do
  7 + let(:raw_line) { { 'number' => rand(999), 'file' => '[PROJECT_ROOT]/app/controllers/pages_controller.rb', 'method' => ActiveSupport.methods.shuffle.first.to_s } }
  8 + it "should leave leading root symbol in filepath" do
  9 + subject.decorated_path.should == 'app/controllers/'
  10 + end
  11 + end
  12 +end
... ...
spec/models/problem_spec.rb
  Diff comments   View file @8839611
... ... @@ -23,6 +23,17 @@ describe Problem do
23 23 end.should change(Comment, :count).by(3)
24 24 end
25 25 end
  26 +
  27 + context "Fabricate(:problem_with_errs)" do
  28 + it 'should be valid' do
  29 + Fabricate.build(:problem_with_errs).should be_valid
  30 + end
  31 + it 'should have 3 errs' do
  32 + lambda do
  33 + Fabricate(:problem_with_errs)
  34 + end.should change(Err, :count).by(3)
  35 + end
  36 + end
26 37 end
27 38  
28 39 context '#last_notice_at' do
... ... @@ -49,7 +60,7 @@ describe Problem do
49 60 problem.first_notice_at.should == notice1.created_at
50 61  
51 62 notice2 = Fabricate(:notice, :err => err)
52   - problem.first_notice_at.should == notice1.created_at
  63 + problem.first_notice_at.to_time.should eq notice1.created_at
53 64 end
54 65 end
55 66  
... ... @@ -134,22 +145,6 @@ describe Problem do
134 145 end
135 146 end
136 147  
137   -
138   - context ".merge!" do
139   - it "collects the Errs from several problems into one and deletes the other problems" do
140   - problem1 = Fabricate(:err).problem
141   - problem2 = Fabricate(:err).problem
142   - problem1.errs.length.should == 1
143   - problem2.errs.length.should == 1
144   -
145   - lambda {
146   - merged_problem = Problem.merge!(problem1, problem2)
147   - merged_problem.reload.errs.length.should == 2
148   - }.should change(Problem, :count).by(-1)
149   - end
150   - end
151   -
152   -
153 148 context "#unmerge!" do
154 149 it "creates a separate problem for each err" do
155 150 problem1 = Fabricate(:notice).problem
... ... @@ -188,17 +183,17 @@ describe Problem do
188 183  
189 184 context "searching" do
190 185 it 'finds the correct record' do
191   - find = Fabricate(:problem, :resolved => false, :error_class => 'theErrorclass::other',
  186 + find = Fabricate(:problem, :resolved => false, :error_class => 'theErrorclass::other',
192 187 :message => "other", :where => 'errorclass', :environment => 'development', :app_name => 'other')
193   - dont_find = Fabricate(:problem, :resolved => false, :error_class => "Batman",
  188 + dont_find = Fabricate(:problem, :resolved => false, :error_class => "Batman",
194 189 :message => 'todo', :where => 'classerror', :environment => 'development', :app_name => 'other')
195 190 Problem.search("theErrorClass").unresolved.should include(find)
196 191 Problem.search("theErrorClass").unresolved.should_not include(dont_find)
197 192 end
198 193 end
199 194 end
200   -
201   -
  195 +
  196 +
202 197 context "notice counter cache" do
203 198 before do
204 199 @app = Fabricate(:app)
... ... @@ -213,15 +208,15 @@ describe Problem do
213 208 it "adding a notice increases #notices_count by 1" do
214 209 lambda {
215 210 Fabricate(:notice, :err => @err, :message => 'ERR 1')
216   - }.should change(@problem, :notices_count).from(0).to(1)
  211 + }.should change(@problem.reload, :notices_count).from(0).to(1)
217 212 end
218 213  
219 214 it "removing a notice decreases #notices_count by 1" do
220 215 notice1 = Fabricate(:notice, :err => @err, :message => 'ERR 1')
221   - lambda {
  216 + expect {
222 217 @err.notices.first.destroy
223 218 @problem.reload
224   - }.should change(@problem, :notices_count).from(1).to(0)
  219 + }.to change(@problem, :notices_count).from(1).to(0)
225 220 end
226 221 end
227 222  
... ...