Trip monitor timing refactor #75

evansiroky · 2020-09-16T06:24:22Z

Checklist

Appropriate branch selected (all PRs must first be merged to dev before they can be merged to master)
Any modified or new methods or classes have helpful JavaDoc and code is thoroughly commented
The description lists all applicable issues this PR seeks to resolve
The description lists any configuration setting(s) that differ from the default settings
All tests and CI builds passing

Description

This PR refactors the timing aspects of the trip monitor. The previous implementation was a little naive and had trouble calculating the proper time to check since it didn't use information from previous checks. This PR changes that and now will store the next upcoming itinerary for a trip and will use that check as a baseline for determining if a trip check should be skipped and how long to wait before the lead time. This results in making requests on the proper date and tracking a trip all the way until the end even if it goes longer than scheduled.

So, this PR addresses both the change in monitored trip monitoring window settings and Recalculate for current day (plan trip at the appropriate time) items in #70.

Here is a summary of changes:

Clarifies the purpose of a few variables by renaming them
- YYYY_MM_DD -> DEFAULT_DATE_FORMAT_PATTERN
- org.opentripplanner.middleware.otp.response.Response -> org.opentripplanner.middleware.otp.response.OtpResponse
Using mocked date/time for items that create Date objects
JourneyState
- now saves just the matchingItinerary instead of an otpResponse
- also saves the target date for easier OTP querying
- renames some variables to have ...millis to make it clear that the values are milliseconds
MonitoredTrip
- Constructor automatically sets the tripTime from the query params
- Adds an isInActive method for checking if the trip is never active
- Adds method getTimezoneForTargetLocation and associated helper methods to get the appropriate timezone to calculate a target time with based off of whether the trip is departing at or arriving by
- Adds convenience methods getHour and getMinute.
Itinerary/Leg
- replaces equality/hashcode check of startTime and endTime with duration instead. This is probably a temporary fix and more nuance will be needed in the future especially when dealing with realtime itineraries
CheckMonitoredTrip
- Prefixes all log outputs in the class with the trip ID. (this required a change to the logback.xml file)
- Refactors a lot of methods to not be static and instead be instance methods to avoid repetitive arguments/variables
- Refactored timing checks to rely heavily on refactored implementation of shouldSkipMonitoredTripCheck. This method now calculates itineraries if none have been calculated before in order to have a baseline to use when determining if a formal check of the trip is warranted that might result in notifications being sent out.
- Introduces new method calculateNextItinerary which advances the trip to the appropriate date and sends off a request to OTP to obtain a new baseline itinerary to compare with in future checks of the trip.
TestUtils
- Adds to the mock OTP server by allowing a series of mock responses to be added for individual test cases
CheckMonitoredTripTest
- significantly modifies the testSkipMonitoredTripCheck ParameterizedTest to check a variety of test cases that test various aspects of the shouldSkipMonitoredTripCheck. These tests sometimes will result in requests to the mock OTP server and as such mock responses are created. These tests hit a lot of the logic around generating OTP queries and therefore, the draft test of canGenerateTripPlanQueryParams was removed.

Refs #70

codecov-commenter · 2020-09-16T06:32:42Z

Codecov Report

Merging #75 into dev will increase coverage by 3.38%.
The diff coverage is 53.12%.

@@             Coverage Diff              @@
##                dev      #75      +/-   ##
============================================
+ Coverage     38.74%   42.13%   +3.38%     
- Complexity      264      297      +33     
============================================
  Files            81       81              
  Lines          2261     2307      +46     
  Branches        258      264       +6     
============================================
+ Hits            876      972      +96     
+ Misses         1301     1231      -70     
- Partials         84      104      +20

Impacted Files	Coverage Δ	Complexity Δ
...nner/middleware/controllers/api/ApiController.java	`34.45% <0.00%> (ø)`	`4.00 <0.00> (ø)`
...eware/controllers/api/MonitoredTripController.java	`19.04% <0.00%> (-9.53%)`	`2.00 <0.00> (ø)`
...iddleware/controllers/api/OtpRequestProcessor.java	`21.27% <0.00%> (ø)`	`3.00 <0.00> (ø)`
...tripplanner/middleware/otp/response/Itinerary.java	`77.41% <0.00%> (+8.66%)`	`7.00 <0.00> (+2.00)`
...g/opentripplanner/middleware/otp/response/Leg.java	`55.17% <0.00%> (+5.17%)`	`5.00 <0.00> (+1.00)`
...anner/middleware/persistence/TypedPersistence.java	`47.82% <0.00%> (ø)`	`14.00 <0.00> (ø)`
.../opentripplanner/middleware/otp/OtpDispatcher.java	`79.31% <33.33%> (+68.59%)`	`4.00 <0.00> (+3.00)`
...entripplanner/middleware/models/MonitoredTrip.java	`60.71% <53.84%> (+5.15%)`	`28.00 <7.00> (+10.00)`
...iddleware/tripMonitor/jobs/CheckMonitoredTrip.java	`41.76% <55.20%> (+16.59%)`	`19.00 <16.00> (+9.00)`
...nner/middleware/controllers/api/LogController.java	`46.29% <66.66%> (ø)`	`2.00 <0.00> (ø)`
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6971187...c9499cc. Read the comment docs.

