Rename ResponseRecorder to ControllerRecorder#71
Closed
rubenvanassche wants to merge 1 commit into
Closed
Conversation
The recorder previously labelled "Response" actually wrapped the period between the controller returning and RequestHandled — semantically the after-middleware phase, not the response. Rename it to ControllerRecorder. spatie/laravel-flare's DispatchesRoutes trait now uses it to wrap the actual $dispatch() call. Wire format change: SpanType::Response (php_response) is now SpanType::Controller (php_controller). Server-side ingestion needs the matching update. Also bundles a few related fixes that surfaced while reworking this: - Tracer::gracefullyEndSpans now iterates all unclosed spans rather than only walking the parent chain. An orphaned span (e.g. a View span whose recordRendered call was skipped because the render threw) is now closed at trace end instead of blocking the trace from being sent. - SpansRecorder / SpanEventsRecorder: keep the instanceof Closure check (added in the previous commit) and reorder the null guard so phpstan can follow the type narrowing through nameAndAttributes. - Lifecycle test rewritten to reflect the new graceful behavior. - ExpectTrace::expectLaravelRequestLifecycle now also asserts the Controller and AfterMiddleware spans when present (skips them on routes that short-circuit before dispatch). - Renames the test directory and rewrites assertions for the new recorder shape.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Rename
ResponseRecordertoControllerRecorderand adjust the wire format to match. Companion to spatie/laravel-flare#41 which uses the renamed recorder to wrap$dispatch().Why
The recorder labelled
Responsewas started byDispatchesRoutes::wrapDispatcherafter$dispatch()had already returned, so semantically it covered the after-middleware time, not the response. The actual controller execution wasn't wrapped at all. Renaming it toControllerRecorderand using it to wrap$dispatch()(in laravel-flare) gives an honest controller-execution span; a separateMiddleware (after)span (already implemented byRoutingRecorder::recordAfterMiddleware*, just never called from laravel-flare) covers the time after.Changes
Spatie\FlareClient\Recorders\ResponseRecorder\ResponseRecorder→Spatie\FlareClient\Recorders\ControllerRecorder\ControllerRecorderRecorderType::Response(response) →RecorderType::Controller(controller)SpanType::Response(php_response) →SpanType::Controller(php_controller) — wire-format change, server-side ingestion needs the matching updateFlare::response()→Flare::controller()recordResponse()helper is removed; userecordStart()/recordEnd()around the controller invocationTracer::gracefullyEndSpansnow iterates all unclosed spans instead of walking only the parent chain. An orphaned span (e.g. aViewwhoserecordRenderedwas skipped because the render threw) is now closed at trace end. Old behavior was leaving such spans open and refusing to send the trace; this hit reliably onceControllerstarted getting popped by thefinallyin laravel-flare's wrapperSpansRecorder/SpanEventsRecorder: reorder the null guard so phpstan can narrow the type throughnameAndAttributes(theinstanceof Closurechange from the previous commit stays)shared/ExpectTrace::expectLaravelRequestLifecyclenow asserts theControllerandMiddleware (after)spans when present (skips them on routes that short-circuit before dispatch)UPGRADING.mdcovers the renameCaveats
gracefullyEndSpansand phpstan-related changes alongside the rename. They could be split out if you'd prefer separate PRs