Source Google Ads: Add support for multiple customer ids in specs #7547

t0hai · 2021-11-01T23:22:25Z

What

Fixing issue 6437

How

Allow customer_ids field in spec to accept an array of customer ID strings
Modify code of google_ads client to iterate over each customer_id and send requests accordingly

Pre-merge Checklist

Updating a connector

Community member or Airbyter

Grant edit access to maintainers (instructions)
Secrets in the connector's spec are annotated with airbyte_secret
Unit & integration tests added and passing.
Code reviews completed
Documentation updated
- Changelog updated in docs/integrations/source/google-ads.md including changelog.
PR name follows PR naming conventions
Connector version bumped like described here

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
Credentials added to Github CI. Instructions.
/test connector=connectors/<name> command is passing.
New Connector version released on Dockerhub by running the /publish command described here

Unit test passed

Test session starts (platform: linux, Python 3.7.0, pytest 6.2.5, pytest-sugar 0.9.4)
cachedir: .pytest_cache
rootdir: /home/hai/workspace/sss/airbyte, configfile: pytest.ini
plugins: sugar-0.9.4, timeout-1.4.2, mock-3.6.1
collecting ... 
 airbyte-integrations/connectors/source-google-ads/unit_tests/test_google_ads.py::test_google_ads_init ✓                                                                                        5% ▌         
 airbyte-integrations/connectors/source-google-ads/unit_tests/test_google_ads.py::test_send_request ✓                                                                                          11% █▏        
 airbyte-integrations/connectors/source-google-ads/unit_tests/test_google_ads.py::test_get_fields_from_schema ✓                                                                                16% █▋        
 airbyte-integrations/connectors/source-google-ads/unit_tests/test_google_ads.py::test_interval_chunking ✓                                                                                     21% ██▏       
 airbyte-integrations/connectors/source-google-ads/unit_tests/test_google_ads.py::test_get_date_params ✓                                                                                       26% ██▋       
 airbyte-integrations/connectors/source-google-ads/unit_tests/test_google_ads.py::test_convert_schema_into_query ✓                                                                             32% ███▎      
 airbyte-integrations/connectors/source-google-ads/unit_tests/test_google_ads.py::test_get_field_value ✓                                                                                       37% ███▊      
 airbyte-integrations/connectors/source-google-ads/unit_tests/test_google_ads.py::test_parse_single_result ✓                                                                                   42% ████▎     
 airbyte-integrations/connectors/source-google-ads/unit_tests/test_source.py::test_chunk_date_range ✓                                                                                          47% ████▊     
 airbyte-integrations/connectors/source-google-ads/unit_tests/test_source.py::test_get_updated_state ✓                                                                                         53% █████▍    
 airbyte-integrations/connectors/source-google-ads/unit_tests/test_source.py::test_get_query_fields[\n    SELecT\n  campaign.id,\n  campaign.name,\n  campaign.status,\n  metrics.impressions FROM campaign\nwheRe campaign.status = 'PAUSED'\nAND metrics.impressions > 100\norder by campaign.status\n    -fields0] ✓58% █████▊    
 airbyte-integrations/connectors/source-google-ads/unit_tests/test_source.py::test_get_query_fields[\n        SELECT\n            campaign.accessible_bidding_strategy,\n            segments.ad_destination_type,\n            campaign.start_date,\n            campaign.end_date\n        FROM campaign\n    -fields1] ✓63% ██████▍   
 airbyte-integrations/connectors/source-google-ads/unit_tests/test_source.py::test_get_query_fields[selet aasdasd from aaa-fields2] ✓                                                          68% ██████▉   
 airbyte-integrations/connectors/source-google-ads/unit_tests/test_source.py::test_insert_date[\nSELect\n  campaign.id,\n  campaign.name,\n  campaign.status,\n  metrics.impressions FROM campaign\nwheRe campaign.status = 'PAUSED'\nAND metrics.impressions > 100\norder by campaign.status\n-\nSELect\n  campaign.id,\n  campaign.name,\n  campaign.status,\n  metrics.impressions , segments.date\nFROM campaign\nwheRe campaign.status = 'PAUSED'\nAND metrics.impressions > 100\n AND segments.date BETWEEN '1980-01-01' AND '2000-01-01'\norder by campaign.status\n] ✓74% ███████▍  
 airbyte-integrations/connectors/source-google-ads/unit_tests/test_source.py::test_insert_date[\nSELect\n  campaign.id,\n  campaign.name,\n  campaign.status,\n  metrics.impressions\nFROM campaign\norder by campaign.status\n-\nSELect\n  campaign.id,\n  campaign.name,\n  campaign.status,\n  metrics.impressions\n, segments.date\nFROM campaign\n\nWHERE segments.date BETWEEN '1980-01-01' AND '2000-01-01'\norder by campaign.status\n] ✓79% ███████▉  
 airbyte-integrations/connectors/source-google-ads/unit_tests/test_source.py::test_insert_date[\nSELect\n  campaign.id,\n  campaign.name,\n  campaign.status,\n  metrics.impressions FROM campaign\nwheRe campaign.status = 'PAUSED'\nAND metrics.impressions > 100\n-\nSELect\n  campaign.id,\n  campaign.name,\n  campaign.status,\n  metrics.impressions , segments.date\nFROM campaign\nwheRe campaign.status = 'PAUSED'\nAND metrics.impressions > 100\n AND segments.date BETWEEN '1980-01-01' AND '2000-01-01'\n] ✓84% ████████▌ 
 airbyte-integrations/connectors/source-google-ads/unit_tests/test_source.py::test_insert_date[SELECT campaign.accessible_bidding_strategy, segments.ad_destination_type, campaign.start_date, campaign.end_date FROM campaign-SELECT campaign.accessible_bidding_strategy, segments.ad_destination_type, campaign.start_date, campaign.end_date , segments.date\nFROM campaign\nWHERE segments.date BETWEEN '1980-01-01' AND '2000-01-01'\n] ✓89% ████████▉ {"type": "LOG", "log": {"level": "INFO", "message": "Request made: ClientCustomerId: None, Host: googleads.googleapis.com, Method: /google.ads.googleads.v8.services.GoogleAdsFieldService/SearchGoogleAdsFields, RequestId: bkXITAQWqVCpsK05ryQKLQ, IsFault: False, FaultMessage: None"}}

 airbyte-integrations/connectors/source-google-ads/unit_tests/test_source.py::test_get_json_schema_parse_query ✓                                                                               95% █████████▌{"type": "LOG", "log": {"level": "INFO", "message": "Request made: ClientCustomerId: None, Host: googleads.googleapis.com, Method: /google.ads.googleads.v8.services.GoogleAdsFieldService/SearchGoogleAdsFields, RequestId: pEQDL2JRvycLZlXl9UYAMQ, IsFault: False, FaultMessage: None"}}

 airbyte-integrations/connectors/source-google-ads/unit_tests/test_source.py::test_google_type_conversion ✓                                                                                   100% ██████████

Results (2.55s):
      19 passed

Integration test passed

Test session starts (platform: linux, Python 3.7.0, pytest 6.2.5, pytest-sugar 0.9.4)
cachedir: .pytest_cache
rootdir: /home/hai/workspace/sss/airbyte, configfile: pytest.ini
plugins: sugar-0.9.4, timeout-1.4.2, mock-3.6.1
collecting ... {"type": "LOG", "log": {"level": "INFO", "message": "Starting syncing SourceGoogleAds"}}
{"type": "LOG", "log": {"level": "INFO", "message": "Syncing stream: ad_group_ad_report "}}
{"type": "LOG", "log": {"level": "INFO", "message": "Setting state of ad_group_ad_report stream to {'segments.date': '2021-10-01'}"}}
{"type": "LOG", "log": {"level": "INFO", "message": "Request made: ClientCustomerId: 46xxxxxxxx, Host: googleads.googleapis.com, Method: /google.ads.googleads.v8.services.GoogleAdsService/Search, RequestId: nxEJnNokHYcs5dxljTjtPQ, IsFault: False, FaultMessage: None"}}
{"type": "LOG", "log": {"level": "INFO", "message": "Request made: ClientCustomerId: 46xxxxxxxx, Host: googleads.googleapis.com, Method: /google.ads.googleads.v8.services.GoogleAdsService/Search, RequestId: EbrZ9BwdHfIIpmbCRZG1Fg, IsFault: False, FaultMessage: None"}}
{"type": "LOG", "log": {"level": "INFO", "message": "Request made: ClientCustomerId: 46xxxxxxxx, Host: googleads.googleapis.com, Method: /google.ads.googleads.v8.services.GoogleAdsService/Search, RequestId: d86zQqQAGTQCGEw41LWLlw, IsFault: False, FaultMessage: None"}}
{"type": "LOG", "log": {"level": "INFO", "message": "Request made: ClientCustomerId: 46xxxxxxxx, Host: googleads.googleapis.com, Method: /google.ads.googleads.v8.services.GoogleAdsService/Search, RequestId: zIJovDEx3xUmPZIlrWqDXw, IsFault: False, FaultMessage: None"}}
{"type": "LOG", "log": {"level": "INFO", "message": "Request made: ClientCustomerId: 86xxxxxxxx, Host: googleads.googleapis.com, Method: /google.ads.googleads.v8.services.GoogleAdsService/Search, RequestId: f6LWRFdRhrfIl3ZHmKg8Sg, IsFault: False, FaultMessage: None"}}
{"type": "LOG", "log": {"level": "INFO", "message": "Setting state of ad_group_ad_report stream to {'segments.date': '2021-10-17'}"}}
type=<Type.STATE: 'STATE'> log=None spec=None connectionStatus=None catalog=None record=None state=AirbyteStateMessage(data={'ad_group_ad_report': {'segments.date': '2021-10-17'}})
{"type": "LOG", "log": {"level": "INFO", "message": "Request made: ClientCustomerId: 46xxxxxxxx, Host: googleads.googleapis.com, Method: /google.ads.googleads.v8.services.GoogleAdsService/Search, RequestId: _kuYXQ0XCHJUiwuBgz292g, IsFault: False, FaultMessage: None"}}
{"type": "LOG", "log": {"level": "INFO", "message": "Request made: ClientCustomerId: 46xxxxxxxx, Host: googleads.googleapis.com, Method: /google.ads.googleads.v8.services.GoogleAdsService/Search, RequestId: S4mL-3nemjTxP6RsqoCo4Q, IsFault: False, FaultMessage: None"}}
{"type": "LOG", "log": {"level": "INFO", "message": "Request made: ClientCustomerId: 46xxxxxxxx, Host: googleads.googleapis.com, Method: /google.ads.googleads.v8.services.GoogleAdsService/Search, RequestId: 3skOf9t2Iv_kHQmwJiyf6A, IsFault: False, FaultMessage: None"}}
{"type": "LOG", "log": {"level": "INFO", "message": "Request made: ClientCustomerId: 86xxxxxxxx, Host: googleads.googleapis.com, Method: /google.ads.googleads.v8.services.GoogleAdsService/Search, RequestId: vWx28sXWTaFGwIL1GLJq8A, IsFault: False, FaultMessage: None"}}
{"type": "LOG", "log": {"level": "INFO", "message": "Setting state of ad_group_ad_report stream to {'segments.date': '2021-11-03'}"}}
type=<Type.STATE: 'STATE'> log=None spec=None connectionStatus=None catalog=None record=None state=AirbyteStateMessage(data={'ad_group_ad_report': {'segments.date': '2021-11-03'}})
{"type": "LOG", "log": {"level": "INFO", "message": "Read 5706 records from ad_group_ad_report stream"}}
{"type": "LOG", "log": {"level": "INFO", "message": "Finished syncing SourceGoogleAds"}}
{"type": "LOG", "log": {"level": "INFO", "message": "Starting syncing SourceGoogleAds"}}
{"type": "LOG", "log": {"level": "INFO", "message": "Syncing stream: ad_group_ad_report "}}
{"type": "LOG", "log": {"level": "INFO", "message": "Setting state of ad_group_ad_report stream to {'segments.date': '2021-10-17'}"}}
{"type": "LOG", "log": {"level": "INFO", "message": "Request made: ClientCustomerId: 46xxxxxxxx, Host: googleads.googleapis.com, Method: /google.ads.googleads.v8.services.GoogleAdsService/Search, RequestId: TWodgRSEWmHktMNK1RCg7g, IsFault: False, FaultMessage: None"}}
{"type": "LOG", "log": {"level": "INFO", "message": "Request made: ClientCustomerId: 46xxxxxxxx, Host: googleads.googleapis.com, Method: /google.ads.googleads.v8.services.GoogleAdsService/Search, RequestId: aOvt4F1cfMp3_nlikLEYFA, IsFault: False, FaultMessage: None"}}
{"type": "LOG", "log": {"level": "INFO", "message": "Request made: ClientCustomerId: 46xxxxxxxx, Host: googleads.googleapis.com, Method: /google.ads.googleads.v8.services.GoogleAdsService/Search, RequestId: pJ941YX8wF_pPXT08ypXvA, IsFault: False, FaultMessage: None"}}
{"type": "LOG", "log": {"level": "INFO", "message": "Request made: ClientCustomerId: 46xxxxxxxx, Host: googleads.googleapis.com, Method: /google.ads.googleads.v8.services.GoogleAdsService/Search, RequestId: _RyKI8z28XptYijkhScXIA, IsFault: False, FaultMessage: None"}}
{"type": "LOG", "log": {"level": "INFO", "message": "Request made: ClientCustomerId: 86xxxxxxxx, Host: googleads.googleapis.com, Method: /google.ads.googleads.v8.services.GoogleAdsService/Search, RequestId: 3tXjgtHX4mepLsbeLS6Ygw, IsFault: False, FaultMessage: None"}}
{"type": "LOG", "log": {"level": "INFO", "message": "Setting state of ad_group_ad_report stream to {'segments.date': '2021-11-03'}"}}
{"type": "LOG", "log": {"level": "INFO", "message": "Request made: ClientCustomerId: 46xxxxxxxx, Host: googleads.googleapis.com, Method: /google.ads.googleads.v8.services.GoogleAdsService/Search, RequestId: Fk22zlOGYmwl-hzlxTRUNQ, IsFault: False, FaultMessage: None"}}
{"type": "LOG", "log": {"level": "INFO", "message": "Request made: ClientCustomerId: 86xxxxxxxx, Host: googleads.googleapis.com, Method: /google.ads.googleads.v8.services.GoogleAdsService/Search, RequestId: KhpS5Zzo31kS9p_StLZ7Rw, IsFault: False, FaultMessage: None"}}
{"type": "LOG", "log": {"level": "INFO", "message": "Setting state of ad_group_ad_report stream to {'segments.date': '2021-11-04'}"}}
{"type": "LOG", "log": {"level": "INFO", "message": "Read 3844 records from ad_group_ad_report stream"}}
{"type": "LOG", "log": {"level": "INFO", "message": "Finished syncing SourceGoogleAds"}}

 airbyte-integrations/connectors/source-google-ads/integration_tests/test_incremental.py::test_incremental_sync ✓                                                                             100% ██████████

Results (106.24s):
       1 passed

harshithmullapudi · 2021-11-02T16:41:48Z

Hey @n0rritt do you mind sharing screenshot about integration tests from local

t0hai · 2021-11-03T09:08:51Z

@harshithmullapudi the unit and integration test logs are already included in the PR description as expandables, is that enough or would you need a screenshot of it running in Airflow web UI?

airbyte-integrations/connectors/source-google-ads/source_google_ads/google_ads.py

airbyte-integrations/connectors/source-google-ads/source_google_ads/spec.json

yevhenii-ldv

Please, see comments

* also use more recent dates for integration tests

t0hai · 2021-11-04T10:45:16Z

@yevhenii-ldv I've made the fixes according to your comments, could you have another look again please?

harshithmullapudi · 2021-11-09T11:48:45Z

@yevhenii-ldv is it a +1 from you ?

yevhenii-ldv · 2021-11-15T14:03:53Z

airbyte-integrations/connectors/source-google-ads/source_google_ads/spec.json

      },
      "login_customer_id": {
        "type": "string",
        "title": "Login Customer ID",
-        "description": "If your access to the customer account is through a manager account, this field is required and must be set to the customer ID of the manager account (10-digit number without dashes). More information about this field you can see <a href=\"https://developers.google.com/google-ads/api/docs/concepts/call-structure#cid\">here</a>"
+        "description": "If your access to the customer account is through a manager account, this field is required and must be set to the customer ID of the manager account (10-digit number without dashes). More information about this field you can see <a href=\"https://developers.google.com/google-ads/api/docs/concepts/call-structure#cid\">here</a>",


Are you sure that for all of the listed Customer ID(s) one specified Login Customer ID will be suitable. Or will we need to make the `Customer Id => Login Customer ID" mapping?

I had the same thought in the beginning but multiple (sub) customer accounts belong to one (manager) login_customer_id.
For another manager account a new connection should be created, otherwise it get's messy with the mapping.
https://developers.google.com/google-ads/api/docs/migration/login-customer-id

yevhenii-ldv · 2021-11-15T14:04:29Z

@n0rritt LGTM in general. Only one quastion to be sure.

sherifnada · 2021-11-15T20:03:05Z

@yevhenii-ldv this does not seem to correctly take into account state logic for different user accounts

yevhenii-ldv · 2021-11-16T07:44:23Z

@n0rritt, hello)
As the @sherifnada wrote, we will not work correctly with the state, because now we have created one single state for one custom ID:

{
   "ad_group_ad_report": {
     "segments.date": "2021-06-07"
   }
}

In case of change, we will need to update the logic of saving the state to the following:

{
   "<customer_id>": {
     "ad_group_ad_report": {
       "segments.date": "2021-06-07"
     }
   }
}

But at the same time, preserve backward compatibility for users who have already started this connector and have an state.json file.

t0hai · 2021-11-16T10:28:30Z

@yevhenii-ldv yes I can make another commit for state per account
thx for pointing out @sherifnada

harshithmullapudi · 2021-11-21T20:58:43Z

Hey @n0rritt did you get time to fix it ?

harshithmullapudi · 2021-12-02T14:47:59Z

Hey @n0rritt any updates on this ?

marcosmarxm · 2022-01-27T22:33:33Z

@n0rritt I'll close this due to inactivity. If you want to return this work you can reopen the PR.

octavia-squidington-iii added the community label Nov 1, 2021

github-actions bot added area/connectors Connector related issues area/documentation Improvements or additions to documentation labels Nov 1, 2021

t0hai force-pushed the n0rritt/source-google-ads/add_support_for_multiple_customer_ids_in_specs branch from 1975cd5 to 7a2b746 Compare November 1, 2021 23:23

marcosmarxm requested a review from yevhenii-ldv November 2, 2021 03:15

harshithmullapudi changed the title ~~Add support for multiple customer ids in specs~~ Source Google Ads: Add support for multiple customer ids in specs Nov 2, 2021

harshithmullapudi self-assigned this Nov 2, 2021

yevhenii-ldv reviewed Nov 3, 2021

View reviewed changes

airbyte-integrations/connectors/source-google-ads/source_google_ads/google_ads.py Outdated Show resolved Hide resolved

yevhenii-ldv reviewed Nov 3, 2021

View reviewed changes

airbyte-integrations/connectors/source-google-ads/source_google_ads/google_ads.py Outdated Show resolved Hide resolved

yevhenii-ldv reviewed Nov 3, 2021

View reviewed changes

airbyte-integrations/connectors/source-google-ads/source_google_ads/spec.json Outdated Show resolved Hide resolved

yevhenii-ldv suggested changes Nov 3, 2021

View reviewed changes

n0rritt added 3 commits November 4, 2021 11:02

Add support for multiple customer_ids in specs

4930d79

Adapt unit tests for multi customer_ids support

9235277

* also use more recent dates for integration tests

Bump version in Dockerfile and google-ads.md

a6aa61e

t0hai force-pushed the n0rritt/source-google-ads/add_support_for_multiple_customer_ids_in_specs branch from 7a2b746 to a401614 Compare November 4, 2021 10:36

Fixes according to code review comments

f8bdc5b

t0hai force-pushed the n0rritt/source-google-ads/add_support_for_multiple_customer_ids_in_specs branch from a401614 to f8bdc5b Compare November 4, 2021 10:44

yevhenii-ldv reviewed Nov 15, 2021

View reviewed changes

marcosmarxm assigned yevhenii-ldv Nov 21, 2021

harshithmullapudi removed their assignment Dec 2, 2021

harshithmullapudi unassigned yevhenii-ldv Dec 2, 2021

harshithmullapudi added the blocked label Dec 2, 2021

marcosmarxm closed this Jan 27, 2022

yevhenii-ldv mentioned this pull request Feb 1, 2022

Improve Google Ads Connector to support multiple customer ids #6437

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Source Google Ads: Add support for multiple customer ids in specs #7547

Source Google Ads: Add support for multiple customer ids in specs #7547

t0hai commented Nov 1, 2021 •

edited

Loading

harshithmullapudi commented Nov 2, 2021

t0hai commented Nov 3, 2021

yevhenii-ldv left a comment

t0hai commented Nov 4, 2021

harshithmullapudi commented Nov 9, 2021

yevhenii-ldv Nov 15, 2021

t0hai Nov 15, 2021 •

edited

Loading

yevhenii-ldv commented Nov 15, 2021

sherifnada commented Nov 15, 2021

yevhenii-ldv commented Nov 16, 2021

t0hai commented Nov 16, 2021

harshithmullapudi commented Nov 21, 2021

harshithmullapudi commented Dec 2, 2021

marcosmarxm commented Jan 27, 2022

Source Google Ads: Add support for multiple customer ids in specs #7547

Source Google Ads: Add support for multiple customer ids in specs #7547

Conversation

t0hai commented Nov 1, 2021 • edited Loading

What

How

Recommended reading order

Pre-merge Checklist

Community member or Airbyter

Airbyter

harshithmullapudi commented Nov 2, 2021

t0hai commented Nov 3, 2021

yevhenii-ldv left a comment

Choose a reason for hiding this comment

t0hai commented Nov 4, 2021

harshithmullapudi commented Nov 9, 2021

yevhenii-ldv Nov 15, 2021

Choose a reason for hiding this comment

t0hai Nov 15, 2021 • edited Loading

Choose a reason for hiding this comment

yevhenii-ldv commented Nov 15, 2021

sherifnada commented Nov 15, 2021

yevhenii-ldv commented Nov 16, 2021

t0hai commented Nov 16, 2021

harshithmullapudi commented Nov 21, 2021

harshithmullapudi commented Dec 2, 2021

marcosmarxm commented Jan 27, 2022

t0hai commented Nov 1, 2021 •

edited

Loading

t0hai Nov 15, 2021 •

edited

Loading