5310 remove logging low hanging fruit #12755
Conversation
| @@ -40,14 +40,6 @@ def validate | |||
| )) | |||
| end | |||
|
|
|||
| # No domestic or military addresses should have a province but some have been coming in as a string 'null' | |||
There was a problem hiding this comment.
This is no longer happening. Pretty sure the bug is resolved.
There was a problem hiding this comment.
Yeah, this was happening very infrequently in production but we brought it to the upstream team and it was a small enough issue and required a big enough fix that it wasn't deemed worth doing. so no point in logging it anymore.
Generated by 🚫 Danger |
| @@ -35,10 +35,6 @@ def adapter | |||
|
|
|||
| def bgs_service_response | |||
| person = BGS::People::Request.new.find_person_by_participant_id(user: current_user) | |||
| if person.response.blank? | |||
There was a problem hiding this comment.
This logging actually did incidentally reveal a minor bug with id.me that sometimes results in users not having ICNs. The identity team is now aware of it.
| begin | ||
| resource = client.get_history_rxs | ||
|
|
||
| # Temporary logging for prescription bug investigation |
There was a problem hiding this comment.
This was already commented out due to performance issues.
| @@ -19,12 +19,6 @@ def parse(requests) | |||
| else | |||
| va_appointments << Templates::VAAppointment.new(request).appointment | |||
| end | |||
| rescue => e | |||
There was a problem hiding this comment.
I think these rescues were valuable when we created them because we didn't want these endpoints to be unstable due to us making adapter errors. But I don't see any instances of these errors, so I think we're safe now to remove the guardrails and let the request fail. Those errors will show in sentry which will alert us faster than logging.
| @@ -345,19 +343,23 @@ def parse_phone(phone) | |||
| # and optional extension (until the end of the string) (?:\sx(\d*))?$ | |||
| phone_captures = phone&.match(/^\(?(\d{3})\)?.?(\d{3})-?(\d{4})(?:\sx(\d*))?$/) | |||
|
|
|||
There was a problem hiding this comment.
This error was happening, but I believe it was only happening because the phone number was nil. I've changed the logic to only log if the phone number is present but phone captures are nil, which would imply that our regex capture failed. Please ensure that my logic here is correct.
| @@ -120,9 +108,7 @@ def authorized_services | |||
|
|
|||
| def direct_deposit_update_access? | |||
| user.authorize(:ppiu, :access_update?) | |||
| rescue => e | |||
There was a problem hiding this comment.
The problem with logging this is that the error is almost always a timeout or backend service error. It's not helpful information.
| @@ -32,7 +32,8 @@ def get_immunizations | |||
| # | |||
| def get_location(id) | |||
| response = perform(:get, "Location/#{id}", nil, headers) | |||
| Rails.logger.info('Mobile Lighthouse Service, Location response', response:) | |||
| # remove this logging later. it doesn't have any lasting value. | |||
| Rails.logger.info('Mobile Lighthouse Service, Location response', response_body: response.body) | |||
There was a problem hiding this comment.
I've kept this for now because I was not seeing the logging but I was seeing all of the logging around it. That puzzled me. I suspect it's because the response is too much data to log, so I'm trying to log less data. Once this is confirmed, I can remove this altogether.
| @@ -17,10 +17,6 @@ def initialize(user) | |||
| end | |||
|
|
|||
| def form_names_from_appointment_practitioners_list(practitioners_list) | |||
| unless practitioners_list.class.in?([Array, NilClass]) | |||
There was a problem hiding this comment.
This wasn't happening.
| @@ -52,8 +51,8 @@ def coordinates | |||
| Mobile::FacilitiesHelper.user_address_coordinates(@current_user) | |||
| end | |||
|
|
|||
| # this doesn't appear to be in active use, presumably because the FE doesn't send the facility id | |||
There was a problem hiding this comment.
this is for appt scheduling which didn't make the cut atm for the app so makes sense it's not being called. it will get called if scheduling does get used though
There was a problem hiding this comment.
The logging above CC providers call start is being logged, so it does seem to be in use somehow. I assume you mean the front end doesn't send facility id because that was only needed for appt scheduling.
There was a problem hiding this comment.
could that logging be coming from the backend mobile tester?
There was a problem hiding this comment.
I think you're right. I assumed I was looking at prod but all of these are for staging.
Summary
That branch includes notes on all logging in the mobile module and the PR includes categories and actions.
Related issue(s)
department-of-veterans-affairs/va-mobile-app#5310
Testing done
Specs.
Screenshots
Note: Optional
What areas of the site does it impact?
(Describe what parts of the site are impacted andifcode touched other areas)
Acceptance criteria
Requested Feedback
Please let me know if any of this logging is worth keeping or if the removal is complex enough to merit a separate PR.