-
Notifications
You must be signed in to change notification settings - Fork 28
Limits Email Sending on Recurring Errors and Configuration Option for Full Backtrace #80
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
4b27794
b11b32f
d466265
bba0eb1
8d9505d
429d2ac
1391b73
6ce78ba
03d6cec
4882100
9d22f4b
d9e7ee8
2f76f69
2214f49
5bc0fdd
b912495
461fe22
8c5415b
ae54100
0b80dd8
1bba415
59db8e1
28f610a
fdf8906
df6b44f
5d0faf3
cf0afbc
40e4603
4d80f4e
6c28ace
b4be15a
870b713
2f634fc
7f8005c
83d4b6c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,3 +9,4 @@ | |
| .DS_Store | ||
| test/dummy/log/*.log | ||
| test/dummy/db/*.sqlite3 | ||
| .idea | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,7 @@ module SolidErrors | |
| class Occurrence < Record | ||
| belongs_to :error, class_name: "SolidErrors::Error" | ||
|
|
||
| after_create_commit :send_email, if: -> { SolidErrors.send_emails? && SolidErrors.email_to.present? } | ||
| after_create :send_email, if: :should_send_email? | ||
| after_create_commit :clear_resolved_errors, if: :should_clear_resolved_errors? | ||
|
|
||
| # The parsed exception backtrace. Lines in this backtrace that are from installed gems | ||
|
|
@@ -45,5 +45,18 @@ def should_clear_resolved_errors? | |
|
|
||
| true | ||
| end | ||
|
|
||
| def should_send_email? | ||
| return false unless SolidErrors.send_emails? && SolidErrors.email_to.present? | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems like a regression, the default is currently we send email whenever an error occured.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Absolutely and that is exactly what I do not want in my app (an email for every time an error occurs). It would send 20 in instances where an error occurred multiple times (like in a job that gets re-run, etc.). This is what I need it to do. So, I will close the PR and continue down my fork. Thanks for the review! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hey, this is not a rejection, I'm not even the maintainer :). I just want to see this feature get attention and ultimately merged into the main repo. In term of regression, this is ultimately not me who decide, this gem is pre 1.0 so i guess regression is fine ? Other option is we can have an option to enable this feature. |
||
|
|
||
| # Check if resolved_at changed from a datetime to nil (resolved error reoccurred) | ||
| resolved_at_changes = error.previous_changes['resolved_at'] | ||
| resolved_error_reoccurred = resolved_at_changes&.first.present? && resolved_at_changes&.last.nil? | ||
|
|
||
| # Check if this is the first occurrence of a brand new error | ||
| first_occurrence = error.occurrences.count == 1 | ||
|
|
||
| resolved_error_reoccurred || first_occurrence | ||
| end | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,11 +7,13 @@ | |
| t.text "severity", null: false | ||
| t.text "source" | ||
| t.datetime "resolved_at" | ||
| t.datetime "prev_resolved_at" | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i dont see this being used? to my understanding, the way we are currently checking it is by checking wether resolved_at previously has value or not. |
||
| t.string "fingerprint", limit: 64, null: false | ||
| t.datetime "created_at", null: false | ||
| t.datetime "updated_at", null: false | ||
| t.index ["fingerprint"], name: "index_solid_errors_on_fingerprint", unique: true | ||
| t.index ["resolved_at"], name: "index_solid_errors_on_resolved_at" | ||
| t.index ["prev_resolved_at"], name: "index_solid_errors_on_prev_resolved_at" | ||
| end | ||
|
|
||
| create_table "solid_errors_occurrences", force: :cascade do |t| | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| module SolidErrors | ||
| VERSION = "0.7.0" | ||
| VERSION = "0.8.0" | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont think we need to change the after_create_commit.