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

✨Source Google Ads: Add possibility to sync all connected accounts #33707

Merged
merged 8 commits into from
Jan 11, 2024

Conversation

tolik0
Copy link
Contributor

@tolik0 tolik0 commented Dec 21, 2023

What

  • Implemented Account Synchronization: This update introduces the capability to sync all connected accounts, resolving the issue: https://github.com/airbytehq/oncall/issues/3741.

  • Removed login_customer_id Property: To simplify configuration, the login_customer_id property has been removed from the spec. Previously, this property was essential for accessing client accounts under a manager account. The Google Ads connector automatically retrieves this information while requesting connected customers.

  • Introduced customer_status_filter Property: A new property, customer_status_filter, has been added. This feature enables users to filter out inactive accounts to speed up the sync.

How

The customer_id property has been revised to be optional. In this new setup, the connector initially requests data from all accessible accounts and then proceeds to iterate over the child customers associated with these accounts. This enhancement significantly streamlines the configuration process, allowing for the aggregation of multiple customers into a single connection. Moreover, this update ensures minimal configuration, requiring only authentication. This simplification greatly reduces setup complexity and enhances user convenience.

Copy link

vercel bot commented Dec 21, 2023

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 Jan 11, 2024 7:07pm

Copy link
Contributor

github-actions bot commented Dec 21, 2023

Warning

🚨 Connector code freeze is in effect until 2024-01-02. This PR is changing connector code. Please contact the current OC engineers if you want to merge this change to master.

Copy link
Contributor

github-actions bot commented Dec 21, 2023

Before Merging a Connector Pull Request

Wow! What a great pull request you have here! 🎉

To merge this PR, ensure the following has been done/considered for each connector added or updated:

  • PR name follows PR naming conventions
  • Breaking changes are considered. If a Breaking Change is being introduced, ensure an Airbyte engineer has created a Breaking Change Plan.
  • Connector version has been incremented in the Dockerfile and metadata.yaml according to our Semantic Versioning for Connectors guidelines
  • You've updated the connector's metadata.yaml file any other relevant changes, including a breakingChanges entry for major version bumps. See metadata.yaml docs
  • Secrets in the connector's spec are annotated with airbyte_secret
  • All documentation files are up to date. (README.md, bootstrap.md, docs.md, etc...)
  • Changelog updated in docs/integrations/<source or destination>/<name>.md with an entry for the new version. See changelog example
  • Migration guide updated in docs/integrations/<source or destination>/<name>-migrations.md with an entry for the new version, if the version is a breaking change. See migration guide example
  • If set, you've ensured the icon is present in the platform-internal repo. (Docs)

If the checklist is complete, but the CI check is failing,

  1. Check for hidden checklists in your PR description

  2. Toggle the github label checklist-action-run on/off to re-run the checklist CI.

@alafanechere
Copy link
Contributor

Hey @tolik0 . As we're under code freeze, would you mind re-creating your PR with a different branch name?
Please follow the instructions here

@alafanechere
Copy link
Contributor

@tolik0 could you please let me know if this is a bug fix or a new feature?
I know it's related to an OC issue, but I'm not sure if:

  • this is an improvement to better handle the problem the user faced but there is a workaround for them to get their connection working
  • this is a bug fix, the impacted customer can't set their connection otherwise

@tolik0
Copy link
Contributor Author

tolik0 commented Dec 21, 2023

@alafanechere, this update is essentially a new feature crucial for a specific user scenario. Currently, our Google Ads connector syncs data exclusively from preselected accounts. The user who requested this feature manages over 100 connected customers, making it challenging to manually identify and input all the necessary IDs. The tree-like structure of the customer accounts further compounds this complexity. Therefore, this feature automates the process, enabling efficient synchronization across all connected customer accounts.

@octavia-squidington-iii octavia-squidington-iii added the area/documentation Improvements or additions to documentation label Dec 21, 2023
@tolik0 tolik0 requested a review from a team December 22, 2023 12:20
@tolik0 tolik0 marked this pull request as ready for review December 22, 2023 12:20
@tolik0 tolik0 self-assigned this Dec 22, 2023
@tolik0 tolik0 requested a review from alafanechere December 27, 2023 13:06
Copy link
Contributor

