Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ jobs:
docker load --input /tmp/docker-deps/deps.tar
fi
images=(
"ghcr.io/getsentry/image-mirror-altinity-clickhouse-server:25.3.6.10034.altinitystable"
"ghcr.io/getsentry/image-mirror-altinity-clickhouse-server:25.3.8.10041.altinitystable"
"ghcr.io/getsentry/image-mirror-confluentinc-cp-kafka:6.2.0"
"ghcr.io/getsentry/image-mirror-confluentinc-cp-zookeeper:6.2.0"
"ghcr.io/getsentry/docker-redis-cluster:7.0.10"
Expand Down Expand Up @@ -283,7 +283,7 @@ jobs:
docker load --input /tmp/docker-deps/deps.tar
fi
images=(
"ghcr.io/getsentry/image-mirror-altinity-clickhouse-server:25.3.6.10034.altinitystable"
"ghcr.io/getsentry/image-mirror-altinity-clickhouse-server:25.3.8.10041.altinitystable"
"ghcr.io/getsentry/image-mirror-confluentinc-cp-kafka:6.2.0"
"ghcr.io/getsentry/image-mirror-confluentinc-cp-zookeeper:6.2.0"
"ghcr.io/getsentry/docker-redis-cluster:7.0.10"
Expand Down Expand Up @@ -458,7 +458,8 @@ jobs:
matrix:
version:
[
"25.3.6.10034.altinitystable",
"25.3.8.10041.altinitystable",
"25.8.16.10001.altinitystable",
]

steps:
Expand Down
2 changes: 1 addition & 1 deletion devservices/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ x-programs:

services:
clickhouse:
image: ghcr.io/getsentry/image-mirror-altinity-clickhouse-server:25.3.6.10034.altinitystable
image: ghcr.io/getsentry/image-mirror-altinity-clickhouse-server:25.3.8.10041.altinitystable
ulimits:
nofile:
soft: 262144
Expand Down
12 changes: 6 additions & 6 deletions docker-compose.gcb.yml
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ services:
KAFKA_LOG4J_ROOT_LOGLEVEL: "WARN"
KAFKA_TOOLS_LOG4J_LOGLEVEL: "WARN"
clickhouse:
image: "${CLICKHOUSE_IMAGE:-ghcr.io/getsentry/image-mirror-altinity-clickhouse-server:25.3.6.10034.altinitystable}"
image: "${CLICKHOUSE_IMAGE:-ghcr.io/getsentry/image-mirror-altinity-clickhouse-server:25.3.8.10041.altinitystable}"
hostname: clickhouse.local
extra_hosts:
- "clickhouse.local:127.0.0.1" # Add entry to /etc/hosts file
Expand All @@ -92,7 +92,7 @@ services:
clickhouse-query:
depends_on:
- zookeeper
image: "${CLICKHOUSE_IMAGE:-ghcr.io/getsentry/image-mirror-altinity-clickhouse-server:25.3.6.10034.altinitystable}"
image: "${CLICKHOUSE_IMAGE:-ghcr.io/getsentry/image-mirror-altinity-clickhouse-server:25.3.8.10041.altinitystable}"
hostname: clickhouse-query.local
extra_hosts:
- "clickhouse-query.local:127.0.0.1" # Add entry to /etc/hosts file
Expand All @@ -108,7 +108,7 @@ services:
clickhouse-01:
depends_on:
- zookeeper
image: "${CLICKHOUSE_IMAGE:-ghcr.io/getsentry/image-mirror-altinity-clickhouse-server:25.3.6.10034.altinitystable}"
image: "${CLICKHOUSE_IMAGE:-ghcr.io/getsentry/image-mirror-altinity-clickhouse-server:25.3.8.10041.altinitystable}"
hostname: clickhouse-01.local
extra_hosts:
- "clickhouse-01.local:127.0.0.1" # Add entry to /etc/hosts file
Expand All @@ -125,7 +125,7 @@ services:
clickhouse-02:
depends_on:
- zookeeper
image: "${CLICKHOUSE_IMAGE:-ghcr.io/getsentry/image-mirror-altinity-clickhouse-server:25.3.6.10034.altinitystable}"
image: "${CLICKHOUSE_IMAGE:-ghcr.io/getsentry/image-mirror-altinity-clickhouse-server:25.3.8.10041.altinitystable}"
hostname: clickhouse-02.local
extra_hosts:
- "clickhouse-02.local:127.0.0.1" # Add entry to /etc/hosts file
Expand All @@ -142,7 +142,7 @@ services:
clickhouse-03:
depends_on:
- zookeeper
image: "${CLICKHOUSE_IMAGE:-ghcr.io/getsentry/image-mirror-altinity-clickhouse-server:25.3.6.10034.altinitystable}"
image: "${CLICKHOUSE_IMAGE:-ghcr.io/getsentry/image-mirror-altinity-clickhouse-server:25.3.8.10041.altinitystable}"
hostname: clickhouse-03.local
extra_hosts:
- "clickhouse-03.local:127.0.0.1" # Add entry to /etc/hosts file
Expand All @@ -159,7 +159,7 @@ services:
clickhouse-04:
depends_on:
- zookeeper
image: "${CLICKHOUSE_IMAGE:-ghcr.io/getsentry/image-mirror-altinity-clickhouse-server:25.3.6.10034.altinitystable}"
image: "${CLICKHOUSE_IMAGE:-ghcr.io/getsentry/image-mirror-altinity-clickhouse-server:25.3.8.10041.altinitystable}"
hostname: clickhouse-04.local
extra_hosts:
- "clickhouse-04.local:127.0.0.1" # Add entry to /etc/hosts file
Expand Down
3 changes: 2 additions & 1 deletion docs/source/clickhouse/supported_versions.rst
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@ ClickHouse supported versions
The following version(s) of Clickhouse have been tested and are known to work
with Snuba:

- 23.8.11.29 (Altinity Stable Build)
- 25.3.8.10041 (Altinity Stable Build)
- 25.8.16.10001 (Altinity Stable Build)

Any version of Clikhouse used outside of this list could potentially work,
but is not guaranteed to work. Some functionality might be broken. Use a
Expand Down
6 changes: 2 additions & 4 deletions snuba/migrations/clickhouse.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,2 @@
CLICKHOUSE_SERVER_MIN_VERSION = "23.8.11.29"
# Note: SaaS, self-hosted, and sentry dev
# environements should all be on 23.8.11.29
CLICKHOUSE_SERVER_MAX_VERSION = "25.3.6.10034"
CLICKHOUSE_SERVER_MIN_VERSION = "25.3.6.10034"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Min version set to old value instead of new default

Medium Severity

CLICKHOUSE_SERVER_MIN_VERSION is set to "25.3.6.10034" (the old default image version) instead of "25.3.8.10041" (the new default). The PR description explicitly states the min bound is 25.3.8.10041, and the supported versions doc lists 25.3.8.10041 as the lowest supported version—yet the version check in check_clickhouse will accept 25.3.6.10034, which is no longer listed as a supported version.

Fix in Cursor Fix in Web

CLICKHOUSE_SERVER_MAX_VERSION = "25.8.16.10001"
Original file line number Diff line number Diff line change
Expand Up @@ -9,27 +9,24 @@
class Migration(migration.ClickhouseNodeMigrationLegacy):
blocking = False

def __forward_migrations(
self, table_name: str
) -> Sequence[operations.SqlOperation]:
def __forward_migrations(self, table_name: str) -> Sequence[operations.SqlOperation]:
return [
operations.ModifyColumn(
storage_set=StorageSetKey.EVENTS,
table_name=table_name,
column=Column(
"level", String(Modifiers(low_cardinality=True, nullable=True))
),
column=Column("level", String(Modifiers(low_cardinality=True, nullable=True))),
)
]

def __backwards_migrations(
self, table_name: str
) -> Sequence[operations.SqlOperation]:
def __backwards_migrations(self, table_name: str) -> Sequence[operations.SqlOperation]:
return [
operations.ModifyColumn(
storage_set=StorageSetKey.EVENTS,
table_name=table_name,
column=Column("level", String(Modifiers(low_cardinality=True))),
column=Column(
"level",
String(Modifiers(low_cardinality=True, default="''")),
),
)
]

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,7 @@
}


_attr_columns = [
Column(f"attrs_{type_name}", type_spec) for type_name, type_spec in _TYPES.items()
]
_attr_columns = [Column(f"attrs_{type_name}", type_spec) for type_name, type_spec in _TYPES.items()]


columns: List[Column[Modifiers]] = [
Expand All @@ -40,7 +38,7 @@
MV_QUERY = f"""
SELECT
project_id,
'span',
'span' AS item_type,
toDate(_sort_timestamp) AS date,
retention_days as retention_days,
mapConcat(attr_str_0, attr_str_1, attr_str_2, attr_str_3, attr_str_4, attr_str_5, attr_str_6, attr_str_7, attr_str_8, attr_str_9, attr_str_10, attr_str_11, attr_str_12, attr_str_13, attr_str_14, attr_str_15, attr_str_16, attr_str_17, attr_str_18, attr_str_19) AS attrs_string, -- `attrs_string` Map(String, String),
Expand Down Expand Up @@ -100,7 +98,6 @@


class Migration(migration.ClickhouseNodeMigration):

blocking = False
storage_set_key = StorageSetKey.EVENTS_ANALYTICS_PLATFORM
granularity = "8192"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,7 @@
}


_attr_columns = [
Column(f"attrs_{type_name}", type_spec) for type_name, type_spec in _TYPES.items()
]
_attr_columns = [Column(f"attrs_{type_name}", type_spec) for type_name, type_spec in _TYPES.items()]


columns: List[Column[Modifiers]] = [
Expand All @@ -40,7 +38,7 @@
MV_QUERY = f"""
SELECT
project_id,
'span',
'span' AS item_type,
toDate(_sort_timestamp) AS date,
retention_days as retention_days,
mapConcat(attr_str_0, attr_str_1, attr_str_2, attr_str_3, attr_str_4, attr_str_5, attr_str_6, attr_str_7, attr_str_8, attr_str_9, attr_str_10, attr_str_11, attr_str_12, attr_str_13, attr_str_14, attr_str_15, attr_str_16, attr_str_17, attr_str_18, attr_str_19) AS attrs_string, -- `attrs_string` Map(String, String),
Expand Down Expand Up @@ -100,7 +98,6 @@


class Migration(migration.ClickhouseNodeMigration):

blocking = False
storage_set_key = StorageSetKey.EVENTS_ANALYTICS_PLATFORM
granularity = "8192"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,33 +2,57 @@

from snuba.clusters.storage_sets import StorageSetKey
from snuba.migrations import migration, operations
from snuba.migrations.migration_utilities import get_clickhouse_version_for_storage_set
from snuba.migrations.operations import OperationTarget


class Migration(migration.ClickhouseNodeMigration):
blocking = False
storage_set_key = StorageSetKey.EVENTS_ANALYTICS_PLATFORM
class _AlterWorkloadOp(operations.RunSql):
"""Version-aware RunSql that picks the correct thread setting at execution time."""

def forwards_ops(self) -> Sequence[operations.SqlOperation]:
alter_workload = """
def __init__(self) -> None:
# Placeholder statement for validation; actual SQL is generated in execute()
super().__init__(
storage_set=StorageSetKey.EVENTS_ANALYTICS_PLATFORM,
statement="SELECT 1",
target=OperationTarget.LOCAL,
)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Dry-run outputs placeholder "SELECT 1" instead of actual SQL

Medium Severity

_AlterWorkloadOp passes statement="SELECT 1" to the RunSql parent __init__. Because RunSql stores the statement via name-mangled self.__statement (becoming _RunSql__statement), the inherited format_sql() method always returns "SELECT 1". The migration framework's dry-run mode calls op.format_sql() to display what SQL will be executed, so operators previewing this migration see "SELECT 1" instead of the actual CREATE OR REPLACE WORKLOAD statement. The SqlOperation.__eq__ method also relies on format_sql(), making equality checks unreliable.

Fix in Cursor Fix in Web


def _build_statement(self, thread_setting: str) -> str:
return f"""
CREATE OR REPLACE WORKLOAD low_priority_deletes
IN all
SETTINGS
priority = 100,
max_requests = 2,
max_threads = 4;
{thread_setting} = 4;
"""

return [
operations.RunSql(
storage_set=self.storage_set_key,
statement=alter_workload,
target=OperationTarget.LOCAL,
),
]
def execute(self) -> None:
ch_version = get_clickhouse_version_for_storage_set(self._storage_set, None)

# max_threads was renamed to max_concurrent_threads in ClickHouse 25.8+
if ch_version >= (25, 8):
thread_setting = "max_concurrent_threads"
else:
thread_setting = "max_threads"

resolved = operations.RunSql(
storage_set=self._storage_set,
statement=self._build_statement(thread_setting),
target=OperationTarget.LOCAL,
)
resolved.execute()


class Migration(migration.ClickhouseNodeMigration):
blocking = False
storage_set_key = StorageSetKey.EVENTS_ANALYTICS_PLATFORM

def forwards_ops(self) -> Sequence[operations.SqlOperation]:
return [_AlterWorkloadOp()]

def backwards_ops(self) -> Sequence[operations.SqlOperation]:
# Restore original settings without max_threads
# Restore original settings without thread limit
alter_workload = """
CREATE OR REPLACE WORKLOAD low_priority_deletes
IN all
Expand Down
8 changes: 4 additions & 4 deletions snuba/snuba_migrations/generic_metrics/0024_gauges_mv.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,9 @@ def forwards_ops(self) -> Sequence[operations.SqlOperation]:
tags.key,
tags.indexed_value,
tags.raw_value,
maxState(timestamp as raw_timestamp) as last_timestamp,
maxState(timestamp) as last_timestamp,
toDateTime(multiIf(granularity=0,10,granularity=1,60,granularity=2,3600,granularity=3,86400,-1) *
intDiv(toUnixTimestamp(raw_timestamp),
intDiv(toUnixTimestamp(timestamp),
multiIf(granularity=0,10,granularity=1,60,granularity=2,3600,granularity=3,86400,-1))) as rounded_timestamp,
least(retention_days,
multiIf(granularity=0,decasecond_retention_days,
Expand All @@ -64,7 +64,7 @@ def forwards_ops(self) -> Sequence[operations.SqlOperation]:
maxState(arrayJoin(gauges_values.max)) as max,
sumState(arrayJoin(gauges_values.sum)) as sum,
sumState(arrayJoin(gauges_values.count)) as count,
argMaxState(arrayJoin(gauges_values.last), raw_timestamp) as last
argMaxState(arrayJoin(gauges_values.last), timestamp) as last
FROM generic_metric_gauges_raw_local
WHERE materialization_version = 2
AND metric_type = 'gauge'
Expand All @@ -76,7 +76,7 @@ def forwards_ops(self) -> Sequence[operations.SqlOperation]:
tags.key,
tags.indexed_value,
tags.raw_value,
raw_timestamp,
timestamp,
granularity,
retention_days
""",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ class Migration(migration.ClickhouseNodeMigration):
Column("timestamp", DateTime(modifiers=Modifiers(codecs=["DoubleDelta"]))),
Column("retention_days", UInt(16)),
Column("tag_values", AggregateFunction("groupUniqArray", [String()])),
Column("value", AggregateFunction("sum", [Float(64)])),
Column("count", AggregateFunction("sum", [Float(64)])),
]
storage_set_key = StorageSetKey.GENERIC_METRICS_COUNTERS

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,7 @@ class Migration(migration.ClickhouseNodeMigration):
blocking = False
granularity = "2048"
tag_value_view_name = "generic_metric_counters_meta_tag_value_aggregation_mv"
tag_value_local_table_name = (
"generic_metric_counters_meta_tag_value_aggregated_local"
)
tag_value_local_table_name = "generic_metric_counters_meta_tag_value_aggregated_local"
tag_value_dist_table_name = "generic_metric_counters_meta_tag_value_aggregated_dist"
tag_value_table_columns: Sequence[Column[Modifiers]] = [
Column("project_id", UInt(64)),
Expand All @@ -38,7 +36,7 @@ class Migration(migration.ClickhouseNodeMigration):
Column("timestamp", DateTime(modifiers=Modifiers(codecs=["DoubleDelta"]))),
Column("retention_days", UInt(16)),
Column("tag_values", AggregateFunction("groupUniqArray", [String()])),
Column("value", AggregateFunction("sum", [Float(64)])),
Column("count", AggregateFunction("sum", [Float(64)])),
]

storage_set_key = StorageSetKey.GENERIC_METRICS_COUNTERS
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ def backwards_ops(self) -> Sequence[operations.SqlOperation]:
storage_set=self.storage_set_key,
table_name=self.old_meta_dist_table_name,
engine=table_engines.Distributed(
local_table_name=self.meta_local_table_name, sharding_key=None
local_table_name=self.old_meta_local_table_name, sharding_key=None
),
columns=self.meta_table_columns,
target=OperationTarget.DISTRIBUTED,
Expand All @@ -259,7 +259,7 @@ def backwards_ops(self) -> Sequence[operations.SqlOperation]:
storage_set=self.storage_set_key,
view_name=self.old_meta_view_name,
columns=self.meta_table_columns,
destination_table_name=self.meta_local_table_name,
destination_table_name=self.old_meta_local_table_name,
target=OperationTarget.LOCAL,
query="""
SELECT
Expand Down Expand Up @@ -302,7 +302,7 @@ def backwards_ops(self) -> Sequence[operations.SqlOperation]:
storage_set=self.storage_set_key,
table_name=self.old_tag_value_dist_table_name,
engine=table_engines.Distributed(
local_table_name=self.tag_value_local_table_name, sharding_key=None
local_table_name=self.old_tag_value_local_table_name, sharding_key=None
),
columns=self.tag_value_table_columns,
target=OperationTarget.DISTRIBUTED,
Expand All @@ -311,7 +311,7 @@ def backwards_ops(self) -> Sequence[operations.SqlOperation]:
storage_set=self.storage_set_key,
view_name=self.old_tag_value_view_name,
columns=self.tag_value_table_columns,
destination_table_name=self.tag_value_local_table_name,
destination_table_name=self.old_tag_value_local_table_name,
target=OperationTarget.LOCAL,
query="""
SELECT
Expand Down
8 changes: 4 additions & 4 deletions snuba/snuba_migrations/generic_metrics/0057_gauges_mv3.py
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,9 @@ def forwards_ops(self) -> Sequence[operations.SqlOperation]:
tags.key,
tags.indexed_value,
tags.raw_value,
maxState(timestamp as raw_timestamp) as last_timestamp,
maxState(timestamp) as last_timestamp,
toDateTime(multiIf(granularity=0,10,granularity=1,60,granularity=2,3600,granularity=3,86400,-1) *
intDiv(toUnixTimestamp(raw_timestamp),
intDiv(toUnixTimestamp(timestamp),
multiIf(granularity=0,10,granularity=1,60,granularity=2,3600,granularity=3,86400,-1))) as rounded_timestamp,
least(retention_days,
multiIf(granularity=0,decasecond_retention_days,
Comment on lines 52 to 60
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Bug: In migration 0057_gauges_mv3.py, the dest_table_columns defines a timestamp column, but the view's query aliases it as rounded_timestamp, causing a mismatch that will fail the migration.
Severity: HIGH

Suggested Fix

Update the dest_table_columns definition in snuba/snuba_migrations/generic_metrics/0057_gauges_mv3.py. Change the Column("timestamp", ...) to Column("rounded_timestamp", ...) to match the alias in the SELECT query and the schema of the destination table.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: snuba/snuba_migrations/generic_metrics/0057_gauges_mv3.py#L52-L60

Potential issue: In the Snuba migration `0057_gauges_mv3.py`, the materialized view is
configured with a `dest_table_columns` list that specifies a column named `timestamp`.
However, the `SELECT` query for this view aliases the corresponding computed time column
as `rounded_timestamp`. The actual destination table,
`generic_metric_gauges_aggregated_local` (created in migration 0022), expects a column
named `rounded_timestamp`. This mismatch between the view's column definition
(`timestamp`) and the alias used in the query (`rounded_timestamp`) will cause the
migration to fail on ClickHouse 25.8, which enforces strict column name matching between
a materialized view's `SELECT` statement and its destination table.

Expand All @@ -68,7 +68,7 @@ def forwards_ops(self) -> Sequence[operations.SqlOperation]:
sumState(sampling_weight * arrayJoin(gauges_values.sum)) AS sum_weighted,
sumState(arrayJoin(gauges_values.count)) as count,
sumState(sampling_weight * arrayJoin(gauges_values.count)) AS count_weighted,
argMaxState(arrayJoin(gauges_values.last), raw_timestamp) as last
argMaxState(arrayJoin(gauges_values.last), timestamp) as last
FROM generic_metric_gauges_raw_local
WHERE materialization_version = 3
AND metric_type = 'gauge'
Expand All @@ -80,7 +80,7 @@ def forwards_ops(self) -> Sequence[operations.SqlOperation]:
tags.key,
tags.indexed_value,
tags.raw_value,
raw_timestamp,
timestamp,
granularity,
retention_days
""",
Expand Down
Loading
Loading