Skip to content

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

Merged
ruslandoga merged 4 commits into
masterfrom
log-oban-errors-in-ce
Oct 8, 2024
Merged

Log Oban errors (and still report them to Sentry)#4657
ruslandoga merged 4 commits into
masterfrom
log-oban-errors-in-ce

Conversation

@ruslandoga

@ruslandoga ruslandoga commented Oct 8, 2024

Copy link
Copy Markdown
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

macobo commented Oct 8, 2024

Copy link
Copy Markdown
Contributor

It's worth adding something to changelog as well!

@aerosol

aerosol commented Oct 8, 2024

Copy link
Copy Markdown
Member

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

@ruslandoga

ruslandoga commented Oct 8, 2024

Copy link
Copy Markdown
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:

analytics/config/runtime.exs

Lines 33 to 35 in e7332d9

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

@aerosol

aerosol commented Oct 8, 2024

Copy link
Copy Markdown
Member

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

@ruslandoga

ruslandoga commented Oct 8, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

@ruslandoga

ruslandoga commented Oct 8, 2024

Copy link
Copy Markdown
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

ruslandoga commented Oct 8, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

ruslandoga commented Oct 8, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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
@ruslandoga
ruslandoga deleted the log-oban-errors-in-ce branch October 8, 2024 10:35
@ruslandoga ruslandoga mentioned this pull request Nov 21, 2024
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aerosol aerosol aerosol approved these changes
+1 more reviewer
@macobo macobo macobo approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ruslandoga @macobo @aerosol