phughk/ottyl
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 1 Wiki Activity

Unify param declarations: metrics reference params by name (ticket 037)

Browse Source 
Collapse the Params-panel ParamDeclaration and the metric-embedded
RegexToMetricPrimitiveMatcher into a single ParamDeclaration type
(name, regex/capture-group-or-json-field-path, kind). MetricDeclaration
now references params by name via label_params: Vec<String> and
value_param: Option<String> instead of embedding its own extractor per
param, so a param declared once in the Params panel can be reused as a
label or value by any number of metrics. Declaring a metric that
references an undeclared param name is now rejected at declare-time.

Backend:
- src/data_value.rs: unified ParamDeclaration struct; MetricDeclaration
  data value now carries label_params/value_param instead of a Vec of
  inline matchers.
- src/engine.rs: DeclareParamRequest is now an alias for
  ParamDeclaration; DeclareMetricRequest carries label_params/
  value_param; validate_metric_request checks referenced param names
  against the declared-param store; apply_metric_to_log resolves
  label_params/value_param against a per-ingest resolved-param map
  instead of reading an inline matcher list.
- src/profile.rs: MetricDeclarationRecord carries label_params/
  value_param plus a private, serialize-skipped legacy_params field
  that only ever gets populated by deserializing a pre-037 profile
  file's old "params" key. ProfileStore::load migrates any such legacy
  metric into the unified shape (reusing an existing same-shaped bare
  param by name, or creating one implicitly named
  "<metric_name>.<param_name>"), then persists the migrated store so
  the on-disk file only carries the legacy shape once. Migration is
  idempotent (deterministic naming) and covered by dedicated tests.
- src/api.rs / src/dto.rs: MetricDeclarationDto and the declare/update
  metric response bodies updated to the new shape.

Frontend:
- webapp/src/lib/api.ts: MetricDeclaration type now has label_params/
  value_param; MetricParamMatcher/ParamRole types removed.
- webapp/src/components/Metrics.tsx: the declaration form now fetches
  declared params and lets you check off label params / pick a value
  param from that list, instead of re-entering a regex/capture-group/
  kind per metric param.

