Uh oh!
There was an error while loading. Please reload this page.
feat(eval): add ondemand evaluate (synchronous, client-side) - #1983
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## refactor #1983 +/- ##
============================================
- Coverage 96.96% 96.95% -0.01%
============================================
Files 364 366 +2 Lines 20758 21093 +335 ============================================
+ Hits 20127 20450 +323 - Misses 631 643 +12 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
23781e6 to
25228b3Compare| const spanId = span.spanId; | ||
| if (typeof spanId !== "string" || spanId.length === 0) continue; | ||
| const attrs = span.attributes as Record<string, unknown> | undefined; | ||
| if (attrs?.["gen_ai.tool.name"] ?? attrs?.["tool.name"]) spanIds.push(spanId); |
There was a problem hiding this comment.
Should tool-call selection use the standard operation/kind markers rather than requiring a tool-name attribute? Current telemetry identifies tool spans using gen_ai.operation.name === "execute_tool", openinference.span.kind === "TOOL", or traceloop.span.kind === "tool". The name fields are not always present. I reproduced a valid LangGraph-style tool span producing an empty toolCallSpanIds, so a TOOL_CALL evaluator makes no Evaluate request. Could we use the same marker logic as the evaluation SDK?
There was a problem hiding this comment.
I'll add support to detect these "gen_ai.operation.name === "execute_tool", openinference.span.kind === "TOOL", or traceloop.span.kind === "tool"
| try { | ||
| const evaluator = await control.send(new GetEvaluatorCommand({ evaluatorId: id })); | ||
| levels.set(id, evaluator.level ?? "SESSION"); | ||
| } catch { |
There was a problem hiding this comment.
Could evaluator lookup failures propagate instead of silently defaulting to SESSION? The level determines whether Evaluate receives traceIds, spanIds, or no target. I reproduced an AccessDeniedException here causing a trace evaluator to be submitted without an evaluation target, which can either evaluate the wrong scope or hide the actual permissions error. If a fallback is needed for compatibility, could it be limited to a missing level rather than every exception?
There was a problem hiding this comment.
AWS SDK v3 responses have | undefined even if they are required. I'm just going to do ! because every evaluator must have a level.
| } | ||
| } | ||
| } | ||
| return { sessionsEvaluated: input.traces.length, results }; |
There was a problem hiding this comment.
Could sessionsEvaluated count sessions for which an Evaluate request was actually sent? TRACE and TOOL_CALL sessions with no IDs are skipped above but still included here. I reproduced sessionsEvaluated: 1 with zero API calls and zero results. Tracking submitted sessions, or naming this sessionsDiscovered, would make the output less misleading.
| // resolve inline / file:// / -, then hand the array to core verbatim — core | ||
| // groups it by session. | ||
| const resolver = new SourceResolver({ stdin: io.stdin }); | ||
| const groundTruth = parseJsonFlag<EvaluationReferenceInput[]>( |
There was a problem hiding this comment.
Can we change this to use parseJsonArrayFlag helper instead? Since groundTruth has to be an array.
Agree with Aidans findings + one additional comment |
d1c79cf to
59e81ddCompare…d-evaluate-clean # Conflicts: # src/core/eval.tsx # src/handlers/eval/index.tsx # src/handlers/eval/types.tsx # src/testing/TestCoreClient.tsx
| // Sessions that actually produced Evaluate results — distinct from the sessions | ||
| // handed in, since a TRACE/TOOL_CALL session with no matching ids makes no call. | ||
| const evaluatedSessions = new Set<string>(); | ||
| for (const evaluatorId of input.evaluatorIds) { |
There was a problem hiding this comment.
nit: should we use levels.keys() here instead of the raw id array? Consistent with rest of code + leverages the dedup logic in resolveEvaluatorLevels(). A follow up item if you think its worth it
| const logGroupName = runtimeLogGroup(runtimeId, qualifier); | ||
| const serviceName = runtimeServiceName(runtimeName, qualifier); | ||
| // CloudWatch Insights takes epoch seconds. Discovery defaults to now-7d when |
There was a problem hiding this comment.
nit: i feel like the code explains this already.
| const [runtimeRows, sharedRows] = await Promise.all([ | ||
| runInsightsQuery(logs, [logGroupName], queryString, startSec, endSec).catch((error) => { | ||
| if (error instanceof ResourceNotFoundException) { | ||
| throw new InputValidationError( |
There was a problem hiding this comment.
should this be a different error type for telemetry? I wonder if it would be useful to distinguish invalid inputs from valid inputs without results.
There was a problem hiding this comment.
I was thinking we create a new Exception called TracesNotFound once we add observability to the cli.
| // (empty batch list); SESSION always makes one call with no target. | ||
| for (const target of targetBatches(level, trace)) { | ||
| const response = await data.send( | ||
| new EvaluateCommand({ |
There was a problem hiding this comment.
q: if a single evaluate fails, do we want to fail the entire run?
There was a problem hiding this comment.
On demand evaluate should be use for small evaluations (1-2 sessions with 2-3 evaluators) so this situation is unlikely to happen and also you could multiple evaluate calls per session if one of those fails, you have an incomplete evaluations
There was a problem hiding this comment.
Yes, fail-fast is intentional for now. One session can require multiple Evaluate API calls across evaluators and target batches. Continuing after one fails could return incomplete results for that session while appearing successful. On-demand evaluation targets small synchronous runs; larger workloads should use batch evaluation. We can add explicit partial-failure handling later if customers need it.
| } | ||
| return { | ||
| sessionsRequested: input.traces.length, | ||
| sessionsEvaluated: evaluatedSessions.size, |
There was a problem hiding this comment.
q: I see we return the sessions evaluated and the results separately. Is there a use case for getting the results for a certain session or is it more useful in aggregate?
There was a problem hiding this comment.
Results are already associated with sessions through context.spanContext.sessionId. Keeping them flat preserves the SDK response shape while still supporting per-session filtering and aggregate analysis. I also confirmed the session ID is present in a live on-demand evaluation response.
| return value.replace(/'/g, ""); | ||
| } | ||
| // buildSpanQuery is the single-phase Insights query: scope to one runtime by its |
There was a problem hiding this comment.
nit: is there info in this comment not expressed by the code?
| try { | ||
| doc = JSON.parse(message) as SpanRecord; | ||
| } catch { | ||
| continue; |
There was a problem hiding this comment.
is it worth logging a warning here or would this be noisy?
| groundTruth?: EvaluationReferenceInput[]; | ||
| }; | ||
| // EvaluateResult returns the raw Evaluate API results across all evaluators and |
There was a problem hiding this comment.
nit: these comments describe usages of the type and feel like they could drift.
| // with start before end. On-demand owns this rather than reusing batch's resolver: | ||
| // batch has no --lookback-days and its window feeds a service-side data source, not | ||
| // a client-side Insights query. | ||
| function resolveWindow(flags: { |
There was a problem hiding this comment.
how does this differ from
?There was a problem hiding this comment.
Batch's resolveWindow feeds the batch evaluation API's DataSourceConfig shape, which supports CloudWatch log groups and onlineEvaluationConfigArn.
| // Record with: RECORD=1 bun test src/handlers/eval/ondemand/ondemand.fixture.test.tsx | ||
| // | ||
| // This exercises the real seam end to end: parsing → handler → CoreClient → |
There was a problem hiding this comment.
nit: this feels overly verbose.
| ).rejects.toThrow(/--agent/); | ||
| }); | ||
| test("requires --evaluator", async () => { |
There was a problem hiding this comment.
would these make sense as a test.each pattern for the different set of flags that reject?
Uh oh!
There was an error while loading. Please reload this page.
Summary
Adds
agentcore eval ondemand evaluate— a client-side evaluation of existing sessions. On-demand gathers the sessions' traces from CloudWatch on the client, calls theEvaluatedata-plane API directly, and prints scores.batch-evaluation evaluateondemand evaluate(this PR)StartBatchEvaluationEvaluate(data plane)get)--agent/--online-eval/--data-source-config--agentonlyUsage
Flags
--agent(required),--endpoint,--evaluator <ids...>(required)--lookback-days Nor--start-time/--end-time(ISO-8601, together)--session-ids <ids...>,--trace-id <id>— independent, AND-ed fetch filters--ground-truth <json>— inline /file:///-→ SDK-nativeEvaluationReferenceInput[]Tests
ondemand.fixture.test.tsx) — recordedGetAgentRuntime+ InsightsStartQuery/GetQueryResults(both log groups) +Evaluatefixtures, driven through the real root handler against a pinned window, diffed againstevaluate.golden.json.ondemand.test.tsx,TestCoreClient) — source-arm validation,getTracesForAgent → evaluateorchestration/order,--lookback-dayswindow math,--trace-id, ground-truth passthrough.tsc --noEmit,bun test src/(1067 pass),oxlint,prettier— all clean.