🐛 Source Bing Ads - Fix Missing Dimensional Columns For Reports

[rach-r]

What

Adding dimensional columns in reports that can't fully be retrieved in other streams; allowing users to make analysis on deleted items (and trend comparison over time).

Closes #23867.

How

Adding basic dimensional columns in report streams, as API and documentation allows it (example: here is the list of available columns to retrieve for CampaignsPerformanceReport).

Pre-merge Checklist

Updating a connector

Community member or Airbyter

• Grant edit access to maintainers (instructions)
• 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
  (https://docs.google.com/document/d/1ypdgmwmEHWv-TrO4_YOQ7pAJGVrMp5BOkEVh831N260/edit?usp=sharing)
  • Changelog updated in docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
• PR name follows PR naming conventions

Tests

Unit

[sajarin]

Hey @rach-r, this looks good thanks for the contribution!

Could you bump the dockerfile? Also I understand that there are more missing dimensional columns that could be potentially added in a future PR. Is there something unique or compelling about the missing dimensional columns you've added here?

[rach-r]

Hey @rach-r, this looks good thanks for the contribution!

Could you bump the dockerfile? Also I understand that there are more missing dimensional columns that could be potentially added in a future PR. Is there something unique or compelling about the missing dimensional columns you've added here?

Hey @sajarin, thanks a lot for taking time to review this PR 👋

You're right, there are many other columns available; and for this PR I chose the most "basic" ones for a human to be able to read reports easily (at least have the Name and Type of the item). I am planning to do a comprehensive one in few weeks but I'm waiting a bit since other contributors wanted to work on it too (see #18476)

[sherifnada]

/test connector=source-bing-ads

🕑 source-bing-ads https://github.com/airbytehq/airbyte/actions/runs/4377538019
❌ source-bing-ads https://github.com/airbytehq/airbyte/actions/runs/4377538019
🐛 https://gradle.com/s/ui2otwn5yvdtw

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/connector_acceptance_test/plugin.py:63: Skipping TestIncremental.test_two_sequential_reads: SAT doesn't support complex nested states used in incremental report streams
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:100: The previous and actual specifications are identical.
============= 1 failed, 33 passed, 2 skipped in 587.46s (0:09:47) ==============

[rach-r]

@sajarin @sherifnada from my understanding the tests were failing on "expected records vs actual records" as I can't add new fields with Airbyte's sandbox account data. As we are going to have multiple PRs to add fields for this connector, I've made a small change to allow extra fields. If you're OK with this, could you relaunch /test connector=source-bing-ads?

Again, thanks a lot for your help 🙏

[sajarin]

/test connector=source-bing-ads

🕑 source-bing-ads https://github.com/airbytehq/airbyte/actions/runs/4407092979
❌ source-bing-ads https://github.com/airbytehq/airbyte/actions/runs/4407092979
🐛 https://gradle.com/s/nhgpudesvxoqe

Build Failed

Test summary info:

Could not find result summary

[rach-r]

So according to this it's actually not possible to both have extra_records and extra_fields enabled:

So I guess I need someone who has access to the sandbox account to update the expected_records?...

[davydov-d]

@rach-r I don't think we should ignore those fields in the test config but rather modify expected records. @sajarin could you please help doing that? I'll approve this request after it passes tests, changes look good to me

[alafanechere]

/test connector=source-bing-ads

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

Build Failed

Test summary info:

Could not find result summary

[rach-r]

@davydov-d much better if someone can help me update the records indeed 🙏 otherwise @alafanechere gave me this workaround in case it takes too long to have someone available to do it. By the way I corrected a typo that made the last test fail on one stream, next one should be good 👍

[alafanechere]

/test connector=source-bing-ads

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

Build Passed

Test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/plugin.py:63: Skipping TestIncremental.test_two_sequential_reads: SAT doesn't support complex nested states used in incremental report streams
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:100: The previous and actual specifications are identical.
================== 34 passed, 2 skipped in 518.69s (0:08:38) ===================

[sajarin]

@rach-r can you rebase on master? There were a few changes to the ci/cd that should fix the error. Looking into adding the columns in our sandbox data as well but would like to get tests running successfully here.

[rach-r]

@sajarin rebase on master is done if you want to try again 🙂

[sajarin]

/test connector=source-bing-ads

🕑 source-bing-ads https://github.com/airbytehq/airbyte/actions/runs/4449829607
✅ source-bing-ads https://github.com/airbytehq/airbyte/actions/runs/4449829607
Python tests coverage:

Name                          Stmts   Miss  Cover
-------------------------------------------------
source_bing_ads/__init__.py       2      0   100%
source_bing_ads/source.py       260     10    96%
source_bing_ads/cache.py         18      1    94%
source_bing_ads/reports.py      133     17    87%
source_bing_ads/client.py        93     21    77%
-------------------------------------------------
TOTAL                           506     49    90%

Build Passed

Test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/plugin.py:63: Skipping TestIncremental.test_two_sequential_reads: SAT doesn't support complex nested states used in incremental report streams
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:100: The previous and actual specifications are identical.
================== 34 passed, 2 skipped in 661.65s (0:11:01) ===================

[rach-r]

@sajarin I can see the tests have successfully passed after the rebase, thanks for the tip!! Should I do something else?

[marcosmarxm]

@sajarin I can see the tests have successfully passed after the rebase, thanks for the tip!! Should I do something else?

This is incorrect. Tests are passing because the change is bypassed. We need to revert the acceptance-test.yml file changes and fix tests properly.

[alafanechere]

Just to summarize the status here:

• @rach-r shared a test output with passing tests
• We can't run the same tests suite because we don't have the fields seeded in our sandbox account
• I suggested @rach-r to mark the field as bypassed to make sure the rest of the changes are passing the tests. They do 🟢 .

@rach-r nothing else should be required from your side.
The next step are on our side:

1. Seed our account with the new fields
2. Revert the field bypass on acceptance test config
3. Run the tests again.
4. Publish the new connector version if test are 🟢
5. Merge

@sajarin could you take care of it ☝️ ?

@rach-r just to add a bit more context: this connector is a generally available connector with high test strictness level. It means we want to get maximum test coverage. We would be a bit more relaxed if this connector was beta or alpha, but for stability reasons we prefer to enforce this rule.

[rach-r]

Thanks a lot for the summary @alafanechere. No problem regarding the tests that need to be updated, I understand the strictness level and I don't want to introduce any bug/regression, especially for a generally available connector 👌 The temporary bypass allowed to isolate some effects and fix other things (like CI broken on master before the rebase sajarin asked for) so thanks for the tip.
And thanks for confirming there is nothing more to do on my side, I'll just wait to have someone available to update the records

[sajarin]

Thanks for the contribution @rach-r, your contributions have been merged in #24717