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 TikTok Marketing - Add country_code and platform audience reports #22134

Merged
merged 20 commits into from
Apr 5, 2023
Merged

🎉 Source TikTok Marketing - Add country_code and platform audience reports #22134

merged 20 commits into from
Apr 5, 2023

Conversation

roisinbolt
Copy link
Contributor

@roisinbolt roisinbolt commented Jan 31, 2023

What

In order to use the TikTok Marketing Audience report effectively we need to have a view of the country_code and platform level in ad pulls.

#22064

How

Both country_code and platform are available dimensions in the Audience Report.

In the current Airbyte connector for TikTok Marketing CampaignsAudienceReportsByCountry has been added as an available stream. I would like to expand the approach taken for this stream to have reports ByCountry and ByPlatform for all id levels (ad, adgroup, campaign, advertiser).

Recommended reading order

  1. streams.py
  2. source.py
  3. integration_tests/

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
  • If new credentials are required for use in CI, add them to GSM. Instructions.
  • /test connector=connectors/<name> command is passing
  • New Connector version released on Dockerhub and connector version bumped by running the /publish command described here

@roisinbolt roisinbolt changed the title Source TikTok Marketing - Add country_code and platform audience reports 🎉 Source TikTok Marketing - Add country_code and platform audience reports Jan 31, 2023
@roisinbolt roisinbolt marked this pull request as ready for review January 31, 2023 16:20
@octavia-squidington-iv octavia-squidington-iv added the area/documentation Improvements or additions to documentation label Jan 31, 2023
…hub.com:roisinbolt/airbyte into roisin-tiktok-basicreport-non-temp-id-dim-split
@sajarin sajarin requested a review from midavadim February 1, 2023 16:35
@marcosmarxm
Copy link
Member

Thanks for the contribution @roisinbolt I'll take a look later today.

@roisinbolt
Copy link
Contributor Author

@marcosmarxm Thanks! I don't have productions credentials locally to test part of the acceptance test suite.

@misteryeo
Copy link
Contributor

Issue was linked to Harvestr Discovery: Source TikTok Marketing: Add country_code and platform Audience Reports

@roisinbolt
Copy link
Contributor Author

Hey @marcosmarxm , just wanted to check in on when you'll have a chance to review this PR?

@roisinbolt
Copy link
Contributor Author

Hey @misteryeo , do you have any context on where this PR sits in the review backlog?

@marcosmarxm
Copy link
Member

Sorry the delay @roisinbolt adding this to my priority list!
@sh4sh fyi too!

@roisinbolt
Copy link
Contributor Author

Hey @marcosmarxm ! I was just wondering if you have an idea of when this will start being reviewed? Just whether it will be this week/next week?

@marcosmarxm
Copy link
Member

marcosmarxm commented Feb 21, 2023

/test connector=connectors/source-tiktok-marketing

🕑 connectors/source-tiktok-marketing https://github.com/airbytehq/airbyte/actions/runs/4237521533
❌ connectors/source-tiktok-marketing https://github.com/airbytehq/airbyte/actions/runs/4237521533
🐛 https://gradle.com/s/fthiqhut5tysq

Build Failed

Test summary info:

Could not find result summary

@marcosmarxm
Copy link
Member

@midavadim can you check this contribution?

@roisinbolt
Copy link
Contributor Author

@marcosmarxm @midavadim Is there any update on when this will be reviewed?

@roisinbolt
Copy link
Contributor Author

Hey @marcosmarxm @midavadim @sh4sh @misteryeo

It has been ~2months since this contribution was originally opened in a PR - it would be helpful to understand any changes I need to make and/or get it merged in to the repo.

@bazarnov bazarnov self-requested a review March 30, 2023 12:49
@bazarnov
Copy link
Collaborator

@roisinbolt Hello, could you please update your branch and PR up to the latest master and fix the conflicts, so we can move forward? I've assigned myself here as a reviewer for a fast review. Your contribution seems reasonable and we would like to have it the part of the newest TikTok release we're preparing these days.

@roisinbolt
Copy link
Contributor Author

@bazarnov Thanks for picking this up! That is done there

@roisinbolt
Copy link
Contributor Author

As per this PR _hourly granularity for audience reports is deprecated so I think we shouldn't have to add that granularity split? I can add in the daily.

@bazarnov
Copy link
Collaborator

bazarnov commented Apr 4, 2023

As per this PR _hourly granularity for audience reports is deprecated so I think we shouldn't have to add that granularity split? I can add in the daily.

Please add them all, we'll clean up later on.

@roisinbolt
Copy link
Contributor Author

As per this PR _hourly granularity for audience reports is deprecated so I think we shouldn't have to add that granularity split? I can add in the daily.

Please add them all, we'll clean up later on.

Okay I will do this now. Could you please link me to where you are seeing this failure?

@bazarnov
Copy link
Collaborator

bazarnov commented Apr 4, 2023

As per this PR _hourly granularity for audience reports is deprecated so I think we shouldn't have to add that granularity split? I can add in the daily.

Please add them all, we'll clean up later on.

Okay I will do this now. Could you please link me to where you are seeing this failure?

https://github.com/airbytehq/airbyte/actions/runs/4606074652/jobs/8138956687 (not sure you can see those logs)

@roisinbolt
Copy link
Contributor Author

As per this PR _hourly granularity for audience reports is deprecated so I think we shouldn't have to add that granularity split? I can add in the daily.

Please add them all, we'll clean up later on.

Okay I will do this now. Could you please link me to where you are seeing this failure?

https://github.com/airbytehq/airbyte/actions/runs/4606074652/jobs/8138956687 (not sure you can see those logs)

yes I can see those thanks! The _hourly empty_streams being missing looks like a WARN so should be fine without those, but I have added in all the suggested ones now.

@bazarnov
Copy link
Collaborator

bazarnov commented Apr 4, 2023

As per this PR _hourly granularity for audience reports is deprecated so I think we shouldn't have to add that granularity split? I can add in the daily.

Please add them all, we'll clean up later on.

Okay I will do this now. Could you please link me to where you are seeing this failure?

https://github.com/airbytehq/airbyte/actions/runs/4606074652/jobs/8138956687 (not sure you can see those logs)

yes I can see those thanks! The _hourly empty_streams being missing looks like a WARN so should be fine without those, but I have added in all the suggested ones now.

Thank you!

Also, please change the version from 2.0.5 > 2.0.6 here: https://github.com/roisinbolt/airbyte/blob/roisin-tiktok-basicreport-non-temp-id-dim-split/airbyte-integrations/connectors/source-tiktok-marketing/Dockerfile#L35

@bazarnov
Copy link
Collaborator

bazarnov commented Apr 4, 2023

/test connector=connectors/source-tiktok-marketing

🕑 connectors/source-tiktok-marketing https://github.com/airbytehq/airbyte/actions/runs/4610867448
✅ connectors/source-tiktok-marketing https://github.com/airbytehq/airbyte/actions/runs/4610867448
Python tests coverage:

Name                                  Stmts   Miss  Cover
---------------------------------------------------------
source_tiktok_marketing/__init__.py       2      0   100%
source_tiktok_marketing/source.py        68      7    90%
source_tiktok_marketing/streams.py      390     41    89%
---------------------------------------------------------
TOTAL                                   460     48    90%

Build Passed

Test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:100: The previous and actual specifications are identical.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_incremental.py:212: Skipping new incremental test based on acceptance-test-config.yml
================== 91 passed, 2 skipped in 3382.22s (0:56:22) ==================

@bazarnov
Copy link
Collaborator

bazarnov commented Apr 4, 2023

Also, please change the version from 2.0.5 > 2.0.6 here: https://github.com/roisinbolt/airbyte/blob/roisin-tiktok-basicreport-non-temp-id-dim-split/airbyte-integrations/connectors/source-tiktok-marketing/Dockerfile#L35

@roisinbolt we're currently waiting for this final change, before the merge.

…hub.com:roisinbolt/airbyte into roisin-tiktok-basicreport-non-temp-id-dim-split
@roisinbolt
Copy link
Contributor Author

@bazarnov that should be done there! The change was sitting on my local and I thought it had been pushed 😅

@bazarnov
Copy link
Collaborator

bazarnov commented Apr 5, 2023

/test connector=connectors/source-tiktok-marketing

🕑 connectors/source-tiktok-marketing https://github.com/airbytehq/airbyte/actions/runs/4617784877
✅ connectors/source-tiktok-marketing https://github.com/airbytehq/airbyte/actions/runs/4617784877
Python tests coverage:

Name                                  Stmts   Miss  Cover
---------------------------------------------------------
source_tiktok_marketing/__init__.py       2      0   100%
source_tiktok_marketing/source.py        68      7    90%
source_tiktok_marketing/streams.py      390     41    89%
---------------------------------------------------------
TOTAL                                   460     48    90%

Build Passed

Test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:100: The previous and actual specifications are identical.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_incremental.py:212: Skipping new incremental test based on acceptance-test-config.yml
================== 91 passed, 2 skipped in 3271.74s (0:54:31) ==================

@bazarnov
Copy link
Collaborator

bazarnov commented Apr 5, 2023

@airbytehq/connector-extensibility @airbytehq/connector-operations I'm going to merge this contribution today. There are no breaking changes, but enhancements only. Just FYI.

@bazarnov
Copy link
Collaborator

bazarnov commented Apr 5, 2023

/publish connector=connectors/source-tiktok-marketing

🕑 Publishing the following connectors:
connectors/source-tiktok-marketing
https://github.com/airbytehq/airbyte/actions/runs/4618441692


Connector Did it publish? Were definitions generated?
connectors/source-tiktok-marketing

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

@roisinbolt
Copy link
Contributor Author

@bazarnov Thanks for your help on this!

@bazarnov bazarnov enabled auto-merge (squash) April 5, 2023 13:47
@bazarnov bazarnov disabled auto-merge April 5, 2023 13:47
@bazarnov
Copy link
Collaborator

bazarnov commented Apr 5, 2023

@marcosmarxm @evantahler Could you please take a look at how we could merge this PR? The publish+auto bump was successful. Now, to merge this in we need to pass CI checks, and this is apparently the blocker.

@bazarnov bazarnov requested a review from a team April 5, 2023 14:11
@marcosmarxm marcosmarxm merged commit 32fb8c2 into airbytehq:master Apr 5, 2023
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 bounty community connectors/source/tiktok-marketing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants