Repository navigation
[kbn-evals] Move online evals onto typed workflow steps with retries - #296582
Conversation
viduni94
added
release_note:skip
|
/ci |
There was a problem hiding this comment.
🟢 Approval recommended
The typed steps, retry behavior, concurrency limits, space scoping, and associated tests are consistent and complete.
0 open findings
What changed in this PR
Moves online evaluations to typed, space-aware workflow steps with retries and bounded evaluator concurrency.
Changes:
- Adds and registers
ai.evals.persistOnlineScores. - Generates resilient workflows with exponential retries and per-trace failure isolation.
- Runs evaluators concurrently and scopes Lens panels to the active space.
| File | Description |
|---|---|
x-pack/platform/plugins/shared/evals/server/workflows/steps.ts |
Implements the persistence step and retry-triggering evaluation behavior. |
x-pack/platform/plugins/shared/evals/server/workflows/steps.test.ts |
Tests partial failures, retries, and score persistence. |
x-pack/platform/plugins/shared/evals/server/workflows/step_definitions_approval.test.ts |
Adds the new step to approval checks. |
x-pack/platform/plugins/shared/evals/server/workflows/lib.ts |
Adds online-score persistence support. |
x-pack/platform/plugins/shared/evals/server/workflows/lib.test.ts |
Tests persistence request mapping and failure counts. |
x-pack/platform/plugins/shared/evals/server/routes/evaluators/shared/execute_evaluators.ts |
Adds bounded concurrent evaluator execution. |
x-pack/platform/plugins/shared/evals/server/routes/evaluators/evaluate.test.ts |
Tests concurrency, ordering, and failure isolation. |
x-pack/platform/plugins/shared/evals/README.md |
Documents the new workflow step. |
x-pack/platform/plugins/shared/evals/public/workflows/steps.ts |
Defines the public persistence step. |
x-pack/platform/plugins/shared/evals/public/workflows/index.ts |
Registers the public persistence step. |
x-pack/platform/plugins/shared/evals/public/pages/online_eval_detail/index.tsx |
Adds active-space filtering to Lens panels. |
x-pack/platform/plugins/shared/evals/public/pages/online_eval_detail/index.test.tsx |
Tests space-scoped panel rendering. |
x-pack/platform/plugins/shared/evals/public/hooks/use_active_space_id.ts |
Resolves the active or default space. |
x-pack/platform/plugins/shared/evals/public/hooks/use_active_space_id.test.tsx |
Tests active-space resolution behavior. |
x-pack/platform/plugins/shared/evals/common/workflows/steps.ts |
Defines typed persistence schemas and metadata. |
x-pack/platform/plugins/shared/evals/common/online_evals/workflow_yaml.ts |
Generates and parses typed retry-enabled workflows. |
x-pack/platform/plugins/shared/evals/common/online_evals/workflow_yaml.test.ts |
Tests typed workflow generation and retry configuration. |
x-pack/platform/plugins/shared/evals/common/online_evals/__snapshots__/workflow_yaml.test.ts.snap |
Updates expected generated YAML. |
src/platform/plugins/shared/workflows_extensions/test/scout/api/fixtures/approved_step_definitions/ai.evals.persistOnlineScores.txt |
Approves the new step-definition hash. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
💛 Build succeeded, but was flaky
Failed CI Steps
Metrics [docs]Async chunks
Page load bundle
Unknown metric groupstotal optimizer output size
warm start memory
workflow yaml validation
Test Failures
cc @viduni94 |
# Conflicts: # x-pack/platform/plugins/shared/evals/common/online_evals/__snapshots__/workflow_yaml.test.ts.snap # x-pack/platform/plugins/shared/evals/common/online_evals/workflow_yaml.test.ts # x-pack/platform/plugins/shared/evals/common/online_evals/workflow_yaml.ts
|
Pinging @elastic/nightshift-context-and-research-team (Team:nightshift-context-and-research) |
botelastic
Bot
added
evals:smoke-tests
Team:One Workflow
| }); | ||
|
|
||
| // A trace whose retries are exhausted is skipped rather than failing the whole run. | ||
| const ITERATION_ON_FAILURE = { continue: true }; |
There was a problem hiding this comment.
with iteration-on-failure: continue the run ends green even if every trace was skipped (e.g. connector down). before it at least went red. is it ok, or should something flag a run where all iterations failed?
Kiryous
left a comment
Kiryous
left a comment
There was a problem hiding this comment.
LGTM with one small question
![infra-vault-gh-plugin-prod[bot]](https://avatars.githubusercontent.com/u/6764390?s=60&v=4){kind=small}
| { | ||
| name: MARK_PERSISTED_STEP_NAME, | ||
| type: 'data.set', | ||
| with: { any_trace_persisted: true }, |
There was a problem hiding this comment.
Severity: P3 (Low)
Only mark a trace as persisted when the persist step reports scores created or already present. An evaluation with only evidence_unmet failures returns empty results without throwing, and persisting those results succeeds with created: 0, skipped: 0. This unconditional flag therefore makes a run succeed when one trace lacks evidence and every other trace exhausts its retries, even though no scores were persisted—the situation the final guard is intended to detect.
Generated by Libra. React with 👍 or 👎 to give feedback on this comment.
SrdjanLL
left a comment
SrdjanLL
left a comment
There was a problem hiding this comment.
Overall LGTM, no blockers - just left some questions/suggestion for you to consider before merging.
| const EVALUATE_RESULTS_TEMPLATE = '${{ steps.evaluate.output.results }}'; | ||
| const EVALUATE_ERRORS_TEMPLATE = '${{ steps.evaluate.output.errors }}'; | ||
| const RETRY_CONDITION_TEMPLATE = | ||
| '${{ error.details.status != 400 and error.details.status != 403 }}'; |
There was a problem hiding this comment.
[Question] So, we're retrying 5xx errors always?
There was a problem hiding this comment.
Thanks for the review @SrdjanLL
The 5xx is retried on purpose, since that's where transient ES and inference failures end up. But your question made me look at the rest of the condition. Initially I had a deny-list (skip 400/403), so it also retried other 4xx that can't succeed, like 413. Now I switched it to an allow-list - no HTTP status (network errors, or every evaluator failing), 404 (trace still exporting), 429 and 5xx. Everything else fails straight away.
There was a problem hiding this comment.
I'll go ahead and merge this PR, but let me know if there are any concerns, i'll address them along with https://github.com/elastic/observability-dev/issues/6336
| const MARK_PERSISTED_STEP_NAME = 'mark_persisted'; | ||
| const FAIL_IF_EVERY_TRACE_FAILED_STEP_NAME = 'fail_if_every_trace_failed'; | ||
| const EVERY_TRACE_FAILED_CONDITION = | ||
| '${{ steps.sample_traces.output.values.size > 0 and variables.any_trace_persisted != true }}'; |
There was a problem hiding this comment.
[thought] Instead of "any trace persisted" we could have a percentage of successfully persisted evals on traces and have better monitoring on future iterations (you could achieve the same outcome with say variables.traces_persisted_ratio == 0). No need to add now, but thinking about raising the failures based on some configuration later on.
There was a problem hiding this comment.
Agreed, a ratio with a configurable failure threshold per monitor would give better monitoring than the all-or-nothing check. I'll do that in a follow-up.
| { | ||
| name: EVALUATE_STEP_NAME, | ||
| type: 'kibana.request', | ||
| type: EVALUATE_TRACE_STEP_TYPE, |
There was a problem hiding this comment.
[Question] Do you think it's worth making this a batch step that can run with some degree of concurrency that we control in the definition?
There was a problem hiding this comment.
Yes, that's a good callout! Will add that in a followup alonf with the ratio
| import { resolveConnectorModel } from '../../../lib/resolve_connector_model'; | ||
|
|
||
| /** Max evaluators graded in parallel for one trace; bounds concurrent judge-model calls. */ | ||
| export const EVALUATOR_CONCURRENCY = 4; |
There was a problem hiding this comment.
[Question] Any reason we can't go higher here? We could also have a single API call for all specified evaluators and evaluate in batch (API handles concurrent invocation of all evaluators sent in the request payload).
There was a problem hiding this comment.
We could go higher. I'll consider this (either the make it configurable or go with your suggestion). Might do in a follow up, if that's okay.
|
PR run: bk-01a16964-d45e-42a5-9a68-0e7c06915cae::smoke-tests::anthropic-claude-4.5-haiku | Baseline (main): bk-01a13419-4e2a-4953-bac3-99e3436a364b::smoke-tests::anthropic-claude-4.5-haiku
Summary View full comparison in UI | Refresh baseline against latest main (click Unblock in the eval build) No significant changes (5 rows)
|
💛 Build succeeded, but was flaky
Failed CI Steps
Metrics [docs]Async chunks
Page load bundle
Unknown metric groupstotal optimizer output size
warm start memory
workflow yaml validation
Test Failures
History
cc @viduni94 |
Closes #279267
Summary
The online eval workflow called
/_evaluateand/online_scoresthrough rawkibana.requeststeps with hardcoded internal headers and no failure handling. One failed call ended the whole run and dropped every remaining trace.ai.evals.evaluateTrace, plus a newai.evals.persistOnlineScores. The generated YAML no longer containskbn-xsrf,elastic-api-version,x-elastic-internal-origin, or/s/{{ workflow.spaceId }}paths, because the steps call the APIs from server code in the workflow's space.evaluateTracethrows so the retry applies, unless every failure was unmet evidence, which a retry can't fix.ai.evals.evaluateDatasetrunning 5 examples at a time, a run can now make up to 20 concurrent judge calls instead of 5.kibana.requestworkflow format, including workflows created after [kbn-evals] Scope online evaluation scores to the active space #296257. Those workflows need to be recreated.The new step needs an approval file under
workflows_extensions(ai.evals.persistOnlineScores.txt), so this needs a review from workflows-eng.Test plan
ai.evals.evaluateTraceandai.evals.persistOnlineScores, withon-failure.retryon both steps anditeration-on-failure: continueon the loop.Checklist
release_note:*label is applied per the guidelinesbackport:*labels.