[NA] [BE] fix: recalculate the estimated cost on bulk span update - #8133
[NA] [BE] fix: recalculate the estimated cost on bulk span update#8133aseem-ai wants to merge 1 commit into
Conversation
Single span updates recompute total_estimated_cost through CostService when model, provider or usage change and no manual cost is sent. The bulk path only wrote the cost columns when the client sent total_estimated_cost, so spans corrected through PATCH /v1/private/spans/batch kept a zero or stale cost and under-reported project and experiment spend. Bulk does not read the rows it updates, so the query itself keeps a cost the user set manually, the way isManualCost does on the single span path.
github-actions
Bot
added
java
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
| boolean shouldRecalculateEstimatedCost = spanUpdate.totalEstimatedCost() == null | ||
| && isUpdateCostRecalculationAvailable(spanUpdate); |
There was a problem hiding this comment.
Partial batch updates leave stale costs
The bulk template/binder forwards null fields from partial requests to CostService, so the recalculation branch is skipped and the old cost remains — should we resolve each span’s effective model, provider, and usage as update(...) does before calling it?
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/SpanDAO.java` around lines
3223-3224, fix `newBulkUpdateTemplate` and its bulk-update parameter flow so cost
recalculation does not evaluate a partial `SpanUpdate` directly. Before invoking the
template/binder, resolve each target span’s effective model, provider, and usage using
the same fallback logic as `update(...)` for single-span updates, then pass those
resolved values to `isUpdateCostRecalculationAvailable` and `CostService`. Preserve
explicit `totalEstimatedCost` values and ensure the resolved values are used
consistently for both template rendering and parameter binding.
There was a problem hiding this comment.
That's the behavior on main too — a partial payload leaves the stored cost untouched, and this PR keeps that. A per-span merge doesn't fit the current bulk design: one statement applies one update to all ids, so a merge would need a per-span cost value. Single-span update only merges because it already loads the row for the manual-cost check. This covers the issue's case, a batch carrying model+usage together. I can open a follow-up issue for a per-span merge if that's wanted.
There was a problem hiding this comment.
Thanks for clarifying the bulk-update constraint and intended scope. I’ll save this to memory once the PR is merged.
| if (shouldRecalculateEstimatedCost) { | ||
| // Bulk doesn't load the rows it updates, so a cost the user set manually is kept by the | ||
| // query itself, the way isManualCost keeps it on the single span path. | ||
| template.add("recalculated_cost", "recalculated_cost"); |
There was a problem hiding this comment.
Multi-trace cost updates stay unaggregated
The bulk-update branch recalculates costs across spans with different persisted trace_ids, but SpanService builds SpansUpdated with only batchUpdate.update().traceId(), so finished-experiment aggregation skips the other traces and leaves denormalized aggregates stale — should we include all affected persisted trace IDs or span IDs in the event?
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/SpanDAO.java` around lines
3229-3232 and 3280-3284, fix the bulk cost-recalculation flow so it reports every span
actually updated, including spans whose persisted `trace_id` differs from the payload.
Refactor the bulk update DAO/service path to return or propagate all affected persisted
trace IDs (or span IDs), and update `SpanService` to construct `SpansUpdated` from that
complete set so experiment aggregations are refreshed for every changed trace.
There was a problem hiding this comment.
That's pre-existing — main posts the same single-traceId event for every batch update, including ones with an explicit cost, and the bulk SQL never rewrites trace_id. This PR doesn't touch the event path. I can open a separate issue for propagating the persisted trace ids.
There was a problem hiding this comment.
Thanks for clarifying—the event-path issue is pre-existing and outside this PR's scope. I'll remember this context after the PR is merged.
| var updatedSpan = spanResourceClient.getById(id, TEST_WORKSPACE, API_KEY); | ||
| assertThat(updatedSpan.totalEstimatedCost()) | ||
| .isEqualByComparingTo(CostService.calculateCost(MODEL, PROVIDER, USAGE, null)); |
There was a problem hiding this comment.
Production cost regressions are masked
The recalculated-cost test derives its expected value with CostService.calculateCost(...), the same production helper used by bindBulkUpdateParams, so a pricing regression can make both sides agree on the wrong value and still pass. Could we assert an independently derived literal cost for the fixed MODEL, PROVIDER, and USAGE fixture, or check the known price components directly?
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/priv/SpansBatchUpdateResourceTest.java`
around lines 427-429, update
`batchUpdate__whenModelBecomesPriceable__thenRecalculateCost` so its expected cost does
not call `CostService.calculateCost`, which is also used by production code. Replace it
with the independently derived literal cost for the fixed `MODEL`, `PROVIDER`, and
`USAGE` fixture, or assert the known pricing components directly; remove the now-unused
`CostService` import if applicable.
There was a problem hiding this comment.
This follows the existing convention — the single-span cost tests in SpansResourceTest derive the expected value with CostService.calculateCost the same way. A hardcoded literal would break each time the price table updates. The version assertion independently checks the estimated-vs-manual distinction.
There was a problem hiding this comment.
Thanks for the context—using CostService.calculateCost here is intentional and consistent with the existing tests. I’ll save this to memory once the PR is merged.
1 participant
Details
PATCH /v1/private/spans/batchonly wrote the cost columns when the client senttotal_estimated_cost. The single-span update recomputes it throughCostServicewhenmodel,providerorusagechange and no manual cost is given, so a span first written without a priceable model kept a zero or stale cost after a batch correction and under-reported project and experiment spend. This mirrors that branch intonewBulkUpdateTemplate/bindBulkUpdateParams.total_estimated_cost > 0 AND total_estimated_cost_version = '') rather than going throughisManualCost.total_estimated_costin the batch payload still wins and is still stored as manual (blank version).Change checklist
Issues
AI-WATERMARK
AI-WATERMARK: [no]
Testing
Run from
apps/opik-backendon JDK 25, local Docker Testcontainers.mvn -B test -Dtest=SpansBatchUpdateResourceTest-> 9 tests, 0 failures.mvn -B test -Dtest="SpansResourceTest,SpansBatchUpdateResourceTest,SpanServiceImplTest,FindSpansResourceTest,SpansLocalV2TableTest"-> 1878 tests, 0 failures.mvn spotless:check-> clean.Scenarios, all in a new
BatchUpdateCostnested class:mainwithExpecting actual not to be null.The existing
batchUpdate__updateAllFields__successcovers an explicittotal_estimated_costin the batch payload and still passes.Not run: the full backend suite. CI splits it into 16 jobs; I ran every span test class instead. No screen recording.
Documentation
None needed. No API shape change.