Merge ticket 051: allow deleting a declared metric

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XFUNZwCvz3kcod3352ZR2o

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<ApiState>,
+    AxumPath(name): AxumPath<String>,
+) -> 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<String, String> {
     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<path>\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<path>\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<path>\S+)", "path", "string").await;
+        send_json(
+            &app,
+            "POST",
+            "/api/metrics",
+            json!({
+                "name": "requests",
+                "message_match_regex": r"request path=(?P<path>\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();

src/engine.rs

@@ -205,7 +205,7 @@ pub async fn run(mut rx: mpsc::Receiver<EngineMessage>, 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 } => {
@@ -516,6 +516,18 @@ fn clear_metric_data(index: &mut BTreeMap<IndexKey, DataValue>, 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<IndexKey, DataValue>, 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<MetricPrimitive> {
     match kind {
@@ -1639,6 +1651,20 @@ fn evaluate_promql(
     start_ms: Option<i64>,
     end_ms: Option<i64>,
 ) -> Result<Vec<PromqlSeriesDto>, 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)
@@ -2260,6 +2286,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();

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<duration>\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() {

tickets/051-delete-metrics.md

@@ -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).

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
           <div className="mb-3 flex flex-wrap gap-1">
             {metrics.length === 0 && <span className="text-xs text-text-faint">No metrics declared yet.</span>}
             {metrics.map((m) => (
-              <button
+              <div
                 key={m.name}
-                onClick={() => selectMetric(m)}
-                className={`rounded border px-2 py-1 text-xs ${
+                className={`flex items-center gap-0.5 rounded border pl-2 pr-1 py-1 text-xs ${
                   selected === m.name
                     ? 'border-accent bg-accent/10 text-accent-text'
                     : 'border-border-soft text-text-secondary hover:bg-surface-2'
                 }`}
-                title={m.message_match_regex}
               >
+                <button type="button" onClick={() => selectMetric(m)} title={m.message_match_regex}>
                   {m.name} ({m.label_params.join(', ') || 'no params'})
                 </button>
+                <button
+                  type="button"
+                  onClick={() => handleDelete(m.name)}
+                  className="rounded px-1 text-text-faint hover:bg-danger-bg/50 hover:text-danger-text"
+                  aria-label={`Delete metric ${m.name}`}
+                  title={`Delete metric "${m.name}"`}
+                >
+                  ×
+                </button>
+              </div>
             ))}
           </div>
 

webapp/src/lib/api.ts

@@ -305,6 +305,10 @@ export function updateMetric(existingName: string, request: MetricDeclaration):
   return putJson(`/api/metrics/${encodeURIComponent(existingName)}`, request)
 }
 
+export function deleteMetric(name: string): Promise<void> {
+  return sendJson(`/api/metrics/${encodeURIComponent(name)}`, 'DELETE')
+}
+
 export function fetchMetricBucketValues(
   name: string,
   params: {