Skip to content

feat(tests): restore manual pid allowance for capturing events #1053

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

Merged
solnic merged 1 commit into from
May 13, 2026
Merged

feat(tests): restore manual pid allowance for capturing events #1053

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
11 changes: 10 additions & 1 deletion lib/sentry/application.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ defmodule Sentry.Application do

alias Sentry.Config

@compile {:no_warn_undefined, [NimbleOwnership]}

@impl true
def start(_type, _opts) do
config = Config.validate!()
Expand All @@ -32,7 +34,14 @@ defmodule Sentry.Application do

maybe_test_registry =
if Config.test_mode?() do
[Sentry.Test.Registry]
if Code.ensure_loaded?(NimbleOwnership) do
[
Sentry.Test.Registry,
{NimbleOwnership, name: Sentry.Test.OwnershipServer}
]
else
[Sentry.Test.Registry]
end
else
[]
end
Expand Down
174 changes: 134 additions & 40 deletions lib/sentry/test.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,12 @@ defmodule Sentry.Test do
Events are captured via the existing `before_send` and `before_send_log` callbacks
and stored in an isolated ETS table per test, preserving the full struct data.

> #### Bypass Required {: .info}
> #### Bypass and NimbleOwnership Required {: .info}
>
> This module requires `bypass` as a test dependency:
> This module requires `bypass` and `nimble_ownership` as test dependencies:
>
> {:bypass, "~> 2.0", only: [:test]}
> {:nimble_ownership, "~> 1.0", only: [:test]}

## Examples

Expand Down Expand Up @@ -52,9 +53,10 @@ defmodule Sentry.Test do

@moduledoc since: "10.2.0"

@compile {:no_warn_undefined, [Bypass, Plug.Conn]}
@compile {:no_warn_undefined, [Bypass, Plug.Conn, NimbleOwnership]}

@registry_table :sentry_test_collectors
@ownership_server Sentry.Test.OwnershipServer
@collector_key :sentry_test_collector

# Public API

Expand Down Expand Up @@ -108,6 +110,7 @@ defmodule Sentry.Test do
@spec setup_sentry(keyword()) :: %{bypass: term(), telemetry_processor: atom()}
def setup_sentry(extra_config \\ []) do
ensure_bypass_loaded!()
ensure_nimble_ownership_loaded!()

# Open a per-test Bypass and stub the envelope endpoint
bypass = Bypass.open()
Expand Down Expand Up @@ -251,19 +254,95 @@ defmodule Sentry.Test do
end

@doc """
Allows `pid_to_allow` to collect events back to the root process via `owner_pid`.
Allows `pid_to_allow` to collect events back to `owner_pid`'s test scope.

Use this when an unrelated process — one that does not appear in the
current test's `$callers` chain — needs to have its captured events
routed into this test's collector. Typical examples include Broadway
workers, processes started by `phoenix_test_playwright`, or
long-lived `GenServer`s that outlive the calling test process.

`pid_to_allow` may be a pid or a zero-arity function returning a pid;
the function form is resolved on call and is convenient when the pid
is not known until later.

This function is idempotent for the same `owner_pid`. It raises
`ArgumentError` when `owner_pid` has not yet called `setup_sentry/1`
(or `start_collecting_sentry_reports/0`), and raises when a different
live test scope already owns `pid_to_allow`.

Cleanup is automatic: allow entries are removed when the test exits
via the same `on_exit` callback registered by `setup_sentry/1`.

## Example

setup do
Sentry.Test.setup_sentry()
end

test "events from a Broadway worker are captured" do
{:ok, worker_pid} = MyApp.Worker.start_link()
:ok = Sentry.Test.allow_sentry_reports(self(), worker_pid)

send(worker_pid, :do_work_that_reports)

assert_receive {:done, _}
assert [%Sentry.Event{}] = Sentry.Test.pop_sentry_reports()
end

> #### Deprecated {: .warning}
>
> This function is deprecated and will be removed in v13.0.0.
> Child processes are automatically tracked via the `$callers` mechanism.
> There is no need to explicitly allow processes.
"""
@doc since: "10.2.0"
@doc deprecated: "Child processes are now automatically tracked via $callers"
@doc since: "13.0.2"
@spec allow_sentry_reports(pid(), pid() | (-> pid())) :: :ok
def allow_sentry_reports(_owner_pid, _pid_to_allow) do
:ok
def allow_sentry_reports(owner_pid, pid_or_fun) when is_pid(owner_pid) do
ensure_nimble_ownership_loaded!()
allowed_pid = resolve_allowed_pid(pid_or_fun)

