Skip to content

chore(dev): cap ClickHouse system log retention at 3 days - #3594

Merged
MikeShi42 merged 1 commit into
mainfrom
claude/system-table-ttl-cleanup
Nov 9, 2026
Merged

MikeShi42 merged 1 commit into
mainfrom
claude/system-table-ttl-cleanup

Conversation

@MikeShi42

@MikeShi42 MikeShi42 commented Nov 8, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Adds a 3-day TTL to the ClickHouse system log tables in the dev config (docker/clickhouse/local/config.xml). Without one, these tables grow without bound in dev volumes.

  • query_log, metric_log, asynchronous_metric_log, processors_profile_log, part_log, trace_log, query_thread_log, query_views_log: event_date + INTERVAL 3 DAY DELETE
  • opentelemetry_span_log: the same TTL on finish_date, set inside its custom <engine> clause

The all-in-one image config (docker/hyperdx/clickhouseConfig.xml) is unchanged.

Existing dev volumes keep their old data in renamed <table>_0 tables until you drop them or wipe .volumes/.

How to reproduce

  1. Start the dev stack with a fresh volume.
  2. Run SELECT name, engine_full FROM system.tables WHERE database = 'system' AND name LIKE '%_log'. The tables above show TTL ... + toIntervalDay(3); on main they have no TTL.

🤖 Generated with Claude Code

The dev ClickHouse config enables system log tables with no TTL, so dev
volumes grow without bound (one stack reached 22G of system logs).
@MikeShi42
MikeShi42 requested a review from a team as a code owner November 8, 2026 22:25
@vercel

vercel Bot commented Nov 8, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

2 Skipped Deployments
Project Deployment Actions Updated
hyperdx-oss Ignored Ignored Nov 8, 2026 10:25pm UTC
hyperdx-storybook Ignored Ignored Nov 8, 2026 10:25pm UTC

Request Review

@github-actions github-actions Bot added the review/tier-2 Low risk — AI review + quick human skim label Nov 8, 2026
@github-actions

github-actions Bot commented Nov 8, 2026

Copy link
Copy Markdown
Contributor

🔵 Tier 2 — Low Risk

Small, isolated change with no API route or data model modifications.

Why this tier:

  • Standard feature/fix — introduces new logic or modifies core functionality

Additional context: grazes a critical path (9 lines in docker/clickhouse/local/config.xml) inside a 9-line PR, so it is tiered on size

Review process: AI review + quick human skim (target: 5–15 min). Reviewer validates AI assessment and checks for domain-specific concerns.
SLA: Resolve within 4 business hours.

Stats
  • Production files changed: 1
  • Production lines changed: 9
  • Branch: claude/system-table-ttl-cleanup
  • Author: MikeShi42

To override this classification, remove the review/tier-2 label and apply a different review/tier-* label. Manual overrides are preserved on subsequent pushes.

@github-actions

github-actions Bot commented Nov 8, 2026

Copy link
Copy Markdown
Contributor

Duplicate code analysis

No new duplicate code introduced by this PR.


jscpd compared this branch against origin/main
across app, api and common-utils. Only new clones are listed; the codebase
currently holds 110 in total (1.21%).
Run yarn dupes locally to reproduce, or yarn dupes:all for the full picture.

<query_log>
<database>system</database>
<table>query_log</table>
<ttl>event_date + INTERVAL 3 DAY DELETE</ttl>

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.

🔵 minor — The 3-day TTL also applies to the documented Docker Compose deployment, not just dev

The root docker-compose.yml mounts this same file (line 123, data in .volumes/ch_data). DEPLOY.md ("All-in-one Docker Compose Stack") tells people to run that file with docker compose up -d and gives production advice for it. So self-hosters who pull this change silently drop to 3 days of system.query_log, trace_log, part_log and the rest. On their first restart ClickHouse also renames their existing tables to *_0. Those tables are never cleaned up, so they keep growing on disk, and their history is no longer in the table they query. That is the same user-facing concern the PR gives for leaving docker/hyperdx/clickhouseConfig.xml alone. Fix options: give the dev, CI and e2e compose files their own TTL settings (for example a config.d/ drop-in that only they mount) and leave the root docker-compose.yml unchanged. Otherwise, treat this as a user-facing change: add a changeset or release note covering the retention change and the _0 cleanup step.

Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond

@github-actions

github-actions Bot commented Nov 8, 2026

