fix(clickstack): raise system log table TTL to 30 days to match the operator default

[marcleblanc2]

Why

Follow-up to #275. That PR pinned a 7 day TTL on the five system log tables to match what ClickHouse/clickhouse-operator#329 proposed at the time. #329 merged with a spec.settings.systemLogsTTLDays field instead, which the operator's webhook defaults to 30 days on new clusters. Chart and operator users should get the same retention.

What

• charts/clickstack/values.yaml: INTERVAL 7 DAY → INTERVAL 30 DAY on query_log, part_log, text_log, metric_log, asynchronous_metric_log.
• Kept the extraConfig approach rather than switching to systemLogsTTLDays: it works on operator versions that predate the field and on clusters that already exist, where the operator default is not applied.
• Updated the helm-unittest assertions and added a patch changeset with the same system.<table>_0 upgrade note as fix(clickstack): bound ClickHouse server log and system log table growth on the data volume #275.

helm unittest charts/clickstack: 254/254. helm template renders five 30 day TTLs.

[changeset-bot]

🦋 Changeset detected

Latest commit: 3a9027f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
helm-charts	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[marcleblanc2]

@wrn14897 follow-up to #275: the operator side (ClickHouse/clickhouse-operator#329) merged with a 30 day default, so this aligns the chart. Could you take a look when you have a minute?

[github-actions]

Deep Review

Scope: e5a3ddf..HEAD — 3 files: charts/clickstack/values.yaml (5 TTL bumps 7→30 DAY), charts/clickstack/tests/clickhouse-service_test.yaml (2 assertions updated), .changeset/system-log-ttl-30-days.md (new).

Intent: Raise the ClickHouse system-log-table TTL from 7 to 30 days across the five extraConfig log tables to match the operator's systemLogsTTLDays default, keeping the extraConfig approach so older operators and pre-existing clusters converge too.

The change is small, internally consistent, and renders correctly through toYaml/tpl in templates/clickhouse/cluster.yaml. TTL syntax is valid and all five tables are updated uniformly. No P0/P1 issues.

🟡 P2 — recommended

• charts/clickstack/tests/clickhouse-service_test.yaml:22 — The diff changes the TTL on all five system-log tables, but the unit test only asserts text_log.ttl and metric_log.ttl; the new 30-day value on query_log, part_log, and asynchronous_metric_log is unverified and could silently drift.
  • Fix: Add three equal assertions for spec.settings.extraConfig.query_log.ttl, part_log.ttl, and asynchronous_metric_log.ttl, each expecting event_date + INTERVAL 30 DAY DELETE.
  • _{testing, maintainability, project-standards, correctness}

🔵 P3 nitpicks (3)

• .changeset/system-log-ttl-30-days.md:4 — On upgrade the operator recreates each retimed system table and renames the old rows to system.<table>_0, which then sit on the shared 10Gi data volume with no automated reclaim; the changeset documents this but there is no cleanup hook.
  • Fix: Consider a documented post-upgrade DROP TABLE system.<table>_0 step or an optional helm hook Job, and mirror the note near the TTL block in values.yaml rather than only in the changeset.
  • _{data-migrations, correctness}
• charts/clickstack/values.yaml:479 — Quadrupling retention (7→30 days) roughly quadruples steady-state system-log footprint on the same 10Gi default volume the adjacent comment already calls tight; impact is deployment/ingest dependent and matches the upstream operator default, so it is an intended trade-off worth flagging, not a defect.
  • Fix: Note the expected footprint so operators on the 10Gi default monitor disk usage or size the volume up before upgrading.
  • _{data-migrations, correctness}
• .changeset/system-log-ttl-30-days.md:2 — The changeset uses a patch bump for a user-facing retention-default change, whereas the sibling changeset that introduced these TTLs used minor; the repo does not codify a bump-type rule, so this is only a consistency nit for a maintainer glance.
  • Fix: Confirm patch is the intended bump, or align with the prior minor precedent for default changes.
  • _{project-standards}

---

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

Testing gaps: query_log, part_log, and asynchronous_metric_log 30-day TTLs are exercised by no assertion; no test/procedure validates _0 orphan cleanup or effective runtime TTL vs the operator's systemLogsTTLDays after reconcile.

_{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.}