Tdl 19235 handle uncaught exceptions #61

prijendev · 2022-06-02T04:12:24Z

Description of change

Added backoff for the following errors (Reference) ,
- InternalServerError
- MethodNotImplemented
- BadGateway
- ServiceUnavailable
- GatewayTimeout
- ResourceExhausted (TooManyRequests)
- ReadTimeout
Added unit test cases for all errors.

QA steps

automated tests passing
manual qa steps passing

Risks

Rollback steps

revert this branch

prijendev · 2022-06-02T04:49:45Z

tap_google_ads/streams.py

                       AttributeError),
                      max_tries=5,
                      jitter=None,
                      giveup=should_give_up,
                      on_giveup=on_giveup_func,
-                      logger=None)
+                      )


Removed logger=None to print logger message of backoff for debug purposes.

dbshah1212 · 2022-06-02T06:27:25Z

tap_google_ads/streams.py

@@ -141,12 +150,15 @@ def on_giveup_func(err):

 @backoff.on_exception(backoff.expo,
                      (GoogleAdsException,
+                       InternalServerError, MethodNotImplemented,


If ServerError is the parent class of InternalServerError, MethodNotImplemented, BadGateway, ServiceUnavailable, GatewayTimeout, can pass ServerError directly here instead of too many classes?

ServerError is the parent class of some other errors also like DataLoss, Unknown, etc (Reference) and we do not want to retry those errors. Thats' why we can not use ServerError directly.

I can't see any harm in retry and we should retry for each serverError as it is from the server side.

Updated error handling to retry all ServerError.

atribed

Just had a couple of questions!

atribed · 2022-06-08T16:03:14Z

tap_google_ads/streams.py

@@ -141,12 +150,14 @@ def on_giveup_func(err):

 @backoff.on_exception(backoff.expo,
                      (GoogleAdsException,
+                       ServerError, TooManyRequests,


Super minor, but since we seem to have all the others on their own line, what would you think about splitting this one up as well?

We have separated out exceptions on separate lines just for better readability. There is no other reason for that.

atribed · 2022-06-08T16:52:05Z

tap_google_ads/streams.py

@@ -7,6 +7,8 @@
 from singer import utils
 from google.protobuf.json_format import MessageToJson
 from google.ads.googleads.errors import GoogleAdsException
+from google.api_core.exceptions import ServerError, TooManyRequests


Looking at the stack trace in TDL-19194, it looks like we have:

2022-05-24 16:14:32,922Z tap - CRITICAL 500 Internal error encountered. [type_url: "type.googleapis.com/google.ads.googleads.v10.errors.GoogleAdsFailure" 2022-05-24 16:14:32,922Z tap - CRITICAL value: "\022\026EDI3x6aWReFYRgzvDSvAVA" 2022-05-24 16:14:32,922Z tap - CRITICAL ] 2022-05-24 16:14:32,922Z tap - Traceback (most recent call last): ... more of the stack trace 2022-05-24 16:14:32,924Z tap - The above exception was the direct cause of the following exception: ... the stack trace for the above referenced exception 2022-05-24 16:14:32,925Z tap - google.api_core.exceptions.InternalServerError: 500 Internal error encountered. [type_url: "type.googleapis.com/google.ads.googleads.v10.errors.GoogleAdsFailure" 2022-05-24 16:14:32,925Z tap - value: "\022\026EDI3x6aWReFYRgzvDSvAVA"

Do you know if these GoogleAdsFailure errors are caught by these exceptions?

From the log, it seems like on the tap side, we would get google.api_core.exceptions.InternalServerError. And in the log, the GoogleAdsFailure error is by InternalServerError.

The following line is just the part of a message in the InternalServerError exception,

[type_url: "type.googleapis.com/google.ads.googleads.v10.errors.GoogleAdsFailure" value: "\022\026EDI3x6aWReFYRgzvDSvAVA"].

We have verified the same kind of error in our local environment,

atribed · 2022-06-09T12:28:25Z

tap_google_ads/streams.py

@@ -7,6 +7,8 @@
 from singer import utils
 from google.protobuf.json_format import MessageToJson
 from google.ads.googleads.errors import GoogleAdsException
+from google.api_core.exceptions import ServerError, TooManyRequests


* Tdl 19235 handle uncaught exceptions (#61) * Added backoff for 5xx, 429 and ReadTimeout errors. * Resolved pylint error. * Updated comments in the unittest cases. * Updated error handling. * TDL-18749 Implement interruptible full table streams. (#60) * Implemented interruptible full table streams. * Resolved pylint error * Resolved error in full table sync test case. * Updated config.yml to pass cci * Updated query building logic. * Updated integration test case. * Resolved review comments. * Resolved comments. * Implemeted logic to skip the duplicate records. * Resolved unittest case error. * Resolved pylint error * Resolved integration test case error * Added empty filter param for call_details and campaign_label stream. * Added unit test cases for should_sync method. * Revert "Implemeted logic to skip the duplicate records." This reverts commit cd06e11. * Added logger message for debugging purpose * Updated integration test case. * Replaced .format with f string. * Updated comment in integration test. Co-authored-by: KrishnanG <[email protected]>

* Bump to v1.0.0, update changelog (singer-io#45) * Qa/future testing (singer-io#48) * documented reasons for untested streams * list high level scenarios for manual qa checks * remove stitch specific pr template Co-authored-by: kspeer <[email protected]> * Add call_details stream (singer-io#49) * Add call_view core stream, filter non-attribute fields from core streams * Change `call_view` stream name to `call_details` * Make pylint happy * Fix integration tests * Remove campaign_id from foreign keys, split call_details foreign keys * Update tests to exclude call_details as needed * Add context around excluding call_details from tests * Remove outdated TODO related to addition of call_details stream. Co-authored-by: dsprayberry <[email protected]> * Add core LABELS streams and campaign.labels fields to relevant reports (singer-io#53) * Add core LABELS streams and campaign.labels fields to relevant reports * Update test exclusions to exclude new core streams. * Update discover to include campaign_label in reports list * Update foreign_key expected metadata to include attributed resource foreign_keys * Update tests to re-include campaign_labels and labels as we now have test data. * Handles case where state does not have currently_syncing (singer-io#54) * Handles case where state does not have currently_syncing * tap-tester test added * takeout unused imports in test Co-authored-by: kspeer <[email protected]> * Version bump and changelog entry (singer-io#52) * Version bump and changelog entry * Update to include PR 53. * Add PR 54 to changelog * Implement Automatic Keys (singer-io#55) * Set geographic_view.location_type as automatic to account for reporting discrepancies * Update Automatic Fields test happy / error paths * Update base to include automatic_keys metadata for use in tests * WIP Add Automatic Report Fields * Pass automatic_keys to BaseStream; use automatic_keys for inclusion * Fix bad field name in streams; Start updating tests * Rename function to match base.py * Add closing brace -_- * Fix tests; rename ad_group_criterion_criterion_id in automatic_keys; reorder test metadata; add campaign.id to campaign_audience_performance_report * Rename report_field_parts to split_report_field * Update transform_keys to raise ad_group_ad.ad fields * Update happy path streams_to_test to exclude streams with no data * Remove field name change for ad_performance_report stream * Accept Andy's Suggestion Co-authored-by: Andy Lu <[email protected]> * Reverting Andy's change because it affects core streams * Explicitly install grpcio-status to avoid from_call attribute errors * Update setup.py with docs explaining required but unused dep Co-authored-by: Andy Lu <[email protected]> Co-authored-by: Arthur Gorka <[email protected]> * Cleanup (singer-io#56) * Remove useless function * Rename "REPORTS" to "STREAMS" for accuracy / readability * Version bump for PRs 56 and 55 (singer-io#57) * Version bump for PRs 56 and 55 * Update to exclude ad_group_ad change for ad_performance_report * [Feature] Add more core streams (singer-io#58) * WIP add new core streams * Set geographic_view.location_type as automatic to account for reporting discrepancies * Update Automatic Fields test happy / error paths * Update base to include automatic_keys metadata for use in tests * WIP Add Automatic Report Fields * Pass automatic_keys to BaseStream; use automatic_keys for inclusion * Fix bad field name in streams; Start updating tests * Rename function to match base.py * Add closing brace -_- * Fix tests; rename ad_group_criterion_criterion_id in automatic_keys; reorder test metadata; add campaign.id to campaign_audience_performance_report * Rename report_field_parts to split_report_field * Update transform_keys to raise ad_group_ad.ad fields * Update happy path streams_to_test to exclude streams with no data * WIP remove IPDB and try except * Fix report_definition typo and add handling for user_interest_id field * Remove field name change for ad_performance_report stream * Accept Andy's Suggestion Co-authored-by: Andy Lu <[email protected]> * Reverting Andy's change because it affects core streams * Explicitly install grpcio-status to avoid from_call attribute errors * Remove ipdb -_- * Update setup.py with docs explaining required but unused dep * WIP w/ failing tests and attributed_resource foreign_keys * WIP with failing tests; remove extraneous attributed_resoruce automatic fields * Create UserInterestStream class to handle its edge case * Add transform_keys to UserInterestStream class; start test updates * Rename obj to json_message; rename variables accordingly; fix UserInterestStream transform_keys * update auto fields test to account for compound pks * Fix campaigns typo * Remove exclusion of feed and feed items from sync canary test Co-authored-by: Andy Lu <[email protected]> Co-authored-by: Arthur Gorka <[email protected]> Co-authored-by: kspeer <[email protected]> Co-authored-by: atribed <[email protected]> * V1.3.0 (singer-io#59) * Version bump for PRs 56 and 55 * Update to exclude ad_group_ad change for ad_performance_report * Version bump for 1.3.0 Co-authored-by: Arthur Gorka <[email protected]> * Qa/fix build notification (singer-io#65) * fix slack notif for build * remove click_performance_report from tests * remove click_performance_report from sync canary test * run only streams that are untested in canary sync * just skip canaray test * put back the assert in the skipped test Co-authored-by: kspeer <[email protected]> * Crest master (singer-io#66) * Tdl 19235 handle uncaught exceptions (singer-io#61) * Added backoff for 5xx, 429 and ReadTimeout errors. * Resolved pylint error. * Updated comments in the unittest cases. * Updated error handling. * TDL-18749 Implement interruptible full table streams. (singer-io#60) * Implemented interruptible full table streams. * Resolved pylint error * Resolved error in full table sync test case. * Updated config.yml to pass cci * Updated query building logic. * Updated integration test case. * Resolved review comments. * Resolved comments. * Implemeted logic to skip the duplicate records. * Resolved unittest case error. * Resolved pylint error * Resolved integration test case error * Added empty filter param for call_details and campaign_label stream. * Added unit test cases for should_sync method. * Revert "Implemeted logic to skip the duplicate records." This reverts commit cd06e11. * Added logger message for debugging purpose * Updated integration test case. * Replaced .format with f string. * Updated comment in integration test. Co-authored-by: KrishnanG <[email protected]> * Bump version (singer-io#67) Co-authored-by: KrishnanG <[email protected]> * TDL-18524 updated readme and added sample config (singer-io#51) * updated readme and added sample config * updated endpoints * add new streams * resolved PR comments * fixed a typo * added the * Add timeout parameter to gas.search (singer-io#64) * Add timeout parameter to search gas.search * Increase timeout to 15 minutes for safety. * Add get_request_timeout function, add config to make_request as needed * Update on_giveup to raise specific exception text for timeoutexception class; add unit test * Make Pylint happy take 1 * Update make_request signature in unittests. * Another Unittest update * More unittest fixes for signature * Add default config param value for ease of implementation in future tests * Fix pylint dangerous-defaul-value * Fix stupid error * Version bump and changelog (singer-io#70) * TDL-19486 Add limit clause to core stream queries (singer-io#68) * Initial commit for add page limit. * Added limit parameter in sync method of ReportStream class. * Fixed issue for call_details stream. * Resolved unit test case error. * Added test cases. * Updated code comments. * Fixed keyerror issue. * Updated default query limit. * Updated pagination test case. * Modify config name to be more explicit. * Update comment for accuracy. * Update property name in base and propogate name change to tests * Exclude feed from streams_to_test because of lack of data * Committing stuff from main that should have already been in Co-authored-by: dsprayberry <[email protected]> * Version Bump and Changelog Update (singer-io#72) * Reintroduce unintentionally removed files. (singer-io#73) * add schemaless * fix schemaless config * update login_customer_ids * fix missing key from dict * add google ads api version in config add log to show the version * use version from config * use version global Co-authored-by: bryantgray <[email protected]> Co-authored-by: Kyle Speer <[email protected]> Co-authored-by: kspeer <[email protected]> Co-authored-by: dsprayberry <[email protected]> Co-authored-by: Andy Lu <[email protected]> Co-authored-by: Arthur Gorka <[email protected]> Co-authored-by: atribed <[email protected]> Co-authored-by: Prijen Khokhani <[email protected]> Co-authored-by: KrishnanG <[email protected]> Co-authored-by: KrisPersonal <[email protected]> Co-authored-by: namrata270998 <[email protected]> Co-authored-by: bbaltagi-dtsl <[email protected]>

prijendev added 2 commits June 1, 2022 18:02

Added backoff for 5xx, 429 and ReadTimeout errors.

7e5a63c

Resolved pylint error.

bfed443

prijendev marked this pull request as ready for review June 2, 2022 04:26

Updated comments in the unittest cases.

15e7b6f

prijendev requested review from dbshah1212, namrata270998 and savan-chovatiya June 2, 2022 04:47

prijendev commented Jun 2, 2022

View reviewed changes

dbshah1212 reviewed Jun 2, 2022

View reviewed changes

prijendev requested a review from dbshah1212 June 2, 2022 08:24

savan-chovatiya approved these changes Jun 2, 2022

View reviewed changes

Updated error handling.

fd6374c

prijendev requested a review from savan-chovatiya June 2, 2022 10:42

savan-chovatiya approved these changes Jun 2, 2022

View reviewed changes

namrata270998 approved these changes Jun 2, 2022

View reviewed changes

dbshah1212 approved these changes Jun 3, 2022

View reviewed changes

dbshah1212 requested review from KrisPersonal, kspeer825 and RushiT0122 June 3, 2022 11:05

atribed reviewed Jun 8, 2022

View reviewed changes

atribed approved these changes Jun 9, 2022

View reviewed changes

kspeer825 approved these changes Jun 14, 2022

View reviewed changes

prijendev merged commit 4bb22a5 into crest-master Jun 15, 2022

prijendev mentioned this pull request Jun 15, 2022

Crest master #66

Merged

2 tasks

KrisPersonal mentioned this pull request Jun 15, 2022

Bump version #67

Merged

2 tasks

dsprayberry deleted the TDL-19235-handle-uncaught-exceptions branch June 30, 2022 17:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tdl 19235 handle uncaught exceptions #61

Tdl 19235 handle uncaught exceptions #61

prijendev commented Jun 2, 2022 •

edited

Loading

prijendev Jun 2, 2022

dbshah1212 Jun 2, 2022

prijendev Jun 2, 2022

dbshah1212 Jun 2, 2022

prijendev Jun 2, 2022

atribed left a comment

atribed Jun 8, 2022

prijendev Jun 9, 2022

atribed Jun 8, 2022

prijendev Jun 9, 2022 •

edited

Loading

atribed Jun 9, 2022

atribed Jun 9, 2022

Tdl 19235 handle uncaught exceptions #61

Tdl 19235 handle uncaught exceptions #61

Conversation

prijendev commented Jun 2, 2022 • edited Loading

Description of change

QA steps

Risks

Rollback steps

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

atribed left a 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

prijendev Jun 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prijendev commented Jun 2, 2022 •

edited

Loading

prijendev Jun 9, 2022 •

edited

Loading