Merge ticket/022-edit-existing-metric-declarations

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))
@@ -272,6 +291,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<ApiState>,
+    AxumPath(name): AxumPath<String>,
+    Json(body): Json<DeclareMetricRequest>,
+) -> 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<String, String> {
     raw.split(',')
@@ -906,6 +958,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<path>\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<path>\S+) duration=(?P<duration>\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<path>\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<path>\S+)",
+                "params": []
+            }),
+        )
+        .await;
+
+        let (status, body) = send_json(
+            &app,
+            "PUT",
+            "/api/metrics/requests",
+            json!({
+                "name": "requests",
+                "message_match_regex": r"request path=(?P<path>\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();

src/engine.rs

@@ -95,6 +95,12 @@ pub enum EngineMessage {
         request: DeclareMetricRequest,
         respond_to: oneshot::Sender<Result<(), String>>,
     },
+    /// 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<Result<(), String>>,
+    },
     RemoveMetricDeclaration {
         name: String,
         respond_to: oneshot::Sender<bool>,
@@ -192,6 +198,10 @@ pub async fn run(mut rx: mpsc::Receiver<EngineMessage>, 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);
@@ -235,11 +245,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<IndexKey, DataValue>,
-    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<Regex, String> {
     let re = Regex::new(&request.message_match_regex)
         .map_err(|err| format!("invalid regex: {err}"))?;
 
@@ -269,6 +276,15 @@ fn declare_metric(
         ));
     }
 
+    Ok(re)
+}
+
+fn declare_metric(
+    index: &mut BTreeMap<IndexKey, DataValue>,
+    request: DeclareMetricRequest,
+) -> Result<(), String> {
+    let re = validate_metric_request(&request)?;
+
     backfill_metric(
         index,
         &request.name,
@@ -289,6 +305,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<IndexKey, DataValue>,
+    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<IndexKey, DataValue>, 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<MetricPrimitive> {
     match kind {
@@ -1495,6 +1562,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<path>\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<path>\S+)", vec![])).unwrap();
+
+        let result = update_metric(
+            &mut index,
+            "requests",
+            metric_request(
+                "requests",
+                r"request path=(?P<path>\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<path>\S+)",
+                vec![("path", "path", MetricPrimitiveKind::String)],
+            ),
+        )
+        .unwrap();
+
+        let result = update_metric(
+            &mut index,
+            "requests",
+            metric_request(
+                "requests",
+                r"request path=(?P<path>\S+) duration=(?P<duration>\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<path>\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<path>/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<path>\S+)", vec![])).unwrap();
+        declare_metric(&mut index, metric_request("errors", r"error code=(?P<code>\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<path>\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<path>\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<IndexKey, DataValue>, sequence: u64, text: &str) {
         let timestamp = Timestamp::from_millis(sequence as i64);
         index.insert(

src/profile.rs

@@ -557,6 +557,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)?;