feat(source-facebook-marketing): add incremental sync to ad_creatives_from_ads - #83704
Conversation
Co-Authored-By: mark.grivnin <mark.grivnin@airbyte.io>
Co-Authored-By: mark.grivnin <mark.grivnin@airbyte.io>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
👋 Greetings, Airbyte Team Member!Here are some helpful tips and reminders for your convenience. 💡 Show Tips and TricksPR Slash CommandsAirbyte Maintainers (that's you!) can execute the following slash commands on your PR:
📚 Show Repo GuidanceHelpful Resources
|
Co-Authored-By: mark.grivnin <mark.grivnin@airbyte.io>
|
Note Autopilot progressive rollouts are not enabled for the following modified connector(s):
This is a courtesy heads-up only — it does not block merge or fail any check. |
|
|
Deploy preview for airbyte-docs ready!
Deployed with vercel-action |
|
|
↪️ Triggering Reason: PR is ready for review and no |
Reviewing PR for connector safety and quality.
|
🛡️ AI PR Review Report🟡 Review Action: NO ACTION (INCONCLUSIVE)
🔶 Risk Level: 3/5Cursor/state logic added to an existing stream ( 🔧 Remediation RequiredLive / E2E Tests (UNKNOWN) — no
📋 PR DetailsConnector(s): 🔍 Gate Evaluation DetailsGate-by-Gate Analysis
Backwards compatibility analysisChanged files matching breaking-change patterns:
Forwards compatibility analysisState/cursor keywords are present in the diff (
Behavioral changes analysisGrepping the diff hunks for operational-risk keywords, the only literal match is CI analysisThe single failing check-run on this HEAD SHA is
📚 Evidence ConsultedEvidence
❓ How to RespondResolving a Failing GateEach non-PASS gate above lists concrete remediation. Apply it, push, then re-run Written explanations do not change a gate verdict. If you believe a gate is a false positive or it cannot be remediated as written, request review from a human maintainer and explain the situation to them in a PR comment — a human reviewer can approve or merge over a bot FAIL. Never edit the PR description to argue with the bot; the description is how reviewers learn what the PR does. |
|
|
What
ad_creatives_from_adsis full refresh. Every sync pages the entire ads edge for an account, then issues oneAdCreative.api_get()per unique creative ID — so the per-creative fan-out is proportional to the account's whole creative catalogue on every run.For one multi-tenant customer this stream is 59% of total Facebook Marketing stream time across 30 days, and ~99.9% of their worst attempts (one job: 19.5h wall clock, of which this stream is 70,055s and every other stream combined is 44s). They're moving to a 3-hour cadence, which the current behaviour can't fit.
This makes the parent ads read incremental, so the fan-out scales with recently changed ads instead of all ads.
How
AdCreativesFromAdsis reparented ontoFBMarketingIncrementalStream. It already declaredentity_prefix = "ad", so the inherited_state_filteremits the same request theAdsstream has used in production for years:{"filtering": [{"field": "ad.updated_time", "operator": "GREATER_THAN", "value": <unix ts>}]}request_paramsmerges viadeep_merge, so this clause is appended to the existingad.effective_status IN [...]filter rather than replacing it — asserted by a new test.The awkward part is that the cursor belongs to the parent ad but the emitted record is a creative, and
AdCreativeexposes no timestamp of its own (Meta's AdCreative reference has nocreated_time/updated_timefield). So:["id", "creative", "updated_time"];read_recordsdeliberately bypasses the incremental base'sread_records— that implementation would take the cursor from whatever it yields, which here is a creative;updated_timeis copied onto the emitted creative, so the declaredcursor_fieldactually exists on the record and in the schema;updated_timeskips the state update instead of raisingKeyError.updated_timeis added toad_creatives.jsonbut excluded from the fields requested from the AdCreative API in bothAdCreativesFromAds._get_creative_fields()andAdCreatives.fields(), since it isn't a valid field on that object.Review guide
source_facebook_marketing/streams/streams.py— the only behavioural change. Worth a close look at theFBMarketingStream.read_records(self, ...)call and the state handling around it.source_facebook_marketing/schemas/ad_creatives.json— additiveupdated_time, plus the two exclusion sites that keep it out of the outbound field list.unit_tests/test_streams.py— filter emitted with state; status and cursor clauses coexisting; no date filter on an empty first sync withoutstart_date; cursor copied onto the creative; state advancing to the max parent cursor; missing parentupdated_timenot crashing.integration_tests/configured_catalog.json/future_state.json— the stream was previously absent from the configured catalog entirely, so CAT never exercised it. It now runs and passes in CI.User Impact
Opt-in. Existing connections keep running full refresh until a user switches the stream's sync mode; the first incremental run still does a full pass and seeds state from it.
Known limitation, worth calling out in review: the cursor is the parent ad's. Meta doesn't document
ad.updated_time's semantics, but creatives are near-immutable —POST /{ad_creative_id}accepts onlyaccount_id,adlabels,nameandstatus, so any content change requires creating a new creative and repointing the ad, which necessarily movesad.updated_time. The residual gaps are an in-place rename/status/label change on a creative, and an edit to the page post behindeffective_object_story_id— neither is guaranteed to move the ad's timestamp. Full refresh catches those today; incremental would not.One adjacent improvement left for a separate PR:
_get_creative_fields()reads the full schema rather than the configured catalog, so deselecting fields in the UI doesn't shrink the per-creative requests.Version bumped 6.0.2 → 6.1.0. Additive, so no
breakingChangesentry and no migration guide.Can this PR be safely reverted and rolled back?
Link to Devin session: https://app.devin.ai/sessions/887ed96fb29b4b439476888f89ff39be