@alafanechere alafanechere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As this is quite a substantial logic change, addressing a single customer issue, I feel it'd be safer to:

  • Not merge this during the code freeze
  • Publish a pre-release version
  • Coordinate with the support team to make the customer create a new connection using this specific version and confirm their problem is addressed.

tz = Timezone(time_zone_name) if time_zone_name else "local"
for account in accounts:
time_zone_name = account.get(f"{table_name}.time_zone")
tz = Timezone(time_zone_name) if time_zone_name else "local"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here tz is either a string a or Timezone, for type consistency can we make sure tz type is invariant?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Copy link
Contributor

@alafanechere alafanechere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your changes and replies. LGTM.

@tolik0 tolik0 force-pushed the tolik0/source-google-ads/sync-all-connected-accounts branch from a0b6a3a to 99ea4f6 Compare January 9, 2024 18:33
@tolik0 tolik0 force-pushed the tolik0/source-google-ads/sync-all-connected-accounts branch from e2689e4 to a204c4c Compare January 9, 2024 19:04
@tolik0 tolik0 merged commit c1574b8 into master Jan 11, 2024
23 of 24 checks passed
@tolik0 tolik0 deleted the tolik0/source-google-ads/sync-all-connected-accounts branch January 11, 2024 19:45
@nazariyme
Copy link

Hey,
these changes should be reverted as it causes failure for Google Ads source if there's customer ID with not finished setup
All our connections failed today
Screenshot_1
Screenshot_2

@tolik0
Copy link
Contributor Author

tolik0 commented Jan 12, 2024

Hi @nazariyme. Thank you for bringing this to our attention. I have created a PR to address the issue and will let you know as soon as it's merged. I'm very sorry for any inconvenience this has caused.

@nazariyme
Copy link

I fixed it by removing that pending customer ID as it was not used by anyone. All works fine for us now but it worth fixing in the source itself. @tolik0 thanks for a quick response and the fix you prepared

@bachik-24
Copy link

Hi @tolik0
After this release, I corrected the name of the variable in the source json from custom_queries to custom_queries_array. Now the connection does not take a single line, but it runs successfully. With what it can be connected?

@tolik0
Copy link
Contributor Author

tolik0 commented Feb 7, 2024

@bachik-24 Hi, what do you mean by "Connection does not take a single line"?

@bachik-24
Copy link

@tolik0  
I mean that the source is configured correctly but does not return a single row (0 bytes, 0 records). On the previous version with the same parameters everything works. What could go wrong?

@tolik0
Copy link
Contributor Author

tolik0 commented Feb 7, 2024

@bachik-24 Can you please share your custom_queries_array parameter?

@bachik-24
Copy link

@tolik0

                    {
                        "query": "SELECT segments.device, customer.id, metrics.cost_micros, campaign.id, campaign.name, metrics.clicks, metrics.conversions, metrics.impressions, geographic_view.country_criterion_id, geographic_view.location_type, geographic_view.resource_name FROM geographic_view",
                        "table_name": "google_acquisition_report_5197072984"
                    }

@tolik0
Copy link
Contributor Author

tolik0 commented Feb 7, 2024

@bachik-24 It looks like you should add the segments.date parameter to your query; previously, it was added automatically. Here is the PR with these changes: #33603

@bachik-24
Copy link

@tolik0 This helped, thank you very much! I think this should be indicated separately in the commit that now this parameter is worth specifying.

@tolik0
Copy link
Contributor Author

tolik0 commented Feb 7, 2024

@bachik-24 This change is described in the PR description and the docs, but actually, you don't need to modify your config manually, as it is updated using config migrations when you run connector with the old config: https://github.com/airbytehq/airbyte/blob/66ce98fdc4eacd6a3d69210630e3b3b9e0762ba9/airbyte-integrations/connectors/source-google-ads/source_google_ads/config_migrations.py

@bachik-24
Copy link

@tolik0 thank you for your work!

jatinyadav-cc pushed a commit to ollionorg/datapipes-airbyte that referenced this pull request Feb 26, 2024
jatinyadav-cc pushed a commit to ollionorg/datapipes-airbyte that referenced this pull request Feb 26, 2024
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 connectors/source/google-ads
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants