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.

Sign up for GitHub

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

Jump to bottom

reach + frequency metrics #35

Merged
merged 9 commits into from
May 6, 2024
Merged

reach + frequency metrics #35

merged 9 commits into from
May 6, 2024

Conversation

fivetran-jamie
Copy link
Contributor

@fivetran-jamie fivetran-jamie commented May 1, 2024 •
edited
Loading

PR Overview

This PR will address the following Issue/Feature:
#34

This PR will result in the following new package version:

v0.7.3

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

Bug Fixes

  • Resolved the "duplicate column" error that would arise when the facebook_ads__basic_ad_passthrough_metrics variable included reach or frequency. Reach and frequency are included by default in the stg_facebook_ads__basic_ad model but will only persist to downstream transform models if specified in the facebook_ads__basic_ad_passthrough_metrics variable.

Under the Hood

  • Updated the maintainer PR template to our most up-to-date standards.

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:

So in my dev dbt_project.yml, I added the following config:

vars:
  facebook_ads__basic_ad_passthrough_metrics: 
    - name: 'reach'
    - name: 'frequency'
      alias: 'fake_freq_name'

I expected the above inclusion of reach (but not frequency actually due to the alias) to cause a failure, which it did in stg_facebook_ads__basic_ad
image

With this branch, running dbt run +ad_reporting completed successfully
image

And I see reach and fake_freq_name in my facebook ads models (and with non-zero/null values)
image
image

If you had to summarize this PR in an emoji, which would it be?

📊

@fivetran-jamie fivetran-jamie self-assigned this May 2, 2024
@fivetran-jamie fivetran-jamie marked this pull request as ready for review May 2, 2024 17:34
fivetran-joemarkiewicz
fivetran-joemarkiewicz requested changes May 2, 2024
View reviewed changes
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 thanks for quickly working through this update and for putting such a strong emphasis on backwards compatibility! I do have one small comment to try and make the code a bit more dynamic in the staging model. Let me know your thoughts.

integration_tests/dbt_project.yml Show resolved Hide resolved
models/stg_facebook_ads__basic_ad.sql Outdated Show resolved Hide resolved
@fivetran-jamie @fivetran-joemarkiewicz
Update models/stg_facebook_ads__basic_ad.sql
d65f982 
Co-authored-by: Joe Markiewicz <74217849+fivetran-joemarkiewicz@users.noreply.github.com>
fivetran-joemarkiewicz
fivetran-joemarkiewicz approved these changes May 3, 2024
View reviewed changes
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 thanks for making these changes! This looks good to me! This is good to have docs regen'd and then kick off the release review!

@fivetran-data-model-bot fivetran-data-model-bot requested review from a team, fivetran-reneeli and fivetran-avinash and removed request for a team May 3, 2024 14:10
fivetran-avinash
fivetran-avinash reviewed May 3, 2024
View reviewed changes
Copy link
Contributor

@fivetran-avinash fivetran-avinash 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 A few comments before approval!

CHANGELOG.md Outdated
@@ -23,10 +32,10 @@

# dbt_facebook_ads_source v0.UPDATE.UPDATE
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this Under the Hood section be incorporated into a release (and this v.0.UPDATE.UPDATE line be removed)?

CHANGELOG.md
@@ -23,10 +32,10 @@

# dbt_facebook_ads_source v0.UPDATE.UPDATE

## Under the Hood:

## Under the Hood:
- Incorporated the new `fivetran_utils.drop_schemas_automation` macro into the end of each Buildkite integration test job.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know which release these changes came from? We should probably move these bullets to that release section.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated!

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated
[PR #35](https://github.com/fivetran/dbt_facebook_ads_source/pull/35) includes the following updates:

## Bug Fixes
- Resolved the "duplicate column" error that would arise when the `facebook_ads__basic_ad_passthrough_metrics` variable included `reach` or `frequency`. `Reach` and `frequency` are included by default in the `stg_facebook_ads__basic_ad` model but will only persist to downstream transform models if specified in the `facebook_ads__basic_ad_passthrough_metrics` variable.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we call out how we resolved it in a sub-bullet using a for loop in the specific models/macros? Doesn't have to be too involved, just always nice to explain how to the customer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added!

fivetran-avinash
fivetran-avinash approved these changes May 3, 2024
View reviewed changes
Copy link
Contributor

@fivetran-avinash fivetran-avinash left a comment

Choose a reason for hiding this comment

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

lgtm

@fivetran-data-model-bot fivetran-data-model-bot requested review from a team and fivetran-catfritz and removed request for a team May 3, 2024 21:10
@fivetran-jamie fivetran-jamie merged commit e8d43da into main May 6, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fivetran-avinash fivetran-avinash fivetran-avinash approved these changes

@fivetran-joemarkiewicz fivetran-joemarkiewicz fivetran-joemarkiewicz approved these changes

@fivetran-reneeli fivetran-reneeli Awaiting requested review from fivetran-reneeli fivetran-reneeli was automatically assigned from fivetran/dbt

@fivetran-catfritz fivetran-catfritz Awaiting requested review from fivetran-catfritz fivetran-catfritz was automatically assigned from fivetran/dbt

Assignees

@fivetran-jamie fivetran-jamie

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fivetran-jamie @fivetran-joemarkiewicz @fivetran-avinash