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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XFUNZwCvz3kcod3352ZR2o
This commit is contained in:
Hugh Kaznowski
2026-09-05 19:38:00 +01:00
co-authored by Claude Sonnet 5
parent e6e5bc8f50
commit 56da086c7a
5 changed files with 467 additions and 21 deletions
+146 -1
View File
@@ -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<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(',')
@@ -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<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();
+192 -5
View File
@@ -91,6 +91,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>,
@@ -188,6 +194,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);
@@ -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<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}"))?;
@@ -257,6 +264,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,
@@ -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<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 {
@@ -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<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(
+25
View File
@@ -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)?;
+71 -9
View File
@@ -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<string | null>(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<string | null>(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 (
<div className="border-b border-border text-sm">
@@ -240,14 +265,31 @@ export function Metrics({ range }: { range: TimeRange | null }) {
))}
</div>
</div>
{editingName && (
<div className="rounded border border-accent bg-accent/5 px-2 py-1 text-xs text-accent-text">
Editing "{editingName}"
</div>
)}
{submitError && <div className="rounded border border-danger-border bg-danger-bg/50 px-2 py-1 text-xs text-danger-text">{submitError}</div>}
<div className="flex items-center gap-2">
<button
type="submit"
disabled={submitting}
className="rounded border border-border-soft px-3 py-1 text-sm text-text-secondary hover:bg-surface-2 disabled:opacity-50"
>
{submitting ? 'Declaring…' : 'Declare metric'}
{submitting ? (editingName ? 'Saving…' : 'Declaring…') : editingName ? 'Save changes' : 'Declare metric'}
</button>
{editingName && (
<button
type="button"
onClick={resetForm}
disabled={submitting}
className="rounded border border-border-soft px-3 py-1 text-sm text-text-secondary hover:bg-surface-2 disabled:opacity-50"
>
Cancel
</button>
)}
</div>
</form>
<div className="min-w-0 flex-1">
@@ -257,7 +299,7 @@ export function Metrics({ range }: { range: TimeRange | null }) {
{metrics.map((m) => (
<button
key={m.name}
onClick={() => setSelected(m.name)}
onClick={() => selectMetric(m)}
className={`rounded border px-2 py-1 text-xs ${
selected === m.name
? 'border-accent bg-accent/10 text-accent-text'
@@ -275,6 +317,26 @@ export function Metrics({ range }: { range: TimeRange | null }) {
<div className="mb-1 text-xs text-text-subtle">
{selected} — {bucketValues.length} occurrence{bucketValues.length === 1 ? '' : 's'} in range
</div>
{selectedMetric && (
<div className="mb-2 space-y-1 rounded border border-border-soft bg-surface-1 px-2 py-1.5 text-xs text-text-secondary">
<div>
<span className="text-text-subtle">Regex: </span>
<span className="font-mono">{selectedMetric.message_match_regex}</span>
</div>
<div>
<span className="text-text-subtle">Params: </span>
{selectedMetric.params.length === 0 ? (
<span className="text-text-faint">none</span>
) : (
<span className="font-mono">
{selectedMetric.params
.map((p) => `${p.param_name}←${p.capture_group} (${p.kind}, ${p.role ?? 'label'})`)
.join(', ')}
</span>
)}
</div>
</div>
)}
{groups.length === 0 ? (
<div className="text-xs text-text-faint">No matching values in the active range.</div>
) : (
+27
View File
@@ -200,6 +200,28 @@ async function postJson<T>(path: string, body: unknown): Promise<T> {
return res.json() as Promise<T>
}
// Same error handling as `postJson` (throws `ApiError` with the parsed message) but for a PUT — used where updating in place, rather than creating, is the operation.
async function putJson<T>(path: string, body: unknown): Promise<T> {
const res = await fetch(path, {
method: 'PUT',
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify(body),
})
if (!res.ok) {
let message = `${path} failed: ${res.status}`
try {
const errorBody = (await res.json()) as Partial<ApiErrorBody>
if (errorBody && typeof errorBody.error === 'string') {
message = errorBody.error
}
} catch {
// response body wasn't JSON; fall back to the generic message
}
throw new ApiError(message)
}
return res.json() as Promise<T>
}
async function sendJson<T>(path: string, method: string, body?: unknown): Promise<T> {
const res = await fetch(path, {
method,
@@ -270,6 +292,11 @@ export function declareMetric(request: MetricDeclaration): Promise<MetricDeclara
return postJson('/api/metrics', request)
}
// Updates the metric declaration named `existingName` in place (`existingName` may differ from `request.name` to rename it), rather than declaring a second metric or failing on a duplicate-name conflict.
export function updateMetric(existingName: string, request: MetricDeclaration): Promise<MetricDeclaration> {
return putJson(`/api/metrics/${encodeURIComponent(existingName)}`, request)
}
export function fetchMetricBucketValues(
name: string,
params: {