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

Update/ad-reporting-v2 #15

Merged
merged 19 commits into from
Sep 2, 2022
Merged

Update/ad-reporting-v2 #15

merged 19 commits into from
Sep 2, 2022

Conversation

fivetran-sheringuyen
Copy link
Contributor

@fivetran-sheringuyen fivetran-sheringuyen commented Jul 5, 2022

Pull Request
Are you a current Fivetran customer?
Fivetran Employee

What change(s) does this PR introduce?

🚨 Breaking Changes 🚨

  • microsoft_ads__ad_adapter report has been renamed to microsoft_ads__url_report to more accurately reflect contents of report.

🎉 Feature Enhancements 🎉

  • Models have been updated to use level specific performance reporting for more accurate reporting.
  • New models have been added:
    • microsoft_ads__ad_report
    • microsoft_ads__keyword_report
    • microsoft_ads__search_report
  • New fields have been added to old models.
  • README updates for easier navigation and use of the package.
  • Passthrough metric functionality for more flexible reporting.
  • Added testing for better data integrity.

Did you update the CHANGELOG?

  • Yes

Does this PR introduce a breaking change?

  • Yes (please provide breaking change details below.)
  • No (please provide an explanation as to how the change is non-breaking below.)

Did you update the dbt_project.yml files with the version upgrade (please leverage standard semantic versioning)? (In both your main project and integration_tests)

  • Yes

Is this PR in response to a previously created Bug or Feature Request

  • Yes, Issue/Feature [link bug/feature number here]
  • No

How did you test the PR changes?

  • CircleCi
  • Local (please provide additional testing details below)

Select which warehouse(s) were used to test the PR

  • BigQuery
  • Redshift
  • Snowflake
  • Postgres
  • Databricks
  • Other (provide details below)

Provide an emoji that best describes your current mood

😌

Feedback

We are so excited you decided to contribute to the Fivetran community dbt package! We continue to work to improve the packages and would greatly appreciate your feedback on our existing dbt packages or what you'd like to see next.

Copy link
Contributor

@fivetran-joemarkiewicz fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

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

@fivetran-sheringuyen thanks so much for putting this PR together! Your updates look mostly good, I just had a few comments inline that should be addressed before we are good to go.

  • A few minor README and CHANGELOG updates.
  • A question about including the match_type field to the keyword report
  • Question around filtering the url report.
  • Addressing any other inline comments I have from this review.

I also did take a closer look at the url report and the logic you applied makes sense to me! I really appreciated your addition to the DECISIONLOG about the auto tagging nature within Microsoft Ads.

@@ -1,3 +1,16 @@
# dbt_microsoft_ads v0.5.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add the PR link to the CHANGELOG

models/microsoft_ads__keyword_report.sql Show resolved Hide resolved
packages.yml Outdated
Comment on lines 2 to 6
# - package: fivetran/microsoft_ads_source
# version: [">=0.4.0", "<0.5.0"]
- git: https://github.com/fivetran/dbt_microsoft_ads_source.git
revision: update/ad-reporting-v2
warn-unpinned: false
Copy link
Contributor

Choose a reason for hiding this comment

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

Reminder to switch before merge.

README.md Outdated
# 📣 What does this dbt package do?
- Produces modeled tables that leverage Microsoft Ads data from [Fivetran's connector](https://fivetran.com/docs/applications/microsoft-advertising) in the format described by [this ERD](https://fivetran.com/docs/applications/microsoft-advertising#schemainformation) and builds off the output of our [Microsoft Ads source package](https://github.com/fivetran/dbt_microsoft_ads_source).
- Enables you to better understand the performance of your ads across varying grains:
- Providing an account, campaign, ad group, keyword, ad, and utm level reports.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you feel it is relevant to also include the search term report in this list?

README.md Outdated
| [microsoft_ads__ad_group_report](https://fivetran.github.io/dbt_microsoft_ads/#!/model/model.microsoft_ads.microsoft_ads__ad_group_report) | Each record in this table represents the daily performance at the ad group level. |
| [microsoft_ads__ad_report](https://fivetran.github.io/dbt_microsoft_ads/#!/model/model.microsoft_ads.microsoft_ads__ad_report) | Each record in this table represents the daily performance at the ad level. |
| [microsoft_ads__keyword_report](https://fivetran.github.io/dbt_microsoft_ads/#!/model/model.microsoft_ads.microsoft_ads__keyword_report) | Each record in this table represents the daily performance at the ad group level for keywords. |
| [microsoft_ads__search_report](https://fivetran.github.io/dbt_microsoft_ads/#!/model/model.microsoft_ads.microsoft_ads__ad_report) | Each record in this table represents the daily performance at the search level. |
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this link is pointing to the ad report instead of the search report.

models/microsoft_ads__url_report.sql Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
@fivetran-sheringuyen
Copy link
Contributor Author

Hey @fivetran-joemarkiewicz, just updated the PR with your suggested fixes. Let me know if you have any other questions!

Copy link
Contributor

@fivetran-joemarkiewicz fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

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

Thanks for making the updates @fivetran-sheringuyen! These look good to go on my end 😄

@fivetran-sheringuyen fivetran-sheringuyen merged commit 30bb5db into main Sep 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants