ref(source-amazon-ads): migrate to low code #48116

artem1205 · 2024-11-01T19:09:22Z

What

Resolving https://github.com/airbytehq/airbyte-internal-issues/issues/10429

How

bump CDK to v6
migrate REST-based streams to low-code
set concurrency to 1 (disable); will enable after migration of async reports

Profiles stream python implementation is left because it is used in reporting logic (will be also removed after migration of async reports)

Review guide

airbyte-integrations/connectors/source-amazon-ads/source_amazon_ads/manifest.yaml

User Impact

Can this PR be safely reverted and rolled back?

YES 💚
NO ❌

Signed-off-by: Artem Inzhyyants <[email protected]>

vercel · 2024-11-01T19:09:26Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment

Name	Status	Preview	Comments	Updated (UTC)
airbyte-docs	⬜️ Ignored (Inspect)	Visit Preview		Nov 19, 2024 3:58pm

Signed-off-by: Artem Inzhyyants <[email protected]>

[skip ci] Signed-off-by: Artem Inzhyyants <[email protected]>

Signed-off-by: Artem Inzhyyants <[email protected]>

…205/source-amazon-ads-migrate-low-code # Conflicts: # airbyte-integrations/connectors/source-amazon-ads/metadata.yaml # airbyte-integrations/connectors/source-amazon-ads/pyproject.toml # docs/integrations/sources/amazon-ads.md

Signed-off-by: Artem Inzhyyants <[email protected]>

[skip ci] Signed-off-by: Artem Inzhyyants <[email protected]>

Signed-off-by: Artem Inzhyyants <[email protected]>

…azon-ads-migrate-low-code # Conflicts: # airbyte-integrations/connectors/source-amazon-ads/metadata.yaml # airbyte-integrations/connectors/source-amazon-ads/pyproject.toml

Signed-off-by: Artem Inzhyyants <[email protected]>

maxi297

The code seems good. Overall, my concern is mostly to make sure we have the right safety net on this one. Can we test a couple of syncs using regression testing?

maxi297 · 2024-11-11T13:30:53Z

airbyte-integrations/connectors/source-amazon-ads/source_amazon_ads/manifest.yaml

+        type: SimpleRetriever
+        requester:
+          $ref: "#/definitions/base_requester"
+          path: sp/budgetRules # WHY sp here???


What about this comment? This was the previous behavior the connector had, right?

added comment line from py version, still not sure about correct naming for this stream.

maxi297 · 2024-11-11T13:33:39Z

airbyte-integrations/connectors/source-amazon-ads/source_amazon_ads/manifest.yaml

+        paginator:
+          type: NoPagination
+
+    sponsored_display_campaigns:


Should Sponsored Display streams have an error handler too? It seems like the previous behavior was the default one so it might be fine here.

maxi297 · 2024-11-11T13:43:50Z

airbyte-integrations/connectors/source-amazon-ads/source_amazon_ads/manifest.yaml

+            type: DefaultErrorHandler
+            response_filters:
+              - http_codes: [400]
+                action: IGNORE


We seem to only ignore this one if the message is This profileID is not authorized to use Amazon Attribution. Should we add this condition here too?

Signed-off-by: Artem Inzhyyants <[email protected]>

artem1205 · 2024-11-12T14:36:09Z

The code seems good. Overall, my concern is mostly to make sure we have the right safety net on this one. Can we test a couple of syncs using regression testing?

Run in https://github.com/airbytehq/airbyte/actions/runs/11799238139/job/32867273755

…azon-ads-migrate-low-code # Conflicts: # airbyte-integrations/connectors/source-amazon-ads/metadata.yaml # airbyte-integrations/connectors/source-amazon-ads/pyproject.toml # airbyte-integrations/connectors/source-amazon-ads/source_amazon_ads/streams/attribution_report.py # airbyte-integrations/connectors/source-amazon-ads/source_amazon_ads/streams/common.py # airbyte-integrations/connectors/source-amazon-ads/source_amazon_ads/streams/portfolios.py # docs/integrations/sources/amazon-ads.md

Signed-off-by: Artem Inzhyyants <[email protected]>

…azon-ads-migrate-low-code # Conflicts: # airbyte-integrations/connectors/source-amazon-ads/metadata.yaml # airbyte-integrations/connectors/source-amazon-ads/pyproject.toml # airbyte-integrations/connectors/source-amazon-ads/unit_tests/integrations/test_report_streams.py # docs/integrations/sources/amazon-ads.md

Signed-off-by: Artem Inzhyyants <[email protected]>

artem1205 · 2024-11-15T17:58:54Z

UPD: tested with regression tool locally; fix 2156e2c,

Results (1467.41s (0:24:27)):
      33 passed
       2 failed
         - regression_tests/test_discover.py:18 test_catalog_are_the_same
         - validation_tests/test_read.py:31 test_read
       6 skipped

test_catalog_are_the_same: streams outputted in different order. (expected)
test_read: No RUNNING status for empty streams (expected)

maxi297

test_catalog_are_the_same: streams outputted in different order. (expected)

I know the diff of the tests can show problems with the order of the streams (I've already been confused by this) but the test seem to remove ordering issue. Can we please validate this a second time knowing this?

test_read: No RUNNING status for empty streams (expected)

Does the test validates the records before failing on the stream statuses? Are there other things that were not validated by regression tests because it failed on the status?

maxi297 · 2024-11-19T13:40:31Z

airbyte-integrations/connectors/source-amazon-ads/source_amazon_ads/run.py

+                )
+            ).decode()
+        )
+        return None


We should probably:

init_uncaught_exception_handler

raise here

Else, the error message won't be so clear if the SourceAmazonAds.init fails

added init_uncaught_exception_handler

maxi297 · 2024-11-19T13:45:21Z

airbyte-integrations/connectors/source-amazon-ads/source_amazon_ads/manifest.yaml

+# TODO: change concurrency after migration of async based reports
+concurrency_level:
+  type: ConcurrencyLevel
+  default_concurrency: 1


Setting concurrency to 1 might be dangerous memory wise in some cases. What this does is that it will allow for only one thread to run at the same time. However, streams are processed in two different threads: one that generates the partitions and another that emits record from those partitions. If we were to have a case where there are a lot of partitions (thing about IssueComments in source-jira for example), we would generate all those partitions without a reader thread to consume them which could lead to memory issues.

So regarding source-amazon-ads, I would have two questions:

Do we have cases where we have a lot of partitions? I know we use a list partition router on the config profiles which should be small but do we have other cases?

Why are we afraid of enabling concurrency?

Do we have cases where we have a lot of partitions? I know we use a list partition router on the config profiles which should be small but do we have other cases?

No, only profile stream is used as a Parent, usually up to 5 profiles.

Why are we afraid of enabling concurrency?

Not at all, I'll set this to default value.

UPD: with ignoring these lines test read is green.

…azon-ads-migrate-low-code

Signed-off-by: Artem Inzhyyants <[email protected]>

maxi297

Approving this change given the green regression testing, thanks Artem!

artem1205 added 2 commits November 1, 2024 20:05

Airbyte CDK: bump CDK to v6

c719051

Signed-off-by: Artem Inzhyyants <[email protected]>

Airbyte CDK: migrate to low-code

9c22411

Signed-off-by: Artem Inzhyyants <[email protected]>

artem1205 self-assigned this Nov 1, 2024

octavia-squidington-iii added area/connectors Connector related issues connectors/source/amazon-ads labels Nov 1, 2024

artem1205 added 3 commits November 2, 2024 18:23

Airbyte CDK: update docs

bcdd20c

Signed-off-by: Artem Inzhyyants <[email protected]>

Airbyte CDK: update docs

6754392

Signed-off-by: Artem Inzhyyants <[email protected]>

Airbyte CDK: update docs

a893a46

[skip ci] Signed-off-by: Artem Inzhyyants <[email protected]>

octavia-squidington-iii added the area/documentation Improvements or additions to documentation label Nov 4, 2024

artem1205 added 3 commits November 4, 2024 18:05

Amazon Ads: migrate attribution streams

4df7e95

Signed-off-by: Artem Inzhyyants <[email protected]>

Amazon Ads: update CDK

a59995f

Signed-off-by: Artem Inzhyyants <[email protected]>

octavia-squidington-iii removed the area/documentation Improvements or additions to documentation label Nov 5, 2024

artem1205 added 11 commits November 5, 2024 13:41

Amazon Ads: fix attribution reports tests + manifest

f9b6472

Signed-off-by: Artem Inzhyyants <[email protected]>

Amazon Ads: ref start date

150ca9f

Signed-off-by: Artem Inzhyyants <[email protected]>

Amazon Ads: ref

8d52447

[skip ci] Signed-off-by: Artem Inzhyyants <[email protected]>

Amazon Ads: ref

759f30b

Signed-off-by: Artem Inzhyyants <[email protected]>

Amazon Ads: update unit tests

a67f839

Signed-off-by: Artem Inzhyyants <[email protected]>

Amazon Ads: remove old tests

81b0693

Signed-off-by: Artem Inzhyyants <[email protected]>

Amazon Ads: remove old tests

f50ceb0

Signed-off-by: Artem Inzhyyants <[email protected]>

Amazon Ads: fix *-sponsored tests

56d38c6

Signed-off-by: Artem Inzhyyants <[email protected]>

Amazon Ads: add error handlers

7704da5

Signed-off-by: Artem Inzhyyants <[email protected]>

Amazon Ads: fmt

efc101d

Signed-off-by: Artem Inzhyyants <[email protected]>

Merge remote-tracking branch 'origin/master' into artem1205/source-am…

8770c43

…azon-ads-migrate-low-code # Conflicts: # airbyte-integrations/connectors/source-amazon-ads/metadata.yaml # airbyte-integrations/connectors/source-amazon-ads/pyproject.toml

vercel bot deployed to Preview November 6, 2024 23:51 View deployment

Amazon Ads: fix tests

78258b4

Signed-off-by: Artem Inzhyyants <[email protected]>

artem1205 mentioned this pull request Nov 7, 2024

feat(airbyte-cdk): add jinja macros today_with_timezone #48400

Merged

2 tasks

artem1205 added 2 commits November 8, 2024 11:31

Airbyte CDK: bump CDK

7e4dfc5

Signed-off-by: Artem Inzhyyants <[email protected]>

Amazon Ads: fix streams

7af9e1f

Signed-off-by: Artem Inzhyyants <[email protected]>

octavia-squidington-iii added the area/documentation Improvements or additions to documentation label Nov 8, 2024

vercel bot deployed to Preview November 8, 2024 15:18 View deployment

artem1205 marked this pull request as ready for review November 8, 2024 15:24

artem1205 requested a review from a team November 8, 2024 15:28

Amazon Ads: ref

611a26c

Signed-off-by: Artem Inzhyyants <[email protected]>

maxi297 reviewed Nov 11, 2024

View reviewed changes

artem1205 added 4 commits November 12, 2024 14:22

Airbyte CDK: add comment

769abf4

Signed-off-by: Artem Inzhyyants <[email protected]>

Airbyte CDK: ref error handlers

de42b45

Signed-off-by: Artem Inzhyyants <[email protected]>

Airbyte CDK: ref error handlers

3ea0f19

Signed-off-by: Artem Inzhyyants <[email protected]>

Airbyte CDK: ref

e4972fb

Signed-off-by: Artem Inzhyyants <[email protected]>

vercel bot deployed to Preview November 12, 2024 14:44 View deployment

artem1205 added 3 commits November 13, 2024 00:09

Airbyte CDK: fix test

1c29146

Signed-off-by: Artem Inzhyyants <[email protected]>

Airbyte CDK: fix CDK model failure type

727c330

Signed-off-by: Artem Inzhyyants <[email protected]>

vercel bot deployed to Preview November 13, 2024 14:39 View deployment

Amazon Ads: fix

2156e2c

Signed-off-by: Artem Inzhyyants <[email protected]>

artem1205 requested a review from maxi297 November 19, 2024 13:37

maxi297 reviewed Nov 19, 2024

View reviewed changes

artem1205 added 5 commits November 19, 2024 15:00

Merge remote-tracking branch 'origin/master' into artem1205/source-am…

6cbbfff

…azon-ads-migrate-low-code

Amazon ADS: add init_uncaught_exception_handler

a18b7ef

Signed-off-by: Artem Inzhyyants <[email protected]>

Amazon ADS: fmt

9c7700e

Signed-off-by: Artem Inzhyyants <[email protected]>

Amazon ADS: update concurrency

5ff100f

Signed-off-by: Artem Inzhyyants <[email protected]>

Amazon ADS: update concurrency

aecef46

Signed-off-by: Artem Inzhyyants <[email protected]>

artem1205 requested a review from maxi297 November 19, 2024 16:01

maxi297 approved these changes Nov 20, 2024

View reviewed changes

artem1205 merged commit 696b979 into master Nov 20, 2024
34 checks passed

artem1205 deleted the artem1205/source-amazon-ads-migrate-low-code branch November 20, 2024 13:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ref(source-amazon-ads): migrate to low code #48116

ref(source-amazon-ads): migrate to low code #48116

artem1205 commented Nov 1, 2024 •

edited

Loading

vercel bot commented Nov 1, 2024 •

edited

Loading

maxi297 left a comment

maxi297 Nov 11, 2024

artem1205 Nov 12, 2024

maxi297 Nov 11, 2024

artem1205 Nov 12, 2024

maxi297 Nov 11, 2024

artem1205 Nov 12, 2024

artem1205 commented Nov 12, 2024

artem1205 commented Nov 15, 2024

maxi297 left a comment

maxi297 Nov 19, 2024

artem1205 Nov 19, 2024

maxi297 Nov 19, 2024

artem1205 Nov 19, 2024 •

edited

Loading

maxi297 left a comment

ref(source-amazon-ads): migrate to low code #48116

ref(source-amazon-ads): migrate to low code #48116

Conversation

artem1205 commented Nov 1, 2024 • edited Loading

What

How

Review guide

User Impact

Can this PR be safely reverted and rolled back?

vercel bot commented Nov 1, 2024 • edited Loading

maxi297 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artem1205 commented Nov 12, 2024

artem1205 commented Nov 15, 2024

maxi297 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artem1205 Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

maxi297 left a comment

Choose a reason for hiding this comment

artem1205 commented Nov 1, 2024 •

edited

Loading

vercel bot commented Nov 1, 2024 •

edited

Loading

artem1205 Nov 19, 2024 •

edited

Loading