Conversation
WalkthroughRefactors Feeb.DB to expose a Repo-backed public API, replaces GenServer orchestration with Repo.* calls, introduces telemetry for driver and query operations, adds structured logger metadata handling, replaces Application module with a Supervisor, updates connection manager/entry, and updates tests to use the new API. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Feeb.DB
participant Repo
participant RepoManager
participant Driver
Client->>Feeb.DB: invoke query/transaction (was GenServer.call)
Feeb.DB->>Repo: Repo.begin(txn_type) / Repo.one/all/insert/...
Repo->>RepoManager: acquire or use repo PID (with metadata)
RepoManager->>Driver: open/exec/prepare/multi_step (via traced_driver_operation)
Driver-->>RepoManager: result (with timing)
RepoManager-->>Repo: repo operation result
Repo-->>Feeb.DB: formatted result / error
Feeb.DB-->>Client: return result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
renatomassaro
force-pushed
the
add-telemetry
branch
from
f8affe5 to
557976c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
test/test_helper.exs (1)
1-2: Consider adding error handling for supervisor startup.The supervisor is now manually started, but there's no error handling if
start_linkfails. Consider using pattern matching to make test failures more explicit:🔎 Proposed improvement
-Feeb.DB.Supervisor.start_link([]) - +{:ok, _pid} = Feeb.DB.Supervisor.start_link([]) +lib/feeb/db/sqlite.ex (1)
92-92: Minor: Remove unnecessary blank line.The blank line before the function definition is unnecessary.
🔎 Proposed cleanup
- defp traced_driver_operation(operation, args) do
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
lib/feeb/db.ex(12 hunks)lib/feeb/db/application.ex(1 hunks)lib/feeb/db/repo.ex(6 hunks)lib/feeb/db/repo/manager.ex(2 hunks)lib/feeb/db/repo/manager/repo_entry.ex(2 hunks)lib/feeb/db/sqlite.ex(3 hunks)mix.exs(2 hunks)test/db/repo/manager_test.exs(2 hunks)test/db/repo_test.exs(7 hunks)test/test_helper.exs(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: renatomassaro
Repo: renatomassaro/FeebDB PR: 15
File: lib/feeb/db/repo.ex:142-153
Timestamp: 2024-11-28T09:59:48.623Z
Learning: In the `Feeb.DB.Repo` module, the GenServer processes messages sequentially, so there is no race condition within `handle_call/3` functions, and `state` mutations are safe from concurrent modifications.
Learnt from: renatomassaro
Repo: renatomassaro/FeebDB PR: 22
File: priv/test/queries/test/posts.sql:4-5
Timestamp: 2025-04-26T13:15:03.540Z
Learning: Feeb.DB requires all database statements to be executed within user-initiated transactions, making additional explicit transaction wrapping unnecessary. The "delete_all" and similar operations are executed as single SQL statements which are inherently atomic even when they affect multiple rows, maintaining ACID guarantees without requiring additional transaction control.
Learnt from: renatomassaro
Repo: renatomassaro/FeebDB PR: 15
File: lib/feeb/db/repo/manager.ex:137-154
Timestamp: 2024-11-28T10:03:22.695Z
Learning: In the `Feeb.DB.Repo.Manager` module (`lib/feeb/db/repo/manager.ex`), it's acceptable to terminate caller processes using `Process.exit(caller_pid, :feebdb_repo_timeout)` in the `handle_info/2` callback when a repository timeout occurs, as the timeout is a contract and the process should die if it exceeds it.
Learnt from: renatomassaro
Repo: renatomassaro/FeebDB PR: 15
File: lib/feeb/db/repo.ex:144-145
Timestamp: 2024-11-28T10:01:57.103Z
Learning: In `lib/feeb/db/repo.ex`, when working with Erlang/Elixir code, it's idiomatic to use the same parameter name in function pattern matching to assert that parameters are equal. For example, defining `defp assert_release_signal_from_manager!(manager_pid, manager_pid), do: :ok` is acceptable and preferred.
Learnt from: renatomassaro
Repo: renatomassaro/FeebDB PR: 16
File: lib/feeb/db/schema.ex:393-401
Timestamp: 2024-12-17T00:35:18.307Z
Learning: In `Feeb.DB.Schema`, prefer to let the system crash if the schema implementation is wrong, rather than handling exceptions in `trigger_after_read_callbacks/2`, to avoid passing around potentially incorrect or invalid data.
📚 Learning: 2024-11-28T10:01:57.103Z
Learnt from: renatomassaro
Repo: renatomassaro/FeebDB PR: 15
File: lib/feeb/db/repo.ex:144-145
Timestamp: 2024-11-28T10:01:57.103Z
Learning: In `lib/feeb/db/repo.ex`, when working with Erlang/Elixir code, it's idiomatic to use the same parameter name in function pattern matching to assert that parameters are equal. For example, defining `defp assert_release_signal_from_manager!(manager_pid, manager_pid), do: :ok` is acceptable and preferred.
Applied to files:
test/db/repo/manager_test.exstest/db/repo_test.exslib/feeb/db.exlib/feeb/db/repo/manager/repo_entry.exlib/feeb/db/repo/manager.exlib/feeb/db/application.exlib/feeb/db/repo.ex
📚 Learning: 2024-11-28T10:03:22.695Z
Learnt from: renatomassaro
Repo: renatomassaro/FeebDB PR: 15
File: lib/feeb/db/repo/manager.ex:137-154
Timestamp: 2024-11-28T10:03:22.695Z
Learning: In the `Feeb.DB.Repo.Manager` module (`lib/feeb/db/repo/manager.ex`), it's acceptable to terminate caller processes using `Process.exit(caller_pid, :feebdb_repo_timeout)` in the `handle_info/2` callback when a repository timeout occurs, as the timeout is a contract and the process should die if it exceeds it.
Applied to files:
test/db/repo/manager_test.exstest/db/repo_test.exslib/feeb/db.exlib/feeb/db/repo/manager/repo_entry.exlib/feeb/db/repo/manager.exlib/feeb/db/application.exlib/feeb/db/repo.ex
📚 Learning: 2024-11-28T09:59:48.623Z
Learnt from: renatomassaro
Repo: renatomassaro/FeebDB PR: 15
File: lib/feeb/db/repo.ex:142-153
Timestamp: 2024-11-28T09:59:48.623Z
Learning: In the `Feeb.DB.Repo` module, the GenServer processes messages sequentially, so there is no race condition within `handle_call/3` functions, and `state` mutations are safe from concurrent modifications.
Applied to files:
test/db/repo/manager_test.exstest/db/repo_test.exslib/feeb/db.exlib/feeb/db/repo/manager/repo_entry.exlib/feeb/db/application.exlib/feeb/db/repo.ex
📚 Learning: 2024-11-28T10:08:23.649Z
Learnt from: renatomassaro
Repo: renatomassaro/FeebDB PR: 15
File: lib/feeb/db/repo/manager.ex:249-250
Timestamp: 2024-11-28T10:08:23.649Z
Learning: In `lib/feeb/db/repo/manager.ex`, within the `do_release_connection/2` function, the line `:ok = Repo.notify_release(pid)` is intentionally designed to match `:ok`, ensuring that if `Repo.notify_release/1` fails, a `MatchError` is raised to trigger a crash and restart of the system. This is by design to handle critical errors, and handling potential errors from `Repo.notify_release/1` is not desired.
Applied to files:
test/db/repo/manager_test.exstest/db/repo_test.exslib/feeb/db.exlib/feeb/db/repo/manager/repo_entry.exlib/feeb/db/repo/manager.exlib/feeb/db/repo.ex
📚 Learning: 2025-04-26T13:15:03.540Z
Learnt from: renatomassaro
Repo: renatomassaro/FeebDB PR: 22
File: priv/test/queries/test/posts.sql:4-5
Timestamp: 2025-04-26T13:15:03.540Z
Learning: Feeb.DB requires all database statements to be executed within user-initiated transactions, making additional explicit transaction wrapping unnecessary. The "delete_all" and similar operations are executed as single SQL statements which are inherently atomic even when they affect multiple rows, maintaining ACID guarantees without requiring additional transaction control.
Applied to files:
test/db/repo_test.exslib/feeb/db.exlib/feeb/db/repo.exlib/feeb/db/sqlite.ex
📚 Learning: 2024-12-17T00:35:18.307Z
Learnt from: renatomassaro
Repo: renatomassaro/FeebDB PR: 16
File: lib/feeb/db/schema.ex:393-401
Timestamp: 2024-12-17T00:35:18.307Z
Learning: In `Feeb.DB.Schema`, prefer to let the system crash if the schema implementation is wrong, rather than handling exceptions in `trigger_after_read_callbacks/2`, to avoid passing around potentially incorrect or invalid data.
Applied to files:
lib/feeb/db.extest/test_helper.exslib/feeb/db/repo.ex
🧬 Code graph analysis (5)
test/db/repo/manager_test.exs (2)
lib/feeb/db.ex (1)
begin(24-32)lib/feeb/db/repo.ex (1)
begin(25-33)
lib/feeb/db.ex (1)
lib/feeb/db/repo.ex (4)
begin(25-33)commit(35-37)rollback(39-43)raw(66-72)
test/test_helper.exs (4)
lib/feeb/db/application.ex (1)
start_link(7-9)lib/feeb/db/repo/manager.ex (1)
start_link(64-66)lib/feeb/db/repo/manager/supervisor.ex (1)
start_link(10-12)lib/feeb/db/repo/manager/registry.ex (1)
start_link(60-62)
lib/feeb/db/application.ex (3)
lib/feeb/db/repo/manager/supervisor.ex (2)
start_link(10-12)init(18-20)lib/feeb/db/repo/manager/registry.ex (2)
start_link(60-62)init(64-67)lib/feeb/db/repo.ex (1)
init(125-160)
lib/feeb/db/repo.ex (1)
lib/feeb/db.ex (1)
begin(24-32)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: linters
🔇 Additional comments (19)
mix.exs (2)
59-59: LGTM! Telemetry dependency added.The telemetry dependency supports the instrumentation added throughout the codebase for observability.
33-33: Themod: []configuration is correct for a library design. FeebDB is a library (not a standalone application), and libraries should not auto-start supervisors. Consuming applications are responsible for manually startingFeeb.DB.Supervisor.start_link([])in their own supervision tree, as demonstrated in the test setup.lib/feeb/db/repo/manager/repo_entry.ex (1)
3-3: LGTM! Time tracking added for observability.The addition of the
timefield to track acquisition timestamps supports telemetry and monitoring of repo lease durations. The implementation correctly captures the system time at acquisition.Also applies to: 23-23
lib/feeb/db/repo/manager.ex (2)
84-86: LGTM! Logger metadata enhancement.Setting logger metadata with context and shard_id in
init/1improves log traceability for this manager process. The subsequent log message will now include this context.
254-256: LGTM! Cleanup ordering clarified.The demonitor operation now follows timer cancellation, making the cleanup sequence more explicit. Both operations are independent, so the reordering doesn't affect correctness.
test/db/repo/manager_test.exs (2)
4-4: LGTM! Alias updated for new API.Adding
Repoto the alias list supports the migration to the Repo-backed public API.
334-334: LGTM! Migration to Repo public API.The test now uses
Repo.begin/2instead of directly calling the GenServer, aligning with the new Repo-backed transaction API introduced in this PR.test/db/repo_test.exs (1)
89-89: LGTM! Comprehensive migration to Repo public API.All test interactions have been successfully migrated from GenServer-based calls to the new Repo public API (
Repo.begin,Repo.commit,Repo.one,Repo.raw,Repo.close,Repo.notify_release). The changes are consistent throughout and align with the PR's architectural shift to Repo-backed operations.Also applies to: 93-93, 103-103, 105-105, 115-115, 117-119, 127-127, 129-129, 135-135, 138-138, 145-145, 149-152, 160-160, 170-170, 177-177, 183-183, 190-190, 193-193, 201-202, 205-206, 209-209, 218-219, 223-224, 236-237, 240-240
lib/feeb/db/sqlite.ex (1)
16-16: LGTM! Driver operations now instrumented.All SQLite driver operations are now routed through
traced_driver_operationfor telemetry instrumentation, enabling observability of driver performance.Also applies to: 18-18, 20-20, 38-38, 46-46, 86-86
lib/feeb/db/application.ex (1)
1-19: Architectural shift to manual supervisor startup looks correct.The module has been successfully converted from an OTP Application to a Supervisor, allowing manual startup instead of automatic application lifecycle management. The implementation follows standard Supervisor patterns with
start_link/1andinit/1callbacks.lib/feeb/db.ex (4)
31-31: LGTM! Transaction operations properly delegated to Repo module.The refactoring from direct GenServer calls to Repo-backed operations is clean and maintains the same semantics while adding telemetry instrumentation at the Repo layer.
Also applies to: 61-61, 72-72
82-82: LGTM! Query operations properly delegated to Repo module.The consistent refactoring pattern for read operations (raw, one, all) maintains the existing API contract while centralizing telemetry and logging at the Repo layer.
Also applies to: 111-111, 142-142
165-165: LGTM! Mutation operations properly delegated to Repo module.All write operations (insert, update, update_all, delete, delete_all) now consistently delegate to the Repo module, completing the refactoring pattern across all database operations.
Also applies to: 187-187, 197-197, 221-221, 231-231
270-270: TODO comment noted for shard-aware telemetry.The placeholder is reasonable for future enhancement. Shard-level telemetry metrics could be valuable for monitoring and debugging multi-shard deployments.
lib/feeb/db/repo.ex (5)
25-43: LGTM! Transaction API with telemetry instrumentation.The public API functions for begin, commit, and rollback properly wrap GenServer calls with telemetry and propagate logger metadata from the caller context. The with_telemetry helper ensures consistent instrumentation across all transaction operations.
45-72: LGTM! Well-designed telemetry instrumentation.The query execution pathway is well-structured:
- Public API functions consistently delegate to
run_queryhelperwith_telemetryproperly implements start/stop/exception event pattern with duration tracking- Exception handling correctly preserves stacktraces using
:erlang.raise/3- Logger metadata is propagated from caller context to GenServer handlers
The telemetry events include rich metadata (query_type, domain, query_name) that will be valuable for observability.
Also applies to: 84-121
126-126: LGTM! Process-level logger metadata initialization.Setting structured logger metadata during initialization ensures all subsequent log entries from this repo process include essential context (context, shard_id, path, mode), which significantly improves observability and log filtering capabilities.
258-362: LGTM! Proper logger metadata scope management in GenServer handlers.All transaction and query handlers correctly:
- Accept
log_metaparameter from callers- Manage metadata scope with start/end helpers
- Clean up metadata on both success and error paths
- Include appropriate debug and error logging
The scope management ensures caller-specific metadata is active during operations without polluting the process-level metadata.
521-530: LGTM! Clean logger metadata scope helpers.The metadata scope management implementation is straightforward and correct:
- Uses namespaced process dictionary key to avoid conflicts
- Properly saves/restores metadata using Logger.reset_metadata
- Simple design that handles the scope lifecycle cleanly
| @@ -1,17 +1,20 @@ | |||
| defmodule Feeb.DB.Application do | |||
| defmodule Feeb.DB.Supervisor do | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check for supervisor and application files in lib/feeb/db/
fd -e ex lib/feeb/db/ | sortRepository: renatomassaro/FeebDB
Length of output: 417
🏁 Script executed:
# List all .ex files in lib/feeb/db/ directory
fd . lib/feeb/db/ -e exRepository: renatomassaro/FeebDB
Length of output: 1064
🏁 Script executed:
# Read the application.ex file to check the module name
cat -n lib/feeb/db/application.ex | head -20Repository: renatomassaro/FeebDB
Length of output: 592
Rename file to match module name: application.ex → supervisor.ex
The module Feeb.DB.Supervisor is defined in lib/feeb/db/application.ex, violating Elixir conventions where module names must match file paths. Rename the file to lib/feeb/db/supervisor.ex.
🤖 Prompt for AI Agents
In lib/feeb/db/application.ex around line 1, the file defines module
Feeb.DB.Supervisor but its filename doesn't match the module name; rename
lib/feeb/db/application.ex to lib/feeb/db/supervisor.ex and update any
references or imports that point to the old file path (ensure the module remains
defmodule Feeb.DB.Supervisor do and update project refs, mix/compile if
necessary) so Elixir module/file conventions are satisfied.
renatomassaro
force-pushed
the
add-telemetry
branch
from
557976c to
5545307
Compare
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/feeb/db.ex (1)
141-146: EnsurereasonfromRepo.allis a valid argument forraise/1.The code calls
raise reasonon errors fromRepo.all, but Exqlite (the underlying SQLite driver) may return atoms or other error terms that are not valid forraise/1. The SQLite module even has a TODO comment noting uncertainty about error types. Wrap the error appropriately before raising—either ensure it's a string/exception struct or useraise RuntimeError, message: inspect(reason)to handle arbitrary error terms.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
lib/feeb/db.ex(12 hunks)lib/feeb/db/application.ex(0 hunks)lib/feeb/db/repo.ex(6 hunks)lib/feeb/db/repo/manager.ex(2 hunks)lib/feeb/db/repo/manager/repo_entry.ex(2 hunks)lib/feeb/db/sqlite.ex(3 hunks)lib/feeb/db/supervisor.ex(1 hunks)mix.exs(2 hunks)test/db/repo/manager_test.exs(2 hunks)test/db/repo_test.exs(7 hunks)test/test_helper.exs(1 hunks)
💤 Files with no reviewable changes (1)
- lib/feeb/db/application.ex
🚧 Files skipped from review as they are similar to previous changes (4)
- lib/feeb/db/sqlite.ex
- lib/feeb/db/repo/manager/repo_entry.ex
- lib/feeb/db/repo/manager.ex
- test/db/repo/manager_test.exs
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: renatomassaro
Repo: renatomassaro/FeebDB PR: 15
File: lib/feeb/db/repo.ex:142-153
Timestamp: 2024-11-28T09:59:48.623Z
Learning: In the `Feeb.DB.Repo` module, the GenServer processes messages sequentially, so there is no race condition within `handle_call/3` functions, and `state` mutations are safe from concurrent modifications.
Learnt from: renatomassaro
Repo: renatomassaro/FeebDB PR: 15
File: lib/feeb/db/repo.ex:144-145
Timestamp: 2024-11-28T10:01:57.103Z
Learning: In `lib/feeb/db/repo.ex`, when working with Erlang/Elixir code, it's idiomatic to use the same parameter name in function pattern matching to assert that parameters are equal. For example, defining `defp assert_release_signal_from_manager!(manager_pid, manager_pid), do: :ok` is acceptable and preferred.
Learnt from: renatomassaro
Repo: renatomassaro/FeebDB PR: 15
File: lib/feeb/db/repo/manager.ex:137-154
Timestamp: 2024-11-28T10:03:22.695Z
Learning: In the `Feeb.DB.Repo.Manager` module (`lib/feeb/db/repo/manager.ex`), it's acceptable to terminate caller processes using `Process.exit(caller_pid, :feebdb_repo_timeout)` in the `handle_info/2` callback when a repository timeout occurs, as the timeout is a contract and the process should die if it exceeds it.
Learnt from: renatomassaro
Repo: renatomassaro/FeebDB PR: 22
File: priv/test/queries/test/posts.sql:4-5
Timestamp: 2025-04-26T13:15:03.540Z
Learning: Feeb.DB requires all database statements to be executed within user-initiated transactions, making additional explicit transaction wrapping unnecessary. The "delete_all" and similar operations are executed as single SQL statements which are inherently atomic even when they affect multiple rows, maintaining ACID guarantees without requiring additional transaction control.
Learnt from: renatomassaro
Repo: renatomassaro/FeebDB PR: 16
File: lib/feeb/db/schema.ex:393-401
Timestamp: 2024-12-17T00:35:18.307Z
Learning: In `Feeb.DB.Schema`, prefer to let the system crash if the schema implementation is wrong, rather than handling exceptions in `trigger_after_read_callbacks/2`, to avoid passing around potentially incorrect or invalid data.
Learnt from: renatomassaro
Repo: renatomassaro/FeebDB PR: 15
File: lib/feeb/db/repo/manager.ex:249-250
Timestamp: 2024-11-28T10:08:23.649Z
Learning: In `lib/feeb/db/repo/manager.ex`, within the `do_release_connection/2` function, the line `:ok = Repo.notify_release(pid)` is intentionally designed to match `:ok`, ensuring that if `Repo.notify_release/1` fails, a `MatchError` is raised to trigger a crash and restart of the system. This is by design to handle critical errors, and handling potential errors from `Repo.notify_release/1` is not desired.
📚 Learning: 2024-11-28T10:01:57.103Z
Learnt from: renatomassaro
Repo: renatomassaro/FeebDB PR: 15
File: lib/feeb/db/repo.ex:144-145
Timestamp: 2024-11-28T10:01:57.103Z
Learning: In `lib/feeb/db/repo.ex`, when working with Erlang/Elixir code, it's idiomatic to use the same parameter name in function pattern matching to assert that parameters are equal. For example, defining `defp assert_release_signal_from_manager!(manager_pid, manager_pid), do: :ok` is acceptable and preferred.
Applied to files:
lib/feeb/db/supervisor.exlib/feeb/db.exlib/feeb/db/repo.extest/db/repo_test.exs
📚 Learning: 2024-11-28T10:03:22.695Z
Learnt from: renatomassaro
Repo: renatomassaro/FeebDB PR: 15
File: lib/feeb/db/repo/manager.ex:137-154
Timestamp: 2024-11-28T10:03:22.695Z
Learning: In the `Feeb.DB.Repo.Manager` module (`lib/feeb/db/repo/manager.ex`), it's acceptable to terminate caller processes using `Process.exit(caller_pid, :feebdb_repo_timeout)` in the `handle_info/2` callback when a repository timeout occurs, as the timeout is a contract and the process should die if it exceeds it.
Applied to files:
lib/feeb/db/supervisor.exlib/feeb/db.exlib/feeb/db/repo.extest/db/repo_test.exs
📚 Learning: 2024-11-28T09:59:48.623Z
Learnt from: renatomassaro
Repo: renatomassaro/FeebDB PR: 15
File: lib/feeb/db/repo.ex:142-153
Timestamp: 2024-11-28T09:59:48.623Z
Learning: In the `Feeb.DB.Repo` module, the GenServer processes messages sequentially, so there is no race condition within `handle_call/3` functions, and `state` mutations are safe from concurrent modifications.
Applied to files:
lib/feeb/db/supervisor.exlib/feeb/db.exlib/feeb/db/repo.extest/db/repo_test.exs
📚 Learning: 2024-11-28T10:08:23.649Z
Learnt from: renatomassaro
Repo: renatomassaro/FeebDB PR: 15
File: lib/feeb/db/repo/manager.ex:249-250
Timestamp: 2024-11-28T10:08:23.649Z
Learning: In `lib/feeb/db/repo/manager.ex`, within the `do_release_connection/2` function, the line `:ok = Repo.notify_release(pid)` is intentionally designed to match `:ok`, ensuring that if `Repo.notify_release/1` fails, a `MatchError` is raised to trigger a crash and restart of the system. This is by design to handle critical errors, and handling potential errors from `Repo.notify_release/1` is not desired.
Applied to files:
lib/feeb/db/supervisor.exlib/feeb/db.exlib/feeb/db/repo.extest/db/repo_test.exs
📚 Learning: 2025-04-26T13:15:03.540Z
Learnt from: renatomassaro
Repo: renatomassaro/FeebDB PR: 22
File: priv/test/queries/test/posts.sql:4-5
Timestamp: 2025-04-26T13:15:03.540Z
Learning: Feeb.DB requires all database statements to be executed within user-initiated transactions, making additional explicit transaction wrapping unnecessary. The "delete_all" and similar operations are executed as single SQL statements which are inherently atomic even when they affect multiple rows, maintaining ACID guarantees without requiring additional transaction control.
Applied to files:
lib/feeb/db.exlib/feeb/db/repo.extest/db/repo_test.exs
📚 Learning: 2024-12-17T00:35:18.307Z
Learnt from: renatomassaro
Repo: renatomassaro/FeebDB PR: 16
File: lib/feeb/db/schema.ex:393-401
Timestamp: 2024-12-17T00:35:18.307Z
Learning: In `Feeb.DB.Schema`, prefer to let the system crash if the schema implementation is wrong, rather than handling exceptions in `trigger_after_read_callbacks/2`, to avoid passing around potentially incorrect or invalid data.
Applied to files:
lib/feeb/db.extest/test_helper.exslib/feeb/db/repo.ex
🧬 Code graph analysis (5)
lib/feeb/db/supervisor.ex (2)
lib/feeb/db/repo/manager.ex (2)
start_link(64-66)init(83-99)lib/feeb/db/repo.ex (1)
init(125-160)
lib/feeb/db.ex (1)
lib/feeb/db/repo.ex (4)
begin(25-33)commit(35-37)rollback(39-43)raw(66-72)
test/test_helper.exs (4)
lib/feeb/db/repo/manager.ex (1)
start_link(64-66)lib/feeb/db/supervisor.ex (1)
start_link(7-9)lib/feeb/db/repo/manager/supervisor.ex (1)
start_link(10-12)lib/feeb/db/repo/manager/registry.ex (1)
start_link(60-62)
lib/feeb/db/repo.ex (1)
lib/feeb/db.ex (1)
begin(24-32)
test/db/repo_test.exs (2)
lib/feeb/db.ex (2)
begin(24-32)raw(81-83)lib/feeb/db/repo.ex (3)
begin(25-33)commit(35-37)raw(66-72)
🔇 Additional comments (19)
mix.exs (2)
30-35: Application module removed — verify production startup path.Setting
mod: []disables automatic supervision tree startup. This is fine for library usage where consumers startFeeb.DB.Supervisormanually, but verify this is the intended behavior for production deployments.
59-59: Telemetry dependency looks good.The
{:telemetry, "~> 1.3"}dependency aligns with the PR objective to add telemetry support.test/test_helper.exs (1)
1-2: LGTM!Correctly starts
Feeb.DB.Supervisorbefore test initialization, aligning with the removal of the automatic application startup module.test/db/repo_test.exs (6)
84-99: LGTM!The transaction lifecycle tests correctly use the new
Repo.begin/2andRepo.commit/1API, maintaining the same test coverage for transaction state management.
101-121: LGTM!Error handling tests for double-begin and commit-without-transaction scenarios properly exercise the new Repo API error paths.
123-140: LGTM!Query tests (
Repo.one) correctly verify both successful lookups and error conditions (multiple results, wrong bindings).
142-166: LGTM!Raw query tests properly exercise
Repo.raw/3for reads, writes with bindings, and implicit transaction warnings.
168-196: LGTM!Connection close tests verify both happy path and the guard against closing during an active transaction.
198-249: No action needed. Theblock_forever/0function is properly defined intest/support/utils.exand is accessible in this test file through theTest.Feeb.DBCasemodule, which importsTest.Utils.lib/feeb/db/supervisor.ex (1)
1-19: LGTM!The supervisor structure is well-organized with appropriate
one_for_onestrategy. The boot task runs as a temporary child, which is suitable for one-shot initialization.lib/feeb/db.ex (5)
31-31: LGTM!Clean migration from GenServer call to
Repo.begin/2API with telemetry support.
60-64: LGTM!
Repo.commit/1usage is correct, anddelete_env/0properly releases the connection after commit.
71-75: LGTM!Rollback follows the same pattern as commit, correctly cleaning up environment state.
81-88: LGTM!Raw query functions properly delegate to
Repo.raw/3.
269-275: LGTM!The setup logic correctly fetches or creates a manager and obtains a repo connection. The TODO comment appropriately flags future telemetry work.
lib/feeb/db/repo.ex (4)
97-121: LGTM! Solid telemetry implementation.The
with_telemetry/3implementation correctly:
- Measures duration using monotonic time
- Emits start/stop/exception events following telemetry conventions
- Preserves stacktraces using
:erlang.raise- Includes comprehensive metadata for exception events
523-530: Logger metadata scope management is sound.The implementation correctly saves and restores Logger metadata using the process dictionary. The scope helpers are used consistently in
handle_callfunctions with cleanup on both success and error paths.Note: If an exception (not
{:error, ...}) occurs betweenstart_custom_log_metadata_scopeandend_custom_log_metadata_scope, metadata won't be restored. However, this would cause a GenServer crash, resetting process state anyway. Based on learnings showing this codebase prefers crashes over defensive handling, the current approach is acceptable.
258-326: Logger metadata handling is well-designed.The handle_call modifications show consistent patterns:
- Operations that execute database commands use
start_custom_log_metadata_scopeto ensure all logs within the operation have caller context- Error-only paths efficiently pass
log_metadirectly to Logger functions- All scope cleanup happens before replying
The implementation correctly balances efficiency with proper metadata propagation.
126-126: Good addition of structured logging context.Setting Logger metadata during initialization ensures all logs from this Repo process include the context, shard_id, path, and mode. This facilitates debugging and log analysis.
1 participant
Summary by CodeRabbit
Release Notes
New Features
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.