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 mailer errors in CE #4846

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Log mailer errors in CE #4846

wants to merge 3 commits into from

Conversation

ruslandoga
Copy link
Contributor

@ruslandoga ruslandoga commented Nov 21, 2024

Changes

This PR improves Plausible.Mailer error messages in CE.
It tries to keep changes to a minimum and only changes how exceptions are logged.

Similar to #4657

Requested in #4844 (comment)

Tests

  • This PR does not require tests

Changelog

  • Entry has been added to changelog

Documentation

  • This change does not need a documentation update

Dark mode

  • This PR does not change the UI

@@ -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

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

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

@ruslandoga ruslandoga marked this pull request as ready for review November 21, 2024 09:25
@ruslandoga ruslandoga requested a review from a team November 21, 2024 09:26
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.

1 participant