Make requests to the CHA more resilient #718
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
https://eaflood.atlassian.net/browse/WATER-4365 As part of looking at re-implementing the SROC annual billing engine in this project we found a solution that allows us to process bills in parallel (will be added in later commits!) However, the more we do the more strain this puts on the [Charging Module API](https://github.com/DEFRA/sroc-charging-module-api) to the point we start seeing timeouts and `ECONNRESET` errors. We found with just a few tweaks on our side we could increase our tolerance of these errors. There is still a point where we are pushing it to hard. But the more throughput we can get, the faster we can complete the bill run process. So, this change does the following. - **double the default timeout we set for web requests just for the charging module.** We don't just double the timeout for all web requests. Some impact the experience of the end user and they shouldn't be made to watch a page spinning for longer than necessary - **retry `ECONNRESET` errors.** We learnt this can happen when an API is under pressure. It doesn't mean the Charging Module can't handle the next attempt so it is safe to try these again.
Cruikshanks
added
the
housekeeping
Was originally going to just double the existing timeout value. But on reflection felt we would have more control and flexibility if we kept them separate.
Having clocked that we hadn't set the `{ retry: shortBackoffLimitRetryOptions }` in our new retry tests we added them and saw run times improve. But then we realised this made needing to override the Lab test timeout redundant so we remove that here.
Also, whilst using the timeout scenarios as the template for updating the network issue ones, we realised that the timeout tests were not checking that a log of the error is record after all retries fail.
So, we've gone back and added them in.
Cruikshanks
requested review from
Demwunz,
robertparkinson,
Jozzey and
Beckyrose200
robertparkinson
approved these changes
Feb 9, 2024
Beckyrose200
approved these changes
Feb 9, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
https://eaflood.atlassian.net/browse/WATER-4365
As part of looking at re-implementing the SROC annual billing engine in this project, we found a solution that allows us to process bills in parallel (will be added in later commits!) However, the more we do the more strain this puts on the Charging Module API to the point we start seeing timeouts and
ECONNRESETerrors.With just a few tweaks on our side, we could increase our tolerance of these errors. There is still a point where we are pushing it too hard. But the more throughput we get, the faster we can complete the bill run process.
So, this change does the following.
ECONNRESETerrors. We learnt this can happen when an API is under pressure. It doesn't mean the Charging Module can't handle the next attempt so it is safe to try these again.Also a bit of housekeeping. Whilst using the timeout scenarios as the template for updating the network issue ones, we realised that the timeout tests were not checking that a log of the error is recorded after all retries fail.
So, we've gone back and added them in.