fivetran / dbt_ad_reporting

Fivetran's ad reporting dbt package. Combine your Facebook, Google, Pinterest, LinkedIn, Twitter, Snapchat, Microsoft, TikTok, Reddit, Amazon, and Apple Search advertising metrics using this package.
https://fivetran.github.io/dbt_ad_reporting/#!/overview
Apache License 2.0
142 stars 56 forks source link

[Bug] Unable to define database variables using Unity Catalog for Google Ads Source #87

Closed nicolaswon closed 1 year ago

nicolaswon commented 1 year ago

Is there an existing issue for this?

Describe the issue

My company is using Databricks' Unity Catalog as metastore. The Fivetran connectors are currently writing to the 'ingestion' catalog, and processed in the 'dev' or 'prod' catalog. Thus, we are required to change the database variables as described in the package documentation.

However, there seems to be a bug with the google_ads_source package that do not correctly change the default database variable from the target database to a custom ('ingestion' in our case). Changing the src_google_ads.yml in the google_ads_source package seems to solve the problem:

Before: database: "{% if target.type not in ['spark', 'databricks'] %}{{ var('google_ads_database', target.database) }}{% endif %}"

After `database: ingestion

Alternatively, changing it to the format used for the other x_ads_source packages also seems to be working fine database: "{% if target.type != 'spark' %}{{ var('google_ads_database', target.database) }}{% endif %}"

Relevant error log or model output

No response

Expected behavior

The google_ads_source package seems to have logic for reading the database variable that differs from the other source packages.

dbt Project configurations

vars:
  facebook_ads_schema: facebook_ads
  facebook_ads_database: ingestion

  google_ads_schema: google_ads
  google_ads_database: ingestion

  linkedin_ads_schema: linkedin_ads
  linkedin_ads_database: ingestion

  apple_search_ads_schema: apple_search_ads
  apple_search_ads_database: ingestion

  ad_reporting__amazon_ads_enabled: False # by default this is assumed to be True
  ad_reporting__pinterest_ads_enabled: False # by default this is assumed to be True
  ad_reporting__microsoft_ads_enabled: False # by default this is assumed to be True
  ad_reporting__twitter_ads_enabled: False # by default this is assumed to be True
  ad_reporting__snapchat_ads_enabled: False # by default this is assumed to be True
  ad_reporting__tiktok_ads_enabled: False # by default this is assumed to be True
  ad_reporting__reddit_ads_enabled: False # by default this is assumed to be True

models:
  ad_reporting:
    +enabled: true
    +schema: mart

  facebook_ads:
    +enabled: true
    +schema: intermediate
  facebook_ads_source:
    +enabled: true
    +schema: staging

  google_ads_source:
    +enabled: true
    +schema: staging # leave blank for just the target_schema
  google_ads:
    +enabled: true
    +schema: intermediate # leave blank for just the target_schema

  linkedin:
    +enabled: true
    +schema: intermediate
  linkedin_source:
    +enabled: true
    +schema: staging

  apple_search_ads:
    +enabled: true
    +schema: intermediate
  apple_search_ads_source:
    +enabled: true
    +schema: staging

Package versions

1.3.1

What database are you using dbt with?

databricks

dbt Version

1.4.6

Additional Context

Screenshot 2023-04-29 at 19 20 14

Are you willing to open a PR to help address this issue?

fivetran-joemarkiewicz commented 1 year ago

Hi @nicolaswon thanks for raising this issue. I have actually seen this pop up in a few other packages as well and believe removing the databricks portion from source.yml in google_ads_source should do the trick. We unfortunately do still need to keep the conditional as the spark adapter requires there be no database linked; however, with the unity catalog available in the databricks adapter, I believe we should be able to remove this.

The one thing I want to check before proceeding with this update is ensuring that a databricks adapter user not using the Unity catalog would not see an error if we removed databricks from the conditional.

I can check on the above mentioned validation, but if you wanted to contribute your finding to the package I would encourage you to open a PR and my team and I can review and fold it into the next release of google_ads_source if all looks to be fine regarding the non unity catalog users.

fivetran-catfritz commented 1 year ago

@nicolaswon Following up on this issue, I made updates to the src_google_ads.yml as @joe-fivetran mentioned above. The changes I made are working on my end, but it would be useful if you could also test it out. I created a test branch that you can install by using the following code snippet in place of the usual installation for ad_reporting. Let me know how it works for you!

packages:
  - git: https://github.com/fivetran/dbt_ad_reporting.git
    revision: test/google_ads_databricks_db_config
    warn-unpinned: false
nicolaswon commented 1 year ago

Hi @fivetran-catfritz! Apologies for the delay in response - been out traveling the last few days. I can confirm that the above change fixes the issue. Let me know if I can be of any help to resolve this. Thanks!

fivetran-catfritz commented 1 year ago

Thank you, @nicolaswon, for testing it! We plan to update the Google Ads package within the next week, and I will notify you here once the update is complete. Afterward, the ad_reporting package will automatically incorporate the changes the next time you run "dbt deps".

fivetran-catfritz commented 1 year ago

@nicolaswon The google_ads_source package has been released. Thanks again for your help identifying this issue!