#1313 source google ads: write less logs #21517

davydov-d · 2023-01-18T12:09:42Z

What

https://github.com/airbytehq/oncall/issues/1313

How

Filter out each 10th log record

davydov-d · 2023-01-18T12:11:26Z

/test connector=connectors/source-google-ads

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

Build Failed

Test summary info:

=========================== short test summary info ============================
FAILED test_core.py::TestBasicRead::test_read[inputs0] - Failed: Stream accou...
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/tests/test_core.py:94: The previous and actual specifications are identical.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/tests/test_core.py:377: The previous and actual discovered catalogs are identical.
============= 1 failed, 28 passed, 2 skipped in 1608.20s (0:26:48) =============

davydov-d · 2023-01-18T14:31:12Z

/test connector=connectors/source-google-ads

🕑 connectors/source-google-ads https://github.com/airbytehq/airbyte/actions/runs/3949865210
✅ connectors/source-google-ads https://github.com/airbytehq/airbyte/actions/runs/3949865210
Python tests coverage:

Name                                       Stmts   Miss  Cover
--------------------------------------------------------------
source_google_ads/models.py                   18      0   100%
source_google_ads/__init__.py                  2      0   100%
source_google_ads/streams.py                 196      6    97%
source_google_ads/source.py                   86      5    94%
source_google_ads/custom_query_stream.py      75      5    93%
source_google_ads/google_ads.py               73     12    84%
--------------------------------------------------------------
TOTAL                                        450     28    94%
	 Name                                                 Stmts   Miss  Cover   Missing
	 ----------------------------------------------------------------------------------
	 source_acceptance_test/base.py                          12      4    67%   16-19
	 source_acceptance_test/config.py                       141      5    96%   87, 93, 239, 243-244
	 source_acceptance_test/conftest.py                     211     95    55%   36, 42-44, 49, 54, 77, 83, 89-91, 110, 115-117, 123-125, 131-132, 137-138, 143, 149, 158-167, 173-178, 193, 217, 248, 254, 262-267, 275-285, 293-306, 311-317, 324-335, 342-358
	 source_acceptance_test/plugin.py                        69     25    64%   22-23, 31, 36, 120-140, 144-148
	 source_acceptance_test/tests/test_core.py              402    115    71%   53, 58, 93-104, 109-116, 120-121, 125-126, 308, 346-363, 376-387, 391-396, 402, 435-440, 478-485, 528-530, 533, 598-606, 618-621, 626, 682-683, 689, 692, 728-738, 751-776
	 source_acceptance_test/tests/test_incremental.py       160     14    91%   58-65, 70-83, 246
	 source_acceptance_test/utils/asserts.py                 39      2    95%   62-63
	 source_acceptance_test/utils/common.py                  94     10    89%   16-17, 32-38, 72, 75
	 source_acceptance_test/utils/compare.py                 62     23    63%   21-51, 68, 97-99
	 source_acceptance_test/utils/connector_runner.py       133     33    75%   24-27, 46-47, 50-54, 57-58, 73-75, 78-80, 83-85, 88-90, 93-95, 124-125, 159-161, 208
	 source_acceptance_test/utils/json_schema_helper.py     107     13    88%   30-31, 38, 41, 65-68, 96, 120, 192-194
	 ----------------------------------------------------------------------------------
	 TOTAL                                                 1609    339    79%

Build Passed

Test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/tests/test_core.py:94: The previous and actual specifications are identical.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/tests/test_core.py:377: The previous and actual discovered catalogs are identical.
================== 29 passed, 2 skipped in 1419.82s (0:23:39) ==================

brianjlai

A few questions about some specifics of how we use the sieve, but overall the approach looks good.

airbyte-integrations/connectors/source-google-ads/source_google_ads/streams.py

brianjlai · 2023-01-18T19:53:57Z

airbyte-integrations/connectors/source-google-ads/source_google_ads/streams.py

@@ -154,6 +177,7 @@ def current_state(self, customer_id, default=None):

    def stream_slices(self, stream_state: Mapping[str, Any] = None, **kwargs) -> Iterable[Optional[MutableMapping[str, any]]]:
        for customer in self.customers:
+            logger = cyclic_sieve(self.logger, 10)


Can we avoid initializing a new cyclic_sieve since it should have already been defined on the stream as self.incremental_sieve_logger?

Technically we can, but this was done on purpose. The idea was to filter out not just 9 out of 10 messages, but 9 out of 10 message cycles to have more consistent logs so an engineer shouldn't have to stick together messages from different routine calls / cycles / exceptions / if statements and so on when debugging. Since stream_slices() is called once per stream before reading records, and read_records() is called once per stream slice, these are two different log cycles.
If you still insist, I can reuse self.incremental_sieve_logger here, technically it is not a problem

Ah got it, that makes sense and I agree that for a better experience we wouldn't want to share the same log cycles. Can you add a comment for that here just mentioning that's why a logger sieve is initialized here.

brianjlai · 2023-01-18T19:54:10Z

airbyte-integrations/connectors/source-google-ads/unit_tests/test_streams.py

@@ -218,3 +219,14 @@ def test_retry_transient_errors(mocker, config, customers, error_cls):
        records = list(stream.read_records(sync_mode=SyncMode.incremental, cursor_field=["segments.date"], stream_slice=stream_slice))
    assert mocked_search.call_count == 5
    assert records == []
+
+
+def test_cyclic_sieve(caplog):


thanks for adding tests for the new logger?

davydov-d · 2023-01-19T08:42:40Z

/test connector=connectors/source-google-ads

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

davydov-d · 2023-01-19T10:01:22Z

/test connector=connectors/source-google-ads

🕑 connectors/source-google-ads https://github.com/airbytehq/airbyte/actions/runs/3957323653
✅ connectors/source-google-ads https://github.com/airbytehq/airbyte/actions/runs/3957323653
Python tests coverage:

Name                                       Stmts   Miss  Cover
--------------------------------------------------------------
source_google_ads/models.py                   18      0   100%
source_google_ads/__init__.py                  2      0   100%
source_google_ads/streams.py                 195      6    97%
source_google_ads/source.py                   86      5    94%
source_google_ads/custom_query_stream.py      75      5    93%
source_google_ads/google_ads.py               73     12    84%
--------------------------------------------------------------
TOTAL                                        449     28    94%
	 Name                                                 Stmts   Miss  Cover   Missing
	 ----------------------------------------------------------------------------------
	 source_acceptance_test/base.py                          12      4    67%   16-19
	 source_acceptance_test/config.py                       141      5    96%   87, 93, 239, 243-244
	 source_acceptance_test/conftest.py                     211     95    55%   36, 42-44, 49, 54, 77, 83, 89-91, 110, 115-117, 123-125, 131-132, 137-138, 143, 149, 158-167, 173-178, 193, 217, 248, 254, 262-267, 275-285, 293-306, 311-317, 324-335, 342-358
	 source_acceptance_test/plugin.py                        69     25    64%   22-23, 31, 36, 120-140, 144-148
	 source_acceptance_test/tests/test_core.py              402    115    71%   53, 58, 93-104, 109-116, 120-121, 125-126, 308, 346-363, 376-387, 391-396, 402, 435-440, 478-485, 528-530, 533, 598-606, 618-621, 626, 682-683, 689, 692, 728-738, 751-776
	 source_acceptance_test/tests/test_incremental.py       160     14    91%   58-65, 70-83, 246
	 source_acceptance_test/utils/asserts.py                 39      2    95%   62-63
	 source_acceptance_test/utils/common.py                  94     10    89%   16-17, 32-38, 72, 75
	 source_acceptance_test/utils/compare.py                 62     23    63%   21-51, 68, 97-99
	 source_acceptance_test/utils/connector_runner.py       133     33    75%   24-27, 46-47, 50-54, 57-58, 73-75, 78-80, 83-85, 88-90, 93-95, 124-125, 159-161, 208
	 source_acceptance_test/utils/json_schema_helper.py     107     13    88%   30-31, 38, 41, 65-68, 96, 120, 192-194
	 ----------------------------------------------------------------------------------
	 TOTAL                                                 1609    339    79%

Build Passed

Test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/tests/test_core.py:94: The previous and actual specifications are identical.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/tests/test_core.py:377: The previous and actual discovered catalogs are identical.
================== 29 passed, 2 skipped in 1699.95s (0:28:19) ==================

brianjlai

Overall no blocking comments, but had one discussion point about whether we want a new sieve per-read_records invocation if that made interpreting the logs more intuitive

brianjlai · 2023-01-19T22:16:22Z

airbyte-integrations/connectors/source-google-ads/source_google_ads/streams.py

@@ -154,6 +177,7 @@ def current_state(self, customer_id, default=None):

    def stream_slices(self, stream_state: Mapping[str, Any] = None, **kwargs) -> Iterable[Optional[MutableMapping[str, any]]]:
        for customer in self.customers:
+            logger = cyclic_sieve(self.logger, 10)


Ah got it, that makes sense and I agree that for a better experience we wouldn't want to share the same log cycles. Can you add a comment for that here just mentioning that's why a logger sieve is initialized here.

brianjlai · 2023-01-19T22:44:32Z

airbyte-integrations/connectors/source-google-ads/source_google_ads/streams.py

@@ -189,7 +213,8 @@ def read_records(
        and update `start_date` key in the `stream_slice` with the latest read record's cursor value, then retry the sync.
        """
        while True:
-            self.logger.info("Starting a while loop iteration")
+            self.incremental_sieve_logger.bump()


Going off the prior comment about separate slice and read_records log cycles, do you think it's also worth initializing a new sieve every time read_records is invoked? If the sieve is shared on the stream, we will emit exactly 1/10 of logs, but it could be a little confusing why certain cycles are skipped on different read_records invocations if a prior one has a partially bumped count.

I'm open to either option and this is non-blocking on approval, but I think worth highlighting when the sieve is shared at this point? If we do decide to have a separate sieve per read_records invocation, we could probably remove it from __init__() and do something similar to slices where it is created within this method.

That's a tricky point cause normally a while loop here contains a single iteration. It only goes further if there were errors during the read. So I do not think there's much sense in initializing new sieve each time read_records is invoked. On the other hand I agree that logs may become inconsistent if there are two or more while loop iterations, therefore I moved incremental_sieve_logger.bump() outside the loop to remain consistent

davydov-d · 2023-01-20T11:04:14Z

/publish connector=connectors/source-google-ads

🕑 Publishing the following connectors:
connectors/source-google-ads
https://github.com/airbytehq/airbyte/actions/runs/3967044312

Connector	Did it publish?	Were definitions generated?
connectors/source-google-ads	✅	✅

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️

github-actions · 2023-01-20T11:57:38Z

Airbyte Code Coverage

There is no coverage information present for the Files changed

Total Project Coverage	26.72%	🍏

…er-logs

#1313 source google ads: write less logs

53784af

octavia-squidington-iv added area/connectors Connector related issues area/documentation Improvements or additions to documentation connectors/source/google-ads labels Jan 18, 2023

#1313 source google ads: upd changelog

77c0608

davydov-d requested a review from brianjlai January 18, 2023 12:10

#1313 source google ads: fix expected records

02f4d86

brianjlai reviewed Jan 18, 2023

View reviewed changes

#1313 source google ads: rm unused call to init

7d8ca4e

davydov-d requested a review from brianjlai January 19, 2023 08:42

#1313 source google ads: fix expected records

544a465

brianjlai approved these changes Jan 19, 2023

View reviewed changes

#1313 source google ads - bump sieve outside the loop

997e326

auto-bump connector version

641ceff

octavia-squidington-iii temporarily deployed to more-secrets January 20, 2023 11:47 — with GitHub Actions Inactive

Merge branch 'master' into ddavydov/#1313-source-google-ads-write-few…

1d995cf

…er-logs

davydov-d temporarily deployed to more-secrets January 20, 2023 12:05 — with GitHub Actions Inactive

davydov-d temporarily deployed to more-secrets January 20, 2023 12:38 — with GitHub Actions Inactive

Merge branch 'master' into ddavydov/#1313-source-google-ads-write-few…

0b61eb3

…er-logs

davydov-d temporarily deployed to more-secrets January 20, 2023 12:49 — with GitHub Actions Inactive

Merge branch 'master' into ddavydov/#1313-source-google-ads-write-few…

8b06641

…er-logs

davydov-d temporarily deployed to more-secrets January 20, 2023 13:51 — with GitHub Actions Inactive

Merge branch 'master' into ddavydov/#1313-source-google-ads-write-few…

155fe03

…er-logs

davydov-d temporarily deployed to more-secrets January 20, 2023 15:17 — with GitHub Actions Inactive

Merge branch 'master' into ddavydov/#1313-source-google-ads-write-few…

8ee1fc5

…er-logs

davydov-d temporarily deployed to more-secrets January 20, 2023 17:58 — with GitHub Actions Inactive

Merge branch 'master' into ddavydov/#1313-source-google-ads-write-few…

9ae17d0

…er-logs

davydov-d temporarily deployed to more-secrets January 23, 2023 07:40 — with GitHub Actions Inactive

davydov-d merged commit e35dc23 into master Jan 23, 2023

davydov-d deleted the ddavydov/#1313-source-google-ads-write-fewer-logs branch January 23, 2023 08:18

octavia-squidington-iii mentioned this pull request Jan 24, 2023

Bump Airbyte version from 0.40.28 to 0.40.29 #21767

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#1313 source google ads: write less logs #21517

#1313 source google ads: write less logs #21517

davydov-d commented Jan 18, 2023

davydov-d commented Jan 18, 2023 •

edited by github-actions bot

Loading

davydov-d commented Jan 18, 2023 •

edited by github-actions bot

Loading

brianjlai left a comment

brianjlai Jan 18, 2023

davydov-d Jan 19, 2023

brianjlai Jan 19, 2023

brianjlai Jan 18, 2023

davydov-d commented Jan 19, 2023 •

edited by github-actions bot

Loading

davydov-d commented Jan 19, 2023 •

edited by github-actions bot

Loading

brianjlai left a comment

brianjlai Jan 19, 2023

brianjlai Jan 19, 2023

davydov-d Jan 20, 2023

davydov-d commented Jan 20, 2023 •

edited by github-actions bot

Loading

github-actions bot commented Jan 20, 2023 •

edited

Loading

#1313 source google ads: write less logs #21517

#1313 source google ads: write less logs #21517

Conversation

davydov-d commented Jan 18, 2023

What

How

davydov-d commented Jan 18, 2023 • edited by github-actions bot Loading

Build Failed

davydov-d commented Jan 18, 2023 • edited by github-actions bot Loading

Build Passed

brianjlai left a comment

Choose a reason for hiding this comment

brianjlai Jan 18, 2023

Choose a reason for hiding this comment

davydov-d Jan 19, 2023

Choose a reason for hiding this comment

brianjlai Jan 19, 2023

Choose a reason for hiding this comment

brianjlai Jan 18, 2023

Choose a reason for hiding this comment

davydov-d commented Jan 19, 2023 • edited by github-actions bot Loading

davydov-d commented Jan 19, 2023 • edited by github-actions bot Loading

Build Passed

brianjlai left a comment

Choose a reason for hiding this comment

brianjlai Jan 19, 2023

Choose a reason for hiding this comment

brianjlai Jan 19, 2023

Choose a reason for hiding this comment

davydov-d Jan 20, 2023

Choose a reason for hiding this comment

davydov-d commented Jan 20, 2023 • edited by github-actions bot Loading

github-actions bot commented Jan 20, 2023 • edited Loading

Airbyte Code Coverage

davydov-d commented Jan 18, 2023 •

edited by github-actions bot

Loading

davydov-d commented Jan 18, 2023 •

edited by github-actions bot

Loading

davydov-d commented Jan 19, 2023 •

edited by github-actions bot

Loading

davydov-d commented Jan 19, 2023 •

edited by github-actions bot

Loading

davydov-d commented Jan 20, 2023 •

edited by github-actions bot

Loading

github-actions bot commented Jan 20, 2023 •

edited

Loading