Skip to content

Commit 284f1ff

Browse files
petrotiurinclaude
andauthored
[OPIK-7103] feat: {{trace}} + {{span}} agentic attachment evaluation for online LLM-as-judge (#7277)
* [OPIK-7103] feat: {{trace}} + {{span}} agentic attachment evaluation for online LLM-as-judge Squashed feature + review fixes; see PR #7277. Reconciled with main: agentic eval is triggered ONLY by the {{trace}}/{{span}} sentinels (not by attachment presence), and main's OnlineEvaluationRecorder monitoring is threaded through the flow. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * [OPIK-7103] perf: hop {{trace}} structure serialization onto parallel scheduler buildTraceStructure ran buildFullJson + compress(FULL) — the most CPU-/GC-expensive part of routing — inside a .map() on the Mono.zip of the attachment lookups, so it executed on the R2DBC/attachment scheduler thread. Wrap the body in Mono.fromCallable(...).subscribeOn(Schedulers.parallel()), mirroring evaluate()'s existing prep hop, so large-trace serialization no longer taxes the DB scheduler. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * [OPIK-7103] refactor: address PR review feedback on online-eval attachment scorers Addresses reviewer comments on #7277: - AttachmentSummaries: serialize EntityType via its @jsonvalue getValue() instead of name().toLowerCase() - GetAttachmentTool: convert the tool/param description concatenations to text blocks (byte-identical strings via \ continuations) - AttachmentService.getAttachmentInfoByEntityIds: null-safe entityIds via CollectionUtils.isEmpty (drop @nonnull) - TraceCompressor: chain the 4-arg compress() through the attachments overload so the WITH_JQ_HINT default lives in one place - OnlineScoringLlmAsJudgeScorer: give TraceStructure @builder(toBuilder=true) + @nonnull and build it via the builder - OnlineScoringBaseScorer: log the best-effort attachment re-read failures (previously swallowed silently) in both the single-entity and span upload-race helpers - OnlineScoringSpanLlmAsJudgeScorer: extract the request-building if-branches into small aux methods returning an LlmRequests carrier - OnlineScoringSpanLlmAsJudgeScorerTest: use imports over fully-qualified names, and Podam-manufacture span fixtures with toBuilder overrides Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * [OPIK-7103] feat: make online-eval prompt cap + attachment upload-race retry configurable Addresses reviewer comments K/L on #7277 (precedent: PR 6871): - OnlineScoringConfig: add maxPromptFieldChars (@min 500, default 4000), attachmentFetchMaxRetries (@min 0 @max 20, default 5) and attachmentFetchRetryDelay (@MinDuration 50ms/@MaxDuration 5s, default 300ms) - OnlineScoringBaseScorer: read the retry count/delay from config; store config as a protected field - Trace/Span scorers: read the prompt cap from config (getMaxPromptFieldChars) - Remove the now-duplicate onlineScoringConfig field from the trace/span/thread scorers (use the base field) - config.yml + config-test.yml: document the three new keys with env overrides The attachment-fetch retry stays an interim guard; the pre-evaluation dispatch delay (OPIK-7224) is intended to replace it, but the values are now tunable per environment in the meantime. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * [OPIK-7103] fix: keep body-referenced transient attachments in {{trace}}/{{span}} structures Addresses reviewer finding "Drops unrelated transient attachments" on #7277. preferPersistentAttachments used an entity-wide gate: if any persistent attachment existed, it dropped every auto-stripped attachment — so a legitimate transient-only attachment (e.g. a REST-ingested image whose only copy is auto-stripped) was dropped whenever an unrelated persistent attachment coexisted on the same entity. The reviewer's suggested "-sdk name pairing" is not implementable: the backend transient name (input-attachment-{seqN}-{backendTs}.ext) and the SDK persistent name (input-attachment-{random}-{clientTs}-sdk.ext) share no key. Use the body reference instead — the reliable per-attachment signal: - AttachmentUtils.collectAttachmentReferences: gather the filenames referenced in the entity body (mirrors the recursive walk of hasAttachmentReferences). - preferPersistentAttachments(attachments, referencedNames): keep every persistent attachment plus any auto-stripped copy still referenced in the body; drop only orphaned auto-stripped copies when a persistent copy coexists (unchanged when none does). - Thread a per-span referenced-name map through listSpanAttachmentsToleratingUploadRace / groupBySpanPreferringPersistent; gatherSpanAttachments builds it in its existing pass. A transient coexisting with its own -sdk twin is not a reachable state (backend-strip and SDK-extract paths are mutually exclusive per upload), so erring toward keeping a referenced attachment is safe. Updated the one test that encoded the old drop behavior; added retention tests for the span + batched-trace paths and CollectAttachmentReferences unit tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * [OPIK-7103] refactor: extract agentic-tools capability into AgenticScoringService Addresses PR review feedback (comment on OnlineScoringBaseScorer#runToolCallLoop): the online scorers' agentic-tools surface (tool-call loop orchestration, upload-race-tolerant attachment listing, media-failure surfacing, tool-spec attachment, provider capability check, size estimation) had grown too large to keep propagating via inheritance from OnlineScoringBaseScorer. Extract it into an injected AgenticScoringService; the trace/span/thread scorers are now users of it instead of subclassing it, and the two Python-metric scorers no longer inherit agentic-tools machinery they never use. Also collapses the thread scorer's hand-rolled duplicate of the tool loop onto the shared implementation. Co-Authored-By: Claude Opus <noreply@anthropic.com> * [OPIK-7103] refactor: address review nits on AgenticScoringService - Drop @nonnull from AgenticScoringService interface method params (validation belongs on the impl, not the interface contract). - surfaceInjectedMediaFailure: quote the attachments/detail log placeholders and pass the throwable as the trailing SLF4J arg so the stack trace isn't lost. - Extract resolveWithUploadRaceTolerance to de-duplicate the retry/best-effort pipeline shared by listAttachmentsToleratingUploadRace and listSpanAttachmentsToleratingUploadRace. Co-Authored-By: Claude Opus <noreply@anthropic.com> * [OPIK-7103] refactor: address andrescrz review nits on agentic-tools PR Favours builders over constructors in the span scorer, narrows a swallowed exception in AttachmentUtils with logging, caps maxPromptFieldChars, uses lombok for the AgenticScoringServiceImpl constructor, avoids the exposed Jackson mapper in favor of JsonUtils, and drops fully-qualified class names in the newly added OnlineScoringEngineTest cases. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 08528ad commit 284f1ff

27 files changed

Lines changed: 2684 additions & 545 deletions

apps/opik-backend/config.yml

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -671,6 +671,20 @@ onlineScoring:
671671
# tool. The URL is consumed within a single tool-call round; 120 s is ample for most providers.
672672
# Capped at 3600 s — longer-lived URLs are a security risk if the URL leaks.
673673
agenticToolsS3PresignTtlSeconds: ${ONLINE_SCORING_AGENTIC_TOOLS_S3_PRESIGN_TTL_SECONDS:-120}
674+
# Default: 4000
675+
# Description: Per-variable substitution cap (in characters) for the {{trace}}/{{span}} structure-injection
676+
# and agentic-tools paths. Large enough that small entity input/output renders inline, small enough that a
677+
# huge entity doesn't blow context (the agent fetches the rest via the read tool). 500-100000.
678+
maxPromptFieldChars: ${ONLINE_SCORING_MAX_PROMPT_FIELD_CHARS:-4000}
679+
# Default: 5
680+
# Description: Attachment-upload race tolerance for the {{trace}}/{{span}} structures — how many times the
681+
# attachment lookup is retried when the entity references an attachment that isn't persisted yet. Interim
682+
# guard (0-20; 0 disables retrying) pending the pre-evaluation dispatch delay (OPIK-7224).
683+
attachmentFetchMaxRetries: ${ONLINE_SCORING_ATTACHMENT_FETCH_MAX_RETRIES:-5}
684+
# Default: 300ms
685+
# Description: Delay between attachment-lookup retries (see attachmentFetchMaxRetries). Worst-case added
686+
# latency ≈ attachmentFetchMaxRetries × this, and only for entities that expect an attachment. 50ms-5s.
687+
attachmentFetchRetryDelay: ${ONLINE_SCORING_ATTACHMENT_FETCH_RETRY_DELAY:-300ms}
674688
## scorer: options from AutomationRuleEvaluatorType
675689
## streamName: the name of the stream in redis
676690
## codec: 'json' when there are non-java consumers, 'java' for java consumers only

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

Lines changed: 474 additions & 0 deletions
Large diffs are not rendered by default.

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

Lines changed: 10 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@
77
import com.comet.opik.api.filter.Operator;
88
import com.comet.opik.api.filter.TraceField;
99
import com.comet.opik.api.filter.TraceFilter;
10-
import com.comet.opik.api.resources.v1.events.tools.TraceToolContext;
1110
import com.comet.opik.domain.FeedbackScoreService;
1211
import com.comet.opik.domain.TraceSearchCriteria;
1312
import com.comet.opik.domain.TraceService;
@@ -27,14 +26,12 @@
2726
import java.math.BigDecimal;
2827
import java.util.List;
2928
import java.util.Map;
30-
import java.util.Optional;
3129
import java.util.UUID;
3230
import java.util.concurrent.atomic.AtomicReference;
3331
import java.util.stream.Collectors;
3432

3533
import static com.comet.opik.api.FeedbackScoreItem.FeedbackScoreBatchItem;
3634
import static com.comet.opik.api.FeedbackScoreItem.FeedbackScoreBatchItemThread;
37-
import static com.comet.opik.infrastructure.log.LogContextAware.wrapWithMdc;
3835

3936
/**
4037
* Base online scorer for all particular implementations to extend. It listens to a Redis stream for
@@ -46,13 +43,22 @@
4643
public abstract class OnlineScoringBaseScorer<M extends RedisSubscriberMessage> extends BaseRedisSubscriber<M> {
4744

4845
public static final int TRACE_PAGE_LIMIT = 2000;
46+
47+
/**
48+
* Truncation marker hint for the no-tools inline {@code {{trace}}} / {@code {{span}}} fallback. There
49+
* are no {@code read}/{@code jq} tools to drill in, so the hint just flags that the value was
50+
* truncated rather than pointing at a (non-existent) follow-up tool.
51+
*/
52+
protected static final String INLINE_TRUNCATION_HINT = "full content not shown";
53+
4954
private static final String ONLINE_SCORING_NAMESPACE = "online_scoring";
5055

5156
/**
5257
* Logger for the actual subclass, in order to have the correct class name in the logs.
5358
*/
5459
private final Logger log = LoggerFactory.getLogger(this.getClass());
5560

61+
protected final OnlineScoringConfig onlineScoringConfig;
5662
protected final FeedbackScoreService feedbackScoreService;
5763
protected final TraceService traceService;
5864
protected final AutomationRuleEvaluatorType type;
@@ -68,41 +74,12 @@ protected OnlineScoringBaseScorer(@NonNull @Config OnlineScoringConfig config,
6874
OnlineScoringConfig.PAYLOAD_FIELD,
6975
ONLINE_SCORING_NAMESPACE,
7076
metricsBaseName);
77+
this.onlineScoringConfig = config;
7178
this.feedbackScoreService = feedbackScoreService;
7279
this.traceService = traceService;
7380
this.type = type;
7481
}
7582

76-
/**
77-
* Shared error surfacing for the agentic-tools path: when the tool-call loop fails after at
78-
* least one attachment was injected as multimodal content, the most likely cause is the judge
79-
* model rejecting that media type (we attempt all types rather than pre-gating). Emit a clear,
80-
* attachment-attributed user-facing message before propagating, so a vision-incapable model
81-
* produces an understandable error rather than a raw provider stack trace. With no injected
82-
* media the failure passes through untouched.
83-
*
84-
* <p>Static + parameterized on {@code userFacingLogger} / {@code modelName} so the trace-, span-
85-
* and thread-level scorers can all reuse it despite each owning its own logger and model accessor.
86-
*/
87-
protected static <T> Mono<T> surfaceInjectedMediaFailure(@NonNull Throwable error,
88-
@NonNull TraceToolContext ctx, String modelName, @NonNull Logger userFacingLogger,
89-
@NonNull Map<String, String> mdc) {
90-
if (ctx.hasInjectedMedia()) {
91-
String attachments = ctx.getInjectedAttachments().stream()
92-
.map(a -> "'%s' (%s)".formatted(a.fileName(), a.category().name().toLowerCase()))
93-
.collect(Collectors.joining(", "));
94-
String detail = Optional.ofNullable(error.getCause()).map(Throwable::getMessage)
95-
.orElse(error.getMessage());
96-
try (var logContext = wrapWithMdc(mdc)) {
97-
userFacingLogger.error(
98-
"Scoring failed after loading attachment(s) {}; the judge model '{}' may not support this"
99-
+ " attachment type. Use a model that supports the attachment's media type. Details: {}",
100-
attachments, modelName, detail);
101-
}
102-
}
103-
return Mono.error(error);
104-
}
105-
10683
/**
10784
* Propagates the workspace/user the message belongs to onto the reactive context for the whole
10885
* scoring chain (feedback-score persistence reads it). Per-message throughput and error metrics are

0 commit comments

Comments
 (0)