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
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 2 additions & 3 deletions lib/plausible/cache/adapter.ex
Original file line number Diff line number Diff line change
Expand Up @@ -120,9 +120,8 @@ defmodule Plausible.Cache.Adapter do
{:ok, result}
catch
:exit, {:timeout, _} ->
Sentry.capture_message(
"Timeout while executing with lock on key in '#{inspect(cache_name)}'",
extra: %{key: key}
Logger.error("Timeout while executing with lock on key in '#{inspect(cache_name)}'",
sentry: %{extra: %{key: key}}
)

{:error, :timeout}
Expand Down
5 changes: 4 additions & 1 deletion lib/plausible/google/http.ex
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,10 @@ defmodule Plausible.Google.HTTP do
{:error, error}

{:error, %{reason: _} = e} ->
Sentry.capture_message("Error fetching Google queries", extra: %{error: inspect(e)})
Logger.error("Error fetching Google queries",
sentry: %{extra: %{error: inspect(e)}}
)

{:error, :unknown_error}
end
end
Expand Down
10 changes: 6 additions & 4 deletions lib/plausible/ingestion/counters.ex
Original file line number Diff line number Diff line change
Expand Up @@ -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

"Caught an error when trying to flush ingest counters.",
extra: %{
number_of_records: Enum.count(records),
error: inspect(thrown)
sentry: %{
extra: %{
number_of_records: Enum.count(records),
error: inspect(thrown)
}
}
)
end
Expand Down
6 changes: 4 additions & 2 deletions lib/plausible/ingestion/event.ex
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ defmodule Plausible.Ingestion.Event do
alias Plausible.ClickhouseEventV2
alias Plausible.Site.GateKeeper

require Logger

defstruct domain: nil,
site: nil,
clickhouse_event_attrs: %{},
Expand Down Expand Up @@ -467,8 +469,8 @@ defmodule Plausible.Ingestion.Event do
nil

%Device{type: type} ->
Sentry.capture_message("Could not determine device type from UAInspector",
extra: %{type: type}
Logger.error("Could not determine device type from UAInspector",
sentry: %{extra: %{type: type}}
)

nil
Expand Down
12 changes: 7 additions & 5 deletions lib/plausible/verification/diagnostics.ex
Original file line number Diff line number Diff line change
Expand Up @@ -363,11 +363,13 @@ defmodule Plausible.Verification.Diagnostics do
end

def interpret(diagnostics, url) do
Sentry.capture_message("Unhandled case for site verification",
extra: %{
message: inspect(diagnostics),
url: url,
hash: :erlang.phash2(diagnostics)
Logger.error("Unhandled case for site verification",
sentry: %{
extra: %{
message: inspect(diagnostics),
url: url,
hash: :erlang.phash2(diagnostics)
}
}
)

Expand Down
2 changes: 1 addition & 1 deletion lib/plausible_web/controllers/api/external_controller.ex
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ defmodule PlausibleWeb.Api.ExternalController do
end

def error(conn, _params) do
Sentry.capture_message("JS snippet error")
Logger.error("JS snippet error")
send_resp(conn, 200, "")
end

Expand Down
2 changes: 1 addition & 1 deletion lib/plausible_web/controllers/auth_controller.ex
Original file line number Diff line number Diff line change
Expand Up @@ -476,7 +476,7 @@ defmodule PlausibleWeb.AuthController do
|> redirect(external: redirect_route)

_any ->
Sentry.capture_message("Google OAuth callback failed. Reason: #{inspect(params)}")
Logger.error("Google OAuth callback failed. Reason: #{inspect(params)}")

conn
|> put_flash(
Expand Down
5 changes: 3 additions & 2 deletions lib/plausible_web/live/sites.ex
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ defmodule PlausibleWeb.Live.Sites do

use PlausibleWeb, :live_view
import PlausibleWeb.Live.Components.Pagination
require Logger

alias Plausible.Sites

Expand Down Expand Up @@ -614,8 +615,8 @@ defmodule PlausibleWeb.Live.Sites do

{:noreply, socket}
else
Sentry.capture_message("Attempting to toggle pin for invalid domain.",
extra: %{domain: domain, user: socket.assigns.current_user.id}
Logger.error("Attempting to toggle pin for invalid domain.",
sentry: %{extra: %{domain: domain, user: socket.assigns.current_user.id}}
)

{:noreply, socket}
Expand Down
18 changes: 9 additions & 9 deletions lib/plausible_web/views/email_view.ex
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,15 @@ defmodule PlausibleWeb.EmailView do
search_query = URI.encode_query(%{query: trace_id})
path = "/organizations/sentry/issues/"

if is_binary(dsn) do
dsn
|> URI.parse()
|> Map.replace(:userinfo, nil)
|> Map.replace(:path, path)
|> Map.replace(:query, search_query)
|> URI.to_string()
else
""
case dsn do
{endpoint_uri, _public_key, _secret_key} when is_binary(endpoint_uri) ->
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"))


_ ->
""
end
end
end
12 changes: 7 additions & 5 deletions lib/workers/import_analytics.ex
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,13 @@ defmodule Plausible.Workers.ImportAnalytics do
:ok

{:error, error, error_opts} ->
Sentry.capture_message("Failed to import from #{site_import.source}",
extra: %{
import_id: site_import.id,
site: site_import.site.domain,
error: inspect(error)
Logger.error("Failed to import from #{site_import.source}",
sentry: %{
extra: %{
import_id: site_import.id,
site: site_import.site.domain,
error: inspect(error)
}
}
)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,9 @@ defmodule PlausibleWeb.ErrorReportControllerTest do
end

test "with dsn" do
sample_dsn = "https://[email protected]/1"
sample_dsn =
Sentry.Config.validate!(dsn: "https://[email protected]/1")
|> Keyword.fetch!(:dsn)

assert EmailView.sentry_link("some-trace", sample_dsn) ==
"https://somehost.example.com/organizations/sentry/issues/?query=some-trace"
Expand Down
Loading