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

✨ TikTok Marketing Source: By country daily/hourly report streams #55681

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

sheinbergon
Copy link
Contributor

@sheinbergon sheinbergon commented Mar 10, 2025

What

Adds new daily/hourly reports streams with country breakdown. Unlike the existing audience reports, these base reports streams allow fetching some additional metrics (reach, being one of them). These streams are currently actively used in production using a modified version of the tiktok-marketing connector in our local K8s deployment, with great success

How

Added 4 new streams to the manifest.yaml, and new country_code dimension that was missing

Review guide

Simple update to manifest.yaml

User Impact

The user will be able to retrieve new streams with additional support metrics

Can this PR be safely reverted and rolled back?

  • YES 💚
  • NO ❌

Copy link

vercel bot commented Mar 10, 2025

@sheinbergon is attempting to deploy a commit to the Airbyte Growth Team on Vercel.

A member of the Team first needs to authorize it.

@octavia-squidington-iii octavia-squidington-iii added area/connectors Connector related issues area/documentation Improvements or additions to documentation labels Mar 10, 2025
@sheinbergon sheinbergon temporarily deployed to community-ci-auto March 10, 2025 16:48 — with GitHub Actions Inactive
@sheinbergon sheinbergon temporarily deployed to community-ci-auto March 10, 2025 17:55 — with GitHub Actions Inactive
@sheinbergon sheinbergon force-pushed the sheinbergon/source-tiktok-marketing-new-streams branch from 44ab08f to ea19e8b Compare March 10, 2025 18:02
@sheinbergon sheinbergon temporarily deployed to community-ci-auto March 10, 2025 18:02 — with GitHub Actions Inactive
@sheinbergon sheinbergon force-pushed the sheinbergon/source-tiktok-marketing-new-streams branch from ea19e8b to adb6f53 Compare March 10, 2025 20:08
@sheinbergon sheinbergon temporarily deployed to community-ci-auto March 10, 2025 20:08 — with GitHub Actions Inactive
@sheinbergon
Copy link
Contributor Author

@natikgadzhi @marcosmarxm time and contribute this to the product, so we can you it in airbyte cloud. Can you please have look and confirm it's all good?

@sheinbergon
Copy link
Contributor Author

@natikgadzhi @marcosmarxm this addition is crucial to us. Can you please have a look?

@marcosmarxm
Copy link
Member

@sheinbergon I ask your patience. Our team has a backlog of community contribution.

@sheinbergon sheinbergon force-pushed the sheinbergon/source-tiktok-marketing-new-streams branch from 8444253 to 0cb055b Compare March 12, 2025 14:14
Copy link

vercel bot commented Mar 12, 2025

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

Name Status Preview Comments Updated (UTC)
airbyte-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 12, 2025 7:00pm

Copy link
Contributor

@natikgadzhi natikgadzhi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This overall looks good. I am not entirely certain about adding country_code to base_report.

I have a couple of options in mind:

  • Either we get @tolik0's or @agarctfi approval and merge as is
  • Or we do progressive rollout

@@ -60,6 +60,14 @@ acceptance_tests:
bypass_reason: "No data in the integration test account. We should seed the sandbox later on."
- name: pixel_events_statistics
bypass_reason: "No data in the integration test account. We should seed the sandbox later on."
- name: ads_reports_by_country_daily
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh my sweet lordy I hate CATs so much

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there's something for me to do with this comment 😅? If not, can I resolve it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

ads_reports_by_country_daily_stream:
type: DeclarativeStream
name: ads_reports_by_country_daily
$parameters:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like the use of $parameters, but it's the best approach for this PR as the other streams are using them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. So this is not an issue, right? can this be resolved?

@@ -3558,6 +3826,11 @@ definitions:
type:
- "null"
- integer
country_code:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sheinbergon @tolik0 is base_report used in any other streams?Is the country_code always there? Is it safe to add here, or should we add this at the level of daily reports instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@natikgadzhi it can be used for other streams. Currently not present of course. Adding this, I saw no other streams break. Again, the reason for adding this is to allow access to by ad_id/date/country breakdown for BASIC tiktok reports. It's very standartized access in their API's documentation. If there's another way for doing that, please let me know

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@natikgadzhi @tolik0 a kind reminder about this concern

@sheinbergon
Copy link
Contributor Author

This overall looks good. I am not entirely certain about adding country_code to base_report.

I have a couple of options in mind:

  • Either we get @tolik0's or @agarctfi approval and merge as is
  • Or we do progressive rollout

Thank you for reviewing and suggesting fixes. What do you mean by doing a "progressive rollout"?

@sheinbergon sheinbergon force-pushed the sheinbergon/source-tiktok-marketing-new-streams branch from f743704 to 240b7a7 Compare March 12, 2025 20:17
@sheinbergon sheinbergon temporarily deployed to community-ci-auto March 12, 2025 20:18 — with GitHub Actions Inactive
@sheinbergon sheinbergon force-pushed the sheinbergon/source-tiktok-marketing-new-streams branch from 240b7a7 to 5b4c792 Compare March 13, 2025 08:59
@sheinbergon sheinbergon temporarily deployed to community-ci-auto March 13, 2025 09:00 — with GitHub Actions Inactive
@sheinbergon sheinbergon force-pushed the sheinbergon/source-tiktok-marketing-new-streams branch from 5b4c792 to 069419b Compare March 16, 2025 01:42
@sheinbergon sheinbergon temporarily deployed to community-ci-auto March 16, 2025 01:43 — with GitHub Actions Inactive
@sheinbergon sheinbergon force-pushed the sheinbergon/source-tiktok-marketing-new-streams branch from 069419b to a698b45 Compare March 17, 2025 15:21
@sheinbergon sheinbergon force-pushed the sheinbergon/source-tiktok-marketing-new-streams branch from a698b45 to 321b40a Compare March 20, 2025 09:52
@sheinbergon sheinbergon temporarily deployed to community-ci-auto March 20, 2025 09:52 — with GitHub Actions Inactive
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 community connectors/source/tiktok-marketing
Projects
Development

Successfully merging this pull request may close these issues.

4 participants