[OPIK-6741] [BE][FE] feat: add EvalTriggerScope to evaluation rules - #7439
Conversation
Add a trigger_scope field (PRODUCTION / EXPERIMENT / BOTH) to online evaluation rules so they can fire on experiment traces, production traces, or both. Existing rules default to PRODUCTION (backward compatible). Clients that omit trigger_scope get PRODUCTION silently. BE: New EvalTriggerScope enum, DB migration 000090, field threaded through all model/DAO/service/mapper layers, OnlineScoringSampler now lets experiment traces through and filters per-evaluator based on trigger scope. Null-safe defaulting in service and sampler. FE: ToggleGroup control on the v2 rule create/edit dialog for selecting trigger scope, wired into form schema and API payload. Tests: 3 dedicated TriggerScope unit tests, TracesUpdated path coverage for EXPERIMENT source, updated existing sampler and integration tests.
github-actions
Bot
added
🟠 size/L
java
⏱️ pre-commit per-hook timing
⏭️ 38 skipped (no matching files changed)
|
Backend Tests - Integration Group 13 50 files - 2 50 suites - 2 3m 45s ⏱️ - 1m 24s Results for commit 8da394b. ± Comparison against base commit d554a6f. This pull request removes 45 and adds 20 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
- Add withTriggerScope to AutomationRuleEvaluatorModel interface + 6 records - Default null triggerScope to PRODUCTION in service save path - Add --comment to migration 000090 - Fix DisabledRulesTest by setting explicit triggerScope on test rules - Add triggerScope to update test expected builder
andriidudar
left a comment
andriidudar
left a comment
There was a problem hiding this comment.
The FE part looks good, approved it.
There was a problem hiding this comment.
Approving. Cleanly threads EvalTriggerScope (production/experiment/both) through the full BE stack, migration 000090, and the v2 FE dialog; backward-compatible via PRODUCTION default. Satisfies OPIK-6741 (automatic eval-rule execution on experiment traces as a single source of truth). See inline comments for non-blocking follow-ups.
| for (var trace : projectTraces) { | ||
| if (Source.isLoggingSource(trace.source())) { | ||
| // For SDK traces all evaluators apply | ||
| if (Source.isLoggingSource(trace.source()) || trace.source() == Source.EXPERIMENT) { |
There was a problem hiding this comment.
Requirement-critical: the feature works only if experiment-run traces actually carry Source.EXPERIMENT. Unit tests build such traces by hand, so the real-experiment E2E run is what proves the acceptance criterion. (Note: experiment traces now take this scorable branch, so their selected_rule_ids are ignored — intended per the ticket.)
There was a problem hiding this comment.
Good observation — yes, adding Source.EXPERIMENT to the scorableTraces set is required for the trigger scope to take effect. Previously experiment traces were only scorable via selectedRuleIds (the playground flow); now they can also be auto-sampled by rules with EXPERIMENT or BOTH scope. The per-evaluator matchesTriggerScope() filter then gates which rules actually fire.
🤖 Reply posted via /address-github-pr-comments
| .toList(); | ||
| } | ||
|
|
||
| private boolean matchesTriggerScope(AutomationRuleEvaluator<?, ?> evaluator, Trace trace) { |
There was a problem hiding this comment.
matchesTriggerScope is only applied in this trace-level path, but trigger_scope is added to all evaluator types. Confirm thread/span rules either honor the scope or don't surface the toggle where it's a no-op.
There was a problem hiding this comment.
Good catch. Thread and span evaluators have separate scoring paths (OnlineScoringSpanSampler, TraceThreadOnlineScoringSamplerListener) that don't check triggerScope — the toggle was a no-op for those rule types. Fixed by hiding the toggle in the FE when scope is thread or span (d8b0156).
🤖 Reply posted via /address-github-pr-comments
There was a problem hiding this comment.
Commit d8b0156 addressed this comment by hiding the trigger-scope toggle for thread- and span-scoped rules, where it would be a no-op.
| boolean hasVersion1AutomationRules(@Bind("workspaceId") String workspaceId); | ||
|
|
||
| @SqlUpdate("INSERT INTO automation_rules(id, workspace_id, `action`, name, sampling_rate, enabled, filters) " | ||
| @SqlUpdate("INSERT INTO automation_rules(id, workspace_id, `action`, name, sampling_rate, enabled, trigger_scope, filters) " |
There was a problem hiding this comment.
INSERT binds the raw enum (PRODUCTION) via @BindMethods (JDBI default BY_NAME), while updateBaseRule binds getValue() (production). Works only because MySQL ENUM assignment is case-insensitive. Consider EvalTriggerScope implements HasValue + an AbstractEnumColumnMapper factory (codebase pattern), or bind getValue() consistently. action has the same pre-existing latent issue.
There was a problem hiding this comment.
Fixed in 8da394b — EvalTriggerScope now implements HasValue, with a new EvalTriggerScopeColumnMapper (following the DashboardScopeMapper pattern). Registered via @RegisterArgumentFactory on the DAO, and the UPDATE @Bind param type changed from String to EvalTriggerScope. Both INSERT (@BindMethods) and UPDATE (@Bind) paths now go through the argument factory for consistent lowercase binding.
🤖 Reply posted via /address-github-pr-comments
There was a problem hiding this comment.
Commit d8b0156 addressed this comment by making EvalTriggerScope implement HasValue, adding its enum column mapper, and registering that mapper for DAO bindings. updateBaseRule now accepts the enum directly, ensuring consistent value binding.
| String name = rs.getString("name"); | ||
| Float samplingRate = rs.getFloat("sampling_rate"); | ||
| boolean enabled = rs.getBoolean("enabled"); | ||
| EvalTriggerScope triggerScope = EvalTriggerScope.fromString(rs.getString("trigger_scope")); |
There was a problem hiding this comment.
fromString throws on null/unknown, failing the whole row map. Safe given NOT NULL DEFAULT, but the shared AbstractEnumColumnMapper.parse() logs + returns null, degrading more gracefully.
There was a problem hiding this comment.
Fixed in 8da394b — added null guard before EvalTriggerScope.fromString() in the RowMapper to handle rows where trigger_scope is NULL (legacy data or edge cases).
🤖 Reply posted via /address-github-pr-comments
There was a problem hiding this comment.
Commit d8b0156 addressed this comment by guarding against null trigger-scope values before calling fromString. Unknown non-null values can still throw, so the concern is only partially addressed.
| } | ||
| }; | ||
|
|
||
| if (evaluator.triggerScope() == null) { |
There was a problem hiding this comment.
Nit: default is applied in 4 places (DTO @Builder.Default, here, update(), and matchesTriggerScope). Aligned with the backward-compat requirement so acceptable; only the sampler guard is strictly needed for robustness.
There was a problem hiding this comment.
Acknowledged — the @Builder.Default to PRODUCTION on the API model ensures new rules default to production-only scope. The DB migration also has DEFAULT 'production' for existing rows. Keeping both layers aligned per the existing enabled field pattern.
🤖 Reply posted via /address-github-pr-comments
Thread and span evaluators have separate scoring paths that don't check triggerScope, so the toggle is a no-op for those rule types. Hide it in the FE to avoid user confusion.
| {!isThreadScope && !isSpanScope && ( | ||
| <FormField | ||
| control={form.control} | ||
| name="triggerScope" | ||
| render={({ field }) => ( | ||
| <FormItem> | ||
| <Label className="flex items-center"> | ||
| Trigger scope{" "} | ||
| <TooltipWrapper content="Choose whether this rule fires on production traces, experiment traces, or both."> | ||
| <Info className="ml-1 size-4 text-light-slate" /> | ||
| </TooltipWrapper> | ||
| </Label> |
There was a problem hiding this comment.
Thread/span rules stay production-only
triggerScope is part of the shared evaluator payload, but this condition hides it for scope thread/span, so create/clone/edit flows keep submitting the production default from defaultValues/reset and can't switch to experiment or both — can we render it for all scopes and only lock it down when a scope needs a fixed value?
Want Baz to fix this for you? Activate Fixer You can also update your AI coding guidelines based on this comment by apply pr to [branch name]
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
apps/opik-frontend/src/v2/pages-shared/automations/AddEditRuleDialog/AddEditRuleDialog.tsx
around lines 630-651, the `triggerScope` FormField is currently wrapped in
`!isThreadScope && !isSpanScope`, which hides the trigger-scope selector for thread/span
rules. Refactor this so the `triggerScope` field is rendered for all `scope` values
(thread/trace/span) and only restricts user changes by disabling the control or forcing
a fixed value when that scope truly must not vary. Update the `onValueChange`/`value`
logic to either allow selection for the supported cases or keep it read-only with the
correct fixed value, so create/clone/edit flows can switch thread/span rules to
experiment or both.
thiagohora
left a comment
thiagohora
left a comment
There was a problem hiding this comment.
Revision 2 addresses the review feedback:
- Thread/span rules: trigger-scope toggle now hidden (FE guard).
- INSERT/UPDATE enum binding: unified via
EvalTriggerScope implements HasValue+EvalTriggerScopeColumnMapperregistered as argument factory — consistent write path. - RowMapper: null-guarded.
All flagged items resolved or reasonably handled; no new issues. LGTM 🚀
| for (var trace : projectTraces) { | ||
| if (Source.isLoggingSource(trace.source())) { | ||
| // For SDK traces all evaluators apply | ||
| if (Source.isLoggingSource(trace.source()) || trace.source() == Source.EXPERIMENT) { |
There was a problem hiding this comment.
💡 suggestion | Performance
Adding Source.EXPERIMENT here means experiment traces now always populate scorableTraces, so a project receiving only experiment traffic no longer hits the scorableTraces.isEmpty() early-out below — it always runs ruleEvaluatorService.findAll(...) (Redis-cached, so no extra DB load) plus a parallelStream over every evaluator, evaluating matchesTriggerScope/isEvaluatorSelectedForTrace/shouldSampleTrace per (evaluator × trace), even when no rule in the project has experiment/both scope.
Not a correctness issue and probably fine, but for experiment-heavy workspaces with no experiment-scoped rules it's wasted CPU. If that ever matters, a cheap guard (skip when all scorable traces are experiment-source and no evaluator has EXPERIMENT/BOTH scope) would restore the old fast path.
🤖 Review posted via /review-github-pr
There was a problem hiding this comment.
Good point. The evaluator fetch is Redis-cached (ruleEvaluatorService.findAll) so there's no extra DB load, and the per-evaluator matchesTriggerScope check is a cheap enum comparison. For experiment-heavy workspaces with no experiment-scoped rules, the overhead is the parallelStream iteration over evaluators that all get filtered out — negligible in practice.
If profiling ever shows this matters, the guard you suggest (skip when all scorable traces are experiment-source and no evaluator has EXPERIMENT/BOTH scope) would be straightforward to add. Leaving as-is for now to keep the logic simple.
🤖 Reply posted via /address-github-pr-comments
|
❓ question | Documentation The "Documentation update" checklist item is unchecked, but this adds a user-facing capability (Trigger scope: production / experiment / both on evaluation rules). The 🤖 Review posted via /review-github-pr |
|
👋 Review summary What looks good
Overall Inline comments: 1 performance suggestion + 1 documentation question — nothing blocking. 🤖 Review posted via /review-github-pr |
4 participants
Details
Add a
trigger_scopefield to online evaluation rules with three options: production (default), experiment, and both. This controls whether a rule fires on production traces, experiment traces, or both — enabling customers to define one set of LLM-judge metrics as the single source of truth across experiments and production monitoring.EvalTriggerScopeenum, DB migration (000090), field threaded through API/model/DAO/service/mapper layers,OnlineScoringSamplerupdated to let experiment traces through and filter per-evaluator by scope. Null-safe defaulting to PRODUCTION for backward compatibility.ToggleGroupcontrol on the v2 rule create/edit dialog (production / experiment / both), wired into form schema and API payload.TriggerScopeTestsin sampler, updatedTracesUpdatedpath coverage, updated integration tests.Change checklist
Issues
AI-WATERMARK
AI-WATERMARK: yes
Testing
mvn clean test -Dtest="OnlineScoringSamplerTest"— 42/42 pass (3 new TriggerScope tests + updated existing)mvn clean test -Dtest="AutomationRuleEvaluatorsResourceTest"— 113/113 assertion-pass (0 failures; 2 pre-existing infra timeouts)npx tsc --noEmit— clean TypeScript checktest_trigger_scope_e2e.py): created 3 rules (production/experiment/both scopes), logged SDK traces + ran real experiment, verified via evaluator logs that each rule scored only its matching tracesDocumentation
@Schemaannotation added totriggerScopefield on the create DTO — flows into OpenAPI spec and Fern-generated docs. No separate docs page changes needed.