source-facebook-marketing-native: new connector#3409
Conversation
There was a problem hiding this comment.
Pull Request Overview
This WIP PR introduces a comprehensive Facebook Marketing Native connector implementation. It provides a source connector for Facebook Marketing API data with comprehensive resource coverage, insights management, and incremental syncing capabilities.
- Implements full Facebook Marketing API connector with 18+ resource types (accounts, campaigns, ads, insights, etc.)
- Adds sophisticated job management for async insights with retry logic and status polling
- Includes permission-aware field filtering and comprehensive error handling
Reviewed Changes
Copilot reviewed 33 out of 36 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| utils.py | Date formatting utilities for Facebook API date handling |
| resources.py | Resource factory functions and validation logic for all Facebook resources |
| permissions.py | Permission management system for field-level access control |
| models.py | Pydantic models for all Facebook resources with type coercion and validation |
| job_manager.py | Async job management for Facebook insights with retry and polling |
| fields.py | Field type mappings and validation constants for Facebook API |
| client.py | HTTP client with pagination, error handling, and permission filtering |
| auth.py | Authentication validation for Facebook credentials and account access |
| api.py | API functions for fetching different resource types with incremental logic |
| main.py | Entry point for running the connector |
| init.py | Main connector class implementing the capture interface |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if isinstance(dt, datetime): | ||
| return dt.strftime(DATE_FORMAT) | ||
| elif isinstance(dt, date): | ||
| return dt.strftime(DATE_FORMAT) |
There was a problem hiding this comment.
The function has redundant code - both datetime and date branches execute identical logic. This can be simplified since datetime is a subclass of date.
| if isinstance(dt, datetime): | |
| return dt.strftime(DATE_FORMAT) | |
| elif isinstance(dt, date): | |
| return dt.strftime(DATE_FORMAT) | |
| if isinstance(dt, date): | |
| return dt.strftime(DATE_FORMAT) |
Alex-Bair
left a comment
There was a problem hiding this comment.
High level, it looks pretty good to me. Nice job figuring out how to hit the Facebook API and building an async client to do so, that was definitely difficult.
I had a couple general comments. I tried not to go through with a super close eye like I usually do, but I did leave a couple specific comments if I came across something & figured I'd say something so I don't have to remember to look for them again later.
1b10b9b to
08d190d
Compare
|
@Alex-Bair I've updated the PR with my latest work, more detailed commit and PR description. I will update the PR description with any other notes/information along with another commit for adding to CI. Please let me know if you think this PR would be easier to review if I broke the commit down into more granular commits or anything else. I'll try to help in any way I can since this is such a large PR and first time doing an "online" migration of imported connectors. |
|
@Alex-Bair I've addressed all of your feedback too. It may be worth chatting through that next week if it isn't immediately clear what I did. Otherwise, I will try to post follow up comments next week on what was done to address feedback! Have a great weekend 🎉 |
b526de9 to
9ce379d
Compare
Alex-Bair
left a comment
There was a problem hiding this comment.
I had some more questions after reviewing a bit closer. LMK if I wasn't clear in some of them or it'd be helpful to meet up to talk through any of them.
|
|
||
| class AdAccount(FacebookResource): | ||
| name: ClassVar[str] = ResourceName.AD_ACCOUNT | ||
| endpoint: ClassVar[str] = "/" |
There was a problem hiding this comment.
It seems a little off that AdAccount's endpoint contains a leading / but none of the other models' endpoints do. Should this be an empty string instead to avoid having a double // constructed in the URL for fetching ad accounts?
There was a problem hiding this comment.
yes it should be, however, it didn't cause any issues surprisingly
e41ef98 to
44bfb1a
Compare
|
@Alex-Bair I will clean up the commits into 2 commits after you have a chance to review again. Not sure what happened after I rebased locally. It got my commit history way out of wack, adding |
Alex-Bair
left a comment
There was a problem hiding this comment.
LGTM % the outstanding items you've mentioned (cleaning up commits, removing the acmeCo directory) and fixing spec snapshot tests that are failing due to the change of title to the various OAuth classes.
| model_config = ConfigDict( | ||
| title="OAuth", | ||
| ) |
There was a problem hiding this comment.
I like these additions! However, they cause some of the connectors' spec snapshot tests to fail, like source-looker's. If you're adding these titles to the various OAuth classes, please also update the snapshots for the tests that are failing because of the title change.
| - "source-facebook-marketing-native/**" | ||
| - "source-navan/**" | ||
| - "source-facebook-marketing-native/**" |
There was a problem hiding this comment.
Noting that source-facebook-marketing-native and source-navan are listed twice in this file. I'm pretty sure this will get resolved when you clean up the commits.
44bfb1a to
27a7d5a
Compare
…ntialsOAuth2Credentials` class
This is a native Facebook Marketing capture connector that replaces the current imported Airbyte connector. The connector includes 17 resource types organized into multiple categories: - Full refresh resources: ad_account, ad_creatives, custom_conversions - Standard incremental resources: campaigns, ad_sets, ads, activities - Reverse-chronological incremental resources: images, videos - Insights resources with various breakdowns: ads_insights (base), ads_insights_age_and_gender, ads_insights_country, ads_insights_region, ads_insights_dma, ads_insights_platform_and_device, ads_insights_action_type - Custom insights: user-configurable insights with custom fields, breakdowns, and action breakdowns Key technical features include: - Async and generalized Client wrapper around Facebook's API that handles pagination, filtering, and other nuances of the API - Async job manager for Facebook's Insights API that handles job submission, polling, and result pagination with configurable retry logic - Comprehensive field enumerations with 600+ marketing API fields and extensive breakdown/action breakdown options for insights customization - Permission validation during discovery to ensure proper access to ad accounts and API endpoints - Support for include_deleted configuration to capture soft-deleted resources - Lookback window support for insights to handle eventual consistency The connector uses Facebook Marketing API v23.0 with proper OAuth2 authentication. The connector should support future Facebook Marketing API upgrades with clearly defined models, abstractions, and separation of concern.
8ed1f55 to
b37edbd
Compare
b37edbd to
dc2f4f3
Compare
Documentation for estuary/connectors#3409
Documentation for estuary/connectors#3409
Documentation for estuary/connectors#3409
Documentation for estuary/connectors#3409
Documentation for estuary/connectors#3409
Documentation for estuary/connectors#3409
Description:
This is an initial implementation of a native Facebook Marketing capture connector. Long-term, this will replace the current imported Airbyte Facebook Marketing connector.
The connector includes 17 resource types organized into multiple categories:
Key technical features include:
The connector uses Facebook Marketing API v23.0 with proper OAuth2 authentication and leverages. The connector should support future Facebook Marketing API upgrades with clearly defined models, abstractions, and separation of concern.
Workflow steps:
Once this PR is ready and merged there will be some follow up work to migrate existing Production capture tasks that are using the Airbyte
source-facebook-marketingconnector to use this newsource-facebook-marketing-nativeconnector. This will be a fully "Online-Migration" where downstream collections and their schemas, Materializations, and destination systems should not be impacted. I will work with the support team to go through this process and ensure a smooth transition for all users.This migration process will be as follows:
EndpointConfigandResourceConfigspecifications from the Airbyte format to match our native format with the necessary removal of removed config options, addition of intervals, etc.flowctlDocumentation links affected:
This PR will require documentation updates in separate follow up PRs once this PR is ready for merge:
source-facebook-marketingconnector in docs.source-facebook-marketing-nativealong with other release steps to make this connector available in the Production UINotes for reviewers:
TBD (will provide more info on testing, etc. next week)