From fb67260c389ed517e144d2c500cd3a6ae322f386 Mon Sep 17 00:00:00 2001 From: Oleksii Sholik Date: Wed, 22 Apr 2026 01:09:54 +0200 Subject: [PATCH 1/2] Accept keyword options in Sentry.add_logger_handler/1 Let callers pass handler config overrides (e.g. :discard_threshold, :sync_threshold) through to Sentry.LoggerHandler without having to update the handler config out-of-band via :logger after the fact. The :id option is still accepted to override the handler id; any other option is merged into the handler's config map over the existing defaults. Co-Authored-By: Claude Opus 4.7 (1M context) --- .changeset/sentry-handler-opts.md | 5 ++ .../lib/electric/telemetry/sentry.ex | 15 +++-- .../test/electric/telemetry/sentry_test.exs | 67 +++++++++++++++++++ 3 files changed, 80 insertions(+), 7 deletions(-) create mode 100644 .changeset/sentry-handler-opts.md create mode 100644 packages/sync-service/test/electric/telemetry/sentry_test.exs diff --git a/.changeset/sentry-handler-opts.md b/.changeset/sentry-handler-opts.md new file mode 100644 index 0000000000..5f950cb378 --- /dev/null +++ b/.changeset/sentry-handler-opts.md @@ -0,0 +1,5 @@ +--- +'@core/sync-service': patch +--- + +`Electric.Telemetry.Sentry.add_logger_handler/1` now accepts a keyword list of options. Any option other than `:id` is merged into the `Sentry.LoggerHandler` config map, letting downstream apps tune handler settings like `:discard_threshold` and `:sync_threshold` without reaching into `:logger` directly. diff --git a/packages/sync-service/lib/electric/telemetry/sentry.ex b/packages/sync-service/lib/electric/telemetry/sentry.ex index ef201b3ca3..51bff5db7c 100644 --- a/packages/sync-service/lib/electric/telemetry/sentry.ex +++ b/packages/sync-service/lib/electric/telemetry/sentry.ex @@ -2,19 +2,20 @@ defmodule Electric.Telemetry.Sentry do use Electric.Telemetry @default_handler_id :electric_sentry_handler + @default_config %{metadata: :all, capture_log_messages: true, level: :error} - @spec add_logger_handler(handler_id :: atom()) :: :ok | {:error, term()} + @spec add_logger_handler(keyword()) :: :ok | {:error, term()} @spec add_logger_handler() :: :ok | {:error, term()} - def add_logger_handler(id \\ @default_handler_id) + def add_logger_handler(opts \\ []) with_telemetry Sentry.LoggerHandler do - def add_logger_handler(id) do - :logger.add_handler(id, Sentry.LoggerHandler, %{ - config: %{metadata: :all, capture_log_messages: true, level: :error} - }) + def add_logger_handler(opts) do + {id, config_overrides} = Keyword.pop(opts, :id, @default_handler_id) + config = Map.merge(@default_config, Map.new(config_overrides)) + :logger.add_handler(id, Sentry.LoggerHandler, %{config: config}) end else - def add_logger_handler(_id), do: :ok + def add_logger_handler(_opts), do: :ok end @spec set_tags_context(keyword()) :: :ok diff --git a/packages/sync-service/test/electric/telemetry/sentry_test.exs b/packages/sync-service/test/electric/telemetry/sentry_test.exs new file mode 100644 index 0000000000..2e1f77a36b --- /dev/null +++ b/packages/sync-service/test/electric/telemetry/sentry_test.exs @@ -0,0 +1,67 @@ +if Electric.telemetry_enabled?() and Code.ensure_loaded?(Sentry.LoggerHandler) do + defmodule Electric.Telemetry.SentryTest do + use ExUnit.Case, async: false + + alias Electric.Telemetry.Sentry, as: ElectricSentry + + # A dedicated handler id per test keeps state isolated and prevents + # interference with any handler that may have been added elsewhere. + setup do + id = :"sentry_test_#{System.unique_integer([:positive])}" + on_exit(fn -> _ = :logger.remove_handler(id) end) + {:ok, handler_id: id} + end + + defp handler_config!(id) do + {:ok, %{config: config}} = :logger.get_handler_config(id) + config + end + + describe "add_logger_handler/1" do + test "installs Sentry.LoggerHandler with default config", %{handler_id: id} do + assert :ok = ElectricSentry.add_logger_handler(id: id) + + {:ok, handler} = :logger.get_handler_config(id) + assert handler.module == Sentry.LoggerHandler + + assert %{metadata: :all, capture_log_messages: true, level: :error} = + handler_config!(id) + end + + test "merges caller-supplied options into the handler config", + %{handler_id: id} do + assert :ok = + ElectricSentry.add_logger_handler( + id: id, + discard_threshold: 2000, + sync_threshold: nil + ) + + assert %{ + metadata: :all, + capture_log_messages: true, + level: :error, + discard_threshold: 2000, + sync_threshold: nil + } = handler_config!(id) + end + + test "caller-supplied options override defaults", %{handler_id: id} do + assert :ok = ElectricSentry.add_logger_handler(id: id, level: :warning) + + assert %{level: :warning} = handler_config!(id) + end + + test "accepts an empty option list and uses the default handler id" do + default_id = :electric_sentry_handler + _ = :logger.remove_handler(default_id) + on_exit(fn -> _ = :logger.remove_handler(default_id) end) + + assert :ok = ElectricSentry.add_logger_handler() + + {:ok, handler} = :logger.get_handler_config(default_id) + assert handler.module == Sentry.LoggerHandler + end + end + end +end From aa072e26b83a1fa894c0ddacb60d031f574b38b8 Mon Sep 17 00:00:00 2001 From: Oleksii Sholik Date: Wed, 22 Apr 2026 01:17:46 +0200 Subject: [PATCH 2/2] Preserve id-only contract; take opts as the 2nd optional arg Revise the API so add_logger_handler/1's original atom-id contract is preserved: id is the first optional positional arg, and a keyword list of handler-config overrides is the second optional arg. Callers passing a bare atom keep working unchanged. Also address review feedback: - Add a @type handler_opts so the opts shape is documented in the spec - Expose default_handler_id/0 so external callers don't have to hard-code :electric_sentry_handler - Rewrite the test's async: false comment to explain the VM-global :logger state (not just handler-id isolation) - Reference default_handler_id/0 from the default-id test instead of duplicating the atom Co-Authored-By: Claude Opus 4.7 (1M context) --- .changeset/sentry-handler-opts.md | 2 +- .../lib/electric/telemetry/sentry.ex | 21 +++++++++++++------ .../test/electric/telemetry/sentry_test.exs | 17 +++++++-------- 3 files changed, 24 insertions(+), 16 deletions(-) diff --git a/.changeset/sentry-handler-opts.md b/.changeset/sentry-handler-opts.md index 5f950cb378..861fbcf960 100644 --- a/.changeset/sentry-handler-opts.md +++ b/.changeset/sentry-handler-opts.md @@ -2,4 +2,4 @@ '@core/sync-service': patch --- -`Electric.Telemetry.Sentry.add_logger_handler/1` now accepts a keyword list of options. Any option other than `:id` is merged into the `Sentry.LoggerHandler` config map, letting downstream apps tune handler settings like `:discard_threshold` and `:sync_threshold` without reaching into `:logger` directly. +`Electric.Telemetry.Sentry.add_logger_handler/1` now accepts an optional second argument — a keyword list whose entries are merged into the `Sentry.LoggerHandler` config map — so downstream apps can tune handler settings like `:discard_threshold` and `:sync_threshold` without reaching into `:logger` after the fact. The existing single-arg `add_logger_handler(id)` form is preserved. diff --git a/packages/sync-service/lib/electric/telemetry/sentry.ex b/packages/sync-service/lib/electric/telemetry/sentry.ex index 51bff5db7c..8578fcdcad 100644 --- a/packages/sync-service/lib/electric/telemetry/sentry.ex +++ b/packages/sync-service/lib/electric/telemetry/sentry.ex @@ -4,18 +4,27 @@ defmodule Electric.Telemetry.Sentry do @default_handler_id :electric_sentry_handler @default_config %{metadata: :all, capture_log_messages: true, level: :error} - @spec add_logger_handler(keyword()) :: :ok | {:error, term()} + @typedoc """ + Extra entries for the `Sentry.LoggerHandler` config map (e.g. + `:discard_threshold`, `:sync_threshold`). Merged over the defaults at + install time. + """ + @type handler_opts :: [{atom(), term()}] + + def default_handler_id, do: @default_handler_id + + @spec add_logger_handler(atom(), handler_opts()) :: :ok | {:error, term()} + @spec add_logger_handler(atom()) :: :ok | {:error, term()} @spec add_logger_handler() :: :ok | {:error, term()} - def add_logger_handler(opts \\ []) + def add_logger_handler(id \\ @default_handler_id, opts \\ []) with_telemetry Sentry.LoggerHandler do - def add_logger_handler(opts) do - {id, config_overrides} = Keyword.pop(opts, :id, @default_handler_id) - config = Map.merge(@default_config, Map.new(config_overrides)) + def add_logger_handler(id, opts) do + config = Map.merge(@default_config, Map.new(opts)) :logger.add_handler(id, Sentry.LoggerHandler, %{config: config}) end else - def add_logger_handler(_opts), do: :ok + def add_logger_handler(_id, _opts), do: :ok end @spec set_tags_context(keyword()) :: :ok diff --git a/packages/sync-service/test/electric/telemetry/sentry_test.exs b/packages/sync-service/test/electric/telemetry/sentry_test.exs index 2e1f77a36b..499dea7e86 100644 --- a/packages/sync-service/test/electric/telemetry/sentry_test.exs +++ b/packages/sync-service/test/electric/telemetry/sentry_test.exs @@ -1,11 +1,11 @@ if Electric.telemetry_enabled?() and Code.ensure_loaded?(Sentry.LoggerHandler) do defmodule Electric.Telemetry.SentryTest do + # async: false because :logger handler state is VM-global — unique ids per + # test avoid name collisions but not the add/remove race across processes. use ExUnit.Case, async: false alias Electric.Telemetry.Sentry, as: ElectricSentry - # A dedicated handler id per test keeps state isolated and prevents - # interference with any handler that may have been added elsewhere. setup do id = :"sentry_test_#{System.unique_integer([:positive])}" on_exit(fn -> _ = :logger.remove_handler(id) end) @@ -17,9 +17,9 @@ if Electric.telemetry_enabled?() and Code.ensure_loaded?(Sentry.LoggerHandler) d config end - describe "add_logger_handler/1" do + describe "add_logger_handler/2" do test "installs Sentry.LoggerHandler with default config", %{handler_id: id} do - assert :ok = ElectricSentry.add_logger_handler(id: id) + assert :ok = ElectricSentry.add_logger_handler(id) {:ok, handler} = :logger.get_handler_config(id) assert handler.module == Sentry.LoggerHandler @@ -31,8 +31,7 @@ if Electric.telemetry_enabled?() and Code.ensure_loaded?(Sentry.LoggerHandler) d test "merges caller-supplied options into the handler config", %{handler_id: id} do assert :ok = - ElectricSentry.add_logger_handler( - id: id, + ElectricSentry.add_logger_handler(id, discard_threshold: 2000, sync_threshold: nil ) @@ -47,13 +46,13 @@ if Electric.telemetry_enabled?() and Code.ensure_loaded?(Sentry.LoggerHandler) d end test "caller-supplied options override defaults", %{handler_id: id} do - assert :ok = ElectricSentry.add_logger_handler(id: id, level: :warning) + assert :ok = ElectricSentry.add_logger_handler(id, level: :warning) assert %{level: :warning} = handler_config!(id) end - test "accepts an empty option list and uses the default handler id" do - default_id = :electric_sentry_handler + test "uses the default handler id when called with no arguments" do + default_id = ElectricSentry.default_handler_id() _ = :logger.remove_handler(default_id) on_exit(fn -> _ = :logger.remove_handler(default_id) end)