Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Source google ads: fix for page token expired #9608

Merged
merged 11 commits into from
Jan 24, 2022

Conversation

augan-rymkhan
Copy link
Contributor

@augan-rymkhan augan-rymkhan commented Jan 19, 2022

What

Resolves page token expired issue provided in On-Call issue #103

Stream slice's date range is 1 month, if it takes more than 2 hour to process all the records from the given slice, next page tokens which records are being retrieved will be expired.
For example:
Slice start: 01.01.2022
Slice end: 30.01.2022
page_size = 1000
Within 2 hours it has read 2 000 000 records (2000 pages).
After that time it tries to read page number 2001, but page tokens are expired already.

The solution: reduce the slice's date range

image

The similar issue was discussed in the following links:

How

Reduce slice range up to 15 days.
Date range is set to 15 days for each slice, because for conversion_window_days default value is 14. Range less than 15 days will break the integration tests.

Recommended reading order

  1. source_google_ads/streams.py
  2. unit_tests/test_google_ads.py
  3. unit_tests/test_source.py

@github-actions github-actions bot added the area/connectors Connector related issues label Jan 19, 2022
@augan-rymkhan augan-rymkhan temporarily deployed to more-secrets January 19, 2022 15:40 Inactive
@augan-rymkhan augan-rymkhan temporarily deployed to more-secrets January 19, 2022 15:45 Inactive
@augan-rymkhan
Copy link
Contributor Author

augan-rymkhan commented Jan 19, 2022

/test connector=connectors/source-google-ads

🕑 connectors/source-google-ads https://github.com/airbytehq/airbyte/actions/runs/1718879049
✅ connectors/source-google-ads https://github.com/airbytehq/airbyte/actions/runs/1718879049
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                        74      6    92%
	 source_acceptance_test/conftest.py                     109    109     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              242     96    60%
	 source_acceptance_test/tests/test_full_refresh.py       38      0   100%
	 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                  54     17    69%
	 source_acceptance_test/utils/compare.py                 62     23    63%
	 source_acceptance_test/utils/connector_runner.py       110     48    56%
	 source_acceptance_test/utils/json_schema_helper.py     115     14    88%
	 ------------------------------------------------------------------------
	 TOTAL                                                  979    404    59%
	 ---------- coverage: platform linux, python 3.8.10-final-0 -----------
	 Name                                       Stmts   Miss  Cover
	 --------------------------------------------------------------
	 source_google_ads/__init__.py                  2      0   100%
	 source_google_ads/custom_query_stream.py      75     50    33%
	 source_google_ads/google_ads.py               67     10    85%
	 source_google_ads/source.py                   64     23    64%
	 source_google_ads/streams.py                  88      4    95%
	 --------------------------------------------------------------
	 TOTAL                                        296     87    71%
	 ---------- coverage: platform linux, python 3.8.10-final-0 -----------
	 Name                                       Stmts   Miss  Cover
	 --------------------------------------------------------------
	 source_google_ads/__init__.py                  2      0   100%
	 source_google_ads/custom_query_stream.py      75      6    92%
	 source_google_ads/google_ads.py               67      6    91%
	 source_google_ads/source.py                   64     17    73%
	 source_google_ads/streams.py                  88      9    90%
	 --------------------------------------------------------------
	 TOTAL                                        296     38    87%

Python short test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.7/site-packages/source_acceptance_test/plugin.py:56: Skipping TestIncremental.test_two_sequential_reads because not found in the config
================== 15 passed, 1 skipped in 1069.80s (0:17:49) ==================

@octavia-squidington-iii octavia-squidington-iii temporarily deployed to more-secrets January 19, 2022 15:49 Inactive
@augan-rymkhan augan-rymkhan temporarily deployed to more-secrets January 19, 2022 16:50 Inactive
@augan-rymkhan
Copy link
Contributor Author

augan-rymkhan commented Jan 19, 2022

/test connector=connectors/source-google-ads

