🎉Source Bing Ads: Add Report streams #5750

yaroslav-dudar · 2021-08-31T07:48:46Z

What

added report streams to the connector #5075

How

Describe the solution

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
- Changelog updated in docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
PR name follows PR naming conventions
Connector version bumped like described here

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

…udar/5075-bing-ads-reports

yaroslav-dudar · 2021-08-31T09:05:00Z

/test connector=connectors/source-bing-ads

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

lazebnyi

LGTM

Leave few minor comments.

airbyte-integrations/connectors/source-bing-ads/source_bing_ads/reports.py

docs/integrations/sources/bing-ads.md

airbyte-integrations/connectors/source-bing-ads/source_bing_ads/source.py

…udar/5075-bing-ads-reports

airbyte-integrations/connectors/source-bing-ads/source_bing_ads/client.py

midavadim · 2021-09-01T22:07:51Z

airbyte-integrations/connectors/source-bing-ads/source_bing_ads/spec.json

+        "default": "2020-01-01",
+        "description": "From which date perform initial sync for report related streams. In YYYY-MM-DD format"
+      },
+      "report_aggregation": {


need clarification:

So now user should choose one of the aggregation types,

Did we consider choosing a few types at the same time - hourly and monthly for example.

Do you have any ideas on how it works in other EL tools?

Did we consider choosing a few types at the same time - hourly and monthly for example.
no, I don't consider this. What's the purpose of that ?
Do you have any ideas on how it works in other EL tools?
one tool has hardcoded Daily value, another tool has 2 separate streams for Daily and Hourly aggregation. I think my solution is more flexible

Please raise this question on Airbyte review: "Did we consider choosing a few types at the same time - hourly and monthly for example"

In my understanding, we should support sync of hourly, daily or another 'agreggations' streams simultaneously.

We may also discuss this question with Maxim before Airbyte review.

I think we can offer flexibility here pretty easily on top of the current code and therefore imo we should.

Options in spec could be boolean toggles for each of hourly, daily, weekly, monthly, where all off could mean no report streams (and possibly help for backwards compat.?)

In SourceBingAds class, we'd then need to run some logic checking which of those toggles are on/off and setting up relevant report streams for each. e.g.
hourly and monthly are toggled on, so we add streams:

AccountPerformanceReportHourly

AccountPerformanceReportMonthly

AdGroupPerformanceReportHourly

AdGroupPerformanceReportMonthly

and so on and so on for all reports...

This would give the user ultimate flexibility and shouldn't require any major changes to the code, wdyt?

Phlair · 2021-09-06T11:27:54Z

Hey @yaroslav-dudar, would you mind adding a bootstrap.md in with this PR please (as described here). Also reading order would be great to help me jump in 👍

yaroslav-dudar · 2021-09-06T14:44:34Z

Hey @yaroslav-dudar, would you mind adding a bootstrap.md in with this PR please (as described here). Also reading order would be great to help me jump in

@Phlair added reading order and bootstrap 66d69d5

Phlair

Really like the approach using a Mixin, clean!

Main change request on allowing for multiple report frequencies, seems like low-hanging fruit worth taking to be on par / better than other like-for-like tools.

Other point worth mentioning is on the state-per-account-id comment. I know this stream already implements this so I won't let it block merge but after discussing with Eugene I think we need a solution to avoid the state becoming too large (we can discuss on the comment itself in the code)

airbyte-integrations/connectors/source-bing-ads/source_bing_ads/reports.py

Phlair · 2021-09-06T16:32:29Z

airbyte-integrations/connectors/source-bing-ads/source_bing_ads/reports.py

+        for account in source_bing_ads.source.Accounts(self.client, self.config).read_records(SyncMode.full_refresh):
+            yield {"account_id": account["Id"]}


I'm assuming this isn't in any sort of time-order using account_id as the slicing of stream slices but you're saving state for each account ID.

I've discussed this with @keu recently and my main concern with this approach is that we have a potentially boundless state size. Is there a hard limit on the amount of account IDs there can be or could it potentially be millions?

I think there should be strict limit of amount of accounts per user

https://help.ads.microsoft.com/#apex/ads/en/52001/0

Phlair · 2021-09-06T16:55:06Z

airbyte-integrations/connectors/source-bing-ads/source_bing_ads/spec.json

+        "default": "2020-01-01",
+        "description": "From which date perform initial sync for report related streams. In YYYY-MM-DD format"
+      },
+      "report_aggregation": {


I think we can offer flexibility here pretty easily on top of the current code and therefore imo we should.

Options in spec could be boolean toggles for each of hourly, daily, weekly, monthly, where all off could mean no report streams (and possibly help for backwards compat.?)

In SourceBingAds class, we'd then need to run some logic checking which of those toggles are on/off and setting up relevant report streams for each. e.g.
hourly and monthly are toggled on, so we add streams:

AccountPerformanceReportHourly

AccountPerformanceReportMonthly

AdGroupPerformanceReportHourly

AdGroupPerformanceReportMonthly

and so on and so on for all reports...

This would give the user ultimate flexibility and shouldn't require any major changes to the code, wdyt?

Phlair · 2021-09-06T17:00:10Z

Also, thanks @yaroslav-dudar for the bootstrap and reading order 👍 !!

…udar/5075-bing-ads-reports

yaroslav-dudar · 2021-09-08T08:49:01Z

/test connector=connectors/source-bing-ads

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

yaroslav-dudar · 2021-09-08T09:15:13Z

/test connector=connectors/source-bing-ads

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

yaroslav-dudar · 2021-09-09T07:54:15Z

/test connector=connectors/source-bing-ads

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

yaroslav-dudar · 2021-09-09T08:04:54Z

@Phlair also please take a look on these changes e1c17c0 . I suspect that you have more knowledge on gradle configuration

Phlair

Awesome, thanks for adding those toggles, offering max flexibility for the user now 👍

It's not ideal bumping down setuptools but I think it should be temporary since there is an open issue and PR to fix that problem in BingAds lib (see my comment)

Phlair · 2021-09-09T11:20:32Z

airbyte-integrations/connectors/source-bing-ads/build.gradle

@@ -7,3 +9,14 @@ plugins {
 airbytePython {
    moduleDirectory 'source_bing_ads'
 }
+
+// setuptools 58.* removed support for use_2to3 which leads to the following issue:
+// error in suds-jurko setup command: use_2to3 is invalid.


looks like there is an open issue for this on BingAds-Python-SDK repo.

Might be worth adding a link to that with a note to update the bingads lib version and remove this step in gradle once it is resolved

hey, Phlair. Will this problem be solved by upgrade bing ads version?

Phlair · 2021-09-09T11:22:13Z

airbyte-integrations/connectors/source-bing-ads/integration_tests/invalid_config.json

+  "hourly_reports": true,
+  "daily_reports": false,
+  "weekly_reports": false,
+  "monthly_reports": true


best coverage here would be testing all of these, any reason not to? (it doesn't need to block this PR, a separate issue maybe?)

it's an invalid config, actual config stored in github secrets. I think we can update secret even without an issue

yaroslav-dudar · 2021-09-09T15:13:25Z

/test connector=connectors/source-bing-ads

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

yaroslav-dudar · 2021-09-09T15:23:34Z

/publish connector=connectors/source-bing-ads

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

SPTKL · 2022-07-12T16:11:30Z

Hi all! Why are we only supporting a subset of the columns provided by the bing ads api?
e.g. based on the CampaignPerformanceReportColumn, a lot of the columns are omitted. (e.g. Conversions is missing)

https://discuss.airbyte.io/t/bing-ads-missing-a-lot-of-fields-is-this-intentional/1824

yaroslav-dudar added 10 commits August 18, 2021 16:09

initial report streams

0cf147b

refactoring; added performance streams; start_date param

6df7000

basic incremental sync for reports

ca549ef

Merge branch 'master' of github.com:airbytehq/airbyte into yaroslav-d…

4648166

…udar/5075-bing-ads-reports

added cursor calculation based of aggregated report data; refactoring

04cba0b

updated state management; added schemas for reports

9588051

upd PK; fix acceptance tests; improve error logging

cbea441

added unit tests

0e6fc48

upd spec; added unit tests; version bump

986a505

Merge branch 'master' of github.com:airbytehq/airbyte into yaroslav-d…

d31e2c0

…udar/5075-bing-ads-reports

yaroslav-dudar requested a review from antixar August 31, 2021 07:48

github-actions bot added area/connectors Connector related issues area/documentation Improvements or additions to documentation labels Aug 31, 2021

yaroslav-dudar changed the title ~~tadaSource Bing Ads: Add Report streams~~ 🎉Source Bing Ads: Add Report streams Aug 31, 2021

yaroslav-dudar linked an issue Aug 31, 2021 that may be closed by this pull request

🚀 Bing Ads Source: Implement Report streams #5075

Closed

antixar requested review from lazebnyi and midavadim and removed request for antixar August 31, 2021 08:43

jrhizor temporarily deployed to more-secrets August 31, 2021 09:07 Inactive

lazebnyi approved these changes Aug 31, 2021

View reviewed changes

yaroslav-dudar added 2 commits September 1, 2021 14:30

minor fixes; refactoring

d8b0923

Merge branch 'master' of github.com:airbytehq/airbyte into yaroslav-d…

94beee7

…udar/5075-bing-ads-reports

yaroslav-dudar requested a review from lazebnyi September 1, 2021 11:33

lazebnyi approved these changes Sep 1, 2021

View reviewed changes

midavadim reviewed Sep 1, 2021

View reviewed changes

typing fix

5f17a53

yaroslav-dudar requested a review from midavadim September 2, 2021 06:28

midavadim approved these changes Sep 3, 2021

View reviewed changes

yaroslav-dudar marked this pull request as ready for review September 3, 2021 16:53

added bootstrap

66d69d5

Phlair suggested changes Sep 6, 2021

View reviewed changes

yaroslav-dudar added 4 commits September 7, 2021 11:00

Merge branch 'master' of github.com:airbytehq/airbyte into yaroslav-d…

96bac88

…udar/5075-bing-ads-reports

upd spec; added report per agg

071ade4

modified schema fetching and streams selection

e992852

upd docs; fixed tests

1af74dc

yaroslav-dudar requested a review from Phlair September 8, 2021 07:56

jrhizor temporarily deployed to more-secrets September 8, 2021 08:51 Inactive

jrhizor temporarily deployed to more-secrets September 8, 2021 09:17 Inactive

added custom setuptools version

e1c17c0

jrhizor temporarily deployed to more-secrets September 9, 2021 07:57 Inactive

Phlair approved these changes Sep 9, 2021

View reviewed changes

upd primary_keys

f6c7383

jrhizor temporarily deployed to more-secrets September 9, 2021 15:16 Inactive

jrhizor temporarily deployed to more-secrets September 9, 2021 15:26 Inactive

yaroslav-dudar merged commit 70513bc into master Sep 9, 2021

yaroslav-dudar deleted the yaroslav-dudar/5075-bing-ads-reports branch September 9, 2021 16:35

jrhizor mentioned this pull request Sep 16, 2021

Bump Airbyte version from 0.29.17-alpha to 0.29.18-alpha #6125

Merged

igrankova added connectors/sources-api connectors/source/bing-ads labels Jan 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🎉Source Bing Ads: Add Report streams #5750

🎉Source Bing Ads: Add Report streams #5750

yaroslav-dudar commented Aug 31, 2021 •

edited

Loading

yaroslav-dudar commented Aug 31, 2021 •

edited by github-actions bot

Loading

lazebnyi left a comment •

edited

Loading

midavadim Sep 1, 2021

yaroslav-dudar Sep 2, 2021 •

edited

Loading

midavadim Sep 3, 2021

midavadim Sep 3, 2021

Phlair Sep 6, 2021

Phlair commented Sep 6, 2021

yaroslav-dudar commented Sep 6, 2021 •

edited

Loading

Phlair left a comment

Phlair Sep 6, 2021

yaroslav-dudar Sep 7, 2021

Phlair Sep 6, 2021

Phlair commented Sep 6, 2021

yaroslav-dudar commented Sep 8, 2021 •

edited by github-actions bot

Loading

yaroslav-dudar commented Sep 8, 2021 •

edited by github-actions bot

Loading

yaroslav-dudar commented Sep 9, 2021 •

edited by github-actions bot

Loading

yaroslav-dudar commented Sep 9, 2021

Phlair left a comment

Phlair Sep 9, 2021

qiyang0221 Sep 14, 2021

Phlair Sep 9, 2021

yaroslav-dudar Sep 9, 2021

yaroslav-dudar commented Sep 9, 2021 •

edited by github-actions bot

Loading

yaroslav-dudar commented Sep 9, 2021 •

edited by github-actions bot

Loading

SPTKL commented Jul 12, 2022 •

edited

Loading

		for account in source_bing_ads.source.Accounts(self.client, self.config).read_records(SyncMode.full_refresh):
		yield {"account_id": account["Id"]}

🎉Source Bing Ads: Add Report streams #5750

🎉Source Bing Ads: Add Report streams #5750

Conversation

yaroslav-dudar commented Aug 31, 2021 • edited Loading

What

How

Recommended reading order

Pre-merge Checklist

Community member or Airbyter

Airbyter

yaroslav-dudar commented Aug 31, 2021 • edited by github-actions bot Loading

lazebnyi left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaroslav-dudar Sep 2, 2021 • edited Loading

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

Phlair commented Sep 6, 2021

yaroslav-dudar commented Sep 6, 2021 • edited Loading

Phlair 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

Phlair commented Sep 6, 2021

yaroslav-dudar commented Sep 8, 2021 • edited by github-actions bot Loading

yaroslav-dudar commented Sep 8, 2021 • edited by github-actions bot Loading

yaroslav-dudar commented Sep 9, 2021 • edited by github-actions bot Loading

yaroslav-dudar commented Sep 9, 2021

Phlair 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

yaroslav-dudar commented Sep 9, 2021 • edited by github-actions bot Loading

yaroslav-dudar commented Sep 9, 2021 • edited by github-actions bot Loading

SPTKL commented Jul 12, 2022 • edited Loading

yaroslav-dudar commented Aug 31, 2021 •

edited

Loading

yaroslav-dudar commented Aug 31, 2021 •

edited by github-actions bot

Loading

lazebnyi left a comment •

edited

Loading

yaroslav-dudar Sep 2, 2021 •

edited

Loading

yaroslav-dudar commented Sep 6, 2021 •

edited

Loading

yaroslav-dudar commented Sep 8, 2021 •

edited by github-actions bot

Loading

yaroslav-dudar commented Sep 8, 2021 •

edited by github-actions bot

Loading

yaroslav-dudar commented Sep 9, 2021 •

edited by github-actions bot

Loading

yaroslav-dudar commented Sep 9, 2021 •

edited by github-actions bot

Loading

yaroslav-dudar commented Sep 9, 2021 •

edited by github-actions bot

Loading

SPTKL commented Jul 12, 2022 •

edited

Loading