airbyte-cdk: Improve Error Handling in Legacy CDK

[pnilan]

What

Part of initiative to improve error handling and debugging by providing new interfaces responsible for managing requests and error handling which include sensible error handling defaults. At this time these interfaces are not integrated into existing HttpStream and are only available for ad-hoc use for connectors.

• Closes https://github.com/airbytehq/airbyte-internal-issues/issues/7325

How

• HttpClient - Prepares and sends requests, executes follow-up actions (retrying, raising AirbyteTracedExceptions) depending on error mapping
• ErrorHandler abstract class and HttpStatusErrorHandler - enables response error/exception mapping to response actions, failure types, and error messages and error mapping lookup. Defaults to HttpStatusErrorHandler.
• ErrorResolution dataclass to encapsulate response actions, failure types, and error messages for defined exceptions or request status codes.
• BackoffStrategy abstract class and DefaultBackoffStrategy - manages max_retries, max_time, and backoff_time. Defaults to DefaultBackoffStrategy.
• ErrorMessageParser abstract class and JsonErrorMessageParser - into be instantiated in in stream and passed to HttpClient to parse error messages in responses. Defaults to JsonErrorMessageParser.

Recommended Reading Order

1. http_client.py, start w/ _init__ and then send_request
2. error_handler.py and http_status_error_handler.py
3. response_models.py
4. backoff_strategy.py and default_backoff_strategy.py
5. rate_limiting.py
6. exceptions.py
7. json_error_message_parser.py and error_message_parser.py

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment

Name	Status	Preview	Comments	Updated (UTC)
airbyte-docs	⬜️ Ignored (Inspect)	Visit Preview		May 16, 2024 6:55pm

[maxi297]

As we discussed, out-of-scope for now but interesting to keep in mind is the parsing of the response. For example:

airbyte/airbyte-integrations/connectors/source-salesforce/source_salesforce/rate_limiting.py

Lines 18 to 26 in 07db1ca

	# We've had a couple of customers with ProtocolErrors, namely:
	# * A self-managed instance during `BulkSalesforceStream.download_data`. This customer had an abnormally high number of ConnectionError
	# which seems to indicate problems with his network infrastructure in general. The exact error was: `urllib3.exceptions.ProtocolError: ('Connection broken: IncompleteRead(905 bytes read, 119 more expected)', IncompleteRead(905 bytes read, 119 more expected))`
	# * A cloud customer with very long syncs. All those syncs would end up with the following error: `urllib3.exceptions.ProtocolError: ("Connection broken: InvalidChunkLength(got length b'', 0 bytes read)", InvalidChunkLength(got length b'', 0 bytes read))`
	# Without much more information, we will make it retryable hoping that performing the same request will work.
	exceptions.ChunkedEncodingError,
	# We've had examples where the response from Salesforce was not a JSON response. Those cases where error cases though. For example:
	# https://github.com/airbytehq/airbyte-internal-issues/issues/6855. We will assume that this is an edge issue and that retry should help
	exceptions.JSONDecodeError,

@girarda @brianjlai, I also had the following question: should we remove the dependency to requests from the interfaces? Right now, HttpRequestSender. send_request returns objects that are specific to the requests library and if we change that in the CDK (I think we discussed about moving to aiohttp at some point maybe), we would have to update all the sources using the HttpRequestSender. This is a much bigger lift though hence why I would like your take on this

[brianjlai]

I also had the following question: should we remove the dependency to requests from the interfaces? Right now, HttpRequestSender. send_request returns objects that are specific to the requests library and if we change that in the CDK (I think we discussed about moving to aiohttp at some point maybe), we would have to update all the sources using the HttpRequestSender. This is a much bigger lift though hence why I would like your take on this

@maxi297 This is a good question! I do like the overall idea that the HttpClient does not bleed an implementation detail like which requester library is being used under the hood. I think our ideal set up would be to return our own Request and Response interfaces that we control instead of the library PreparedRequest/PreparedResponse.

What does concern me is as you mention the increase in lift. I think to avoid scope creep we can say that we can leave the interface as it is today and return the request/response from the requests library. A lot of our existing code like the existing HttpStream still reference prepared request/response classes too which also might add to the complexity of a migration. I feel like we may need to just punt on this in the immediate even though I agree that it's probably the ideal interface

[pnilan]

As we discussed, out-of-scope for now but interesting to keep in mind is the parsing of the response. For example:

airbyte/airbyte-integrations/connectors/source-salesforce/source_salesforce/rate_limiting.py

Lines 18 to 26 in 07db1ca

	# We've had a couple of customers with ProtocolErrors, namely:
	# * A self-managed instance during `BulkSalesforceStream.download_data`. This customer had an abnormally high number of ConnectionError
	# which seems to indicate problems with his network infrastructure in general. The exact error was: `urllib3.exceptions.ProtocolError: ('Connection broken: IncompleteRead(905 bytes read, 119 more expected)', IncompleteRead(905 bytes read, 119 more expected))`
	# * A cloud customer with very long syncs. All those syncs would end up with the following error: `urllib3.exceptions.ProtocolError: ("Connection broken: InvalidChunkLength(got length b'', 0 bytes read)", InvalidChunkLength(got length b'', 0 bytes read))`
	# Without much more information, we will make it retryable hoping that performing the same request will work.
	exceptions.ChunkedEncodingError,
	# We've had examples where the response from Salesforce was not a JSON response. Those cases where error cases though. For example:
	# https://github.com/airbytehq/airbyte-internal-issues/issues/6855. We will assume that this is an edge issue and that retry should help
	exceptions.JSONDecodeError,

@girarda @brianjlai, I also had the following question: should we remove the dependency to requests from the interfaces? Right now, HttpRequestSender. send_request returns objects that are specific to the requests library and if we change that in the CDK (I think we discussed about moving to aiohttp at some point maybe), we would have to update all the sources using the HttpRequestSender. This is a much bigger lift though hence why I would like your take on this

Re: aiohttp -- This is a valid point. I'm not going to include this in this PR but will incorporate down the line.

[maxi297]

I feel like we may need to just punt on this in the immediate even though I agree that it's probably the ideal interface

I'm fine with this, yes. We can do the interface pretty similar with the request one anyway to smooth the transition

[maxi297]

I'll confirm everything once I have time to integrate this properly in salesforce but looking at the code, I don't see any blocker. Thanks for addressing all those comments!

[brianjlai]

one last comment to discuss if you don't mind checking before ✅

[brianjlai]

amazing work patrick. this looks great and thanks for and the discussion points