Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Log failed itin checks and bus notifs #263

Merged
merged 18 commits into from
Nov 5, 2024

Conversation

binh-dam-ibigroup
Copy link
Collaborator

@binh-dam-ibigroup binh-dam-ibigroup commented Oct 21, 2024

Checklist

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

Description

This PR adds logging to track data sent for trip monitoring to OTP and for bus notifications to respective services.

Copy link
Contributor

@JymDyerIBI JymDyerIBI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Good changes. I have something like the first one in my own repo (a LOG.debug() line) but I don't check those in.

@JymDyerIBI JymDyerIBI assigned br648 and unassigned JymDyerIBI Oct 21, 2024
Copy link
Contributor

@br648 br648 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it also worth adding a warning at the top of the method in case the otpResponse is null?

@@ -301,7 +302,9 @@ private boolean makeOTPRequestAndUpdateMatchingItineraryInternal() {
}

// If this point is reached, a matching itinerary was not found
LOG.warn("No comparison itinerary found in otp response for trip");
LOG.warn("No comparison itinerary found in otp response for trip - params: {}", JsonUtils.toJson(this.trip.otp2QueryParams));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit. this. is not needed.

@@ -301,7 +302,9 @@ private boolean makeOTPRequestAndUpdateMatchingItineraryInternal() {
}

// If this point is reached, a matching itinerary was not found
LOG.warn("No comparison itinerary found in otp response for trip");
LOG.warn("No comparison itinerary found in otp response for trip - params: {}", JsonUtils.toJson(this.trip.otp2QueryParams));
LOG.warn("No comparison itinerary found in otp response for trip - saved itinerary: {}", JsonUtils.toJson(this.trip.itinerary));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit. this. is not needed.

LOG.warn("No comparison itinerary found in otp response for trip");
LOG.warn("No comparison itinerary found in otp response for trip - params: {}", JsonUtils.toJson(this.trip.otp2QueryParams));
LOG.warn("No comparison itinerary found in otp response for trip - saved itinerary: {}", JsonUtils.toJson(this.trip.itinerary));
LOG.warn("No comparison itinerary found in otp response for trip - OTP itineraries: {}", JsonUtils.toJson(otpResponse.plan.itineraries));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this guard against a NPE in case plan is null?

@br648 br648 assigned binh-dam-ibigroup and unassigned br648 Oct 22, 2024
@binh-dam-ibigroup
Copy link
Collaborator Author

I am also going to add logging for when an itinerary check fails on POSTing a monitored trip.

Copy link
Contributor

@JymDyerIBI JymDyerIBI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes LGTM.

Copy link
Contributor

@JymDyerIBI JymDyerIBI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change approved.

@JymDyerIBI JymDyerIBI self-requested a review October 23, 2024 21:01
@binh-dam-ibigroup binh-dam-ibigroup changed the title Log failed itin checks and bus notifs DIRTY - Do not merge - Log failed itin checks and bus notifs Oct 23, 2024
Copy link
Contributor

@JymDyerIBI JymDyerIBI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, these make sense. (Looked it over, now will look at other PR.)

@binh-dam-ibigroup binh-dam-ibigroup changed the title DIRTY - Do not merge - Log failed itin checks and bus notifs Log failed itin checks and bus notifs Nov 4, 2024
@JymDyerIBI JymDyerIBI self-requested a review November 4, 2024 21:56
Copy link
Contributor

@JymDyerIBI JymDyerIBI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 LGTM

Copy link
Contributor

@br648 br648 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving with a few "nits"!

// (The matching itinerary will replace the original trip.itinerary.)
if (
ItineraryUtils.occursOnSameServiceDay(itineraryCandidate, otpRequest.dateTime, tripIsArriveBy) &&
ItineraryUtils.itinerariesMatch(referenceItinerary, itineraryCandidate)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit. Reduce indentation.

// If no match was found for the date, mark day of week as non-existent for the itinerary.
result.handleInvalidDate(otpRequest.dateTime);

// Log if the itinerary didn't exist "today"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit. Full stop.


// Log if the itinerary didn't exist "today"
if (index == 1) {
LOG.warn("Itinerary existence check failed 'today' for trip {} - params: {}", trip.id, JsonUtils.toJson(trip.otp2QueryParams));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit. Could this be done in one warning? If not, could Itinerary existence check failed 'today' for trip be put into a separate String and reused?

@@ -301,7 +305,9 @@ private boolean makeOTPRequestAndUpdateMatchingItineraryInternal() {
}

// If this point is reached, a matching itinerary was not found
LOG.warn("No comparison itinerary found in otp response for trip");
LOG.warn("No comparison itinerary found for trip {} - params: {}", trip.id, JsonUtils.toJson(trip.otp2QueryParams));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit. Similar comment to above.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or perhap create a method to control the logging which is called from both places.

Copy link
Collaborator Author

@binh-dam-ibigroup binh-dam-ibigroup Nov 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@br648 I extracted the common code in ccbde41, let me know if 6f0de6e looks better than what was there before.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@binh-dam-ibigroup Looks good now. Thanks.

@br648 br648 assigned binh-dam-ibigroup and unassigned br648 Nov 5, 2024
@binh-dam-ibigroup binh-dam-ibigroup merged commit b4184f8 into dev Nov 5, 2024
2 checks passed
@binh-dam-ibigroup binh-dam-ibigroup deleted the log-failed-itin-checks-and-bus-notifs branch November 5, 2024 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants