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

Replace some Sentry.capture_message with Logger.error for CE #4998

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

Conversation

ruslandoga
Copy link
Contributor

@ruslandoga ruslandoga commented Jan 21, 2025

This PR attempts to make CE errors more visible by replacing some Sentry.capture_message calls with equivalent Logger.error ones. Sentry.capture_message in billing, Paddle, HelpScout and other EE-only contexts are left as-is.


:sentry context (extra, user, tags, etc.) is not logged right now, leaving that for later as I couldn't figure out how to do it in an easy way. The default Logger formatter ignores map values in metadata so the following is no-op:

config :logger, :default_formatter, metadata: [:request_id, :sentry]

And ExJsonLogger logs "couldn't format" errors since it has very limited support for metadata values.

So I see two ways that could be explored:

  • use a single, good JSON log formatter (both in prod CE and EE), which would support :sentry in metadata
  • introduce Plausible.Logger or Plausible.Sentry or Plausible.ErrorLogger which would log different messages based on the version (CE vs EE) with CE rendering the metadata into the log line:
defmodule Plausible.Sentry do
  use Plausible
  require Logger
  
  @doc """
  Example:
  
      Plausible.Sentry.capture_message("something went wrong", extra: %{request: request})

  """
  def capture_message(message, context \\ [])
 
  on_ee do
    def capture_message(message, context) do
      Logger.error(message, sentry: context)
    end
  else
    def capture_message(message, context) do
      process_context = Sentry.Context.get_all()
      context = # ... merge
      Logger.error(message <> format_context(context))
    end
  end
  
  # etc.
end

@ruslandoga ruslandoga added the self-hosting Anything self-hosted label Jan 21, 2025
@ruslandoga ruslandoga changed the title replace Sentry.capture_message with Logger.error where appropriate for CE Replace some Sentry.capture_message with Logger.error for CE Jan 21, 2025
@ruslandoga ruslandoga force-pushed the replace-sentry-with-logger branch from 4c14a9d to 5b226cd Compare January 23, 2025 11:38
@@ -84,11 +84,13 @@ defmodule Plausible.Ingestion.Counters do
{_, _} = AsyncInsertRepo.insert_all(Record, records)
catch
_, thrown ->
Sentry.capture_message(
Logger.error(
Copy link
Contributor Author

@ruslandoga ruslandoga Jan 23, 2025

Choose a reason for hiding this comment

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

This one can possibly be improved with exceptions (CE gets a more descriptive log message, Sentry gets a stacktrace):

try do
  AsyncInsertRepo.insert_all(Record, records)
rescue
  e ->
    msg = Exception.format(:error, e, __STACKTRACE__)
    Logger.error("Caught an error when trying to flush ingest counters.\n\n  " <> msg, crash_reason: {e, __STACKTRACE__}, sentry: %{extra: %{number_of_records: Enum.count(records)}})
end

URI.parse(endpoint_uri)
|> Map.replace(:path, path)
|> Map.replace(:query, search_query)
|> URI.to_string()
Copy link
Contributor Author

@ruslandoga ruslandoga Jan 23, 2025

Choose a reason for hiding this comment

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

This is something I missed in #3843, I can split it into a separate PR.


With the current Sentry client version (10.2.0) and

export SENTRY_DSN=https://[email protected]/6643873

this returns

iex> Sentry.Config.dsn
#==> {"https://o1012425.ingest.sentry.io/api/6643873/envelope/",
#==>  "7f16d5d6ee70465789e082bd09481556", nil}

iex> PlausibleWeb.EmailView.sentry_link("trace_id")
#==> "https://o1012425.ingest.sentry.io/organizations/sentry/issues/?query=trace_id"

Note that the URL still doesn't seem correct. At least for the sentry.io cloud version. It probably works for Plausible's self-hosted Sentry since the ingest and dashboard domains are the same, but the hardcoded org id might still be problematic.


In case its needed for context, here's the original PR: #2617 (at the time Sentry was at 8.0.6 and Sentry.Config.dsn was basically Application.get_env(:sentry, :dsn) || System.get_env("SENTRY_DSN"))

@ruslandoga ruslandoga marked this pull request as ready for review January 23, 2025 11:45
@ruslandoga ruslandoga requested a review from a team January 23, 2025 11:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
self-hosting Anything self-hosted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant