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: connector doesn't pull data for today #8225

Merged
merged 20 commits into from Dec 9, 2021

Conversation

annalvova05
Copy link
Contributor

@annalvova05 annalvova05 commented Nov 24, 2021

What

For incremental streams end_date param is yesterday's date now. Data is not pulled for today. Could reproduce it locally.

Closes #7047 and OnCall#37.

How

Update date to today and take timezone into account.

Recommended reading order

  1. streams.py

🚨 User Impact 🚨

Are there any breaking changes? If yes, please make sure to include it here and in any changelogs with the 🚨🚨 emoji
What is the end result perceived by the user?

Pre-merge Checklist

Expand the relevant checklist and delete the others.

Updating a connector

Community member or Airbyter

  • Grant edit access to maintainers (instructions)
  • Secrets in the connector's spec are annotated with airbyte_secret
  • 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

@annalvova05 annalvova05 self-assigned this Nov 24, 2021
@github-actions github-actions bot added the area/connectors Connector related issues label Nov 24, 2021
@annalvova05
Copy link
Contributor Author

annalvova05 commented Nov 24, 2021

/test connector=connectors/source-google-ads

🕑 connectors/source-google-ads https://github.com/airbytehq/airbyte/actions/runs/1499189482
❌ connectors/source-google-ads https://github.com/airbytehq/airbyte/actions/runs/1499189482
🐛 https://gradle.com/s/nmzlo3jufmtfs

@annalvova05 annalvova05 linked an issue Nov 24, 2021 that may be closed by this pull request
@annalvova05 annalvova05 temporarily deployed to more-secrets November 24, 2021 11:50 Inactive
@jrhizor jrhizor temporarily deployed to more-secrets November 24, 2021 11:51 Inactive
@annalvova05
Copy link
Contributor Author

annalvova05 commented Nov 24, 2021

/test connector=connectors/source-google-ads

🕑 connectors/source-google-ads https://github.com/airbytehq/airbyte/actions/runs/1499303227
✅ connectors/source-google-ads https://github.com/airbytehq/airbyte/actions/runs/1499303227
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_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                   35     15    57%
	 source_google_ads/streams.py                  91      5    95%
	 --------------------------------------------------------------
	 TOTAL                                        270     80    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      7    90%
	 source_google_ads/source.py                   35     22    37%
	 source_google_ads/streams.py                  91     21    77%
	 --------------------------------------------------------------
	 TOTAL                                        270     56    79%

@annalvova05 annalvova05 temporarily deployed to more-secrets November 24, 2021 12:22 Inactive
@jrhizor jrhizor temporarily deployed to more-secrets November 24, 2021 12:22 Inactive
@antixar antixar self-requested a review November 25, 2021 17:27
Copy link
Contributor

@antixar antixar left a comment

Choose a reason for hiding this comment

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

Please add an unit test for this case when a time_zone value can influence data-time values.

@@ -83,9 +92,8 @@ def stream_slices(self, stream_state: Mapping[str, Any] = None, **kwargs) -> Ite
days_of_data_storage=self.days_of_data_storage,
)

@staticmethod
def get_date_params(stream_slice: Mapping[str, Any], cursor_field: str, end_date: pendulum.datetime = None, time_unit: str = "months"):
end_date = end_date or pendulum.yesterday()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think there were some reasons to do it.

I suspect that Google Ads do not generate reports in real time.

As far as I remember google need some time (a day) to have full report for given day.

If we set end_day to today then report can be empty or incomplete. So please do additional research for this question

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't find any information about impossibility sync reports in real-time. Google Ads returns data for today

Copy link
Collaborator

Choose a reason for hiding this comment

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

@sherifnada We need your opinion here.

  1. As for me if we include 'today' into the current sync means that next sync starts from tomorrow so client will lose data between now and the end of today's day.

  2. as far as I remember from my own experience with Google Ads - it does not provide 'real-time' reports and full data is available only in a few hours .

If we want to include todays report into sync then an alternative solution can be - we should have 1 day overlaps between scans. It guarantees that customers have full reports.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, I have found Google docs about it

https://support.google.com/google-ads/answer/2544985?hl=en

  • such as clicks, conversions, and impressions) are delayed by less than 3 hours
  • conversions attributed using attribution models other than “Last click” are typically delayed up to 15 hours
    etc

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's acceptable to sync data only up to yesterday if there is no other way to guarantee data loss. Ideally, we can pick up a sync exactly where we left off. But if it's not possible, that's acceptable imo.

What is the technical possibility of doing it that way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sherifnada we cannot sync data up to today. We use segments.date field for filtering data in incremental sync. Google Ads accepts only date without time for filtering. I also update google ads doc.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, lets sync only until yesterday then. we can update that ticket with this information, and mention we can't do it due to limit from api side

Copy link
Collaborator

@midavadim midavadim left a comment

Choose a reason for hiding this comment

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

Please do not forget to update version and docs

@annalvova05 annalvova05 temporarily deployed to more-secrets November 29, 2021 17:38 Inactive
@annalvova05 annalvova05 temporarily deployed to more-secrets November 29, 2021 17:43 Inactive
@annalvova05 annalvova05 temporarily deployed to more-secrets November 30, 2021 09:21 Inactive
@github-actions github-actions bot added the area/documentation Improvements or additions to documentation label Dec 3, 2021
…ads-fix-end-date

# Conflicts:
#	airbyte-integrations/connectors/source-google-ads/source_google_ads/spec.json
@annalvova05 annalvova05 temporarily deployed to more-secrets December 3, 2021 13:18 Inactive
@annalvova05 annalvova05 temporarily deployed to more-secrets December 3, 2021 13:20 Inactive
@annalvova05
Copy link
Contributor Author

annalvova05 commented Dec 3, 2021

/test connector=connectors/source-google-ads

🕑 connectors/source-google-ads https://github.com/airbytehq/airbyte/actions/runs/1535379004
❌ connectors/source-google-ads https://github.com/airbytehq/airbyte/actions/runs/1535379004
🐛 https://gradle.com/s/un7fyns4bxlys

@jrhizor jrhizor temporarily deployed to more-secrets December 3, 2021 13:23 Inactive
@annalvova05 annalvova05 temporarily deployed to more-secrets December 9, 2021 10:57 Inactive
@annalvova05 annalvova05 temporarily deployed to more-secrets December 9, 2021 11:47 Inactive
@annalvova05 annalvova05 temporarily deployed to more-secrets December 9, 2021 15:28 Inactive
@@ -83,9 +92,8 @@ def stream_slices(self, stream_state: Mapping[str, Any] = None, **kwargs) -> Ite
days_of_data_storage=self.days_of_data_storage,
)

@staticmethod
def get_date_params(stream_slice: Mapping[str, Any], cursor_field: str, end_date: pendulum.datetime = None, time_unit: str = "months"):
end_date = end_date or pendulum.yesterday()
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, lets sync only until yesterday then. we can update that ticket with this information, and mention we can't do it due to limit from api side

docs/integrations/sources/google-ads.md Outdated Show resolved Hide resolved
Co-authored-by: Sherif A. Nada <snadalive@gmail.com>
@annalvova05 annalvova05 temporarily deployed to more-secrets December 9, 2021 17:00 Inactive
@annalvova05
Copy link
Contributor Author

annalvova05 commented Dec 9, 2021

/test connector=connectors/source-google-ads

🕑 connectors/source-google-ads https://github.com/airbytehq/airbyte/actions/runs/1559792732
❌ connectors/source-google-ads https://github.com/airbytehq/airbyte/actions/runs/1559792732
🐛 https://gradle.com/s/vqntz6xs4mtmw

@jrhizor jrhizor temporarily deployed to more-secrets December 9, 2021 17:02 Inactive
@annalvova05
Copy link
Contributor Author

annalvova05 commented Dec 9, 2021

/test connector=connectors/source-google-ads

🕑 connectors/source-google-ads https://github.com/airbytehq/airbyte/actions/runs/1559876817
✅ connectors/source-google-ads https://github.com/airbytehq/airbyte/actions/runs/1559876817
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                        76      8    89%
	 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              235     95    60%
	 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                  54     24    56%
	 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                                                  946    442    53%
	 ---------- 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                  84      4    95%
	 --------------------------------------------------------------
	 TOTAL                                        292     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                  84      9    89%
	 --------------------------------------------------------------
	 TOTAL                                        292     38    87%

@annalvova05 annalvova05 temporarily deployed to more-secrets December 9, 2021 17:21 Inactive
@jrhizor jrhizor temporarily deployed to more-secrets December 9, 2021 17:21 Inactive
@annalvova05
Copy link
Contributor Author

annalvova05 commented Dec 9, 2021

/publish connector=connectors/source-google-ads

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

@jrhizor jrhizor temporarily deployed to more-secrets December 9, 2021 17:41 Inactive
@annalvova05
Copy link
Contributor Author

annalvova05 commented Dec 9, 2021

/publish connector=connectors/source-google-ads

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

@annalvova05 annalvova05 temporarily deployed to more-secrets December 9, 2021 18:02 Inactive
@jrhizor jrhizor temporarily deployed to more-secrets December 9, 2021 18:03 Inactive
@annalvova05 annalvova05 merged commit b0fb656 into master Dec 9, 2021
@annalvova05 annalvova05 deleted the alvova/7047-google-ads-fix-end-date branch December 9, 2021 18:36
@annalvova05 annalvova05 temporarily deployed to more-secrets December 9, 2021 18:37 Inactive
schlattk pushed a commit to schlattk/airbyte that referenced this pull request Jan 4, 2022
…8225)

* fix end_date for incremental streams

* upd imports

* upd unit test

* upd after review

* fix unit tests

* change type

* remove streams for manager account

* bump version

* add unit_tests

* upd imports

* update is_manager_account method

* add unit tests

* Update docs/integrations/sources/google-ads.md

Co-authored-by: Sherif A. Nada <snadalive@gmail.com>

* check metrics in custom query

* change end date and upd doc

* Update docs/integrations/sources/google-ads.md

Co-authored-by: Sherif A. Nada <snadalive@gmail.com>

* optimize imports

* bump version

* bump version

Co-authored-by: Sherif A. Nada <snadalive@gmail.com>
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.

Source Google ads: connector doesn't pull data for today
5 participants