Skip to content

Enable package to use Snowflake Organisation Account views #1

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

Open
wants to merge 32 commits into
base: main
Choose a base branch
from

Conversation

bisset-a
Copy link
Owner

@bisset-a bisset-a commented Feb 26, 2025

This PR adds support of views in the new Snowflake organisation account, while still being compatible with single account Snowflake views.

To use the dbt_snowflake_monitoring package in multi-account mode, Project variable uses_org_view should be set to true in the downstream project.

Notes

  • When using org account views, query acceleration is not baked into cost per query. This is because the metering_history view is not currently available in the organisation account. Snowflake have confirmed it will be available in Q2 (May - July) so right now we are making a pragmatic decision to exclude QA from the query cost calculation until this view is enabled.
  • models hourly_spend and daily_spend do not include serverless task spend. This is because severless_task_history does not exist in the organisation account. Snowflake confirmed this will be enabled in Q2 aswell.

@bisset-a bisset-a changed the title update models Enable package to use org level views Mar 20, 2025
@bisset-a bisset-a changed the title Enable package to use org level views Enable package to use Snowflake Organisation Account views Mar 20, 2025
{{ config(materialized='table') }}
{{ config(
materialized='table',
enabled=not(var('uses_org_view', false))

Choose a reason for hiding this comment

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

Let's add a comment just to show that we can do this once metering_history is available at org level?

Copy link
Owner Author

Choose a reason for hiding this comment

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

done!

Choose a reason for hiding this comment

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

Oh just saw there's a separate org level model, how comes?

Copy link
Owner Author

Choose a reason for hiding this comment

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

I originally did this because there was too much conditional logic - but you're right I think we should just completely disable this and daily_spend until Snowflake makes the metering_history view available

warehouse_snapshots.organization_name,
warehouse_snapshots.account_name,
warehouse_snapshots.account_locator,
{% endif %}

Choose a reason for hiding this comment

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

Would you be able to simplify this significantly if you set this to always include these new columns? I'm not sure the conditional rendering is actually necessary. In the single account version, they'd just be for one account, but it still works

Copy link

@NiallRees NiallRees left a comment

Choose a reason for hiding this comment

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

This is really cool. My main suggestion would be to avoid all of the conditional rendering around the new columns, and instead always render them, but just have them represent a single account in the case that uses_org_view is false.

@@ -0,0 +1,7 @@
{% macro generate_scoped_unique_key(base_fields) %}

Choose a reason for hiding this comment

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

I just realised that if we're now adding the account_name column everywhere, we can always add the account_name to the unique key config! :)

Copy link
Owner Author

Choose a reason for hiding this comment

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

I just realised this too!! 😆 my only concern was for backwards compatibility for incremental models, account_name would be null for historical data but I think in this case it might be ok to have nulls in the unique_key since the other fields will be unique anyway. other option would be to create a new column called _unique_key that uses generate_surrogate_keys to handle the nulls and use that as the unique_key. wdyt?

Choose a reason for hiding this comment

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

I think a composite PK works totally fine without making a new column - honestly I could go either way.

Copy link
Owner Author

Choose a reason for hiding this comment

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

perfect - lets leave it as it! thanks Niall!

Choose a reason for hiding this comment

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

👌

@NiallRees
Copy link

Sweet. Want to open this PR against our main repo?

@bisset-a
Copy link
Owner Author

Sweet. Want to open this PR against our main repo?

PR created: get-select#178

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants