🐛 Source FB Marketing: deprecate `INSIGHTS_DAYS_PER_JOB` from connector's specification. #8234

bazarnov · 2021-11-24T15:10:17Z

What

#8027

How

edited source.py
corrected Readme.md

Pre-merge Checklist

Expand the relevant checklist and delete the others.

Updating a connector

Community member or Airbyter

Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
Code reviews completed
Documentation updated
- Connector's README.md
- Connector's bootstrap.md. See description and examples
- Changelog updated in docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
PR name follows PR naming conventions

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
Credentials added to Github CI. Instructions.
/test connector=connectors/<name> command is passing.
New Connector version released on Dockerhub by running the /publish command described here
After the new connector version is published, connector version bumped in the seed directory as described here
Seed specs have been re-generated by building the platform and committing the changes to the seed spec files, as described here

This change is

…rketing-insigths-days-per-job

bazarnov · 2021-11-24T16:00:44Z

/test connector=connectors/source-facebook-marketing

🕑 connectors/source-facebook-marketing https://github.com/airbytehq/airbyte/actions/runs/1500157622
❌ connectors/source-facebook-marketing https://github.com/airbytehq/airbyte/actions/runs/1500157622
🐛 https://gradle.com/s/hn5pthx33go4a
🕑 connectors/source-facebook-marketing https://github.com/airbytehq/airbyte/actions/runs/1500157622
✅ connectors/source-facebook-marketing https://github.com/airbytehq/airbyte/actions/runs/1500157622
Python tests coverage:

	 ---------- coverage: platform linux, python 3.8.10-final-0 -----------
	 Name                                                 Stmts   Miss  Cover
	 ------------------------------------------------------------------------
	 source_acceptance_test/__init__.py                       2      0   100%
	 source_acceptance_test/base.py                          10      4    60%
	 source_acceptance_test/config.py                        75      8    89%
	 source_acceptance_test/conftest.py                     108    108     0%
	 source_acceptance_test/plugin.py                        47     47     0%
	 source_acceptance_test/tests/__init__.py                 4      0   100%
	 source_acceptance_test/tests/test_core.py              200     94    53%
	 source_acceptance_test/tests/test_full_refresh.py       38     27    29%
	 source_acceptance_test/tests/test_incremental.py        69     38    45%
	 source_acceptance_test/utils/__init__.py                 6      0   100%
	 source_acceptance_test/utils/asserts.py                 37      2    95%
	 source_acceptance_test/utils/common.py                  41     24    41%
	 source_acceptance_test/utils/compare.py                 62     25    60%
	 source_acceptance_test/utils/connector_runner.py        82     49    40%
	 source_acceptance_test/utils/json_schema_helper.py     115     14    88%
	 ------------------------------------------------------------------------
	 TOTAL                                                  896    440    51%
	 ---------- coverage: platform linux, python 3.8.10-final-0 -----------
	 Name                                     Stmts   Miss  Cover
	 ------------------------------------------------------------
	 source_facebook_marketing/__init__.py        2      0   100%
	 source_facebook_marketing/api.py            75     17    77%
	 source_facebook_marketing/async_job.py      92     58    37%
	 source_facebook_marketing/common.py         37     11    70%
	 source_facebook_marketing/source.py        112     65    42%
	 source_facebook_marketing/streams.py       239     80    67%
	 ------------------------------------------------------------
	 TOTAL                                      557    231    59%
	 ---------- coverage: platform linux, python 3.8.10-final-0 -----------
	 Name                                     Stmts   Miss  Cover
	 ------------------------------------------------------------
	 source_facebook_marketing/__init__.py        2      0   100%
	 source_facebook_marketing/api.py            75     18    76%
	 source_facebook_marketing/async_job.py      92      1    99%
	 source_facebook_marketing/common.py         37      1    97%
	 source_facebook_marketing/source.py        112     72    36%
	 source_facebook_marketing/streams.py       239     80    67%
	 ------------------------------------------------------------
	 TOTAL                                      557    172    69%

airbyte-integrations/connectors/source-paypal-transaction/source_paypal_transaction/source.py

bazarnov · 2021-11-25T12:14:47Z

/test connector=connectors/source-facebook-marketing

🕑 connectors/source-facebook-marketing https://github.com/airbytehq/airbyte/actions/runs/1503640804
❌ connectors/source-facebook-marketing https://github.com/airbytehq/airbyte/actions/runs/1503640804
🐛 https://gradle.com/s/4jjbfhxzfzjka
🕑 connectors/source-facebook-marketing https://github.com/airbytehq/airbyte/actions/runs/1503640804
✅ connectors/source-facebook-marketing https://github.com/airbytehq/airbyte/actions/runs/1503640804
Python tests coverage:

	 ---------- coverage: platform linux, python 3.8.10-final-0 -----------
	 Name                                                 Stmts   Miss  Cover
	 ------------------------------------------------------------------------
	 source_acceptance_test/__init__.py                       2      0   100%
	 source_acceptance_test/base.py                          10      4    60%
	 source_acceptance_test/config.py                        75      8    89%
	 source_acceptance_test/conftest.py                     108    108     0%
	 source_acceptance_test/plugin.py                        47     47     0%
	 source_acceptance_test/tests/__init__.py                 4      0   100%
	 source_acceptance_test/tests/test_core.py              200     94    53%
	 source_acceptance_test/tests/test_full_refresh.py       38     27    29%
	 source_acceptance_test/tests/test_incremental.py        69     38    45%
	 source_acceptance_test/utils/__init__.py                 6      0   100%
	 source_acceptance_test/utils/asserts.py                 37      2    95%
	 source_acceptance_test/utils/common.py                  41     24    41%
	 source_acceptance_test/utils/compare.py                 62     25    60%
	 source_acceptance_test/utils/connector_runner.py        82     49    40%
	 source_acceptance_test/utils/json_schema_helper.py     115     14    88%
	 ------------------------------------------------------------------------
	 TOTAL                                                  896    440    51%
	 ---------- coverage: platform linux, python 3.8.10-final-0 -----------
	 Name                                     Stmts   Miss  Cover
	 ------------------------------------------------------------
	 source_facebook_marketing/__init__.py        2      0   100%
	 source_facebook_marketing/api.py            75     17    77%
	 source_facebook_marketing/async_job.py      92     58    37%
	 source_facebook_marketing/common.py         37     11    70%
	 source_facebook_marketing/source.py        112     65    42%
	 source_facebook_marketing/streams.py       239     80    67%
	 ------------------------------------------------------------
	 TOTAL                                      557    231    59%
	 ---------- coverage: platform linux, python 3.8.10-final-0 -----------
	 Name                                     Stmts   Miss  Cover
	 ------------------------------------------------------------
	 source_facebook_marketing/__init__.py        2      0   100%
	 source_facebook_marketing/api.py            75     18    76%
	 source_facebook_marketing/async_job.py      92      1    99%
	 source_facebook_marketing/common.py         37      1    97%
	 source_facebook_marketing/source.py        112     72    36%
	 source_facebook_marketing/streams.py       239     80    67%
	 ------------------------------------------------------------
	 TOTAL                                      557    172    69%

…rketing-insigths-days-per-job

bazarnov · 2021-11-26T09:12:32Z

/test connector=connectors/source-facebook-marketing

