Proposed docs changes

[dwreeves]

Hi, really like this tool!

I see the docs are not tied to the repo however, so I'd like to propose changes to the docs but cannot directly do so.

If there is a way to directly edit docs and then open a PR somewhere, let me know. I think that would be preferable as I am willing to do so.

1. ACCOUNTADMIN not required.

The current set-up is over-privileged. ACCOUNTADMIN is not required to run Snowkill.

Each instance of GRANT ROLE ACCOUNTADMIN TO ROLE [X] should be replaced with:

GRANT IMPORTED PRIVILEGES ON DATABASE SNOWFLAKE TO ROLE [X];
GRANT MANAGE WAREHOUSES ON ACCOUNT TO ROLE [X];

The accountadmin role is not required, basically. All references to requiring ACCOUNTADMIN should be relaxed.

The main documentation grants ACCOUNTADMIN to ADMIN_MONITOR. This should be replaced with a SNOWKILL_ROLE or something similar with the above grants + ownership of the table+schema with Snowkill resources.

This can also be asserted in the tests by changing account_setup.sql.

2. Unistore / hybrid table example should be provided.

Hybrid tables are not GA but have become available in many regions since the creation of this framework, and setting up a Snowflake table is much easier than spinning up an entire database in many situations. It would vastly improve the adoption of this tool to have an example of the proper hybrid table setup, and also encourage users to use it if it's available to them:

CREATE HYBRID TABLE xxx.xxx.snowkill_log
(
    query_id VARCHAR(16777216),
    check_result_level NUMBER(38,0),
    check_result_name VARCHAR(16777216),
    check_result_description VARCHAR(16777216),
    check_result_time TIMESTAMP_NTZ(6),
    CONSTRAINT pkey_snowkill_log PRIMARY KEY (query_id, check_result_level)
)

3. Airflow example.

I have an Airflow example of use of that I'd like to add to the docs. This is, loosely speaking, how I am running Snowkill right now.

Again, like the previous section, this is aimed at improving adoption. It's a much lower ask to just copy+paste into an existing cron system than having users set up their own way of running the script.

This is adapted from the example script, just with some more Airflow appropriate things.

from contextlib import closing
from datetime import datetime
from functools import cached_property
from typing import TYPE_CHECKING

from airflow import DAG
from airflow.models.baseoperator import BaseOperator

if TYPE_CHECKING:
    from airflow.providers.slack.hooks.slack_webhook import SlackWebhookHook
    from airflow.providers.snowflake.hooks.snowflake import SnowflakeHook
    from airflow.utils.context import Context


default_args = {
    "snowflake_conn_id": "snowflake_default",
    "slack_webhook_conn_id": "slack_webhook_default"
}


class RunSnowkillOperator(BaseOperator):

    def __init__(
            self,
            *,
            snowkill_target_table: str,
            snowsight_base_url: str,
            slack_webhook_conn_id: str | None = None,
            snowflake_conn_id: str | None = None,
            **kwargs,
    ) -> None:
        super().__init__(**kwargs)
        self.slack_webhook_conn_id = slack_webhook_conn_id
        self.snowflake_conn_id = snowflake_conn_id
        self.snowkill_target_table = snowkill_target_table
        self.snowsight_base_url = snowsight_base_url

    @cached_property
    def slack_hook(self) -> "SlackWebhookHook":
        from airflow.providers.slack.hooks.slack_webhook import SlackWebhookHook
        return SlackWebhookHook(slack_webhook_conn_id=self.slack_webhook_conn_id)

    @cached_property
    def snowflake_hook(self) -> "SnowflakeHook":
        from airflow.providers.snowflake.hooks.snowflake import SnowflakeHook
        return SnowflakeHook(
            snowflake_conn_id=self.snowflake_conn_id,
            # database="UTILS",
            # schema="MONITOR",
            # role="ADMIN_MONITOR_ROLE",
            # warehouse="ADMIN_MONITOR_WH"
        )

    def execute(self, context: "Context") -> None:
        import snowkill as sk

        with closing(self.snowflake_hook.get_conn()) as conn:
            snowkill_engine = sk.SnowKillEngine(conn)
            snowkill_storage = sk.SnowflakeTableStorage(conn, self.snowkill_target_table)
            snowkill_formatter = sk.SlackFormatter(self.snowsight_base_url)

            checks = [
                sk.ExecuteDurationCondition(
                    warning_duration=60 * 30,  # 30 minutes for warning
                    kill_duration=60 * 60,  # 60 minutes for kill
                ),
                sk.CartesianJoinExplosionCondition(
                    min_output_rows=1_000_000,  # join emits at least 1M output rows
                    min_explosion_rate=5,  # ratio of output rows to input rows is at least 5x
                    warning_duration=60 * 5,  # 5 minutes for warning
                    kill_duration=60 * 10,  # 10 minutes for kill
                ),
                sk.JoinExplosionCondition(
                    min_output_rows=10_000_000,  # join emits at least 10M output rows
                    min_explosion_rate=10,  # ratio of output rows to input rows is at least 10x
                    warning_duration=60 * 10,  # 10 minutes for warning
                    kill_duration=60 * 20,  # 20 minutes for kill
                ),
                sk.UnionWithoutAllCondition(
                    min_input_rows=10_000_000,  # at least 10M input rows for UNION without ALL
                    notice_duration=60 * 10,  # 10 minutes for notice
                ),
                sk.StorageSpillingCondition(
                    min_local_spilling_gb=50,  # 50Gb spill to local storage
                    min_remote_spilling_gb=1,  # 1Gb spill to remote storage
                    warning_duration=60 * 10,  # 10 minutes for waring
                    kill_duration=60 * 20,  # 20 minutes for kill
                ),
                sk.QueuedDurationCondition(
                    notice_duration=60 * 30,  # query was in queue for 30 minutes
                ),
                sk.BlockedDurationCondition(
                    notice_duration=60 * 5,  # query was locked by another transaction for 5 minutes
                ),
                sk.EstimatedScanDurationCondition(
                    min_estimated_scan_duration=60 * 60 * 2,  # query scan is estimated to take longer than 2 hours
                    warning_duration=60 * 10,  # warning after 10 minutes
                    kill_duration=60 * 20,  # kill after 20 minutes
                ),
            ]

            check_results = snowkill_engine.check_and_kill_pending_queries(checks)
            self.log.info(f"[{len(check_results)}] queries matched check conditions")
            check_results = snowkill_storage.store_and_remove_duplicate(check_results)
            self.log.info(f"[{len(check_results)}] queries remained after store deduplication")

            # Send notification for each new check result
            for result in check_results:
                self.slack_hook.send(blocks=snowkill_formatter.format(result))


with DAG(
        dag_id="snowkill",
        description="Snowkill performs real time query monitoring in Snowflake.",
        schedule="*/10 * * * *",  # Every 10 minutes
        catchup=False,
        start_date=datetime(2024, 1, 1),  # Set an appropriate start date here
        default_args=default_args
) as dag:
    RunSnowkillOperator(
        task_id="run_snowkill",
        snowkill_target_table="UTILS.MONITOR.SNOWKILL_LOG",
        snowsight_base_url="https://app.snowflake.com/my-org/my-account",
        max_active_tis_per_dag=1
    )

[littleK0i]

ACCOUNTADMIN is unfortunately required to run SHOW LOCKS IN ACCOUNT here: https://github.com/littleK0i/SnowKill/blob/master/snowkill/engine.py#L361

In theory, ACCOUNTADMIN should be a collection of privileges, and each specific grant should be available normally. Snowflake reps like to say that, but it is simply not true. Quite a lot of technical commands are attached specifically to ACCOUNTADMIN role.

Another notable example is RESOURCE MONITOR object type. Currently only ACCOUNTADMIN can attach a monitor to a warehouse.

---

I delayed HYBRID TABLE until this feature becomes generally available. Guess it is still in preview, after years of development.

But I agree, maybe it is a good time to add it.

---

I'll add Airflow example, thank you.

Does this example guarantee that only one instance of script can run in parallel? It is somewhat important.

[dwreeves]

Ah OK, I didn't run into that with my testing.

Poking through the code, it seems to only look for if a condition that's a AbstractBlockedQueryCondition is met, which right now is just BlockedDurationCondition(). I personally would rather not check for blocked queries in exchange for not granting ACCOUNTADMIN, as our company is strict on security stuff.

^ I mention this because I imagine others may be in the same situation, where granting ACCOUNTADMIN is a non-starter, and I wouldn't be able to use the package if that was strictly required. So, another solution could be telling users, use grant role ACCOUNTADMIN for everything, and if you use GRANT MANAGE WAREHOUSES ON ACCOUNT then you don't get to use BlockedDurationCondition() but you get the other features. I dunno, just my 2 cents!

---

Does this example guarantee that only one instance of script can run in parallel? It is somewhat important.

I just updated the code to add max_active_tis_per_dag=1 so that no more than 1 instance can run at a time!

---

Thanks for your speedy response and thank you again for the great package!