BUG: Missing SID for app: VETEXT in Prod

[cris-oddball]

Description

Logs show that VETEXT_SID is not found:
Missing SID for app: VETEXT

Every time a push notification is sent we check if the task definition has set the mobile app registry sids (VETEXT_SID and VA_FLAGSHIP_APP_SID). Add the VeText prod appSid to our task definitions and optimize out the work with MobileAppRegistry and MobileApp. EDIT by Mack: See comment in ticket.

Note especially that we want to initialize VETEXT_SID and VA_FLAGSHIP_APP_SID only once, not for every call.

• Ticket is understood, and QA has been contacted (if the ticket has a QA label).
• This work is added to the sprint review slide deck (key win bullet point and demo slide)

Steps to Reproduce

Unclear how to reproduce, but we can see in the logs that something is trying to use the missing SID.

1. Search Datadog Prod logs for Missing SID for app: VETEXT and observe the number of entries

Workaround

Unknown

Impact/Urgency

Unknown

Expected Behavior

• TDD
• Obtain the appSid from Vetext and populate it in the Prod task definition (see Staging task definition as a reference point) There is no Vetext mobile app for production. Our code should default to using the VA Flagship app.
• When the app is initialized, we set the VETEXT_SID without calling it for every instance it is needed.
• Missing VETEXT_SID from the task definition (environment) should not be catastrophic. (This is the case in Prod. Sending should default to the VA Flagship app.)

QA Considerations

Additional Info & Resources

[kbelikova-oddball]

Hey team! Please add your planning poker estimate with Zenhub @coreycarvalho @cris-oddball @EvanParish @k-macmillan @kalbfled @MackHalliday @mchlwellman

[MackHalliday]

Friday

• Started updating unit testing and changing code

Monday

• Test on DEV
• PR likely?

[MackHalliday]

Carrying over.

This was pulled into the sprint.
It's carrying over because I had little time to work on this Friday and, after talking to @kalbfled, decided to initialize MobileAppRegistry in the app.init.py. Unit testing not possible.

[MackHalliday]

Today

• Tried to move MobileAppRegistry into mobile_app/init.py, issues with passing in current_app.logger so MobileAppRegistry can have logging.
• Tried moving MobileAppRegistry in the app/__init__.py > create_app method. Works with the health status route and related tests. Running into issues with unit tests for push notifications. At the moment, I'm not sure if its a mocking issues or how the request module is initialized in app/__init__.py

Tomorrow

• Continue working on initializing MobileAppRegistry in app/__init__.py
• Test on DEV to confirm MobileAppRegistry is only created once.
• Get PROD value for VETEXT_SID

[MackHalliday]

Mack
11:08 AM
I could not get that MobileAppRegistery class to work within mobile_app/init.py because the logger was not accessible.
I'm trying to get the MobileAppRegistery class to initialize in the app/init.py > create_app method instead so the class would have access to logging.
It's starting to feel a little unhinged.
Does this look right?
https://github.com/department-of-veterans-affairs/notification-api/pull/2087/files (edited)
11:08
Specifically - https://github.com/department-of-veterans-affairs/notification-api/pull/2087/files#diff-9cec7b11237bc29d77a439e81c[…]c003d8e8855731eb6bc130b5a8ce602

Michael Wellman
11:10 AM
I think so, but I don't think you need line 59, just instantiate in create_app
👀
1
👍
1

Mack
11:11 AM
Note: This is causing errors in the unit tests for Push notifications that I'm currently working thru. I think it's an issue with how the client is setup for the testing (It's setup without a logger).
EDIT: I hoping this is just a unit testing issue and not an indication of a larger issue with how clients are passed to the Push Notifications feature. (edited)

David Kalbfleisch
11:12 AM
On line 59, you need the "()" to create an instance; not assign the class itself.

Thread
Mack
3 minutes ago
So I tried that - The MobileAppRegistery app, as least how I currently have it setup, needs an instance of Logger. Logger is not available until it's in the create_app method

11:14
I think I'll do what
@michael Wellman
suggested and just move the whole thing into the create_app method.

David Kalbfleisch
2 minutes ago
In that case, set it to None on line 59, and initialize it in create_app using the "globals" keyword.
👍
1

Mack
2 minutes ago
I'll try that.

[MackHalliday]

Today

• Issues with initializing MobileAppRegistry as a singleton in app/__init__.py > create_app. MobileAppRegistry needs a logger. The MobileAppRegistry instance the health check status endpoint has proper access to the logger. BUT it appears the MobileAppRegistry is either unavailable or does not have access to the current_app.logger in the PUSH notification features.
• Talked to David about issue and made plans to pair, but meeting was pushed back to focus on Kyle's PR which took most of my afternoon.
• David and I scheduled our meeting for 9:30 AM tomorrow morning.

Tomorrow

• Meet with David
• Figure out issue with PUSH notification feature and MobileAppRegistry class
• Get VEText_SID for PROD
• Submit PR?

[kalbfled]

@cris-oddball and I discussed that additional QA work is no longer necessary for this ticket after the most recent change in the objective. There is no SID for Vetext in Production.

[MackHalliday]

[MackHalliday]

@cris-oddball noticed these logs upon deploy.

I spent sometime investigating the logs, and they only appear once per deploy. As far as I can tell - the two features that use the updated MobileAppRegistry class (/_status and /v2/notifications/push) are not impacted by whatever this is. Both appear to be getting the appSIDs as expected (Example - I just sent a PUSH and the SID was included) And they are not initializing the MobileAppRegistry to do so (I check the logs for the Initializing MobileAppRegistry log I added. ).

We have decided since there appears to be no impact on functionality and the MobileAppRegistry is behaving like a Singleton as expected - We will close this ticket and make a new ticket for the unexpected logging behavior.

Slack thread on issue.