|
| 1 | +# OpenTelemetry Integration - PR Split Plan |
| 2 | + |
| 3 | +## Overview |
| 4 | + |
| 5 | +This document outlines how to split the OpenTelemetry integration into multiple, reviewable PRs. The goal is to: |
| 6 | +1. Get a minimal working version merged first (MVP) - **each PR must deliver actual tracing functionality** |
| 7 | +2. Incrementally add features - **no PR should be just helpers/attributes without instrumentation** |
| 8 | +3. Keep each PR focused and reviewable - **each PR should create real spans that users can see** |
| 9 | + |
| 10 | +**Key Principle**: Every PR must include instrumentation that creates actual spans. Helpers and attributes are only included when they're needed by the instrumentation in the same PR. |
| 11 | + |
| 12 | +## PR Summary |
| 13 | + |
| 14 | +| PR | What It Delivers | Spans Created | Status | |
| 15 | +|----|------------------|---------------|--------| |
| 16 | +| **PR 1** | Event handler tracing | `commanded.event.handle`<br>`commanded.event.batch` | 🔴 MVP - Simpler, can work standalone | |
| 17 | +| **PR 2** | Command → Aggregate tracing | `commanded.application.dispatch`<br>`commanded.aggregate.execute` | 🟡 High priority - Completes flow | |
| 18 | +| **PR 3** | Aggregate population tracing | `commanded.aggregate.populate` | 🟢 Nice-to-have | |
| 19 | +| **PR 4** | Event store operations | `commanded.event_store.*` | 🟢 Optional (disabled by default) | |
| 20 | +| **PR 5** | Documentation | N/A | 📚 Final step | |
| 21 | + |
| 22 | +--- |
| 23 | + |
| 24 | +## PR 1: Event Handler Tracing (MVP - CRITICAL) 🔴 |
| 25 | + |
| 26 | +**Goal**: Enable event handler tracing - simpler starting point, can work standalone |
| 27 | + |
| 28 | +**Status**: Must merge first - simpler implementation, establishes foundation |
| 29 | + |
| 30 | +**Rationale**: Event handlers are simpler (only need metadata, no execution_context) and can work independently. This establishes the infrastructure (helpers, attributes, setup) that PR 2 will reuse. |
| 31 | + |
| 32 | +### Tasks Checklist |
| 33 | + |
| 34 | +#### Core Setup & Configuration (required for instrumentation) |
| 35 | +- [ ] Create `lib/commanded/opentelemetry.ex` - Main setup module with validation |
| 36 | +- [ ] Create `lib/commanded/opentelemetry/helper.ex` - Context propagation helpers (used by instrumentation) |
| 37 | +- [ ] Create `lib/commanded/opentelemetry/attributes.ex` - Attribute builders (used by instrumentation) |
| 38 | + |
| 39 | +#### Instrumentation (THE MEAT - this is what makes the PR meaningful) |
| 40 | +- [ ] Create `lib/commanded/opentelemetry/event_handler.ex` - Single + batch event handler tracing |
| 41 | + |
| 42 | +#### Test Infrastructure |
| 43 | +- [ ] Create `test/support/opentelemetry_case.ex` - Test case template |
| 44 | +- [ ] Create `test/opentelemetry/helper_test.exs` - Helper function tests |
| 45 | +- [ ] Create `test/opentelemetry/options_test.exs` - Configuration validation tests |
| 46 | +- [ ] Create `test/opentelemetry/opentelemetry_setup_test.exs` - Setup tests |
| 47 | +- [ ] Create `test/opentelemetry/event_handler_test.exs` - Event handler tests |
| 48 | + |
| 49 | +#### Dependencies & Configuration |
| 50 | +- [ ] Add `opentelemetry_telemetry` to `mix.exs` (optional dependency) |
| 51 | +- [ ] Add `opentelemetry_semantic_conventions` to `mix.exs` (optional dependency) |
| 52 | +- [ ] Add `nimble_options` to `mix.exs` (optional dependency) |
| 53 | +- [ ] Add `Commanded.OpenTelemetry` to docs extras in `mix.exs` |
| 54 | +- [ ] Add `Commanded.OpenTelemetry` to `nest_modules_by_prefix` in `mix.exs` |
| 55 | +- [ ] Add `opentelemetry_telemetry` to dialyzer plt_add_apps in `mix.exs` |
| 56 | + |
| 57 | +### What This Delivers (Real Functionality) |
| 58 | + |
| 59 | +**Primary Value:** |
| 60 | +- ✅ **`commanded.event.handle` spans** - Actual tracing of single event handling |
| 61 | +- ✅ **`commanded.event.batch` spans** - Actual tracing of batch event handling |
| 62 | +- ✅ Event handler visibility (can work standalone) |
| 63 | +- ✅ Exception tracking in event handlers |
| 64 | +- ✅ Span linking/child relationships based on `span_relationship` config |
| 65 | +- ✅ Trace context extraction from metadata (if traceparent present) |
| 66 | + |
| 67 | +**Supporting Infrastructure (only included because instrumentation needs it):** |
| 68 | +- ✅ `Commanded.OpenTelemetry.setup/0` function |
| 69 | +- ✅ Option validation with NimbleOptions |
| 70 | +- ✅ Helper functions (used by instrumentation modules) |
| 71 | +- ✅ Attribute builders (used by instrumentation modules) |
| 72 | +- ✅ Test infrastructure (validates the functionality) |
| 73 | + |
| 74 | +### Spans Created |
| 75 | + |
| 76 | +1. **`commanded.event.handle`** |
| 77 | + - Kind: `:consumer` |
| 78 | + - Attributes: event, event_id, handler_name, stream_id, etc. |
| 79 | + - Span relationship: Configurable (`:link`, `:child`, `:none`) |
| 80 | + - Created when: Single event is handled |
| 81 | + |
| 82 | +2. **`commanded.event.batch`** |
| 83 | + - Kind: `:consumer` |
| 84 | + - Attributes: batch_count, handler_name |
| 85 | + - Span relationship: Always `:link` (cannot be child of multiple commands) |
| 86 | + - Created when: Batch of events is handled |
| 87 | + |
| 88 | +### Verification Checklist |
| 89 | + |
| 90 | +- [ ] `Commanded.OpenTelemetry.setup()` can be called without errors |
| 91 | +- [ ] Options are validated correctly (tested in `options_test.exs`) |
| 92 | +- [ ] **Spans created for single event handling** (verified in `event_handler_test.exs`) |
| 93 | +- [ ] **Spans created for batch event handling** (verified in `event_handler_test.exs`) |
| 94 | +- [ ] **Span links work** (`:link` mode) - tested with traceparent in metadata |
| 95 | +- [ ] **Span children work** (`:child` mode) - tested with parent span context |
| 96 | +- [ ] **Exceptions recorded properly** (tested in event handler exception test) |
| 97 | +- [ ] All attributes present and correct |
| 98 | +- [ ] All tests pass: `mix test test/opentelemetry/` |
| 99 | +- [ ] Code compiles without warnings: `mix compile` |
| 100 | + |
| 101 | +--- |
| 102 | + |
| 103 | +## PR 2: Command Dispatch & Aggregate Execution Tracing 🟡 |
| 104 | + |
| 105 | +**Goal**: Complete end-to-end tracing with command → aggregate visibility |
| 106 | + |
| 107 | +**Status**: High priority - completes the full flow |
| 108 | + |
| 109 | +**Depends on**: PR 1 (reuses helpers, attributes, setup infrastructure) |
| 110 | + |
| 111 | +### Tasks Checklist |
| 112 | + |
| 113 | +#### Instrumentation (THE MEAT) |
| 114 | +- [ ] Create `lib/commanded/opentelemetry/application.ex` - Command dispatch tracing |
| 115 | +- [ ] Create `lib/commanded/opentelemetry/aggregate.ex` - Aggregate execution tracing |
| 116 | + - Uses helpers and attributes from PR 1 (no new infrastructure needed) |
| 117 | + |
| 118 | +#### Tests |
| 119 | +- [ ] Create `test/opentelemetry/application_test.exs` - Command dispatch tests |
| 120 | +- [ ] Create `test/opentelemetry/aggregate_test.exs` - Aggregate execution tests |
| 121 | +- [ ] Create `test/opentelemetry/trace_context_propagator_e2e_test.exs` - Full flow E2E test |
| 122 | + |
| 123 | +### What This Delivers (Real Functionality) |
| 124 | + |
| 125 | +- ✅ **`commanded.application.dispatch` spans** - Actual tracing of command dispatch |
| 126 | +- ✅ **`commanded.aggregate.execute` spans** - Actual tracing of aggregate execution |
| 127 | +- ✅ Full command → aggregate trace visibility |
| 128 | +- ✅ Exception tracking in aggregates |
| 129 | +- ✅ Parent-child span relationships (aggregate is child of dispatch) |
| 130 | +- ✅ Completes end-to-end tracing: command → aggregate → event handler (with PR 1) |
| 131 | + |
| 132 | +### Spans Created |
| 133 | + |
| 134 | +1. **`commanded.application.dispatch`** |
| 135 | + - Kind: `:producer` |
| 136 | + - Attributes: application, command, causation_id, correlation_id |
| 137 | + - Created when: Command is dispatched |
| 138 | + |
| 139 | +2. **`commanded.aggregate.execute`** |
| 140 | + - Kind: `:consumer` |
| 141 | + - Attributes: application, aggregate_uuid, aggregate_version, command |
| 142 | + - Created when: Aggregate executes command |
| 143 | + - Includes: Exception tracking, event count |
| 144 | + |
| 145 | +### Critical Feature: Completes Trace Flow |
| 146 | + |
| 147 | +This PR completes the end-to-end trace: |
| 148 | +- Command dispatch creates producer span |
| 149 | +- Aggregate execution creates child consumer span |
| 150 | +- Event handlers (from PR 1) can link to these spans via trace context |
| 151 | + |
| 152 | +### Verification Checklist |
| 153 | + |
| 154 | +- [ ] **Spans created for command dispatch** (verified in `application_test.exs`) |
| 155 | +- [ ] **Spans created for aggregate execution** (verified in `aggregate_test.exs`) |
| 156 | +- [ ] **Spans linked correctly** (aggregate is child of dispatch) |
| 157 | +- [ ] **Exceptions recorded properly** (tested in aggregate exception test) |
| 158 | +- [ ] Trace context propagation works (verified in E2E test with PR 1) |
| 159 | +- [ ] All attributes present and correct |
| 160 | +- [ ] All tests pass: `mix test test/opentelemetry/application_test.exs test/opentelemetry/aggregate_test.exs test/opentelemetry/trace_context_propagator_e2e_test.exs` |
| 161 | +- [ ] Code compiles without warnings: `mix compile` |
| 162 | + |
| 163 | +--- |
| 164 | + |
| 165 | +## PR 3: Aggregate Population Tracing 🟢 |
| 166 | + |
| 167 | +**Goal**: Trace aggregate loading from events |
| 168 | + |
| 169 | +**Status**: Nice-to-have - useful for performance analysis |
| 170 | + |
| 171 | +**Depends on**: PR 1 |
| 172 | + |
| 173 | +### Tasks Checklist |
| 174 | + |
| 175 | +#### Instrumentation |
| 176 | +- [ ] Create `lib/commanded/opentelemetry/aggregate_populate.ex` |
| 177 | + - Uses helpers and attributes from PR 1 (no new infrastructure needed) |
| 178 | + |
| 179 | +#### Tests |
| 180 | +- [ ] Add tests to `test/opentelemetry/aggregate_test.exs` OR create new test file `test/opentelemetry/aggregate_populate_test.exs` |
| 181 | + |
| 182 | +### What This Delivers (Real Functionality) |
| 183 | + |
| 184 | +- ✅ **`commanded.aggregate.populate` spans** - Actual tracing of aggregate loading |
| 185 | +- ✅ Visibility into aggregate loading performance |
| 186 | +- ✅ Event count tracking during population |
| 187 | + |
| 188 | +### Spans Created |
| 189 | + |
| 190 | +1. **`commanded.aggregate.populate`** |
| 191 | + - Kind: `:internal` |
| 192 | + - Attributes: aggregate_uuid, aggregate_version, event_count |
| 193 | + - Created when: Aggregate is loaded from events |
| 194 | + |
| 195 | +### Verification Checklist |
| 196 | + |
| 197 | +- [ ] Spans created during aggregate population (verified in tests) |
| 198 | +- [ ] Event count attribute set correctly in span |
| 199 | +- [ ] Aggregate version tracked in span attributes |
| 200 | +- [ ] All tests pass: `mix test test/opentelemetry/aggregate*test.exs` |
| 201 | +- [ ] Code compiles without warnings: `mix compile` |
| 202 | + |
| 203 | +--- |
| 204 | + |
| 205 | +## PR 4: Event Store Operations Tracing 🟢 |
| 206 | + |
| 207 | +**Goal**: Low-level event store operation visibility |
| 208 | + |
| 209 | +**Status**: Optional - disabled by default |
| 210 | + |
| 211 | +**Depends on**: PR 1 |
| 212 | + |
| 213 | +### Tasks Checklist |
| 214 | + |
| 215 | +#### Instrumentation |
| 216 | +- [ ] Create `lib/commanded/opentelemetry/event_store.ex` |
| 217 | + - Uses helpers and attributes from PR 1 (no new infrastructure needed) |
| 218 | + - Handles all event store operations: append_to_stream, stream_forward, subscribe_to, ack_event, etc. |
| 219 | + |
| 220 | +#### Tests |
| 221 | +- [ ] Create `test/opentelemetry/event_store_test.exs` |
| 222 | + |
| 223 | +### What This Delivers (Real Functionality) |
| 224 | + |
| 225 | +- ✅ **`commanded.event_store.*` spans** - Actual tracing of event store operations |
| 226 | +- ✅ Low-level debugging visibility |
| 227 | +- ✅ Performance analysis of event store operations |
| 228 | + |
| 229 | +### Spans Created |
| 230 | + |
| 231 | +Multiple spans for event store operations: |
| 232 | +- `commanded.event_store.append_to_stream` |
| 233 | +- `commanded.event_store.stream_forward` |
| 234 | +- `commanded.event_store.subscribe_to` |
| 235 | +- `commanded.event_store.ack_event` |
| 236 | +- And more... |
| 237 | + |
| 238 | +### Configuration |
| 239 | + |
| 240 | +- **Disabled by default** (`event_store: false`) |
| 241 | +- Enable with: `Commanded.OpenTelemetry.setup(tracing: [event_store: true])` |
| 242 | + |
| 243 | +### Verification Checklist |
| 244 | + |
| 245 | +- [ ] All event store operations traced (append_to_stream, stream_forward, subscribe_to, ack_event, etc.) |
| 246 | +- [ ] Disabled by default (`event_store: false` in default config) |
| 247 | +- [ ] Can be enabled via config: `Commanded.OpenTelemetry.setup(tracing: [event_store: true])` |
| 248 | +- [ ] All attributes present and correct |
| 249 | +- [ ] All tests pass: `mix test test/opentelemetry/event_store_test.exs` |
| 250 | +- [ ] Code compiles without warnings: `mix compile` |
| 251 | + |
| 252 | +--- |
| 253 | + |
| 254 | +## PR 5: Documentation 📚 |
| 255 | + |
| 256 | +**Goal**: User-facing documentation |
| 257 | + |
| 258 | +**Status**: Final step |
| 259 | + |
| 260 | +**Depends on**: PRs 1-2 (minimum), ideally PRs 1-4 |
| 261 | + |
| 262 | +### Tasks Checklist |
| 263 | + |
| 264 | +#### Documentation Files |
| 265 | +- [ ] Create/update `guides/howtos/setting-up-opentelemetry-tracing.md` - Complete user guide |
| 266 | +- [ ] Update `guides/explanations/fork-differences.md` - Add OpenTelemetry section |
| 267 | + |
| 268 | +#### Documentation Content |
| 269 | +- [ ] Write installation instructions |
| 270 | +- [ ] Write setup examples (basic and advanced) |
| 271 | +- [ ] Document all configuration options |
| 272 | +- [ ] Document span relationship modes (`:link`, `:child`, `:none`) |
| 273 | +- [ ] Write troubleshooting section |
| 274 | +- [ ] Add examples with Jaeger/OTLP exporters |
| 275 | +- [ ] Document middleware usage (`Commanded.Middleware.TraceContextPropagator`) |
| 276 | + |
| 277 | +### Verification Checklist |
| 278 | + |
| 279 | +- [ ] Complete setup guide with working examples |
| 280 | +- [ ] All configuration options documented with examples |
| 281 | +- [ ] Examples tested and verified to work |
| 282 | +- [ ] Troubleshooting section includes common issues |
| 283 | +- [ ] Documentation builds successfully: `mix docs` |
| 284 | + |
| 285 | +--- |
| 286 | + |
| 287 | +## Critical Path Summary |
| 288 | + |
| 289 | +### Minimum Viable Product (MVP) |
| 290 | + |
| 291 | +**PR 1** → **PR 2** |
| 292 | + |
| 293 | +This gives you: |
| 294 | +- ✅ Event handler tracing (PR 1 - can work standalone) |
| 295 | +- ✅ Full command → aggregate → event handler tracing (PR 1 + PR 2) |
| 296 | +- ✅ Trace context propagation |
| 297 | +- ✅ End-to-end visibility |
| 298 | + |
| 299 | +### Full Feature Set |
| 300 | + |
| 301 | +**PR 1** → **PR 2** → **PR 3** → **PR 4** → **PR 5** |
| 302 | + |
| 303 | +--- |
| 304 | + |
| 305 | +## Notes |
| 306 | + |
| 307 | +### Middleware |
| 308 | + |
| 309 | +**Decision**: Use `Commanded.Middleware.TraceContextPropagator` directly. |
| 310 | + |
| 311 | +- All documentation references `Commanded.Middleware.TraceContextPropagator` |
| 312 | +- No alias module needed - keeps the codebase simpler |
| 313 | +- The middleware already exists and works correctly |
| 314 | +- Users can add it to their routers as: `middleware Commanded.Middleware.TraceContextPropagator` |
| 315 | + |
| 316 | +### Testing Strategy |
| 317 | + |
| 318 | +- **PR 1**: Includes test infrastructure AND event handler tests (validates real spans, simpler to test) |
| 319 | +- **PR 2**: Includes command/aggregate tests and E2E validation (validates real spans, completes flow) |
| 320 | +- **PR 3+**: Can reuse test infrastructure from PR 1 (validates real spans) |
| 321 | +- Each PR must demonstrate actual functionality with passing tests |
| 322 | + |
| 323 | +### Why Event Handlers First? |
| 324 | + |
| 325 | +**Advantages:** |
| 326 | +- Simpler implementation (only needs metadata, no execution_context) |
| 327 | +- Can work standalone (event handlers process events independently) |
| 328 | +- Establishes infrastructure (helpers, attributes, setup) that PR 2 reuses |
| 329 | +- Easier to test (just need event metadata) |
| 330 | + |
| 331 | +**Trade-off:** |
| 332 | +- Event handler spans may be orphaned until PR 2 (but still useful for debugging event processing) |
| 333 | +- Full trace flow only visible after PR 2 (but PR 1 provides immediate value) |
| 334 | + |
| 335 | +### Breaking Changes |
| 336 | + |
| 337 | +- None expected - all optional dependencies |
| 338 | +- All features opt-in via `setup()` call |
| 339 | +- Defaults are sensible (event_store disabled) |
| 340 | + |
| 341 | +### Migration Notes |
| 342 | + |
| 343 | +The middleware `Commanded.Middleware.TraceContextPropagator` is used directly: |
| 344 | +- Already exists and works correctly |
| 345 | +- No migration needed - it's the same module |
| 346 | +- Simply add to router: `middleware Commanded.Middleware.TraceContextPropagator` |
| 347 | + |
0 commit comments