Skip to content
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

Log Oban errors (and still report them to Sentry) #4657

Merged
merged 4 commits into from
Oct 8, 2024

Conversation

ruslandoga
Copy link
Contributor

This PR adds logging for Oban errors. The reasoning is that most self-hosters don't have Sentry configured which makes these errors invisible.

Here's how it'd look after this PR:

Screenshot 2024-10-08 at 16 23 14

@ruslandoga ruslandoga added the self-hosting Anything self-hosted label Oct 8, 2024
@ruslandoga ruslandoga requested a review from a team October 8, 2024 09:26
@ruslandoga ruslandoga changed the title Log oban errors in CE Log Oban errors in CE Oct 8, 2024
@macobo
Copy link
Contributor

macobo commented Oct 8, 2024

It's worth adding something to changelog as well!

@aerosol
Copy link
Member

aerosol commented Oct 8, 2024

IMHO it's ok not to do the CE branching. It doesn't hurt to log regardless of the build.

@ruslandoga
Copy link
Contributor Author

I thoughts so too, but decided to go with CE-only since otherwise EE would receive duplicate Sentry reports since error logs get sent there too:

config :logger, Sentry.LoggerBackend,
capture_log_messages: true,
level: :error

@aerosol
Copy link
Member

aerosol commented Oct 8, 2024

Just replace Sentry.capture_exception that's beneath with Logger.error then?

@ruslandoga
Copy link
Contributor Author

ruslandoga commented Oct 8, 2024

The reports would be a bit different (#3336), is that OK? I'll get some screenshot for the differences for these particular errors.

@ruslandoga
Copy link
Contributor Author

Sentry.capture_exception screenshot

from-sentry

Logger.error screenshot

from-logger

The differences I see are:

  • Sentry.capture_message groups / fingerprints by stacktrace instead of error message (this info is at the very bottom of the page)
  • using Sentry.capture_message renders stacktrace in a nice table :)

Also, to make it work, I needed to modify the Logger call to

Logger.error(
  Exception.format(:error, meta.reason, meta.stacktrace),
  sentry: %{extra: extra}
)

which removes the "Background job failed" message and makes the actual log appear like this

16:52:52.129 [error] ** (ArgumentError) The email address `plausible@localhost` is invalid.

    (mail 0.3.1) lib/mail/renderers/rfc_2822.ex:134: Mail.Renderers.RFC2822.validate_address/1
    (mail 0.3.1) lib/mail/renderers/rfc_2822.ex:141: Mail.Renderers.RFC2822.render_address/1
    (mail 0.3.1) lib/mail/renderers/rfc_2822.ex:97: Mail.Renderers.RFC2822.render_header/2
    (elixir 1.17.1) lib/enum.ex:1703: Enum."-map/2-lists^map/1-1-"/2
    (elixir 1.17.1) lib/enum.ex:1703: Enum."-map/2-lists^map/1-1-"/2
    (mail 0.3.1) lib/mail/renderers/rfc_2822.ex:174: Mail.Renderers.RFC2822.render_headers/2
    (mail 0.3.1) lib/mail/renderers/rfc_2822.ex:56: Mail.Renderers.RFC2822.render_part/2
    (bamboo_mua 0.2.2) lib/bamboo_mua.ex:57: Bamboo.Mua.deliver/2
    (bamboo 2.3.0) lib/bamboo/mailer.ex:208: Bamboo.Mailer.deliver_now/4
    (bamboo 2.3.0) lib/bamboo/mailer.ex:231: Bamboo.Mailer.deliver_now!/4
    (plausible 0.0.1) lib/workers/notify_exported_analytics.ex:36: Plausible.Workers.NotifyExportedAnalytics.perform/1
    (oban 2.17.2) lib/oban/queue/executor.ex:129: Oban.Queue.Executor.perform/1
    (oban 2.17.2) lib/oban/queue/executor.ex:74: Oban.Queue.Executor.call/1
    (elixir 1.17.1) lib/task/supervised.ex:101: Task.Supervised.invoke_mfa/2
    (elixir 1.17.1) lib/task/supervised.ex:36: Task.Supervised.reply/4

which is probably less intuitive for self-hosters.

@ruslandoga
Copy link
Contributor Author

ruslandoga commented Oct 8, 2024

Seems like I can make both look the same by passing crash_reason: {meta.reason, meta.stacktrace} to Logger. Then I would also be able to modify the message to be more user-friendly since it won't be used in Sentry.

@ruslandoga ruslandoga requested a review from aerosol October 8, 2024 10:18
@ruslandoga
Copy link
Contributor Author

ruslandoga commented Oct 8, 2024

I've updated the PR and now it logs a "self-hoster-friendly" message but reports a more structured Sentry event, the same way as before.

@ruslandoga ruslandoga removed the self-hosting Anything self-hosted label Oct 8, 2024
@ruslandoga ruslandoga changed the title Log Oban errors in CE Log Oban errors (and report them to Sentry) Oct 8, 2024
@ruslandoga ruslandoga changed the title Log Oban errors (and report them to Sentry) Log Oban errors (and still report them to Sentry) Oct 8, 2024
@ruslandoga ruslandoga enabled auto-merge October 8, 2024 10:25
@ruslandoga ruslandoga added this pull request to the merge queue Oct 8, 2024
Merged via the queue into master with commit f28a1b8 Oct 8, 2024
10 checks passed
@ruslandoga ruslandoga deleted the log-oban-errors-in-ce branch October 8, 2024 10:35
@ruslandoga ruslandoga mentioned this pull request Nov 21, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants