-
Notifications
You must be signed in to change notification settings - Fork 6
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
Add conversions #34
Add conversions #34
Conversation
packages.yml
Outdated
revision: feature/add-conversions | ||
warn-unpinned: false | ||
# - package: fivetran/microsoft_ads_source |
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.
switch
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.
LGTM with one small suggestion in the tests to ensure we don't forget about conversions in future releases.
account_id, | ||
sum(clicks) as clicks, | ||
sum(impressions) as impressions, | ||
sum(spend) as spend |
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.
Not a requirement before release, but should we consider including the conversion fields in here as well (commented out) so we don't forget to include them in subsequent releases?
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.
Same comment for other consistency tests
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.
added
PR Overview
This PR will address the following Issue/Feature: Add conversions project.
This PR will result in the following new package version: v0.9.0
Breaking change for users who were not already bringing in conversion fields via passthrough columns. This is designed to be backwards compatible for those cases.
Please provide the finalized CHANGELOG entry which details the relevant changes included in this PR:
dbt_microsoft_ads v0.9.0
PR #34 includes the following updates:
Feature Updates: Conversion Support!
microsoft_ads
end model:conversions
: Number of conversions, measured by completion of an action by a customer after viewing your ad.conversions_value
: The revenue reported by the advertiser as a result of theconversions
figure.Under the Hood
microsoft_ads_persist_pass_through_columns
macro to ensure that the new conversion fields are backwards compatible with users who have already included them via passthrough fields.integration_tests
folder for the transformation models (to be used by maintainers only).Documentation Update
microsoft_ads.yml
with new fields mentioned above.Contributors
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:
Using internal data we validated that
(1) data was passing through as expected
(2) customers leveraging passthrough columns were now only grabbing the necessary columns
(3) leveraged devprod on our internal data set to ensure validation tests were matching conversion values on all consistency and integrity tests
If you had to summarize this PR in an emoji, which would it be?
🖱️