br648 · 2020-09-16T09:14:04Z

@evansiroky the unit test "canMonitorTrip()" fails with the following exception:

java.io.FileNotFoundException: src\test\resources\org\opentripplanner\middleware\planResponse.json (The system cannot find the file specified)

This file is located in src\test\resources\org\opentripplanner\middleware\persistence.

br648

On the face of it this looks really good. I don't feel I have enough experience to comment on the finer workings so will leave that to others. Perhaps a walk through would be useful, at least for me?

src/main/java/org/opentripplanner/middleware/models/JourneyState.java

binh-dam-ibigroup

I will need to play some more with it, but see my comments so far.

src/main/java/org/opentripplanner/middleware/models/MonitoredTrip.java

binh-dam-ibigroup · 2020-09-16T14:44:54Z

src/main/java/org/opentripplanner/middleware/models/MonitoredTrip.java

@@ -279,5 +302,68 @@ public boolean delete() {
        // TODO: Add journey state deletion.
        return Persistence.monitoredTrips.removeById(this.id);
    }
+
+    public List<NameValuePair> getParsedParams() throws URISyntaxException {


I think this should return a Map, and it would simplify method isArriveBy below and the class constructor.

Should the be a @BsonIgnore annotation?

I have been trying to see if the stuff plays well with the UI. This is what I understand is happening:

~~@BsonIgnore seems to prevent reflection on the field/method on serialization, but~~ reflection on the underlying types is still attempted on deserialization.

~~@JsonIgnore seems to prevent reflection both on serialization and on deserialization. This is what we should use.~~

Never mind that didn't seem to work. So this method should probably be renamed, e.g. parseParams()

src/main/java/org/opentripplanner/middleware/tripMonitor/jobs/CheckMonitoredTrip.java

binh-dam-ibigroup · 2020-09-16T16:19:45Z

src/main/java/org/opentripplanner/middleware/tripMonitor/jobs/CheckMonitoredTrip.java

        // parse query params
        List<NameValuePair> params = URLEncodedUtils.parse(
            new URI(String.format("http://example.com/%s", trip.queryParams)),
            UTF_8
        );

-        // begin building a new list and along the way and figure out if this trip is a depart at or arrive by trip
+        // building a new list by copying all values, except for the date which is set to the target date
        List<NameValuePair> newParams = new ArrayList<>();


Echoing my comment for MonitoredTrip#getParsedParams, a map might be useful here too.

I disagree because this variable is sent to the URLEncodedUtils.format which needs some kind of Iterable.

binh-dam-ibigroup · 2020-09-16T18:25:53Z

src/main/java/org/opentripplanner/middleware/models/MonitoredTrip.java

+        List<NameValuePair> parsedParams = getParsedParams();
+        for (NameValuePair parsedParam : parsedParams) {
+            if (parsedParam.getName().equals("time")) {
+                tripTime = parsedParam.getValue();


At least of these things should also take place with this change:

Update all MonitoredTrip records in Mongo with a value for tripTime (and on UI side, make sure that field gets populated too),

If reading a MonitoredTrip record from Mongo with no tripTime value set, initialize tripTime on first use of getHour() or getMinute(), which would be more fool-proof.

binh-dam-ibigroup · 2020-09-16T19:30:04Z

src/main/java/org/opentripplanner/middleware/models/MonitoredTrip.java

+     * use the local time at the destination. Therefore, this method will return the timezone at the destination if this
+     * trip is an arriveBy trip, or the timezone at the origin if the trip is a depart at trip.
+     */
+    @BsonIgnore


Same here. @BsonIgnore and JsonIgnore are... ignored by the Mongo POJO codec on serialization. The easiest way is to rename the method (findTimezoneForTargetLocation?). If a get prefix is still needed, we can use the transient keyword (we've faced a similar issue here)

binh-dam-ibigroup

Some MonitoredTrip methods need to be modified to play well with serialization.
MonitoredTrip#tripTime may need to be initialized on first use, or the databases need to be updated, let me know what approach you'd prefer.

evansiroky · 2020-09-23T07:13:20Z

Should be ready for review again.

binh-dam-ibigroup

Sorry, there is one gap regarding the initialization of tripTime I would like to get resolved.

binh-dam-ibigroup · 2020-09-23T15:40:52Z

src/main/java/org/opentripplanner/middleware/models/MonitoredTrip.java

+        // extract trip time from parsed params
+        Map<String, String> params = parseQueryParams();
+        tripTime = params.get("time");
+        if (tripTime == null) {


To follow up on #75 (comment),
the UI does not currently initialize tripTime, so tripTime also needs to be initialized when polling tripTimeHour or tripTimeMinute. (Those methods are called by CheckMonitoredTrip#shouldSkipMonitoredTripCheck).

Ah, ok I see what's happening. I added some logic to the MonitoredTripController's preCreateHook and preUpdateHook to enforce the existence of the trip time before it gets saved to the database.

binh-dam-ibigroup · 2020-09-23T15:41:41Z

src/main/java/org/opentripplanner/middleware/models/MonitoredTrip.java

+     */
+    @BsonIgnore
+    public int tripTimeHour() {
+        return Integer.valueOf(tripTime.split(":")[0]);


There should still be a null check (at least) here and in tripTimeMinutes.

The tripTime should be guaranteed to exist due to #75 (comment).

landonreed · 2020-10-07T15:10:52Z

src/main/java/org/opentripplanner/middleware/controllers/api/LogController.java

@@ -24,7 +24,7 @@
 import static com.beerboy.ss.descriptor.EndpointDescriptor.endpointPath;
 import static com.beerboy.ss.descriptor.MethodDescriptor.path;
 import static org.opentripplanner.middleware.auth.Auth0Connection.isUserAdmin;
-import static org.opentripplanner.middleware.utils.DateTimeUtils.YYYY_MM_DD;
+import static org.opentripplanner.middleware.utils.DateTimeUtils.DEFAULT_DATE_FORMAT_PATTERN;


In my view, this variable just became harder to read with this name change.

I disagree and believe it has resulted in a more explicit meaning with intended usage conveyed through its name.

src/main/java/org/opentripplanner/middleware/models/JourneyState.java

src/main/java/org/opentripplanner/middleware/models/MonitoredTrip.java

landonreed · 2020-10-07T15:19:56Z

src/main/java/org/opentripplanner/middleware/models/MonitoredTrip.java

+    public boolean isArriveBy() throws URISyntaxException {
+        // if arriveBy is not included in query params, OTP will default to false, so initialize to false
+        return parseQueryParams().getOrDefault("arriveBy", "false").equals("true");
+    }


I find the code in parseQueryParams and isArriveBy to be a little hard to read and overly clever. I would prefer the use of one or two variables to improve legibility. Also, these could use some basic javadoc.

I also agree that the parseQueryParams is clever, but don't think it's too clever. I think breaking it up wouldn't really add much value. I went ahead and added JavaDoc though in 6fd00a9

src/main/java/org/opentripplanner/middleware/models/MonitoredTrip.java

landonreed · 2020-10-08T12:52:42Z

src/main/java/org/opentripplanner/middleware/tripMonitor/jobs/CheckMonitoredTrip.java

+     * - the current time is after the lead time before the next itinerary starts, but is over an hour until the
+     *     itinerary start time and the trip has already been checked within the last 60 minutes
+     * - the current time is after the lead time before the next itinerary starts and between 60-15 minutes prior to the
+     *     itinerary start time, but a check has occurred within the last 15 minutes


This feels a bit wordy. The language could be simplified to improve how it reads (without losing clarity). Also, I don't think this accounts for the every minute check within 30 minutes that has been added.

I updated this a little bit in 6fd00a9, but prefer to be hyper-specific when talking about timing matters.

src/main/java/org/opentripplanner/middleware/tripMonitor/jobs/CheckMonitoredTrip.java

src/main/resources/logback.xml

src/main/java/org/opentripplanner/middleware/otp/response/OtpResponse.java

landonreed

This is looking good, but I do have some concerns about how some of the CheckMonitoredTrips methods have been structured and how they update class variables (could use some touch ups to comments as well).

See #75

codecov-io · 2020-10-09T05:43:26Z

Codecov Report

Merging #75 into dev will increase coverage by 3.39%.
The diff coverage is 54.43%.

@@             Coverage Diff              @@
##                dev      #75      +/-   ##
============================================
+ Coverage     38.74%   42.13%   +3.39%     
- Complexity      264      296      +32     
============================================
  Files            81       81              
  Lines          2261     2302      +41     
  Branches        258      265       +7     
============================================
+ Hits            876      970      +94     
+ Misses         1301     1228      -73     
- Partials         84      104      +20

Impacted Files	Coverage Δ	Complexity Δ
...nner/middleware/controllers/api/ApiController.java	`34.45% <0.00%> (ø)`	`4.00 <0.00> (ø)`
...eware/controllers/api/MonitoredTripController.java	`19.04% <0.00%> (-9.53%)`	`2.00 <0.00> (ø)`
...iddleware/controllers/api/OtpRequestProcessor.java	`21.27% <0.00%> (ø)`	`3.00 <0.00> (ø)`
...tripplanner/middleware/otp/response/Itinerary.java	`77.41% <0.00%> (+8.66%)`	`7.00 <0.00> (+2.00)`
...g/opentripplanner/middleware/otp/response/Leg.java	`55.17% <0.00%> (+5.17%)`	`5.00 <0.00> (+1.00)`
...anner/middleware/persistence/TypedPersistence.java	`47.82% <0.00%> (ø)`	`14.00 <0.00> (ø)`
.../opentripplanner/middleware/otp/OtpDispatcher.java	`79.31% <33.33%> (+68.59%)`	`4.00 <0.00> (+3.00)`
...iddleware/tripMonitor/jobs/CheckMonitoredTrip.java	`41.71% <54.90%> (+16.54%)`	`19.00 <16.00> (+9.00)`
...pentripplanner/middleware/utils/DateTimeUtils.java	`63.04% <60.00%> (+11.88%)`	`12.00 <2.00> (+2.00)`
...nner/middleware/controllers/api/LogController.java	`46.29% <66.66%> (ø)`	`2.00 <0.00> (ø)`
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6971187...02379c1. Read the comment docs.

landonreed

Thanks for the changes/consideration.

evansiroky added 4 commits September 10, 2020 22:37

WIP on trip monitor timing refactor

593affd

WIP on trip monitor timing refactor

c293bdc

refactor(trip monitor): refactor trip monitor timing

38e9557

Refs #70

test(trip monitor): time-proof test case

1bbbac0

evansiroky assigned landonreed, br648 and binh-dam-ibigroup Sep 16, 2020

evansiroky mentioned this pull request Sep 16, 2020

Trip Monitoring TODO list #70

Open

8 tasks

Merge branch 'dev' into trip-monitor-timing

ce5a9ab

br648 approved these changes Sep 16, 2020

View reviewed changes

src/main/java/org/opentripplanner/middleware/models/JourneyState.java Show resolved Hide resolved

binh-dam-ibigroup reviewed Sep 16, 2020

View reviewed changes

binh-dam-ibigroup requested changes Sep 16, 2020

View reviewed changes

binh-dam-ibigroup assigned evansiroky and unassigned binh-dam-ibigroup Sep 16, 2020

br648 removed their assignment Sep 17, 2020

binh-dam-ibigroup mentioned this pull request Sep 17, 2020

Do not close trip settings screen on error saving trip. opentripplanner/otp-react-redux#232

Merged

evansiroky mentioned this pull request Sep 19, 2020

Check trip existence #77

Merged

3 tasks

binh-dam-ibigroup added a commit that referenced this pull request Sep 21, 2020

refactor(MonitoredTrip): Use timezoneForTargetLocation() from #75.

a2e1ec3

evansiroky added 3 commits September 22, 2020 21:44

Merge branch 'dev' into trip-monitor-timing

705de29

refactor: fix some broken stuff after merge

1d374d2

refactor: address PR review and fix tests

90308bb

evansiroky assigned br648 and binh-dam-ibigroup and unassigned evansiroky Sep 23, 2020

binh-dam-ibigroup requested changes Sep 23, 2020

View reviewed changes

landonreed reviewed Oct 7, 2020

View reviewed changes

src/main/java/org/opentripplanner/middleware/models/JourneyState.java Show resolved Hide resolved

landonreed reviewed Oct 7, 2020

View reviewed changes

src/main/java/org/opentripplanner/middleware/models/MonitoredTrip.java Show resolved Hide resolved

landonreed reviewed Oct 7, 2020

View reviewed changes

src/main/java/org/opentripplanner/middleware/models/MonitoredTrip.java Outdated Show resolved Hide resolved

landonreed reviewed Oct 7, 2020

View reviewed changes

src/main/java/org/opentripplanner/middleware/models/MonitoredTrip.java Outdated Show resolved Hide resolved

landonreed reviewed Oct 7, 2020

View reviewed changes

src/main/java/org/opentripplanner/middleware/models/MonitoredTrip.java Outdated Show resolved Hide resolved