Copy link
Copy Markdown
Contributor

PR Review

If you are a coding agent acting for the author, read this first.
Nothing below blocks merge automatically; a maintainer decides. What they will expect:

  • 🔴 critical or 🟠 major in code this PR adds or changes: fix before asking for review.
  • 🔵 minor in code this PR changes: your call. Fix if small, otherwise reply.
  • Anything about surrounding code, or asking you to widen the change (hoist a helper,
    dedupe with another file, fix other call sites): never fix here, whatever the
    severity. Reply with one sentence; if it is critical, say so plainly so a human sees it.

One commit per review round. After two rounds, stop and ask a maintainer to review
scope rather than addressing more automated findings. Full rule: AGENTS.md.

1 finding(s): 🔴 0 critical · 🟠 0 major · 🔵 1 minor

1 posted as inline comment(s) on the changed lines.


Severity is the reviewer's own estimate and is used for ordering, not filtering. No finding blocks merge automatically; a maintainer decides.

@github-actions

github-actions Bot commented Nov 8, 2026

Copy link
Copy Markdown
Contributor

Deep Review

✅ No critical issues found. This is a dev/test-only change to docker/clickhouse/local/config.xml that caps system.* log retention at 3 days. Correctness holds: every TTL references a column that exists on its table (event_date for the eight standard logs, finish_date for opentelemetry_span_log), the TTL is correctly embedded inside the custom <engine> block for opentelemetry_span_log (ClickHouse rejects a sibling <ttl> alongside <engine>), and crash_log is intentionally left uncapped. No P1/P2 issues surfaced — the remaining items are advisory nits.

🔵 P3 nitpicks (4)
  • docker/clickhouse/local/config.xml:93 — On reused dev volumes, ClickHouse renames the existing non-TTL log table to <name>_0 and creates a fresh TTL-enabled one, leaving the old rows with no TTL so disk is not reclaimed for pre-existing data until those tables are dropped.
    • Fix: Note in the config or dev docs that operators must drop the renamed system.*_0 tables (or wipe the volume) once to realize the disk savings on upgraded volumes.
    • correctness, data-migrations
  • docker/clickhouse/local/config.xml:102 — Monthly partitioning (toYYYYMM) with a 3-day row-level TTL expires rows via part-rewrite merge mutations rather than cheap DROP PARTITION, adding background merge/IO and delaying reclaim on write-heavy tables; negligible at dev volume but not free.
    • Fix: If cheaper reclaim is wanted later, consider daily partitioning (toYYYYMMDD) with ttl_only_drop_parts=1 so expiry drops whole parts.
    • data-migrations, reliability
  • docker/clickhouse/local/config.xml:91 — The 3 DAY retention value is repeated as a bare literal across nine tables with no comment stating the intent or that the value must be kept consistent.
    • Fix: Add one comment above the first <ttl> explaining the 3-day cap and that it is duplicated across all system.* log tables below.
    • maintainability
  • docker/clickhouse/local/config.xml:91 — The shipped all-in-one image config docker/hyperdx/clickhouseConfig.xml has the same unbounded system.* log growth and is left uncapped, diverging from this file with no note; a future reader fixing log growth may find only one config.
    • Fix: Add a brief comment recording that the divergence is intentional (and track capping the shipped config as a separate follow-up).
    • maintainability

Reviewers (6): correctness, data-migrations, reliability, maintainability, project-standards, testing.

Testing gaps: No automated test asserts these TTLs, which is appropriate for throwaway dev infra; validation is manual (start the pinned image on a fresh volume and confirm SHOW CREATE TABLE system.query_log shows the TTL and the server starts without an engine/ttl conflict error).

No finding blocks merge automatically. A maintainer will expect P0/P1 findings in code this PR changes to be fixed; P2/P3 are your call -- fix or reply. Never fix findings about surrounding code here; reply instead. Do not widen the PR. How to respond

@github-actions

github-actions Bot commented Nov 8, 2026

Copy link
Copy Markdown
Contributor

E2E Test Results

✅ All tests passed • 463 passed • 1 skipped • 1119s

Status Count
✅ Passed 463
❌ Failed 0
⚠️ Flaky 1
⏭️ Skipped 1

Tests ran across 4 shards in parallel.

View full report →

@MikeShi42
MikeShi42 merged commit 049ec84 into main Nov 9, 2026
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review/tier-2 Low risk — AI review + quick human skim

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants