Build vehicles-for-agency tripStatus from the shared builder - #1332
Build vehicles-for-agency tripStatus from the shared builder#1332dev-aditya-hub wants to merge 2 commits into
Conversation
BuildTripStatus resolves a trip's block to compute position, distance and schedule deviation. A handler that calls it once per response row repeats that resolution for every row, and each one reloads the stop times and shape points of every trip in the block. The work scales with (rows x block size) rather than with the number of distinct blocks. GetActiveServiceIDsForDate is a smaller repeat in the same path: it is called once per row for a date that cannot change within a request. Add a request-scoped memo, carried on the context, covering loadBlockTripData (keyed on the trip-ID set), the active service calendar (keyed on the date), and stop times seeded by prefetchStopTimes. This mirrors WithSnapshotCache, which already solves the same problem one level up for whole block snapshots; these entries are still reached on the schedule-deviation path, which resolves a block without a snapshot. Handlers that do not opt in are unaffected, since every accessor falls through to the original query when the context carries no memo. Shape points are deliberately not prefetched. GetShapePointsByTripIDs joins shapes to trips, so a shape shared by many trips is returned once per trip with no deduplication, which for a large agency would materialize hundreds of megabytes of duplicated geometry for the life of a request. loadBlockTripData hands out a clone because loadShiftTrips sorts in place. A nil result there means the query failed rather than that the block is empty, so it is not cached; doing so would turn one transient error into a block-wide degradation for the rest of the request.
vehicles-for-agency built its tripStatus inline and filled only a subset of the spec'd fields. closestStop, nextStop and their offsets, scheduleDeviation, the three distance-along-trip fields, predicted and situationIds were all left at their zero values. api.BuildTripStatus already computes every one of them, and every other real-time endpoint uses it. This handler carried a reduced copy of the same logic instead. Replace the inline block with a call to the shared builder, and re-apply the three behaviours specific to this endpoint: activeTripId resolved against the request's time parameter with blockTripSequence reporting -1 when unresolvable, status/phase and the update timestamps mirrored from the enclosing vehicleStatus so the nested object cannot contradict it, and the raw GPS position restored when the builder leaves it unset. That last one is not cosmetic. BuildVehicleStatus returns early for a vehicle it considers stale, and StaleDetector compares an absolute difference, so any time parameter more than fifteen minutes from the feed timestamp makes every vehicle stale. Without the fallback the entry pairs a real top-level location with a tripStatus position of (0, 0). Extract OrientationFromGTFSBearing rather than keep a second copy of the bearing conversion. Fixes OneBusAway#1195
|
Aditya Kuchekar seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
|
Warning Review limit reached
Next review available in: 59 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
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 |
|
Code reviewFound 1 issue:
maglev/internal/restapi/vehicles_for_agency_handler.go Lines 230 to 244 in db72902 Two smaller things, not blocking, for the maintainer to rule on:
🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
aaronbrethorst
left a comment
aaronbrethorst
left a comment
There was a problem hiding this comment.
This will need some changes before merging — please see the notes below.
I did the field-by-field comparison this kind of refactor demands, and it came out clean: serviceDate still normalizes to service-day midnight, occupancyStatus is still set unconditionally from the vehicle, occupancyCapacity/occupancyCount still default to -1 through NewTripStatus(), and the status/phase mapping is unchanged. The raw-GPS fallback for stale vehicles correctly preserves the endpoint's long-standing behavior rather than quietly regressing it. Replacing fifty lines of inline construction with the shared builder while re-applying only the genuinely endpoint-specific bits is exactly the right shape for this change, and the description is unusually candid about its own trade-offs — that made reviewing it much faster.
One thing to fix:
references.stops is never populated. The handler sets Agencies, Routes, Trips, and Situations, and NewEmptyReferences() leaves Stops as []. On main that was fine because closestStop and nextStop were always empty strings — but the shared builder emits them now, so those IDs come back as dangling references. #1195, which this PR says it fixes, names that requirement directly, and all three sibling tripStatus handlers populate it.
Related and worth handling at the same time: situationIds can carry trip-scoped alert IDs that aren't in references.situations either. Rarer, since it needs an alert whose informed entity names only a trip, but it's the same class of gap.
Two notes, not blocking:
- For a stale vehicle,
tripStatus.positionis the old GPS fix whiledistanceAlongTripandscheduledDistanceAlongTripcome from the schedule snapshot atreferenceTime— so the two describe different places. That's inherent to the fallback you chose and it beats the previous behavior of reporting 0, but it's worth a comment so the next reader doesn't think it's a bug. - The
buildVehicleTripStatusdoc comment says "the two behaviours specific to this endpoint," but four are re-applied.
The allocation increase you flagged — per-vehicle shape query, per-vehicle GetTrip+GetRoute inside GetSituationIDsForTrip, per-vehicle block snapshot — is real and I'd like to see numbers on a large agency before this lands, since vehicles-for-agency is on the hot path.
Finally, sequencing: this is stacked on #1331, which I've asked for changes on (its cache has no production caller yet), so this can't land before that resolves. The CLA check is also still pending on both.
|
Thanks for your work on this. The PR for the caching mechanism that this PR depends on has been closed. The linked issue is currently blocked until the underlying scalability issue has been addressed in a different way. |
4 participants
Fixes #1195.
Why
vehicles-for-agencybuilds itstripStatusinline, and only fills a subset of the fields the spec defines. Missing today:closestStopandclosestStopTimeOffset,nextStopandnextStopTimeOffset,scheduleDeviation,scheduledDistanceAlongTrip,totalDistanceAlongTrip,distanceAlongTrip,predicted(always false) andsituationIds(always empty).All of it is already computed by
api.BuildTripStatus, which every other real-time endpoint uses: trip-details, trip-for-vehicle, trips-for-route, and both arrivals endpoints. This handler was the odd one out, carrying a reduced copy of the same logic.What this changes
The inline block is replaced by a call to the shared builder, wrapped in
buildVehicleTripStatus. Roughly fifty lines of duplicated field assignment come out.Three behaviours specific to this endpoint are re-applied on top, because changing them is outside the scope of this issue:
activeTripIdstill comes fromresolveActiveTripID, resolved against the request'stimeparameter, andblockTripSequencestill reports -1 when it cannot be resolved rather than the builder's 0. Both are pinned by existing tests.status,phaseand the two update timestamps are mirrored from the enclosingvehicleStatus, so the nested object cannot contradict the outer one within a single entry.The stale-vehicle case
Worth calling out, because I nearly shipped it as a regression.
BuildVehicleStatusreturns early for a vehicle it considers stale, leaving position at (0, 0) and orientation at 0. The old inline code set both unconditionally.StaleDetectorcompares an absolute difference, so anytimeparameter more than fifteen minutes from the feed timestamp makes every vehicle stale, not just genuinely old ones.The result was an entry carrying a real
locationat the top level next to atripStatus.positionof (0, 0), which is Null Island in the middle of the Atlantic.applyRawVehiclePositionrestores the reported position and bearing when the builder has not set them. There is a regression test for it.While in there I pulled the GTFS-bearing-to-OBA-orientation conversion out into
OrientationFromGTFSBearingrather than have a second copy of it.Discrepancies worth flagging
Two things I did not change but that a reviewer should probably rule on:
lastLocationUpdateTimesemantics differ between handlers.BuildVehicleStatusonly sets it when the vehicle has a position; this endpoint has always set it from the vehicle timestamp regardless. Adopting the shared behaviour breaksTestVehiclesForAgencyHandler_UpdateTimesPresentWhenSet. I preserved the existing behaviour rather than flip it inside this PR. If the shared semantics are the correct ones, I will file it separately.activeTripIdcan now describe a different trip than the distance fields. The override in point 1 above picks the active trip by time window, whilescheduledDistanceAlongTrip,totalDistanceAlongTripanddistanceAlongTripwere computed by the builder against its own distance-along-block choice. On an interlined block at a trip transition those two disagree, and the response reports one trip's ID with another trip's distances. This is a pre-existing tension between the endpoint's pinnedactiveTripIdbehaviour and the shared builder, not something this PR introduces, but it becomes visible here.tripStatus.positionis now shape-projected for fresh vehicles rather than raw GPS, since that is what the builder produces. Consistent with the other endpoints, but it is a change in the emitted number.Cost
Reusing the builder is more expensive per vehicle: it loads that vehicle's stop times and shape and does the projection maths, which is what computing
closestStopanddistanceAlongTripactually requires. The memoization in the prerequisite PR removes the part of that cost that was quadratic in vehicle count. What remains is linear and, as far as I can tell, irreducible without changing what the fields mean.Against main at 50 vehicles it is still roughly 15x the allocations, all of it per-vehicle work that main was simply not doing. If that trade is not acceptable for large agencies I would rather hear it now than after more of the response is built on it.
Testing
TestVehiclesForAgencyHandler_TripStatusSpecFieldsparks a vehicle at a real stop on an active trip and asserts the previously-missing fields. I confirmed it fails on main on exactly those assertions and passes with the change, so it is a real regression test rather than one written to fit.TestVehiclesForAgencyHandler_StaleVehicleKeepsPositioncovers the Null Island case above.Also adds
BenchmarkVehiclesForAgency_1VehicleandBenchmarkVehiclesForAgency_50Vehicles. The existingBenchmarkVehiclesForAgencyruns against the single-vehicle RABA fixture whose position goes stale during a long run, so it ends up measuring the empty path; these hold the clock still and take a vehicle count.go fmtclean and the full suite passes on the pure-Go path (CGO_ENABLED=0 go test -tags purego ./...).