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

Add conversions to Linkedin Ads Transform package #36

Merged
merged 13 commits into from
Jul 30, 2024

Conversation

fivetran-avinash
Copy link
Contributor

PR Overview

This PR will address the following Issue/Feature: Adding conversion support

This PR will result in the following new package version:

Although no breaking changes are introduced, the PR is significantly robust enough to require a version bump.

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

Feature Updates: Conversion Support!

We have added more robust support for conversions in our data models by doing the following:

  • Adds a conversion_value_by_local_currency field to each _report end model, representing the value of conversions that occurred on each day for each campaign/campaign_group/creative/url/account.
  • Created a linkedin_ads__conversion_fields variable to pass through and transform additional conversion value metrics into their aggregate sums.
    • Set current variable defaults in the dbt_project.yml to bring in the most used conversion fields external_website_conversions and one_click_leads by default.
    • Instructions on how to set your own fields are available in the README.
  • Adds a total_conversions metric in our end models to track all conversions being brought in by the linkedin_ads_conversion_fields variable.

Documentation Update

  • Documents how to set your own passthrough fields with the variable linkedin_ads__conversion_fields in the README.
  • Created a DECISIONLOG to describe best practices in configuring the linkedin_ads__conversion_fields variable.

Under the Hood

  • Added a new version of the persist_pass_through_columns() macro in which we can include coalesces and properly check between conversion field values and the existing passthrough column.
  • Updated the PR templates to align with our most up-to-date standards.
  • Included auto-releaser GitHub Actions workflow to automate future releases.
  • Addition of integrity and consistency validation tests within integration_tests for the Linkedin transformation models.
  • Updated linkedin_ad_analytics_by_creative_data seed file with relevant conversion fields for more robust testing.

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) && dbt test

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

  • [NA] 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:

Validation tests passing! Mostly mirroring Jamie's validations in Facebook.

Screenshot 2024-06-25 at 11 25 25 AM

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

🥼

@fivetran-avinash fivetran-avinash self-assigned this Jun 25, 2024
Copy link
Contributor

@fivetran-jamie fivetran-jamie left a comment

Choose a reason for hiding this comment

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

looks GREAT! just doc comments really

DECISIONLOG.md Outdated Show resolved Hide resolved
DECISIONLOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
models/linkedin_ads__campaign_group_report.sql Outdated Show resolved Hide resolved
models/linkedin_ads__campaign_report.sql Outdated Show resolved Hide resolved
models/linkedin_ads__creative_report.sql Outdated Show resolved Hide resolved
models/linkedin_ads__url_report.sql Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Comment on lines 12 to 14
{% for field in additional_exclude_fields %}
{% do except_fields.append(field) %}
{% endfor %}
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like we can remove this part? is additional_exclude_fields supposed to be used somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fivetran-jamie Made a slight modification to the code, let me know if this makes more sense.

@fivetran-avinash fivetran-avinash added type:enhancement New functionality or enhancement status:in_review Currently in review update_type:feature Primary focus is to add new functionality labels Jul 22, 2024
Copy link
Contributor Author

@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 Ready for re-review!

Comment on lines 12 to 14
{% for field in additional_exclude_fields %}
{% do except_fields.append(field) %}
{% endfor %}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fivetran-jamie Made a slight modification to the code, let me know if this makes more sense.

models/linkedin.yml Show resolved Hide resolved
Copy link
Contributor

@fivetran-jamie fivetran-jamie left a comment

Choose a reason for hiding this comment

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

🥳 Looks great to me! I'll take on merging this in this/next sprint

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 and @fivetran-avinash great work on these PRs! I only have the one callout to update the README section for the new conversion variable, but all other stress tests I performed look good! Once that final update is applied this should be good to go.

@@ -1,3 +1,3 @@
packages:
- package: fivetran/linkedin_source
version: [">=0.8.0", "<0.9.0"]
version: [">=0.9.0", "<0.10.0"]
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 hold off releasing this package until the source package is live on the dbt hub.

README.md Show resolved Hide resolved
@fivetran-jamie fivetran-jamie merged commit 51d8b74 into main Jul 30, 2024
2 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:in_review Currently in review type:enhancement New functionality or enhancement update_type:feature Primary focus is to add new functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants