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

DIRTY - Do not merge - Log failed itin checks and bus notifs #263

Open
wants to merge 13 commits into
base: dev
Choose a base branch
from

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.)

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