-
Notifications
You must be signed in to change notification settings - Fork 27
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
Feature/add conversions #43
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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!
packages.yml
Outdated
# - 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reminder to swap before merge
{{ 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to confirm, should 0 and null
be represented the same here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
@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>
Co-authored-by: Joe Markiewicz <74217849+fivetran-joemarkiewicz@users.noreply.github.com>
Great effort! Looking forward to this release. |
FYI for anyone subscribed here, this is currently blocked. We are working with Engineering to ensure that the conversion monetary values ( |
PR Overview
This PR will address the following Issue/Feature:
no issue made
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)!
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.facebook_ads__basic_ad_actions_passthrough_metrics
variable to pass through additional conversion value metrics that are calculated using different attribution windows.Documentation
facebook_ads__basic_ad_passthrough_metrics
variable. See README for details (PR #43).Under the Hood
quickstart.yml
file to allow for automated Quickstart data model deployments (PR #40).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
conversion_value
from each_report
end modelclicks
,impressions
, andspend
across dev and prod in each of the_report
modelsNo exceptions needed, just a teeny tiny buffer to account for differences in rounding