case NimbleOwnership.allow(@ownership_server, owner_pid, allowed_pid, @collector_key) do
:ok ->
# Also route per-test config overrides (DSN, before_send hooks,
# the internal collector callback, etc.) through the owner's
# scope so that Sentry callbacks invoked from `allowed_pid`
# resolve to the same configuration the test set up.
Sentry.Test.Config.allow(owner_pid, allowed_pid)
:ok

{:error, %{reason: {:already_allowed, ^owner_pid}}} ->
# Idempotent re-allow under the same owner.
Sentry.Test.Config.allow(owner_pid, allowed_pid)
:ok

{:error, %{reason: {:already_allowed, existing_owner}}} ->
raise ArgumentError,
"cannot allow #{inspect(allowed_pid)} for #{inspect(owner_pid)}: " <>
"already allowed by another live test scope " <>
"(owner: #{inspect(existing_owner)})"

{:error, %{reason: :not_allowed}} ->
raise ArgumentError,
"owner #{inspect(owner_pid)} is not collecting Sentry reports; " <>
"call Sentry.Test.setup_sentry/1 or " <>
"Sentry.Test.start_collecting_sentry_reports/0 first"

{:error, %{reason: :already_an_owner}} ->
raise ArgumentError,
"cannot allow #{inspect(allowed_pid)} for #{inspect(owner_pid)}: " <>
"#{inspect(allowed_pid)} is already collecting Sentry reports " <>
"itself (called setup_sentry/1 or start_collecting_sentry_reports/0)"
end
end

defp resolve_allowed_pid(pid) when is_pid(pid), do: pid

defp resolve_allowed_pid(fun) when is_function(fun, 0) do
case fun.() do
pid when is_pid(pid) ->
pid

other ->
raise ArgumentError,
"expected the function passed to allow_sentry_reports/2 to return a pid, " <>
"got: #{inspect(other)}"
end
end

@doc """
Expand Down Expand Up @@ -636,16 +715,39 @@ defmodule Sentry.Test do
end
end

defp ensure_nimble_ownership_loaded! do
unless Code.ensure_loaded?(NimbleOwnership) do
raise """
NimbleOwnership is required for Sentry.Test but is not available.

Add it to your test dependencies in mix.exs:

