Skip to content

Commit 9f7c9b8

Browse files
solnicclaude
andcommitted
feat(test): make TelemetryProcessor handling work with async tests
Closes the gap that left buffered events (logs and metrics) emitted from processes registered via Sentry.Test.allow_sentry_reports/2 silently dropped instead of routed to the test's collector under async tests. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 parent ae617b2 commit 9f7c9b8

7 files changed

Lines changed: 349 additions & 75 deletions

File tree

lib/sentry/application.ex

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -53,15 +53,15 @@ defmodule Sentry.Application do
5353
[]
5454
end
5555

56-
telemetry_processor =
56+
telemetry_processor_opts =
5757
[
58-
{Sentry.TelemetryProcessor,
59-
[
60-
buffer_capacities: Config.telemetry_buffer_capacities(),
61-
scheduler_weights: Config.telemetry_scheduler_weights(),
62-
transport_capacity: Config.transport_capacity()
63-
]}
58+
buffer_capacities: Config.telemetry_buffer_capacities(),
59+
scheduler_weights: Config.telemetry_scheduler_weights(),
60+
transport_capacity: Config.transport_capacity()
6461
]
62+
|> maybe_put_test_processor_resolver()
63+
64+
telemetry_processor = [{Sentry.TelemetryProcessor, telemetry_processor_opts}]
6565

6666
children =
6767
maybe_test_registry ++
@@ -162,4 +162,12 @@ defmodule Sentry.Application do
162162
else
163163
defp maybe_rate_limiter, do: [Sentry.Transport.RateLimiter]
164164
end
165+
166+
defp maybe_put_test_processor_resolver(opts) do
167+
if Config.test_mode?() do
168+
Keyword.put(opts, :processor_resolver, &Sentry.Test.Registry.lookup_processor_for/1)
169+
else
170+
opts
171+
end
172+
end
165173
end

lib/sentry/telemetry_processor.ex

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,9 @@ defmodule Sentry.TelemetryProcessor do
5454
| {:scheduler_weights, %{Category.priority() => pos_integer()}}
5555
| {:on_envelope, (Sentry.Envelope.t() -> any())}
5656
| {:transport_capacity, pos_integer()}
57+
| {:processor_resolver, (pid() -> atom() | nil) | nil}
58+
59+
@resolver_key {__MODULE__, :resolver}
5760

5861
## Public API
5962

@@ -74,6 +77,11 @@ defmodule Sentry.TelemetryProcessor do
7477
* `:scheduler_weights` - Map of priority to weight override (optional)
7578
* `:on_envelope` - Callback function invoked when envelopes are ready to send (optional)
7679
* `:transport_capacity` - Maximum number of items the transport queue can hold (default: 1000). For log envelopes, each log event counts as one item.
80+
* `:processor_resolver` - Optional `(pid() -> atom() | nil)` function used by `add/1` to discover
81+
the per-pid processor name when no value is set in the process dictionary. The resolver is
82+
stored in `:persistent_term` and shared across all processor instances; passing the same
83+
function from every `start_link/1` call is safe and idempotent. Defaults to `nil`, in which
84+
case `add/1` falls back to the default processor name.
7785
7886
## Examples
7987
@@ -294,6 +302,11 @@ defmodule Sentry.TelemetryProcessor do
294302
on_envelope = Keyword.get(opts, :on_envelope)
295303
transport_capacity = Keyword.get(opts, :transport_capacity, 1000)
296304

305+
case Keyword.get(opts, :processor_resolver) do
306+
nil -> :ok
307+
resolver when is_function(resolver, 1) -> :persistent_term.put(@resolver_key, resolver)
308+
end
309+
297310
buffer_names =
298311
for category <- Category.all(), into: %{} do
299312
{category, buffer_name(processor_name, category)}
@@ -374,7 +387,20 @@ defmodule Sentry.TelemetryProcessor do
374387
end
375388

376389
defp processor_name do
377-
Process.get(:sentry_telemetry_processor, @default_name)
390+
case Process.get(:sentry_telemetry_processor) do
391+
name when is_atom(name) and not is_nil(name) ->
392+
name
393+
394+
_ ->
395+
resolve_processor_name() || @default_name
396+
end
397+
end
398+
399+
defp resolve_processor_name do
400+
case :persistent_term.get(@resolver_key, nil) do
401+
nil -> nil
402+
resolver when is_function(resolver, 1) -> resolver.(self())
403+
end
378404
end
379405

380406
defp maybe_add_opt(opts, _key, nil), do: opts

lib/sentry/test.ex

Lines changed: 18 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -163,32 +163,35 @@ defmodule Sentry.Test do
163163
def setup_telemetry_processor do
164164
case Process.get(:sentry_telemetry_processor) do
165165
name when is_atom(name) and not is_nil(name) ->
166-
if processor_alive?(name), do: name, else: do_setup_telemetry_processor()
166+
if processor_alive?(name), do: name, else: start_telemetry_processor()
167167

168168
_ ->
169-
do_setup_telemetry_processor()
169+
start_telemetry_processor()
170170
end
171171
end
172172

173-
defp do_setup_telemetry_processor do
173+
defp start_telemetry_processor do
174174
uid = System.unique_integer([:positive])
175175
processor_name = :"test_telemetry_processor_#{uid}"
176176

177177
ExUnit.Callbacks.start_supervised!(
178-
{Sentry.TelemetryProcessor, name: processor_name},
178+
{Sentry.TelemetryProcessor,
179+
name: processor_name, processor_resolver: &Sentry.Test.Registry.lookup_processor_for/1},
179180
id: processor_name
180181
)
181182

183+
Process.put(:sentry_telemetry_processor, processor_name)
184+
182185
scheduler_pid = Sentry.TelemetryProcessor.get_scheduler(processor_name)
183186

184187
if scheduler_pid do
185188
# Goes through the unified `:sentry_test_scope` key, which also
186189
# populates the merged routing ETS row so `Config.namespace/1`
187190
# resolves the scheduler pid back to this test's scope.
188191
Sentry.Test.Registry.claim_allow(self(), scheduler_pid, :soft)
192+
tag_processor_for_allowed_pid(self(), scheduler_pid)
189193
end
190194

191-
Process.put(:sentry_telemetry_processor, processor_name)
192195
processor_name
193196
end
194197

@@ -227,8 +230,7 @@ defmodule Sentry.Test do
227230
228231
> #### Deprecated {: .warning}
229232
>
230-
> This function is deprecated and will be removed in v13.0.0.
231-
> Use `setup_sentry/1` or `start_collecting_sentry_reports/0` instead.
233+
> This function is deprecated and will be removed in v14.0.0. Use `setup_sentry/1` instead.
232234
233235
The `:owner`, `:cleanup`, and `:key` options are no longer supported and are ignored.
234236
"""
@@ -249,11 +251,11 @@ defmodule Sentry.Test do
249251
250252
> #### Deprecated {: .warning}
251253
>
252-
> This function is deprecated and will be removed in v13.0.0.
253-
> Cleanup is now handled automatically via `on_exit` callbacks.
254+
> This function is deprecated and will be removed in v14.0.0.
255+
> Cleanup is now handled automatically when the owning test process exits.
254256
"""
255257
@doc since: "10.2.0"
256-
@doc deprecated: "Cleanup is now automatic via on_exit callbacks"
258+
@doc deprecated: "Cleanup is now automatic when the owning test process exits"
257259
@spec cleanup(pid()) :: :ok
258260
def cleanup(owner_pid) when is_pid(owner_pid) do
259261
:ok
@@ -844,22 +846,24 @@ defmodule Sentry.Test do
844846

845847
if scheduler_pid do
846848
Sentry.Test.Registry.claim_allow(self(), scheduler_pid, :soft)
849+
tag_processor_for_allowed_pid(self(), scheduler_pid)
847850
end
848851

849852
# Register cleanup for the collector ETS table only. NimbleOwnership
850853
# cleans up the key and allowances automatically when this test exits.
851854
# Drop any worker→processor routing rows that point at this test's
852-
# processor so a test that exits before its allowed pids do does not
855+
# processor so a test that exits before its allowed pids do not
853856
# leave stale rows pointing at a stopped per-test processor.
854-
processor_name =
855-
Process.get(:sentry_telemetry_processor, Sentry.TelemetryProcessor.default_name())
857+
processor_name = Process.get(:sentry_telemetry_processor)
856858

857859
ExUnit.Callbacks.on_exit(fn ->
858860
if :ets.whereis(collector_table) != :undefined do
859861
:ets.delete(collector_table)
860862
end
861863

862-
Sentry.Test.Registry.drop_processor_routing_for(processor_name)
864+
if is_atom(processor_name) and not is_nil(processor_name) do
865+
Sentry.Test.Registry.drop_processor_routing_for(processor_name)
866+
end
863867
end)
864868

865869
:ok

lib/sentry/test/registry.ex

Lines changed: 68 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -55,21 +55,18 @@ defmodule Sentry.Test.Registry do
5555
end
5656

5757
@doc """
58-
Removes every routing row whose owner is `owner_pid`. Direct ETS
59-
match_delete — atomic, no GenServer round-trip.
60-
61-
Kept for the case where a scope wants to drop its allowances
62-
explicitly (e.g. `Sentry.Test.Scope.Registry.unregister/1`); the
63-
`:DOWN` handler also prunes rows automatically when the owner pid
64-
exits.
58+
Ensures `owner_pid` is monitored by the registry so that the
59+
`:DOWN` handler runs cleanup (routing-table prune + scope-state
60+
erase via `Sentry.Test.Scope.Registry.handle_owner_down/1`) when
61+
the owner exits. Idempotent.
62+
63+
Called from `Sentry.Test.Scope.Registry.update/1` on first scope
64+
creation so cleanup does not depend on `claim_allow` ever being
65+
invoked for this owner.
6566
"""
66-
@spec drop_allows_for(pid()) :: :ok
67-
def drop_allows_for(owner_pid) when is_pid(owner_pid) do
68-
if :ets.whereis(@routing_table) != :undefined do
69-
:ets.match_delete(@routing_table, {:_, owner_pid, :_})
70-
end
71-
72-
:ok
67+
@spec monitor_owner(pid()) :: :ok
68+
def monitor_owner(owner_pid) when is_pid(owner_pid) do
69+
GenServer.call(__MODULE__, {:monitor_owner, owner_pid})
7370
end
7471

7572
@doc """
@@ -166,38 +163,63 @@ defmodule Sentry.Test.Registry do
166163
{:ok, %{owner_monitors: %{}}}
167164
end
168165

166+
# Serialization note: every claim funnels through this single named
167+
# GenServer and holds it across TWO blocking round-trips to the
168+
# ownership server — `ensure_scope_owner/1`'s
169+
# `NimbleOwnership.get_and_update/4` and `NimbleOwnership.allow/4`.
170+
# This is the deliberate price of atomicity (no two concurrent async
171+
# tests can both pass a check-then-write race for the same
172+
# `allowed_pid`). It is acceptable because claims happen at test
173+
# setup, not per event, and the hot config/buffer read paths
174+
# (`lookup_allow_owner/1`, `lookup_processor_for/1`) bypass this
175+
# GenServer with lock-free direct ETS reads.
169176
@impl true
170177
def handle_call({:claim_allow, owner_pid, allowed_pid, mode}, _from, state) do
171178
state = ensure_owner_monitored(state, owner_pid)
172-
ensure_scope_owner(owner_pid)
173179

174180
reply =
175-
case NimbleOwnership.allow(@ownership_server, owner_pid, allowed_pid, @scope_key) do
181+
case ensure_scope_owner(owner_pid) do
182+
{:error, {:taken, existing_owner}} ->
183+
if mode == :strict, do: {:error, {:taken, existing_owner}}, else: :skipped
184+
176185
:ok ->
177-
upsert_owner(allowed_pid, owner_pid)
178-
:ok
186+
case NimbleOwnership.allow(@ownership_server, owner_pid, allowed_pid, @scope_key) do
187+
:ok ->
188+
upsert_owner(allowed_pid, owner_pid)
189+
:ok
179190

180-
{:error, %{reason: {:already_allowed, ^owner_pid}}} ->
181-
upsert_owner(allowed_pid, owner_pid)
182-
:ok
191+
{:error, %{reason: {:already_allowed, ^owner_pid}}} ->
192+
upsert_owner(allowed_pid, owner_pid)
193+
:ok
183194

184-
{:error, %{reason: {:already_allowed, other}}} ->
185-
if mode == :strict, do: {:error, {:taken, other}}, else: :skipped
195+
{:error, %{reason: {:already_allowed, other}}} ->
196+
if mode == :strict, do: {:error, {:taken, other}}, else: :skipped
186197

187-
{:error, %{reason: :already_an_owner}} ->
188-
# `allowed_pid` is itself a scope owner — treat as a conflict.
189-
if mode == :strict, do: {:error, {:taken, allowed_pid}}, else: :skipped
198+
{:error, %{reason: :already_an_owner}} ->
199+
# `allowed_pid` is itself a scope owner — treat as a conflict.
200+
if mode == :strict, do: {:error, {:taken, allowed_pid}}, else: :skipped
201+
202+
{:error, %{reason: :not_allowed}} ->
203+
if mode == :strict, do: {:error, {:taken, allowed_pid}}, else: :skipped
204+
end
190205
end
191206

192207
{:reply, reply, state}
193208
end
194209

210+
def handle_call({:monitor_owner, owner_pid}, _from, state) do
211+
state = ensure_owner_monitored(state, owner_pid)
212+
{:reply, :ok, state}
213+
end
214+
195215
@impl true
196216
def handle_info({:DOWN, _ref, :process, pid, _reason}, state) do
197217
if :ets.whereis(@routing_table) != :undefined do
198218
:ets.match_delete(@routing_table, {:_, pid, :_})
199219
end
200220

221+
Sentry.Test.Scope.Registry.handle_owner_down(pid)
222+
201223
{:noreply, %{state | owner_monitors: Map.delete(state.owner_monitors, pid)}}
202224
end
203225

@@ -220,21 +242,38 @@ defmodule Sentry.Test.Registry do
220242
# `Sentry.Test.setup_collector/1` (e.g. a test that uses
221243
# `Sentry.Test.Config.put/1` standalone). When the owner already
222244
# owns the key, the existing metadata is preserved.
245+
#
246+
# INVARIANT: the `:sentry_test_scope` key's metadata is overloaded —
247+
# `Sentry.Test.setup_collector/1` stores the per-test collector ETS
248+
# table name (an atom) under it, while this function stores a bare
249+
# `%{}` marker for collector-less scopes. `Sentry.Test`'s
250+
# `owner_collecting?/1` distinguishes the two purely by value type
251+
# (atom = collecting, map = not). Therefore the update fun below MUST
252+
# preserve an existing value (`current -> {:ok, current}`) and MUST
253+
# NOT overwrite it with `%{}`; doing so would silently turn a
254+
# collecting scope into a non-collecting one with no type error.
223255
defp ensure_scope_owner(owner_pid) do
224256
case NimbleOwnership.get_and_update(
225257
@ownership_server,
226258
owner_pid,
227259
@scope_key,
228260
# Metadata MUST be non-nil so that NimbleOwnership treats
229261
# `owner_pid` as a key owner (its `cond` in `allow/4` checks
230-
# truthiness of the metadata). Preserve any existing value.
262+
# truthiness of the metadata). Preserve any existing value
263+
# (see the INVARIANT above — never clobber a collector atom).
231264
fn
232265
nil -> {:ok, %{}}
233266
current -> {:ok, current}
234267
end
235268
) do
236-
{:ok, _} -> :ok
237-
{:error, _} -> :ok
269+
{:ok, _} ->
270+
:ok
271+
272+
{:error, %{reason: {:already_allowed, existing_owner}}} ->
273+
{:error, {:taken, existing_owner}}
274+
275+
{:error, _} ->
276+
:ok
238277
end
239278
end
240279

0 commit comments

Comments
 (0)