Skip to content

Commit 01393c5

Browse files
thiagohoraclaude
andcommitted
[OPIK-8262] [BE] fix: classify provider errors by status and split thread fan-out
Two coupled fixes to online scoring. They ship together because the second only becomes a correctness problem once the first lands. Retry classification. scoreTrace answered every provider failure with a blanket InternalServerErrorException, so a request the provider can never accept -- an oversized body rejected with a plain-text 400 -- was replayed maxRetries times, one delivery per pendingMessageDuration, before being retired. It now classifies by status code rather than by 4xx/5xx family: 400/401/403 and friends become ClientErrorException, which BaseRedisSubscriber.NON_RETRYABLE_EXCEPTIONS retires on the first delivery, while 408/425/429 and every 5xx stay a retryable 500. Family alone is too blunt -- 408/425/429 are "not now", not "not ever". Permanence is decided only from a status the provider actually put on the wire. The provider mappers are not consulted, not even as a fallback: they synthesize a code when they cannot parse the body (CustomLlm 400, OpenAi 500), and nothing downstream can tell a parsed 400 from that default, so trusting them would drop every unparseable CustomLlm failure on its first delivery. Needlessly retrying a permanent error costs maxRetries attempts; dropping an unknown failure loses it forever, so an absent wire status stays retryable. VertexAI was the one provider this could not reach. The Google Cloud SDK raises GAX ApiException, which carries no HttpException and which langchain4j does not model as NonRetriableException, so a permanent Vertex failure was retried in-process and then redelivered by the subscriber. Its status now comes from GAX's own transport-neutral translation, identical for the gRPC and HTTP-JSON transports, and a permanent status additionally fails fast inside the retry action so the in-process attempts are skipped too. An exception GAX itself marks retryable yields no status, so the mapping can only ever prevent a drop, never cause one. Thread fan-out. The two trace-thread scorers took a message carrying a list of thread ids, fanned out, and collapsed all per-thread errors into one arbitrary re-emitted error. That was harmless only while every provider failure was retryable; after the split a fan-out can mix a permanent 400 with a transient 429, and an arbitrary pick either drops retryable work or replays threads that already succeeded. Rather than choose a better victim, remove the collapse: the subscriber acks and removes per stream entry, so OnlineScorePublisher now writes one entry per thread id and failure granularity matches ack granularity. A retry re-runs only the thread that failed, so no sibling is ever replayed. Entries left by the previous build carry several ids. Those are migrated, not scored: the consumer republishes them as N single-id entries and completes, which is what acks the original. A failed republish does not ack, so the entry redelivers and the split retries; a successful republish whose ack fails splits twice, and the duplicate scores are absorbed by feedback_scores being a ReplacingMergeTree. The branch is a shim, deletable once no pre-deploy entry can be in flight. The pipeline lives in OnlineScoringBaseScorer so it cannot drift between the two scorers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 9520bc5 commit 01393c5

12 files changed

Lines changed: 1228 additions & 102 deletions

apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/events/OnlineScoringBaseScorer.java

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
import com.comet.opik.api.FeedbackScoreItem;
44
import com.comet.opik.api.Span;
55
import com.comet.opik.api.Trace;
6+
import com.comet.opik.api.Visibility;
67
import com.comet.opik.api.evaluators.AutomationRuleEvaluatorType;
78
import com.comet.opik.api.events.RedisSubscriberMessage;
89
import com.comet.opik.api.filter.Operator;
@@ -12,6 +13,7 @@
1213
import com.comet.opik.domain.SpanService;
1314
import com.comet.opik.domain.TraceSearchCriteria;
1415
import com.comet.opik.domain.TraceService;
16+
import com.comet.opik.domain.evaluators.OnlineScorePublisher;
1517
import com.comet.opik.infrastructure.OnlineScoringConfig;
1618
import com.comet.opik.infrastructure.OnlineScoringStreamConfigurationAdapter;
1719
import com.comet.opik.infrastructure.auth.RequestContext;
@@ -31,6 +33,7 @@
3133
import java.util.Set;
3234
import java.util.UUID;
3335
import java.util.concurrent.atomic.AtomicReference;
36+
import java.util.function.Function;
3437
import java.util.stream.Collectors;
3538

3639
import static com.comet.opik.api.FeedbackScoreItem.FeedbackScoreBatchItem;
@@ -252,4 +255,44 @@ protected Flux<Trace> retrieveFullThreadContext(@NotNull String threadId,
252255
.defer(() -> retrieveFullThreadContext(threadId, lastReceivedIdRef, projectId)));
253256
}));
254257
}
258+
259+
/**
260+
* Routes a trace-thread entry down one of two branches: an entry carrying several thread ids was
261+
* written by an older build and is <b>migrated</b> — republished as one entry per id, never scored —
262+
* while the single-id entries this build writes are <b>scored</b> normally. Completion therefore does
263+
* not mean "scored", so per-outcome logging belongs inside {@code scoreThread} or in the migrate
264+
* branch, never on the returned Mono.
265+
*
266+
* <p>Migrating rather than scoring avoids needing a rule for reducing N per-thread outcomes into the
267+
* one verdict a stream entry gets, which must mis-serve somebody when a permanent failure and a
268+
* retryable one land together. Each replacement entry gets its own retry budget instead.
269+
*
270+
* <p>The ack is implicit: this returns the republish, so a failure leaves the entry unacked and it
271+
* redelivers. If the republish succeeds but the ack fails, the entry splits twice and some threads are
272+
* scored twice — tolerable because {@code feedback_scores} is a {@code ReplacingMergeTree} versioned on
273+
* {@code last_updated_at}, so the second score overwrites rather than duplicating.
274+
*
275+
* <p>The migrate branch is a temporary shim, deletable once no pre-split entry can be in flight.
276+
*/
277+
protected Mono<Void> migrateOrScoreThreadIds(@NonNull M message, @NonNull List<String> threadIds,
278+
@NonNull OnlineScorePublisher publisher, @NonNull Function<String, M> singleThreadIdCopy,
279+
@NonNull Function<String, Mono<Void>> scoreThread) {
280+
if (threadIds.size() > 1) {
281+
// Logged on success, and worded so it can never be mistaken for a scoring log.
282+
return publisher.enqueueMessage(threadIds.stream().map(singleThreadIdCopy).toList(), type)
283+
.doOnSuccess(unused -> log.info(
284+
"Migrated '{}' legacy thread ids to single-id entries for workspace '{}'; "
285+
+ "no scoring performed for this entry",
286+
threadIds.size(), message.workspaceId()));
287+
}
288+
// @NotEmpty says this cannot happen; if it does, no retry would help, so let it be acked away.
289+
if (threadIds.isEmpty()) {
290+
log.warn("Discarding trace-thread entry with no thread ids for workspace '{}'", message.workspaceId());
291+
return Mono.empty();
292+
}
293+
return scoreThread.apply(threadIds.getFirst())
294+
.contextWrite(context -> context.put(RequestContext.WORKSPACE_ID, message.workspaceId())
295+
.put(RequestContext.USER_NAME, message.userName())
296+
.put(RequestContext.VISIBILITY, Visibility.PRIVATE));
297+
}
255298
}

apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/events/OnlineScoringTraceThreadLlmAsJudgeScorer.java

Lines changed: 13 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@
44
import com.comet.opik.api.ScoreSource;
55
import com.comet.opik.api.Span;
66
import com.comet.opik.api.Trace;
7-
import com.comet.opik.api.Visibility;
87
import com.comet.opik.api.attachment.EntityType;
98
import com.comet.opik.api.evaluators.AutomationRuleEvaluator;
109
import com.comet.opik.api.evaluators.LlmAsJudgeMessage;
@@ -19,6 +18,7 @@
1918
import com.comet.opik.domain.evaluation.EvaluationRecorder;
2019
import com.comet.opik.domain.evaluation.OnlineEvaluationRecorder;
2120
import com.comet.opik.domain.evaluators.AutomationRuleEvaluatorService;
21+
import com.comet.opik.domain.evaluators.OnlineScorePublisher;
2222
import com.comet.opik.domain.evaluators.UserLog;
2323
import com.comet.opik.domain.llm.ChatCompletionService;
2424
import com.comet.opik.domain.llm.LlmProviderFactory;
@@ -35,7 +35,6 @@
3535
import lombok.extern.slf4j.Slf4j;
3636
import org.redisson.api.RedissonReactiveClient;
3737
import org.slf4j.Logger;
38-
import reactor.core.publisher.Flux;
3938
import reactor.core.publisher.Mono;
4039
import reactor.core.scheduler.Schedulers;
4140
import ru.vyarus.dropwizard.guice.module.installer.feature.eager.EagerSingleton;
@@ -70,6 +69,7 @@ public class OnlineScoringTraceThreadLlmAsJudgeScorer extends OnlineScoringBaseS
7069
private final ServiceTogglesConfig serviceTogglesConfig;
7170
private final OnlineEvaluationRecorder onlineEvaluationRecorder;
7271
private final AttachmentService attachmentService;
72+
private final OnlineScorePublisher onlineScorePublisher;
7373