🕑 connectors/source-google-ads https://github.com/airbytehq/airbyte/actions/runs/1719173600
✅ connectors/source-google-ads https://github.com/airbytehq/airbyte/actions/runs/1719173600
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                        74      6    92%
	 source_acceptance_test/conftest.py                     109    109     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              242     96    60%
	 source_acceptance_test/tests/test_full_refresh.py       38      0   100%
	 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                  54     17    69%
	 source_acceptance_test/utils/compare.py                 62     23    63%
	 source_acceptance_test/utils/connector_runner.py       110     48    56%
	 source_acceptance_test/utils/json_schema_helper.py     115     14    88%
	 ------------------------------------------------------------------------
	 TOTAL                                                  979    404    59%
	 ---------- coverage: platform linux, python 3.8.10-final-0 -----------
	 Name                                       Stmts   Miss  Cover
	 --------------------------------------------------------------
	 source_google_ads/__init__.py                  2      0   100%
	 source_google_ads/custom_query_stream.py      75     50    33%
	 source_google_ads/google_ads.py               67     10    85%
	 source_google_ads/source.py                   64     23    64%
	 source_google_ads/streams.py                  86      4    95%
	 --------------------------------------------------------------
	 TOTAL                                        294     87    70%
	 ---------- coverage: platform linux, python 3.8.10-final-0 -----------
	 Name                                       Stmts   Miss  Cover
	 --------------------------------------------------------------
	 source_google_ads/__init__.py                  2      0   100%
	 source_google_ads/custom_query_stream.py      75      6    92%
	 source_google_ads/google_ads.py               67      6    91%
	 source_google_ads/source.py                   64     17    73%
	 source_google_ads/streams.py                  86      9    90%
	 --------------------------------------------------------------
	 TOTAL                                        294     38    87%

Python short test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.7/site-packages/source_acceptance_test/plugin.py:56: Skipping TestIncremental.test_two_sequential_reads because not found in the config
================== 15 passed, 1 skipped in 896.79s (0:14:56) ===================

@octavia-squidington-iii octavia-squidington-iii temporarily deployed to more-secrets January 19, 2022 16:54 Inactive
@augan-rymkhan augan-rymkhan changed the title Arymkhan/fix google ads page token expired Source google ads: fix page token expired Jan 19, 2022
@augan-rymkhan augan-rymkhan marked this pull request as ready for review January 20, 2022 07:05
@augan-rymkhan augan-rymkhan changed the title Source google ads: fix page token expired Source google ads: fix for page token expired Jan 20, 2022
days_of_data_storage = 90
range_days = 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And description of this overriding of the value coul be usefel as well.

Copy link
Contributor Author

@augan-rymkhan augan-rymkhan Jan 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before the changes provided in this PR, in the class ClickView time_unit was overriden with "days" and
start_date = start_date.add(**{time_unit: 1}), the final result means start_date.add(days=1)
So, to save the behaviour, I just set range_days to 1.

@sherifnada
Copy link
Contributor

@augan-rymkhan can you also include the root cause in the PR description or in a source code comment (even better) to help retain this knowledge?

@augan-rymkhan augan-rymkhan temporarily deployed to more-secrets January 21, 2022 08:37 Inactive
@github-actions github-actions bot added the area/documentation Improvements or additions to documentation label Jan 24, 2022
@augan-rymkhan augan-rymkhan temporarily deployed to more-secrets January 24, 2022 07:59 Inactive
@codecov
Copy link

codecov bot commented Jan 24, 2022

Codecov Report

❗ No coverage uploaded for pull request base (master@b269b9f). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #9608   +/-   ##
=========================================
  Coverage          ?   66.55%           
=========================================
  Files             ?        5           
  Lines             ?      293           
  Branches          ?        0           
=========================================
  Hits              ?      195           
  Misses            ?       98           
  Partials          ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b269b9f...7b23ccc. Read the comment docs.

@augan-rymkhan
Copy link
Contributor Author

augan-rymkhan commented Jan 24, 2022

/publish connector=connectors/source-google-ads

🕑 connectors/source-google-ads https://github.com/airbytehq/airbyte/actions/runs/1738725497
✅ connectors/source-google-ads https://github.com/airbytehq/airbyte/actions/runs/1738725497

@octavia-squidington-iii octavia-squidington-iii temporarily deployed to more-secrets January 24, 2022 08:14 Inactive
@augan-rymkhan augan-rymkhan temporarily deployed to more-secrets January 24, 2022 08:47 Inactive
@augan-rymkhan augan-rymkhan merged commit aaef33e into master Jan 24, 2022
@augan-rymkhan augan-rymkhan deleted the arymkhan/fix-google-ads-page-token-expired branch January 24, 2022 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/connectors Connector related issues area/documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants