Feature/add conversions #43

fivetran-jamie · 2024-05-21T23:05:29Z

PR Overview

This PR will address the following Issue/Feature:
no issue made for conversions + #42

This PR will result in the following new package version:

v0.8.0

Please provide the finalized CHANGELOG entry which details the relevant changes included in this PR:

Feature Updates

Introducing...conversion metrics (PR #43)!

Adds a conversion_value field to each _report end model, representing the value of conversions (calculated using the default attribution window set in Meta) that occurred on each day for each ad/campaign/ad set/url/account.
Creates a facebook_ads__basic_ad_actions_passthrough_metrics variable to pass through additional conversion value metrics that are calculated using different attribution windows.
- By default, the package includes only the conversion value calculated using the default attribution window, but your report may include calculations using the other windows defined here. See README for details on how to use the new var.

Documentation

Documents the ability to transform metrics provided to the facebook_ads__basic_ad_passthrough_metrics variable. See README for details (PR #43).

Under the Hood

Updated the quickstart.yml file to allow for automated Quickstart data model deployments (PR #40).
Updated the PR templates to align with our most up-to-date standards (PR #43).

PR Checklist

Basic Validation

Please acknowledge that you have successfully performed the following commands locally:

dbt run –full-refresh && dbt test
dbt run (if incremental models are present)

Before marking this PR as "ready for review" the following have been applied:

The appropriate issue has been linked, tagged, and properly assigned
All necessary documentation and version upgrades have been applied
docs were regenerated (unless this PR does not include any code or yml updates)
BuildKite integration tests are passing
Detailed validation steps have been provided below

Detailed Validation

Please share any and all of your validation steps:

Tests are passing

Horizontal integrity test compares total sum of conversion_value from each _report end model
Consistency tests compare clicks, impressions, and spend across dev and prod in each of the _report models

No exceptions needed, just a teeny tiny buffer to account for differences in rounding

fivetran-joemarkiewicz

@fivetran-jamie these changes look great! I just have a few minor suggestions, but none related to the actual code. I see this being a huge value add for our users!

Once you address the comments in this review let's hold off merging until the other updates in the Google and LinkedIn packages are ready as well. Thanks and great job!

integration_tests/dbt_project.yml

fivetran-joemarkiewicz · 2024-05-23T15:20:16Z

packages.yml

+# - package: fivetran/facebook_ads_source
+#   version: [">=0.8.0", "<0.9.0"]
+# - local: ../Facebook_ads/dbt_facebook_ads_source
+- git: https://github.com/fivetran/dbt_facebook_ads_source.git
+  revision: feature/add-conversions
+  warn-unpinned: false


Reminder to swap before merge

fivetran-joemarkiewicz · 2024-05-23T15:21:32Z

models/facebook_ads__account_report.sql

        {{ fivetran_utils.persist_pass_through_columns(pass_through_variable='facebook_ads__basic_ad_passthrough_metrics', transform = 'sum') }}
+        , sum(coalesce(conversion_report.conversion_value, 0)) as conversion_value


Just to confirm, should 0 and null be represented the same here?

yeah, though i have actually yet to see a null (or even 0) conversion value recorded in basic_ad_actions

the individual/passthrough values are null when representing 0 however

Thanks for confirming. Good to see we haven't seen null in the conversion value, but great to see we are being proactive with this logic. This looks good to me!

models/facebook_ads__account_report.sql

CHANGELOG.md

README.md

CHANGELOG.md

fivetran-catfritz · 2024-05-23T18:53:54Z

@fivetran-jamie @fivetran-joemarkiewicz Now might be a good time to delete the defunct 2nd reviewer bot.

fivetran-joemarkiewicz · 2024-05-23T19:00:39Z

@fivetran-jamie @fivetran-joemarkiewicz Now might be a good time to delete the defunct 2nd reviewer bot.

Agreed! @fivetran-jamie would you be able to remove that from this branch.

Co-authored-by: Joe Markiewicz <74217849+fivetran-joemarkiewicz@users.noreply.github.com>

maxtuzz · 2024-05-28T02:44:58Z

Great effort! Looking forward to this release.

fivetran-joemarkiewicz

@fivetran-jamie great work on this PR and you're nearly there to releasing full support for conversions in our ad reporting packages!!

I have a few comments and suggestions in line. Additionally, I noticed I ran into a similar error from before where I run into an issue when trying to passthrough a default conversion value from this PR into the facebook_ads__basic_ad_passthrough_metrics variable. Would you be able to look into this and confirm if this is intentional or if there are some adjustments we need to make to this logic within the models.

Thanks!

docs/run_results.json

integration_tests/tests/consistency/consistency_account_report.sql

fivetran-joemarkiewicz · 2024-11-06T21:44:57Z

packages.yml

+# - package: fivetran/facebook_ads_source
+#   version: [">=0.8.0", "<0.9.0"]
+# - local: ../Facebook_ads/dbt_facebook_ads_source
+# - local: ../../dbt_facebook_ads_source
+- git: https://github.com/fivetran/dbt_facebook_ads_source.git
+  revision: feature/add-conversions
+  warn-unpinned: false


Reminder to clean and swap before merge

fivetran-jamie · 2024-11-06T23:05:57Z

I noticed I ran into a similar error from before where I run into an issue when trying to passthrough a default conversion value from this PR into the facebook_ads__basic_ad_passthrough_metrics variable. Would you be able to look into this and confirm if this is intentional or if there are some adjustments we need to make to this logic within the models.

@fivetran-joemarkiewicz Yeah this is the expected behavior, as conversions are explicitly not included in basic_ad according to our docs. However, I suppose there is nothing stopping someone from adding conversions as a passthrough field, even if null (especially if they were using passthrough columns to add conversions to the ad_reporting models).

To avoid duplicate column errors, we can employ the same _c suffix method as ad_reporting, or we can just choose to select our version of conversions since any previous field was likely null

fivetran-joemarkiewicz · 2024-11-06T23:19:16Z

conversions are explicitly not included in basic_ad according to our docs.

Thanks for calling this out @fivetran-jamie! I was going through and stress testing so I see how I tested something that realistically should never occur. I would recommend if the conversion fields should not exist in the BASIC_AD table, then I feel comfortable giving our default value the preference and not including the _c field name as it will most likely be null.

Thanks for clarifying this!

fivetran-joemarkiewicz

LGTM!

fivetran-reneeli

Just had some questions!

CHANGELOG.md

fivetran-reneeli · 2024-11-12T22:29:29Z

README.md

@@ -89,19 +93,62 @@ vars:
 To connect your multiple schema/database sources to the package models, follow the steps outlined in the [Union Data Defined Sources Configuration](https://github.com/fivetran/dbt_fivetran_utils/tree/releases/v0.4.latest#union_data-source) section of the Fivetran Utils documentation for the union_data macro. This will ensure a proper configuration and correct visualization of connections in the DAG.

 #### Passing Through Additional Metrics
-By default, this package will select `clicks`, `impressions`, and `cost` from the source reporting tables to store into the staging models. If you would like to pass through additional metrics to the staging models, add the below configurations to your `dbt_project.yml` file. These variables allow for the pass-through fields to be aliased (`alias`) if desired, but not required. Use the below format for declaring the respective pass-through variables:
+By default, this package will select `clicks`, `impressions`, `cost`, and conversion `value` (using the default attribution window) from the source reporting tables (`BASIC_AD`, `BASIC_AD_ACTIONS`, and `BASIC_AD_ACTION_VALUES`) to store into the output models. If you would like to pass through additional metrics to the output models, add the below configurations to your `dbt_project.yml` file. These variables allow for the pass-through fields to be aliased (`alias`) and transformed (`transform_sql`) if desired, but not required. Only the `name` of each metric field is required. Use the below format for declaring the respective pass-through variables:


also stating what the default is here?

README.md

fivetran-reneeli · 2024-11-12T22:35:25Z

models/facebook_ads__campaign_report.sql

    from report 
+    left join conversion_report
+        on report.date_day = conversion_report.date_day


do we need to join by source_relation here as well?

jeez I can't believe I forgot these, good catch

fivetran-reneeli · 2024-11-12T22:35:42Z

models/facebook_ads__url_report.sql

    from report
+    left join conversion_report
+        on report.date_day = conversion_report.date_day


source_relation join?

Co-authored-by: Renee Li <91097070+fivetran-reneeli@users.noreply.github.com>

fivetran-reneeli

LGTM

fivetran-jamie added 9 commits May 21, 2024 09:30

working locally

b95c3d9

document

bbea210

docs

67d9c0b

cleanup

b7fe4d1

coalesces

95f3ec6

docs

d864c93

tests passing

cedacf8

revert local stuff

2ded6de

buildkite

bb28474

fivetran-jamie mentioned this pull request May 21, 2024

Feature/add conversions fivetran/dbt_facebook_ads_source#36

Merged

7 tasks

bk

ff24d9e

fivetran-jamie self-assigned this May 21, 2024

fivetran-jamie requested a review from fivetran-joemarkiewicz May 21, 2024 23:32

fivetran-jamie added 2 commits May 22, 2024 10:57

empty var work

a7a82f3

whitespace

c01bda4

fivetran-joemarkiewicz approved these changes May 23, 2024

View reviewed changes

fivetran-data-model-bot requested review from a team, fivetran-reneeli and fivetran-catfritz and removed request for a team May 23, 2024 18:44

fivetran-jamie and others added 5 commits May 23, 2024 13:21

Update integration_tests/dbt_project.yml

fffe659

Co-authored-by: Joe Markiewicz <74217849+fivetran-joemarkiewicz@users.noreply.github.com>

Update CHANGELOG.md

64006dc

Co-authored-by: Joe Markiewicz <74217849+fivetran-joemarkiewicz@users.noreply.github.com>

use macro

33c4fc4

more docs and remove second reviewer bot

7ccbf4a

docs

c00cc87

fivetran-avinash mentioned this pull request Jun 25, 2024

Add conversions to Linkedin Ads Transform package fivetran/dbt_linkedin#36

Merged

6 tasks

update readme model docs

593b180

fivetran-jamie added 5 commits November 5, 2024 12:27

fix

42ffa65

make seed file uniquw

a9b6c1d

document stuff

5850ac6

remove commented out code and polish things

e5b74f3

polishes

3ede410

fivetran-joemarkiewicz requested changes Nov 6, 2024

View reviewed changes

fivetran-jamie added status:in_progress Currently being worked on and removed status:blocked Need additional information or requirements before proceeding labels Nov 6, 2024

joe feedback

78e3f65

fivetran-jamie added 2 commits November 7, 2024 12:20

backwards compatible

23aaa7e

docs

96c732d

fivetran-joemarkiewicz approved these changes Nov 7, 2024

View reviewed changes

regen docs and adjust seeds to produce output

11d9074

fivetran-reneeli reviewed Nov 12, 2024

View reviewed changes

fivetran-jamie and others added 3 commits November 12, 2024 15:52

Update README.md

6402d52

Co-authored-by: Renee Li <91097070+fivetran-reneeli@users.noreply.github.com>

renee feedback

b26afc7

bk mistake

a5f7109

fivetran-reneeli approved these changes Nov 13, 2024

View reviewed changes

fivetran-data-model-bot requested review from a team and fivetran-avinash and removed request for a team November 13, 2024 15:30

fivetran-jamie added 6 commits November 13, 2024 09:35

bk

1a15cff

bk working

b5b9dc2

bk

d499390

bk

7b3e171

docs

ed63e94

source package ref

a790467

fivetran-jamie merged commit 6c67bba into main Nov 14, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/add conversions #43

Feature/add conversions #43

fivetran-jamie commented May 21, 2024 •

edited

Loading

fivetran-joemarkiewicz left a comment

fivetran-joemarkiewicz May 23, 2024

fivetran-joemarkiewicz May 23, 2024

fivetran-jamie May 23, 2024

fivetran-joemarkiewicz May 24, 2024

fivetran-catfritz commented May 23, 2024

fivetran-joemarkiewicz commented May 23, 2024

maxtuzz commented May 28, 2024

fivetran-joemarkiewicz left a comment

fivetran-joemarkiewicz Nov 6, 2024

fivetran-jamie commented Nov 6, 2024

fivetran-joemarkiewicz commented Nov 6, 2024

fivetran-joemarkiewicz left a comment

fivetran-reneeli left a comment

fivetran-reneeli Nov 12, 2024

fivetran-reneeli Nov 12, 2024

fivetran-jamie Nov 12, 2024

fivetran-reneeli Nov 12, 2024

fivetran-jamie Nov 12, 2024

fivetran-reneeli left a comment

		{{ fivetran_utils.persist_pass_through_columns(pass_through_variable='facebook_ads__basic_ad_passthrough_metrics', transform = 'sum') }}
		, sum(coalesce(conversion_report.conversion_value, 0)) as conversion_value

Feature/add conversions #43

Feature/add conversions #43

Conversation

fivetran-jamie commented May 21, 2024 • edited Loading

PR Overview

Feature Updates

Documentation

Under the Hood

PR Checklist

Basic Validation

Detailed Validation

fivetran-joemarkiewicz left a 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-catfritz commented May 23, 2024

fivetran-joemarkiewicz commented May 23, 2024

maxtuzz commented May 28, 2024

fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fivetran-jamie commented Nov 6, 2024

fivetran-joemarkiewicz commented Nov 6, 2024

fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

fivetran-reneeli left a 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-reneeli left a comment

Choose a reason for hiding this comment

fivetran-jamie commented May 21, 2024 •

edited

Loading