🕑 connectors/source-facebook-marketing https://github.com/airbytehq/airbyte/actions/runs/1506881247
✅ connectors/source-facebook-marketing https://github.com/airbytehq/airbyte/actions/runs/1506881247
Python tests coverage:

	 ---------- coverage: platform linux, python 3.8.10-final-0 -----------
	 Name                                                 Stmts   Miss  Cover
	 ------------------------------------------------------------------------
	 source_acceptance_test/__init__.py                       2      0   100%
	 source_acceptance_test/base.py                          10      4    60%
	 source_acceptance_test/config.py                        75      8    89%
	 source_acceptance_test/conftest.py                     108    108     0%
	 source_acceptance_test/plugin.py                        47     47     0%
	 source_acceptance_test/tests/__init__.py                 4      0   100%
	 source_acceptance_test/tests/test_core.py              200     94    53%
	 source_acceptance_test/tests/test_full_refresh.py       38     27    29%
	 source_acceptance_test/tests/test_incremental.py        69     38    45%
	 source_acceptance_test/utils/__init__.py                 6      0   100%
	 source_acceptance_test/utils/asserts.py                 37      2    95%
	 source_acceptance_test/utils/common.py                  41     24    41%
	 source_acceptance_test/utils/compare.py                 62     25    60%
	 source_acceptance_test/utils/connector_runner.py        82     49    40%
	 source_acceptance_test/utils/json_schema_helper.py     115     14    88%
	 ------------------------------------------------------------------------
	 TOTAL                                                  896    440    51%
	 ---------- coverage: platform linux, python 3.8.10-final-0 -----------
	 Name                                     Stmts   Miss  Cover
	 ------------------------------------------------------------
	 source_facebook_marketing/__init__.py        2      0   100%
	 source_facebook_marketing/api.py            75     17    77%
	 source_facebook_marketing/async_job.py      92     58    37%
	 source_facebook_marketing/common.py         37     11    70%
	 source_facebook_marketing/source.py        112     65    42%
	 source_facebook_marketing/streams.py       239     80    67%
	 ------------------------------------------------------------
	 TOTAL                                      557    231    59%
	 ---------- coverage: platform linux, python 3.8.10-final-0 -----------
	 Name                                     Stmts   Miss  Cover
	 ------------------------------------------------------------
	 source_facebook_marketing/__init__.py        2      0   100%
	 source_facebook_marketing/api.py            75     18    76%
	 source_facebook_marketing/async_job.py      92      1    99%
	 source_facebook_marketing/common.py         37      1    97%
	 source_facebook_marketing/source.py        112     72    36%
	 source_facebook_marketing/streams.py       239     80    67%
	 ------------------------------------------------------------
	 TOTAL                                      557    172    69%

keu · 2021-11-26T22:03:26Z

@keu fair enough. What's the path forward for deprecating this option, then?

@sherifnada @bazarnov I can't give a perfect solution right now, if I had one, I wouldn't add such a configuration option. The problem with any hard-coded default value is that the user or us can't tune the value.

If we set this value too big we might have too many fails on accounts with a large amount of data and vise versa.
Some connector (don't remember the name) that Dmytro implemented has dynamic slices that adopt the amount of data it fetches to the percentage of fails. Maybe we can decrease the size of the interval each time AsyncJob fails. The challenge here is new jobs for missing subslices.

let say we have jobs with interval = 5
slices/jobs:
ddddd ddddd ddddd

the second slice failed, we retry with interval 2, but 3rd slice is still running, and we need to add missing jobs for the rest of 2nd slice

airbyte-integrations/connectors/source-facebook-marketing/source_facebook_marketing/source.py

…rketing-insigths-days-per-job

bazarnov · 2021-11-30T13:22:39Z

@keu fair enough. What's the path forward for deprecating this option, then?

@sherifnada @bazarnov I can't give a perfect solution right now, if I had one, I wouldn't add such a configuration option. The problem with any hard-coded default value is that the user or us can't tune the value.

If we set this value too big we might have too many fails on accounts with a large amount of data and vise versa. Some connector (don't remember the name) that Dmytro implemented has dynamic slices that adopt the amount of data it fetches to the percentage of fails. Maybe we can decrease the size of the interval each time AsyncJob fails. The challenge here is new jobs for missing subslices.

let say we have jobs with interval = 5 slices/jobs: ddddd ddddd ddddd

the second slice failed, we retry with interval 2, but 3rd slice is still running, and we need to add missing jobs for the rest of 2nd slice

@keu What are the root causes of the possible job failure with date_slices? I've tested using the values of 300 and 1000 for the insights days per job - all are successfully processed on our test account.
Therefore we can try to stay on value of 3 or 5 as a default value. WDYT?

keu · 2021-12-01T00:27:27Z

@keu fair enough. What's the path forward for deprecating this option, then?

@sherifnada @bazarnov I can't give a perfect solution right now, if I had one, I wouldn't add such a configuration option. The problem with any hard-coded default value is that the user or us can't tune the value.
If we set this value too big we might have too many fails on accounts with a large amount of data and vise versa. Some connector (don't remember the name) that Dmytro implemented has dynamic slices that adopt the amount of data it fetches to the percentage of fails. Maybe we can decrease the size of the interval each time AsyncJob fails. The challenge here is new jobs for missing subslices.
let say we have jobs with interval = 5 slices/jobs: ddddd ddddd ddddd
the second slice failed, we retry with interval 2, but 3rd slice is still running, and we need to add missing jobs for the rest of 2nd slice

@keu What are the root causes of the possible job failure with date_slices? I've tested using the values of 300 and 1000 for the insights days per job - all are successfully processed on our test account. Therefore we can try to stay on value of 3 or 5 as a default value. WDYT?

Since we have a very little amount of data testing on our test account is not even close to the production accounts of users.
I don't know an exact root cause, my guess from my experience and singer implementation the more data we need to fetch the more chance that job will fail (probably memory limitation or something) so decreasing insights days per job to 1 day theoretically should help (and helps), but not always (my guess it can be solved by querying fewer aggregation fields).

@avida is currently investigating this issue with the account provided by our customer (Cart).

sherifnada · 2021-12-03T08:50:11Z

@bazarnov let's postpone deprecating the inisght days per job until #8385 is merged

keu · 2022-01-19T09:32:19Z

The deprecation was done in the scope of #8385 because it also touches the logic.
The only missing part is the re-ordering of columns. I think it makes sense to move it there as well and close this PR.

bazarnov · 2022-01-19T10:56:10Z

Closed with reference to this: #8234 (comment)

* Facebook Marketing performance improvement * add comments and little refactoring * fix integration tests with the new config * improve job status handling, limit concurrency to 10 * fix campaign jobs, refactor manager * big refactoring of async jobs, support random order of slices * update source _read_incremental to hook new state logic * fix issues with timeout * remove debugging and clean up, improve retry logic * merge changes from #8234 * fix call super _read_increment * generalize batch execution, add use_batch flag * improve coverage, do some refactoring of spec * update test, remove overrides of source * add split by AdSet * add smaller insights * fix end_date < start_date case * add account_id to PK * add notes * fix new streams * fix reversed incremental stream * update spec.json for SAT * upgrade CDK and bump version Co-authored-by: Dmytro Rezchykov <dmitry.rezchykov@zazmic.com> Co-authored-by: Eugene Kulak <kulak.eugene@gmail.com>

bazarnov added 3 commits November 24, 2021 14:14

first changes

2a37e25

Merge remote-tracking branch 'origin/master' into bazarnov/8027-fb-ma…

9df31ed

…rketing-insigths-days-per-job

removed INSIGHTS_DAYS_PER_JOB from spec

a21d0f4

bazarnov self-assigned this Nov 24, 2021

bazarnov linked an issue Nov 24, 2021 that may be closed by this pull request

Source FB Marketing: Deprecate the Insights Days per job parameter #8027

Closed

github-actions bot added the area/connectors Connector related issues label Nov 24, 2021

jrhizor temporarily deployed to more-secrets November 24, 2021 15:13 Inactive

bazarnov added 3 commits November 24, 2021 17:57

fixed spec.json for invalid_spec

b335918

fixed SAT tests

bfd6b2b

increased timeout_seconds in SAT

688633e

bazarnov temporarily deployed to more-secrets November 24, 2021 16:01 Inactive

jrhizor temporarily deployed to more-secrets November 24, 2021 16:03 Inactive

jrhizor temporarily deployed to more-secrets November 24, 2021 18:39 Inactive

bazarnov linked an issue Nov 24, 2021 that may be closed by this pull request

Clarify the setup fields for Source Facebook Marketing #7848

Closed

sherifnada reviewed Nov 24, 2021

View reviewed changes

airbyte-integrations/connectors/source-paypal-transaction/source_paypal_transaction/source.py Outdated Show resolved Hide resolved

reverted paypal files

39eac30

bazarnov temporarily deployed to more-secrets November 25, 2021 10:19 Inactive

reordered spec fields

e9b0862

bazarnov temporarily deployed to more-secrets November 25, 2021 11:10 Inactive

jrhizor temporarily deployed to more-secrets November 25, 2021 11:26 Inactive

bumbed version, added changelog

97b9fd6

github-actions bot added the area/documentation Improvements or additions to documentation label Nov 25, 2021

bazarnov temporarily deployed to more-secrets November 25, 2021 11:32 Inactive

edited spec validation

2930735

bazarnov temporarily deployed to more-secrets November 25, 2021 12:16 Inactive

jrhizor temporarily deployed to more-secrets November 25, 2021 12:17 Inactive

bazarnov requested review from annalvova05 and midavadim November 25, 2021 12:29

bazarnov added 2 commits November 26, 2021 11:06

Merge remote-tracking branch 'origin/master' into bazarnov/8027-fb-ma…

d1ea462

…rketing-insigths-days-per-job

corrected formating

89589e0

bazarnov temporarily deployed to more-secrets November 26, 2021 09:11 Inactive

changed insights_days_per_job to 7

c87be7d

bazarnov temporarily deployed to more-secrets November 26, 2021 09:13 Inactive

jrhizor temporarily deployed to more-secrets November 26, 2021 09:14 Inactive

annalvova05 approved these changes Nov 26, 2021

View reviewed changes

midavadim approved these changes Nov 26, 2021

View reviewed changes

bazarnov requested a review from keu November 26, 2021 12:40

avida reviewed Nov 30, 2021

View reviewed changes

airbyte-integrations/connectors/source-facebook-marketing/source_facebook_marketing/source.py Outdated Show resolved Hide resolved

bazarnov added 3 commits November 30, 2021 14:38

Merge remote-tracking branch 'origin/master' into bazarnov/8027-fb-ma…

352c2e7

…rketing-insigths-days-per-job

updated default value of insigts_days_per_job

31415bf

updated default value of insigts_days_per_job

d363222

bazarnov temporarily deployed to more-secrets November 30, 2021 13:21 Inactive

karinakuz added connectors/sources-api connectors/source/facebook-marketing labels Jan 5, 2022

bazarnov closed this Jan 19, 2022

This was referenced Jan 19, 2022

Clarify the setup fields for Source Facebook Marketing #7848

Closed

Source FB Marketing: Deprecate the Insights Days per job parameter #8027

Closed

keu mentioned this pull request Jan 20, 2022

Source FB Marketing: improve insights jobs reliability & runtime #8282

Closed

keu pushed a commit that referenced this pull request Jan 22, 2022

merge changes from #8234

218db5c

bazarnov deleted the bazarnov/8027-fb-marketing-insigths-days-per-job branch September 20, 2022 09:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Source FB Marketing: deprecate `INSIGHTS_DAYS_PER_JOB` from connector's specification. #8234

🐛 Source FB Marketing: deprecate `INSIGHTS_DAYS_PER_JOB` from connector's specification. #8234

bazarnov commented Nov 24, 2021 •

edited by htrueman

Loading

bazarnov commented Nov 24, 2021 •

edited by github-actions bot

Loading

bazarnov commented Nov 25, 2021 •

edited by github-actions bot

Loading

bazarnov commented Nov 26, 2021 •

edited by github-actions bot

Loading

keu commented Nov 26, 2021 •

edited

Loading

bazarnov commented Nov 30, 2021

keu commented Dec 1, 2021

sherifnada commented Dec 3, 2021

keu commented Jan 19, 2022

bazarnov commented Jan 19, 2022

🐛 Source FB Marketing: deprecate INSIGHTS_DAYS_PER_JOB from connector's specification. #8234

🐛 Source FB Marketing: deprecate INSIGHTS_DAYS_PER_JOB from connector's specification. #8234

Conversation

bazarnov commented Nov 24, 2021 • edited by htrueman Loading

What

How

Pre-merge Checklist

Community member or Airbyter

Airbyter

bazarnov commented Nov 24, 2021 • edited by github-actions bot Loading

bazarnov commented Nov 25, 2021 • edited by github-actions bot Loading

bazarnov commented Nov 26, 2021 • edited by github-actions bot Loading

keu commented Nov 26, 2021 • edited Loading

bazarnov commented Nov 30, 2021

keu commented Dec 1, 2021

sherifnada commented Dec 3, 2021

keu commented Jan 19, 2022

bazarnov commented Jan 19, 2022

🐛 Source FB Marketing: deprecate `INSIGHTS_DAYS_PER_JOB` from connector's specification. #8234

🐛 Source FB Marketing: deprecate `INSIGHTS_DAYS_PER_JOB` from connector's specification. #8234

bazarnov commented Nov 24, 2021 •

edited by htrueman

Loading

bazarnov commented Nov 24, 2021 •

edited by github-actions bot

Loading

bazarnov commented Nov 25, 2021 •

edited by github-actions bot

Loading

bazarnov commented Nov 26, 2021 •

edited by github-actions bot

Loading

keu commented Nov 26, 2021 •

edited

Loading