diff --git a/src/api.rs b/src/api.rs index 89d7aea..bedb541 100644 --- a/src/api.rs +++ b/src/api.rs @@ -1527,4 +1527,52 @@ mod tests { let (_, body) = get_json(&app, "/api/logs").await; assert_eq!(body.as_array().unwrap().len(), 3); } + + /// Ticket 045: seeded fixture mixing lines that carry the sort param with lines that don't, + /// exercised through the actual HTTP endpoint rather than just `handle_query` directly. + #[tokio::test] + async fn get_logs_sort_by_orders_by_param_and_toggles_missing_param_handling() { + let (app, engine, _settings, _dir) = test_app(); + + declare_test_param(&app, "priority", r"priority=(?P\d+)", "priority", "number").await; + + ingest_lines( + &engine, + 0, + &[ + "priority=3 boom", // sequence 0, has param + "no param here", // sequence 1, missing + "priority=1 boom", // sequence 2, has param + "also missing", // sequence 3, missing + ], + ) + .await; + + // Default order (no sort_by): unchanged, plain ingest/ottyl_seq order. + let (status, body) = get_json(&app, "/api/logs").await; + assert_eq!(status, StatusCode::OK); + let sequences: Vec = body.as_array().unwrap().iter().map(|e| e["sequence"].as_u64().unwrap()).collect(); + assert_eq!(sequences, vec![0, 1, 2, 3]); + + // sort_by=priority with the fallback toggle on (the default): entries carrying the param + // sorted ascending by its value, then the missing-param entries ordered by ottyl_seq. + let (status, body) = get_json(&app, "/api/logs?sort_by=priority").await; + assert_eq!(status, StatusCode::OK); + let sequences: Vec = body.as_array().unwrap().iter().map(|e| e["sequence"].as_u64().unwrap()).collect(); + assert_eq!(sequences, vec![2, 0, 1, 3]); + + // Same sort_by, but with the fallback toggle explicitly off: missing-param lines are dropped. + let (status, body) = get_json(&app, "/api/logs?sort_by=priority&sort_missing_fallback=false").await; + assert_eq!(status, StatusCode::OK); + let sequences: Vec = body.as_array().unwrap().iter().map(|e| e["sequence"].as_u64().unwrap()).collect(); + assert_eq!(sequences, vec![2, 0]); + } + + #[tokio::test] + async fn get_logs_sort_by_unknown_param_returns_400() { + let (app, _engine, _settings, _dir) = test_app(); + let (status, body) = get_json(&app, "/api/logs?sort_by=does_not_exist").await; + assert_eq!(status, StatusCode::BAD_REQUEST); + assert!(body["error"].as_str().unwrap().contains("does_not_exist")); + } } diff --git a/src/dto.rs b/src/dto.rs index 3f010f8..4b13dde 100644 --- a/src/dto.rs +++ b/src/dto.rs @@ -78,8 +78,16 @@ pub struct LogsParams { pub start_ms: Option, pub end_ms: Option, pub limit: Option, - /// Raw FilterBar query string, e.g. `regex("error") and sequence<5`. + /// Raw FilterBar query string, e.g. `regex("error") and ottyl_seq<5`. pub filter: Option, + /// Ticket 045: an already-declared param name to order the returned entries by, in place of the + /// default ingest order (which tracks `ottyl_ts`). `None` (the default) leaves ordering + /// unchanged. + pub sort_by: Option, + /// Ticket 045: only meaningful alongside `sort_by`. `true` (the default) orders lines missing + /// that param among themselves by `ottyl_seq` and appends them after the sorted lines; `false` + /// drops those lines from the response entirely. + pub sort_missing_fallback: Option, } #[derive(Debug, Deserialize)] diff --git a/src/engine.rs b/src/engine.rs index 5ed83fd..a6794ff 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -1220,10 +1220,17 @@ fn handle_query( }, None => None, }; + // Ticket 045: `sort_by` must name an already-declared param — reject anything else up + // front rather than silently falling through to "every line is missing it". + if let Some(sort_by) = params.sort_by.as_deref() { + if !param_declarations_range(index).any(|(name, ..)| name == sort_by) { + return EngineResponse::Error(format!("unknown param '{sort_by}' for sort_by")); + } + } let limit = params.limit.unwrap_or(500).min(10_000); // O(lines_returned * declared_params) per query — fine at v1 scale; if volume/params grow, move to incremental derivation at ingest time like apply_all_metrics does for buckets. let compiled_params = compiled_param_declarations(index); - let entries: Vec = logs_range(index) + let filtered = logs_range(index) .filter(|(sequence, timestamp, _)| { params.since_sequence.is_none_or(|since| *sequence > since) && params.start_ms.is_none_or(|start| timestamp.as_millis() >= start) @@ -1233,15 +1240,50 @@ fn handle_query( compiled_filter .as_ref() .is_none_or(|filter| eval_compiled(filter, *sequence, *timestamp, text)) - }) - .take(limit) - .map(|(sequence, timestamp, text)| LogEntryDto { - sequence, - timestamp_ms: timestamp.as_millis(), - text: text.to_string(), - params: derive_log_params(text, &compiled_params), - }) - .collect(); + }); + + let entries: Vec = match params.sort_by.as_deref() { + // Default path: unchanged from pre-045 behavior — take(limit) before mapping, so + // only the entries actually returned pay the per-line param-derivation cost. + None => filtered + .take(limit) + .map(|(sequence, timestamp, text)| LogEntryDto { + sequence, + timestamp_ms: timestamp.as_millis(), + text: text.to_string(), + params: derive_log_params(text, &compiled_params), + }) + .collect(), + // An alternate sort key needs every matching line's params derived up front (there's + // no way to know which lines will end up in the first `limit` before sorting), then + // reordered before the limit is finally applied. + Some(sort_by) => { + let mut all: Vec = filtered + .map(|(sequence, timestamp, text)| LogEntryDto { + sequence, + timestamp_ms: timestamp.as_millis(), + text: text.to_string(), + params: derive_log_params(text, &compiled_params), + }) + .collect(); + let sort_missing_fallback = params.sort_missing_fallback.unwrap_or(true); + if sort_missing_fallback { + let (mut with_param, mut without_param): (Vec<_>, Vec<_>) = + all.drain(..).partition(|entry| entry.params.contains_key(sort_by)); + with_param.sort_by(|a, b| a.params[sort_by].cmp(&b.params[sort_by])); + // Lines missing the sort param are only ordered relative to each other (by + // `ottyl_seq`) and appended after the sorted lines, rather than interleaved + // into the overall ordering — see ticket 045. + without_param.sort_by_key(|entry| entry.sequence); + with_param.into_iter().chain(without_param).collect() + } else { + all.retain(|entry| entry.params.contains_key(sort_by)); + all.sort_by(|a, b| a.params[sort_by].cmp(&b.params[sort_by])); + all + } + } + }; + let entries: Vec = entries.into_iter().take(limit).collect(); EngineResponse::Logs(entries) } EngineQuery::Timeline(params) => { @@ -2972,6 +3014,21 @@ mod tests { end_ms: None, limit: None, filter: filter.map(str::to_string), + sort_by: None, + sort_missing_fallback: None, + }) + } + + /// Ticket 045: like `logs_query`, but with an alternate sort key and its missing-param handling. + fn logs_query_sorted(sort_by: &str, sort_missing_fallback: Option) -> EngineQuery { + EngineQuery::Logs(LogsParams { + since_sequence: None, + start_ms: None, + end_ms: None, + limit: None, + filter: None, + sort_by: Some(sort_by.to_string()), + sort_missing_fallback, }) } @@ -3037,6 +3094,125 @@ mod tests { } } + // -- Ticket 045: sort_by / sort_missing_fallback -- + + #[test] + fn logs_query_sort_by_orders_lines_by_declared_param_value() { + let mut index = BTreeMap::new(); + declare_test_param( + &mut index, + "priority", + r"priority=(?P\d+)", + "priority", + MetricPrimitiveKind::Number, + ); + // Ingested (and therefore default-ordered) as sequence 0, 1, 2, with priority values + // deliberately out of that order so a passing test can only mean the sort actually ran. + ingest_log(&mut index, 0, "priority=3 boom"); + ingest_log(&mut index, 1, "priority=1 boom"); + ingest_log(&mut index, 2, "priority=2 boom"); + + match handle_query(&index, logs_query_sorted("priority", None), 0) { + EngineResponse::Logs(entries) => { + let sequences: Vec = entries.iter().map(|e| e.sequence).collect(); + assert_eq!(sequences, vec![1, 2, 0]); + } + _ => panic!("expected Logs response"), + } + } + + /// Seeded fixture mixing lines that carry the sort param with lines that don't, per the ticket + /// 045 acceptance criteria: with the fallback toggle on, lines missing the param are ordered by + /// `ottyl_seq` relative to each other (and appended after the sorted lines); with it off, they're + /// excluded from the response entirely. + #[test] + fn logs_query_sort_by_missing_param_fallback_orders_by_sequence_and_appends() { + let mut index = BTreeMap::new(); + declare_test_param( + &mut index, + "priority", + r"priority=(?P\d+)", + "priority", + MetricPrimitiveKind::Number, + ); + ingest_log(&mut index, 0, "priority=3 boom"); // has param + ingest_log(&mut index, 1, "no param here"); // missing + ingest_log(&mut index, 2, "priority=1 boom"); // has param + ingest_log(&mut index, 3, "also missing"); // missing + + match handle_query(&index, logs_query_sorted("priority", Some(true)), 0) { + EngineResponse::Logs(entries) => { + let sequences: Vec = entries.iter().map(|e| e.sequence).collect(); + // Lines with the param sorted ascending by value (2 then 0), followed by the + // missing-param lines in ottyl_seq order (1 then 3). + assert_eq!(sequences, vec![2, 0, 1, 3]); + } + _ => panic!("expected Logs response"), + } + } + + #[test] + fn logs_query_sort_by_missing_param_drop_excludes_them() { + let mut index = BTreeMap::new(); + declare_test_param( + &mut index, + "priority", + r"priority=(?P\d+)", + "priority", + MetricPrimitiveKind::Number, + ); + ingest_log(&mut index, 0, "priority=3 boom"); + ingest_log(&mut index, 1, "no param here"); + ingest_log(&mut index, 2, "priority=1 boom"); + ingest_log(&mut index, 3, "also missing"); + + match handle_query(&index, logs_query_sorted("priority", Some(false)), 0) { + EngineResponse::Logs(entries) => { + let sequences: Vec = entries.iter().map(|e| e.sequence).collect(); + assert_eq!(sequences, vec![2, 0]); + } + _ => panic!("expected Logs response"), + } + } + + #[test] + fn logs_query_sort_by_unknown_param_returns_error() { + let mut index = BTreeMap::new(); + ingest_log(&mut index, 0, "hello"); + + match handle_query(&index, logs_query_sorted("does_not_exist", None), 0) { + EngineResponse::Error(err) => assert!(err.contains("does_not_exist")), + _ => panic!("expected EngineResponse::Error"), + } + } + + /// Explicit regression check for the ticket 045 acceptance criterion that default ordering + /// (no `sort_by`) is unchanged: same fixture as the fallback/drop tests above, but with + /// `sort_by: None` the response stays in plain ingest (`ottyl_seq`) order. + #[test] + fn logs_query_without_sort_by_keeps_default_ottyl_ts_order() { + let mut index = BTreeMap::new(); + declare_test_param( + &mut index, + "priority", + r"priority=(?P\d+)", + "priority", + MetricPrimitiveKind::Number, + ); + ingest_log(&mut index, 0, "priority=3 boom"); + ingest_log(&mut index, 1, "no param here"); + ingest_log(&mut index, 2, "priority=1 boom"); + ingest_log(&mut index, 3, "also missing"); + + match handle_query(&index, logs_query(None), 0) { + EngineResponse::Logs(entries) => { + let sequences: Vec = entries.iter().map(|e| e.sequence).collect(); + assert_eq!(sequences, vec![0, 1, 2, 3]); + } + _ => panic!("expected Logs response"), + } + } + #[test] fn logs_query_with_ottyl_fmt_comparison_filters_by_detected_format_for_mixed_format_lines() { let mut index = BTreeMap::new(); diff --git a/src/profile.rs b/src/profile.rs index 6096014..11e7c4a 100644 --- a/src/profile.rs +++ b/src/profile.rs @@ -1214,6 +1214,8 @@ mod tests { end_ms: None, limit: None, filter: None, + sort_by: None, + sort_missing_fallback: None, }), ) .await diff --git a/webapp/src/App.tsx b/webapp/src/App.tsx index e796a7e..97deb90 100644 --- a/webapp/src/App.tsx +++ b/webapp/src/App.tsx @@ -9,7 +9,7 @@ import { type TimeRange, type TimeRangePresetId, } from './components/LogTimeline' -import { LogPreview } from './components/LogPreview' +import { LogPreview, loadLogSortSettings, saveLogSortSettings, type LogSortSettings } from './components/LogPreview' import { Params } from './components/Params' import { Metrics } from './components/Metrics' import { Dashboards } from './components/Dashboards' @@ -260,6 +260,12 @@ function StandaloneLog() { const [activePreset, setActivePreset] = useState('all') const [filter, setFilter] = useState('') const [filterError, setFilterError] = useState(null) + const [sortSettings, setSortSettings] = useState(loadLogSortSettings) + + function handleSortSettingsChange(next: LogSortSettings) { + setSortSettings(next) + saveLogSortSettings(next) + } const rangeRef = useRef(range) useEffect(() => { @@ -310,7 +316,14 @@ function StandaloneLog() { bucketMs: effectiveRange ? computeBucketMs(effectiveRange.endMs - effectiveRange.startMs) : undefined, filter: filter || undefined, }), - fetchLogs({ startMs: effectiveRange?.startMs, endMs: effectiveRange?.endMs, limit: 1000, filter: filter || undefined }), + fetchLogs({ + startMs: effectiveRange?.startMs, + endMs: effectiveRange?.endMs, + limit: 1000, + filter: filter || undefined, + sortBy: sortSettings.sortBy ?? undefined, + sortMissingFallback: sortSettings.sortBy ? sortSettings.sortMissingFallback : undefined, + }), ]) if (!cancelled) { setBuckets(nextBuckets) @@ -331,7 +344,7 @@ function StandaloneLog() { clearInterval(id) } // eslint-disable-next-line react-hooks/exhaustive-deps - }, [filter, activePreset]) + }, [filter, activePreset, sortSettings]) return ( @@ -345,7 +358,7 @@ function StandaloneLog() { activePreset={activePreset} onSelectPreset={selectPreset} /> - + @@ -373,6 +386,12 @@ function FullApp() { const [filterError, setFilterError] = useState(null) const [logViewerCollapsed, setLogViewerCollapsed] = useSectionCollapsed('log-viewer') const [logViewerHeight, setLogViewerHeight] = useState(() => loadLogViewerHeight()) + const [sortSettings, setSortSettings] = useState(loadLogSortSettings) + + function handleSortSettingsChange(next: LogSortSettings) { + setSortSettings(next) + saveLogSortSettings(next) + } function handleLogViewerResize(deltaY: number) { setLogViewerHeight((current) => clampLogViewerHeight(current + deltaY)) @@ -442,7 +461,14 @@ function FullApp() { bucketMs: effectiveRange ? computeBucketMs(effectiveRange.endMs - effectiveRange.startMs) : undefined, filter: filter || undefined, }), - fetchLogs({ startMs: effectiveRange?.startMs, endMs: effectiveRange?.endMs, limit: 1000, filter: filter || undefined }), + fetchLogs({ + startMs: effectiveRange?.startMs, + endMs: effectiveRange?.endMs, + limit: 1000, + filter: filter || undefined, + sortBy: sortSettings.sortBy ?? undefined, + sortMissingFallback: sortSettings.sortBy ? sortSettings.sortMissingFallback : undefined, + }), ]) if (!cancelled) { setStats(nextStats) @@ -465,7 +491,7 @@ function FullApp() { } // Intentionally excludes `range` to avoid restarting this interval on every rolling-preset tick. // eslint-disable-next-line react-hooks/exhaustive-deps - }, [filter, activePreset]) + }, [filter, activePreset, sortSettings]) return ( @@ -498,7 +524,7 @@ function FullApp() { activePreset={activePreset} onSelectPreset={selectPreset} /> - + {!logViewerCollapsed && } diff --git a/webapp/src/components/LogPreview.tsx b/webapp/src/components/LogPreview.tsx index 42e6049..2ce6759 100644 --- a/webapp/src/components/LogPreview.tsx +++ b/webapp/src/components/LogPreview.tsx @@ -25,6 +25,87 @@ function saveVisibleParamColumns(columns: string[]) { } } +// Ticket 045: ordering key + missing-param handling for the log preview / timeline. Scoped like +// `VISIBLE_PARAM_COLUMNS_STORAGE_KEY` and `App.tsx`'s `LOG_VIEWER_HEIGHT_STORAGE_KEY` above — one +// shared "log viewer" concept, so a single localStorage key covers both the inline viewer +// (`FullApp`) and its popout (`StandaloneLog`) rather than each keeping its own copy, which would +// surprise a user switching between the two. There's no dashboard/panel-scoped analog for this +// setting to divide by (unlike e.g. per-dashboard panels), so a single global key matches how the +// rest of the log viewer's own settings are scoped. +const LOG_SORT_STORAGE_KEY = 'ottyl.logPreview.sort' + +export interface LogSortSettings { + /** An already-declared param name to order log lines by, or `null` for the default (`ottyl_ts`-tracking) order. */ + sortBy: string | null + /** Only meaningful when `sortBy` is set. `true` (default) orders lines missing that param by `ottyl_seq`, appended after the sorted lines; `false` drops them from view. */ + sortMissingFallback: boolean +} + +const DEFAULT_LOG_SORT_SETTINGS: LogSortSettings = { sortBy: null, sortMissingFallback: true } + +export function loadLogSortSettings(): LogSortSettings { + try { + const raw = localStorage.getItem(LOG_SORT_STORAGE_KEY) + if (!raw) return DEFAULT_LOG_SORT_SETTINGS + const parsed = JSON.parse(raw) + if (!parsed || typeof parsed !== 'object') return DEFAULT_LOG_SORT_SETTINGS + const sortBy = typeof parsed.sortBy === 'string' ? parsed.sortBy : null + const sortMissingFallback = typeof parsed.sortMissingFallback === 'boolean' ? parsed.sortMissingFallback : true + return { sortBy, sortMissingFallback } + } catch { + return DEFAULT_LOG_SORT_SETTINGS + } +} + +export function saveLogSortSettings(settings: LogSortSettings) { + try { + localStorage.setItem(LOG_SORT_STORAGE_KEY, JSON.stringify(settings)) + } catch { + // best-effort — private browsing or full storage just means the selection won't persist across reloads + } +} + +function SortPicker({ + paramNames, + settings, + onChange, +}: { + paramNames: string[] + settings: LogSortSettings + onChange: (next: LogSortSettings) => void +}) { + return ( +
+ + {settings.sortBy && ( + + )} +
+ ) +} + function ColumnPicker({ paramNames, visible, @@ -209,7 +290,18 @@ function useScrollAnchor(containerRef: React.RefObject, d }, deps) } -export function LogPreview({ logs, params }: { logs: LogEntry[]; params: ParamDeclaration[] }) { +export function LogPreview({ + logs, + params, + sortSettings, + onSortSettingsChange, +}: { + logs: LogEntry[] + params: ParamDeclaration[] + /** Ticket 045: current ordering choice, owned by the caller (it also feeds `fetchLogs`) rather than internally like `visibleParamColumns` — see `App.tsx`'s poll loop. */ + sortSettings: LogSortSettings + onSortSettingsChange: (next: LogSortSettings) => void +}) { const paramNames = params.map((d) => d.name) const [visibleParamColumns, setVisibleParamColumns] = useState(loadVisibleParamColumns) const scrollContainerRef = useRef(null) @@ -237,7 +329,10 @@ export function LogPreview({ logs, params }: { logs: LogEntry[]; params: ParamDe Log preview - +
+ + +
diff --git a/webapp/src/lib/api.ts b/webapp/src/lib/api.ts index ed1255d..73c6207 100644 --- a/webapp/src/lib/api.ts +++ b/webapp/src/lib/api.ts @@ -244,6 +244,10 @@ export function fetchLogs(params: { endMs?: number limit?: number filter?: string + /** Ticket 045: an already-declared param name to order entries by, instead of the default (`ottyl_ts`-tracking) order. */ + sortBy?: string + /** Ticket 045: only meaningful alongside `sortBy`. `true` (default) orders lines missing that param by `ottyl_seq`, appended after the sorted lines; `false` drops them. */ + sortMissingFallback?: boolean }): Promise { const search = new URLSearchParams() if (params.sinceSequence !== undefined) search.set('since_sequence', String(params.sinceSequence)) @@ -251,6 +255,8 @@ export function fetchLogs(params: { if (params.endMs !== undefined) search.set('end_ms', String(params.endMs)) if (params.limit !== undefined) search.set('limit', String(params.limit)) if (params.filter) search.set('filter', params.filter) + if (params.sortBy) search.set('sort_by', params.sortBy) + if (params.sortMissingFallback !== undefined) search.set('sort_missing_fallback', String(params.sortMissingFallback)) return getJson(`/api/logs?${search.toString()}`) }