{:nimble_ownership, "~> 1.0", only: [:test]}
"""
end
end

# Sets up collection infrastructure (ETS table, before_send wrapping, config)
# without opening a new Bypass. When no :dsn is provided in extra_config,
# falls back to the default Bypass DSN from Registry.
defp setup_collector(extra_config) do
ensure_nimble_ownership_loaded!()

uid = System.unique_integer([:positive])
collector_table = :"sentry_test_collector_#{uid}"
:ets.new(collector_table, [:ordered_set, :public, :named_table])

# Register this test's collector
:ets.insert(@registry_table, {self(), collector_table})
# Register this test as the NimbleOwnership owner of the collector key,
# with the collector ETS table as its metadata. NimbleOwnership monitors
# the owner pid and auto-cleans the key + every transitive allowance
# when the test process exits.
{:ok, _} =
NimbleOwnership.get_and_update(
@ownership_server,
self(),
@collector_key,
fn _prev -> {:ok, collector_table} end
)

# Store in process dict for pop_* lookups
Process.put(:sentry_test_collector, collector_table)
Expand Down Expand Up @@ -676,30 +778,19 @@ defmodule Sentry.Test do
Sentry.Test.Config.put_override(:_internal_before_send_log, collector)
Sentry.Test.Config.put_override(:_internal_before_send_metric, collector)

# The TelemetryProcessor's scheduler is not in `$callers` of this test —
# allow it explicitly so log/metric events routed through the buffered
# pipeline can find this test's collector.
scheduler_pid = get_scheduler_pid()

if scheduler_pid do
:ets.insert_new(@registry_table, {scheduler_pid, collector_table})
:ok =
NimbleOwnership.allow(@ownership_server, self(), scheduler_pid, @collector_key)
end

# Register cleanup
test_pid = self()

# Register cleanup for the collector ETS table only. NimbleOwnership
# cleans up the key and allowances automatically when this test exits.
ExUnit.Callbacks.on_exit(fn ->
if :ets.whereis(@registry_table) != :undefined do
:ets.delete(@registry_table, test_pid)

if scheduler_pid do
case :ets.lookup(@registry_table, scheduler_pid) do
[{^scheduler_pid, ^collector_table}] ->
:ets.delete(@registry_table, scheduler_pid)

_ ->
:ok
end
end
end

if :ets.whereis(collector_table) != :undefined do
:ets.delete(collector_table)
end
Expand All @@ -723,12 +814,15 @@ defmodule Sentry.Test do
defp find_collector do
pids = [self() | Process.get(:"$callers", [])]

Enum.find_value(pids, fn pid ->
case :ets.lookup(@registry_table, pid) do
[{^pid, table}] -> table
[] -> nil
end
end)
case NimbleOwnership.fetch_owner(@ownership_server, pids, @collector_key) do
{:ok, owner_pid} ->
@ownership_server
|> NimbleOwnership.get_owned(owner_pid, %{})
|> Map.get(@collector_key)

Comment on lines +818 to +822
Copy link
Copy Markdown

@sentry sentry Bot May 13, 2026

Choose a reason for hiding this comment

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

Bug: The pattern match for NimbleOwnership.fetch_owner/3 is incorrect. It expects {:ok, owner_pid} but receives {:ok, {pid, metadata}}, causing a runtime error in subsequent calls.
Severity: HIGH

Suggested Fix

Update the case statement in find_collector/0. The success clause should pattern match on {:ok, {owner_pid, _metadata}} to correctly extract the owner_pid. The error clause should match {:error, _reason} to correctly handle cases where no owner is found and avoid a CaseClauseError.

Prompt for AI Agent 
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: lib/sentry/test.ex#L818-L822

Potential issue: The `find_collector/0` function incorrectly pattern-matches the return
value from `NimbleOwnership.fetch_owner/3`. According to NimbleOwnership v1.0
documentation, `fetch_owner/3` returns `{:ok, {owner_pid, metadata}}`. However, the code
at line 818 matches `{:ok, owner_pid}`, which incorrectly binds the variable `owner_pid`
to the tuple `{actual_pid, metadata}`. This tuple is then passed as the `owner` argument
to `NimbleOwnership.get_owned/3`, which expects a PID, leading to a runtime failure.
Additionally, the `case` statement's error handling clause incorrectly matches `:error`
instead of `{:error, reason}`, which will raise a `CaseClauseError` if no owner is
found. This will cause all event collection in test mode to fail.

Did we get this right? 👍 / 👎 to inform future reviews.

:error ->
nil
end
end

# Standalone collecting callback. Records the struct in this test's
Expand Down
2 changes: 0 additions & 2 deletions lib/sentry/test/registry.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ defmodule Sentry.Test.Registry do
# Bypass and Plug.Conn may not be available at compile time (optional deps).
@compile {:no_warn_undefined, [Bypass, Bypass.Instance, Bypass.Supervisor, Plug.Conn]}

@table :sentry_test_collectors
@allows_table :sentry_test_scope_allows

@spec start_link(keyword()) :: GenServer.on_start()
Expand Down Expand Up @@ -86,7 +85,6 @@ defmodule Sentry.Test.Registry do

@impl true
def init(nil) do
_table = :ets.new(@table, [:named_table, :public, :set])
_allows_table = :ets.new(@allows_table, [:named_table, :public, :set])
maybe_start_default_bypass()
{:ok, :no_state}
Expand Down
1 change: 1 addition & 0 deletions mix.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ defmodule Sentry.Mixfile do
{:plug_cowboy, "~> 2.7", only: [:test]},
{:bandit, "~> 1.0", only: [:test]},
{:bypass, "~> 2.0", only: [:test]},
{:nimble_ownership, "~> 1.0", only: [:test]},
{:dialyxir, "~> 1.0", only: [:test, :dev], runtime: false},
{:ex_doc, "~> 0.29", only: :dev},
{:excoveralls, "~> 0.17.1", only: [:test]},
Expand Down
1 change: 1 addition & 0 deletions mix.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
"mimerl": {:hex, :mimerl, "1.3.0", "d0cd9fc04b9061f82490f6581e0128379830e78535e017f7780f37fea7545726", [:rebar3], [], "hexpm", "a1e15a50d1887217de95f0b9b0793e32853f7c258a5cd227650889b38839fe9d"},
"mint": {:hex, :mint, "1.7.1", "113fdb2b2f3b59e47c7955971854641c61f378549d73e829e1768de90fc1abf1", [:mix], [{:castore, "~> 0.1.0 or ~> 1.0", [hex: :castore, repo: "hexpm", optional: true]}, {:hpax, "~> 0.1.1 or ~> 0.2.0 or ~> 1.0", [hex: :hpax, repo: "hexpm", optional: false]}], "hexpm", "fceba0a4d0f24301ddee3024ae116df1c3f4bb7a563a731f45fdfeb9d39a231b"},
"nimble_options": {:hex, :nimble_options, "1.1.1", "e3a492d54d85fc3fd7c5baf411d9d2852922f66e69476317787a7b2bb000a61b", [:mix], [], "hexpm", "821b2470ca9442c4b6984882fe9bb0389371b8ddec4d45a9504f00a66f650b44"},
"nimble_ownership": {:hex, :nimble_ownership, "1.0.2", "fa8a6f2d8c592ad4d79b2ca617473c6aefd5869abfa02563a77682038bf916cf", [:mix], [], "hexpm", "098af64e1f6f8609c6672127cfe9e9590a5d3fcdd82bc17a377b8692fd81a879"},
"nimble_parsec": {:hex, :nimble_parsec, "1.4.0", "51f9b613ea62cfa97b25ccc2c1b4216e81df970acd8e16e8d1bdc58fef21370d", [:mix], [], "hexpm", "9c565862810fb383e9838c1dd2d7d2c437b3d13b267414ba6af33e50d2d1cf28"},
"nimble_pool": {:hex, :nimble_pool, "1.1.0", "bf9c29fbdcba3564a8b800d1eeb5a3c58f36e1e11d7b7fb2e084a643f645f06b", [:mix], [], "hexpm", "af2e4e6b34197db81f7aad230c1118eac993acc0dae6bc83bac0126d4ae0813a"},
"oban": {:hex, :oban, "2.18.3", "1608c04f8856c108555c379f2f56bc0759149d35fa9d3b825cb8a6769f8ae926", [:mix], [{:ecto_sql, "~> 3.10", [hex: :ecto_sql, repo: "hexpm", optional: false]}, {:ecto_sqlite3, "~> 0.9", [hex: :ecto_sqlite3, repo: "hexpm", optional: true]}, {:jason, "~> 1.1", [hex: :jason, repo: "hexpm", optional: false]}, {:postgrex, "~> 0.16", [hex: :postgrex, repo: "hexpm", optional: true]}, {:telemetry, "~> 0.4 or ~> 1.0", [hex: :telemetry, repo: "hexpm", optional: false]}], "hexpm", "36ca6ca84ef6518f9c2c759ea88efd438a3c81d667ba23b02b062a0aa785475e"},
Expand Down
Loading
Loading