Skip to content
Merged
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
f377865
[OPIK-7172] [BE][FE][SDK] feat: re-expose custom-code metric in Optim…
awkoy Jul 13, 2026
4660dd7
[OPIK-7172] fix: CI — ruff-format optimizer + updateById test errorInfo
awkoy Jul 14, 2026
2b5139c
[OPIK-7172] fix: surface error status when pinned opik SDK lacks erro…
awkoy Jul 14, 2026
e345ea7
[OPIK-7172] fix: rename optimizations error_info migration 000102 -> …
awkoy Jul 14, 2026
55b4d4e
[OPIK-7172] fix: address review — static build validation, strict-sig…
awkoy Jul 14, 2026
82aa3e8
Merge remote-tracking branch 'origin/main' into awkoy/opik-7172-custo…
awkoy Jul 14, 2026
b53b543
Merge origin/main into awkoy/opik-7172-custom-code-metric
awkoy Jul 15, 2026
6da7b7e
[OPIK-7172] refactor: optimization error_info as structured ErrorInfo
awkoy Jul 15, 2026
7ab1d71
[OPIK-7172] fix: e2e reads structured error_info as dict (pinned SDK)
awkoy Jul 15, 2026
54b49ec
[OPIK-7172] fix: address review — blank error message + validate Erro…
awkoy Jul 15, 2026
1097c7a
[OPIK-7172] [FE] style: prettier-wrap errorMessage useMemo in RunErro…
awkoy Jul 15, 2026
d9dfc01
Merge remote-tracking branch 'origin/main' into awkoy/opik-7172-custo…
awkoy Jul 15, 2026
f497772
[OPIK-7172] address alexkuzmik review: metric-name idioms, aliased Ba…
awkoy Jul 15, 2026
1381d3d
[OPIK-7172] self-review fixes: submit-gate accuracy + consistent clas…
awkoy Jul 15, 2026
26eb689
[OPIK-7172] fix: rename error_info migration 000103 -> 000105 (main m…
awkoy Jul 16, 2026
2f8a8fa
[OPIK-7172] address Baz review: base-ctor name scan, **kwargs require…
awkoy Jul 16, 2026
3ad607f
[OPIK-7172] fix: FE metric-name extractor mirrors backend AST (object…
awkoy Jul 16, 2026
0168155
Merge remote-tracking branch 'origin/main' into awkoy/opik-7172-custo…
awkoy Jul 16, 2026
e6fcf35
[OPIK-7172] address review: cascade errorInfo validation, preserve on…
awkoy Jul 16, 2026
7c8841d
[OPIK-7172] test: cover error_info preservation on SDK re-upsert
awkoy Jul 16, 2026
ca9a338
[OPIK-7172] address baz re-review: multi-class name extractor, malfor…
awkoy Jul 16, 2026
92c8ac2
[OPIK-7172] fix: FE metric-name extractor resolves the same class the…
awkoy Jul 17, 2026
08c31cc
fix: renumber optimizations error_info migration to 000107 (prefix co…
Jul 17, 2026
1ae3e62
[OPIK-7172] address review: multiline alias imports, PEP 695 headers,…
Jul 17, 2026
64a6823
Merge origin/main into awkoy/opik-7172-custom-code-metric
awkoy Jul 17, 2026
a93e979
[OPIK-7172] fix: metric-name extractor captures balanced constructor …
awkoy Jul 20, 2026
031df35
[OPIK-7172] review: narrow mapToDto JSON catch to UncheckedIOException
awkoy Jul 20, 2026
94b3cbc
[OPIK-7172] fix: only inject output into code metrics that accept it
awkoy Jul 20, 2026
2be66a6
[OPIK-7172] docs: correct arguments-map docstring to match code
awkoy Jul 20, 2026
fa18af0
[OPIK-7172] fix: keep traceback tail (not head) when truncating error…
awkoy Jul 20, 2026
b8b2ca8
[OPIK-7172] fix: make update_status error_info/metadata keyword-only
awkoy Jul 20, 2026
a29c967
[OPIK-7172] test: cover error_info send / suppression / truncation
awkoy Jul 20, 2026
0351daf
[OPIK-7172] fix: align build-time metric-class selection with runtime
awkoy Jul 20, 2026
5c05baa
[OPIK-7172] fix: FE metric-class resolver follows indirect subclass c…
awkoy Jul 20, 2026
6127284
[OPIK-7172] review: read real signature in imported-base fallback + b…
awkoy Jul 20, 2026
280f012
[OPIK-7172] review: restrict metric-class selection to top-level classes
awkoy Jul 21, 2026
a865ef5
[OPIK-7172] chore: bump error_info migration 000107 -> 000108 (prefix…
awkoy Jul 21, 2026
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ public record Optimization(
Optimization.View.Write.class}) JsonNode metadata,
@JsonView({Optimization.View.Public.class,
Optimization.View.Write.class}) @Valid OptimizationStudioConfig studioConfig,
@JsonView({Optimization.View.Public.class,
Optimization.View.Write.class}) @Valid @Schema(description = "Failure reason persisted when the optimization ends in error") ErrorInfo errorInfo,
Comment thread
thiagohora marked this conversation as resolved.
@JsonView({Optimization.View.Public.class}) @Schema(accessMode = Schema.AccessMode.READ_ONLY) UUID datasetId,
@JsonView({Optimization.View.Public.class}) @Schema(accessMode = Schema.AccessMode.READ_ONLY) Long numTrials,
@JsonView({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,11 @@
import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
import com.fasterxml.jackson.databind.PropertyNamingStrategies;
import com.fasterxml.jackson.databind.annotation.JsonNaming;
import jakarta.validation.Valid;
import lombok.Builder;

@Builder(toBuilder = true)
@JsonIgnoreProperties(ignoreUnknown = true)
@JsonNaming(PropertyNamingStrategies.SnakeCaseStrategy.class)
public record OptimizationUpdate(String name, OptimizationStatus status) {
public record OptimizationUpdate(String name, OptimizationStatus status, @Valid ErrorInfo errorInfo) {

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.

[medium] @Valid on errorInfo never cascades — update endpoint param is not @Valid

Adding @Valid to OptimizationUpdate.errorInfo is intended to enforce ErrorInfo's nested @notblank exceptionType / @notblank traceback constraints. But Bean Validation only cascades from a root that is itself validated, and the only endpoint that consumes this DTO — OptimizationsResource.updateOptimizationsById (apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/priv/OptimizationsResource.java:202) — declares the body as @NotNull OptimizationUpdate request without @Valid. As a result the @Valid added here is a no-op: an update carrying an ErrorInfo with a blank/whitespace exceptionType or traceback is accepted and persisted (OptimizationDAO.UPDATE_BY_ID writes error_info as-is) instead of being rejected with HTTP 400. The change does not achieve its purpose unless validation is actually activated at the request boundary. Contrast the create/upsert endpoints (OptimizationsResource.java:75,153) which correctly use @NotNull @Valid Optimization.

💡 Add @Valid to the request parameter of updateOptimizationsById (@NotNull @Valid OptimizationUpdate request) so the nested ErrorInfo constraints are enforced, and add a negative-path test that a blank exceptionType/traceback returns 400.

rule: backend-null-safety-guards-add-bean-validation-constraints-on-api-request-fields-query-

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in e6fcf35. Added @Valid to the updateOptimizationsById request param (@NotNull @Valid OptimizationUpdate request), so the @Valid on OptimizationUpdate.errorInfo now actually activates and a blank exception_type/traceback is rejected with 400. Matches the create/upsert endpoints.

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.

Commit e6fcf35 addressed this comment by adding @Valid to updateOptimizationsById’s OptimizationUpdate request parameter, so bean validation now cascades into errorInfo. It also added test coverage for an additional update shape, reinforcing the validated update path.

}
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ public Response deleteOptimizationsById(
@ApiResponse(responseCode = "204", description = "No content")})
@RequiredPermissions(WorkspaceUserPermission.OPTIMIZATION_STUDIO_USE)
public Response updateOptimizationsById(@PathParam("id") UUID id,
@RequestBody(content = @Content(schema = @Schema(implementation = OptimizationUpdate.class))) @NotNull OptimizationUpdate request) {
@RequestBody(content = @Content(schema = @Schema(implementation = OptimizationUpdate.class))) @NotNull @Valid OptimizationUpdate request) {
log.info("Update optimization with id '{}', with request '{}'", id, request);

optimizationService.update(id, request)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package com.comet.opik.domain;

import com.comet.opik.api.DatasetLastOptimizationCreated;
import com.comet.opik.api.ErrorInfo;
import com.comet.opik.api.Optimization;
import com.comet.opik.api.OptimizationStatus;
import com.comet.opik.api.OptimizationStudioConfig;
Expand Down Expand Up @@ -37,6 +38,7 @@
import java.util.Set;
import java.util.UUID;

import static com.comet.opik.api.ErrorInfo.ERROR_INFO_TYPE;
import static com.comet.opik.domain.AsyncContextUtils.bindUserNameAndWorkspaceContextToStream;
import static com.comet.opik.domain.AsyncContextUtils.bindWorkspaceIdToFlux;
import static com.comet.opik.domain.ExperimentDAO.getFeedbackScores;
Expand Down Expand Up @@ -235,6 +237,7 @@ INSERT INTO optimizations (
status,
metadata,
studio_config,
error_info,
created_by,
last_updated_by,
last_updated_at
Expand All @@ -249,6 +252,7 @@ INSERT INTO optimizations (
:status,
:metadata,
:studio_config,
:error_info,
:created_by,
:last_updated_by,
COALESCE(parseDateTime64BestEffortOrNull(:last_updated_at, 6), now64(6))
Expand Down Expand Up @@ -557,7 +561,7 @@ ORDER BY (workspace_id, dataset_id, id) DESC, last_updated_at DESC

private static final String UPDATE_BY_ID = """
INSERT INTO optimizations (
id, dataset_id, name, workspace_id, project_id, objective_name, status, metadata, created_at, created_by, last_updated_by, studio_config
id, dataset_id, name, workspace_id, project_id, objective_name, status, metadata, created_at, created_by, last_updated_by, studio_config, error_info
)
Comment on lines 621 to 623

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.

Missing migration breaks optimization updates

The new error_info column is now always included in the INSERT INTO optimizations (...) statements behind PUT /optimizations/{id}, upsert(), and the dataset-deleted reinsert path, so ClickHouse rejects those writes before 000102_add_error_info_to_optimizations.sql lands — should we keep the old column list until the migration is guaranteed to deploy first?

Severity web_search

Want Baz to fix this for you? Activate Fixer

Other fix methods

Fix in Cursor

Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
apps/opik-backend/src/main/java/com/comet/opik/domain/OptimizationDAO.java around lines
560-583, in the `UPDATE_BY_ID` SQL template used by
`update()`/`OptimizationDAO.update()`, the INSERT column list now always includes
`error_info`, which will hard-fail on older ClickHouse instances where
`000102_add_error_info_to_optimizations.sql` hasn’t run. Refactor by removing
`error_info` from the `UPDATE_BY_ID` (and also from the
`SET_DATASET_DELETED_TO_TRUE_BY_DATASET_ID` template) until you can guarantee the column
exists; then adjust `buildUpdateTemplate()` and `createUpdateStatement()` so they only
add/bind `error_info` when a new one-time boolean (e.g., `canWriteErrorInfo`) determined
at startup via `system.columns` confirms the column is present. Apply the same gating to
`upsert()` (the UPSERT statement/bindings) and `mapToDto()` (`row.get("error_info"...)`)
so reads/writes don’t break pre-migration.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not applicable to how opik deploys. Migrations from the same image run before the server starts serving: entrypoint.shrun_db_migrations.sh runs db migrate + dbAnalytics migrate with set -e, so a pod that fails migration never serves. During a rolling deploy the old replicas keep running the old SQL, which does not reference error_info at all — only the new binary (whose migration has already applied ADD COLUMN IF NOT EXISTS) includes it in the column list. This is the same pattern as every prior column-add shipped together with its DAO change (e.g. 000105 id_at + SpanDAO in the same release).

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.

Thanks, that makes sense given your migration-before-serve rollout model and existing pattern for column adds. I’ll save this to memory once the PR is merged.

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.

Commit 280f012 addressed this comment by relying on the existing migration-before-serve deployment guarantee. New instances apply migrations successfully before serving, while old replicas continue using the old SQL during rolling deploys.

SELECT
id,
Expand All @@ -571,7 +575,8 @@ INSERT INTO optimizations (
created_at,
created_by,
:user_name as last_updated_by,
studio_config
studio_config,
<if(error_info)> :error_info <else> error_info <endif> as error_info

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.

[low] No path clears error_info via update once it is set

UPDATE_BY_ID only overwrites error_info when :error_info is supplied (' error_info' otherwise). There is no branch that can reset error_info to '' on a status-only update. If an optimization is updated to a non-error/terminal status without an errorInfo in the payload, a previously-persisted failure reason is retained, leaving a stale error surfaced alongside a non-error status. Confirm the lifecycle never recovers from an error state (in which case this is intended); otherwise a status transition out of error should clear error_info.

💡 If clearing is a real requirement, allow the update to explicitly reset error_info (e.g., treat an empty/blank errorInfo as an intent to clear) or clear it whenever status transitions to a non-error terminal value.

rule: keep-null-epoch-sentinel-presence-guards-consistent-across-read-and-write

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed intended — leaving as-is. An optimization that ends in error is terminal in this lifecycle: the Studio worker/cancel paths only ever transition into an error state and never back out to a running/success status, so there is no valid transition that should clear error_info. UPDATE_BY_ID's <if(error_info)> guard preserves the reason across unrelated status-only patches, which is the desired behavior. If a recover-from-error flow is added later, we'd add an explicit clear (treat blank errorInfo as intent-to-clear) at that point.

FROM optimizations
WHERE id = :id
AND workspace_id = :workspace_id
Expand All @@ -582,7 +587,7 @@ INSERT INTO optimizations (

private static final String SET_DATASET_DELETED_TO_TRUE_BY_DATASET_ID = """
INSERT INTO optimizations (
id, dataset_id, name, workspace_id, project_id, objective_name, status, metadata, created_at, created_by, last_updated_at, last_updated_by, dataset_deleted, studio_config
id, dataset_id, name, workspace_id, project_id, objective_name, status, metadata, created_at, created_by, last_updated_at, last_updated_by, dataset_deleted, studio_config, error_info
)
SELECT
id,
Expand All @@ -598,7 +603,8 @@ INSERT INTO optimizations (
last_updated_at,
last_updated_by,
true as dataset_deleted,
studio_config
studio_config,
error_info
FROM optimizations
WHERE workspace_id = :workspace_id
AND dataset_id IN :dataset_ids
Expand Down Expand Up @@ -884,7 +890,9 @@ private Publisher<? extends Result> upsert(Optimization optimization, Connection
.bind("project_id", optimization.projectId() != null ? optimization.projectId().toString() : "")
.bind("objective_name", optimization.objectiveName())
.bind("status", optimization.status().getValue())
.bind("metadata", getStringOrDefault(optimization.metadata()));
.bind("metadata", getStringOrDefault(optimization.metadata()))
.bind("error_info",
optimization.errorInfo() != null ? JsonUtils.writeValueAsString(optimization.errorInfo()) : "");
Comment on lines +952 to +953

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.

Upsert clears prior error reason

optimizationService.upsert() writes error_info as "" when optimization.errorInfo() is null, so PUT /v1/private/optimizations clears any previously saved error_info on sparse updates — should we mirror the sparse update path and leave it unchanged when the field is omitted?

Severity

Want Baz to fix this for you? Activate Fixer

Other fix methods

Fix in Cursor

Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
apps/opik-backend/src/main/java/com/comet/opik/domain/OptimizationDAO.java around lines
883-895 inside the `upsert(Optimization optimization, Connection connection)` method,
the UPSERT statement always binds `error_info` with either serialized JSON or an empty
string when `optimization.errorInfo()` is null. This causes updates to overwrite
previously stored `error_info` even when the API caller omits the field (null),
effectively erasing it. Refactor this binding to mirror the sparse update behavior in
`createUpdateStatement(...)`: when `errorInfo` is null/absent, do not overwrite the
column (bind as null or skip the bind if the prepared statement/SQL supports it), and
only serialize/bind `error_info` when a non-null value is provided. Ensure the
corresponding UPSERT SQL uses the bound parameter in a way that preserves existing
`error_info` when it’s not supplied.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

upsert() is the create path (POST/PUT of a full optimization); at creation error_info is legitimately empty. The failure reason is written only via the sparse update_optimizations_by_id (UPDATE_BY_ID), which preserves error_info via <if(error_info)> :error_info <else> error_info <endif> and never sends a blank one (status_manager guards on a non-empty dict). The studio flow never re-upserts an already-errored optimization, so no prior reason is cleared in practice. A read-before-write preserve in upsert (for arbitrary external full-PUTs) would be a broader change, out of scope here.

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.

Thanks, that makes sense. I'll save this to memory once the PR is merged.

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.

Commit e6fcf35 addressed this comment by preserving the existing errorInfo during update merges when the incoming value is null. That prevents the subsequent upsert from binding error_info as an empty string and wiping the previously stored failure reason on sparse updates.

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.

[medium] upsert blanket-nulls error_info to '' on re-upsert of an existing optimization

UPSERT now writes error_info from optimization.errorInfo() (or '' when null) and, unlike UPDATE_BY_ID, does a full-row replace. OptimizationService.upsert (OptimizationService.java lines 189-198) re-reads the existing row and explicitly preserves studioConfig and name for an existing optimization, but does NOT preserve errorInfo. The failure reason is normally persisted through the separate update()/PATCH path (see OptimizationUpdate.errorInfo and the UPDATE_BY_ID '<if(error_info)> :error_info error_info ' preservation). So when an optimization that has already recorded a failure (error_info populated) is re-upserted by the SDK with a null errorInfo, this bind overwrites the persisted failure reason with '' and it is lost. This is the same provenance-parity gap the studioConfig preservation branch was added to avoid.

💡 Preserve errorInfo on re-upsert the same way studioConfig is preserved: in OptimizationService.upsert, when optimization.errorInfo() == null and existing.errorInfo() != null, copy the existing value into the builder (or have the DAO fall back to the existing column when the bind value is blank).

rule: preserve-partial-update-semantics-merge-dont-replace

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in e6fcf35. OptimizationService.upsert now preserves the persisted failure reason the same way studioConfig is preserved: when optimization.errorInfo() == null && existing.errorInfo() != null, the existing value is copied into the builder before the full-row replace. So a re-upsert with a null errorInfo no longer clobbers a previously recorded failure.

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.

Commit e6fcf35 addressed this comment by preserving errorInfo during upsert when the incoming optimization has a null value but the existing row already has one. This matches the existing studioConfig preservation logic and prevents re-upserts from blanking a previously recorded failure reason.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up: added an integration test errorInfoPreservedOnReUpsertWithNullErrorInfo (commit 7c8841d) that records a failure via the update path, re-upserts with a null errorInfo, and asserts the persisted reason survives the full-row replace. Verified green locally against testcontainers.


if (optimization.studioConfig() != null) {
try {
Expand Down Expand Up @@ -933,6 +941,17 @@ private Publisher<Optimization> mapToDto(Result result) {
}
}

ErrorInfo errorInfo = null;
String errorInfoJson = row.get("error_info", String.class);
if (StringUtils.isNotBlank(errorInfoJson)) {
try {
errorInfo = JsonUtils.readValue(errorInfoJson, ERROR_INFO_TYPE);
} catch (Exception e) {
Comment thread
awkoy marked this conversation as resolved.
Outdated
log.error("Failed to deserialize error_info for optimization: '{}'",
row.get("id", UUID.class), e);
}
Comment on lines +1002 to +1010

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.

Duplicated error_info row mapper

error_info deserialization repeats the JSON→ErrorInfo mapping already in TraceDAO and SpanDAO, so any persisted-shape or error-handling change now has to be updated in three places — can we extract a shared readErrorInfo helper for the DAO layer?

Severity

Want Baz to fix this for you? Activate Fixer

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point on duplication, but leaving as-is for this PR. The error_info block deliberately mirrors the studio_config guard a few lines above it in the same mapper (local consistency), and that one isn't shared either. Extracting a shared readErrorInfo helper across TraceDAO/SpanDAO/OptimizationDAO is a reasonable cleanup but a cross-cutting refactor beyond this feature's scope — better as a dedicated follow-up so it can touch all three call sites and their tests together.

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.

Thanks, that makes sense — keeping local consistency here and deferring the cross-cutting deduplication to a follow-up is a reasonable boundary. I'll save this to memory once the PR is merged.

}

String projectIdStr = row.get("project_id", String.class);
UUID projectId = StringUtils.isNotBlank(projectIdStr) ? UUID.fromString(projectIdStr) : null;

Expand All @@ -945,6 +964,7 @@ private Publisher<Optimization> mapToDto(Result result) {
.status(OptimizationStatus.fromString(row.get("status", String.class)))
.metadata(getJsonNodeOrDefault(row.get("metadata", String.class)))
.studioConfig(studioConfig)
.errorInfo(errorInfo)
.createdAt(row.get("created_at", Instant.class))
.lastUpdatedAt(row.get("last_updated_at", Instant.class))
.createdBy(row.get("created_by", String.class))
Expand Down Expand Up @@ -999,6 +1019,9 @@ private ST buildUpdateTemplate(OptimizationUpdate update) {
Optional.ofNullable(update.status())
.ifPresent(status -> template.add("status", status.getValue()));

Optional.ofNullable(update.errorInfo())
.ifPresent(errorInfo -> template.add("error_info", errorInfo));

return template;
}

Expand All @@ -1011,6 +1034,9 @@ private Statement createUpdateStatement(UUID id, OptimizationUpdate update, Conn
Optional.ofNullable(update.status())
.ifPresent(status -> statement.bind("status", status.getValue()));

Optional.ofNullable(update.errorInfo())
.ifPresent(errorInfo -> statement.bind("error_info", JsonUtils.writeValueAsString(errorInfo)));

statement.bind("id", id);

return statement;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,15 @@ public Mono<UUID> upsert(@NonNull Optimization optimization) {
builder.studioConfig(existing.studioConfig());
}

// Preserve the persisted failure reason if not provided in update
// (upsert does a full-row replace; the SDK re-upserts with a null
// errorInfo and would otherwise clobber a previously recorded failure).
// errorInfo is normally set through the PATCH/update path.
if (optimization.errorInfo() == null
&& existing.errorInfo() != null) {
builder.errorInfo(existing.errorInfo());
Comment on lines +219 to +225

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.

existing.errorInfo() is copied forward whenever an upsert omits optimization.errorInfo(), so a row that transitions out of ERROR keeps the stale failure payload and OptimizationDAO.mapToDto() keeps rehydrating it into getById()/find() — should we only preserve existing.errorInfo() when the incoming status remains ERROR, and let all other updates (including null) overwrite it?

Severity

Want Baz to fix this for you? Activate Fixer

Other fix methods

Fix in Cursor

Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
apps/opik-backend/src/main/java/com/comet/opik/domain/OptimizationService.java around
lines 200-206 inside the `upsert(Optimization optimization)` method, refactor the
`builder.errorInfo(...)` logic so that: 1. `existing.errorInfo()` is only carried
forward when the optimization remains in an error state (i.e., both existing and
incoming status indicate ERROR). 2. For all other updates — including normal PUT calls
where `optimization.errorInfo()` deserializes to null — the incoming value (including
null) is used directly, allowing clients to clear a prior failure reason. 3. The
clearing/overwrite happens before `builder.build()` so `OptimizationDAO.mapToDto()`
rehydrates the correct value on subsequent `getById()`/`find()` calls. Add/adjust tests
to verify that a successful PUT can clear `errorInfo`, while the explicit
error-finalization path still preserves it as intended.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keeping the unconditional preserve — here's the reasoning. This is exactly the behavior the human review (thiagohora) explicitly requested for this finding, and it mirrors the existing UPDATE_BY_ID semantics, which already preserve error_info whenever it isn't supplied (<if(error_info)> :error_info <else> error_info). OptimizationStatus.ERROR is a terminal state (ERROR("error", true)), and neither the Studio worker nor the SDK transitions an optimization back out of a terminal status, so the "row transitions out of ERROR" case isn't part of the lifecycle. Making upsert conditionally clear on status would diverge from the update path and reintroduce the clobber thiagohora asked us to fix. If a recover-from-error flow is ever added, we'd add an explicit clear (treat blank errorInfo as intent-to-clear) at that point — consistent with the reply on the related error_info clearing thread.

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.

Thanks, that makes sense — preserving errorInfo unconditionally here matches the existing update semantics and current terminal-state lifecycle. I'll save this to memory once the PR is merged.

}

// Preserve original name only if incoming name is blank
// (SDK sends blank name, but explicit updates should be honored)
if (StringUtils.isBlank(optimization.name())) {
Expand Down Expand Up @@ -310,7 +319,7 @@ public Flux<DatasetLastOptimizationCreated> getMostRecentCreatedOptimizationFrom

@Override
public Mono<Long> update(@NonNull UUID id, @NonNull OptimizationUpdate update) {
if (update.name() == null && update.status() == null) {
if (update.name() == null && update.status() == null && update.errorInfo() == null) {

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.

[low] New errorInfo-only update branch is untested

This diff loosens the early-return guard so an update carrying ONLY errorInfo (name == null && status == null && errorInfo != null) now flows through to optimizationDAO.update and persists error_info, whereas before it short-circuited to Mono.empty() and was silently dropped. This is the entire behavior the PR enables, but the parameterized test OptimizationsResourceTest#updateById (updateById() MethodSource, lines 886-891) never exercises it: the three cases are a full pojo, name=null (status+errorInfo still set), and status=null (name+errorInfo still set) — none isolates the errorInfo-only branch. The complementary no-op case (all three null -> Mono.empty(), 204/no change) is also uncovered. The changed behavior branch ships without a dedicated test.

💡 Add an argument to the updateById() MethodSource for an errorInfo-only update, e.g. arguments(podamFactory.manufacturePojo(OptimizationUpdate.class).toBuilder().name(null).status(null).build()), and assert the persisted optimization reflects the new errorInfo while name/status are preserved. Optionally add a case with all fields null to assert the no-op path.

general finding

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in e6fcf35. Added an errorInfo-only argument to the updateById() MethodSource (...toBuilder().name(null).status(null).build()), which exercises the loosened guard branch (name == null && status == null && errorInfo != null) that now flows through to optimizationDAO.update and persists error_info, and asserts name/status are preserved.

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.

Commit e6fcf35 addressed this comment by adding an explicit errorInfo-only update case to OptimizationsResourceTest#updateById, so the newly enabled branch is now exercised. The optional all-null no-op case was not added, but the core missing branch coverage was fixed.

return Mono.empty();
}
Comment on lines 339 to 344

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.

Error info lacks test coverage

OptimizationDAO/service now propagate errorInfo, but we don't have a regression test proving a non-null value round-trips through save/update and back, so future changes could drop the failure-reason field or break the documented no-op guard — should we add a targeted unit or integration test for that path?

Severity

Want Baz to fix this for you? Activate Fixer

Other fix methods

Fix in Cursor

Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
apps/opik-backend/src/main/java/com/comet/opik/domain/OptimizationService.java around
lines 311-315 in the `update(UUID id, OptimizationUpdate update)` method, the guard now
only short-circuits when `update.name()`, `update.status()`, and `update.errorInfo()`
are all null, but there is no test coverage proving `errorInfo` is persisted and
returned. Add a targeted unit or integration test that creates/saves an Optimization
with a non-null `errorInfo`, calls `update` with a new or same `errorInfo`, then asserts
the returned/loaded Optimization payload includes `errorInfo` (and that when all three
fields are null the method returns Mono.empty / no changes). Prefer using existing
OptimizationService/OptimizationDAO test infrastructure and assertions used for
`name`/`status`, and make sure the test covers both the “errorInfo survives update”
and “no-op when nothing changes” behaviors.

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.

Commit 4660dd7 addressed this comment by extending the optimization update test to carry errorInfo through the update path and assert it is present in the returned object. The backend model/DAO changes also persist and read the field, so the non-null round-trip concern is covered, though there is no dedicated no-op assertion in the diff.


Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
--liquibase formatted sql
--changeset admin:000106_add_error_info_to_optimizations
--comment: Add error_info column to optimizations table to persist the failure reason surfaced in Optimization Studio

ALTER TABLE ${ANALYTICS_DB_DATABASE_NAME}.optimizations ON CLUSTER '{cluster}'
ADD COLUMN IF NOT EXISTS error_info String DEFAULT '';

--rollback ALTER TABLE ${ANALYTICS_DB_DATABASE_NAME}.optimizations ON CLUSTER '{cluster}' DROP COLUMN IF EXISTS error_info;
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import com.comet.opik.api.Dataset;
import com.comet.opik.api.DatasetItem;
import com.comet.opik.api.DatasetItemBatch;
import com.comet.opik.api.ErrorInfo;
import com.comet.opik.api.Experiment;
import com.comet.opik.api.ExperimentItem;
import com.comet.opik.api.ExperimentScore;
Expand Down Expand Up @@ -875,6 +876,7 @@ void updateById(OptimizationUpdate update) {
optimization = optimization.toBuilder().id(id)
.name(update.name() != null ? update.name() : optimization.name())
.status(update.status() != null ? update.status() : optimization.status())
.errorInfo(update.errorInfo() != null ? update.errorInfo() : optimization.errorInfo())
.build();

var actualOptimization = optimizationResourceClient.get(id, API_KEY, TEST_WORKSPACE_NAME, 200);
Expand All @@ -886,7 +888,11 @@ private Stream<Arguments> updateById() {
return Stream.of(
arguments(podamFactory.manufacturePojo(OptimizationUpdate.class)),
arguments(podamFactory.manufacturePojo(OptimizationUpdate.class).toBuilder().name(null).build()),
arguments(podamFactory.manufacturePojo(OptimizationUpdate.class).toBuilder().status(null).build()));
arguments(podamFactory.manufacturePojo(OptimizationUpdate.class).toBuilder().status(null).build()),
// errorInfo-only update: exercises the branch that persists a failure reason without
// name/status (previously short-circuited to Mono.empty() before this behavior was added)
arguments(podamFactory.manufacturePojo(OptimizationUpdate.class).toBuilder()
.name(null).status(null).build()));
}

@Nested
Expand Down Expand Up @@ -1466,6 +1472,40 @@ void studioConfigPreservedAfterStatusUpdate() {
assertThat(completedOptimization.studioConfig()).isEqualTo(studioConfig);
}

@Test
@DisplayName("error_info is preserved when SDK re-upserts with a null errorInfo")
void errorInfoPreservedOnReUpsertWithNullErrorInfo() {
// Create a Studio optimization (upsert full-row replace path)
var studioConfig = createStudioConfig();
var optimization = optimizationResourceClient.createPartialOptimization()
.studioConfig(studioConfig)
.errorInfo(null)
.build();
var id = optimizationResourceClient.create(optimization, API_KEY, TEST_WORKSPACE_NAME);

// Record a failure reason through the PATCH/update path (as the worker does)
var errorInfo = podamFactory.manufacturePojo(ErrorInfo.class);
optimizationResourceClient.update(id,
OptimizationUpdate.builder().status(OptimizationStatus.CANCELLED).errorInfo(errorInfo).build(),
API_KEY, TEST_WORKSPACE_NAME, 204);

var afterFailure = optimizationResourceClient.get(id, API_KEY, TEST_WORKSPACE_NAME, 200);
assertThat(afterFailure.errorInfo()).isEqualTo(errorInfo);

// Re-upsert the same optimization with a null errorInfo (SDK behavior): the persisted
// failure reason must survive the full-row replace instead of being clobbered to blank.
optimizationResourceClient.upsert(optimization.toBuilder().id(id).errorInfo(null).build(),
API_KEY, TEST_WORKSPACE_NAME);

var afterReUpsert = optimizationResourceClient.get(id, API_KEY, TEST_WORKSPACE_NAME, 200);
// The re-upsert with a null errorInfo must not drop the persisted failure reason
// nor the sibling studioConfig (both preserved through the upsert path). Status
// ordering across the update/upsert rows is governed by last_updated_at, so it is
// not asserted here.
assertThat(afterReUpsert.errorInfo()).isEqualTo(errorInfo);
Comment on lines +1516 to +1524

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.

Clobber regressions stay hidden

afterReUpsert is only asserted on errorInfo, so a re-upsert that drops other persisted fields like status or studioConfig would still pass — should we assert the fetched row (or at least those fields) to cover the null-preservation path?

Severity

Want Baz to fix this for you? Activate Fixer

Other fix methods

Fix in Cursor

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/OptimizationsResourceTest.java
around lines 1476-1502, in the test `errorInfoPreservedOnReUpsertWithNullErrorInfo`,
strengthen the post-re-upsert assertions. Right now you only assert
`afterReUpsert.errorInfo()`; update the test to also verify that the other persisted
fields that could be clobbered by the full-row replace path (at least `status` and any
previously set `studioConfig` if applicable, plus `id`) still match what was persisted
after the cancel/update step. Prefer asserting key fields on `afterReUpsert` directly
(or compare them against `afterFailure`) so the test fails if the re-upsert resets
unrelated columns when `errorInfo` is null.

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.

Commit ca9a338 addressed this comment by adding an assertion that studioConfig still matches after the re-upsert, which strengthens the null-preservation coverage beyond errorInfo alone. It does not assert status or the full row, but it does verify one of the potentially clobbered persisted fields the comment called out.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in ca9a338. The test now also asserts studioConfig survives the full-row-replace re-upsert. I intentionally did not assert status: the persisted value across the update-row vs upsert-row is decided by last_updated_at ordering (podam sets a random lastUpdatedAt), so a fixed status assertion would be non-deterministic. errorInfo and studioConfig are deterministic (both preserved regardless of which row wins) and are asserted.

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.

Thanks, that makes sense. I’ll save this to memory once the PR is merged.

assertThat(afterReUpsert.studioConfig()).isEqualTo(studioConfig);
}

@Test
@DisplayName("Cancel Studio optimization returns NOT_IMPLEMENTED")
void cancelStudioOptimization__returnsNotImplemented() {
Expand Down
5 changes: 5 additions & 0 deletions apps/opik-frontend/src/constants/optimizations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,11 @@ export const OPTIMIZATION_METRIC_OPTIONS = [
description:
"Compares numeric output against a reference value. Closer values score higher.",
},
{
value: METRIC_TYPE.CODE,
label: "Custom code",
description: "Score outputs with a custom Python metric.",
},
];

export type IconConfig = { icon: LucideIcon; color: string };
Expand Down
Loading
Loading