|
1 | | -# ADR 0002: Function-Level Single Responsibility Refactor |
2 | | - |
3 | | -- **Status:** Proposed |
| 1 | +# ADR 0002: Function-Level SRP |
| 2 | +- **Status:** Accepted |
4 | 3 | - **Date:** 2025-10-15 |
5 | | -- **Deciders:** Platform / Runtime Tracing Team |
6 | | -- **Consulted:** Python Tooling WG, Developer Experience WG |
7 | 4 |
|
8 | 5 | ## Context |
9 | | - |
10 | | -The codetracer runtime currently exposes several high-traffic functions that blend unrelated concerns, making them difficult to understand, test, and evolve. |
11 | | - |
12 | | -- [`codetracer-python-recorder/src/session.rs:start_tracing`](../../codetracer-python-recorder/src/session.rs) performs logging setup, state guards, filesystem validation and creation, format parsing, Python metadata collection, tracer instantiation, and sys.monitoring installation within one 70+ line function. |
13 | | -- [`codetracer-python-recorder/src/runtime/mod.rs:on_py_start`](../../codetracer-python-recorder/src/runtime/mod.rs) handles activation gating, synthetic filename filtering, argument collection via unsafe PyFrame calls, error logging, and call registration in a single block. |
14 | | -- [`codetracer-python-recorder/src/runtime/mod.rs:on_line`](../../codetracer-python-recorder/src/runtime/mod.rs) interleaves activation checks, frame navigation, locals/globals materialisation, value encoding, variable registration, and memory hygiene for reference counted objects. |
15 | | -- [`codetracer-python-recorder/src/runtime/mod.rs:on_py_return`](../../codetracer-python-recorder/src/runtime/mod.rs) combines activation lifecycle management with value encoding and logging. |
16 | | -- [`codetracer-python-recorder/codetracer_python_recorder/session.py:start`](../../codetracer-python-recorder/codetracer_python_recorder/session.py) mixes backend state checks, path normalisation, format coercion, and PyO3 bridge calls. |
17 | | - |
18 | | -These hotspots violate the Single Responsibility Principle at the function level. When we add new formats, richer activation flows, or additional capture types, we risk regressions because each modification touches fragile, monolithic code blocks. |
| 6 | +`start_tracing`, `RuntimeTracer::on_py_start/on_line/on_py_return`, and Python`s `session.start` each cram validation, activation logic, frame poking, and logging into long blocks. That makes fixes risky. |
19 | 7 |
|
20 | 8 | ## Decision |
21 | | - |
22 | | -We will refactor high-traffic functions so that each public entry point coordinates narrowly-scoped helpers, each owning a single concern. |
23 | | - |
24 | | -1. **Trace session start-up:** Introduce a `TraceSessionBootstrap` (Rust) that encapsulates directory preparation, format resolution, and program metadata gathering. `start_tracing` will delegate to helpers like `ensure_trace_directory`, `resolve_trace_format`, and `collect_program_metadata`. Python-side `start` will mirror this by delegating validation to dedicated helpers (`validate_trace_path`, `coerce_format`). |
25 | | -2. **Frame inspection & activation gating:** Extract frame traversal and activation decisions into dedicated helpers inside `runtime/frame_inspector.rs` and `runtime/activation.rs`. Callback methods (`on_py_start`, `on_line`, `on_py_return`) will orchestrate the helpers instead of performing raw pointer work inline. |
26 | | -3. **Value capture pipeline:** Move argument, locals, globals, and return value capture to a `runtime::value_capture` module that exposes high-level functions such as `capture_call_arguments(frame, code)` and `record_visible_scope(writer, frame)`. These helpers will own error handling and ensure reference counting invariants, allowing callbacks to focus on control flow. |
27 | | -4. **Logging and error reporting:** Concentrate logging into small, reusable functions (e.g., `log_trace_event(event_kind, code, lineno)`) so that callbacks do not perform ad hoc logging alongside functional work. |
28 | | -5. **Activation lifecycle:** Ensure `ActivationController` remains the single owner for activation state transitions. Callbacks will query `should_process_event` and `handle_deactivation` helpers instead of duplicating checks. |
29 | | - |
30 | | -The refactor maintains public APIs but reorganises internal call graphs to keep each function focused on orchestration. |
| 9 | +Turn those hotspots into thin coordinators that call focused helpers: |
| 10 | +- Bootstrap helpers prep directories, formats, and metadata before `start_tracing` proceeds. |
| 11 | +- `frame_inspector` handles unsafe frame access; `ActivationController` owns gating. |
| 12 | +- `value_capture` records arguments, scopes, and returns. |
| 13 | +- Logging/error helpers keep messaging consistent. |
| 14 | +Python mirrors this with `_validate_trace_path`, `_coerce_format`, etc. |
| 15 | +APIs stay the same for callers. |
31 | 16 |
|
32 | 17 | ## Consequences |
| 18 | +- 👍 Smaller functions, better unit tests, clearer error handling. |
| 19 | +- 👎 More helper modules to navigate; moving unsafe code needs care. |
33 | 20 |
|
34 | | -- **Positive:** |
35 | | - - Smaller, intention-revealing functions improve readability and lower the mental load for reviewers modifying callback behaviour. |
36 | | - - Reusable helpers unlock targeted unit tests (e.g., for path validation or locals capture) without invoking the entire tracing stack. |
37 | | - - Error handling becomes consistent and auditable when concentrated in dedicated helpers. |
38 | | - - Future features (streaming writers, selective variable capture) can extend isolated helpers rather than modifying monoliths. |
39 | | -- **Negative / Risks:** |
40 | | - - Increased number of private helper modules/functions may introduce slight organisational overhead for newcomers. |
41 | | - - Extracting FFI-heavy logic requires careful lifetime management; mistakes could introduce reference leaks or double-frees. |
42 | | - - Interim refactors might temporarily duplicate logic until all call sites migrate to the new helpers. |
43 | | - |
44 | | -## Implementation Guidelines |
45 | | - |
46 | | -1. **Preserve semantics:** Validate each step with `just test` and targeted regression fixtures to ensure helper extraction does not change runtime behaviour. |
47 | | -2. **Guard unsafe code:** When moving PyFrame interactions, wrap unsafe blocks in documented helpers with clear preconditions and postconditions. |
48 | | -3. **Keep interfaces narrow:** Expose helper functions as `pub(crate)` or module-private to prevent leaking unstable APIs. |
49 | | -4. **Add focused tests:** Unit test helpers for error cases (e.g., invalid path, unsupported format, missing frame) and integrate them into existing test suites. |
50 | | -5. **Stage changes:** Land extractions in small PRs, updating the surrounding code incrementally to avoid giant rewrites. |
51 | | -6. **Document intent:** Update docstrings and module-level docs to describe helper responsibilities, keeping comments aligned with SRP boundaries. |
52 | | - |
53 | | -## Alternatives Considered |
54 | | - |
55 | | -- **Status quo:** Rejected; expanding functionality would keep bloating already-complex functions. |
56 | | -- **Entirely new tracer abstraction:** Unnecessary; existing `RuntimeTracer` shape is viable once responsibilities are modularised. |
57 | | - |
58 | | -## Follow-Up |
| 21 | +## Guidance |
| 22 | +- Extract one concern at a time, keep helpers `pub(crate)` when possible. |
| 23 | +- Wrap unsafe code in documented helpers and add unit tests for each new module. |
| 24 | +- Run `just test` + fixture comparisons after every extraction. |
59 | 25 |
|
60 | | -- Align sequencing with `design-docs/function-level-srp-refactor-plan.md`. |
61 | | -- Revisit performance benchmarks after extraction to ensure added indirection does not materially affect tracing overhead. |
| 26 | +## Follow up |
| 27 | +- Track work in the function-level SRP plan and watch performance to ensure the extra indirection stays cheap. |
0 commit comments