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
146 stars 55 forks source link

[Bug] Turning off Apple ads package still runs source freshness #73

Closed nszoni closed 1 year ago

nszoni commented 1 year ago

Is there an existing issue for this?

Describe the issue

Turning off the usage of Apple ads platform as stated in the package README doesn't actually turn off source freshness tests for the models, although the config is conditioned on the variable set to False

sources:
  - name: apple_search_ads
    schema: "{{ var('apple_search_ads_schema', 'apple_search_ads') }}"
    database: "{% if target.type != 'spark'%}{{ var('apple_search_ads_database', target.database) }}{% endif %}"

    loader: Fivetran
    loaded_at_field: _fivetran_synced

    freshness: 
      warn_after: {count: 48, period: hour}
      error_after: {count: 168, period: hour}

    config:
      enabled: "{{ var('ad_reporting__apple_search_ads_enabled', true) }}"

Relevant error log or model output

15:27:40  1 of 36 START freshness of apple_search_ads.search_term_report ................. [RUN]
15:27:41  1 of 36 ERROR freshness of apple_search_ads.search_term_report ................. [ERROR in 0.68s]

15:27:50  Runtime Error in source search_term_report (models/src_apple_search_ads.yml)
15:27:50    404 Not found: Dataset xxxxx:apple_search_ads was not found in location US
15:27:50    
15:27:50    (job ID: b656a397-d0c4-4f36-b651-898edfaf03cb)
15:27:50  
15:27:50  
15:27:50  Done.

Expected behavior

I would expect dbt not running source freshness tests for Apple because we are not creating those.

dbt Project configurations

vars:
  ad_reporting__apple_search_ads_enabled: False
  ad_reporting__pinterest_ads_enabled: false
  ad_reporting__linkedin_ads_enabled: false
  ad_reporting__twitter_ads_enabled: false
  ad_reporting__snapchat_ads_enabled: false
  ad_reporting__tiktok_ads_enabled: false

  apple_search_ads__using_search_terms: False

Package versions

packages:
  - package: calogica/dbt_date
    version: [">=0.5.0", "<0.6.0"]
  - package: dbt-labs/dbt_utils
    version: [">=0.8.0", "<0.9.0"]
  - package: dbt-labs/codegen
    version: [">=0.7.0", "<0.8.0"]
  - package: fivetran/shopify
    version: [">=0.6.0", "<0.7.0"]
  - package: fivetran/google_ads
    version: [">=0.8.0", "<0.9.0"]
  - package: fivetran/google_ads_source
    version: [">=0.8.0", "<0.9.0"]
  - package: fivetran/microsoft_ads
    version: [">=0.5.0", "<0.6.0"]
  - package: fivetran/facebook_ads
    version: [">=0.5.0", "<0.6.0"]
  - package: fivetran/ad_reporting
    version: [">=1.0.0", "<1.1.0"]
  - package: calogica/dbt_expectations
    version: [">=0.5.0", "<0.6.0"]
# - package: fivetran/klaviyo
#   version: [">=0.4.0", "<0.5.0"]
  - package: dbt-labs/segment
    version: [">=0.8.0", "<0.9.0"]
  - package: fivetran/instagram_business
    version: [">=0.1.0", "<0.2.0"]
  - package: fivetran/fivetran_utils
    version: [">=0.3.0", "<0.4.0"]
  - package: dbt-labs/metrics
    version: [">=0.3.0", "<0.4.0"]

What database are you using dbt with?

bigquery

dbt Version

❯ dbt --version
Core:
  - installed: 1.2.0
  - latest:    1.3.1 - Update available!

  Your version of dbt-core is out of date!
  You can find instructions for upgrading here:
  https://docs.getdbt.com/docs/installation

Plugins:
  - bigquery: 1.2.0 - Update available!

Additional Context

No response

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

fivetran-joemarkiewicz commented 1 year ago

Hi @nszoni thanks so much for opening this Issue and I am sorry you are experiencing this bug.

I would believe the inclusion of the ad_reporting__apple_search_ads_enabled: False would properly disable the entire apple ad source file and not run the source freshness.

Before delving too deep I am curious about one item in particular. Would you be able to try and lowercase the false in your variable declarations for the apple ads variables. I have experienced dbt getting confused with the mixed casing in the past and want to rule that out initially.

nszoni commented 1 year ago

hey @fivetran-joemarkiewicz thanks for taking this up! I've tried keeping all of them lowercase, but it still wants to check the apple ad source.

75820

fivetran-joemarkiewicz commented 1 year ago

Hi @nszoni thanks for checking my suspicious and showing that there is something more going on here. I have been able to replicate your variables in my local environment and notice the same error you are seeing.

I have been able to narrow the issue down to this line within the Apple Search Ads Source package.

Our intention is that this enablement would be disabled if either (or both) the apple search variables are set to false. However, after checking the under the hood rendering, it looks like it simply returns as False and False which does not in fact disable the source freshness. I am currently tinkering to see what new config would work to achieve the expected outcome we are looking for. I will be sure the share an update once I find the fix!

fivetran-joemarkiewicz commented 1 year ago

On the topic of the False and False rendering, I just stumbled upon this Issue from dbt-core that mentions this exact issue and how there is no validation for situations like this.

It seems dbt Labs released a fix in dbt-core v1.3.1 that raises a more appropriate warning. I imagine if we both upgrade to the latest v1.3.1 then we will se the appropriate error. Nevertheless, I will continue working to uncover a fix for this issue.

nszoni commented 1 year ago

@fivetran-joemarkiewicz nice catch! I will also keep investigating if i find something else:) Thanks!

fivetran-joemarkiewicz commented 1 year ago

Looks like my team and I were able to find the root cause! When we adjusted the line noted above to be the below snippet it worked as intended!

        config:
          enabled: "{{ var('ad_reporting__apple_search_ads_enabled', true) and var('apple_search_ads__using_search_terms', true) }}"

This should result in a quick update to the apple_search_ads_source package to use the above line in place of what we currently have.

I noticed you are open to creating a PR to fix this issue. Would you be interested in opening a PR on our Apple Search Ads Source package to address this? Otherwise we can fold this update into our coming sprint.

nszoni commented 1 year ago

Awesome! Sure thing! Will do it tomorrow:) Thanks a ton!

nszoni commented 1 year ago

@jonatfivetran here you go!:) 👆

fivetran-joemarkiewicz commented 1 year ago

Thanks again for the contributor @nszoni 🏅

Now that the apple_search_ads_source PR has been merged and released, this issue should be resolved. Closing out this issue. Please feel free to reopen if the issue persists.