[OPIK-7852] [BE] Persist cipx call attribution: trigger, trigger_detail, turn_key, parent_tool_use_id - #7937
[OPIK-7852] [BE] Persist cipx call attribution: trigger, trigger_detail, turn_key, parent_tool_use_id#7937aadereiko wants to merge 4 commits into
Conversation
…nt_tool_use_id cipx already ships all four on metadata.cipx.call, but none reached a typed column, so the spend tables cannot tell a subagent's spend from the main agent's, name the agent, or reconstruct the parent/child tree. Migration 000118 adds them in one ALTER, additive with DEFAULT '' so pre-existing rows read as unknown - honest, since they were written before the proxy carried the fields. trigger_detail is the agent NAME when trigger='subagent' and the tool name when trigger='tool_continuation', so consumers must gate on trigger before grouping per agent. Empty is deliberately not defaulted: it means unknown, never a guessed agent name. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…lerance CipxSpendDAO binds by position and nothing checks the order against the INSERT tuple at compile time - the DAO's own comment says a mismatch "silently writes each value into the neighbouring column". Give every column a distinct sentinel and assert each holds its own, with the column name as the assertion description so a regression names the column that received the wrong value. Two rows, not one: the bind index accumulates across rows while workspace_id is bound once at index 0 and its repeats dedup. If that dedup assumption is ever wrong the stride becomes 21 instead of 20 and only the SECOND row is corrupted, which a single-row insert cannot see. The ids are UUIDs because project_id/trace_id/span_id are FixedString(36), so a rotation among those three is silently accepted by ClickHouse. Second test: cipx adds fields to metadata.cipx.call on its own release cadence and ships to laptops independently of this service, so a newer proxy talking to an older backend is the normal state. Unknown fields at every level - on the call, inside usage (inference_geo, the real pending one from OPIK-7757), inside cache_creation, inside config, beside call, on a block, and beside cipx in metadata - must be ignored rather than reject the row. Dropping the row would lose the spend entirely, and spend totals are the one thing already correct. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
java
⏱️ pre-commit per-hook timing
⏭️ 32 skipped (no matching files changed)
|
|
No test needed here. The four new columns are write-only today: nothing in opik SELECTs from cipx_spends (no resource, no endpoint), and the AI Spend / Cost Intelligence entry point in the frontend is the comet Advisory, from the QA test radar. Nothing here blocks this PR, and anything it proposes is a draft for review. Re-checked after a push on 20 Aug 16:50 UTC — nothing the verdict depends on changed. |
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
| // The block writer sees the same metadata, so an unknown field on a block must not drop | ||
| // the blocks either. | ||
| assertThat(getCipxBlocks(span.id(), ws.workspaceId())).isNotEmpty(); |
There was a problem hiding this comment.
Flaky unknown-field ingestion test
The test stops waiting when getCipxSpend(...) returns a row, so asynchronous block ingestion may still be incomplete when it asserts getCipxBlocks(...) is non-empty and intermittently fails — should we await the block-table condition separately or move that assertion into the existing untilAsserted callback?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/events/CostIntelligenceIngestionTest.java`
around lines 263-265, update `unknownFieldsDoNotRejectTheRow` because waiting for the
spend row does not guarantee that asynchronous block ingestion has completed. Add a
separate `await().untilAsserted` around the `getCipxBlocks(span.id(), ws.workspaceId())`
non-empty assertion, or perform that assertion inside the existing callback, so the test
does not intermittently fail due to timing.
There was a problem hiding this comment.
Commit 79b112a addressed this comment by moving the block non-empty assertion inside the existing untilAsserted callback, so it is retried until asynchronous block ingestion completes.
| .trigger(call.path("trigger").asText("")) | ||
| .triggerDetail(call.path("trigger_detail").asText("")) | ||
| .turnKey(call.path("turn_key").asText("")) | ||
| .parentToolUseId(call.path("parent_tool_use_id").asText("")) |
There was a problem hiding this comment.
Malformed attribution silently persisted
metadata.cipx.call accepts any object, then parses its four call fields with asText("") without validation, so malformed values become the empty unknown sentinel or unsupported text and are indistinguishable from genuine unresolved attribution or unusable in downstream spend queries. Should we validate field shapes and trigger against migration 000118’s closed set (user_turn, tool_continuation, subagent, automated, unknown), or normalize malformed values to unknown while preserving the row insert and covering each JSON node kind?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
apps/opik-backend/src/main/java/com/comet/opik/domain/CipxSpendDAO.java around lines
98-101, update SpanRow.from so the attribution fields are not blindly converted with
asText(""). Validate `trigger` against migration 000118’s allowed values (`user_turn`,
`tool_continuation`, `subagent`, `automated`, `unknown`) and require the other
attribution fields to have the expected string shape, normalizing null, numeric, object,
array, and other malformed values to the documented unknown representation without
rejecting the row. Add regression tests covering all relevant JSON node kinds and
unsupported trigger tokens.
There was a problem hiding this comment.
Declining — validating trigger against a closed set would work against what the sibling test in this PR exists to protect.
cipx ships to laptops on its own cadence, independently of this service's deploys, so a newer proxy talking to an older backend is the normal state, not an edge case. trigger is a closed enum as of today; the moment cipx adds a value (a fork trigger is already on the roadmap — OPIK_8026), validating against migration 000118's list would reject or mangle a legitimate value. That is exactly the failure unknownFieldsDoNotRejectTheRow guards against, and dropping the row would lose the spend entirely — spend totals being the one thing that is already correct today.
On the normalization half: asText("") already does it. A null, numeric, object or array node yields "", which is the documented unknown sentinel (see the SpanRow.from comment and the 000118 changeset). So malformed input already lands on "unknown" without rejecting the row.
You're right that this makes malformed values indistinguishable from genuinely unresolved attribution. That's deliberate and stated: "" means unknown, and a guessed agent name would book real spend against an agent that never ran. Distinguishing "the proxy could not resolve this" from "the proxy sent something we could not parse" would need a separate signal on the wire, not a decision made at parse time here.
There was a problem hiding this comment.
Thanks for the clarification—forward compatibility and the intentional unknown sentinel make sense here. I’ll save this context to memory once the PR is merged.
| ADD COLUMN IF NOT EXISTS turn_key String DEFAULT '', | ||
| ADD COLUMN IF NOT EXISTS parent_tool_use_id String DEFAULT ''; | ||
|
|
||
| --rollback ALTER TABLE ${ANALYTICS_DB_DATABASE_NAME}.cipx_spends ON CLUSTER '{cluster}' DROP COLUMN IF EXISTS `trigger`, DROP COLUMN IF EXISTS trigger_detail, DROP COLUMN IF EXISTS turn_key, DROP COLUMN IF EXISTS parent_tool_use_id; |
There was a problem hiding this comment.
Migration violates required final blank line
The migration ends immediately after the --rollback ALTER TABLE ... line, so it lacks the required trailing blank line — should we add one, as .agents/skills/opik-backend/migrations.md requires?
Want Baz to fix this for you? Activate Fixer You can also update your AI coding guidelines based on this comment by apply pr to [branch name]
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`apps/opik-backend/src/main/resources/liquibase/db-app-analytics/migrations/000118_add_trigger_to_cipx_spends.sql`
around line 30, update the migration file formatting so the `--rollback ALTER TABLE ...`
statement is followed by one empty line. Do not change the SQL or rollback behavior;
only add the required trailing blank line per the backend migration guidelines.
There was a problem hiding this comment.
Commit 79b112a addressed this comment by adding a trailing blank line after the rollback statement.
…mpty line Both from baz review feedback on #7937. The unknown-field test awaited the spend row and then asserted the blocks outside the callback. CostIntelligenceIngestionListener subscribes to the spend insert and the block insert independently, so the spend row landing says nothing about whether the blocks have - a real flake. Moved inside the same await. Migration now ends with an empty line, per .agents/skills/opik-backend/ migrations.md ("Always end with empty line"). Note 000117 does not, so the convention is documented but not uniformly followed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| // The block writer sees the same metadata, so an unknown field on a block must not | ||
| // drop the blocks either. Asserted inside the same await: the listener subscribes to | ||
| // the spend insert and the block insert independently | ||
| // (CostIntelligenceIngestionListener), so the spend row landing says nothing about | ||
| // whether the blocks have. |
There was a problem hiding this comment.
Incomplete test rationale
The comment ends with the incomplete phrase “whether the blocks have.”, so the rationale is unfinished — should we complete it as “whether the blocks have landed.”?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/events/CostIntelligenceIngestionTest.java`
around lines 261-265, update the comment in the block-ingestion assertion logic so the
incomplete phrase “whether the blocks have.” reads “whether the blocks have
landed.” Preserve the existing meaning and test behavior.
There was a problem hiding this comment.
Commit b0941b3 addressed this comment by completing the unfinished comment phrase from “whether the blocks have.” to “whether the blocks have landed.”
From baz review feedback on #7937. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| @Test | ||
| @DisplayName("a call carrying no trigger fields ingests with empty attribution columns") | ||
| void spanWithoutTriggerFieldsLandsWithEmptyAttribution() { | ||
| var ws = newWorkspace(); | ||
| String projectName = "cipx-" + UUID.randomUUID(); | ||
|
|
||
| // systemToolsCipxMetadata carries no trigger/turn_key/parent_tool_use_id — the shape | ||
| // every span written before the proxy shipped them has. Ingestion must still land the | ||
| // row and must leave the attribution columns empty rather than substituting a default: | ||
| // "" means unknown, and a guessed agent name would book real spend against an agent | ||
| // that never ran. | ||
| var span = factory.manufacturePojo(Span.class).toBuilder() |
There was a problem hiding this comment.
Legacy-row compatibility remains untested
spanWithoutTriggerFieldsLandsWithEmptyAttribution runs after runClickhouseDbMigration(CLICKHOUSE) has applied 000118, so it tests that the DAO supplies "" rather than that ClickHouse preserves empty values for pre-migration rows, allowing regressions in existing spend rows to pass — should we insert a row before 000118, run that changeset, then read all four columns as strings and assert they remain empty?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/events/CostIntelligenceIngestionTest.java`
around lines 176-187, update `spanWithoutTriggerFieldsLandsWithEmptyAttribution` so it
does not rely on the class-level migration having already applied `000118`; that only
proves the current DAO supplies empty defaults. Arrange for a spend row to be written
before `000118`, apply that changeset, then read the row through the current DAO and
assert `trigger`, `triggerDetail`, `turnKey`, and `parentToolUseId` are all empty
strings, verifying migration compatibility for existing rows.
There was a problem hiding this comment.
Correct that it is untested, and fair to ask given the PR body claims pre-existing rows read as unknown. Declining anyway, on cost/benefit.
What such a test would assert is ClickHouse's own ADD COLUMN … DEFAULT '' semantics — materializing the default for rows written before the column existed. That is not behaviour this repo implements, and there is no code path here that could regress it: the DAO only ever writes new rows, and the migration is a single additive ALTER. A green assertion would be testing the database, not us.
The cost is also not small. CostIntelligenceIngestionTest applies the full migration set at construction (MigrationUtils.runClickhouseDbMigration), so exercising the pre-migration state needs either a second database or custom changeset ordering in the harness. That is a meaningful amount of test infrastructure to protect a property we cannot break.
For what it's worth, the equivalent case on the read side is covered, because there it protects our code rather than ClickHouse's: ai-cost-backend #84 has a test that queries a genuinely column-less table and asserts the endpoint degrades to available=false instead of raising — that one guards a try/except we wrote.
There was a problem hiding this comment.
Thanks, that distinction makes sense—this would test ClickHouse rather than a regressible repository behavior. I’ll save this guidance to memory once the PR is merged.
2 participants
Details
cipx already ships
trigger,trigger_detail,turn_keyandparent_tool_use_idonmetadata.cipx.call, but none reached a typed column — so the spend tables could not tell a sub-agent's spend from the main agent's, name the agent, or reconstruct the parent/child tree. This adds the four columns and the DAO mapping.trigger_detailis overloaded: it carries the agent NAME whentrigger='subagent'and the TOOL name whentrigger='tool_continuation', so consumers must gate ontriggerbefore grouping per agent.000118is additive withDEFAULT '', so pre-existing rows read as unknown, which is honest: they were written before the proxy carried the fields.Change checklist
Issues
Follow-up found while testing this work, not resolved here: OPIK_8026 (a fork's inherited-cache spend is billed to the main agent). Related but not resolved: OPIK_7757 (
inference_geo), whose field name is used as the "unknown field" in one test precisely because it is a real pending addition.AI-WATERMARK
AI-WATERMARK: yes
Testing
mvn test -Dtest='CostIntelligenceIngestionTest'— 12 tests, 0 failures at the time the tests were written.Two tests, both aimed at failure modes that would otherwise be silent:
workspace_idis bound once at index 0 and its repeats dedup, so a wrong stride corrupts only the second row and a single-row insert cannot see it. The ids are UUIDs becauseproject_id/trace_id/span_idareFixedString(36), where a rotation among the three is silently accepted by ClickHouse.End-to-end: migration applied to a local Opik and exercised by real Claude Code sessions through a branch-built proxy. Result: 9 agent invocations, all named and linked,
agents_dispatched=9 linked=9 ambiguous=0, depth-2 nesting intact, and the auto-mode permission classifier correctly bucketed asautomatedrather than counted as user work.Not re-run locally: the last commit moves one assertion inside an existing
await(a strictly narrower timing window). The local Maven surefire environment has since started failing this whole test class withClassNotFoundException: DropwizardAppExtensionProvider— including on test methods untouched by this PR, so it is environmental — and CI is the verification for that commit.Documentation
No user-facing documentation change. The columns are internal analytics storage; the semantics (especially
trigger_detail's overloading and the empty-means-unknown rule) are documented in migration000118's changeset comment and in the DAO's javadoc, which is where the next person to touch the INSERT will look.