Update/ad-reporting-v2 #20

fivetran-sheringuyen · 2022-07-05T19:43:58Z

Pull Request
Are you a current Fivetran customer?
Fivetran employee

What change(s) does this PR introduce?
Breaking Changes
PR [] makes the below updates that may affect your workflow:

modified_timestamp columns have been renamed to modified_at and is_most_recent_version has been renamed to is_most_recent_record to reflect more recent package coding standards for the below models:
- stg_microsoft_ads__account_history
- stg_microsoft_ads__ad_group_history
- stg_microsoft_ads__ad_history
- stg_microsoft_ads__ad_performance_daily_report
- stg_microsoft_ads__campaign_history
Deprecating *_version_id fields in *_history models.

Feature Enhancements
We have added the below feature enhancements to this package in (PR #)[]:

Add get_*_columns macros for previously included models and newly added models.
Updated staging model standards on old models to conform with recent package development standards. Updates were made to the below models:
- stg_microsoft_ads__account_history
- stg_microsoft_ads__ad_group_history
- stg_microsoft_ads__ad_history
- stg_microsoft_ads__ad_performance_daily_report
- stg_microsoft_ads__campaign_history
New history and daily performance staging models including:
- stg_microsoft_ads__account_daily_report
- stg_microsoft_ads__campaign_daily_report
- stg_microsoft_ads__ad_group_daily_report
- stg_microsoft_ads__search_daily_report
- stg_microsoft_ads__keyword_daily_report
- stg_microsoft_ads__keyword_history
README updates for easier navigation and use of the package.
Addition of identifier variables for each of the source tables to allow for further flexibility in source table direction within the dbt project.
More robust testing for better data integrity including:
- Freshness tests
- Model grain tests
Passthrough metric functionality for more flexibility in reporting.
Additional documentation for new models added.

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

N/A

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

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.

fivetran-joemarkiewicz

@fivetran-sheringuyen thanks so much for this PR and putting this package through a much needed overhaul! I have a few comments you can find below and inline. Let me know if you want to chat more about any of them!

I really like the depth you put into the staging model tests. I do wonder if this would break if individuals add more metrics via the passthrough columns? Is this something we should consider?
It looks like the is_most_recent_record field was not included for all of the history staging model yml documentation. Would you be able to add that in.
I have a pretty open ended question/suggestion on what fields to include in the staging column macros and how to pass through the variable metrics. See my deeper comments in the review, happy to chat more about the approach we should take.
Address any other inline comments.

fivetran-joemarkiewicz · 2022-07-12T14:33:45Z

CHANGELOG.md

+# dbt_microsoft_ads_source v0.6.0
+
+## 🚨 Breaking Changes 🚨
+PR [] makes the below updates that may affect your workflow:


Reminder to add the PR link.

fivetran-joemarkiewicz · 2022-07-12T14:33:57Z

CHANGELOG.md

+- Deprecating `*_version_id` fields in `*_history` models.
+
+## 🎉 Feature Enhancements 🎉
+We have added the below feature enhancements to this package in (PR #)[]:


Similar reminder to add the PR link.

fivetran-joemarkiewicz · 2022-07-12T14:51:10Z

macros/get_account_daily_report_columns.sql

+    {"name": "bid_match_type", "datatype": dbt_utils.type_string()},
+    {"name": "clicks", "datatype": dbt_utils.type_int()},
+    {"name": "conversion_rate", "datatype": dbt_utils.type_float()},
+    {"name": "conversions", "datatype": dbt_utils.type_int()},


The more I think about it, the more I am in the camp of removing fields from the macro that are not included in the final CTE of the staging model. The reason being, if a customer has a field that they want to passthrough as a variable, but we don't have that field in this macro, then it would fail.

I think it would be better if we just add the passthrough jinja in both CTE pieces instead. This way there would be no possibilities of the model failing if we do not have the field in our macro. Thoughts?

So we would have the passthrough logic as follows:

This same comment will apply to all the macros.

I've been thinking about this for a while as well. I actually have some questions here:

The only cases I could imagine where a customer would include a field that isn't already in the macro are:

(A) we've added new columns to the table in the connector, customer adds as passthrough column but we have not updated the package macros to reflect that change

(B) customer has custom fields that we don't see on our end. So outside of those two cases, are there others where the fields added as passthroughs aren't already included in the macro?

In the case that a customer adds a passthrough column..that doesn't actually have any values (off chance), would we want to leverage the macro as a safety?

models/stg_microsoft_ads__account_daily_report.sql

fivetran-joemarkiewicz · 2022-07-12T14:56:28Z

models/stg_microsoft_ads__account_daily_report.sql

+
+    select * 
+    from {{ ref('stg_microsoft_ads__account_daily_report_tmp') }}
+


Super small style note/question. Should we remove this extra enter. The other style guide suggests there is no whitespace following the from and before the ),.

fivetran-joemarkiewicz · 2022-07-12T14:58:37Z

models/tmp/stg_microsoft_ads__account_daily_report_tmp.sql

@@ -0,0 +1 @@
+select * from {{ var('account_performance_daily_report') }}


Super small style note/question - should we have this query as two lines instead of one? I notice this is pretty inconsistent across packages. My preference would be two lines.

fivetran-reneeli · 2022-07-14T19:33:39Z

README.md

-# dbt_project.yml
+vars:
+  microsoft_ads__account_report_passthrough_metrics: ['the', 'list', 'of', 'metric', 'columns', 'to', 'include'] # from microsoft_ads.account_performance_daily_report
+  microsoft_ads__campaign_report_passthrough_columns: ['the', 'list', 'of', 'metric', 'columns', 'to', 'include'] # from microsoft_ads.campaign_performance_daily_report


Just was passing by, noticed that these columns should be renamed to metrics!

good catch, thanks!

fivetran-sheringuyen · 2022-07-20T20:24:29Z

Hey @fivetran-joemarkiewicz, re:

I really like the depth you put into the staging model tests. I do wonder if this would break if individuals add more metrics via the passthrough columns? Is this something we should consider?

I think in my head I've only been thinking of customers using that variable to passthrough metrics - in which case, this shouldn't be a problem at all. If they are passing through non-metrics, this would be a concern we should consider -- however, it would also break some of the sum( {{ metric }} ) logic that we have incorporated. But that would open another can of worms one that I think can be avoided since we do name the variables *_metrics so there should be the assumption that passthrough columns should only be metrics.

Additionally, before adding changes for the macro + CTE suggestions, I'd like to discuss that as a team first and walk through the nuances.

fivetran-joemarkiewicz · 2022-07-21T15:29:04Z

Hey @fivetran-joemarkiewicz, re:

I really like the depth you put into the staging model tests. I do wonder if this would break if individuals add more metrics via the passthrough columns? Is this something we should consider?

I think in my head I've only been thinking of customers using that variable to passthrough metrics - in which case, this shouldn't be a problem at all. If they are passing through non-metrics, this would be a concern we should consider -- however, it would also break some of the sum( {{ metric }} ) logic that we have incorporated. But that would open another can of worms one that I think can be avoided since we do name the variables *_metrics so there should be the assumption that passthrough columns should only be metrics.

Additionally, before adding changes for the macro + CTE suggestions, I'd like to discuss that as a team first and walk through the nuances.

@fivetran-sheringuyen this makes sense to me! Maybe we can add a small clarifying statement in the README to only include metrics in this passthrough variable. This way there is nothing left up to interpretation (although it does seem very obvious to me haha).

I also agree that we should discuss the passthrough + macro behavior closer as a team. We can do so during standup this morning!

fivetran-sheringuyen · 2022-07-21T22:23:41Z

Hey @fivetran-joemarkiewicz! Just updated the repo for PR changes. Let me know if you have any follow up questions!

fivetran-joemarkiewicz

@fivetran-sheringuyen thanks so much for applying the review notes to this PR! After re-reviewing, this looks good to go on my end. Great job with this overhaul 🏋️

…dbt_microsoft_ads_source into update/ad-reporting-v2

fivetran-sheringuyen added 12 commits July 5, 2022 12:41

models

1b99fa8

passthrough columns

a26087e

passthrough columns

8872574

ymls and docs.md

af1b305

dbt tests

7ce31fc

integration tests and docs

a8dd49e

integration test datatype

2907467

integration tests

ad3c9b6

integration tests

636455d

moving spark to the end

f9dc79f

docs

bafa6cd

docs

844e830

fivetran-joemarkiewicz self-requested a review July 12, 2022 14:31

fivetran-joemarkiewicz assigned fivetran-sheringuyen Jul 12, 2022

fivetran-joemarkiewicz added documentation labels Jul 12, 2022

fivetran-joemarkiewicz requested changes Jul 12, 2022

View reviewed changes

fivetran-reneeli reviewed Jul 14, 2022

View reviewed changes

fivetran-sheringuyen added 2 commits July 21, 2022 15:07

updated passthrough logic

f7cb5e2

passthrough readme

cf10e37

fivetran-joemarkiewicz self-requested a review July 22, 2022 14:15

fivetran-joemarkiewicz approved these changes Jul 22, 2022

View reviewed changes

fivetran-joemarkiewicz and others added 5 commits August 9, 2022 17:18

Update packages.yml

6c8b780

configs, vars, macros

334fa36

get columns

37ed071

before merge additional changes

2927ce5

seed data

b84342f

fivetran-sheringuyen added 6 commits August 30, 2022 22:21

Merge branch 'update/ad-reporting-v2' of https://github.com/fivetran/…

de1a79d

…dbt_microsoft_ads_source into update/ad-reporting-v2

seed data

f0a09fd

seed data

5744d27

seed data

58a4c8a

seed data

5fbf3c3

readme

3e0c7b5

fivetran-sheringuyen merged commit 3d56e17 into main Sep 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update/ad-reporting-v2 #20

Update/ad-reporting-v2 #20

fivetran-sheringuyen commented Jul 5, 2022 •

edited

Loading

fivetran-joemarkiewicz left a comment •

edited by fivetran-sheringuyen

Loading

fivetran-joemarkiewicz Jul 12, 2022

fivetran-joemarkiewicz Jul 12, 2022

fivetran-joemarkiewicz Jul 12, 2022

fivetran-joemarkiewicz Jul 12, 2022

fivetran-sheringuyen Jul 20, 2022

fivetran-joemarkiewicz Jul 12, 2022

fivetran-joemarkiewicz Jul 12, 2022

fivetran-reneeli Jul 14, 2022

fivetran-sheringuyen Jul 20, 2022

fivetran-sheringuyen commented Jul 20, 2022

fivetran-joemarkiewicz commented Jul 21, 2022

fivetran-sheringuyen commented Jul 21, 2022

fivetran-joemarkiewicz left a comment


		select *
		from {{ ref('stg_microsoft_ads__account_daily_report_tmp') }}

		@@ -0,0 +1 @@
		select * from {{ var('account_performance_daily_report') }}

Update/ad-reporting-v2 #20

Update/ad-reporting-v2 #20

Conversation

fivetran-sheringuyen commented Jul 5, 2022 • edited Loading

fivetran-joemarkiewicz left a comment • edited by fivetran-sheringuyen 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

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

Choose a reason for hiding this comment

fivetran-sheringuyen commented Jul 20, 2022

fivetran-joemarkiewicz commented Jul 21, 2022

fivetran-sheringuyen commented Jul 21, 2022

fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

fivetran-sheringuyen commented Jul 5, 2022 •

edited

Loading

fivetran-joemarkiewicz left a comment •

edited by fivetran-sheringuyen

Loading