feat: improve observability#3
Conversation
yordis
commented
Apr 19, 2026
yordis
commented
- Mirror of feat: improve observability commanded/eventstore#323 into our fork
PR SummaryMedium Risk Overview Public APIs like Introduces Reviewed by Cursor Bugbot for commit 3ea659c. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 8 minutes and 43 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
WalkthroughAdds telemetry: new Changes
Sequence DiagramsequenceDiagram
participant Client
participant API as EventStore.PublicAPI
participant Telemetry as EventStore.Telemetry
participant Storage as Storage/DB
Client->>API: call operation(args)
API->>Telemetry: start span (operation, start_metadata)
activate Telemetry
Telemetry-->>API: span started
deactivate Telemetry
API->>Storage: perform operation (read/append/delete/subscribe)
activate Storage
alt streaming with batches
Storage-->>API: batch of events
API->>Telemetry: start :stream_batch_read (batch metadata)
Telemetry-->>API: batch span started
API-->>Telemetry: stop :stream_batch_read (event_count, result)
else single call
Storage-->>API: result or error
end
deactivate Storage
alt success
API->>Telemetry: stop span (result)
else raise
API->>Telemetry: exception (kind, reason, stacktrace)
end
activate Telemetry
Telemetry-->>API: span completed
deactivate Telemetry
API-->>Client: return or raise
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/event_store.ex (1)
470-489:⚠️ Potential issue | 🟡 MinorRoute
$alldelete errors through the telemetry span.The
$allfunction head returns beforetelemetry_span/4, sodelete_stream("$all", ...)emits no:delete_streamstart/stop event. Remove the early clause and let the existingcaseinside the span handle this error path.Proposed fix
- def delete_stream(`@all_stream`, _expected_version, _type, _opts), - do: {:error, :cannot_delete_all_stream} - def delete_stream(stream_uuid, expected_version, type, opts) when type in [:soft, :hard] do telemetry_span( :delete_stream,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/event_store.ex` around lines 470 - 489, Remove the early clause that short-circuits delete_stream for `@all_stream` so the telemetry_span in the main delete_stream/4 clause can observe and emit start/stop events; specifically delete the head def delete_stream(`@all_stream`, _expected_version, _type, _opts) -> {:error, :cannot_delete_all_stream} and keep the existing def delete_stream(stream_uuid, expected_version, type, opts) when type in [:soft, :hard] so the internal case (matching `@all_stream`) runs inside telemetry_span/4 and returns the same {:error, :cannot_delete_all_stream} via Stream.delete or the case branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/event_store/streams/stream.ex`:
- Around line 257-260: The stream resource's case over
read_stream_batch(:forward, ...) only matches {:ok, []} and {:ok, events},
causing a CaseClauseError when read_stream_batch/6 returns {:error, reason};
update both stream loops (the one using read_stream_batch(:forward, conn,
stream, next_version, read_batch_size, opts) and the other similar loop) to
explicitly match {:error, reason} and propagate the error by returning a halting
tuple that preserves the error (e.g., {:halt, {:error, reason}}) so the
enumerator can surface the original storage failure instead of failing with a
CaseClauseError.
---
Outside diff comments:
In `@lib/event_store.ex`:
- Around line 470-489: Remove the early clause that short-circuits delete_stream
for `@all_stream` so the telemetry_span in the main delete_stream/4 clause can
observe and emit start/stop events; specifically delete the head def
delete_stream(`@all_stream`, _expected_version, _type, _opts) -> {:error,
:cannot_delete_all_stream} and keep the existing def delete_stream(stream_uuid,
expected_version, type, opts) when type in [:soft, :hard] so the internal case
(matching `@all_stream`) runs inside telemetry_span/4 and returns the same
{:error, :cannot_delete_all_stream} via Stream.delete or the case branch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 20996183-d154-40fe-82f2-e1e09060308d
📒 Files selected for processing (8)
guides/Usage.mdlib/event_store.exlib/event_store/streams/stream.exlib/event_store/telemetry.exmix.exstest/migrate_event_store_test.exstest/storage/append_events_test.exstest/telemetry_test.exs
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
yordis
force-pushed
the
yordis/feat-add-telemetry
branch
from
6770d72 to
298c572
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
test/streams/single_stream_test.exs (1)
488-519: Same hard-coded schema issue as the forward test.Mirror the fix from the forward case so the backward test honors the
schemavariable as well. Additionally, consider extracting the shared "force batch read failure inside a transaction" scaffolding into a helper to keep the two tests in sync going forward.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/streams/single_stream_test.exs` around lines 488 - 519, The backward-stream test "should raise the underlying batch read failure when streaming backward" currently uses a hard-coded schema name; update the Stream.stream_backward call to pass the test's schema variable (schema: schema) and serializer variable (serializer: serializer) just like the forward test, so the query uses the provided schema rather than a literal; optionally extract the repeated "force batch read failure inside a transaction" scaffolding (the Postgrex.transaction block, the ALTER TABLE rename, Enum.reduce_while pattern, and Postgrex.rollback) into a shared helper function used by both the forward and backward tests to keep them in sync.test/telemetry_test.exs (2)
236-262: Minor asymmetry between forward and backward per-batch tests.The backward test ends with
refute_receive {:telemetry_event, [:eventstore, :stream_batch_read, _], _, _}to ensure no extra events leaked, but the forward test has no equivalent guard. Withread_batch_size: 1and 2 events, forward produces 3 batch reads (including the terminating empty read) and the threeassert_stream_batch_read/5calls drain them — but a trailingrefute_receivewould make that intent explicit and catch future regressions where an extra batch is read.🛠️ Suggested addition
assert_stream_batch_read(:forward, stream_uuid, 1, 1, 1) assert_stream_batch_read(:forward, stream_uuid, 2, 1, 1) assert_stream_batch_read(:forward, stream_uuid, 3, 1, 0) + + refute_receive {:telemetry_event, [:eventstore, :stream_batch_read, _], _, _} end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/telemetry_test.exs` around lines 236 - 262, In the "emits per-batch telemetry for stream_forward" test, add the same final guard used in the backward test to ensure no extra telemetry events leak: after the three assert_stream_batch_read/5 calls insert a refute_receive {:telemetry_event, [:eventstore, :stream_batch_read, _], _, _} to mirror the backward test and explicitly verify no additional EventStore.stream_forward batch events are emitted; keep the attach_telemetry(:stream_batch_read) and existing EventStore.stream_forward invocation unchanged.
221-234: Test name is slightly broader than what's being asserted.This test only attaches handlers for
[:eventstore, :stream_forward, …]/[:eventstore, :stream_backward, …], which by design are never emitted — the actual per-batch telemetry fires under:stream_batch_read(and IS emitted here, sinceEnum.count/1forces enumeration). Therefute_receivetherefore passes trivially. Consider renaming to e.g. "does not emit top-level span telemetry for lazy stream APIs" so the invariant under test is unambiguous.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/telemetry_test.exs` around lines 221 - 234, The test name is too broad: update the test "does not emit telemetry for lazy stream APIs" to explicitly state the invariant being checked (e.g. "does not emit top-level span telemetry for lazy stream APIs") so it matches the handlers attached via attach_telemetry and the actual telemetry signals under test; locate the test function and rename it (the test block and its string description) while keeping the attach_telemetry([...]) call targeting [:eventstore, :stream_forward] and [:eventstore, :stream_backward] and leaving assertions on EventStore.stream_forward/1, EventStore.stream_all_forward/0, EventStore.stream_backward/1, and EventStore.stream_all_backward/0 unchanged. Ensure any test helper references (like attach_telemetry) are consistent with the new description.lib/event_store/telemetry.ex (1)
6-12:metadata/3LGTM; just be aware of key precedence.
extra_metadatakeys are overwritten by:event_store(always) and:name(whenopts[:name]applies) — that precedence is correct for this codebase but worth a single-line comment or module doc note, since callers that happen to pass:event_store/:nameinextra_metadatawill silently have those values replaced.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/event_store/telemetry.ex` around lines 6 - 12, The metadata/3 function silently overwrites keys from extra_metadata with :event_store (always) and :name (via maybe_put_name/3), so add a brief single-line comment above metadata/3 (or a module doc note) stating that extra_metadata keys may be overridden and that :event_store and :name take precedence; reference metadata/3 and maybe_put_name/3 in the comment so future callers know that passing :event_store or :name in extra_metadata will be replaced.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/streams/single_stream_test.exs`:
- Around line 362-393: The test uses a hard-coded "public.events" in the
Postgrex.query! ALTER TABLE which ignores the provided schema; change the ALTER
TABLE command to interpolate the test's schema variable (schema) and the events
table name so it renames "#{schema}.events" instead of "public.events" when run
inside Postgrex.transaction with Stream.stream_forward; also rename the
accumulator variable batches_read to events_processed (or similar) in the
Enum.reduce_while call to reflect that the loop iterates per event, not per
batch, and update any related comparisons (e.g., if events_processed == 0) so
the logic remains the same.
---
Nitpick comments:
In `@lib/event_store/telemetry.ex`:
- Around line 6-12: The metadata/3 function silently overwrites keys from
extra_metadata with :event_store (always) and :name (via maybe_put_name/3), so
add a brief single-line comment above metadata/3 (or a module doc note) stating
that extra_metadata keys may be overridden and that :event_store and :name take
precedence; reference metadata/3 and maybe_put_name/3 in the comment so future
callers know that passing :event_store or :name in extra_metadata will be
replaced.
In `@test/streams/single_stream_test.exs`:
- Around line 488-519: The backward-stream test "should raise the underlying
batch read failure when streaming backward" currently uses a hard-coded schema
name; update the Stream.stream_backward call to pass the test's schema variable
(schema: schema) and serializer variable (serializer: serializer) just like the
forward test, so the query uses the provided schema rather than a literal;
optionally extract the repeated "force batch read failure inside a transaction"
scaffolding (the Postgrex.transaction block, the ALTER TABLE rename,
Enum.reduce_while pattern, and Postgrex.rollback) into a shared helper function
used by both the forward and backward tests to keep them in sync.
In `@test/telemetry_test.exs`:
- Around line 236-262: In the "emits per-batch telemetry for stream_forward"
test, add the same final guard used in the backward test to ensure no extra
telemetry events leak: after the three assert_stream_batch_read/5 calls insert a
refute_receive {:telemetry_event, [:eventstore, :stream_batch_read, _], _, _} to
mirror the backward test and explicitly verify no additional
EventStore.stream_forward batch events are emitted; keep the
attach_telemetry(:stream_batch_read) and existing EventStore.stream_forward
invocation unchanged.
- Around line 221-234: The test name is too broad: update the test "does not
emit telemetry for lazy stream APIs" to explicitly state the invariant being
checked (e.g. "does not emit top-level span telemetry for lazy stream APIs") so
it matches the handlers attached via attach_telemetry and the actual telemetry
signals under test; locate the test function and rename it (the test block and
its string description) while keeping the attach_telemetry([...]) call targeting
[:eventstore, :stream_forward] and [:eventstore, :stream_backward] and leaving
assertions on EventStore.stream_forward/1, EventStore.stream_all_forward/0,
EventStore.stream_backward/1, and EventStore.stream_all_backward/0 unchanged.
Ensure any test helper references (like attach_telemetry) are consistent with
the new description.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d1959ec3-4d6d-4571-918b-0c4d02e9c07e
📒 Files selected for processing (9)
guides/Usage.mdlib/event_store.exlib/event_store/streams/stream.exlib/event_store/telemetry.exmix.exstest/migrate_event_store_test.exstest/storage/append_events_test.exstest/streams/single_stream_test.exstest/telemetry_test.exs
✅ Files skipped from review due to trivial changes (3)
- mix.exs
- test/storage/append_events_test.exs
- guides/Usage.md
🚧 Files skipped from review as they are similar to previous changes (2)
- test/migrate_event_store_test.exs
- lib/event_store.ex
yordis
force-pushed
the
yordis/feat-add-telemetry
branch
2 times, most recently
from
1ab9ade to
ec67004
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/streams/single_stream_test.exs (1)
362-393: Duplicated test body across forward/backward variants.The two new tests are structurally identical apart from
stream_forward/stream_backwardand the start version. Consider extracting a small helper (e.g.assert_batch_read_failure_raises/4) taking the stream-building function and start version to remove the duplication.Also applies to: 488-519
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/streams/single_stream_test.exs` around lines 362 - 393, The two tests duplicate the same logic for Stream.stream_forward and Stream.stream_backward; extract a helper (e.g. assert_batch_read_failure_raises/4) that accepts a stream builder function (or module+function) and a start_version, then move the transaction+stream construction and the Enum.reduce_while/assert_raise logic into that helper and call it from both tests passing Stream.stream_forward and Stream.stream_backward (or anonymous wrappers) along with the appropriate start version, keeping existing arguments schema, serializer, stream_uuid and the Postgrex.transaction/rollback behavior intact.lib/event_store.ex (1)
642-649: Keep lazy stream:namemetadata consistent withTelemetry.metadata/3.Line 648 always re-adds
:namewhen one was passed, even when it equals__MODULE__. Other spans omit:namein that case, so:stream_batch_readmetadata can differ from the public API spans for the same default event store.♻️ Proposed consistency fix
- defp annotate_stream_telemetry_opts(opts, nil) do + defp annotate_stream_telemetry_opts(opts, event_store_name) + when event_store_name in [nil, __MODULE__] do Keyword.put(opts, :event_store, __MODULE__) end defp annotate_stream_telemetry_opts(opts, event_store_name) do🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/event_store.ex` around lines 642 - 649, The helper annotate_stream_telemetry_opts currently always sets :name when event_store_name is provided, causing :name to be present even when it equals __MODULE__; change annotate_stream_telemetry_opts so it only adds the :name key if event_store_name is non-nil and not equal to __MODULE__ (i.e., keep putting :event_store => __MODULE__ but conditionally add :name only when event_store_name != __MODULE__), updating the clause for defp annotate_stream_telemetry_opts(opts, event_store_name) to perform that conditional put.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/streams/single_stream_test.exs`:
- Around line 362-393: The test pins a latent bug by expecting a CaseClauseError
caused when read_stream_batch/6 returns {:error, reason} and falls through the
case in stream_storage_forward/2 and stream_storage_backward/2; update these
functions to explicitly handle {:error, reason} (e.g., convert to {:halt,
{:error, reason}} or re-raise a clear descriptive error) inside the per-batch
Telemetry.span handling so batch-errors are surfaced deterministically, and
tighten the test to assert the surfaced error reason (or the new descriptive
error) instead of CaseClauseError; reference stream_storage_forward/2,
stream_storage_backward/2, and read_stream_batch/6 when making the change.
---
Nitpick comments:
In `@lib/event_store.ex`:
- Around line 642-649: The helper annotate_stream_telemetry_opts currently
always sets :name when event_store_name is provided, causing :name to be present
even when it equals __MODULE__; change annotate_stream_telemetry_opts so it only
adds the :name key if event_store_name is non-nil and not equal to __MODULE__
(i.e., keep putting :event_store => __MODULE__ but conditionally add :name only
when event_store_name != __MODULE__), updating the clause for defp
annotate_stream_telemetry_opts(opts, event_store_name) to perform that
conditional put.
In `@test/streams/single_stream_test.exs`:
- Around line 362-393: The two tests duplicate the same logic for
Stream.stream_forward and Stream.stream_backward; extract a helper (e.g.
assert_batch_read_failure_raises/4) that accepts a stream builder function (or
module+function) and a start_version, then move the transaction+stream
construction and the Enum.reduce_while/assert_raise logic into that helper and
call it from both tests passing Stream.stream_forward and Stream.stream_backward
(or anonymous wrappers) along with the appropriate start version, keeping
existing arguments schema, serializer, stream_uuid and the
Postgrex.transaction/rollback behavior intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9be9e0ce-24ac-4a88-b7f4-69d06e1dc228
📒 Files selected for processing (9)
guides/Usage.mdlib/event_store.exlib/event_store/streams/stream.exlib/event_store/telemetry.exmix.exstest/migrate_event_store_test.exstest/storage/append_events_test.exstest/streams/single_stream_test.exstest/telemetry_test.exs
✅ Files skipped from review due to trivial changes (5)
- test/migrate_event_store_test.exs
- mix.exs
- test/storage/append_events_test.exs
- guides/Usage.md
- lib/event_store/streams/stream.ex
yordis
force-pushed
the
yordis/feat-add-telemetry
branch
2 times, most recently
from
82e7376 to
5266518
Compare
There was a problem hiding this comment.
Pull request overview
This PR mirrors upstream changes to improve observability by instrumenting EventStore’s public API with :telemetry spans, adding consistent metadata/result normalization, and documenting the emitted events.
Changes:
- Add
EventStore.Telemetrywrapper around:telemetry.spanwith normalized:resultand consistent metadata (:event_store, optional:name). - Instrument public operations (append/read/delete/subscribe/snapshots/pagination) and emit per-batch telemetry for lazy stream enumeration via
:stream_batch_read. - Add/adjust tests and documentation for telemetry plus a couple of test-suite robustness tweaks (timeout/cancellation + pg_dump directives).
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
lib/event_store/telemetry.ex |
Introduces Telemetry helper module for metadata shaping and span emission. |
lib/event_store.ex |
Wraps multiple public operations in telemetry spans; annotates lazy stream opts for per-batch telemetry. |
lib/event_store/streams/stream.ex |
Emits :stream_batch_read telemetry per batch during stream enumeration. |
mix.exs |
Adds explicit :telemetry dependency. |
test/telemetry_test.exs |
Adds new tests validating emitted telemetry events/metadata for several operations. |
test/storage/append_events_test.exs |
Makes timeout-related error assertion tolerant to query cancellation vs connection timeout. |
test/migrate_event_store_test.exs |
Normalizes pg_dump output by ignoring \\restrict / \\unrestrict directives. |
guides/Usage.md |
Documents telemetry event names, metadata, measurements, and handler example. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 5266518. Configure here.
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
yordis
force-pushed
the
yordis/feat-add-telemetry
branch
from
5266518 to
3ea659c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
