From 56da086c7a3abaa1c34db531bff53d391aaa158c Mon Sep 17 00:00:00 2001 From: Hugh Kaznowski Date: Sat, 5 Sep 2026 19:38:00 +0100 Subject: [PATCH] Allow editing an existing metric declaration in place (ticket 022) Clicking a declared metric badge in Metrics.tsx now loads its regex/params into the declaration form for editing (in addition to selecting it for the bucket-values view), and shows those properties inline once selected rather than only on hover. The backend gains PUT /api/metrics/{name}, backed by a new engine::update_metric that reuses declare_metric's validation, requires the metric to already exist, supports renaming, and re-backfills bucket data from the new regex/params instead of leaving stale buckets from the old one. Submitting the form while editing calls the update endpoint in place rather than creating a second declaration; declaring a brand-new metric is unchanged. Note: tickets/022-edit-existing-metric-declarations.md does not exist on this worktree's branch (it was added to tickets/ on main after this branch was created), so there is nothing to delete here. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01XFUNZwCvz3kcod3352ZR2o --- src/api.rs | 147 +++++++++++++++++++++- src/engine.rs | 197 +++++++++++++++++++++++++++++- src/profile.rs | 25 ++++ webapp/src/components/Metrics.tsx | 92 +++++++++++--- webapp/src/lib/api.ts | 27 ++++ 5 files changed, 467 insertions(+), 21 deletions(-) diff --git a/src/api.rs b/src/api.rs index 7601118..e3c14f2 100644 --- a/src/api.rs +++ b/src/api.rs @@ -69,6 +69,22 @@ impl ApiState { } } + /// Same as `persist_metric_declaration`, for an in-place edit (`PUT /api/metrics/{name}`) — also drops the persisted record under `old_name` when the edit renames the metric, so a rename doesn't leave a stale duplicate declaration in the profile file. + fn persist_metric_declaration_update(&self, old_name: &str, request: &DeclareMetricRequest) { + let Some((store_path, profile_name)) = self.active_profile_location() else { + return; + }; + let mut store = ProfileStore::load(&store_path); + if let Err(err) = + store.record_metric_declaration_update(&store_path, &profile_name, old_name, request) + { + eprintln!( + "warning: failed to persist metric declaration update '{}' into profile '{profile_name}': {err}", + request.name + ); + } + } + fn active_profile_location(&self) -> Option<(PathBuf, String)> { let settings = self.settings.lock().unwrap(); let profile_name = settings.profiles.active_profile.clone()?; @@ -90,7 +106,10 @@ pub fn router(engine: EngineHandle, settings: SettingsHandle, settings_path: Pat .route("/api/stats", get(get_stats)) .route("/api/params", get(list_params).post(declare_param)) .route("/api/metrics", get(list_metrics).post(declare_metric)) - .route("/api/metrics/{name}", get(get_metric_bucket_values)) + .route( + "/api/metrics/{name}", + get(get_metric_bucket_values).put(update_metric), + ) .route("/api/query", get(run_promql_query)) .route("/api/settings", get(get_settings).patch(update_settings)) .route("/api/settings/css", get(list_css).post(import_css)) @@ -271,6 +290,39 @@ async fn declare_metric( } } +/// Updates the metric declaration named by the URL path in place, reusing `declare_metric`'s validation (via `engine::update_metric`) rather than creating a second declaration or erroring on a duplicate-name conflict. `body.name` may differ from the path segment to rename the metric, so long as no other declaration already uses the new name. +async fn update_metric( + State(state): State, + AxumPath(name): AxumPath, + Json(body): Json, +) -> impl IntoResponse { + let (respond_to, response) = oneshot::channel(); + let request = body.clone(); + if state + .engine + .send(EngineMessage::UpdateMetric { name: name.clone(), request: body, respond_to }) + .await + .is_err() + { + return StatusCode::SERVICE_UNAVAILABLE.into_response(); + } + match response.await { + Ok(Ok(())) => { + state.persist_metric_declaration_update(&name, &request); + let dto = MetricDeclarationDto { + name: request.name, + message_match_regex: request.message_match_regex, + params: request.params, + matched_labels: request.matched_labels, + unmatched_labels: request.unmatched_labels, + }; + (StatusCode::OK, Json(dto)).into_response() + } + Ok(Err(err)) => (StatusCode::BAD_REQUEST, Json(ErrorDto { error: err })).into_response(), + Err(_) => StatusCode::SERVICE_UNAVAILABLE.into_response(), + } +} + /// Malformed pairs (no `=`) are skipped rather than rejected. fn parse_labels_filter(raw: &str) -> BTreeMap { raw.split(',') @@ -905,6 +957,99 @@ mod tests { assert!(body["error"].as_str().unwrap().contains("duration")); } + #[tokio::test] + async fn updates_metric_declaration_in_place_rather_than_duplicating() { + let (app, _engine, _settings, _dir) = test_app(); + send_json( + &app, + "POST", + "/api/metrics", + json!({ + "name": "requests", + "message_match_regex": r"request path=(?P\S+)", + "params": [ + { "param_name": "path", "capture_group": "path", "kind": "string" } + ] + }), + ) + .await; + + let (status, body) = send_json( + &app, + "PUT", + "/api/metrics/requests", + json!({ + "name": "requests", + "message_match_regex": r"request path=(?P\S+) duration=(?P\d+)", + "params": [ + { "param_name": "path", "capture_group": "path", "kind": "string" }, + { "param_name": "duration", "capture_group": "duration", "kind": "number" } + ] + }), + ) + .await; + assert_eq!(status, StatusCode::OK); + assert_eq!(body["name"], "requests"); + assert_eq!(body["params"].as_array().unwrap().len(), 2); + + // Still exactly one declaration named "requests", now with the updated regex/params — not a second declaration alongside the original. + let (status, body) = get_json(&app, "/api/metrics").await; + assert_eq!(status, StatusCode::OK); + let metrics = body.as_array().unwrap(); + assert_eq!(metrics.len(), 1); + assert_eq!(metrics[0]["params"].as_array().unwrap().len(), 2); + } + + #[tokio::test] + async fn update_metric_rejects_unknown_metric_name() { + let (app, _engine, _settings, _dir) = test_app(); + let (status, body) = send_json( + &app, + "PUT", + "/api/metrics/does-not-exist", + json!({ + "name": "does-not-exist", + "message_match_regex": r"request path=(?P\S+)", + "params": [] + }), + ) + .await; + assert_eq!(status, StatusCode::BAD_REQUEST); + assert!(body["error"].as_str().unwrap().contains("does-not-exist")); + } + + #[tokio::test] + async fn update_metric_reuses_declare_metric_validation() { + let (app, _engine, _settings, _dir) = test_app(); + send_json( + &app, + "POST", + "/api/metrics", + json!({ + "name": "requests", + "message_match_regex": r"request path=(?P\S+)", + "params": [] + }), + ) + .await; + + let (status, body) = send_json( + &app, + "PUT", + "/api/metrics/requests", + json!({ + "name": "requests", + "message_match_regex": r"request path=(?P\S+)", + "params": [ + { "param_name": "duration", "capture_group": "duration", "kind": "number" } + ] + }), + ) + .await; + assert_eq!(status, StatusCode::BAD_REQUEST); + assert!(body["error"].as_str().unwrap().contains("duration")); + } + #[tokio::test] async fn queries_bucket_values_with_label_filter_and_range() { let (app, engine, _settings, _dir) = test_app(); diff --git a/src/engine.rs b/src/engine.rs index d9b11ee..d3bc796 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -91,6 +91,12 @@ pub enum EngineMessage { request: DeclareMetricRequest, respond_to: oneshot::Sender>, }, + /// Updates an existing metric declaration named `name` in place — same validation as `DeclareMetric`, but fails if no declaration named `name` exists (rather than silently creating one), and re-backfills bucket data from scratch so edited params/regex don't leave stale buckets from the old declaration mixed in. + UpdateMetric { + name: String, + request: DeclareMetricRequest, + respond_to: oneshot::Sender>, + }, RemoveMetricDeclaration { name: String, respond_to: oneshot::Sender, @@ -188,6 +194,10 @@ pub async fn run(mut rx: mpsc::Receiver, settings: SettingsHandle let result = declare_metric(&mut index, request); let _ = respond_to.send(result); } + EngineMessage::UpdateMetric { name, request, respond_to } => { + let result = update_metric(&mut index, &name, request); + let _ = respond_to.send(result); + } EngineMessage::RemoveMetricDeclaration { name, respond_to } => { let existed = index.remove(&IndexKey::MetricDeclaration { name }).is_some(); let _ = respond_to.send(existed); @@ -225,11 +235,8 @@ fn declare_param( Ok(()) } -/// Validates every param matcher's capture-group reference (not just the first bad one) so a rejection reports every problem at once. -fn declare_metric( - index: &mut BTreeMap, - request: DeclareMetricRequest, -) -> Result<(), String> { +/// Validates every param matcher's capture-group reference (not just the first bad one) so a rejection reports every problem at once, and the at-most-one-Value-param rule. Shared by `declare_metric` and `update_metric` so both reject a bad declaration the same way. +fn validate_metric_request(request: &DeclareMetricRequest) -> Result { let re = Regex::new(&request.message_match_regex) .map_err(|err| format!("invalid regex: {err}"))?; @@ -257,6 +264,15 @@ fn declare_metric( )); } + Ok(re) +} + +fn declare_metric( + index: &mut BTreeMap, + request: DeclareMetricRequest, +) -> Result<(), String> { + let re = validate_metric_request(&request)?; + backfill_metric( index, &request.name, @@ -277,6 +293,57 @@ fn declare_metric( Ok(()) } +/// Updates the metric declaration named `existing_name` in place, reusing `declare_metric`'s validation so an edit is rejected the same way a bad new declaration would be. Fails (rather than creating a fresh declaration) if no metric named `existing_name` exists yet. Existing bucket/last-event data for this metric is cleared first and rebuilt from the full log history under the new regex/params, so edited matchers don't leave stale buckets from the previous declaration mixed in with fresh ones. Supports renaming (when `request.name` differs from `existing_name`) as long as the new name isn't already taken by another declaration. +fn update_metric( + index: &mut BTreeMap, + existing_name: &str, + request: DeclareMetricRequest, +) -> Result<(), String> { + if !index.contains_key(&IndexKey::MetricDeclaration { name: existing_name.to_string() }) { + return Err(format!("no metric declaration named '{existing_name}'")); + } + if request.name != existing_name + && index.contains_key(&IndexKey::MetricDeclaration { name: request.name.clone() }) + { + return Err(format!("a metric named '{}' already exists", request.name)); + } + + let re = validate_metric_request(&request)?; + + clear_metric_data(index, existing_name); + if request.name != existing_name { + index.remove(&IndexKey::MetricDeclaration { name: existing_name.to_string() }); + } + + backfill_metric( + index, + &request.name, + &re, + &request.params, + request.matched_labels.as_ref(), + request.unmatched_labels.as_ref(), + ); + index.insert( + IndexKey::MetricDeclaration { name: request.name.clone() }, + DataValue::MetricDeclaration { + message_match_regex: request.message_match_regex, + params: request.params, + matched_labels: request.matched_labels, + unmatched_labels: request.unmatched_labels, + }, + ); + Ok(()) +} + +/// Removes every `Bucket` and `LastMetricEvent` entry belonging to `metric_name`, ahead of a full re-backfill in `update_metric` — otherwise the old declaration's buckets would sit alongside freshly backfilled ones under the same metric name. +fn clear_metric_data(index: &mut BTreeMap, metric_name: &str) { + index.retain(|key, _| match key { + IndexKey::Bucket { metric_name: name, .. } => name != metric_name, + IndexKey::LastMetricEvent { metric_name: name, .. } => name != metric_name, + _ => true, + }); +} + /// Returns `None` on a parse failure, which the caller treats as "this match doesn't apply". fn parse_metric_primitive(raw: &str, kind: MetricPrimitiveKind) -> Option { match kind { @@ -1319,6 +1386,126 @@ mod tests { assert!(metric_declarations_range(&index).next().is_none()); } + #[test] + fn update_metric_rejects_when_no_declaration_exists_yet() { + let mut index = BTreeMap::new(); + let result = update_metric( + &mut index, + "requests", + metric_request("requests", r"request path=(?P\S+)", vec![]), + ); + assert!(result.is_err()); + assert!(result.unwrap_err().contains("requests")); + } + + #[test] + fn update_metric_reuses_declare_metric_validation() { + let mut index = BTreeMap::new(); + declare_metric(&mut index, metric_request("requests", r"request path=(?P\S+)", vec![])).unwrap(); + + let result = update_metric( + &mut index, + "requests", + metric_request( + "requests", + r"request path=(?P\S+)", + vec![("duration", "duration", MetricPrimitiveKind::Number)], + ), + ); + assert!(result.is_err()); + assert!(result.unwrap_err().contains("duration")); + } + + #[test] + fn update_metric_replaces_declaration_in_place_without_duplicating() { + let mut index = BTreeMap::new(); + declare_metric( + &mut index, + metric_request( + "requests", + r"request path=(?P\S+)", + vec![("path", "path", MetricPrimitiveKind::String)], + ), + ) + .unwrap(); + + let result = update_metric( + &mut index, + "requests", + metric_request( + "requests", + r"request path=(?P\S+) duration=(?P\d+)", + vec![ + ("path", "path", MetricPrimitiveKind::String), + ("duration", "duration", MetricPrimitiveKind::Number), + ], + ), + ); + assert!(result.is_ok()); + + // Still one declaration named "requests", now with the updated regex/params. + let declarations: Vec<_> = metric_declarations_range(&index).collect(); + assert_eq!(declarations.len(), 1); + assert_eq!(declarations[0].0, "requests"); + assert_eq!(declarations[0].2.len(), 2); + } + + #[test] + fn update_metric_rebackfills_bucket_data_from_the_new_declaration() { + let mut index = BTreeMap::new(); + ingest_log(&mut index, 0, "request path=/a"); + ingest_log(&mut index, 1, "request path=/b"); + declare_metric( + &mut index, + metric_request( + "requests", + r"request path=(?P\S+)", + vec![("path", "path", MetricPrimitiveKind::String)], + ), + ) + .unwrap(); + assert_eq!(bucket_values(&index, "requests", None).count(), 2); + + // Editing to a narrower regex should leave exactly the buckets matching the *new* declaration — not the old ones layered underneath. + update_metric( + &mut index, + "requests", + metric_request( + "requests", + r"request path=(?P/a)", + vec![("path", "path", MetricPrimitiveKind::String)], + ), + ) + .unwrap(); + assert_eq!(bucket_values(&index, "requests", None).count(), 1); + } + + #[test] + fn update_metric_can_rename_and_rejects_a_rename_onto_an_existing_name() { + let mut index = BTreeMap::new(); + declare_metric(&mut index, metric_request("requests", r"request path=(?P\S+)", vec![])).unwrap(); + declare_metric(&mut index, metric_request("errors", r"error code=(?P\d+)", vec![])).unwrap(); + + // Renaming onto an already-taken name is rejected. + let result = update_metric( + &mut index, + "requests", + metric_request("errors", r"request path=(?P\S+)", vec![]), + ); + assert!(result.is_err()); + + // Renaming onto a fresh name succeeds and drops the old declaration. + update_metric( + &mut index, + "requests", + metric_request("http_requests", r"request path=(?P\S+)", vec![]), + ) + .unwrap(); + let names: Vec<_> = metric_declarations_range(&index).map(|(name, ..)| name.to_string()).collect(); + assert!(names.contains(&"http_requests".to_string())); + assert!(!names.contains(&"requests".to_string())); + } + fn ingest_log(index: &mut BTreeMap, sequence: u64, text: &str) { let timestamp = Timestamp::from_millis(sequence as i64); index.insert( diff --git a/src/profile.rs b/src/profile.rs index f8fb82c..a31f119 100644 --- a/src/profile.rs +++ b/src/profile.rs @@ -541,6 +541,31 @@ impl ProfileStore { Ok(()) } + /// Same as `record_metric_declaration`, for an in-place edit that may rename the metric: drops any persisted record under `old_name` first (when it differs from the new name) so a rename doesn't leave a stale duplicate declaration behind, then upserts by the new name as usual. + pub fn record_metric_declaration_update( + &mut self, + store_path: &Path, + profile_name: &str, + old_name: &str, + request: &DeclareMetricRequest, + ) -> Result<(), ProfileError> { + let profile = self + .profiles + .iter_mut() + .find(|p| p.name == profile_name) + .ok_or(ProfileError::NotFound)?; + if old_name != request.name { + profile.metrics.retain(|r| r.name != old_name); + } + let record: MetricDeclarationRecord = request.into(); + match profile.metrics.iter_mut().find(|r| r.name == record.name) { + Some(existing) => *existing = record, + None => profile.metrics.push(record), + } + self.save(store_path)?; + Ok(()) + } + /// Lists the dashboards saved on `profile_name`; fails with `NotFound` if it doesn't exist. pub fn list_dashboards(&self, profile_name: &str) -> Result<&[Dashboard], ProfileError> { let profile = self.get(profile_name).ok_or(ProfileError::NotFound)?; diff --git a/webapp/src/components/Metrics.tsx b/webapp/src/components/Metrics.tsx index bc18408..d6f5138 100644 --- a/webapp/src/components/Metrics.tsx +++ b/webapp/src/components/Metrics.tsx @@ -6,6 +6,7 @@ import { fetchMetricBucketValues, fetchMetricDeclarations, metricPrimitiveToDisplayString, + updateMetric, type BucketValue, type MetricDeclaration, type MetricParamMatcher, @@ -80,6 +81,8 @@ export function Metrics({ range }: { range: TimeRange | null }) { const [submitError, setSubmitError] = useState(null) const [submitting, setSubmitting] = useState(false) const [collapsed, setCollapsed] = useSectionCollapsed('metrics') + // Name of the declaration currently loaded into the form for editing, or `null` when the form is declaring a brand-new metric. Kept separate from `selected` (which drives the bucket-values view below) so clicking a badge can populate the form *and* select it for the values table at the same time, without either hiding the other. + const [editingName, setEditingName] = useState(null) async function loadMetrics() { try { @@ -125,29 +128,51 @@ export function Metrics({ range }: { range: TimeRange | null }) { setMatchers((prev) => prev.filter((_, i) => i !== index)) } + function resetForm() { + setEditingName(null) + setName('') + setRegex('') + setMatchers([{ ...EMPTY_MATCHER }]) + setSubmitError(null) + } + + // Loads `metric`'s current values into the declaration form for editing, in addition to (not instead of) selecting it for the bucket-values view below — both a click handler for a declared-metric badge, so the two behaviors are always available together. + function editMetric(metric: MetricDeclaration) { + setEditingName(metric.name) + setName(metric.name) + setRegex(metric.message_match_regex) + setMatchers(metric.params.length > 0 ? metric.params.map((p) => ({ ...p })) : [{ ...EMPTY_MATCHER }]) + setSubmitError(null) + } + + function selectMetric(metric: MetricDeclaration) { + setSelected(metric.name) + editMetric(metric) + } + async function handleSubmit(e: React.FormEvent) { e.preventDefault() setSubmitError(null) setSubmitting(true) try { - const declared = await declareMetric({ + const request: MetricDeclaration = { name, message_match_regex: regex, params: matchers.filter((m) => m.param_name && m.capture_group), - }) - setName('') - setRegex('') - setMatchers([{ ...EMPTY_MATCHER }]) + } + const saved = editingName ? await updateMetric(editingName, request) : await declareMetric(request) + resetForm() await loadMetrics() - setSelected(declared.name) + setSelected(saved.name) } catch (err) { - setSubmitError(err instanceof ApiError ? err.message : 'Failed to declare metric') + setSubmitError(err instanceof ApiError ? err.message : `Failed to ${editingName ? 'update' : 'declare'} metric`) } finally { setSubmitting(false) } } const groups = groupByLabels(bucketValues) + const selectedMetric = metrics.find((m) => m.name === selected) ?? null return (
@@ -240,14 +265,31 @@ export function Metrics({ range }: { range: TimeRange | null }) { ))}
+ {editingName && ( +
+ Editing "{editingName}" +
+ )} {submitError &&
{submitError}
} - +
+ + {editingName && ( + + )} +
@@ -257,7 +299,7 @@ export function Metrics({ range }: { range: TimeRange | null }) { {metrics.map((m) => (