Skip to content
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

Open
wants to merge 33 commits into
base: main
Choose a base branch
from
Open

Conversation

fivetran-jamie
Copy link
Contributor

@fivetran-jamie fivetran-jamie commented May 21, 2024

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)!

  • 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
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

Copy link
Contributor

@fivetran-joemarkiewicz fivetran-joemarkiewicz left a 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!

integration_tests/dbt_project.yml Show resolved Hide resolved
packages.yml Outdated
Comment on lines 2 to 7
# - 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
Copy link
Contributor

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
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
image

Copy link
Contributor

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!

models/facebook_ads__account_report.sql Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
@fivetran-data-model-bot 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-catfritz
Copy link
Contributor

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

@fivetran-joemarkiewicz
Copy link
Contributor

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

fivetran-jamie and others added 5 commits May 23, 2024 13:21
Co-authored-by: Joe Markiewicz <74217849+fivetran-joemarkiewicz@users.noreply.github.com>
Co-authored-by: Joe Markiewicz <74217849+fivetran-joemarkiewicz@users.noreply.github.com>
@maxtuzz
Copy link

maxtuzz commented May 28, 2024

Great effort! Looking forward to this release.

@fivetran-jamie fivetran-jamie added the status:blocked Need additional information or requirements before proceeding label Sep 11, 2024
@fivetran-jamie
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:blocked Need additional information or requirements before proceeding
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants