Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#12486 and #49 from alpha-beta-issues fixes #12914

Merged
merged 5 commits into from
May 25, 2022

Conversation

davydov-d
Copy link
Collaborator

@davydov-d davydov-d commented May 17, 2022

What

Source Google Ads Sync option to skip inactive accounts rather than failing sync for all accounts:

  • Since the standard Accounts stream was used to fetch the customers info, it used a where clause in its GAQL query with a segment.date filter which filtered out all the customers
  • Even more, only the first customer account among those that could be fetched(if any) was checked for being a manager
  • If that account was not a manager, all the streams were instantiated with a full customer_ids list, despite of that some of them could be managers which would lead to errors further when reading

Source: Google Ads - with GAQL not able to create connection:

  • After making the request for validating the GAQL query to Google Ads API, we get the generator, which would raise an exception in case of error only when being iterated over. Which wasn't done
  • A bug in formatting the custom query: some keyword patterns missed whitespaces, what led to making an invalid query (parameters in regex instead of _parameters_ made the function insert where clause right inside the select clause)

Other bug fixes:

  • end_date param was partially ignored

How

  • Added another service stream, not exposed to a user, that would fetch the customers by their IDs independent of date segments
  • Refactored the code to check is_manager attribute of each customer, not only the first one.
  • Instantiate the streams depending on the customer.is_manager field only with non-manager accounts as params
  • Try-catch auth and query errors and log them when reading records and checking the connection instead of failing the stream for all customer accounts
  • Fixed the bug in the custom query formatter (added whitespaces so that partial word matches would not be treated as key words)
  • Added iteration over the response when validating the custom query so that API errors get raised instead of being ignored
  • Updated all the imports and links from API v8 to API v9
  • Fixed issue with the end_date parameter
  • Updated tests

@github-actions github-actions bot added the area/connectors Connector related issues label May 17, 2022
@davydov-d davydov-d marked this pull request as draft May 17, 2022 13:53
@github-actions github-actions bot added the area/documentation Improvements or additions to documentation label May 18, 2022
@davydov-d
Copy link
Collaborator Author

davydov-d commented May 18, 2022

/test connector=connectors/source-google-ads

🕑 connectors/source-google-ads https://github.com/airbytehq/airbyte/actions/runs/2344072097
✅ connectors/source-google-ads https://github.com/airbytehq/airbyte/actions/runs/2344072097
Python tests coverage:

Name                                                 Stmts   Miss  Cover
------------------------------------------------------------------------
source_acceptance_test/utils/__init__.py                 6      0   100%
source_acceptance_test/tests/__init__.py                 4      0   100%
source_acceptance_test/__init__.py                       2      0   100%
source_acceptance_test/tests/test_full_refresh.py       52      2    96%
source_acceptance_test/utils/asserts.py                 37      2    95%
source_acceptance_test/config.py                        75      6    92%
source_acceptance_test/utils/json_schema_helper.py     105     13    88%
source_acceptance_test/utils/common.py                  80     17    79%
source_acceptance_test/tests/test_incremental.py        85     25    71%
source_acceptance_test/utils/compare.py                 62     23    63%
source_acceptance_test/tests/test_core.py              285    106    63%
source_acceptance_test/base.py                          10      4    60%
source_acceptance_test/utils/connector_runner.py       110     48    56%
------------------------------------------------------------------------
TOTAL                                                  913    246    73%
Name                                       Stmts   Miss  Cover
--------------------------------------------------------------
source_google_ads/models.py                   18      0   100%
source_google_ads/__init__.py                  2      0   100%
source_google_ads/google_ads.py               67     10    85%
source_google_ads/streams.py                 162     26    84%
source_google_ads/source.py                   75     28    63%
source_google_ads/custom_query_stream.py      75     50    33%
--------------------------------------------------------------
TOTAL                                        399    114    71%
Name                                       Stmts   Miss  Cover
--------------------------------------------------------------
source_google_ads/models.py                   18      0   100%
source_google_ads/__init__.py                  2      0   100%
source_google_ads/streams.py                 162      8    95%
source_google_ads/source.py                   75      4    95%
source_google_ads/custom_query_stream.py      75      6    92%
source_google_ads/google_ads.py               67     12    82%
--------------------------------------------------------------
TOTAL                                        399     30    92%

Python short test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/plugin.py:56: Skipping TestIncremental.test_two_sequential_reads because not found in the config
================== 23 passed, 1 skipped in 539.83s (0:08:59) ===================

@davydov-d
Copy link
Collaborator Author

davydov-d commented May 23, 2022

/test connector=connectors/source-google-ads

🕑 connectors/source-google-ads https://github.com/airbytehq/airbyte/actions/runs/2370773088
✅ connectors/source-google-ads https://github.com/airbytehq/airbyte/actions/runs/2370773088
Python tests coverage:

Name                                                 Stmts   Miss  Cover
------------------------------------------------------------------------
source_acceptance_test/utils/__init__.py                 6      0   100%
source_acceptance_test/tests/__init__.py                 4      0   100%
source_acceptance_test/__init__.py                       2      0   100%
source_acceptance_test/tests/test_full_refresh.py       52      2    96%
source_acceptance_test/utils/asserts.py                 37      2    95%
source_acceptance_test/config.py                        75      6    92%
source_acceptance_test/utils/json_schema_helper.py     105     13    88%
source_acceptance_test/utils/common.py                  80     17    79%
source_acceptance_test/tests/test_incremental.py        85     25    71%
source_acceptance_test/utils/compare.py                 62     23    63%
source_acceptance_test/tests/test_core.py              285    106    63%
source_acceptance_test/base.py                          10      4    60%
source_acceptance_test/utils/connector_runner.py       110     48    56%
------------------------------------------------------------------------
TOTAL                                                  913    246    73%
Name                                       Stmts   Miss  Cover
--------------------------------------------------------------
source_google_ads/models.py                   18      0   100%
source_google_ads/__init__.py                  2      0   100%
source_google_ads/google_ads.py               67     10    85%
source_google_ads/streams.py                 163     27    83%
source_google_ads/source.py                   80     23    71%
source_google_ads/custom_query_stream.py      75     46    39%
--------------------------------------------------------------
TOTAL                                        405    106    74%
Name                                       Stmts   Miss  Cover
--------------------------------------------------------------
source_google_ads/models.py                   18      0   100%
source_google_ads/__init__.py                  2      0   100%
source_google_ads/streams.py                 163      8    95%
source_google_ads/source.py                   80      4    95%
source_google_ads/custom_query_stream.py      75      6    92%
source_google_ads/google_ads.py               67     12    82%
--------------------------------------------------------------
TOTAL                                        405     30    93%

Python short test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/plugin.py:56: Skipping TestIncremental.test_two_sequential_reads because not found in the config
================== 23 passed, 1 skipped in 426.12s (0:07:06) ===================

@codecov
Copy link

codecov bot commented May 23, 2022

Codecov Report

❗ No coverage uploaded for pull request base (master@ec8c8c6). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master   #12914   +/-   ##
=========================================
  Coverage          ?   92.59%           
=========================================
  Files             ?        6           
  Lines             ?      405           
  Branches          ?        0           
=========================================
  Hits              ?      375           
  Misses            ?       30           
  Partials          ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ec8c8c6...82924cf. Read the comment docs.

@evantahler evantahler removed their request for review May 24, 2022 16:12
@davydov-d
Copy link
Collaborator Author

davydov-d commented May 24, 2022

/publish connector=connectors/source-google-ads

🕑 connectors/source-google-ads https://github.com/airbytehq/airbyte/actions/runs/2379567665
❌ Failed to publish connectors/source-google-ads
❌ Couldn't auto-bump version for connectors/source-google-ads

@davydov-d
Copy link
Collaborator Author

davydov-d commented May 25, 2022

/publish connector=connectors/source-google-ads

🕑 connectors/source-google-ads https://github.com/airbytehq/airbyte/actions/runs/2382548925
🚀 Successfully published connectors/source-google-ads
🚀 Auto-bumped version for connectors/source-google-ads
✅ connectors/source-google-ads https://github.com/airbytehq/airbyte/actions/runs/2382548925

@octavia-squidington-iii octavia-squidington-iii temporarily deployed to more-secrets May 25, 2022 06:42 Inactive
@octavia-squidington-iii octavia-squidington-iii temporarily deployed to more-secrets May 25, 2022 06:42 Inactive
@davydov-d davydov-d merged commit a3f6a18 into master May 25, 2022
@davydov-d davydov-d deleted the ddavydov/12486-alpha-beta-issues-49-fixes branch May 25, 2022 07:13
jscottpolevault pushed a commit to jscottpolevault/airbyte that referenced this pull request Jun 1, 2022
…ehq#12914)

* airbytehq#12486 and airbytehq#49 from alpha-beta-issues fixes

* test updates for #airbyte/12486 and #alpha-beta-issues/49

* airbytehq#12486 airbytehq#49-alpha-beta-issues PR comment fixes

* airbytehq#12486 doc upd: add note on use of custom queries

* auto-bump connector version

Co-authored-by: Octavia Squidington III <octavia-squidington-iii@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/connectors Connector related issues area/documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Source Google Ads Sync option to skip inactive accounts rather than failing sync for all accounts
5 participants