Service communication exception handling #94

melnikovi · 2019-02-21T04:36:20Z

Problem

Solution

Requested Reviewers

…vices

YevSent · 2019-02-21T15:41:13Z

design-documents/service-communication-exception-handling.md

+
+#### Error code as HTTP status code or in message
+
+Some APIs put status code in the HTTP response code (introduce custom codes). I don't have strong openion on this, so I went with approach that Facebook, Twitter and Twilio use.


I think we should put response code as HTTP code and in the response. In the future, if the communication protocol changes (for ex. a message queue will be used), we won't need to change the response format.

Not sure it would work for all protocols, in RPC you always should return 200 response code.

YevSent · 2019-02-21T15:43:32Z

design-documents/service-communication-exception-handling.md

+Pros
+    * Error codes make sense regardless of technology
+Cons
+    * We would have to maintain some sort of mapping to make it work.


Now, most of the exceptions will be the same, I think it's not a big deal.

I think mapping should be per API endpoint. Anyway, I agree that it's not a big problem.

Mapping will still require additional effort for microservice deployment support from our developers and, most importantly, extension developers

YevSent · 2019-02-21T15:48:28Z

design-documents/service-communication-exception-handling.md

+
+When we consume services in BFF we could benefit from knowing that network exception happen, because we could make UI more friendly. For instance, we were not able to calculate totals on shopping cart page. We can still show shopping art and display the message that we retrying total calculation.
+
+In order to make this improvements in UI, we would have to declare new type of exception on service contracts `DataRetrievalFailedException`. This is would be backwards incompatible change for monolith.


Does this exception work only for GET operations? What about POST, PUT, DELETE? I think, for the service, the exception might be the same as now NotFoundException, CouldNotSaveException, etc. Because the service doesn't know about the network communication. In general, the service might be called via REST, a message queue, etc.

We can't replace DataRetrievalFailedException with NotFoundException because you may need to handle them differently. I will think about exceptions for POST, PUT and DELETE methods.

AlexMaxHorkun · 2019-02-22T01:25:13Z

design-documents/service-communication-exception-handling.md

+Pros
+    * No mapping needed, exceptions from API can easily be caught or thrown
+Cons
+    * If consumer or producer service has non PHP or Magento implementation PHP exception class name doesn't make a lot of sense


While exception class names wouldn't make a difference for services produced without Magento an exception class is still as good identifier of error as a number ID. But having exception class names will also make it easier for services using Magento - it's a win-win solution

paliarush · 2019-02-28T19:41:32Z

design-documents/service-communication-exception-handling.md

+</TwilioResponse>
+```
+
+### Exception class name or class name


Suggested change

### Exception class name or class name

### Exception class name or error code

paliarush · 2019-02-28T19:42:08Z

design-documents/service-communication-exception-handling.md

+
+For now, I suggest to go with option #1. If we would want to handle network communication exceptions in UI, we could do #5 or #2.
+
+### Limiting amount of services to deploy after adding new exception in our of the producers


paliarush · 2019-02-28T19:42:32Z

design-documents/service-communication-exception-handling.md

+
+Return exception class names
+Pros
+    * No mapping needed, exceptions from API can easily be caught or thrown


Lists are formatted incorrectly

paliarush · 2019-02-28T19:43:30Z

design-documents/service-communication-exception-handling.md

+
+In order to make this improvements in UI, we would have to declare new type of exception on service contracts `DataRetrievalFailedException`. This is would be backwards incompatible change for monolith.
+
+Possible options:


We can extend new exceptions from LocalizedException, which is already declared for most of the services. Then it will be backward compatible change.

We declare derived classes of LocalizedException but not LocalizedException on most of the interfaces. And I think we should use LocalizedException only in presentation layer.

paliarush · 2019-02-28T19:46:22Z

design-documents/service-communication-exception-handling.md

+
+### Exception class name or class name
+
+Existing REST API returns only error message, so we need to add exception class name or error code in addition to HTTP response code. This would allow different consumers of the service handle errors or understand what exception need to be handled or which exception type need to be thrown.


Please describe specific use cases when having existing HTTP status codes + error messages is not enough?

BTW, in current implementation codes are not implemented on purpose because we have a way to distinguish exceptions on the client based on the message template (which has placeholders instead of real variable names). So there is a way to distinguish different exception instances, however there is no way to distinguish exception classes. It is also possible to distinguish specific exceptions like Authorization, Authentication, Not Found, Internal Server error and maybe some others.

Having a mapping of message texts to exceptions is not convenient. Also the fact that messages can be used to understand what type of exception is thrown might be confusing, having error codes is common practice. I personally didn't know that you can depend on message text.

This is legacy architecture. We need to weight pros and cons of introducing alternative.

melnikovi · 2019-03-19T15:58:47Z

Will reopen after we clarify technical direction.

melnikovi added 2 commits February 20, 2019 22:30

Draft of proposal for exception handling in communication between ser…

25de574

…vices

Responce code as HTTP response code or in message

23bc935

YevSent reviewed Feb 21, 2019

View reviewed changes

buskamuza added the services isolation Design documents in scope of Services Isolation work label Feb 21, 2019

AlexMaxHorkun reviewed Feb 22, 2019

View reviewed changes

paliarush reviewed Feb 28, 2019

View reviewed changes

melnikovi added the deferred label Mar 15, 2019

melnikovi closed this Mar 19, 2019


		#### Error code as HTTP status code or in message

		Some APIs put status code in the HTTP response code (introduce custom codes). I don't have strong openion on this, so I went with approach that Facebook, Twitter and Twilio use.


		When we consume services in BFF we could benefit from knowing that network exception happen, because we could make UI more friendly. For instance, we were not able to calculate totals on shopping cart page. We can still show shopping art and display the message that we retrying total calculation.

		In order to make this improvements in UI, we would have to declare new type of exception on service contracts `DataRetrievalFailedException`. This is would be backwards incompatible change for monolith.

	### Exception class name or class name
	### Exception class name or error code


		For now, I suggest to go with option #1. If we would want to handle network communication exceptions in UI, we could do #5 or #2.

		### Limiting amount of services to deploy after adding new exception in our of the producers


		In order to make this improvements in UI, we would have to declare new type of exception on service contracts `DataRetrievalFailedException`. This is would be backwards incompatible change for monolith.

		Possible options:


		### Exception class name or class name

		Existing REST API returns only error message, so we need to add exception class name or error code in addition to HTTP response code. This would allow different consumers of the service handle errors or understand what exception need to be handled or which exception type need to be thrown.

Service communication exception handling #94

Service communication exception handling #94

Uh oh!

Conversation

melnikovi commented Feb 21, 2019

Problem

Solution

Requested Reviewers

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

melnikovi commented Mar 19, 2019

Uh oh!

Uh oh!