add healthchecks.io integration

[StevenMassaro]

Creating this pull request to start a discussion before continuing the implementation.

Over the past couple months, there have been a few times where features of this app have stopped working unexpectedly. Integration with healthchecks.io would allow more close monitoring of the app, particularly the fare check part (which is most useful to me).

Wondering if you have any thoughts on this work.

To-do/thoughts:

• The healthchecks calls around check in are likely not needed because notifications are sent when check in fails or succeeds.

[jdholtz]

I really like this idea. And I agree that it is important to monitor these processes under the hood as they aren’t sent out as notifications unless a major event happens.

Currently, the script uses Apprise to send notifications. It looks like healthcheck.io has integration with Apprise. One thing we can do is add a debug_notification_urls config (perhaps a better name?) to give people the opportunity to send these types of notifications to any supported platform, not just healthcheck.io.

If we do go the Apprise route, the code can be added to the lib/notification_handler.py (maybe refactored to use the send_notification function) and just called within each class sending a debug notification.

If you don’t want to do the implementation and/or documentation, I can certainly handle that as well.

[StevenMassaro]

The apprise implementation in healthchecks goes the opposite direction. In other words, healthchecks itself can use apprise to send out its (healthcheck's) notifications. I don't see any support within apprise for healthchecks integration.

Given that info, what are your thoughts on integrating now?

[jdholtz]

The apprise implementation in healthchecks goes the opposite direction

Ah, I didn't too far into that and it does seem to be intended according to caronc/apprise#139.

Given that info, what are your thoughts on integrating now?

I like this integration. Here are my thoughts on the steps to make this PR ready for production (in addition to my review comments):

1. Document the option in CONFIGURATION.md
2. Move the POST request into the NotificationHandler class so it can be called easily (passing in just the data parameter). Possibly two functions as such: healthchecks_success and healthchecks_fail
3. Write/fix tests and formatting

I can help with any of my comments or these steps as well. Just let me know. I will also handle changing the Changelog before the next release.

[StevenMassaro]

Would you mind doing numbers 1 and 3 from your to-do list?

[jdholtz]

Would you mind doing numbers 1 and 3 from your to-do list?

Yes, I can take care of that

[StevenMassaro]

@jdholtz Could you take a look at this again?

[jdholtz]

Tested it out and I like it a lot. One thing I noticed is that the healthchecks config is only applied when it is in a specific reservation config or account config. However, it is never called at _merge_globals, so the top-level option is never applied. I think that we only need one healthchecks URL as, like you said, it will just be used to ensure correct operation of the fare checker. Do you agree with this? If so, the healthchecks URL option can just be parsed in the GlobalConfig, not Config. I can also make this change if we agree, along with adding tests, documentation, and formatting if you want.

[StevenMassaro]

Tested it out and I like it a lot. One thing I noticed is that the healthchecks config is only applied when it is in a specific reservation config or account config. However, it is never called at _merge_globals, so the top-level option is never applied. I think that we only need one healthchecks URL as, like you said, it will just be used to ensure correct operation of the fare checker. Do you agree with this? If so, the healthchecks URL option can just be parsed in the GlobalConfig, not Config. I can also make this change if we agree, along with adding tests, documentation, and formatting if you want.

It sounds like you are proposing a single healthchecks URL that operates for all of the fare checker operations. Is that right? Is the fare checking done on a per-account basis, or are all of the flights combined into one bucket and checked independently of the original account? If it's the former, I don't agree with this idea, if it's the latter, I can get on board with the caveat that I do think we need individual account-level healthchecks calls after log in, like you originally proposed here.

I am seeing the light now that login failed for my account this morning (due to wrong password) and I only noticed when the healthchecks ping was late. Combining the healthchecks URLs for all of the accounts means that I would not have noticed this (because I did not get a failed log in notification, weirdly).

[StevenMassaro]

because I did not get a failed log in notification, weirdly

Here are the logs from that failed log in, I can create a separate issue if you want. (The issue would be: send a failed log in notification in this case, not fail silently, which is what currently happens):

2024-02-11 23:16:57 DEBUG Process-1[utils:32]: Successfully made request after 1 attempts
2024-02-11 23:16:57 DEBUG Process-1[fare_checker:82]: Retrieving matching flights
2024-02-11 23:16:59 DEBUG Process-1[utils:32]: Successfully made request after 1 attempts
2024-02-11 23:16:59 DEBUG Process-1[fare_checker:50]: Found 13 matching flights
2024-02-11 23:16:59 DEBUG Process-1[fare_checker:37]: Flight price change found for -1 USD
2024-02-11 23:16:59 DEBUG Process-1[reservation_monitor:170]: Lock released
2024-02-11 23:16:59 DEBUG Process-1[reservation_monitor:119]: Sleeping for 86365 seconds
2024-02-11 23:16:59 DEBUG Process-2[reservation_monitor:159]: Lock acquired
2024-02-11 23:16:59 DEBUG Process-2[reservation_monitor:181]: Retrieving reservations for account
2024-02-11 23:16:59 DEBUG Process-2[webdriver:103]: Starting webdriver for current session
2024-02-11 23:17:01 DEBUG Process-2[webdriver:119]: Using browser version: 121.0.6167.160
2024-02-11 23:17:01 DEBUG Process-2[webdriver:123]: Loading Southwest Check-In page
2024-02-11 23:17:08 DEBUG Process-2[webdriver:80]: Logging into account to get a list of reservations and valid headers
2024-02-11 23:17:10 DEBUG Process-2[webdriver:152]: Waiting for headers_set to be set
2024-02-11 23:17:10 DEBUG Process-2[webdriver:156]: headers_set set successfully
2024-02-11 23:17:11 DEBUG Process-2[webdriver:152]: Waiting for login_request_id to be set
2024-02-11 23:17:11 DEBUG Process-2[webdriver:144]: Login response has been received
2024-02-11 23:17:12 DEBUG Process-2[webdriver:156]: login_request_id set successfully
2024-02-11 23:17:12 DEBUG Process-2[webdriver:212]: Logging in failed for an unknown reason
2024-02-11 23:17:12 WARNING Process-2[reservation_monitor:190]: Encountered a Too Many Requests error while logging in. Skipping reservation retrieval
2024-02-11 23:17:12 DEBUG Process-2[reservation_monitor:170]: Lock released
2024-02-11 23:17:12 DEBUG Process-2[reservation_monitor:119]: Sleeping for 86353 seconds

[jdholtz]

If it's the former, I don't agree with this idea

Yes it is the former. Thinking through it, that makes a lot more sense to only have it on a per-account. We'll stick with this. I'll try to get the rest of the PR finished in the next couple of days.

I am seeing the light now that login failed for my account this morning (due to wrong password)...Here are the logs from that failed log in, I can create a separate issue if you want.

It looks like the invalid login is still producing a 429 error, which is most likely related to the underlying issue in #201 (although it seems that the fare check was working). I tried it and it does notify my correctly with the 'Invalid Credentials' error.

(The issue would be: send a failed log in notification in this case, not fail silently, which is what currently happens):

Currently there is not a notification sent when a 429 happens. Skipping on 429s was implemented in 6c0186c and from the description it seems to be because people were getting 429s occasionally. Would you like to see a notification there?

[StevenMassaro]

If it's the former, I don't agree with this idea

Yes it is the former. Thinking through it, that makes a lot more sense to only have it on a per-account. We'll stick with this. I'll try to get the rest of the PR finished in the next couple of days.

Okay sounds good!

I am seeing the light now that login failed for my account this morning (due to wrong password)...Here are the logs from that failed log in, I can create a separate issue if you want.

It looks like the invalid login is still producing a 429 error, which is most likely related to the underlying issue in #201 (although it seems that the fare check was working). I tried it and it does notify my correctly with the 'Invalid Credentials' error.

(The issue would be: send a failed log in notification in this case, not fail silently, which is what currently happens):

Currently there is not a notification sent when a 429 happens. Skipping on 429s was implemented in 6c0186c and from the description it seems to be because people were getting 429s occasionally. Would you like to see a notification there?

I would've expected to see a notification here. Basically I would expect a notification any time something isn't working right so I can go check in on it. This would fall under that category IMO. Maybe this behavior could be configurable?

[jdholtz]

I would've expected to see a notification here. Basically I would expect a notification any time something isn't working right so I can go check in on it. This would fall under that category IMO. Maybe this behavior could be configurable?

That makes sense. I'll add a notification before the next release. For now it'll be under the error notification level, but I can adjust it (maybe make a new level) if people want that configurable.

Going back to the Healthchecks URL, since it is an account/reservation-specific config only, it won't be able to be added as an environment variable (the feature was added in v7.2 after this PR was created). I don't think that should be an issue though as if people really want the Healthchecks URL then they can just use the config file.

[StevenMassaro]

I would've expected to see a notification here. Basically I would expect a notification any time something isn't working right so I can go check in on it. This would fall under that category IMO. Maybe this behavior could be configurable?

That makes sense. I'll add a notification before the next release. For now it'll be under the error notification level, but I can adjust it (maybe make a new level) if people want that configurable.

Going back to the Healthchecks URL, since it is an account/reservation-specific config only, it won't be able to be added as an environment variable (the feature was added in v7.2 after this PR was created). I don't think that should be an issue though as if people really want the Healthchecks URL then they can just use the config file.

That all sounds good to me. Thanks!

[jdholtz]

Thanks @StevenMassaro! This integration will be a part of the next release.

[jdholtz]

I would've expected to see a notification here. Basically I would expect a notification any time something isn't working right so I can go check in on it. This would fall under that category IMO. Maybe this behavior could be configurable?

Hey @StevenMassaro. Wanted to let you know I added a notification for this in #281. Didn't come out in the next release from this, but it's added now under the Notice notification level which is the lowest level.