Leaves ticket 038 (duration as a derived param kind) and 039 (param
query resolving declared params) untouched, as scoped.

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 22:41:14 +01:00
co-authored by Claude Sonnet 5
parent 85fb145be7
commit e397f9fbdb
7 changed files with 755 additions and 594 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/profile.rs
+304 -69
View File
@@ -13,7 +13,7 @@ use std::time::{SystemTime, UNIX_EPOCH};
use serde::{Deserialize, Serialize};
use crate::data_value::{MetricPrimitiveKind, ParamRole, RegexToMetricPrimitiveMatcher};
use crate::data_value::MetricPrimitiveKind;
use crate::engine::{DeclareMetricRequest, DeclareParamRequest, EngineHandle, EngineMessage};
use crate::settings::Settings;
use tokio::sync::oneshot;
@@ -58,13 +58,13 @@ impl From<ProfileMetricPrimitiveKind> for MetricPrimitiveKind {
}
}
/// A stored param declaration, matching `DeclareParamRequest`'s shape.
/// A stored param declaration, matching `DeclareParamRequest`'s shape (== `data_value::ParamDeclaration`).
#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
pub struct ParamDeclarationRecord {
pub name: String,
pub message_match_regex: String,
pub capture_group: String,
/// See `crate::data_value::RegexToMetricPrimitiveMatcher::json_field_path`; `#[serde(default)]`
/// See `crate::data_value::ParamDeclaration::json_field_path`; `#[serde(default)]`
/// so profiles saved before this field existed still load as plain regex-capture-group params.
#[serde(default)]
pub json_field_path: Option<String>,
@@ -95,33 +95,56 @@ impl From<&ParamDeclarationRecord> for DeclareParamRequest {
}
}
/// A stored metric param matcher, matching `RegexToMetricPrimitiveMatcher`'s shape.
#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
pub struct MetricParamRecord {
pub param_name: String,
pub capture_group: String,
/// See `crate::data_value::RegexToMetricPrimitiveMatcher::json_field_path`; `#[serde(default)]`
/// so profiles saved before this field existed still load as plain regex-capture-group matchers.
#[serde(default)]
pub json_field_path: Option<String>,
pub kind: ProfileMetricPrimitiveKind,
/// See `ParamRole`; `#[serde(default)]` so profiles saved before this field existed still load, defaulting to `Label`.
#[serde(default)]
pub role: ParamRole,
/// Pre-037 role of an inline metric param matcher — a label identifying a bucket, or the (at most
/// one) value that becomes the event's captured value. Kept only so `LegacyMetricParamRecord` can
/// still deserialize a profile saved before params were unified into standalone `ParamDeclaration`s
/// referenced by name; see `migrate_legacy_metric_params`.
#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, Default)]
#[serde(rename_all = "lowercase")]
enum LegacyParamRole {
#[default]
Label,
Value,
}
/// A stored metric declaration, matching `DeclareMetricRequest`'s shape.
/// Pre-037 inline metric param matcher shape (formerly `data_value::RegexToMetricPrimitiveMatcher`),
/// kept only to support migrating profiles saved before ticket 037 unified params — see
/// `migrate_legacy_metric_params`. Never produced when serializing a record built from a
/// `DeclareMetricRequest`.
#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
struct LegacyMetricParamRecord {
param_name: String,
capture_group: String,
#[serde(default)]
json_field_path: Option<String>,
kind: ProfileMetricPrimitiveKind,
#[serde(default)]
role: LegacyParamRole,
}
/// A stored metric declaration, matching `DeclareMetricRequest`'s shape. `legacy_params` is only
/// ever populated by deserializing a pre-037 profile file (old key name `"params"`) — see
/// `migrate_legacy_metric_params`, which converts it into `label_params`/`value_param` plus any
/// implicitly-created standalone params, and empties it back out so a subsequent save never writes
/// the old shape again.
#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
pub struct MetricDeclarationRecord {
pub name: String,
pub message_match_regex: String,
pub params: Vec<MetricParamRecord>,
/// Names of standalone `ParamDeclarationRecord`s used as this metric's bucket labels.
#[serde(default)]
pub label_params: Vec<String>,
/// Name of the standalone `ParamDeclarationRecord` (if any) whose value becomes this metric's bucket value.
#[serde(default)]
pub value_param: Option<String>,
/// See `crate::engine::DeclareMetricRequest::matched_labels`; `#[serde(default)]` so older profiles still load.
#[serde(default)]
pub matched_labels: Option<std::collections::BTreeMap<String, String>>,
/// See `crate::engine::DeclareMetricRequest::unmatched_labels`; `#[serde(default)]` so older profiles still load.
#[serde(default)]
pub unmatched_labels: Option<std::collections::BTreeMap<String, String>>,
#[serde(default, rename = "params", skip_serializing_if = "Vec::is_empty")]
legacy_params: Vec<LegacyMetricParamRecord>,
}
impl From<&DeclareMetricRequest> for MetricDeclarationRecord {
@@ -129,19 +152,11 @@ impl From<&DeclareMetricRequest> for MetricDeclarationRecord {
MetricDeclarationRecord {
name: request.name.clone(),
message_match_regex: request.message_match_regex.clone(),
params: request
.params
.iter()
.map(|matcher| MetricParamRecord {
param_name: matcher.param_name.clone(),
capture_group: matcher.capture_group.clone(),
json_field_path: matcher.json_field_path.clone(),
kind: matcher.kind.into(),
role: matcher.role,
})
.collect(),
label_params: request.label_params.clone(),
value_param: request.value_param.clone(),
matched_labels: request.matched_labels.clone(),
unmatched_labels: request.unmatched_labels.clone(),
legacy_params: Vec::new(),
}
}
}
@@ -151,23 +166,81 @@ impl From<&MetricDeclarationRecord> for DeclareMetricRequest {
DeclareMetricRequest {
name: record.name.clone(),
message_match_regex: record.message_match_regex.clone(),
label_params: record.label_params.clone(),
value_param: record.value_param.clone(),
matched_labels: record.matched_labels.clone(),
unmatched_labels: record.unmatched_labels.clone(),
params: record
.params
.iter()
.map(|param| RegexToMetricPrimitiveMatcher {
param_name: param.param_name.clone(),
capture_group: param.capture_group.clone(),
json_field_path: param.json_field_path.clone(),
kind: param.kind.into(),
role: param.role,
})
.collect(),
}
}
}
/// Migrates every metric in `profile` still carrying pre-037 inline matchers
/// (`MetricDeclarationRecord::legacy_params`, deserialized from an old profile file's `"params"`
/// key) into the unified shape: each matcher becomes a reference to a standalone
/// `ParamDeclarationRecord`, resolved by reusing an existing declared param of the exact same name
/// and shape (regex/capture-group-or-json-field-path/kind) when one exists, or else creating a new
/// one implicitly named `<metric_name>.<param_name>` (added to `profile.params`). Deterministic
/// naming means re-running this on an already-migrated profile is a no-op, and running it twice on
/// the same legacy data produces the same result. Returns whether anything changed, so the caller
/// can decide whether to persist the migration.
fn migrate_legacy_metric_params(profile: &mut Profile) -> bool {
let mut migrated = false;
for metric_index in 0..profile.metrics.len() {
if profile.metrics[metric_index].legacy_params.is_empty() {
continue;
}
let metric_name = profile.metrics[metric_index].name.clone();
let message_match_regex = profile.metrics[metric_index].message_match_regex.clone();
let legacy = std::mem::take(&mut profile.metrics[metric_index].legacy_params);
let mut label_params = Vec::new();
let mut value_param = None;
for matcher in &legacy {
let param_name =
resolve_or_create_legacy_param(profile, &metric_name, &message_match_regex, matcher);
match matcher.role {
LegacyParamRole::Label => label_params.push(param_name),
LegacyParamRole::Value => value_param = Some(param_name),
}
}
profile.metrics[metric_index].label_params = label_params;
profile.metrics[metric_index].value_param = value_param;
migrated = true;
}
migrated
}
/// Resolves one legacy matcher to a standalone param name, reusing an existing declared param of
/// the same bare name and shape, or else creating `<metric_name>.<param_name>` on `profile.params`.
fn resolve_or_create_legacy_param(
profile: &mut Profile,
metric_name: &str,
message_match_regex: &str,
matcher: &LegacyMetricParamRecord,
) -> String {
if let Some(existing) = profile.params.iter().find(|p| p.name == matcher.param_name) {
if existing.message_match_regex == message_match_regex
&& existing.capture_group == matcher.capture_group
&& existing.json_field_path == matcher.json_field_path
&& existing.kind == matcher.kind
{
return existing.name.clone();
}
}
let implicit_name = format!("{metric_name}.{}", matcher.param_name);
if !profile.params.iter().any(|p| p.name == implicit_name) {
profile.params.push(ParamDeclarationRecord {
name: implicit_name.clone(),
message_match_regex: message_match_regex.to_string(),
capture_group: matcher.capture_group.clone(),
json_field_path: matcher.json_field_path.clone(),
kind: matcher.kind,
});
}
implicit_name
}
/// A chart type a dashboard panel can render; kept in sync with the frontend's `'bar' | 'line' | 'pie'` union (see `webapp/src/lib/api.ts`).
#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)]
#[serde(rename_all = "lowercase")]
@@ -223,9 +296,11 @@ fn seeded_error_split_metric() -> MetricDeclarationRecord {
MetricDeclarationRecord {
name: SEEDED_METRIC_NAME.to_string(),
message_match_regex: r"(?i)error".to_string(),
params: Vec::new(),
label_params: Vec::new(),
value_param: None,
matched_labels: Some(BTreeMap::from([(SEEDED_METRIC_LABEL.to_string(), "error".to_string())])),
unmatched_labels: Some(BTreeMap::from([(SEEDED_METRIC_LABEL.to_string(), "other".to_string())])),
legacy_params: Vec::new(),
}
}
@@ -384,7 +459,7 @@ pub struct ProfileStore {
impl ProfileStore {
/// Loads the store from `store_path`; a missing or unreadable/unparsable file is treated as an empty store rather than failing startup.
pub fn load(store_path: &Path) -> ProfileStore {
match fs::read_to_string(store_path) {
let mut store = match fs::read_to_string(store_path) {
Ok(contents) => match serde_json::from_str::<Vec<Profile>>(&contents) {
Ok(profiles) => ProfileStore { profiles },
Err(err) => {
@@ -403,7 +478,29 @@ impl ProfileStore {
);
ProfileStore::default()
}
};
// Ticket 037: turn any pre-unification inline metric matchers into standalone param
// references. Deterministic naming makes this idempotent, so it's safe to run on every
// load; persisting the result isn't required for correctness (migration re-runs
// identically next time), but it's done here anyway so the on-disk file only carries the
// legacy `"params"` shape once, on the very next save.
let mut any_migrated = false;
for profile in &mut store.profiles {
if migrate_legacy_metric_params(profile) {
any_migrated = true;
}
}
if any_migrated {
if let Err(err) = store.save(store_path) {
eprintln!(
"warning: failed to persist migrated profile store at {}: {err}",
store_path.display()
);
}
}
store
}
/// Persists the store to `store_path` as pretty-printed JSON, creating any missing parent directories first.
@@ -672,19 +769,14 @@ mod tests {
}
}
fn metric_request(name: &str, pattern: &str, param_name: &str, capture_group: &str) -> DeclareMetricRequest {
fn metric_request(name: &str, pattern: &str, param_name: &str) -> DeclareMetricRequest {
DeclareMetricRequest {
name: name.to_string(),
message_match_regex: pattern.to_string(),
matched_labels: None,
unmatched_labels: None,
params: vec![RegexToMetricPrimitiveMatcher {
param_name: param_name.to_string(),
capture_group: capture_group.to_string(),
json_field_path: None,
kind: MetricPrimitiveKind::Number,
role: ParamRole::Label,
}],
label_params: vec![param_name.to_string()],
value_param: None,
}
}
@@ -938,12 +1030,7 @@ mod tests {
let param_req = param_request("level", r"level=(?P<level>\w+)", "level");
store.record_param_declaration(&store_path, "web", &param_req).unwrap();
let metric_req = metric_request(
"requests",
r"duration=(?P<duration>\d+)",
"duration",
"duration",
);
let metric_req = metric_request("requests", r"duration=(?P<duration>\d+)", "duration");
store.record_metric_declaration(&store_path, "web", &metric_req).unwrap();
let profile = store.get("web").unwrap();
@@ -984,16 +1071,20 @@ mod tests {
&param_request("level", r"level=(?P<level>\w+)", "level"),
)
.unwrap();
// A metric now references params by name (ticket 037), so "duration" must be declared as a
// standalone param before the metric below can reference it as a label param.
store
.record_param_declaration(
&store_path,
"web",
&param_request("duration", r"request duration=(?P<duration>\d+)", "duration"),
)
.unwrap();
store
.record_metric_declaration(
&store_path,
"web",
&metric_request(
"requests",
r"request duration=(?P<duration>\d+)",
"duration",
"duration",
),
&metric_request("requests", r"request duration=(?P<duration>\d+)", "duration"),
)
.unwrap();
store.set_active(&mut settings, &settings_path, Some("web".to_string())).unwrap();
@@ -1026,11 +1117,11 @@ mod tests {
EngineResponse::ParamDeclarations(params) => params,
_ => panic!("unexpected response"),
};
assert_eq!(params.len(), 1);
assert_eq!(params[0].name, "level");
assert_eq!(params[0].message_match_regex, r"level=(?P<level>\w+)");
assert_eq!(params[0].capture_group, "level");
assert!(matches!(params[0].kind, MetricPrimitiveKind::String));
assert_eq!(params.len(), 2);
let level = params.iter().find(|p| p.name == "level").unwrap();
assert_eq!(level.message_match_regex, r"level=(?P<level>\w+)");
assert_eq!(level.capture_group, "level");
assert!(matches!(level.kind, MetricPrimitiveKind::String));
let metrics = match ask(&engine_tx, EngineQuery::MetricDeclarations).await {
EngineResponse::MetricDeclarations(metrics) => metrics,
@@ -1039,8 +1130,7 @@ mod tests {
assert_eq!(metrics.len(), 1);
assert_eq!(metrics[0].name, "requests");
assert_eq!(metrics[0].message_match_regex, r"request duration=(?P<duration>\d+)");
assert_eq!(metrics[0].params.len(), 1);
assert_eq!(metrics[0].params[0].param_name, "duration");
assert_eq!(metrics[0].label_params, vec!["duration".to_string()]);
let logs = match ask(
&engine_tx,
@@ -1111,4 +1201,149 @@ mod tests {
Err(ProfileError::NotFound)
));
}
/// Writes a pre-037 `profiles.json` (metrics carrying inline `"params"` matchers rather than
/// `label_params`/`value_param`) directly to disk, bypassing `ProfileStore`/`Profile` entirely
/// so this doesn't accidentally exercise the current (post-migration) shape.
fn write_legacy_profiles_json(store_path: &Path, contents: &str) {
fs::create_dir_all(store_path.parent().unwrap()).unwrap();
fs::write(store_path, contents).unwrap();
}
/// Acceptance criterion: a profile saved before ticket 037 (inline `param_name`/`capture_group`/
/// `kind`/`role` matchers embedded in the metric) loads with those matchers turned into
/// standalone param declarations referenced by name, and the metric still ingests identically.
#[test]
fn migrates_legacy_inline_metric_params_into_standalone_declarations() {
let dir = tempfile::tempdir().unwrap();
let store_path = dir.path().join("profiles.json");
write_legacy_profiles_json(
&store_path,
r#"[
{
"name": "legacy",
"params": [],
"metrics": [
{
"name": "requests",
"message_match_regex": "request path=(?P<path>\\S+) duration=(?P<duration>\\d+)",
"params": [
{ "param_name": "path", "capture_group": "path", "kind": "string", "role": "label" },
{ "param_name": "duration", "capture_group": "duration", "kind": "number", "role": "value" }
]
}
],
"dashboards": []
}
]"#,
);
let store = ProfileStore::load(&store_path);
let profile = store.get("legacy").unwrap();
// No pre-existing bare "path"/"duration" param, so implicit ones were created, named after the metric.
assert_eq!(profile.params.len(), 2);
let path_param = profile.params.iter().find(|p| p.name == "requests.path").unwrap();
assert_eq!(path_param.message_match_regex, profile.metrics[0].message_match_regex);
assert_eq!(path_param.capture_group, "path");
assert!(matches!(path_param.kind, ProfileMetricPrimitiveKind::String));
let duration_param = profile.params.iter().find(|p| p.name == "requests.duration").unwrap();
assert!(matches!(duration_param.kind, ProfileMetricPrimitiveKind::Number));
assert_eq!(profile.metrics.len(), 1);
assert_eq!(profile.metrics[0].label_params, vec!["requests.path".to_string()]);
assert_eq!(profile.metrics[0].value_param, Some("requests.duration".to_string()));
// The migration is persisted, so the on-disk file no longer carries the legacy "params" shape.
let reloaded_raw = fs::read_to_string(&store_path).unwrap();
assert!(!reloaded_raw.contains("param_name"));
// Replaying into a fresh engine behaves identically to before migration: the metric still ingests and buckets.
let (engine_tx, engine_rx) = mpsc::channel(16);
let settings_handle = std::sync::Arc::new(std::sync::Mutex::new(Settings::default()));
tokio::runtime::Runtime::new().unwrap().block_on(async {
tokio::spawn(crate::engine::run(engine_rx, settings_handle));
load_into_engine(&engine_tx, profile).await;
engine_tx
.send(EngineMessage::Ingest(IngestedLine {
sequence: 0,
timestamp: Timestamp::from_millis(0),
text: "request path=/a duration=42".to_string(),
}))
.await
.unwrap();
let EngineResponse::MetricDeclarations(metrics) =
ask(&engine_tx, EngineQuery::MetricDeclarations).await
else {
panic!("unexpected response");
};
assert_eq!(metrics.len(), 1);
assert_eq!(metrics[0].label_params, vec!["requests.path".to_string()]);
});
}
/// When a bare-named param already declared on the profile matches a legacy matcher's shape
/// exactly (same regex/capture-group/kind), migration reuses it by name instead of creating a
/// redundant `<metric>.<param>` declaration.
#[test]
fn migration_reuses_an_existing_matching_bare_named_param() {
let dir = tempfile::tempdir().unwrap();
let store_path = dir.path().join("profiles.json");
write_legacy_profiles_json(
&store_path,
r#"[
{
"name": "legacy",
"params": [
{ "name": "path", "message_match_regex": "request path=(?P<path>\\S+)", "capture_group": "path", "kind": "string" }
],
"metrics": [
{
"name": "requests",
"message_match_regex": "request path=(?P<path>\\S+)",
"params": [
{ "param_name": "path", "capture_group": "path", "kind": "string", "role": "label" }
]
}
],
"dashboards": []
}
]"#,
);
let store = ProfileStore::load(&store_path);
let profile = store.get("legacy").unwrap();
// Reused the existing "path" declaration rather than creating "requests.path".
assert_eq!(profile.params.len(), 1);
assert_eq!(profile.params[0].name, "path");
assert_eq!(profile.metrics[0].label_params, vec!["path".to_string()]);
}
/// Migration is idempotent: loading an already-migrated (or never-legacy) profile a second time
/// doesn't change anything or re-trigger a save.
#[test]
fn migration_is_a_noop_on_an_already_migrated_profile() {
let dir = tempfile::tempdir().unwrap();
let store_path = dir.path().join("profiles.json");
let mut settings = Settings::default();
let mut store = ProfileStore::load(&store_path);
store.create(&store_path, &mut settings, &dir.path().join("settings.json"), "web".to_string()).unwrap();
store
.record_param_declaration(
&store_path,
"web",
&param_request("path", r"path=(?P<path>\S+)", "path"),
)
.unwrap();
store
.record_metric_declaration(&store_path, "web", &metric_request("requests", r"path=(?P<path>\S+)", "path"))
.unwrap();
let before = store.get("web").unwrap().clone();
let reloaded = ProfileStore::load(&store_path);
let after = reloaded.get("web").unwrap().clone();
assert_eq!(before, after);
}
}