Return typed errors from realtime updates, prepare for realtime statistics - #4424
Conversation
a6d26cc to
b31a2e7
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev-2.x #4424 +/- ##
===========================================
Coverage 58.22% 58.23%
- Complexity 11204 11215 +11
===========================================
Files 1480 1482 +2
Lines 59131 59249 +118
Branches 6783 6784 +1
===========================================
+ Hits 34431 34502 +71
- Misses 22644 22686 +42
- Partials 2056 2061 +5 ☔ View full report in Codecov by Sentry. |
leonardehrenfried
added
the
+Real-Time
leonardehrenfried
changed the title
0294a0e to
16e962e
Compare
optionsome
left a comment
optionsome
left a comment
There was a problem hiding this comment.
I think I misunderstood you in the developer meeting. I thought you meant you had changed the default log level of these classes from the logback config but instead you have changed the log level of some calls. I think this is ok. Only minor problem is that there is quite a lot of spam on the debug log level (at least when using an MQTT updater). Should we have these realtime classes grouped in the logback.xml file so that the log level could be easily changed by a developer?
| warn(tripId, "TripUpdate contains no updates, skipping."); | ||
| return false; | ||
| debug(tripId, "TripUpdate contains no updates, skipping."); | ||
| UpdateError.of(tripId, NO_UPDATES); |
There was a problem hiding this comment.
I think this check is duplicate of the check from createUpdatedTripTimes which returned the TOO_FEW_STOPS error. Should we remove one of these checks?
3cd8494 to
2c3247d
Compare
…apshotSource.java Co-authored-by: Hannes Junnila <hannes.junnila@gmail.com>
5dbeded to
8e24059
Compare
| } else { | ||
| // Updated trip | ||
| if (handleModifiedTrip(transitModel, fuzzyTripMatcher, feedId, journey)) { | ||
| if (handleModifiedTrip(transitModel, fuzzyTripMatcher, feedId, journey).isEmpty()) { |
There was a problem hiding this comment.
This method returns a List<UpdateError>. Would you prefer it to return a Result<Void, List<UpdateError>>?
After having thought about it for a bit I think that using a Result is correct, even though it's a slightly overcomplicated type. WDYT?
| <logger name="org.opentripplanner.model.Timetable" level="warn" /> | ||
| <logger name="org.opentripplanner.ext.siri.SiriTimetableSnapshotSource" level="warn" /> | ||
| <logger name="org.opentripplanner.ext.siri.TimetableHelper" level="warn" /> |
There was a problem hiding this comment.
Is it intentional that these have warn as default? I don't know if it's better for these classes than info but just wanted to ask if you had left them like that by accident.
|
|
||
| // Add new trip times to the buffer and return success | ||
| final boolean updated = buffer.update(pattern, updatedTripTimes, serviceDate); | ||
| var maybeError = buffer.update(pattern, updatedTripTimes, serviceDate); |
There was a problem hiding this comment.
Should we convert this into a Result?
5 participants
Summary
Right now the realtime updates return either a
falseornullto indicate an error. Instead of proper statistics OTP instead logs lots of errors which are hard to analyse.In this PR I return proper error types:
Optional<UpdateError>Result<TripTimesPatch, UpdateError>Resultis a newly introduced type that contains two result states of a computation: a success or a failure. This is very similar to an Either in functional languages.Future
This PR enables future development where you can aggregate metrics and return them in Prometheus or an HTTP endpoint.
Basically what is described in #4206.
Log improvements
Before
Lots and lots of these:
After
Unit tests
Added.
Documentation
n/a