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: added handling for 401 error while parsing response. added metrics.cost_micros to ad_groups stream #33494

Merged
merged 20 commits into from Jan 9, 2024

Conversation

darynaishchenko
Copy link
Collaborator

@darynaishchenko darynaishchenko commented Dec 14, 2023

What

resolved: https://github.com/airbytehq/oncall/issues/3466
401 Request is missing required authentication credential

resolved: #19455

How

Added config error in case of 401 error. Updated ad_groups schema.

@darynaishchenko darynaishchenko self-assigned this Dec 14, 2023
Copy link

vercel bot commented Dec 14, 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 9, 2024 3:54pm

Copy link
Contributor

github-actions bot commented Dec 14, 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.

@octavia-squidington-iv octavia-squidington-iv requested a review from a team December 18, 2023 12:26
@darynaishchenko darynaishchenko changed the title 🐛 Source Google Ads: added handling for 401 error while parsing response 🐛 Source Google Ads: added handling for 401 error while parsing response. added metrics.cost_micros to ad_groups stream Dec 18, 2023
Copy link
Contributor

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.

try:
yield self.google_ads_client.parse_single_result(self.get_json_schema(), result)
except Unauthenticated:
raise AirbyteTracedException(
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible to incorporate this error handling into the traced_exception function located in utils.py? This would centralize error handling, making it more manageable and consistent across the connector.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated with traced_exception

@darynaishchenko darynaishchenko merged commit 0764678 into master Jan 9, 2024
23 of 24 checks passed
@darynaishchenko darynaishchenko deleted the daryna/source-google-ads/oc-auth-error branch January 9, 2024 16:35
@tolik0
Copy link
Contributor

tolik0 commented Jan 11, 2024

@darynaishchenko, the update to add a new field to the ad_group stream is leading to errors for users with the connector set up using a manager account. This is because metrics are not accessible for manager accounts. Here's a snippet from the log indicating the issue:

2024-01-11 18:05:50 source > Marking stream ad_group as STARTED
2024-01-11 18:05:50 source > Syncing stream: ad_group 
2024-01-11 18:05:51 source > Request made: ClientCustomerId: 9016006472, Host: googleads.googleapis.com, Method: /google.ads.googleads.v15.services.GoogleAdsService/Search, RequestId: 07AX5lxXUaVGq_jhuKSV3w, IsFault: True, FaultMessage: Metrics cannot be requested for a manager account. To retrieve metrics, issue separate requests against each client account under the manager account.
2024-01-11 18:05:51 source > Finished syncing ad_group
2024-01-11 18:05:51 source > SourceGoogleAds runtimes:
Syncing stream ad_group 0:00:00.317514
2024-01-11 18:05:51 source > (<_InactiveRpcError of RPC that terminated with:
        status = StatusCode.INVALID_ARGUMENT
        details = "Request contains an invalid argument."
        debug_error_string = "UNKNOWN:Error received from peer ipv4:142.251.208.170:443 {created_time:"2024-01-11T18:05:51.006095582+00:00", grpc_status:3, grpc_message:"Request contains an invalid argument."}"
>, <_InactiveRpcError of RPC that terminated with:
        status = StatusCode.INVALID_ARGUMENT
        details = "Request contains an invalid argument."
        debug_error_string = "UNKNOWN:Error received from peer ipv4:142.251.208.170:443 {created_time:"2024-01-11T18:05:51.006095582+00:00", grpc_status:3, grpc_message:"Request contains an invalid argument."}"
>, errors {
  error_code {
    query_error: REQUESTED_METRICS_FOR_MANAGER

Since blocking this stream for manager accounts impacts other customers' data, I suggest we revert this change.

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
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.

Source Google Ads: ad_groups stream does not have cost_micros field
4 participants