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.

Sign up for GitHub

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

Jump to bottom

Log mailer errors in CE #4846

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
+13 −0
Open

Log mailer errors in CE #4846

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3795eb2
log mailer errors in ce
ruslandoga Nov 21, 2024
5b2e256
changelog
ruslandoga Nov 21, 2024
71e33bb
Merge branch 'master' into log-mailer-errors-in-ce
ruslandoga Nov 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@ All notable changes to this project will be documented in this file.

### Removed
### Changed

- Provide more inforamtion in error logs for failed emails plausible/analytics#4846

### Fixed

- Fix returning filter suggestions for multiple custom property values in the dashboard Filter modal
Expand Down
10 changes: 10 additions & 0 deletions lib/plausible/mailer.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,16 @@ defmodule Plausible.Mailer do
end
end

defp handle_error(error) when is_exception(error) do
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mua errors are Exception.ts so they would be handled by this clause.

# this message is ignored by Sentry, but it's useful in CE
Logger.error("Failed to send e-mail:\n\n " <> Exception.format(:error, error),
# Sentry report is built entirely from crash_reason
crash_reason: {error, _stacktrace = []}
Copy link
Contributor Author

@ruslandoga ruslandoga Nov 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can add :message or :fingerprint option like "Failed to send e-mail" to make it appear the same way as before in Sentry if that's needed.

I don't think this clause would be reached in EE under normal circumstances. The first handle_error (that handles API response) would be the one that matches since all Bamboo.Postmark errors are Bamboo.ApiErrors with a :response field.

Hm... there is no :response field in https://github.com/beam-community/bamboo/blob/main/lib/bamboo/api_error.ex so the first handle_error won't ever match.

Context for the adapter used in EE (AFAIK): https://github.com/plausible/bamboo_postmark/blob/ed55d273dac6c9c121e86f04823acaa9be33114f/lib/bamboo/postmark_adapter.ex#L25-L40 -- it calls Bamboo.ApiError.build_api_error which returns %Bamboo.ApiError{message: "not json"}

Copy link
Contributor Author

@ruslandoga ruslandoga Nov 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was #2361 verified to work at the time? It doesn't seem like %Bamboo.ApiError{} ever contained :response field: https://github.com/beam-community/bamboo/commits/82ff20890821f365a1ea9501acc0e0d79288d290/lib/bamboo/api_error.ex

)

{:error, :unknown_error}
end

defp handle_error(error) do
Logger.error("Failed to send e-mail", sentry: %{extra: %{response: error}})
{:error, :unknown_error}
Expand Down
Loading