Skip to content

[OPIK-7859] [BE] feat: add cipx_savings_facts table for realized savings measurement - #7913

Open
andriidudar wants to merge 2 commits into
mainfrom
andriid/OPIK-7859-cipx-savings-facts
Open

[OPIK-7859] [BE] feat: add cipx_savings_facts table for realized savings measurement#7913
andriidudar wants to merge 2 commits into
mainfrom
andriid/OPIK-7859-cipx-savings-facts

Conversation

@andriidudar

Copy link
Copy Markdown
Contributor

Details

Adds the ClickHouse table that stores measured savings: one row per (workspace, day, user, recommendation, harness, quantity, tier, causal link), written nightly by the cost API's measurement job from cipx_spend_blocks and the recommendation apply log. Table only — no Java reads or writes it; the writer lives in ai-cost-backend.

  • Dollars are computed on the day the row is written, per (model, billing tier), so money not spent on a day is priced at that day's rates and stays stable through later rate-table changes. A rate correction is a partition drop plus a recompute from the source blocks.
  • The three attribution axes are columns rather than something a read derives: quantity (was the spend real, or never paid), tier (are the tokens observed or modelled), causal link (configured, performed, or merely detected). The engine derives each from the evidence that fed the row, then freezes it as provenance.
  • harness is in the sort key because the apply log is keyed by it: the same recommendation applied for two harnesses is two applied things, and rows sharing a key would collapse on the ReplacingMergeTree, silently dropping one harness's savings.
  • Versioned on last_updated_at with a now64(6) default, matching every other cipx ReplacingMergeTree, so an interrupted run or a deliberate recompute is idempotent. Keyed by workspace_id like the rest of the family.

Deploy order: this migration must be applied before the ai-cost-backend change that writes to the table (comet-ml/ai-cost-backend#TBD) — that writer expects harness in the key and does not send a version column.

Change checklist

  • User facing
  • Documentation update

Issues

  • Resolves #
  • OPIK-7859

AI-WATERMARK

AI-WATERMARK: yes

  • Tools: Claude Code
  • Model(s): Claude Opus 5
  • Scope: full implementation
  • Human verification: schema reviewed against the cipx table family; DDL exercised locally end to end

Testing

  • Created the table from this DDL on local ClickHouse and ran the measurement job against it end to end (baseline frozen from real blocks, 19 fact rows written across 2026-08-06 → 08-18).
  • Verified idempotency directly: two inserts of the same key collapse under FINAL to the later row, and the freshness read returns that row's last_updated_at.
  • Verified the sort key cannot be extended after ship except by ADD COLUMN without a default in the same statement as MODIFY ORDER BY — which is why harness is included now rather than later.
  • No Java touched, so no backend test run: this changeset adds a table the Java service never queries.

Documentation

Column-by-column rationale is in the migration's own header comment. Measurement methodology and the calibration procedure live in ai-cost-backend/docs/auto-memory-calibration.md.

…ngs measurement

The storage for measured savings: one row per (workspace, day, user, recommendation,
harness, quantity, tier, causal link), written nightly by the cost API's measurement job
from cipx_spend_blocks and the recommendation apply log.

Characters are frozen, not dollars — but the dollar figure is computed on the day the row
is written, per (model, billing tier) inside the job, so the money not spent on a day is
priced at that day's rates and stays stable through later rate-table changes. A rate
correction is a partition drop and a recompute from the source blocks.

The three attribution axes are columns rather than something the read derives: quantity
(was the spend real, or never paid), tier (are the tokens observed or modelled), and
causal link (did we configure it, perform it, or merely detect it). All three are derived
by the engine from the evidence that fed each row, then frozen as provenance, so the
presentation reads from this table alone with no cross-store join and no recomputation.

Harness is in the key because the apply log is keyed by it: the same recommendation
applied for two harnesses is two applied things, and rows that shared a key would
collapse to one, silently dropping one harness's savings.

Versioned on last_updated_at with a now64(6) default, like every other cipx
ReplacingMergeTree — a run interrupted mid-insert, or a deliberate recompute after a
measurement fix, is idempotent. Keyed by workspace_id like the rest of the family, so the
whole cipx set filters by one scope key.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

⏱️ pre-commit per-hook timing

Hook Description Result Duration
🐍 mypy — python sdk Static type check 1.53s
🐍 fix end of files — python sdk Ensure files end in a newline 0.04s
🐍 trim trailing whitespace — python sdk Strip trailing whitespace 0.04s
🐍 ruff-format — python sdk Format Python code (ruff) 0.01s
🐍 ruff — python sdk Lint + autofix Python (ruff) 0.01s
Total (5 ran) 1.63s
⏭️ 38 skipped (no matching files changed)
Hook Description Result
🤖 trim trailing whitespace — optimizer Strip trailing whitespace ⏭️
🤖 fix end of files — optimizer Ensure files end in a newline ⏭️
🤖 check yaml — optimizer Validate YAML syntax ⏭️
🤖 check json — optimizer Validate JSON syntax ⏭️
🤖 check toml — optimizer Validate TOML syntax ⏭️
🤖 check for added large files — optimizer Block large files (>1MB) ⏭️
🔐 detect private key — optimizer Block committed private keys ⏭️
🤖 check for merge conflicts — optimizer Block merge-conflict markers ⏭️
🤖 check for case conflicts — optimizer Block case-only name clashes ⏭️
🤖 pyupgrade — optimizer Modernize Python syntax ⏭️
🤖 ruff — optimizer Lint + autofix Python (ruff) ⏭️
🤖 ruff-format — optimizer Format Python code (ruff) ⏭️
🤖 mypy — optimizer Static type check ⏭️
📓 nbstripout — optimizer notebooks Strip notebook output ⏭️
📝 markdownlint — optimizer Lint Markdown ⏭️
🔤 codespell — optimizer Fix common misspellings ⏭️
📊 radon cc — optimizer Cyclomatic-complexity gate ⏭️
📊 radon raw — optimizer Raw size metrics gate ⏭️
📊 xenon — optimizer Fail on complexity thresholds ⏭️
📊 lizard — optimizer Cyclomatic-complexity gate ⏭️
🧹 vulture — optimizer Find dead code ⏭️
🛡️ trim trailing whitespace — guardrails Strip trailing whitespace ⏭️
🛡️ fix end of files — guardrails Ensure files end in a newline ⏭️
🛡️ ruff — guardrails Lint + autofix Python (ruff) ⏭️
🛡️ ruff-format — guardrails Format Python code (ruff) ⏭️
🛡️ mypy — guardrails Static type check ⏭️
⚓ helm-docs Regenerate Helm chart README ⏭️
block non-public FE plugins Block non-public FE plugins ⏭️
☕ spotless — java backend Format Java code ⏭️
🧪 pre-commit wrapper smoke tests Self-test the wrapper scripts ⏭️
🧪 rebaseline script tests Self-test the changelog re-baseline script ⏭️
🌐 eslint — frontend Lint + autofix JS/TS ⏭️
🌐 typecheck — frontend Whole-project tsc type check ⏭️
📘 eslint — typescript sdk Lint + autofix JS/TS ⏭️
📘 typecheck — typescript sdk Whole-project tsc type check ⏭️
⚙️ actionlint — github workflows Lint GitHub Actions workflows ⏭️
🐳 hadolint — dockerfiles Lint Dockerfiles ⏭️
🌈 zizmor — github workflows security Security-scan GitHub Actions workflows ⏭️

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Python SDK E2E Tests Results (Python 3.10)

296 tests   289 ✅  4m 56s ⏱️
  1 suites    7 💤
  1 files      0 ❌

Results for commit 2461209.

♻️ This comment has been updated with latest results.

@andriidudar
andriidudar marked this pull request as ready for review August 19, 2026 16:32
@andriidudar
andriidudar requested a review from a team as a code owner August 19, 2026 16:32
@CometActions

Copy link
Copy Markdown
Collaborator

No test needed here.

This is a schema-only migration: it creates cipx_savings_facts and nothing else. grep -rn cipx_savings_facts at this head returns only the migration itself — no DAO writes it, no resource reads it, and the measurement job that populates it lives in the cost API, outside this repo. There is no request, response or page whose behaviour changes when this merges, so there is nothing for an e2e test to assert yet. Worth revisiting when the read path lands: the quantity / tier / causal_link split (realized vs avoided never summed, and the three-axis presentation the comment describes) is exactly the kind of thing that would ship silently wrong, and the ReplacingMergeTree means any read that forgets FINAL/argMax will double-count after a recompute.

Run

Advisory, from the QA test radar. Nothing here blocks this PR, and anything it proposes is a draft for review.

@BorisTkachenko BorisTkachenko left a comment

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.

Left a comments

last_updated_at
)
PARTITION BY toYYYYMM(day)
ORDER BY (workspace_id, day, user_uuid, recommendation_id, harness, quantity, tier, causal_link);

@BorisTkachenko BorisTkachenko Aug 20, 2026

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.

We should have project_id here to match opik project level concept same as in other tables. Also I don't think you need to put so many columns here, as those end up being the primary key and sorting will be done for all of them, looks like an overhead. Probably we can remove some of these: recommendation_id, harness, quantity, tier, causal_link.
Not really in the context, but the key should be unique, but shouldn't have values like quantity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants