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

GoogleAdsToGcsOperator sends default headers which is no longer supported by the API #43486

Open
2 tasks done
shervinrad100 opened this issue Oct 29, 2024 · 11 comments · May be fixed by #43515
Open
2 tasks done

GoogleAdsToGcsOperator sends default headers which is no longer supported by the API #43486

shervinrad100 opened this issue Oct 29, 2024 · 11 comments · May be fixed by #43515
Assignees
Labels
area:providers kind:bug This is a clearly a bug provider:google Google (including GCP) related issues

Comments

@shervinrad100
Copy link

shervinrad100 commented Oct 29, 2024

Apache Airflow Provider(s)

google

Versions of Apache Airflow Providers

10.21.1

Apache Airflow version

Version 2.0

Operating System

Debian GNU/Linux 12 (bookworm)

Deployment

Docker-Compose

Deployment details

Pretty standard docker-compose up. In the docker compose mounted the volumes to the standard directories and set some variables to be loaded into xcoms (eg user, gcs_conn_id). Nothing's customised otherwise.

What happened

I wanted to test GoogleAdsToGcsOperator and I wrote a GAQL query to hit the customer_client resource and the task failed with the following error:

Fault: errors {
  error_code {
    request_error: PAGE_SIZE_NOT_SUPPORTED
  }
  message: "Setting the page size is not supported. Search Responses will have fixed page size of \'10000\' rows."
}

I did not specify any page size (this is initialised here)

This was the task I ran in my DAG:

# I get these from xcoms. It's loaded in with docker-compose
DEFAULT_ARGS = {
    # connections
    'gcp_conn_id': 'google_cloud_default', 
    'google_ads_conn_id': 'google_ads_conn',
    'retries': 2
}

GoogleAdsToGcsOperator(
        task_id='get_google_ads_sub_accounts',
        bucket=BUCKET,
        obj=file_path,
        api_version=API_VERSION, # v17
        gzip=False,
        # default_args=DEFAULT_ARGS,
        client_ids=[MCC_ACCOUNT_ID], # MCC accountID
        attributes=[
            "customer_client.descriptive_name"
            , "customer_client.id"
            , "customer_client.manager"
            , "customer.id"
            , "customer_client.hidden"
            , "customer_client.status"
        ],
        query="""
            SELECT customer_client.descriptive_name
                 , customer_client.id
                 , customer_client.manager
                 , customer.id
                 , customer_client.hidden
                 , customer_client.status
            FROM customer_client
            WHERE customer_client.status NOT IN ('CANCELED', 'CLOSED')
            """
    )

What you think should happen instead

a file should be created in my GCS bucket with the request response.

How to reproduce

If you run the following task with your connection ID information you would probably get the same error. Do not set the page_size parameter so that the init func passes the default.

Anything else

The entire request response headers are:

Headers: {
  "google.ads.googleads.v17.errors.googleadsfailure-bin": "\ni\n\u0002\b(\u0012cSetting the page size is not supported. Search Responses will have fixed page size of '10000' rows.\u0012\u0016kUF6eMWJI6-xDnmxB7Dhww",
  "grpc-status-details-bin": "\b\u0003\u0012%Request contains an invalid argument.\u001a\u0001\nDtype.googleapis.com/google.ads.googleads.v17.errors.GoogleAdsFailure\u0012\u0001\ni\n\u0002\b(\u0012cSetting the page size is not supported. Search Responses will have fixed page size of '10000' rows.\u0012\u0016kUF6eMWJI6-xDnmxB7Dhww",
  "request-id": ""
}
Fault: errors {
  error_code {
    request_error: PAGE_SIZE_NOT_SUPPORTED
  }
  message: "Setting the page size is not supported. Search Responses will have fixed page size of \'10000\' rows."
}

Are you willing to submit PR?

  • Yes I am willing to submit a PR!

Code of Conduct

@shervinrad100 shervinrad100 added area:providers kind:bug This is a clearly a bug needs-triage label for new issues that we didn't triage yet labels Oct 29, 2024
Copy link

boring-cyborg bot commented Oct 29, 2024

Thanks for opening your first issue here! Be sure to follow the issue template! If you are willing to raise PR to address this issue please do so, no need to wait for approval.

@dosubot dosubot bot added the provider:google Google (including GCP) related issues label Oct 29, 2024
@bugraoz93
Copy link
Collaborator

bugraoz93 commented Oct 29, 2024

@shervinrad100
Yes, indeed, this was changed in the last release of the API. I saw you want to commit to submitting a PR. I can assign it to you if you want. My only nit is to be careful with older API versions, as the code should be backwards compatible with all versions.

Note: Setting a page_size in version v17 of the Google Ads API will return an error. The earlier versions will change start throwing errors on August 12, 2024. This field will be removed in a future version of the Google Ads API. See the announcement.

https://developers.google.com/google-ads/api/docs/reporting/paging

@bugraoz93 bugraoz93 removed the needs-triage label for new issues that we didn't triage yet label Oct 29, 2024
@bugraoz93
Copy link
Collaborator

bugraoz93 commented Oct 29, 2024

In the documents, it seems like they will drop this for earlier versions as well. Never mind about the backward compatibility part. I think they are already deprecating older versions of their API.
https://developers.google.com/google-ads/api/docs/sunset-dates

@shervinrad100
Copy link
Author

shervinrad100 commented Oct 29, 2024

I saw you want to commit to submitting a PR. I can assign it to you if you want

Yes please would love to contribute and this seems like a quick fix anyway.

I was thinking of simply removing the default assignment. If you want backward compatibility then I can add the following to the init:

def __init__(...
...
self.api_version = api_version
if self.api_version < 17:
    self.page_size = page_size or 10000
    # warning message on deprecation. something like:
    # warnings.warn("Setting a page_size in version v17 of the Google Ads API will return an error. The earlier versions will change start throwing errors on August 12, 2024.")

   

@potiuk
Copy link
Member

potiuk commented Oct 29, 2024

I was thinking of simply removing the default assignment.

This is already done and (just) merged via #43474

@shervinrad100
Copy link
Author

@potiuk I might be wrong here but I believe #43474 would force us to declare the API version but would still assign page_size as default which can cause the error.

I believe the page_size default assignment should also be removed.

@bugraoz93
Copy link
Collaborator

This is already done and (just) merged via #43474

I think they deprecated this part where we are passing the page_size in the API call. ads_to_gcs.py

@bugraoz93
Copy link
Collaborator

If you want backward compatibility then I can add the following to the init:

I think we already passed the 12 of August 2024 so I don't think we should support any backward compatibility in this case. This should already deprecated for all the APIs. Maybe we don't even need to pass any page_size at all.

@shervinrad100
Copy link
Author

shervinrad100 commented Oct 30, 2024

@bugraoz93 If you assign the issue to me I'd be happy to push a PR today

@potiuk
Copy link
Member

potiuk commented Oct 30, 2024

Feel free.

@shervinrad100 shervinrad100 linked a pull request Oct 30, 2024 that will close this issue
@shervinrad100
Copy link
Author

ready for your review ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers kind:bug This is a clearly a bug provider:google Google (including GCP) related issues
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants