diff --git a/src/api.rs b/src/api.rs index 3c3a9ae..a1a155f 100644 --- a/src/api.rs +++ b/src/api.rs @@ -115,6 +115,21 @@ impl ApiState { ); } } + + /// Best-effort, matching `persist_param_declaration`'s intent — a no-op if no profile is + /// active, and I/O failures are only logged, since the engine's removal is already the source + /// of truth for what the live process shows. + fn persist_metric_declaration_removal(&self, name: &str) { + let mut settings = self.settings.lock().unwrap(); + let Some(profile_name) = settings.active_profile.clone() else { + return; + }; + if let Err(err) = settings.remove_metric_declaration(&self.settings_path, &profile_name, name) { + eprintln!( + "warning: failed to remove metric declaration '{name}' from profile '{profile_name}': {err}" + ); + } + } } pub fn router(engine: EngineHandle, settings: SettingsHandle, settings_path: PathBuf) -> Router { @@ -132,7 +147,7 @@ pub fn router(engine: EngineHandle, settings: SettingsHandle, settings_path: Pat .route("/api/metrics", get(list_metrics).post(declare_metric)) .route( "/api/metrics/{name}", - get(get_metric_bucket_values).put(update_metric), + get(get_metric_bucket_values).put(update_metric).delete(delete_metric), ) .route("/api/query", get(run_promql_query)) .route("/api/settings", get(get_settings).patch(update_settings)) @@ -395,6 +410,35 @@ async fn update_metric( } } +/// Deletes the metric declaration named by the URL path — both from the live engine index and, +/// if a profile is active, from persisted settings (ticket 051), so it doesn't reappear on +/// restart. Its recorded bucket-value history is purged along with it (see the comment on +/// `EngineMessage::RemoveMetricDeclaration`'s handler in `engine.rs`), rather than left orphaned, +/// so a dashboard panel or PromQL query still referencing the metric by name fails with a clear +/// "unknown metric" error instead of silently keeping stale data alive. +async fn delete_metric( + State(state): State, + AxumPath(name): AxumPath, +) -> impl IntoResponse { + let (respond_to, response) = oneshot::channel(); + if state + .engine + .send(EngineMessage::RemoveMetricDeclaration { name: name.clone(), respond_to }) + .await + .is_err() + { + return StatusCode::SERVICE_UNAVAILABLE.into_response(); + } + match response.await { + Ok(true) => { + state.persist_metric_declaration_removal(&name); + StatusCode::NO_CONTENT.into_response() + } + Ok(false) => StatusCode::NOT_FOUND.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(',') @@ -1106,6 +1150,112 @@ mod tests { assert!(body["error"].as_str().unwrap().contains("duration")); } + #[tokio::test] + async fn deletes_metric_and_it_disappears_from_the_list() { + let (app, _engine, _settings, _dir) = test_app(); + send_json( + &app, + "POST", + "/api/metrics", + json!({ + "name": "requests", + "message_match_regex": r"request path=(?P\S+)", + "label_params": [] + }), + ) + .await; + + let status = delete_request(&app, "/api/metrics/requests").await; + assert_eq!(status, StatusCode::NO_CONTENT); + + let (status, body) = get_json(&app, "/api/metrics").await; + assert_eq!(status, StatusCode::OK); + assert!(body.as_array().unwrap().is_empty()); + } + + #[tokio::test] + async fn deleting_unknown_metric_name_returns_404_not_500() { + let (app, _engine, _settings, _dir) = test_app(); + let status = delete_request(&app, "/api/metrics/does-not-exist").await; + assert_eq!(status, StatusCode::NOT_FOUND); + } + + /// Acceptance criterion: deleting a metric persists the removal into the active profile too, so + /// it doesn't reappear from `settings.json` on the next restart. + #[tokio::test] + async fn deleting_metric_persists_removal_and_it_does_not_reappear_after_restart() { + let (app, _engine, settings_handle, dir) = test_app(); + + let settings_path = dir.path().join("settings.json"); + { + let mut settings = settings_handle.lock().unwrap(); + settings.create_profile(&settings_path, "web".to_string()).unwrap(); + settings.set_active_profile(&settings_path, Some("web".to_string())).unwrap(); + } + + send_json( + &app, + "POST", + "/api/metrics", + json!({ + "name": "requests", + "message_match_regex": r"request path=(?P\S+)", + "label_params": [] + }), + ) + .await; + assert_eq!(Settings::load(&settings_path).profile("web").unwrap().metrics.len(), 1); + + let status = delete_request(&app, "/api/metrics/requests").await; + assert_eq!(status, StatusCode::NO_CONTENT); + + assert!(Settings::load(&settings_path).profile("web").unwrap().metrics.is_empty()); + } + + /// Acceptance criterion: a dashboard panel (PromQL query) that still references a metric by + /// name after it's been deleted fails with a readable "unknown metric" error, rather than a + /// panic or a silently-empty series — seeded here by declaring, ingesting into, deleting, then + /// re-querying the same metric name. + #[tokio::test] + async fn promql_query_against_deleted_metric_fails_with_readable_error() { + let (app, engine, _settings, _dir) = test_app(); + + declare_test_param(&app, "path", r"request path=(?P\S+)", "path", "string").await; + send_json( + &app, + "POST", + "/api/metrics", + json!({ + "name": "requests", + "message_match_regex": r"request path=(?P\S+)", + "label_params": ["path"] + }), + ) + .await; + engine + .send(EngineMessage::Ingest(IngestedLine { + sequence: 0, + timestamp: Timestamp::from_millis(0), + text: "request path=/a".to_string(), + })) + .await + .unwrap(); + + // Sanity check: the dashboard-panel-style query works before deletion. + let (status, _body) = get_json(&app, "/api/query?promql=requests").await; + assert_eq!(status, StatusCode::OK); + + let status = delete_request(&app, "/api/metrics/requests").await; + assert_eq!(status, StatusCode::NO_CONTENT); + + let (status, body) = get_json(&app, "/api/query?promql=requests").await; + assert_eq!(status, StatusCode::BAD_REQUEST); + assert!( + body["error"].as_str().unwrap().contains("unknown metric"), + "expected a readable 'unknown metric' error, got: {body}" + ); + } + #[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 a6794ff..3f0bd80 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -204,7 +204,7 @@ pub async fn run(mut rx: mpsc::Receiver, settings: SettingsHandle let _ = respond_to.send(result); } EngineMessage::RemoveMetricDeclaration { name, respond_to } => { - let existed = index.remove(&IndexKey::MetricDeclaration { name }).is_some(); + let existed = remove_metric_declaration(&mut index, &name); let _ = respond_to.send(existed); } EngineMessage::Query { query, respond_to } => { @@ -442,6 +442,18 @@ fn clear_metric_data(index: &mut BTreeMap, metric_name: &st }); } +/// Removes `name`'s declaration and, per ticket 051, purges its bucket history along with it +/// (reusing `clear_metric_data`, same as `update_metric`'s re-backfill does for the old +/// declaration) rather than leaving it orphaned — an orphaned bucket would let a stale dashboard +/// panel or PromQL query keep silently returning old data by name instead of failing with a clear +/// "unknown metric" error (see the declared-or-has-data check in `evaluate_promql`). Returns +/// whether a declaration existed, which the caller maps to 404 vs success at the API layer. +fn remove_metric_declaration(index: &mut BTreeMap, name: &str) -> bool { + let existed = index.remove(&IndexKey::MetricDeclaration { name: name.to_string() }).is_some(); + clear_metric_data(index, name); + existed +} + /// 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 { @@ -1565,6 +1577,20 @@ fn evaluate_promql( start_ms: Option, end_ms: Option, ) -> Result, String> { + // Ticket 051: a metric with neither a declaration nor any recorded bucket data is genuinely + // unknown (as opposed to a declared-but-empty-so-far metric, or an undeclared scraped + // Prometheus metric ingested via `--metrics-url`'s `IngestMetricSample`, which never gets a + // `MetricDeclaration` but still has bucket data) — most commonly because a dashboard panel + // still references a metric that has since been deleted. Failing here with a clear error beats + // a panic or a silently-empty result. + if !index.contains_key(&IndexKey::MetricDeclaration { name: parsed.metric_name.clone() }) + && bucket_values(index, &parsed.metric_name, None).next().is_none() + { + return Err(format!( + "unknown metric \"{}\": no declaration or recorded data for it (was it deleted?)", + parsed.metric_name + )); + } if matches!(parsed.agg, Some(AggOp::Sum) | Some(AggOp::Avg)) && !parsed.inner_rate && declared_value_kind(index, &parsed.metric_name) == Some(MetricPrimitiveKind::String) @@ -2098,6 +2124,64 @@ mod tests { assert!(metric_declarations_range(&index).next().is_none()); } + #[test] + fn remove_metric_declaration_reports_whether_it_existed() { + let mut index = BTreeMap::new(); + declare_requests_metric(&mut index); + assert!(remove_metric_declaration(&mut index, "requests")); + // Removing an already-gone (or never-declared) metric is a clean `false`, not a panic — + // this is what the API layer's 404 branch relies on. + assert!(!remove_metric_declaration(&mut index, "requests")); + assert!(!remove_metric_declaration(&mut index, "never-declared")); + } + + /// Ticket 051: deleting a metric purges its bucket history too, so a stale PromQL query + /// (e.g. a dashboard panel that still names the deleted metric) fails with a readable + /// "unknown metric" error instead of silently returning orphaned data. + #[test] + fn remove_metric_declaration_purges_bucket_history_so_promql_reports_unknown_metric() { + let mut index = BTreeMap::new(); + declare_requests_metric(&mut index); + ingest_log(&mut index, 0, "request path=/a"); + + let parsed = promql::parse_query("requests").unwrap(); + assert!(evaluate_promql(&index, &parsed, None, None).unwrap().len() == 1); + + assert!(remove_metric_declaration(&mut index, "requests")); + assert!(bucket_values(&index, "requests", None).next().is_none()); + + let err = evaluate_promql(&index, &parsed, None, None).unwrap_err(); + assert!(err.contains("unknown metric"), "unexpected error message: {err}"); + } + + /// A metric that was ingested via `--metrics-url` scraping (`IngestMetricSample`) never gets a + /// `MetricDeclaration` at all, so the "unknown metric" check must key off having *either* a + /// declaration *or* recorded data — not declaration alone — or every scraped metric would be + /// rejected as unknown. + #[test] + fn undeclared_metric_with_bucket_data_is_still_queryable_via_promql() { + let mut index = BTreeMap::new(); + push_bucket_value( + &mut index, + "scraped_metric", + BTreeMap::new(), + 0, + Timestamp::from_millis(0), + MetricPrimitive::Number(Number::Float(1.0)), + ); + let parsed = promql::parse_query("scraped_metric").unwrap(); + let series = evaluate_promql(&index, &parsed, None, None).unwrap(); + assert_eq!(series.len(), 1); + } + + #[test] + fn promql_rejects_metric_with_no_declaration_and_no_data() { + let index = BTreeMap::new(); + let parsed = promql::parse_query("never_declared").unwrap(); + let err = evaluate_promql(&index, &parsed, None, None).unwrap_err(); + assert!(err.contains("unknown metric"), "unexpected error message: {err}"); + } + #[test] fn update_metric_rejects_when_no_declaration_exists_yet() { let mut index = BTreeMap::new(); diff --git a/src/profile.rs b/src/profile.rs index c17e648..597263a 100644 --- a/src/profile.rs +++ b/src/profile.rs @@ -694,6 +694,27 @@ impl Settings { Ok(()) } + /// Removes `name`'s persisted metric declaration from `profile_name` (ticket 051), so it doesn't + /// reappear from `settings.json` on the next restart. A no-op (not an error) if the profile has + /// no record under that name — matches `Settings::remove_css`'s sibling calls in spirit, but + /// deleting an already-gone declaration is fine here since the caller (the API layer) has + /// already confirmed against the live engine index whether the metric existed at all. + pub fn remove_metric_declaration( + &mut self, + settings_path: &Path, + profile_name: &str, + name: &str, + ) -> Result<(), ProfileError> { + let profile = self + .profiles + .iter_mut() + .find(|p| p.name == profile_name) + .ok_or(ProfileError::NotFound)?; + profile.metrics.retain(|r| r.name != name); + self.save(settings_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.profile(profile_name).ok_or(ProfileError::NotFound)?; @@ -1036,6 +1057,28 @@ mod tests { assert_eq!(profile.metrics.len(), 1); } + #[test] + fn remove_metric_declaration_drops_it_and_does_not_reappear_after_restart() { + let dir = tempfile::tempdir().unwrap(); + let settings_path = dir.path().join("settings.json"); + let mut settings = Settings::default(); + + settings.create_profile(&settings_path, "web".to_string()).unwrap(); + let metric_req = metric_request("requests", r"duration=(?P\d+)", "duration"); + settings.record_metric_declaration(&settings_path, "web", &metric_req, &[]).unwrap(); + assert_eq!(settings.profile("web").unwrap().metrics.len(), 1); + + settings.remove_metric_declaration(&settings_path, "web", "requests").unwrap(); + assert!(settings.profile("web").unwrap().metrics.is_empty()); + + // Removing an unknown metric name is a harmless no-op, not an error. + settings.remove_metric_declaration(&settings_path, "web", "never-declared").unwrap(); + + // Reload from disk: the removal itself survives a restart (ticket 051's acceptance criterion). + let reloaded = Settings::load(&settings_path); + assert!(reloaded.profile("web").unwrap().metrics.is_empty()); + } + /// Acceptance criterion: declaring params/metrics, "restarting" (reloading settings from disk), and replaying into a fresh engine reproduces the same declarations and still ingests lines, without re-declaring by hand. #[tokio::test] async fn profile_round_trip_reproduces_declarations_in_fresh_engine() { diff --git a/tickets/051-delete-metrics.md b/tickets/051-delete-metrics.md deleted file mode 100644 index 47e38f2..0000000 --- a/tickets/051-delete-metrics.md +++ /dev/null @@ -1,64 +0,0 @@ -# 051 — Allow deleting a declared metric - -## Summary - -There is currently no way to delete a metric once declared. `src/api.rs` -only routes `GET`/`POST /api/metrics` and `GET`/`PUT /api/metrics/{name}` -(`src/api.rs:132-135`) — there is no `DELETE`. The engine already has the -machinery for it internally: `EngineMessage::RemoveMetricDeclaration` -(`src/engine.rs:103`) removes a metric's declaration from the index, but -it is only ever sent as part of `delete_profile`'s cleanup -(`src/api.rs:689`) when an entire profile is deleted, not as a -user-facing action on a single metric. Persistence has the same gap: -`Settings`/profile storage (`src/profile.rs`, post ticket 049 now inline -in `settings.json`) has `record_metric_declaration`/ -`record_metric_declaration_update` but no corresponding removal method, so -even if a metric were removed from the live engine index there'd be -nothing to stop it reappearing on next load. The webapp UI -(`webapp/src/components/Metrics.tsx`) has no delete affordance either — -compare to param declarations, which have the same underlying gap (no -`RemoveParamDeclaration`-triggering UI or persisted-removal path either), -though this ticket is scoped to metrics only. - -## Scope - -- Add `DELETE /api/metrics/{name}` in `src/api.rs`, sending - `EngineMessage::RemoveMetricDeclaration` to the engine (same message - the profile-delete cleanup path already uses) and returning 404 if the - metric doesn't exist. -- Add a persisted-removal method (e.g. `Settings::remove_metric_declaration` - or equivalent, alongside `record_metric_declaration`/ - `record_metric_declaration_update` in `src/profile.rs`/`src/settings.rs`) - so a deleted metric doesn't reappear when declarations replay from - `settings.json` on the next restart, and call it from the new route the - same way `persist_metric_declaration` is called from `declare_metric`. -- Add a delete affordance in `webapp/src/components/Metrics.tsx` (icon - button or similar, following whatever confirm-before-destructive-action - pattern already exists elsewhere in the settings/profile UI — check - `NavMenu.tsx`'s profile deletion for the existing convention) that calls - the new endpoint and removes the metric from the list on success. -- Decide and document what happens to bucket-value history already - recorded under the deleted metric's name (e.g. `MetricPrimitive` samples - indexed by metric name in the engine) — either leave it orphaned - (queryable by name won't work without a declaration, but doesn't need - active cleanup) or purge it; state the choice in the PR. -- Decide and document what happens to dashboard panels or alert rules - whose PromQL query text/filter references the deleted metric by name — - at minimum they should fail gracefully (a clear "unknown metric" error - rather than a panic or silent empty result) rather than needing to be - actively kept in sync; a full dependent-reference check/warning before - delete is out of scope unless trivial. - -## Acceptance criteria - -- A metric can be deleted via the API and the UI; it disappears from - `GET /api/metrics` immediately and does not reappear after a restart. -- Deleting an unknown metric name returns a clear 404, not a 500 or panic. -- A dashboard panel or alert rule referencing the deleted metric by name - fails with a readable error rather than crashing the engine or API, - verified with a seeded fixture. - -## Depends on - -None (touches the same engine/persistence seam as ticket 049, but that -ticket is already merged). diff --git a/webapp/src/components/Metrics.tsx b/webapp/src/components/Metrics.tsx index 05dd41c..81af441 100644 --- a/webapp/src/components/Metrics.tsx +++ b/webapp/src/components/Metrics.tsx @@ -3,6 +3,7 @@ import type { TimeRange, TimeRangePresetId } from './LogTimeline' import { ApiError, declareMetric, + deleteMetric, fetchMetricBucketValues, fetchMetricDeclarations, fetchParamDeclarations, @@ -166,6 +167,19 @@ export function Metrics({ range, activePreset }: { range: TimeRange | null; acti editMetric(metric) } + // No confirm-before-delete dialog — matches the rest of this settings UI (e.g. CSS entry / profile + // deletion in NavMenu.tsx), which also deletes directly on click rather than prompting first. + async function handleDelete(metricName: string) { + try { + await deleteMetric(metricName) + if (selected === metricName) setSelected(null) + if (editingName === metricName) resetForm() + await loadMetrics() + } catch { + // best-effort; the declared-metrics list simply keeps showing it if the delete failed + } + } + async function handleSubmit(e: React.FormEvent) { e.preventDefault() setSubmitError(null) @@ -312,18 +326,27 @@ export function Metrics({ range, activePreset }: { range: TimeRange | null; acti
{metrics.length === 0 && No metrics declared yet.} {metrics.map((m) => ( - + + +
))} diff --git a/webapp/src/lib/api.ts b/webapp/src/lib/api.ts index 7832faf..c905bbf 100644 --- a/webapp/src/lib/api.ts +++ b/webapp/src/lib/api.ts @@ -300,6 +300,10 @@ export function updateMetric(existingName: string, request: MetricDeclaration): return putJson(`/api/metrics/${encodeURIComponent(existingName)}`, request) } +export function deleteMetric(name: string): Promise { + return sendJson(`/api/metrics/${encodeURIComponent(name)}`, 'DELETE') +} + export function fetchMetricBucketValues( name: string, params: {