fivetran / dbt_facebook_ads

Fivetran data models for Facebook Ads built using dbt.
https://fivetran.github.io/dbt_facebook_ads/
Apache License 2.0
27 stars 27 forks source link

Feature/add conversions #43

Closed fivetran-jamie closed 1 week ago

fivetran-jamie commented 6 months ago

PR Overview

This PR will address the following Issue/Feature: no issue made for conversions + https://github.com/fivetran/dbt_facebook_ads/issues/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)!

Documentation

Under the Hood

PR Checklist

Basic Validation

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

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

Detailed Validation

Please share any and all of your validation steps:

Tests are passing image

  1. Horizontal integrity test compares total sum of conversion_value from each _report end model
  2. 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 image

fivetran-catfritz commented 6 months ago

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

fivetran-joemarkiewicz commented 6 months ago

@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.

maxtuzz commented 5 months ago

Great effort! Looking forward to this release.

fivetran-jamie commented 2 months ago

FYI for anyone subscribed here, this is currently blocked.

We are working with Engineering to ensure that the conversion monetary values (action_values) are included in a default report and therefore accessible to the package (currently, only the raw # of conversions are supported by the basic_ad_actions report).

fivetran-jamie commented 2 weeks ago

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 commented 2 weeks ago

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!