Support non-exception error values from Oban jobs #807
Conversation
| Map.take(job, [:args, :attempt, :id, :max_attempts, :meta, :queue, :tags, :worker]), | ||
| integration_meta: %{oban: %{job: job}} | ||
| ) | ||
| if is_struct(exception) do |
There was a problem hiding this comment.
You may want more specifically is_exception here, rather than just is_struct, because I think Exception.message/1 below won't work on just any general struct.
This may not really matter in practice, though, because I think the errors will probably never end up being a struct other than an Exception.
There was a problem hiding this comment.
Probably not but good to use the right tool for the job. Thanks and updated!
| ) | ||
| else | ||
| Sentry.capture_message("Error with %s", interpolation_parameters: [exception]) | ||
| end |
There was a problem hiding this comment.
It would be more clear to have reason instead of exception here since it may not be an exception after all :)
There was a problem hiding this comment.
True! Updated, thanks!
There was a problem hiding this comment.
This is a wildly generic error message we got going on here 😄 I think it'd be really hard to debug as it's just a generic message plus there are no metadata about the job (which you have available).
This needs to be completely restructured to basically build shared options, and then use Sentry.capture_message or Sentry.capture_exception based on whether reason is an exception or not. Also, doesn't :interpolation_parameters require string parameters? If so, do inspect(reason) here.
There was a problem hiding this comment.
Also let's have the message be something like
"Oban job #{job.worker} errored out: %s"Btw the fingerprint option needs to be customized, if it's just reason use inspect(reason) insted of Exception.message/1
| ) | ||
| else | ||
| Sentry.capture_message("Error with %s", interpolation_parameters: [exception]) | ||
| end |
There was a problem hiding this comment.
This is a wildly generic error message we got going on here 😄 I think it'd be really hard to debug as it's just a generic message plus there are no metadata about the job (which you have available).
This needs to be completely restructured to basically build shared options, and then use Sentry.capture_message or Sentry.capture_exception based on whether reason is an exception or not. Also, doesn't :interpolation_parameters require string parameters? If so, do inspect(reason) here.
| ) | ||
| else | ||
| Sentry.capture_message("Error with %s", interpolation_parameters: [exception]) | ||
| end |
There was a problem hiding this comment.
Also let's have the message be something like
"Oban job #{job.worker} errored out: %s"Btw the fingerprint option needs to be customized, if it's just reason use inspect(reason) insted of Exception.message/1
|
@whatyouhide updated with your feedback! Yeah, I assumed that error message was not up to code 😆 . This restructure feels better |
There was a problem hiding this comment.
One more thing to consider here is logging differently based on attempt counter - in one project I used a strategy where only the last attempt would result in an error, all other attempts were logged to sentry as warnings. Probably out of scope here but I figured I should mention it here for future consideration.
There was a problem hiding this comment.
Last comment and we can ship this.
@solnic yeah that's probably something that could be configurable I think.
| (is_exception(reason) && [inspect(reason.__struct__), Exception.message(reason)]) || | ||
| [inspect(reason)] |
There was a problem hiding this comment.
Please use if, this is pretty hard to read and tries to mimic the ? : operator thing that Elixir doesn't have 🙃
| fingerprint: | ||
| [ | ||
| inspect(job.worker) | ||
| ] ++ fingerprint_opts, |
There was a problem hiding this comment.
Nit but saves some space 😉
| fingerprint: | |
| [ | |
| inspect(job.worker) | |
| ] ++ fingerprint_opts, | |
| fingerprint: [inspect(job.worker)] ++ fingerprint_opts, |
| Sentry.capture_exception( | ||
| reason, | ||
| opts | ||
| ) |
There was a problem hiding this comment.
Saves some space.
| Sentry.capture_exception( | |
| reason, | |
| opts | |
| ) | |
| Sentry.capture_exception(reason, opts) |
savhappy
force-pushed
the
sav/oban-errorreporter-crash
branch
from
9a6dccd to
078a802
Compare
whatyouhide
changed the title
-added
capture_messagetohandle_eventinSentry.Integrations.Oban.ErrorReporterto account for non-exception args that are passed-closes #805