fix(source-facebook-marketing): apply top-level include_incrementality fallback to custom insight streams#76116
Conversation
…y fallback to custom insight streams Co-Authored-By: bot_apk <apk@cognition.ai>
🤖 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:
|
Co-Authored-By: bot_apk <apk@cognition.ai>
👋 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
|
|
|
Deploy preview for airbyte-docs ready! ✅ Preview Built with commit 8dd6a8f. |
|
↪️ Triggering |
|
🔍 Prove-Fix Validation:
|
| Verb | Result | Details |
|---|---|---|
SPEC |
✅ | No difference |
CHECK |
✅ | No difference |
DISCOVER |
✅ | No difference |
READ |
Both versions failed identically (expired GSM creds). No regression. |
Unit Test Coverage
PR includes 4 parametrized tests covering all (config, insight) boolean combinations for include_incrementality — all pass.
Live Connection Tests
Not performed — not required. Connector is not on the forced OAuth list, fix is non-breaking and reversible, and regression tests + unit tests provide sufficient evidence.
Evidence Details
Fix Analysis: One-line change at source.py:356 follows the identical fallback pattern used on adjacent lines 353-354 for insights_lookback_window and insights_job_timeout.
READ dev) and control (5.2.4) failed with exit code 1, identical message counts (254 LOG, 97 TRACE), 0 records. This indicates expired GSM integration test credentials or a transient Facebook API issue — unrelated to this PR.
Pre-release Image: airbyte/source-facebook-marketing:5.2.5-preview.8dd6a8f (publish workflow)
Recommendation
✅ Safe to merge. No regressions detected. Post-merge rollout will be monitored by standard automation.
|
Reviewing PR for connector safety and quality.
|
|
↪️ Triggering Reason: |
AI PR Review ReportReview Action: NO ACTION (NOT ELIGIBLE)
📋 PR Details & EligibilityConnector & PR InfoConnector(s): Auto-Approve EligibilityEligible: No Review Action DetailsNO ACTION (NOT ELIGIBLE) — All gates pass but the PR contains functional code changes and is not eligible for auto-approval. No PR review submitted. Human review required.
🔍 Gate Evaluation DetailsGate-by-Gate Analysis
PR Hygiene
Code Hygiene
Test Coverage
Code Security
Per-Record Performance
Breaking Dependencies
Backwards Compatibility
Forwards Compatibility
Behavioral Changes
Live / E2E Tests
📚 Evidence ConsultedEvidence
|
What
Resolves https://github.com/airbytehq/oncall/issues/11903:
When
include_incrementalityis enabled at the top-level config, it only applied to built-in insight streams — custom insight streams ignored the top-level value and only used their own per-insight setting (which defaults toFalse).How
One-line fix in
source.py:356: addor config.include_incrementalityfallback, matching the existing pattern used on adjacent lines forinsights_lookback_windowandinsights_job_timeout.Version bumped from 5.2.4 → 5.2.5 (patch).
Review guide
source_facebook_marketing/source.py— the one-line fix (line 356)unit_tests/test_source.py— parametrized test covering all 4 boolean combinationsNote: The
oroperator on booleans means a custom insight cannot explicitly disable incrementality when the top-level config enables it. This is consistent with howinsights_lookback_windowandinsights_job_timeoutalready behave on the adjacent lines.Human review checklist
orfallback semantics are acceptable (top-levelTruealways wins over per-insightFalse)action_attribution_windowsis a valid proxy —include_incrementalityis consumed in__init__to append"incrementality"to the windows list but is not stored as a public attributeUser Impact
Users who enable
include_incrementalityat the top-level config will now see it correctly applied to custom insight streams, not just built-in ones.Can this PR be safely reverted and rolled back?
Link to Devin session: https://app.devin.ai/sessions/4918e123cd5f49e1999bcf3f93fe415d