7474
@Inject
7575
public OnlineScoringTraceThreadLlmAsJudgeScorer(@NonNull @Config("onlineScoring") OnlineScoringConfig config,
@@ -85,7 +85,8 @@ public OnlineScoringTraceThreadLlmAsJudgeScorer(@NonNull @Config("onlineScoring"
8585
@NonNull AgenticScoringService agenticScoringService,
8686
@NonNull SpanService spanService,
8787
@NonNull OnlineEvaluationRecorder onlineEvaluationRecorder,
88-
@NonNull AttachmentService attachmentService) {
88+
@NonNull AttachmentService attachmentService,
89+
@NonNull OnlineScorePublisher onlineScorePublisher) {
8990
super(config, redisson, feedbackScoreService, traceService, spanService, TRACE_THREAD_LLM_AS_JUDGE,
9091
Constants.TRACE_THREAD_LLM_AS_JUDGE);
9192
this.aiProxyService = aiProxyService;
@@ -97,6 +98,7 @@ public OnlineScoringTraceThreadLlmAsJudgeScorer(@NonNull @Config("onlineScoring"
9798
this.serviceTogglesConfig = serviceTogglesConfig;
9899
this.onlineEvaluationRecorder = onlineEvaluationRecorder;
99100
this.attachmentService = attachmentService;
101+
this.onlineScorePublisher = onlineScorePublisher;
100102
this.userFacingLogger = UserFacingLoggingFactory.getLogger(OnlineScoringTraceThreadLlmAsJudgeScorer.class);
101103
}
102104

@@ -112,25 +114,14 @@ protected Mono<Void> score(@NonNull TraceThreadToScoreLlmAsJudge message) {
112114
log.info("Message received with projectId: '{}', ruleId: '{}', threadIds: '{}' for workspace '{}'",
113115
message.projectId(), message.ruleId(), message.threadIds(), message.workspaceId());
114116

115-
return Flux.fromIterable(message.threadIds())
116-
// Score each thread id independently: a single thread's failure must not stop scoring the
117-
// sibling thread ids. Per-thread errors are materialized (onErrorResume) so the flatMap
118-
// completes for every thread; the batch's first failure is then re-surfaced below. This keeps
119-
// the failure on the Mono error path handled by BaseRedisSubscriber.processMessage's
120-
// onErrorResume — classified as a processing error, following the normal retryable/
121-
// non-retryable path — instead of leaking into the enclosing onErrorContinue via Flux.flatMap
122-
// (which would drop the element and count it as an "unexpected" error).
123-
.flatMap(threadId -> processThreadScores(message, threadId)
124-
.then(Mono.<Throwable>empty())
125-
.onErrorResume(Mono::just))
126-
.collectList()
127-
.flatMap(errors -> errors.isEmpty() ? Mono.<Void>empty() : Mono.error(errors.getFirst()))
128-
.contextWrite(context -> context.put(RequestContext.WORKSPACE_ID, message.workspaceId())
129-
.put(RequestContext.USER_NAME, message.userName())
130-
.put(RequestContext.VISIBILITY, Visibility.PRIVATE))
131-
.doOnSuccess(unused -> log.info(
132-
"Processed trace threads for projectId '{}', ruleId '{}' for workspace '{}'",
133-
message.projectId(), message.ruleId(), message.workspaceId()))
117+
// The success log sits inside the scoring callback: a migrated entry completes this chain without
118+
// scoring anything, so a doOnSuccess out here would claim work that never happened.
119+
return migrateOrScoreThreadIds(message, message.threadIds(), onlineScorePublisher,
120+
threadId -> message.toBuilder().threadIds(List.of(threadId)).build(),
121+
threadId -> processThreadScores(message, threadId)
122+
.doOnSuccess(unused -> log.info(
123+
"Processed trace thread '{}' for projectId '{}', ruleId '{}' for workspace '{}'",
124+
threadId, message.projectId(), message.ruleId(), message.workspaceId())))
134125
.doOnError(error -> log.error(
135126
"Error processing trace thread for projectId '{}', ruleId '{}' for workspace '{}'",
136127
message.projectId(), message.ruleId(), message.workspaceId(), error))

apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/events/OnlineScoringTraceThreadUserDefinedMetricPythonScorer.java

Lines changed: 13 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -4,14 +4,14 @@
44
import com.comet.opik.api.ScoreSource;
55
import com.comet.opik.api.Span;
66
import com.comet.opik.api.Trace;
7-
import com.comet.opik.api.Visibility;
87
import com.comet.opik.api.evaluators.AutomationRuleEvaluator;
98
import com.comet.opik.api.events.TraceThreadToScoreUserDefinedMetricPython;
109
import com.comet.opik.domain.FeedbackScoreService;
1110
import com.comet.opik.domain.ProjectService;
1211
import com.comet.opik.domain.SpanService;
1312
import com.comet.opik.domain.TraceService;
1413
import com.comet.opik.domain.evaluators.AutomationRuleEvaluatorService;
14+
import com.comet.opik.domain.evaluators.OnlineScorePublisher;
1515
import com.comet.opik.domain.evaluators.UserLog;
1616
import com.comet.opik.domain.evaluators.python.PythonEvaluatorService;
1717
import com.comet.opik.domain.threads.TraceThreadService;
@@ -26,7 +26,6 @@
2626
import org.apache.commons.lang3.tuple.Pair;
2727
import org.redisson.api.RedissonReactiveClient;
2828
import org.slf4j.Logger;
29-
import reactor.core.publisher.Flux;
3029
import reactor.core.publisher.Mono;
3130
import reactor.core.scheduler.Schedulers;
3231
import ru.vyarus.dropwizard.guice.module.installer.feature.eager.EagerSingleton;
@@ -62,6 +61,7 @@ public class OnlineScoringTraceThreadUserDefinedMetricPythonScorer
6261
private final ProjectService projectService;
6362
private final AutomationRuleEvaluatorService automationRuleEvaluatorService;
6463
private final AgenticScoringService agenticScoringService;
64+
private final OnlineScorePublisher onlineScorePublisher;
6565

6666
@Inject
6767
public OnlineScoringTraceThreadUserDefinedMetricPythonScorer(
@@ -75,7 +75,8 @@ public OnlineScoringTraceThreadUserDefinedMetricPythonScorer(
7575
@NonNull ProjectService projectService,
7676
@NonNull AutomationRuleEvaluatorService automationRuleEvaluatorService,
7777
@NonNull SpanService spanService,
78-
@NonNull AgenticScoringService agenticScoringService) {
78+
@NonNull AgenticScoringService agenticScoringService,
79+
@NonNull OnlineScorePublisher onlineScorePublisher) {
7980
super(config, redisson, feedbackScoreService, traceService, spanService,
8081
TRACE_THREAD_USER_DEFINED_METRIC_PYTHON,
8182
Constants.TRACE_THREAD_USER_DEFINED_METRIC_PYTHON);
@@ -85,6 +86,7 @@ public OnlineScoringTraceThreadUserDefinedMetricPythonScorer(
8586
this.projectService = projectService;
8687
this.automationRuleEvaluatorService = automationRuleEvaluatorService;
8788
this.agenticScoringService = agenticScoringService;
89+
this.onlineScorePublisher = onlineScorePublisher;
8890
this.userFacingLogger = UserFacingLoggingFactory
8991
.getLogger(OnlineScoringTraceThreadUserDefinedMetricPythonScorer.class);
9092
}
@@ -104,25 +106,14 @@ protected Mono<Void> score(@NonNull TraceThreadToScoreUserDefinedMetricPython me
104106
log.info("Message received with projectId '{}', ruleId '{}' for workspace '{}'",
105107
message.projectId(), message.ruleId(), message.workspaceId());
106108

107-
return Flux.fromIterable(message.threadIds())
108-
// Score each thread id independently: a single thread's failure must not stop scoring the
109-
// sibling thread ids. Per-thread errors are materialized (onErrorResume) so the flatMap
110-
// completes for every thread; the batch's first failure is then re-surfaced below. This keeps
111-
// the failure on the Mono error path handled by BaseRedisSubscriber.processMessage's
112-
// onErrorResume — classified as a processing error, following the normal retryable/
113-
// non-retryable path — instead of leaking into the enclosing onErrorContinue via Flux.flatMap
114-
// (which would drop the element and count it as an "unexpected" error).
115-
.flatMap(threadId -> processThreadScores(message, threadId)
116-
.then(Mono.<Throwable>empty())
117-
.onErrorResume(Mono::just))
118-
.collectList()
119-
.flatMap(errors -> errors.isEmpty() ? Mono.<Void>empty() : Mono.error(errors.getFirst()))
120-
.contextWrite(context -> context.put(RequestContext.WORKSPACE_ID, message.workspaceId())
121-
.put(RequestContext.USER_NAME, message.userName())
122-
.put(RequestContext.VISIBILITY, Visibility.PRIVATE))
123-
.doOnSuccess(unused -> log.info(
124-
"Processed trace threads for projectId '{}', ruleId '{}' for workspace '{}'",
125-
message.projectId(), message.ruleId(), message.workspaceId()))
109+
// The success log sits inside the scoring callback: a migrated entry completes this chain without
110+
// scoring anything, so a doOnSuccess out here would claim work that never happened.
111+
return migrateOrScoreThreadIds(message, message.threadIds(), onlineScorePublisher,
112+
threadId -> message.toBuilder().threadIds(List.of(threadId)).build(),
113+
threadId -> processThreadScores(message, threadId)
114+
.doOnSuccess(unused -> log.info(
115+
"Processed trace thread '{}' for projectId '{}', ruleId '{}' for workspace '{}'",
116+
threadId, message.projectId(), message.ruleId(), message.workspaceId())))
126117
.doOnError(error -> log.error(
127118
"Error processing trace thread for projectId '{}', ruleId '{}' for workspace '{}'",
128119
message.projectId(), message.ruleId(), message.workspaceId(), error))

apps/opik-backend/src/main/java/com/comet/opik/domain/evaluators/ManualEvaluationService.java

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -185,8 +185,8 @@ private Mono<Integer> evaluateTraces(List<UUID> traceIds, List<AutomationRuleEva
185185
.toList();
186186
Mono<Void> traceThreadMono = Flux.fromIterable(traceThreadRules)
187187
.flatMap(rule -> {
188-
log.info("Enqueueing trace-thread evaluation for rule '{}' with '{}' trace IDs", rule.getId(),
189-
traceIdStrings.size());
188+
log.info("Enqueueing '{}' trace-thread evaluation messages, one per trace ID, for rule '{}'",
189+
traceIdStrings.size(), rule.getId());
190190
return onlineScorePublisher.enqueueThreadMessage(traceIdStrings, rule, projectId, workspaceId,
191191
userName);
192192
})
@@ -384,8 +384,8 @@ private Mono<Integer> evaluateThreads(List<UUID> threadModelIds, List<Automation
384384
// reactive context. enqueueThreadMessage does a blocking rule lookup, so defer onto boundedElastic.
385385
return Flux.fromIterable(rules)
386386
.flatMap(rule -> {
387-
log.info("Enqueueing evaluation for rule '{}' with '{}' thread IDs", rule.getId(),
388-
threadIds.size());
387+
log.info("Enqueueing '{}' evaluation messages, one per thread ID, for rule '{}'",
388+
threadIds.size(), rule.getId());
389389
return onlineScorePublisher.enqueueThreadMessage(threadIds, rule, projectId,
390390
workspaceId, userName);
391391
})

0 commit comments

Comments
 (0)