PC-1429: Open EPC API Error handling #403

samyou-softwire · 2024-11-25T15:29:55Z

Link to Jira ticket

Description

adds some error handling to the epc interface

404: this was confirmed to mean no recommendations
- don't log as this is reasonably expected
- default to no recommendations
500: try up to 3 times if 500 responses are returned
- log the error. if there are many 500 errors we would like to know
- default to no recommendations to allow the user to continue through the flow

Checklist

I have made any necessary updates to the documentation
I have checked there are no unnecessary IDE warnings in this PR
I have checked there are no unintentional line ending changes
I have added tests where applicable
If I have made any changes to the code, I have used the IDE auto-formatter on it
If I have made any changes to Python files (even if not changing any content strings), I have run make extract-translations

samyou-softwire · 2024-11-25T15:30:20Z

pyproject.toml

@@ -63,7 +63,7 @@ phonenumbers = "^8.13.38"

 [tool.black]
 line-length = 120
-target-version = ['py38']
+target-version = ['py311']


needed to allow for the match syntax to be parsed

Oh we should have changed this anyway. Would you mind have a quick look around the codebase for any other references to old Python versions? I can see there's one to 3.9 in the README, for example.

had a look through and I think that's the only remaining reference, so have fixed both

samyou-softwire · 2024-11-25T16:27:44Z

help_to_heat/frontdoor/interface.py

+                            # raise this to logs, though let user continue with no recommendations
+                            logger.exception(requestException)


could go either way on this behaviour, though I think logging & returning blank allows the user to proceed while giving us visibility if too many errors occur

Do you mean vs throwing the error? I think this is more correct in that case: it's something we're expecting might happen, and we're handling it here rather than replying on something else further up the chain,

samyou-softwire · 2024-11-25T16:28:15Z

help_to_heat/frontdoor/views.py

-            epc_details_response, epc_recommendations_response = interface.api.epc.get_epc(lmk)
-            epc_details = epc_details_response["rows"][0]
-            recommendations = epc_recommendations_response["rows"]
+            epc_details, recommendations = interface.api.epc.get_epc(lmk)


moved the extracting of data from response to interface, where it makes a bit more sense

jdgage · 2024-11-26T12:19:16Z

help_to_heat/frontdoor/interface.py

+                        # else if not on last try, try again
+                        continue
+
+                    # 404 means no recommendations, so no need to retry


...but we want to swallow the exception as it's expected behaviour from the API. In general I think it's worth documenting why we're doing this and what we've learned about the API.

jdgage · 2024-11-26T12:20:40Z

help_to_heat/frontdoor/interface.py

+    def _get_epc_details(self, epc_api, lmk):
+        return epc_api.get_epc_details(lmk)["rows"][0]
+
+    RECOMMENDATION_RETRIES_COUNT = 3


Would probably bump up to 5, but that's just my instinct

jdgage · 2024-11-26T12:27:32Z

help_to_heat/frontdoor/interface.py

+                            return []
+
+                        # else if not on last try, try again
+                        continue


I think we should have some exponential backoff here - there's a good article here which explains why it's a good idea. It's probably not a strict necessity at this scale, and I don't think we need to worry about jitter, but it's good practice and also gives us some more resilience against short-term problems with the API.

have added some

jdgage · 2024-11-26T12:30:21Z

help_to_heat/frontdoor/interface.py

+    RECOMMENDATION_RETRIES_COUNT = 3
+
+    def _get_epc_recommendations(self, epc_api, lmk):
+        for i in range(self.RECOMMENDATION_RETRIES_COUNT):


Not really necessary to change, but I'd instinctively have expected this sort of thing to be a while loop with an incrementing counter. Suppose it means we don't have to create this range (technically more scalable if we were doing colossal numbers of retries).

for what it's worth range in python 3 is lazy, so it'll probably be using a counter behind the scenes. I don't think it'll ever construct the range unless we call list() or similar?

Yeah I was wondering whether it ends up being equivalent once you get down to internals.

jdgage · 2024-11-26T12:31:39Z

help_to_heat/frontdoor/interface.py

+                            # raise this to logs, though let user continue with no recommendations
+                            logger.exception(requestException)


Do you mean vs throwing the error? I think this is more correct in that case: it's something we're expecting might happen, and we're handling it here rather than replying on something else further up the chain,

jdgage · 2024-11-26T19:54:28Z

pyproject.toml

@@ -63,7 +63,7 @@ phonenumbers = "^8.13.38"

 [tool.black]
 line-length = 120
-target-version = ['py38']
+target-version = ['py311']


Oh we should have changed this anyway. Would you mind have a quick look around the codebase for any other references to old Python versions? I can see there's one to 3.9 in the README, for example.

jdgage · 2024-11-26T19:55:38Z

tests/test_frontdoor.py

+
+
+@unittest.mock.patch("help_to_heat.frontdoor.interface.EPCApi", MockRecommendationsInternalServerErrorEPCApi)
+def test_on_recommendations_internal_server_error_response_recommendations_are_empty_list():


Would it be easy to have a test asserting specifically that we retry? e.g. mock an error the first time and then resolve the second?

don't think so; the EPC API classes are stateless (theyre reconstructed each time theyre used) & using some sort of global state could have issues with this state leaking out of the test

I was imagining something like

class MockRecommendationsTransientInternalServerErrorEPCApi(MockEPCApi): number_of_calls = 0 def get_epc_recommendations(self, lmk_key): number_of_calls++ if number_of_calls == 1: raise InternalServerErrorRequestException() return something normal

Would that work? We're retrying calls on the same instance.

ah yes it can store an epc api per request, have added one now

…point on 404: this means no results on 500: could be due to a timeout. try 3 more times, if fails after 3rd raise error and return no results

this allows it to parse match syntax

samyou-softwire requested review from jdgage and Glenn-Clarke November 25, 2024 15:29

samyou-softwire self-assigned this Nov 25, 2024

samyou-softwire commented Nov 25, 2024

View reviewed changes

samyou-softwire force-pushed the PC-1429-open-epc-api-error-handling branch from d91193f to 90c8d70 Compare November 25, 2024 15:31

samyou-softwire commented Nov 25, 2024

View reviewed changes

jdgage requested changes Nov 26, 2024

View reviewed changes

samyou-softwire force-pushed the PC-1429-open-epc-api-error-handling branch from 90c8d70 to b14c9f3 Compare November 26, 2024 15:20

jdgage reviewed Nov 26, 2024

View reviewed changes

PC-1429: add logic for handling response codes on recommendations end…

35d9d71

…point on 404: this means no results on 500: could be due to a timeout. try 3 more times, if fails after 3rd raise error and return no results

samyou-softwire force-pushed the PC-1429-open-epc-api-error-handling branch from b14c9f3 to b94763e Compare November 27, 2024 11:21

samyou-softwire added 3 commits November 27, 2024 12:36

PC-1429: add tests for when such responses are thrown

5ea6d68

PC-1429: bump black targetted python version to 3.11

f0d93f5

this allows it to parse match syntax

PC-1429: extract translations

68c0c2b

samyou-softwire force-pushed the PC-1429-open-epc-api-error-handling branch from b94763e to 68c0c2b Compare November 27, 2024 12:36

jdgage approved these changes Nov 27, 2024

View reviewed changes

samyou-softwire merged commit f86cdbf into develop Nov 28, 2024
4 checks passed

samyou-softwire deleted the PC-1429-open-epc-api-error-handling branch November 28, 2024 09:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PC-1429: Open EPC API Error handling #403

PC-1429: Open EPC API Error handling #403

samyou-softwire commented Nov 25, 2024

samyou-softwire Nov 25, 2024

jdgage Nov 26, 2024

samyou-softwire Nov 27, 2024

samyou-softwire Nov 25, 2024

jdgage Nov 26, 2024

samyou-softwire Nov 25, 2024

jdgage Nov 26, 2024

jdgage Nov 26, 2024

samyou-softwire Nov 26, 2024

jdgage Nov 26, 2024

samyou-softwire Nov 26, 2024

jdgage Nov 26, 2024

samyou-softwire Nov 26, 2024

jdgage Nov 26, 2024

jdgage Nov 26, 2024

jdgage Nov 26, 2024

jdgage Nov 26, 2024

samyou-softwire Nov 27, 2024

jdgage Nov 27, 2024

samyou-softwire Nov 27, 2024

		# raise this to logs, though let user continue with no recommendations
		logger.exception(requestException)



		@unittest.mock.patch("help_to_heat.frontdoor.interface.EPCApi", MockRecommendationsInternalServerErrorEPCApi)
		def test_on_recommendations_internal_server_error_response_recommendations_are_empty_list():

PC-1429: Open EPC API Error handling #403

PC-1429: Open EPC API Error handling #403

Conversation

samyou-softwire commented Nov 25, 2024

Description

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment