Source Zendesk Support: handle 400 error for tickets stream #23246

roman-yermilov-gl · 2023-02-20T10:10:44Z

What

Handle 400 error for Tickets stream

roman-yermilov-gl · 2023-02-20T10:24:08Z

/test connector=connectors/source-zendesk-support

🕑 connectors/source-zendesk-support https://github.com/airbytehq/airbyte/actions/runs/4222390558
✅ connectors/source-zendesk-support https://github.com/airbytehq/airbyte/actions/runs/4222390558
Python tests coverage:

Name                                 Stmts   Miss  Cover
--------------------------------------------------------
source_zendesk_support/__init__.py       2      0   100%
source_zendesk_support/streams.py      400     42    90%
source_zendesk_support/source.py        56      8    86%
--------------------------------------------------------
TOTAL                                  458     50    89%
	 Name                                                    Stmts   Miss  Cover   Missing
	 -------------------------------------------------------------------------------------
	 connector_acceptance_test/base.py                          12      4    67%   16-19
	 connector_acceptance_test/config.py                       142      5    96%   87, 93, 242, 246-247
	 connector_acceptance_test/conftest.py                     220    102    54%   37, 43-45, 50, 55, 60, 83, 89, 95-97, 116, 121-123, 129-131, 137-138, 143-144, 149, 160, 169-178, 184-189, 204, 228, 259, 265, 273-281, 289-302, 310-323, 328-334, 341-352, 359-375
	 connector_acceptance_test/plugin.py                        69     25    64%   22-23, 31, 36, 120-140, 144-148
	 connector_acceptance_test/tests/test_core.py              476    117    75%   53, 58, 97-108, 113-120, 124-125, 129-130, 380, 400, 438, 476-493, 506-517, 521-526, 532, 565-570, 608-615, 658-660, 663, 728-736, 748-751, 756, 812-813, 819, 822, 858-868, 881-906
	 connector_acceptance_test/tests/test_incremental.py       162     14    91%   58-65, 70-83, 252
	 connector_acceptance_test/utils/asserts.py                 39      2    95%   62-63
	 connector_acceptance_test/utils/common.py                  94     10    89%   16-17, 32-38, 72, 75
	 connector_acceptance_test/utils/compare.py                 62     23    63%   21-51, 68, 97-99
	 connector_acceptance_test/utils/connector_runner.py       134     33    75%   30-33, 53-54, 57-61, 64-65, 80-82, 85-87, 90-92, 95-97, 100-102, 132-133, 167-169, 216
	 connector_acceptance_test/utils/json_schema_helper.py     114     13    89%   31-32, 39, 42, 66-69, 97, 121, 203-205
	 -------------------------------------------------------------------------------------
	 TOTAL                                                    1716    348    80%

Build Passed

Test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:98: The previous and actual specifications are identical.
SKIPPED [2] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:507: The previous and actual discovered catalogs are identical.
================== 44 passed, 3 skipped in 380.76s (0:06:20) ===================

evantahler · 2023-02-20T19:09:09Z

Removing my review in favor of @erohmensing

erohmensing · 2023-02-22T17:08:19Z

@roman-yermilov-gl Am I missing something - I thought this was already added in version 2.22.2 here. Did that code get removed from master somehow? 🤔

maxi297 · 2023-02-22T17:26:41Z

airbyte-integrations/connectors/source-zendesk-support/source_zendesk_support/streams.py

+        try:
+            yield from super(Tickets, self).read_records(sync_mode, cursor_field, stream_slice, stream_state)
+        except HTTPError as e:
+            if e.response.status_code == 400 and e.response.json().get('error', '') == 'StartTimeTooRecent':


Assuming super(Tickets, self).read_records(sync_mode, cursor_field, stream_slice, stream_state) would yield [1, 2, <raise HTTPError>, 4, 5], we would not get entries 4 and 5. Is this a case that can happen here? If so, is this the expected behaviour?

Else, is this a case of ErrorHandler that would return ResponseAction.IGNORE when this specific condition is met?

Would yield from [] instead of return [] help here at all, or are we still out of the generator original yield from loop?

I definitely assumed that the third record would return [] and 4 and 5 would still be returned last time I looked at this, so thanks for raising

@maxi297 @erohmensing
Why this scenario should even happen? As I see from code the .read_records() is called per slice, not per page. It means that if a slice has wrong dates then it fails entirely on first HTTP query with those wrong dates and no pagination will happen. Could you please describe how the error can happen in the middle of per-page iteration or maybe I missing something?

I'm just wondering what should be the behaviour there. It feels wrong to me to just return [] for a slice since we don't know the records that will be provided for this slice. Will the AbstractSource consider this slice as "done" and update the stream state? If this is the case, we will miss records when the startTime won't be too recent anymore. For me, this HTTPError is an error and we should handle it as such

This case also should never happen. In normal case we should not have slices when start_date > now. But for case when state is abnormal we must return empty slice (at least acceptance tests expect us to do it)

When I run tests the stream fails with error:

ERROR root:connector_runner.py:172 Docker container failed, code 1, error: {"type": "TRACE", "trace": {"type": "ERROR", ..., "error": {"message": "StartTimeTooRecent", ...}

Results (431.74s): 43 passed 1 failed - test_incremental.py:262 TestIncremental.test_state_with_abnormally_large_values[inputs0] 3 skipped

Traceback (most recent call last): File "/usr/local/lib/python3.9/site-packages/airbyte_cdk/sources/abstract_source.py", line 120, in read yield from self._read_stream( File "/usr/local/lib/python3.9/site-packages/airbyte_cdk/sources/abstract_source.py", line 189, in _read_stream for record in record_iterator: File "/usr/local/lib/python3.9/site-packages/airbyte_cdk/sources/abstract_source.py", line 256, in _read_incremental for message_counter, record_data_or_message in enumerate(records, start=1): File "/usr/local/lib/python3.9/site-packages/airbyte_cdk/sources/streams/http/http.py", line 419, in read_records yield from self._read_pages( File "/usr/local/lib/python3.9/site-packages/airbyte_cdk/sources/streams/http/http.py", line 435, in _read_pages request, response = self._fetch_next_page(stream_slice, stream_state, next_page_token) File "/usr/local/lib/python3.9/site-packages/airbyte_cdk/sources/streams/http/http.py", line 458, in _fetch_next_page response = self._send_request(request, request_kwargs) File "/usr/local/lib/python3.9/site-packages/airbyte_cdk/sources/streams/http/http.py", line 360, in _send_request return backoff_handler(user_backoff_handler)(request, request_kwargs) File "/usr/local/lib/python3.9/site-packages/backoff/_sync.py", line 105, in retry ret = target(*args, **kwargs) File "/usr/local/lib/python3.9/site-packages/backoff/_sync.py", line 105, in retry ret = target(*args, **kwargs) File "/usr/local/lib/python3.9/site-packages/airbyte_cdk/sources/streams/http/http.py", line 327, in _send raise exc File "/usr/local/lib/python3.9/site-packages/airbyte_cdk/sources/streams/http/http.py", line 324, in _send response.raise_for_status() File "/usr/local/lib/python3.9/site-packages/requests/models.py", line 1021, in raise_for_status raise HTTPError(http_error_msg, response=self) requests.exceptions.HTTPError: 400 Client Error: Bad Request for url: https://d3v-airbyte.zendesk.com/api/v2/incremental/tickets.json?start_time=1677178666

So the only reason I made the fix is to fix error above

@maxi297
I made the fix in a different way. Increased recent time gap for Tickets stream

lazebnyi · 2023-02-22T22:38:37Z

@roman-yermilov-gl Am I missing something - I thought this was already added in version 2.22.2 here. Did that code get removed from master somehow? 🤔

@erohmensing It's look like Charles revert it - #22483 (comment)

erohmensing · 2023-02-22T23:14:39Z

@lazebnyi Got it, thanks! I'll let @maxi297 take this one as I think his question is valid

roman-yermilov-gl · 2023-02-23T17:10:29Z

@roman-yermilov-gl Am I missing something - I thought this was already added in version 2.22.2 here. Did that code get removed from master somehow? thinking

Yes, It was rolled back because of code freeze

roman-yermilov-gl · 2023-02-24T13:52:59Z

/test connector=connectors/source-zendesk-support

🕑 connectors/source-zendesk-support https://github.com/airbytehq/airbyte/actions/runs/4263061895
✅ connectors/source-zendesk-support https://github.com/airbytehq/airbyte/actions/runs/4263061895
Python tests coverage:

Name                                 Stmts   Miss  Cover
--------------------------------------------------------
source_zendesk_support/__init__.py       2      0   100%
source_zendesk_support/streams.py      394     38    90%
source_zendesk_support/source.py        56      8    86%
--------------------------------------------------------
TOTAL                                  452     46    90%

Build Passed

Test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:98: The previous and actual specifications are identical.
SKIPPED [2] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:507: The previous and actual discovered catalogs are identical.
================== 44 passed, 3 skipped in 385.21s (0:06:25) ===================

maxi297

LGTM! Just a non-blocking comment

maxi297 · 2023-02-27T13:18:41Z

airbyte-integrations/connectors/source-zendesk-support/source_zendesk_support/streams.py

@@ -544,6 +544,10 @@ class Tickets(SourceZendeskIncrementalExportStream):
    response_list_name: str = "tickets"
    transformer: TypeTransformer = TypeTransformer(TransformConfig.DefaultSchemaNormalization)

+    @staticmethod
+    def check_start_time_param(requested_start_time: int, value: int = 1):
+        return SourceZendeskIncrementalExportStream.check_start_time_param(requested_start_time, value=3)


Can we add a description why the value is 3 minutes lookback instead of 1? This will help the devs maintain this class in case there is a change made to check_start_time_param

Sure. Added description

roman-yermilov-gl · 2023-02-27T16:39:24Z

/publish connector=connectors/source-zendesk-support

🕑 Publishing the following connectors:
connectors/source-zendesk-support
https://github.com/airbytehq/airbyte/actions/runs/4284667973

Connector	Did it publish?	Were definitions generated?
connectors/source-zendesk-support	✅	✅

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️

…q#23246) * Source Zendesk Support: increase recent start time for ticket stream * Source Zendesk Support: added more comments * auto-bump connector version --------- Co-authored-by: Octavia Squidington III <octavia-squidington-iii@users.noreply.github.com>

roman-yermilov-gl requested review from evantahler, erohmensing and lazebnyi February 20, 2023 10:10

roman-yermilov-gl self-assigned this Feb 20, 2023

octavia-squidington-iii added area/connectors Connector related issues area/documentation Improvements or additions to documentation connectors/source/zendesk-support labels Feb 20, 2023

evantahler removed their request for review February 20, 2023 19:08

maxi297 reviewed Feb 22, 2023

View reviewed changes

roman-yermilov-gl requested a review from maxi297 February 23, 2023 17:09

roman-yermilov-gl force-pushed the ryermilov/source-zendesk-support-fix-tests branch from 1ad5d2c to 563de78 Compare February 24, 2023 13:47

Source Zendesk Support: increase recent start time for ticket stream

563de78

maxi297 approved these changes Feb 27, 2023

View reviewed changes

bazarnov approved these changes Feb 27, 2023

View reviewed changes

Source Zendesk Support: added more comments

d7c2686

erohmensing removed their request for review February 27, 2023 16:54

auto-bump connector version

3741868

lazebnyi approved these changes Feb 27, 2023

View reviewed changes

roman-yermilov-gl merged commit 3b7bbb7 into master Feb 27, 2023

roman-yermilov-gl deleted the ryermilov/source-zendesk-support-fix-tests branch February 27, 2023 17:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Source Zendesk Support: handle 400 error for tickets stream #23246

Source Zendesk Support: handle 400 error for tickets stream #23246

roman-yermilov-gl commented Feb 20, 2023

roman-yermilov-gl commented Feb 20, 2023 •

edited by github-actions bot

Loading

evantahler commented Feb 20, 2023

erohmensing commented Feb 22, 2023 •

edited

Loading

maxi297 Feb 22, 2023

erohmensing Feb 22, 2023

erohmensing Feb 22, 2023

roman-yermilov-gl Feb 23, 2023

maxi297 Feb 23, 2023

roman-yermilov-gl Feb 23, 2023

roman-yermilov-gl Feb 23, 2023

roman-yermilov-gl Feb 24, 2023 •

edited

Loading

lazebnyi commented Feb 22, 2023

erohmensing commented Feb 22, 2023

roman-yermilov-gl commented Feb 23, 2023

roman-yermilov-gl commented Feb 24, 2023 •

edited by github-actions bot

Loading

maxi297 left a comment

maxi297 Feb 27, 2023

roman-yermilov-gl Feb 27, 2023

roman-yermilov-gl commented Feb 27, 2023 •

edited by github-actions bot

Loading

Source Zendesk Support: handle 400 error for tickets stream #23246

Source Zendesk Support: handle 400 error for tickets stream #23246

Conversation

roman-yermilov-gl commented Feb 20, 2023

What

roman-yermilov-gl commented Feb 20, 2023 • edited by github-actions bot Loading

Build Passed

evantahler commented Feb 20, 2023

erohmensing commented Feb 22, 2023 • edited Loading

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

roman-yermilov-gl Feb 24, 2023 • edited Loading

Choose a reason for hiding this comment

lazebnyi commented Feb 22, 2023

erohmensing commented Feb 22, 2023

roman-yermilov-gl commented Feb 23, 2023

roman-yermilov-gl commented Feb 24, 2023 • edited by github-actions bot Loading

Build Passed

maxi297 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roman-yermilov-gl commented Feb 27, 2023 • edited by github-actions bot Loading

roman-yermilov-gl commented Feb 20, 2023 •

edited by github-actions bot

Loading

erohmensing commented Feb 22, 2023 •

edited

Loading

roman-yermilov-gl Feb 24, 2023 •

edited

Loading

roman-yermilov-gl commented Feb 24, 2023 •

edited by github-actions bot

Loading

roman-yermilov-gl commented Feb 27, 2023 •

edited by github-actions bot

Loading