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.

Sign up for GitHub

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

Jump to bottom

🚀Source Google Ads: support main streams #4788

Merged
merged 10 commits into from
Jul 23, 2021
Merged

🚀Source Google Ads: support main streams #4788

merged 10 commits into from
Jul 23, 2021

Conversation

yevhenii-ldv
Copy link
Contributor

@yevhenii-ldv yevhenii-ldv commented Jul 16, 2021 •
edited by sherifnada
Loading

What

closes #4596

How

Describe the solution

Recommended reading order

  1. x.java
  2. y.python

Pre-merge Checklist

  • Issue acceptance criteria met
  • PR name follows PR naming conventions
  • Secrets are annotated with airbyte_secret in the connector's spec
  • Credentials added to Github CI if needed and not already present. instructions for injecting secrets into CI.
  • Unit & integration tests added as appropriate (and are passing)
    • Community members: please provide proof of this succeeding locally e.g: screenshot or copy-paste acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
  • /test connector=connectors/<name> command as documented here is passing.
    • Community members can skip this, Airbyters will run this for you.
  • Code reviews completed
  • Documentation updated
    • README.md
    • docs/SUMMARY.md if it's a new connector
    • Created or updated reference docs in docs/integrations/<source or destination>/<name>.
    • Changelog in the appropriate page in docs/integrations/.... See changelog example
    • docs/integrations/README.md contains a reference to the new connector
    • Build status added to build page
  • Build is successful
  • Connector version bumped like described here
  • New Connector version released on Dockerhub by running the /publish command described here
  • No major blockers
  • PR merged into master branch
  • Follow up tickets have been created
  • Associated tickets have been closed & stakeholders notified

@github-actions github-actions bot added area/connectors Connector related issues area/documentation Improvements or additions to documentation labels Jul 16, 2021
@yevhenii-ldv
Copy link
Contributor Author

yevhenii-ldv commented Jul 16, 2021 •
edited by github-actions bot
Loading

/test connector=connectors/source-google-ads

🕑 connectors/source-google-ads https://github.com/airbytehq/airbyte/actions/runs/1038054336
✅ connectors/source-google-ads https://github.com/airbytehq/airbyte/actions/runs/1038054336

@yevhenii-ldv yevhenii-ldv changed the title Source Google Ads: support main streams 🚀 Source Google Ads: support main streams Jul 19, 2021
@yevhenii-ldv yevhenii-ldv changed the title 🚀 Source Google Ads: support main streams 🚀Source Google Ads: support main streams Jul 19, 2021
Zirochkaa
Zirochkaa approved these changes Jul 20, 2021
View reviewed changes
Copy link
Contributor

@Zirochkaa Zirochkaa left a comment

Choose a reason for hiding this comment

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

lgtm, just few small comments.

airbyte-integrations/connectors/source-google-ads/source_google_ads/google_ads.py Outdated Show resolved Hide resolved
airbyte-integrations/connectors/source-google-ads/source_google_ads/google_ads.py Show resolved Hide resolved
airbyte-integrations/connectors/source-google-ads/source_google_ads/streams.py Show resolved Hide resolved
airbyte-integrations/connectors/source-google-ads/source_google_ads/streams.py Show resolved Hide resolved
@yevhenii-ldv yevhenii-ldv mentioned this pull request Jul 20, 2021
Closed
26 tasks
@yevhenii-ldv
Copy link
Contributor Author

yevhenii-ldv commented Jul 20, 2021 •
edited by github-actions bot
Loading

/test connector=connectors/source-google-ads

🕑 connectors/source-google-ads https://github.com/airbytehq/airbyte/actions/runs/1049087413
✅ connectors/source-google-ads https://github.com/airbytehq/airbyte/actions/runs/1049087413

@Zirochkaa Zirochkaa linked an issue Jul 20, 2021 that may be closed by this pull request
source google ads exception DATE_RANGE_TOO_NARROW #4806
Closed
sherifnada
sherifnada reviewed Jul 20, 2021
View reviewed changes
...-integrations/connectors/source-google-ads/source_google_ads/schemas/ad_group_ad_report.json
@@ -22,37 +24,37 @@
},
"metrics.active_view_cpm": {
"description": "ActiveViewCpm",
"type": ["null", "string"],
"type": ["null", "number"],
Copy link
Contributor

Choose a reason for hiding this comment

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

good change!

airbyte-integrations/connectors/source-google-ads/source_google_ads/google_ads.py
"accounts": "customer",
"campaigns": "campaign",
"ad_groups": "ad_group",
"ad_group_ads": "ad_group_ad",
Copy link
Contributor

Choose a reason for hiding this comment

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

is this the same as the ad_group_ad_report? if so should we just rename the former?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think they are the same thing. The differences between these streams are detailed below.

Copy link
Contributor

Choose a reason for hiding this comment

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

fundamentally aren't these both mapping to the same entity in the Google API? The reason they have different fields is because we are SELECTing different fields for each of these streams (presumably because the json schemas for those fields are different)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fundamentally yes. But ad_group_ad_report in this case is presented as Ad Performance Report. This stream gives us not only information about Google Ads, but also statistical and metric data that we can receive daily.
If we combine these two streams, then the essence of Google Ads will cease to exist as such, and we will receive one record for each Google Ad every day, which contains all the information about Ad and a lot of daily statistics. In this case, the user may simply not understand how many Ads he has, and he will also not be able to work only with Ads, he will have to work only with their reports.

airbyte-integrations/connectors/source-google-ads/source_google_ads/google_ads.py Show resolved Hide resolved
airbyte-integrations/connectors/source-google-ads/source_google_ads/google_ads.py Show resolved Hide resolved
airbyte-integrations/connectors/source-google-ads/source_google_ads/streams.py Show resolved Hide resolved
sherifnada
sherifnada reviewed Jul 22, 2021
View reviewed changes
airbyte-integrations/connectors/source-google-ads/source_google_ads/google_ads.py Outdated Show resolved Hide resolved
airbyte-integrations/connectors/source-google-ads/source_google_ads/google_ads.py
"accounts": "customer",
"campaigns": "campaign",
"ad_groups": "ad_group",
"ad_group_ads": "ad_group_ad",
Copy link
Contributor

Choose a reason for hiding this comment

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

fundamentally aren't these both mapping to the same entity in the Google API? The reason they have different fields is because we are SELECTing different fields for each of these streams (presumably because the json schemas for those fields are different)

airbyte-integrations/connectors/source-google-ads/source_google_ads/google_ads.py Show resolved Hide resolved
airbyte-integrations/connectors/source-google-ads/source_google_ads/schemas/accounts.json Outdated
"type": ["null", "number"]
},
"customer.pay_per_conversion_eligibility_failure_reasons": {
"type": ["null", "array"]
Copy link
Contributor

Choose a reason for hiding this comment

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

should we describe the schema of the array as well using items? same question for all array type fields

airbyte-integrations/connectors/source-google-ads/source_google_ads/google_ads.py Show resolved Hide resolved
sherifnada
sherifnada approved these changes Jul 22, 2021
View reviewed changes
airbyte-integrations/connectors/source-google-ads/source_google_ads/google_ads.py Outdated
query_template = f"SELECT {fields} FROM {from_category} "

if cursor_field:
query_template += f"WHERE {cursor_field} > '{from_date}' AND {cursor_field} < '{to_date}' ORDER BY {cursor_field}"
Copy link
Contributor

Choose a reason for hiding this comment

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

would be better to explicitly say ORDER BY {cursor_field} ASC if possible

@yevhenii-ldv
Copy link
Contributor Author

yevhenii-ldv commented Jul 23, 2021 •
edited by github-actions bot
Loading

/test connector=connectors/source-google-ads

🕑 connectors/source-google-ads https://github.com/airbytehq/airbyte/actions/runs/1059460353
❌ connectors/source-google-ads https://github.com/airbytehq/airbyte/actions/runs/1059460353

@yevhenii-ldv
Copy link
Contributor Author

Should not forget to publish this connector later, when the GitHub test will be normalized.

@yevhenii-ldv yevhenii-ldv merged commit 3e8f12d into master Jul 23, 2021
@yevhenii-ldv yevhenii-ldv deleted the ykurochkin/source-google-ads-expand-api-support branch July 23, 2021 11:25
@jaimefr
Copy link
Contributor

jaimefr commented Jul 26, 2021

Is this still using the Googe Adwords API or is it already using the Google Ads API? See #4556

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Zirochkaa Zirochkaa Zirochkaa approved these changes

@sherifnada sherifnada sherifnada approved these changes

@htrueman htrueman Awaiting requested review from htrueman

@ChristopheDuong ChristopheDuong Awaiting requested review from ChristopheDuong

@tuliren tuliren Awaiting requested review from tuliren

Assignees
No one assigned
Labels
area/connectors Connector related issues area/documentation Improvements or additions to documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

source google ads exception DATE_RANGE_TOO_NARROW 🚀Source Google Ads: Expand API Support
4 participants
@yevhenii-ldv @jaimefr @sherifnada @Zirochkaa