fivetran / dbt_google_ads

Fivetran data transformations for Google Ads built using dbt.
https://fivetran.github.io/dbt_google_ads/
Apache License 2.0
13 stars 29 forks source link

Updated utms and add campaign channel fields #42

Closed jocelyn-metricbox closed 1 year ago

jocelyn-metricbox commented 1 year ago

Are you a current Fivetran customer? Jocelyn Chan, Data Analyst, Paper Culture

What change(s) does this PR introduce?

The changes have dependencies on the [dbt_google_ads_source] package detailed in PR #33

Did you update the CHANGELOG?

Does this PR introduce a breaking change?

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)

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

How did you test the PR changes?

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

Provide an emoji that best describes your current mood

:dancer: **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](https://www.surveymonkey.com/r/DQ7K7WW) on our existing dbt packages or what you'd like to see next.
fivetran-joemarkiewicz commented 1 year ago

Hi @jocelyn-metricbox thank you so much for opening these PRs to add the features to the Google Ads packages. At first glance, I think these additions look relevant to include. One thing I did want to highlight is that these utm fields will most likely not be able to be brought through to our final ad reporting roll up package. Is that a concern of yours that these updates would most likely be isolated to this package?

If not, then we can move forward with this review. Additionally, I saw you scheduled some time to discuss the PR during our office hours! I look forward to chatting more with you then ๐Ÿ˜„

jocelyn-metricbox commented 1 year ago

Hi @jocelyn-metricbox thank you so much for opening these PRs to add the features to the Google Ads packages. At first glance, I think these additions look relevant to include. One thing I did want to highlight is that these utm fields will most likely not be able to be brought through to our final ad reporting roll up package. Is that a concern of yours that these updates would most likely be isolated to this package?

If not, then we can move forward with this review. Additionally, I saw you scheduled some time to discuss the PR during our office hours! I look forward to chatting more with you then ๐Ÿ˜„

@fivetran-joemarkiewicz To your question, we are fine with the UTM fields only in the url_reporting model as is originally in the final package. So no concern there. Thank you!

jocelyn-metricbox commented 1 year ago

Hi Joe,

How are you? I met with your team the same week as your previous email a month ago. After I clarified a few things, it doesn't seem to be any concerns from the team. I wonder if there is any update about the PRs?

https://github.com/fivetran/dbt_google_ads_source/pull/33 https://github.com/fivetran/dbt_google_ads/pull/42

We hope they can be approved and merged soon. Looking forward to hearing from you! Thank you!

Best Regards, Jocelyn Chan

On Mon, Feb 13, 2023 at 10:56โ€ฏAM Joe Markiewicz @.***> wrote:

Hi @jocelyn-metricbox https://github.com/jocelyn-metricbox thank you so much for opening these PRs to add the features to the Google Ads packages. At first glance, I think these additions look relevant to include. One thing I did want to highlight is that these utm fields will most likely not be able to be brought through to our final ad reporting roll up package. Is that a concern of yours that these updates would most likely be isolated to this package?

If not, then we can move forward with this review. Additionally, I saw you scheduled some time to discuss the PR during our office hours! I look forward to chatting more with you then ๐Ÿ˜„

โ€” Reply to this email directly, view it on GitHub https://github.com/fivetran/dbt_google_ads/pull/42#issuecomment-1428186142, or unsubscribe https://github.com/notifications/unsubscribe-auth/AVFD6LGTSWTBYD5BNETOG6LWXJKRTANCNFSM6AAAAAAURQM7DE . You are receiving this because you were mentioned.Message ID: @.***>

fivetran-joemarkiewicz commented 1 year ago

HI @jocelyn-metricbox my apologies for not responding to your previous email from a month ago. If there is no intention of these fields flowing downstream to the ad reporting package, then I don't have high concerns with incorporating this into the package.

I will add this review to our next sprint. You can expect to hear from us next week and may have some follow up or small edits to apply to your PRs before merging into the main branch.

jocelyn-metricbox commented 1 year ago

Hi Joe,

Sounds good. Thank you!

Best, Jocelyn Chan

On Thu, Mar 16, 2023 at 4:34 PM Joe Markiewicz @.***> wrote:

HI @jocelyn-metricbox https://github.com/jocelyn-metricbox my apologies for not responding to your previous email from a month ago. If there is no intention of these fields flowing downstream to the ad reporting package, then I don't have high concerns with incorporating this into the package.

I will add this review to our next sprint. You can expect to hear from us next week and may have some follow up or small edits to apply to your PRs before merging into the main branch.

โ€” Reply to this email directly, view it on GitHub https://github.com/fivetran/dbt_google_ads/pull/42#issuecomment-1472703091, or unsubscribe https://github.com/notifications/unsubscribe-auth/AVFD6LFIGSNKOOAXBMJLVXLW4N2NXANCNFSM6AAAAAAURQM7DE . You are receiving this because you were mentioned.Message ID: @.***>

fivetran-joemarkiewicz commented 1 year ago

Hi @jocelyn-metricbox just wanted to mention here that I will be able to finish this review once the comments within the dbt_google_ads_source PR are addressed.

Let me know if you have any questions on my comments within the above PR ๐Ÿ˜ƒ

fivetran-joemarkiewicz commented 1 year ago

@jocelyn-metricbox thanks for working on these updates. There are a few changes I would like to make before integrating these updates into the next release. As such, I will merge them into a staging branch where more changes will be made before release. Be sure to follow the soon to accompany PR to be in the loop on when these changes will be live.