feature(examples): Add Admob connector #458
Conversation
fivertran-karunveluru
added
the
hackathon
🧹 Python Code Quality Check✅ No issues found in Python Files. This comment is auto-updated with every commit. |
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new AdMob connector for syncing Google AdMob advertising data including publisher accounts and network reports. The connector implements OAuth2 authentication with automatic token refresh, memory-efficient streaming patterns, and incremental synchronization using date-based cursors. While the implementation demonstrates good understanding of API integration patterns, there are several critical SDK compliance issues and unused configuration parameters that need to be addressed.
Key changes:
- New AdMob connector with OAuth2 authentication and retry logic with exponential backoff
- Streaming data processing for accounts and network reports with checkpointing
- Configuration template and comprehensive documentation
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 20 comments.
| File | Description |
|---|---|
| connectors/admob/connector.py | Main connector implementation with OAuth2, API request handling, data mapping, and sync orchestration |
| connectors/admob/configuration.json | Configuration template with OAuth credentials and sync parameters |
| connectors/admob/README.md | Documentation covering connector overview, authentication, features, and configuration |
| README.md | Updated connector list to include AdMob example |
| def schema(configuration: dict): | ||
| """ | ||
| Define database schema with table names and primary keys for the connector. | ||
| This function specifies the destination tables and their primary keys for Fivetran to create. | ||
|
|
||
| Args: | ||
| configuration: Configuration dictionary (not used but required by SDK). | ||
|
|
||
| Returns: | ||
| list: List of table schema dictionaries with table names and primary keys. | ||
| """ |
There was a problem hiding this comment.
The schema() function is missing the required standard docstring. It must use this exact format:
def schema(configuration: dict):
"""
Define the schema function which lets you configure the schema your connector delivers.
See the technical reference documentation for more details on the schema function:
https://fivetran.com/docs/connectors/connector-sdk/technical-reference#schema
Args:
configuration: a dictionary that holds the configuration settings for the connector.
"""| # The 'upsert' operation is used to insert or update data in the destination table. | ||
| # The op.upsert method is called with two arguments: | ||
| # - The first argument is the name of the table to upsert the data into. | ||
| # - The second argument is a dictionary containing the data to be upserted, | ||
| op.upsert(table="accounts", data=account) |
There was a problem hiding this comment.
The comment before op.upsert() doesn't follow the required format. It must be exactly:
# The 'upsert' operation is used to insert or update data in the destination table.
# The first argument is the name of the destination table.
# The second argument is a dictionary containing the record to be upserted.
op.upsert(table="accounts", data=account)The current comment has additional text that should be removed.
| f"Sync completed successfully. Processed {account_count} accounts and {report_count} report records." | ||
| ) | ||
|
|
||
| except Exception as e: |
There was a problem hiding this comment.
Generic exception catching without specific exception types. According to SDK guidelines, you should catch specific exceptions instead of bare except Exception:.
The code should catch specific exceptions:
except (RuntimeError, requests.exceptions.RequestException, ValueError) as e:
log.severe(f"Sync failed: {str(e)}")
raise RuntimeError(f"Failed to sync AdMob data: {str(e)}")This makes error handling more explicit and prevents catching unexpected exceptions like KeyboardInterrupt.
| except Exception as e: | |
| except (RuntimeError, requests.exceptions.RequestException, ValueError) as e: |
| from fivetran_connector_sdk import Operations as op | ||
|
|
||
| # Private constants (use __ prefix) | ||
| __INVALID_LITERAL_ERROR = "invalid literal" |
There was a problem hiding this comment.
The __INVALID_LITERAL_ERROR constant is defined but never used in the code. This is dead code that should be removed to improve maintainability.
| __INVALID_LITERAL_ERROR = "invalid literal" |
| "client_id": "<YOUR_ADMOB_OAUTH_CLIENT_ID>", | ||
| "client_secret": "<YOUR_ADMOB_OAUTH_CLIENT_SECRET>", | ||
| "refresh_token": "<YOUR_ADMOB_OAUTH_REFRESH_TOKEN>", | ||
| "sync_frequency_hours": "<YOUR_ADMOB_API_SYNC_FREQUENCY_HOURS>", |
There was a problem hiding this comment.
The sync_frequency_hours field is defined in the configuration but is never used in connector.py. This is an unused configuration parameter that should be removed to avoid confusion. The connector doesn't implement any sync frequency logic - it runs whenever triggered by Fivetran.
| ## Additional files | ||
| The connector includes several additional files to support functionality, testing, and deployment: | ||
|
|
||
| - `requirements.txt` – Python dependency specification for AdMob API integration and connector requirements including faker for mock testing. | ||
|
|
||
| - `configuration.json` – Configuration template for API credentials and connector parameters (should be excluded from version control). | ||
|
|
||
|
|
There was a problem hiding this comment.
The "Additional files" section also mentions configuration.json, but according to the SDK guidelines, this section should only list additional Python files beyond connector.py. Configuration files like configuration.json and requirements.txt are standard files and shouldn't be listed here unless they contain something unusual worth explaining.
If there are no additional Python files (like helper modules), the "Additional files" section should be removed entirely.
| ## Additional files | |
| The connector includes several additional files to support functionality, testing, and deployment: | |
| - `requirements.txt` – Python dependency specification for AdMob API integration and connector requirements including faker for mock testing. | |
| - `configuration.json` – Configuration template for API credentials and connector parameters (should be excluded from version control). |
| Raises: | ||
| RuntimeError: If sync fails due to API errors or configuration issues. | ||
| """ | ||
| log.info("Starting AdMob API connector sync") |
There was a problem hiding this comment.
Missing required first log statement in update() function. According to SDK guidelines, the update() function MUST start with:
log.warning("Example: <CATEGORY> : <EXAMPLE_NAME>")For this connector, it should be:
log.warning("Example: connectors : admob")This should be the very first statement in the update function, before any other code.
| # The 'upsert' operation is used to insert or update data in the destination table. | ||
| # The op.upsert method is called with two arguments: | ||
| # - The first argument is the name of the table to upsert the data into. | ||
| # - The second argument is a dictionary containing the data to be upserted, | ||
| op.upsert(table="network_reports", data=report) |
There was a problem hiding this comment.
The comment before op.upsert() doesn't follow the required format. It must be exactly:
# The 'upsert' operation is used to insert or update data in the destination table.
# The first argument is the name of the destination table.
# The second argument is a dictionary containing the record to be upserted.
op.upsert(table="network_reports", data=report)The current comment has additional text that should be removed.
| ## Configuration file | ||
| ```json | ||
| { | ||
| "client_id": "<YOUR_ADMOB_OAUTH_CLIENT_ID>", | ||
| "client_secret": "<YOUR_ADMOB_OAUTH_CLIENT_SECRET>", | ||
| "refresh_token": "<YOUR_ADMOB_OAUTH_REFRESH_TOKEN>", | ||
| "sync_frequency_hours": "<YOUR_ADMOB_API_SYNC_FREQUENCY_HOURS>", | ||
| "initial_sync_days": "<YOUR_ADMOB_API_INITIAL_SYNC_DAYS>", | ||
| "max_records_per_page": "<YOUR_ADMOB_API_MAX_RECORDS_PER_PAGE>", | ||
| "request_timeout_seconds": "<YOUR_ADMOB_API_REQUEST_TIMEOUT_SECONDS>", | ||
| "retry_attempts": "<YOUR_ADMOB_API_RETRY_ATTEMPTS>", | ||
| "enable_incremental_sync": "<YOUR_ADMOB_API_ENABLE_INCREMENTAL_SYNC>", | ||
| "enable_debug_logging": "<YOUR_ADMOB_API_ENABLE_DEBUG_LOGGING>" | ||
| } | ||
| ``` |
There was a problem hiding this comment.
The Configuration file section is missing the required statement about not checking configuration.json into version control. According to the template, this section must end with:
Note: Ensure that the `configuration.json` file is not checked into version control to protect sensitive information.This should be added after the JSON code block and before the "Configuration parameters" section.
| ## Additional files | ||
| The connector includes several additional files to support functionality, testing, and deployment: | ||
|
|
||
| - `requirements.txt` – Python dependency specification for AdMob API integration and connector requirements including faker for mock testing. | ||
|
|
||
| - `configuration.json` – Configuration template for API credentials and connector parameters (should be excluded from version control). | ||
|
|
||
|
|
There was a problem hiding this comment.
The "Additional files" section mentions requirements.txt with dependencies including "faker for mock testing", but there is no requirements.txt file in this PR. According to the README.md content (line 54), this connector doesn't require any additional packages.
Either:
- Remove the "Additional files" section entirely if there are no additional Python files beyond connector.py, OR
- If requirements.txt exists, update the description to match the actual contents
The current description is inconsistent with the "Requirements file" section which states no additional packages are needed.
| ## Additional files | |
| The connector includes several additional files to support functionality, testing, and deployment: | |
| - `requirements.txt` – Python dependency specification for AdMob API integration and connector requirements including faker for mock testing. | |
| - `configuration.json` – Configuration template for API credentials and connector parameters (should be excluded from version control). |
varundhall
left a comment
varundhall
left a comment
There was a problem hiding this comment.
Same as #256 (review)
fivetran-chinmayichandrasekar
left a comment
fivetran-chinmayichandrasekar
left a comment
There was a problem hiding this comment.
Left a couple of suggestions. Thanks.
| # AdMob API Connector Example | ||
|
|
||
| ## Connector overview | ||
| This connector syncs advertising data from Google AdMob API including publisher accounts, network reports, and mediation analytics. It fetches ad performance metrics such as estimated earnings, ad requests, impressions, clicks, and detailed breakdowns by app, ad unit, country, platform, and ad type. The connector supports incremental synchronization using date-based cursors and handles OAuth2 authentication with automatic token refresh. |
There was a problem hiding this comment.
| This connector syncs advertising data from Google AdMob API including publisher accounts, network reports, and mediation analytics. It fetches ad performance metrics such as estimated earnings, ad requests, impressions, clicks, and detailed breakdowns by app, ad unit, country, platform, and ad type. The connector supports incremental synchronization using date-based cursors and handles OAuth2 authentication with automatic token refresh. | |
| This connector syncs advertising data from the Google AdMob API, including publisher accounts, network reports, and mediation analytics. It fetches ad performance metrics, including estimated earnings, ad requests, impressions, and clicks, along with detailed breakdowns by app, ad unit, country, platform, and ad type. The connector supports incremental synchronization using date-based cursors and handles OAuth2 authentication with automatic token refresh. |
| ## Pagination | ||
| AdMob API uses report-based data retrieval with streaming response handling (refer to `get_network_reports` function). The connector processes report data as it's received from the API without accumulating large datasets in memory. Generator-based processing prevents memory accumulation for large advertising datasets. |
There was a problem hiding this comment.
| ## Pagination | |
| AdMob API uses report-based data retrieval with streaming response handling (refer to `get_network_reports` function). The connector processes report data as it's received from the API without accumulating large datasets in memory. Generator-based processing prevents memory accumulation for large advertising datasets. | |
| ## Pagination | |
| The AdMob API uses report-based data retrieval with streaming response handling (refer to the `get_network_reports` function). The connector processes report data as it's received from the API without accumulating large datasets in memory. Generator-based processing prevents memory accumulation for large advertising datasets. |
3 participants
AdMob Connector
Created: 2025-10-31
Business Owner: Marketing Analytics & Revenue Operations Team
Technical Owner: Data Engineering Team
Last Updated: 2025-10-31
Business Context
Technical Context
Operational Context
API-Specific Details
/accounts- Publisher account information and settings/accounts/{publisher_id}/networkReport:generate- Network performance reportsData Schema Overview
Data Replication Expectations
Operational Requirements
Rate Limiting Strategy
Data Quality Considerations
Integration Points
Disaster Recovery
Compliance & Security
Performance Optimization
Troubleshooting Guide
Configuration Parameters
client_id: OAuth2 client identifier from Google Cloud Consoleclient_secret: OAuth2 client secret from Google Cloud Consolerefresh_token: OAuth2 refresh token for automatic access token renewalsync_frequency_hours: Incremental sync frequency (default: 4 hours)initial_sync_days: Historical data range for initial sync (default: 90 days, max: 365)max_records_per_page: Batch size for checkpointing (default: 100, range: 1-1000)request_timeout_seconds: HTTP request timeout (default: 30 seconds)retry_attempts: Number of retry attempts for failed requests (default: 3)enable_incremental_sync: Enable date-based incremental sync (default: true)enable_debug_logging: Enable detailed logging (default: false)Data Transformation Details
Checklist
Some tips and links to help validate your PR:
fivetran debugcommand.