-
Notifications
You must be signed in to change notification settings - Fork 58
fix(add): support sessionTimeoutMinutes and filters for online eval config #1166
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
f261cd5
5002fbf
7954a0f
ea6dc9c
e9b9200
127b3e7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,5 @@ | ||
| import { OnlineEvalConfigNameSchema } from '../../../../schema'; | ||
| import type { OnlineEvalFilter, OnlineEvalFilterOperator } from '../../../../schema'; | ||
| import type { SelectableItem } from '../../components'; | ||
| import { | ||
| ConfirmReview, | ||
|
|
@@ -13,7 +14,12 @@ | |
| import { useListNavigation, useMultiSelectNavigation } from '../../hooks'; | ||
| import { generateUniqueName } from '../../utils'; | ||
| import type { AddOnlineEvalConfig, EvaluatorItem, RuntimeEndpointEntry } from './types'; | ||
| import { DEFAULT_SAMPLING_RATE, ONLINE_EVAL_STEP_LABELS } from './types'; | ||
| import { | ||
| DEFAULT_SAMPLING_RATE, | ||
| DEFAULT_SESSION_TIMEOUT_MINUTES, | ||
| ONLINE_EVAL_FILTER_OPERATORS, | ||
| ONLINE_EVAL_STEP_LABELS, | ||
| } from './types'; | ||
| import { useAddOnlineEvalWizard } from './useAddOnlineEvalWizard'; | ||
| import { Box, Text } from 'ink'; | ||
| import React, { useCallback, useEffect, useMemo } from 'react'; | ||
|
|
@@ -99,6 +105,8 @@ | |
| const isEndpointStep = wizard.step === 'endpoint'; | ||
| const isEvaluatorsStep = wizard.step === 'evaluators'; | ||
| const isSamplingRateStep = wizard.step === 'samplingRate'; | ||
| const isSessionTimeoutStep = wizard.step === 'sessionTimeout'; | ||
| const isFiltersStep = wizard.step === 'filters'; | ||
| const isEnableOnCreateStep = wizard.step === 'enableOnCreate'; | ||
| const isConfirmStep = wizard.step === 'confirm'; | ||
|
|
||
|
|
@@ -230,6 +238,73 @@ | |
| </Box> | ||
| )} | ||
|
|
||
| {isSessionTimeoutStep && ( | ||
| <Box flexDirection="column"> | ||
| <Text dimColor> | ||
| Minutes of inactivity before an agent session is considered complete (1–1440). Leave blank to use the | ||
| default of {DEFAULT_SESSION_TIMEOUT_MINUTES}. | ||
| </Text> | ||
| <TextInput | ||
| key="sessionTimeout" | ||
| prompt="Session timeout (minutes, blank=default)" | ||
| initialValue="" | ||
| onSubmit={value => { | ||
| const trimmed = value.trim(); | ||
| if (trimmed === '') { | ||
| wizard.setSessionTimeoutMinutes(undefined); | ||
| return; | ||
| } | ||
| const minutes = parseInt(trimmed, 10); | ||
| if (isNaN(minutes) || minutes < 1 || minutes > 1440) return; | ||
| wizard.setSessionTimeoutMinutes(minutes); | ||
| }} | ||
| onCancel={() => wizard.goBack()} | ||
| customValidation={value => { | ||
| const trimmed = value.trim(); | ||
| if (trimmed === '') return true; | ||
| const minutes = parseInt(trimmed, 10); | ||
| if (isNaN(minutes)) return 'Must be an integer or blank'; | ||
| if (minutes < 1 || minutes > 1440) return 'Must be between 1 and 1440'; | ||
| return true; | ||
| }} | ||
| /> | ||
| </Box> | ||
| )} | ||
|
|
||
| {isFiltersStep && ( | ||
| <Box flexDirection="column"> | ||
| <Text dimColor> | ||
| Optional filters that scope which traces are evaluated. Format: {'<key>'} {'<op>'} {'<value>'}, separated | ||
| by ";". Operators: {ONLINE_EVAL_FILTER_OPERATORS.join(', ')}. Values are parsed as boolean (true/false), | ||
|
Check failure on line 278 in src/cli/tui/screens/online-eval/AddOnlineEvalScreen.tsx
|
||
| number, or string. Leave blank for no filters. | ||
| </Text> | ||
| <TextInput | ||
| key="filters" | ||
| prompt='Filters (e.g. "model Equals claude-3; latencyMs LessThan 1000")' | ||
| initialValue="" | ||
| onSubmit={value => { | ||
| const trimmed = value.trim(); | ||
| if (trimmed === '') { | ||
| wizard.setFilters(undefined); | ||
| return; | ||
| } | ||
| const parsed = parseFiltersInput(trimmed); | ||
| if (!parsed) return; | ||
| wizard.setFilters(parsed); | ||
| }} | ||
| onCancel={() => wizard.goBack()} | ||
| customValidation={value => { | ||
| const trimmed = value.trim(); | ||
| if (trimmed === '') return true; | ||
| const parsed = parseFiltersInput(trimmed); | ||
| if (!parsed) | ||
| return 'Each filter must be "<key> <operator> <value>" with a valid operator (separate with ";")'; | ||
| return true; | ||
| }} | ||
| /> | ||
| </Box> | ||
| )} | ||
|
|
||
| {isEnableOnCreateStep && ( | ||
| <WizardSelect | ||
| title="Enable on deploy?" | ||
|
|
@@ -247,6 +322,20 @@ | |
| ...(effectiveConfig.endpoint ? [{ label: 'Endpoint', value: effectiveConfig.endpoint }] : []), | ||
| { label: 'Evaluators', value: effectiveConfig.evaluators.join(', ') }, | ||
| { label: 'Sampling Rate', value: `${effectiveConfig.samplingRate}%` }, | ||
| { | ||
| label: 'Session Timeout', | ||
| value: | ||
| effectiveConfig.sessionTimeoutMinutes !== undefined | ||
| ? `${effectiveConfig.sessionTimeoutMinutes} min` | ||
| : `${DEFAULT_SESSION_TIMEOUT_MINUTES} min (default)`, | ||
| }, | ||
| { | ||
| label: 'Filters', | ||
| value: | ||
| effectiveConfig.filters && effectiveConfig.filters.length > 0 | ||
| ? effectiveConfig.filters.map(formatFilter).join('; ') | ||
| : '(none)', | ||
| }, | ||
| { label: 'Enable on Deploy', value: effectiveConfig.enableOnCreate ? 'Yes' : 'No' }, | ||
| ]} | ||
| /> | ||
|
|
@@ -255,3 +344,53 @@ | |
| </Screen> | ||
| ); | ||
| } | ||
|
|
||
| // ────────────────────────────────────────────────────────────────────────────── | ||
| // Filter parsing helpers | ||
| // ────────────────────────────────────────────────────────────────────────────── | ||
|
|
||
| function formatFilter(f: OnlineEvalFilter): string { | ||
| const v = | ||
| f.value.stringValue !== undefined | ||
| ? f.value.stringValue | ||
| : f.value.doubleValue !== undefined | ||
| ? String(f.value.doubleValue) | ||
| : f.value.booleanValue !== undefined | ||
| ? String(f.value.booleanValue) | ||
| : ''; | ||
| return `${f.key} ${f.operator} ${v}`; | ||
| } | ||
|
|
||
| /** | ||
| * Parse a filter input string such as: | ||
| * "model Equals claude-3; latencyMs LessThan 1000; success Equals true" | ||
| * Returns undefined if any segment is malformed. | ||
| */ | ||
| function parseFiltersInput(input: string): OnlineEvalFilter[] | undefined { | ||
| const segments = input | ||
| .split(';') | ||
| .map(s => s.trim()) | ||
| .filter(s => s.length > 0); | ||
| if (segments.length === 0) return undefined; | ||
|
|
||
| const filters: OnlineEvalFilter[] = []; | ||
| for (const segment of segments) { | ||
| const parts = segment.split(/\s+/); | ||
| if (parts.length < 3) return undefined; | ||
| const key = parts[0]!; | ||
| const operator = parts[1] as OnlineEvalFilterOperator; | ||
| if (!ONLINE_EVAL_FILTER_OPERATORS.includes(operator)) return undefined; | ||
| const rawValue = parts.slice(2).join(' '); | ||
|
|
||
| let value: OnlineEvalFilter['value']; | ||
| if (rawValue === 'true' || rawValue === 'false') { | ||
| value = { booleanValue: rawValue === 'true' }; | ||
| } else if (/^-?\d+(\.\d+)?$/.test(rawValue)) { | ||
| value = { doubleValue: parseFloat(rawValue) }; | ||
| } else { | ||
| value = { stringValue: rawValue }; | ||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The value auto-typing here is lossy:
Since filter keys are service-defined attributes, this will come up in real configs (e.g. a trace attribute whose value is the literal string A couple of options:
Whichever you pick, please also document the behavior in the |
||
| filters.push({ key, operator, value }); | ||
| } | ||
| return filters; | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parseFiltersInputis the whole contract for filter input — regex-based segment parsing, quoted vs bare value handling, boolean/numeric coercion, operator validation — and it's untested. Given the failure modes are silent (customValidationjust re-runs the parser), please add unit tests covering at minimum:"true","false","12345"asstringValue(the fix for the round-1 concern — worth locking in)true/false→booleanValue-?\d+(\.\d+)?→doubleValue, including negatives and decimalsstringValue(.+?)\s*$, so it's testable behavior worth pinning)undefinedundefined;and empty segments don't break parsingExtracting this into a sibling helper module (e.g.
./filter-input.ts) would make it trivial to test and keep the screen component lean.