Refactor: extract Fingerprint#generate from ErrorReport #253

Merged
merged 1 commit into from Jun 21, 2013

Projects

None yet

5 participants

@boblail
Contributor
boblail commented Sep 20, 2012

This commit is just a refactor to get the fingerprinting concern by itself; but it paves the way for testing the fingerprinting algorithm, evaluating substitute algorithms, and even supporting different algorithms in different scenarios.

@nashby
Member
nashby commented Sep 21, 2012

As for me it doesn't look like a lib code and I think it should be placed in app dir somewhere.

@martinciu martinciu commented on an outdated diff Sep 21, 2012
lib/fingerprint.rb
+ def self.generate(notice, api_key)
+ normalized_backtrace = notice.read_attribute(:backtrace)[0...3].map do |trace|
+ trace.merge 'method' => trace['method'].gsub(/[0-9_]{10,}+/, "__FRAGMENT__")
+ end
+
+ fingerprint_source = {
+ :backtrace => normalized_backtrace.to_s,
+ :error_class => notice.error_class,
+ :component => notice.component || 'unknown',
+ :action => notice.action,
+ :environment => notice.server_environment['environment-name'] || 'development',
+ :api_key => api_key
+ }
+
+ Digest::SHA1.hexdigest(fingerprint_source.to_s)
+ end
@martinciu
martinciu Sep 21, 2012 Member

I don't like long class methods especially when it is the only method in a class. I think they are not very OO. I would instantiate this class and divide generate to smaller methods.

Also I wonder if fingerprint without api_key would be good enough. What do you think guys?

@martinciu
Member

@nashby +1 I think it should be in app/models

@boblail
Contributor
boblail commented Sep 25, 2012

Agreed!

I moved fingerprint.rb to app/models and broke generate apart. Good points!

FWIW, I think fingerprint would be better off not including api_key. I'm not sure how likely this scenario is: but suppose a user regenerates the API key for a project. Errbit would start creating all new problems for any repeat notices.

@nashby nashby commented on an outdated diff Sep 30, 2012
spec/models/fingerprint_spec.rb
@@ -0,0 +1,23 @@
+require 'spec_helper'
+
+describe Fingerprint do
+
+ context '#generate' do
+ before do
+ @backtrace = [
@nashby
nashby Sep 30, 2012 Member

please use let instead of before and instance variable.

@nashby nashby commented on an outdated diff Sep 30, 2012
app/models/fingerprint.rb
@@ -0,0 +1,38 @@
+require 'digest/sha1'
+
+class Fingerprint
+
+ def self.generate(notice, api_key)
+ new(notice, api_key).to_s
+ end
+
+ def initialize(notice, api_key)
+ @notice, @api_key = notice, api_key
+ end
+
+ attr_reader :notice, :api_key
+
+
@nashby
nashby Sep 30, 2012 Member

could you remove these extra new lines?

@nashby
Member
nashby commented Oct 29, 2012

@boblail hey, could you please rebase this branch against master? Thanks!

@boblail
Contributor
boblail commented Nov 9, 2012

@nashby Rebased!

@shingara
Member
shingara commented May 9, 2013

I agree with this kind of refactor. This pull request can't be merge. Can you update it to be merged ?

@coveralls

Coverage Status

Changes Unknown when pulling 0d890b1 on concordia-publishing-house:extract-fingerprint into * on errbit:master*.

@shingara shingara merged commit eb199be into errbit:master Jun 21, 2013

1 check passed

default The Travis CI build passed
Details
@shingara
Member

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment