Ad reporting v2 updates #21
Conversation
There was a problem hiding this comment.
@fivetran-jamie thanks for making this updates. The PR looks mostly good, I just have a few suggestions below. Additionally, there are a number of suggestions I have that are dependent on some source package updates. We should settle on those before tackling these updates. Let me know if you have any questions.
- I noticed the partial_parse.mgpack and graph.gpickle files are included. Can we remove them if they are not needed.
- Address any suggestions in the comments below
models/linkedin.yml
Outdated
| - date_day | ||
| - campaign_id |
There was a problem hiding this comment.
Would it be appropriate to also test on account_id and campaign_group_id as well?
| - date_day | ||
| - campaign_group_id |
There was a problem hiding this comment.
Should we also test on account_id?
| - date_day | ||
| - creative_id |
There was a problem hiding this comment.
Should we also test on campaign_group_id, campaign_id, and account_id?
| - date_day | ||
| - creative_id |
There was a problem hiding this comment.
Should we also test on campaign_group_id, campaign_id, and account_id?
|
|
||
| select * | ||
| from {{ var('account_history') }} | ||
| where valid_to is null -- get latest |
There was a problem hiding this comment.
Once we update the source package to leverage the is_most_recent logic instead, we will want to update this to reflect that change.
models/linkedin_ads__url_report.sql
Outdated
|
|
||
| select * | ||
| from {{ var('campaign_group_history') }} | ||
| where valid_to is null -- get latest |
There was a problem hiding this comment.
Same as other is_most_recent comments.
models/linkedin_ads__url_report.sql
Outdated
|
|
||
| select * | ||
| from {{ var('account_history') }} | ||
| where valid_to is null |
There was a problem hiding this comment.
Same as other is_most_recent comments.
README.md
Outdated
| | [linkedin_ads__account_report](https://github.com/fivetran/dbt_linkedin/blob/main/models/linkedin_ads__account_report.sql) | Each record represents the daily ad performance of each account. | | ||
| | [linkedin_ads__campaign_report](https://github.com/fivetran/dbt_linkedin/blob/main/models/linkedin_ads__campaign_report.sql) | Each record represents the daily ad performance of each campaign. Linkedin campaigns map onto ad groups in other ad platforms. | | ||
| | [linkedin_ads__campaign_group_report](https://github.com/fivetran/dbt_linkedin/blob/main/models/linkedin_ads__campaign_group_report.sql) | Each record represents the daily ad performance of each campaign group. Linkedin | | ||
| | [linkedin_ads__creative_report](https://github.com/fivetran/dbt_linkedin/blob/main/models/linkedin_ads__creative_report.sql) | Each record represents the daily ad performance of each creative. | | ||
| | [linkedin_ads__url_report](https://github.com/fivetran/dbt_linkedin/blob/main/models/linkedin_ads__url_report.sql) | Each record represents the daily ad performance of each url. | |
There was a problem hiding this comment.
Can we update these links to use the dbt docs links instead? It may be easiest to wait to update these until we have merged to main and the docs are officially hosted.
There was a problem hiding this comment.
will just wanna double check that the links are good after merging and enabling github pages
README.md
Outdated
| linkedin_ads__passthrough_metrics: | ||
| - name: "new_custom_field" | ||
| alias: "custom_field" | ||
| - name: "unique_int_field" | ||
| alias: "field_id" | ||
| transform_sql: "cast(field_id as int)" | ||
| - name: "that_field" |
There was a problem hiding this comment.
Based on the outcome of the question posed in the source package, we may or may not need to update this.
| sum(report.impressions) as impressions, | ||
| sum(report.cost) as cost | ||
|
|
||
| {{ fivetran_utils.persist_pass_through_columns('linkedin_ads__passthrough_metrics', transform='sum') }} |
There was a problem hiding this comment.
Same comment from the source regarding our decision to possibly split this out into two variables.
There was a problem hiding this comment.
The change suggested here would need to be applied to the other models if we decide to update.
There was a problem hiding this comment.
split the variables
|
@fivetran-jamie can you also rename this PR. Just so it doesn't seem deceiving upon first glance. |
Pull Request
Are you a current Fivetran customer?
fivetran made PR
What change(s) does this PR introduce?
Did you update the CHANGELOG?
Does this PR introduce a breaking change?
Did you update the dbt_project.yml files with the version upgrade (please leverage standard semantic versioning)? (In both your main project and integration_tests)
Is this PR in response to a previously created Bug or Feature Request
How did you test the PR changes?
Select which warehouse(s) were used to test the PR
Provide an emoji that best describes your current mood
🎸
Feedback
We are so excited you decided to contribute to the Fivetran community dbt package! We continue to work to improve the packages and would greatly appreciate your feedback on our existing dbt packages or what you'd like to see next.