Repository navigation
Add support for multiple AdAccount when getting Bulk Facebook Report from Facebook Ads Hook #19269
Description
Description
Currently, the Facebook Ads Hook only supports one account_id for every hook(facebook_conn_id). It would be great if we could pass multiple account_id from the Facebook connection(facebook_conn_id). When multiple account_id is provided in the connection, we could support getting insights from one facebook_conn_id for all of them rather than creating a Facebook connection for every account_id.
Use case/motivation
We are using FacebookAdsReportToGcsOperator to export our Facebook Ad data since our infrastructure is on the Google Cloud Platform products and services. This feature will prevent us to create and maintain a connection per account id which will lead us to provide a feature to export multiple CSV files from one connection(one task in a DAG|one operator in a DAG) with one operator. In addition, this feature should not prevent adding multiple Facebook connections per account id to use as it is.
Related issues
No response
Are you willing to submit a PR?
- Yes I am willing to submit a PR!
Code of Conduct
- I agree to follow this project's Code of Conduct
Activity
boring-cyborg commentedon Oct 28, 2021
Thanks for opening your first issue here! Be sure to follow the issue template!
mik-laj commentedon Oct 28, 2021
@randr97 WDYT?
randr97 commentedon Oct 28, 2021
@mik-laj This looks like a good idea to me. Have not been really updated with FB ads in sometime but @bugraoz93 my only concern would be rate limiting and making sure we are handling that when you pull multiple reports. If we do it serially then I think we should be pretty much sorted!
bugraoz93 commentedon Oct 29, 2021
Hey @mik-laj, @randr97, and @eladkal, thank you for all of your quick responses and assistance.
@randr97 That is one of my concerns too. I agree with your idea about serial reporting. I will do it like that. If it is okay for you too, I am also thinking of updating the default API version to 11.0 since I will do some changes on the hook. It could help people who forget to put the API version to the operator. Otherwise, Facebook Business API is throwing an error to use 11.0 instead.
mik-laj commentedon Oct 29, 2021
Recently, we made changes about the supported API version: #18883 Is it related?
randr97 commentedon Oct 29, 2021
@bugraoz93 perfect!! FB API 11.0 would be amazing. FB rapidly update their APIs (once in couple months) and it makes it hard for us to propagate those upgrades. Quick suggestion - jus make sure the requirements are a little relaxed in
setup.pycus updating requirements in airflow can get a lil tricky!randr97 commentedon Oct 29, 2021
@bugraoz93 just take a pull and you should be good
Thanks for that @mik-laj
bugraoz93 commentedon Oct 29, 2021
Great news! Thanks, @randr97 and @mik-laj. I will take a pull. I will try to be more careful about looking at Pull Requests before suggesting.
bugraoz93 commentedon Oct 31, 2021
Hey @randr97 and @mik-laj, I think the default API version update reverted in #18996. When I pull from the main branch, I am getting
api_versionas an Optional constructor parameter assigned asNone. So, I will not change that part. I wanted to inform you all.eladkal commentedon Oct 31, 2021
The change was to take defaults from the upstream package - whatever that may be. As soon as they update Airflow allign with it. If user needs a specific version he/she can use the api_version param.
See review comments: #18883 (comment)
bugraoz93 commentedon Oct 31, 2021
Hey @eladkal, thank you, yes I saw your changes. I liked the idea behind it. Since Facebook can update its API regularly, we need to continuously update our default version to the latest acceptable one. I think this change has eliminated that maintenance cost in the future. So that, I wanted to state that I will leave that logic as it is with your changes.