Remove unnecesary retrying on creating events #16954
Conversation
eduardoj
force-pushed
the
refactoring/remove_retrying
branch
from
42e9597 to
1708591
Compare
eduardoj
changed the title
eduardoj
force-pushed
the
refactoring/remove_retrying
branch
from
1708591 to
7344369
Compare
eduardoj
force-pushed
the
refactoring/remove_retrying
branch
from
7344369 to
1d8fb70
Compare
|
After three months of tracking if a retry was triggered (see #16959), no retries were performed. The retrying code can be safely removed. |
| rescue StandardError => e | ||
| Airbrake.notify("Failed to create Event : #{type.inspect}: #{data} #{e}") | ||
| end | ||
| event = Event::Factory.new_from_type(type, data) |
There was a problem hiding this comment.
How would you know which Airbrake.notify you look at in errbit?
There was a problem hiding this comment.
How would you know which
Airbrake.notifyyou look at in errbit?
In errbit we could have received three different messages, starting with:
- 1: "Failed to create Event :"
- 2: "Failed to update PackageIssue :"
- 3: "Failed while running PackageUpdateIfDirtyJob:"
We only have received messages for this one: 2: "Failed to update PackageIssue :".
Now we are confident we potentially won't receive any "Failed to create Event :" messages, we don't need to identify which Airbrake.notify we need to send. That's why we remove this unnecessary code to distinguish between Aribrake.notify options.
There was a problem hiding this comment.
Well that only means there was never any problem that required retrying right?
There was a problem hiding this comment.
There was a problem hiding this comment.
Well that only means there was never any problem that required retrying right?
Exactly. That's why we don't need retrying.
There was a problem hiding this comment.
There are cases of this still in errbit...
https://errbit.opensuse.org/apps/5620ca2bdc71fa00b1000000/problems/670d2a5584bf460c5bef1380 https://errbit.opensuse.org/apps/5620ca2bdc71fa00b1000000/problems/65f2e3fa84bf4608f8817fd6
Both cases happened due to general failures with the database, affecting the whole application. Nothing that retrying in this part of the code could make a difference.
There was a problem hiding this comment.
That may be but how do you know this never saved us? You can't because every exception looks the same. But feel free to ignore me.
There was a problem hiding this comment.
This could have saved us in the past: we had another version/set-up of the database and the codebase was different.
Now, just describing with other words the same written in the commit body, we don't need this code to handle in a different way potential failures happening accessing the database.
Fortunately, we have the backtrace for every exception saved in Errbit. Even if they look the same, we can always know where the exceptions have been triggered.
If saving an event results in an ActiveRecord::StatementInvalid exception, just retrying to save that event 10 times doesn't make a difference. Also, don't rescue from StandardError exceptions to handle them exactly as we do in the rest of the application.
eduardoj
force-pushed
the
refactoring/remove_retrying
branch
from
1d8fb70 to
4158b17
Compare
Also unifies the way database errors are reported. Fixes #16953.
This PR is the first of a group of pull requests: