feat(llma): add clickhouse migration for tracing tables - #70737
Conversation
🤖 CI report
|
|
Note 🤖 stamphog reviewed Gates denied this PR outright (deny-listed migration files, over size ceiling, classified T2-never), and it introduces a new ClickHouse migration plus rewritten SQL table definitions on the logs cluster — risky infra/data territory needing human review, and two bot reviewers still show in-flight (eyes) reactions.
Gate mechanics and policy version
|
|
Reviews (1): Last reviewed commit: "fix(llma): model trace tables in local l..." | Re-trigger Greptile |
|
Reviewed the ClickHouse migration, trace table/MV definitions, named Kafka collection configuration, and the trace ingestion/authentication flow. I found no security issues to report. |
PR overviewAll previously flagged issues have been addressed. No open security concerns remain on this pull request. Security reviewNo open security issues remain on this pull request. Fixed/addressed: 1 · PR risk: 0/10 |
7a1efd8 to
da9c834
Compare
Add an idempotent migration creating the tracing product ClickHouse tables on the logs cluster: trace_spans, trace_attributes (plain, matches current prod) and trace_attributes2 (aggregating), the shared distributed tables, the span attribute materialized views (existing + v2 feeding trace_attributes2), the Kafka ingestion table + MV, and the kafka-metrics table + MV. Every statement uses CREATE ... IF NOT EXISTS so it is a no-op against the objects already created manually in prod. Base and distributed tables are registered in schema.py for local/CI parity; the materialized views and Kafka table stay migration-only, mirroring the logs cluster convention. Adds the warpstream_traces named collection to local ClickHouse config for the Kafka table. Generated-By: PostHog Code Task-Id: b927db46-acf2-4a29-9c9d-59bf46d38cb6
The clickhouse smoke test (per-cluster topology) runs migrations against the multinode ClickHouse config, not the local dev default.xml. The kafka_trace_spans_avro table uses the warpstream_traces named collection, which was only added to default.xml, so the migration failed on the logs node with NAMED_COLLECTION_DOESNT_EXIST. Add it to every multinode node config alongside the existing warpstream_logs entry. Generated-By: PostHog Code Task-Id: b927db46-acf2-4a29-9c9d-59bf46d38cb6
The multinode migration smoke test introspects the migrated local logs node and diffs it against posthog/clickhouse/hcl/golden/local-logs.hcl. The new tracing migration creates 15 objects on the logs node that the HCL layer never modeled, so check-live reported drift and failed. Add the trace objects (trace_spans, trace_attributes, trace_attributes2, the distributed tables, the span/resource/name attribute MVs and their v2 variants, the Kafka ingestion table + MV, and the kafka-metrics table + MV) to roles/logs/local and regenerate the golden and build-from-scratch SQL via hclexp. Scoped to the local logs node, which is what the smoke test checks. Generated-By: PostHog Code Task-Id: b927db46-acf2-4a29-9c9d-59bf46d38cb6
The kafka_trace_spans_avro_mv projected everything except an original_expiry_timestamp, so inserted spans took the trace_spans column default (epoch). With PARTITION BY toDate(original_expiry_timestamp) and TTL original_expiry_timestamp, those rows land in a 1970 partition that TTL immediately drops. Derive it from observed_timestamp plus the retention-days header (default 15), matching the logs ingestion mv. Regenerate the local logs HCL golden and build-from-scratch SQL for the updated mv. Generated-By: PostHog Code Task-Id: b927db46-acf2-4a29-9c9d-59bf46d38cb6
Add is_root_span as a dimension to projection_aggregate_counts. The Spans-view sparkline keeps summing event_count per minute across the new dimension; the Traces-view sparkline (distinct traces per minute) can now serve from the projection via sumIf(event_count, is_root_span = 1) — one root span per trace — instead of the uniqExactIf(trace_id, ...) raw scan. Regenerate the schema snapshot and the local logs HCL golden and SQL. Generated-By: PostHog Code Task-Id: b927db46-acf2-4a29-9c9d-59bf46d38cb6
_bytes_uncompressed and _bytes_compressed used toInt64OrNull on the Kafka byte-size headers, but the trace_spans target columns are non-nullable UInt64. A missing or malformed header yields NULL, and inserting NULL into a non-nullable column fails the materialized-view insert (CANNOT_INSERT_NULL_IN_ORDINARY_COLUMN), dropping otherwise valid spans. Default to 0 via toInt64OrDefault, matching the _record_count handling on the line above. Regenerate the local logs HCL golden and SQL for the updated mv. Generated-By: PostHog Code Task-Id: b927db46-acf2-4a29-9c9d-59bf46d38cb6
Master added a duplicate-object guard (duplicates-baseline.txt) and renamed the local logs env to local-multi. Modeling the trace objects in the self-contained roles/logs/local layer makes trace_spans_kafka_metrics and trace_spans_to_kafka_metrics_mv appear in two composed layers (local plus prod-us), which the guard flags. The self-contained local layer re-declaring cloud objects is intentional — the same reason the other trace objects are already baselined — so add these two to the baseline. Generated-By: PostHog Code Task-Id: b927db46-acf2-4a29-9c9d-59bf46d38cb6
419ac6f to
8b98fd7
Compare
| toStartOfInterval(timestamp, toIntervalMinute(10)) AS time_bucket, | ||
| service_name AS service_name, | ||
| resource_fingerprint, | ||
| arrayJoin(resource_attributes) AS attribute, |
There was a problem hiding this comment.
Bound resource-attribute fanout
resource_attributes originates from the authenticated OTLP payload and has no count or key/value-length cap before arrayJoin. A sender can put a large resource map once in a ≤2 MB request and attach it to many spans; capture-logs duplicates that map into every span, while this view and its …2 counterpart each emit one indexed attribute row per key per span. Quota and rate limiting charge only the original request bytes, so a project-token holder can turn a small accepted payload into millions of ClickHouse writes and exhaust the shared logs cluster.
Prompt To Fix With AI
Enforce trace resource-attribute limits at ingestion before rows are created: cap the number of resource attributes and the encoded key/value sizes (and ideally cap spans per request). Apply equivalent validation/filtering in the ClickHouse resource-attribute materialized views as defense in depth, including both trace_attributes destinations. Ensure quota/rate-limit accounting reflects the bounded payload that will be expanded.Severity: medium | Confidence: 94% | React with 👍 if useful or 👎 if not
Problem
The tracing product's ClickHouse tables were created manually in prod but never captured in a migration or in local/CI schema setup. That means new environments (and local dev) don't get them, and the repo's SQL definitions had drifted from what's actually running in prod.
Changes
Adds an idempotent migration (
0289_trace_spans_and_attributes) that creates the full tracing table set on the logs cluster (NodeRole.LOGS). Every statement isCREATE ... IF NOT EXISTS, so it's a no-op against the objects already present in prod and creates them fresh everywhere else.Objects created:
trace_spans,trace_attributes,trace_attributes2,trace_spans_kafka_metricstrace_spans_distributed,trace_attributes_distributedtrace_span_to_attributes,trace_span_to_resource_attributes,trace_span_to_span_attributes(→trace_attributes) and their…2variants (→trace_attributes2)kafka_trace_spans_avro+kafka_trace_spans_avro_mvtrace_spans_to_kafka_metrics_mvThe SQL definitions in
posthog/clickhouse/traces/were rewritten to match the live prod schema exactly (dumped fromsystem.tables), since the earlier drafts had diverged (wrong engine, extra codecs, differently-named MVs).Note
trace_attributesis kept as the plainReplicatedMergeTreethat's live in prod today.trace_attributes2is a newReplicatedAggregatingMergeTreewith the same shape, dual-written by the…2MVs. Attribute counts are summed at read time so the plain engine isn't wrong, but it never collapses partial rows, which bloats storage and slows attribute queries. The cutover (backfilltrace_attributes2, repoint the singletrace_attributes_distributedat it, movelogic.pyqueries onto the distributed tables, then drop the old base table and its MVs) lands in a separate PR.Registration follows the logs-cluster convention: base and distributed tables go in
schema.pyfor local/CI parity, while the materialized views and the Kafka table stay migration-only (creating cascade MVs ontrace_spansin the shared test schema breaks the tracing tests, which hand-manage those tables). Adds thewarpstream_tracesnamed collection to local ClickHouse config so the Kafka table can be created locally.How did you test this code?
All automated, run locally by me (Claude):
trace_spans(with the_part_offsetprojections andis_root_span) and thattrace_attributes2reportsReplicatedAggregatingMergeTree.posthog/clickhouse/test/test_schema.py(schema snapshots regenerated for the new base tables; the existing MV snapshots came out byte-identical after refactoring the MV bodies into shared helpers, confirming no behavior change).test_attribute_value_search,test_attribute_breakdown,test_flat_span_query, etc.) pass against a fresh test DB.I did not test against prod ClickHouse directly; the migration is a pure
IF NOT EXISTSno-op there by construction, validated against the prod schema dump.Docs update
No user-facing docs change.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Frank drove this; I (Claude) wrote the migration and reconciled the repo SQL against a prod
system.tablesdump he provided. I invoked the/clickhouse-migrationsskill for the migration conventions (node roles,IF NOT EXISTS, local-parity registration, migration-only PR scope).Key decisions: matched prod's schema verbatim rather than the pre-existing drafts; caught that
trace_attributeswas plain MergeTree in prod and added the aggregatingtrace_attributes2alongside it for a later cutover (single distributed table, repointed then, per Frank's call); kept the MVs and Kafka table out ofschema.pyafter finding that cascade MVs ontrace_spansbroke the product's hand-managed test tables.Created with PostHog Code