Initial exception handling enhancements #77
Conversation
Enhance the exception handling support for code paths related to the
the initial start up of the CSP Billing Adapter.
* failures retrieving existing data from data stores that may not
exist yet are handled gracefully, without the main event loop
seeing them.
* failures creating/initializing key data stores are reported and
an exception is raised that will cause the main event loop to
fail.
* failures to retrieve usage data, which may not have been published
yet, are handled gracefully, skipping follow on metering attempt
for now, and without the main event loop seeing them.
NOTE: If this issue persists this would lead to metering not being
performed, but performing metering would overwrite the error that
was reported for the get_usage_data() operation.
Add tests to exercise and validate the new exception handling code.
Remove stale code from test_event_loop_handler() relating to log var.
Re-word the cba_pm docstring message to better reflect what it does.
Implements: #70
rtamalin
requested review from
rjschwei,
smarlowucf,
KeithMnemonic and
brett060102
| 'timestamp': date_to_string(now), | ||
| 'billing_api_access_ok': False, | ||
| 'errors': [ | ||
| f'Usage data retrieval failed: {exc}' |
There was a problem hiding this comment.
It might be helpful to track state during each cycle and any error messages and update it at the end of the loop. This way the state of the adapter is updated separately of the actions and state independent data.
There was a problem hiding this comment.
Agreed, but for such a case we may want to clarify how that is reflected in the csp_config data, as per my comments in #70.
| # worth considering. | ||
| # However, doing so will currently result in the error state | ||
| # that was saved in the csp_config being lost, as it would be | ||
| # overwritten with a success state, or a different error. |
There was a problem hiding this comment.
A number of calls inside else clause could also fall over. It might make for cleaner exception handling if any early exit of event loop is handled with a "continue". Also the rest of the loop is not necessarily tied to one function. In this case if the adapter cannot get usage data it probably shouldn't prevent the adapter from billing if it's time to bill and there's something in cache to bill. To avoid loss of state it could be tracked and only updated once at the end of loop.
There was a problem hiding this comment.
Agreed that the else clause had more candidates for exception handling improvements.
However, some refactoring will be needed to support that effort, per my comments in #70, so it may be worth doing this work in incremental steps, organically growing towards the desired end goal of more complete exception handling.
|
@smarlowucf given that this PR, while not completely addressing all of the relevant exception handling scenarios, is focused on addressing a number of such cases that occur during the initial deployment startup, I'd prefer to get these changes landed, and work on further exception handling enhancements in follow-on PRs. |
|
Github actions is having issues running the checks right now; happy to merge as is if you are ;-) |
|
Fine by me. It looked like the runs the worked passed. 👍 |
Enhance the exception handling support for code paths related to the the initial start up of the CSP Billing Adapter.
failures retrieving existing data from data stores that may not exist yet are handled gracefully, without the main event loop seeing them.
failures creating/initializing key data stores are reported and an exception is raised that will cause the main event loop to fail.
failures to retrieve usage data, which may not have been published yet, are handled gracefully, skipping follow on metering attempt for now, and without the main event loop seeing them. NOTE: If this issue persists this would lead to metering not being performed, but performing metering would overwrite the error that was reported for the get_usage_data() operation.
Add tests to exercise and validate the new exception handling code.
Remove stale code from test_event_loop_handler() relating to log var.
Re-word the cba_pm docstring message to better reflect what it does.
Implements: #70