Add gauge/counter semantics to metric declarations (ticket 097)
Metrics previously only declared a data type (String/Number/...), so rate() could be applied to a point-in-time gauge reading and sum()/avg() could be applied directly to a cumulative counter reading with no guard against either. Adds a MetricSemantics enum (Gauge, default; Counter) threaded through DeclareMetricRequest, the engine's MetricDeclaration data value, profile persistence, the API/MCP declaration routes, and the Metrics panel's declaration form, then uses it in evaluate_promql to reject rate() on a declared gauge and bare sum()/avg() on a declared counter (suggesting rate() first). The guard only applies to metrics with a captured value_param — bare occurrence-count metrics are always safe under both operations, so existing dashboards are unaffected. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TeyhRfMYjMditek6geDRoQ
This commit is contained in:
co-authored by
Claude Sonnet 5
parent
c3108677ce
commit
4a0fe92ce8
+2
-1
@@ -1,7 +1,7 @@
|
|||||||
use crate::config::Settings;
|
use crate::config::Settings;
|
||||||
use crate::config::profile::{Panel, Profile};
|
use crate::config::profile::{Panel, Profile};
|
||||||
use crate::engine::alerts::AlertCondition;
|
use crate::engine::alerts::AlertCondition;
|
||||||
use crate::engine::data_value::MetricPrimitiveKind;
|
use crate::engine::data_value::{MetricPrimitiveKind, MetricSemantics};
|
||||||
use crate::engine::metric_primitive::MetricPrimitive;
|
use crate::engine::metric_primitive::MetricPrimitive;
|
||||||
use serde::{Deserialize, Serialize};
|
use serde::{Deserialize, Serialize};
|
||||||
use std::collections::BTreeMap;
|
use std::collections::BTreeMap;
|
||||||
@@ -35,6 +35,7 @@ pub struct MetricDeclarationDto {
|
|||||||
pub value_param: Option<String>,
|
pub value_param: Option<String>,
|
||||||
pub matched_labels: Option<BTreeMap<String, String>>,
|
pub matched_labels: Option<BTreeMap<String, String>>,
|
||||||
pub unmatched_labels: Option<BTreeMap<String, String>>,
|
pub unmatched_labels: Option<BTreeMap<String, String>>,
|
||||||
|
pub semantics: MetricSemantics,
|
||||||
}
|
}
|
||||||
|
|
||||||
#[derive(Debug, Clone, Serialize)]
|
#[derive(Debug, Clone, Serialize)]
|
||||||
|
|||||||
@@ -69,6 +69,10 @@ pub struct DeclareMetricParams {
|
|||||||
pub matched_labels: Option<BTreeMap<String, String>>,
|
pub matched_labels: Option<BTreeMap<String, String>>,
|
||||||
#[serde(default)]
|
#[serde(default)]
|
||||||
pub unmatched_labels: Option<BTreeMap<String, String>>,
|
pub unmatched_labels: Option<BTreeMap<String, String>>,
|
||||||
|
/// Whether this metric's value is a point-in-time reading ("gauge", e.g. queue depth) or a
|
||||||
|
/// monotonically-increasing total ("counter", e.g. requests served). Defaults to "gauge".
|
||||||
|
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||||
|
pub semantics: Option<String>,
|
||||||
}
|
}
|
||||||
|
|
||||||
#[derive(Debug, Deserialize, schemars::JsonSchema)]
|
#[derive(Debug, Deserialize, schemars::JsonSchema)]
|
||||||
|
|||||||
@@ -486,6 +486,7 @@ async fn declare_metric(
|
|||||||
value_param: request.value_param,
|
value_param: request.value_param,
|
||||||
matched_labels: request.matched_labels,
|
matched_labels: request.matched_labels,
|
||||||
unmatched_labels: request.unmatched_labels,
|
unmatched_labels: request.unmatched_labels,
|
||||||
|
semantics: request.semantics,
|
||||||
};
|
};
|
||||||
(StatusCode::CREATED, Json(dto)).into_response()
|
(StatusCode::CREATED, Json(dto)).into_response()
|
||||||
}
|
}
|
||||||
@@ -526,6 +527,7 @@ async fn update_metric(
|
|||||||
value_param: request.value_param,
|
value_param: request.value_param,
|
||||||
matched_labels: request.matched_labels,
|
matched_labels: request.matched_labels,
|
||||||
unmatched_labels: request.unmatched_labels,
|
unmatched_labels: request.unmatched_labels,
|
||||||
|
semantics: request.semantics,
|
||||||
};
|
};
|
||||||
(StatusCode::OK, Json(dto)).into_response()
|
(StatusCode::OK, Json(dto)).into_response()
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -13,7 +13,7 @@ use serde::{Deserialize, Serialize};
|
|||||||
|
|
||||||
use crate::config::Settings;
|
use crate::config::Settings;
|
||||||
use crate::consts::{DEFAULT_PROFILE_NAME, SEEDED_METRIC_LABEL, SEEDED_METRIC_NAME};
|
use crate::consts::{DEFAULT_PROFILE_NAME, SEEDED_METRIC_LABEL, SEEDED_METRIC_NAME};
|
||||||
use crate::engine::data_value::MetricPrimitiveKind;
|
use crate::engine::data_value::{MetricPrimitiveKind, MetricSemantics};
|
||||||
use crate::engine::{DeclareMetricRequest, DeclareParamRequest, EngineHandle, EngineMessage};
|
use crate::engine::{DeclareMetricRequest, DeclareParamRequest, EngineHandle, EngineMessage};
|
||||||
use tokio::sync::oneshot;
|
use tokio::sync::oneshot;
|
||||||
|
|
||||||
@@ -122,6 +122,8 @@ pub struct MetricDeclarationRecord {
|
|||||||
pub matched_labels: Option<std::collections::BTreeMap<String, String>>,
|
pub matched_labels: Option<std::collections::BTreeMap<String, String>>,
|
||||||
#[serde(default)]
|
#[serde(default)]
|
||||||
pub unmatched_labels: Option<std::collections::BTreeMap<String, String>>,
|
pub unmatched_labels: Option<std::collections::BTreeMap<String, String>>,
|
||||||
|
#[serde(default)]
|
||||||
|
pub semantics: MetricSemantics,
|
||||||
#[serde(default, rename = "params", skip_serializing_if = "Vec::is_empty")]
|
#[serde(default, rename = "params", skip_serializing_if = "Vec::is_empty")]
|
||||||
legacy_params: Vec<LegacyMetricParamRecord>,
|
legacy_params: Vec<LegacyMetricParamRecord>,
|
||||||
}
|
}
|
||||||
@@ -135,6 +137,7 @@ impl From<&DeclareMetricRequest> for MetricDeclarationRecord {
|
|||||||
value_param: request.value_param.clone(),
|
value_param: request.value_param.clone(),
|
||||||
matched_labels: request.matched_labels.clone(),
|
matched_labels: request.matched_labels.clone(),
|
||||||
unmatched_labels: request.unmatched_labels.clone(),
|
unmatched_labels: request.unmatched_labels.clone(),
|
||||||
|
semantics: request.semantics,
|
||||||
legacy_params: Vec::new(),
|
legacy_params: Vec::new(),
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -149,6 +152,7 @@ impl From<&MetricDeclarationRecord> for DeclareMetricRequest {
|
|||||||
value_param: record.value_param.clone(),
|
value_param: record.value_param.clone(),
|
||||||
matched_labels: record.matched_labels.clone(),
|
matched_labels: record.matched_labels.clone(),
|
||||||
unmatched_labels: record.unmatched_labels.clone(),
|
unmatched_labels: record.unmatched_labels.clone(),
|
||||||
|
semantics: record.semantics,
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -295,6 +299,7 @@ impl MetricDeclarationRecord {
|
|||||||
SEEDED_METRIC_LABEL.to_string(),
|
SEEDED_METRIC_LABEL.to_string(),
|
||||||
"other".to_string(),
|
"other".to_string(),
|
||||||
)])),
|
)])),
|
||||||
|
semantics: MetricSemantics::default(),
|
||||||
legacy_params: Vec::new(),
|
legacy_params: Vec::new(),
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -741,6 +746,7 @@ mod tests {
|
|||||||
unmatched_labels: None,
|
unmatched_labels: None,
|
||||||
label_params: vec![param_name.to_string()],
|
label_params: vec![param_name.to_string()],
|
||||||
value_param: None,
|
value_param: None,
|
||||||
|
semantics: MetricSemantics::default(),
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -80,6 +80,20 @@ pub enum MetricPrimitiveKind {
|
|||||||
Boolean,
|
Boolean,
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Whether a declared metric's value is a point-in-time reading (`Gauge`, e.g. queue depth) or a
|
||||||
|
/// monotonically-increasing total (`Counter`, e.g. requests served) — orthogonal to
|
||||||
|
/// `MetricPrimitiveKind`, which describes the value's data type rather than how it accumulates.
|
||||||
|
/// `Gauge` is the default so existing declarations (persisted before this field existed) keep
|
||||||
|
/// behaving the way bare selectors and the `gauge()` PromQL wrapper already did.
|
||||||
|
#[allow(dead_code)]
|
||||||
|
#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, Default)]
|
||||||
|
#[serde(rename_all = "lowercase")]
|
||||||
|
pub enum MetricSemantics {
|
||||||
|
#[default]
|
||||||
|
Gauge,
|
||||||
|
Counter,
|
||||||
|
}
|
||||||
|
|
||||||
#[allow(dead_code)]
|
#[allow(dead_code)]
|
||||||
#[derive(Debug, Clone, Serialize)]
|
#[derive(Debug, Clone, Serialize)]
|
||||||
pub enum DataValue {
|
pub enum DataValue {
|
||||||
@@ -93,6 +107,7 @@ pub enum DataValue {
|
|||||||
value_param: Option<String>,
|
value_param: Option<String>,
|
||||||
matched_labels: Option<BTreeMap<String, String>>,
|
matched_labels: Option<BTreeMap<String, String>>,
|
||||||
unmatched_labels: Option<BTreeMap<String, String>>,
|
unmatched_labels: Option<BTreeMap<String, String>>,
|
||||||
|
semantics: MetricSemantics,
|
||||||
},
|
},
|
||||||
BucketValue {
|
BucketValue {
|
||||||
values: Vec<(u64, Timestamp, MetricPrimitive)>,
|
values: Vec<(u64, Timestamp, MetricPrimitive)>,
|
||||||
|
|||||||
+3
-1
@@ -22,7 +22,7 @@ pub use handle::{EngineHandle, spawn};
|
|||||||
pub use message::{EngineMessage, EngineQuery, EngineResponse};
|
pub use message::{EngineMessage, EngineQuery, EngineResponse};
|
||||||
pub(crate) use snapshot::{EngineSnapshot, eval_json_comparison};
|
pub(crate) use snapshot::{EngineSnapshot, eval_json_comparison};
|
||||||
|
|
||||||
use crate::engine::data_value::ParamDeclaration;
|
use crate::engine::data_value::{MetricSemantics, ParamDeclaration};
|
||||||
use crate::engine::timestamp::Timestamp;
|
use crate::engine::timestamp::Timestamp;
|
||||||
use serde::Deserialize;
|
use serde::Deserialize;
|
||||||
use std::collections::BTreeMap;
|
use std::collections::BTreeMap;
|
||||||
@@ -61,4 +61,6 @@ pub struct DeclareMetricRequest {
|
|||||||
pub matched_labels: Option<BTreeMap<String, String>>,
|
pub matched_labels: Option<BTreeMap<String, String>>,
|
||||||
#[serde(default)]
|
#[serde(default)]
|
||||||
pub unmatched_labels: Option<BTreeMap<String, String>>,
|
pub unmatched_labels: Option<BTreeMap<String, String>>,
|
||||||
|
#[serde(default)]
|
||||||
|
pub semantics: MetricSemantics,
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -18,7 +18,9 @@ use crate::consts::{
|
|||||||
MAX_AUTO_DECLARED_PARAMS, MAX_NEW_AUTO_PARAMS_PER_LINE, OTTYL_RESERVED_PARAM_NAMES,
|
MAX_AUTO_DECLARED_PARAMS, MAX_NEW_AUTO_PARAMS_PER_LINE, OTTYL_RESERVED_PARAM_NAMES,
|
||||||
SORT_HELPER_INDEX_PURGE_AFTER,
|
SORT_HELPER_INDEX_PURGE_AFTER,
|
||||||
};
|
};
|
||||||
use crate::engine::data_value::{DataValue, MetricPrimitiveKind, ParamDeclaration};
|
use crate::engine::data_value::{
|
||||||
|
DataValue, MetricPrimitiveKind, MetricSemantics, ParamDeclaration,
|
||||||
|
};
|
||||||
use crate::engine::filter_parser::{self, CompareOp, FilterExpr, FilterValue};
|
use crate::engine::filter_parser::{self, CompareOp, FilterExpr, FilterValue};
|
||||||
use crate::engine::index_key::IndexKey;
|
use crate::engine::index_key::IndexKey;
|
||||||
use crate::engine::message::{EngineQuery, EngineResponse};
|
use crate::engine::message::{EngineQuery, EngineResponse};
|
||||||
@@ -53,6 +55,7 @@ type MetricDeclarationRow<'a> = (
|
|||||||
Option<&'a str>,
|
Option<&'a str>,
|
||||||
Option<&'a BTreeMap<String, String>>,
|
Option<&'a BTreeMap<String, String>>,
|
||||||
Option<&'a BTreeMap<String, String>>,
|
Option<&'a BTreeMap<String, String>>,
|
||||||
|
MetricSemantics,
|
||||||
);
|
);
|
||||||
|
|
||||||
/// Per-label-group point series, keyed by the group's label set, before merging into one
|
/// Per-label-group point series, keyed by the group's label set, before merging into one
|
||||||
@@ -91,6 +94,7 @@ impl EngineSnapshot {
|
|||||||
value_param,
|
value_param,
|
||||||
matched_labels,
|
matched_labels,
|
||||||
unmatched_labels,
|
unmatched_labels,
|
||||||
|
semantics,
|
||||||
},
|
},
|
||||||
) => Some((
|
) => Some((
|
||||||
name.as_str(),
|
name.as_str(),
|
||||||
@@ -99,6 +103,7 @@ impl EngineSnapshot {
|
|||||||
value_param.as_deref(),
|
value_param.as_deref(),
|
||||||
matched_labels.as_ref(),
|
matched_labels.as_ref(),
|
||||||
unmatched_labels.as_ref(),
|
unmatched_labels.as_ref(),
|
||||||
|
*semantics,
|
||||||
)),
|
)),
|
||||||
_ => None,
|
_ => None,
|
||||||
})
|
})
|
||||||
@@ -420,6 +425,7 @@ impl EngineSnapshot {
|
|||||||
value_param: request.value_param,
|
value_param: request.value_param,
|
||||||
matched_labels: request.matched_labels,
|
matched_labels: request.matched_labels,
|
||||||
unmatched_labels: request.unmatched_labels,
|
unmatched_labels: request.unmatched_labels,
|
||||||
|
semantics: request.semantics,
|
||||||
},
|
},
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
@@ -618,7 +624,7 @@ impl EngineSnapshot {
|
|||||||
let metrics: Vec<SeededMetric> = self
|
let metrics: Vec<SeededMetric> = self
|
||||||
.metric_declarations_range()
|
.metric_declarations_range()
|
||||||
.map(
|
.map(
|
||||||
|(name, _, label_params, value_param, matched_labels, unmatched_labels)| {
|
|(name, _, label_params, value_param, matched_labels, unmatched_labels, _)| {
|
||||||
(
|
(
|
||||||
name.to_string(),
|
name.to_string(),
|
||||||
label_params.to_vec(),
|
label_params.to_vec(),
|
||||||
@@ -708,6 +714,32 @@ impl EngineSnapshot {
|
|||||||
_ => None,
|
_ => None,
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
pub(crate) fn declared_semantics(&self, metric_name: &str) -> Option<MetricSemantics> {
|
||||||
|
match self.index.get(&IndexKey::MetricDeclaration {
|
||||||
|
name: metric_name.to_string(),
|
||||||
|
}) {
|
||||||
|
Some(DataValue::MetricDeclaration { semantics, .. }) => Some(*semantics),
|
||||||
|
_ => None,
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
/// Whether `metric_name` captures a per-event value (rather than just counting matching log
|
||||||
|
/// lines). The gauge/counter semantics guard in `evaluate_promql` only applies to metrics
|
||||||
|
/// with a captured value: a bare occurrence count is always safe to both `rate()` (events per
|
||||||
|
/// window) and `sum()`/`avg()` (total/average occurrences) since each event contributes
|
||||||
|
/// exactly one point, unlike a captured reading that might itself be a cumulative total.
|
||||||
|
fn declares_value_param(&self, metric_name: &str) -> bool {
|
||||||
|
matches!(
|
||||||
|
self.index.get(&IndexKey::MetricDeclaration {
|
||||||
|
name: metric_name.to_string(),
|
||||||
|
}),
|
||||||
|
Some(DataValue::MetricDeclaration {
|
||||||
|
value_param: Some(_),
|
||||||
|
..
|
||||||
|
})
|
||||||
|
)
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
// --- params: declaring and resolving param values ---
|
// --- params: declaring and resolving param values ---
|
||||||
@@ -1068,6 +1100,33 @@ impl EngineSnapshot {
|
|||||||
parsed.metric_name
|
parsed.metric_name
|
||||||
));
|
));
|
||||||
}
|
}
|
||||||
|
// A metric with no captured value (just occurrence counts of matching log lines) is
|
||||||
|
// always safe under both rate() and sum()/avg(): each event contributes exactly one
|
||||||
|
// point, so there's no cumulative-total to misinterpret. The guard below only matters
|
||||||
|
// once a metric captures a per-event value via `value_param`.
|
||||||
|
let declared_semantics = self
|
||||||
|
.declares_value_param(&parsed.metric_name)
|
||||||
|
.then(|| self.declared_semantics(&parsed.metric_name))
|
||||||
|
.flatten();
|
||||||
|
if matches!(parsed.agg, Some(AggOp::Rate))
|
||||||
|
&& declared_semantics == Some(MetricSemantics::Gauge)
|
||||||
|
{
|
||||||
|
return Err(format!(
|
||||||
|
"cannot rate() \"{}\": it's declared as a gauge, not a counter; gauges are point-in-time \
|
||||||
|
readings, so rate() over them isn't meaningful — use it bare or wrap it in gauge()",
|
||||||
|
parsed.metric_name
|
||||||
|
));
|
||||||
|
}
|
||||||
|
if matches!(parsed.agg, Some(AggOp::Sum) | Some(AggOp::Avg))
|
||||||
|
&& !parsed.inner_rate
|
||||||
|
&& declared_semantics == Some(MetricSemantics::Counter)
|
||||||
|
{
|
||||||
|
return Err(format!(
|
||||||
|
"cannot sum/avg \"{}\" directly: it's declared as a counter, so summing/averaging its raw \
|
||||||
|
values double-counts the running total — wrap it in rate() first, e.g. sum(rate({}[5m]))",
|
||||||
|
parsed.metric_name, parsed.metric_name
|
||||||
|
));
|
||||||
|
}
|
||||||
|
|
||||||
let mut series_points: BTreeMap<BTreeMap<String, String>, Vec<(i64, f64)>> =
|
let mut series_points: BTreeMap<BTreeMap<String, String>, Vec<(i64, f64)>> =
|
||||||
BTreeMap::new();
|
BTreeMap::new();
|
||||||
|
|||||||
@@ -280,6 +280,7 @@ pub(super) fn handle_query_inner(
|
|||||||
value_param,
|
value_param,
|
||||||
matched_labels,
|
matched_labels,
|
||||||
unmatched_labels,
|
unmatched_labels,
|
||||||
|
semantics,
|
||||||
)| {
|
)| {
|
||||||
MetricDeclarationDto {
|
MetricDeclarationDto {
|
||||||
name: name.to_string(),
|
name: name.to_string(),
|
||||||
@@ -288,6 +289,7 @@ pub(super) fn handle_query_inner(
|
|||||||
value_param: value_param.map(|s| s.to_string()),
|
value_param: value_param.map(|s| s.to_string()),
|
||||||
matched_labels: matched_labels.cloned(),
|
matched_labels: matched_labels.cloned(),
|
||||||
unmatched_labels: unmatched_labels.cloned(),
|
unmatched_labels: unmatched_labels.cloned(),
|
||||||
|
semantics,
|
||||||
}
|
}
|
||||||
},
|
},
|
||||||
)
|
)
|
||||||
|
|||||||
@@ -719,6 +719,7 @@ fn declarations_stay_within_their_range() {
|
|||||||
value_param: None,
|
value_param: None,
|
||||||
matched_labels: None,
|
matched_labels: None,
|
||||||
unmatched_labels: None,
|
unmatched_labels: None,
|
||||||
|
semantics: Default::default(),
|
||||||
},
|
},
|
||||||
);
|
);
|
||||||
|
|
||||||
@@ -803,6 +804,22 @@ fn metric_request_with_value(
|
|||||||
pattern: &str,
|
pattern: &str,
|
||||||
label_params: Vec<&str>,
|
label_params: Vec<&str>,
|
||||||
value_param: Option<&str>,
|
value_param: Option<&str>,
|
||||||
|
) -> DeclareMetricRequest {
|
||||||
|
metric_request_with_value_and_semantics(
|
||||||
|
name,
|
||||||
|
pattern,
|
||||||
|
label_params,
|
||||||
|
value_param,
|
||||||
|
MetricSemantics::default(),
|
||||||
|
)
|
||||||
|
}
|
||||||
|
|
||||||
|
fn metric_request_with_value_and_semantics(
|
||||||
|
name: &str,
|
||||||
|
pattern: &str,
|
||||||
|
label_params: Vec<&str>,
|
||||||
|
value_param: Option<&str>,
|
||||||
|
semantics: MetricSemantics,
|
||||||
) -> DeclareMetricRequest {
|
) -> DeclareMetricRequest {
|
||||||
DeclareMetricRequest {
|
DeclareMetricRequest {
|
||||||
name: name.to_string(),
|
name: name.to_string(),
|
||||||
@@ -811,6 +828,7 @@ fn metric_request_with_value(
|
|||||||
value_param: value_param.map(str::to_string),
|
value_param: value_param.map(str::to_string),
|
||||||
matched_labels: None,
|
matched_labels: None,
|
||||||
unmatched_labels: None,
|
unmatched_labels: None,
|
||||||
|
semantics,
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -1551,6 +1569,161 @@ fn sum_of_rate_over_string_kind_metric_is_still_allowed_since_rate_only_counts_o
|
|||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
fn declare_gauge_metric_with_value(index: &mut EngineSnapshot) {
|
||||||
|
declare_test_param(
|
||||||
|
index,
|
||||||
|
"depth",
|
||||||
|
r"queue depth=(?P<depth>\d+)",
|
||||||
|
"depth",
|
||||||
|
MetricPrimitiveKind::Number,
|
||||||
|
);
|
||||||
|
index
|
||||||
|
.declare_metric(metric_request_with_value_and_semantics(
|
||||||
|
"queue_depth",
|
||||||
|
r"queue depth=(?P<depth>\d+)",
|
||||||
|
vec![],
|
||||||
|
Some("depth"),
|
||||||
|
MetricSemantics::Gauge,
|
||||||
|
))
|
||||||
|
.unwrap();
|
||||||
|
}
|
||||||
|
|
||||||
|
fn declare_counter_metric_with_value(index: &mut EngineSnapshot) {
|
||||||
|
declare_test_param(
|
||||||
|
index,
|
||||||
|
"total",
|
||||||
|
r"served total=(?P<total>\d+)",
|
||||||
|
"total",
|
||||||
|
MetricPrimitiveKind::Number,
|
||||||
|
);
|
||||||
|
index
|
||||||
|
.declare_metric(metric_request_with_value_and_semantics(
|
||||||
|
"served_total",
|
||||||
|
r"served total=(?P<total>\d+)",
|
||||||
|
vec![],
|
||||||
|
Some("total"),
|
||||||
|
MetricSemantics::Counter,
|
||||||
|
))
|
||||||
|
.unwrap();
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn rate_over_a_declared_gauge_with_a_value_param_is_rejected_with_an_actionable_error() {
|
||||||
|
let mut index = EngineSnapshot::default();
|
||||||
|
declare_gauge_metric_with_value(&mut index);
|
||||||
|
ingest_log(&mut index, 0, "queue depth=3");
|
||||||
|
|
||||||
|
let parsed = PromqlQuery::parse("rate(queue_depth[5s])").unwrap();
|
||||||
|
let err = index.evaluate_promql(&parsed, None, None).unwrap_err();
|
||||||
|
|
||||||
|
assert!(
|
||||||
|
err.contains("queue_depth"),
|
||||||
|
"error should name the metric: {err}"
|
||||||
|
);
|
||||||
|
assert!(
|
||||||
|
err.to_lowercase().contains("gauge"),
|
||||||
|
"error should explain why: {err}"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn bare_query_over_a_declared_gauge_with_a_value_param_is_still_allowed() {
|
||||||
|
let mut index = EngineSnapshot::default();
|
||||||
|
declare_gauge_metric_with_value(&mut index);
|
||||||
|
ingest_log(&mut index, 0, "queue depth=3");
|
||||||
|
|
||||||
|
let parsed = PromqlQuery::parse("queue_depth").unwrap();
|
||||||
|
let series = index.evaluate_promql(&parsed, None, None).unwrap();
|
||||||
|
|
||||||
|
assert_eq!(series.len(), 1);
|
||||||
|
assert_eq!(series[0].points, vec![(0, 3.0)]);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn sum_over_a_declared_counter_with_a_value_param_is_rejected_with_an_actionable_error() {
|
||||||
|
let mut index = EngineSnapshot::default();
|
||||||
|
declare_counter_metric_with_value(&mut index);
|
||||||
|
ingest_log(&mut index, 0, "served total=10");
|
||||||
|
|
||||||
|
let parsed = PromqlQuery::parse("sum(served_total)").unwrap();
|
||||||
|
let err = index.evaluate_promql(&parsed, None, None).unwrap_err();
|
||||||
|
|
||||||
|
assert!(
|
||||||
|
err.contains("served_total"),
|
||||||
|
"error should name the metric: {err}"
|
||||||
|
);
|
||||||
|
assert!(
|
||||||
|
err.to_lowercase().contains("counter"),
|
||||||
|
"error should explain why: {err}"
|
||||||
|
);
|
||||||
|
assert!(
|
||||||
|
err.contains("rate("),
|
||||||
|
"error should suggest wrapping in rate(): {err}"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn avg_over_a_declared_counter_with_a_value_param_is_rejected_with_an_actionable_error() {
|
||||||
|
let mut index = EngineSnapshot::default();
|
||||||
|
declare_counter_metric_with_value(&mut index);
|
||||||
|
ingest_log(&mut index, 0, "served total=10");
|
||||||
|
|
||||||
|
let parsed = PromqlQuery::parse("avg(served_total)").unwrap();
|
||||||
|
let err = index.evaluate_promql(&parsed, None, None).unwrap_err();
|
||||||
|
|
||||||
|
assert!(err.contains("served_total"));
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn sum_of_rate_over_a_declared_counter_with_a_value_param_is_allowed() {
|
||||||
|
let mut index = EngineSnapshot::default();
|
||||||
|
declare_counter_metric_with_value(&mut index);
|
||||||
|
ingest_log(&mut index, 0, "served total=10");
|
||||||
|
ingest_log(&mut index, 1000, "served total=20");
|
||||||
|
|
||||||
|
let parsed = PromqlQuery::parse("sum(rate(served_total[2s]))").unwrap();
|
||||||
|
let series = index.evaluate_promql(&parsed, None, None);
|
||||||
|
|
||||||
|
assert!(
|
||||||
|
series.is_ok(),
|
||||||
|
"sum(rate(...)) already applies rate() first, so it should not be rejected: {series:?}"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn rate_over_a_declared_counter_with_a_value_param_is_allowed() {
|
||||||
|
let mut index = EngineSnapshot::default();
|
||||||
|
declare_counter_metric_with_value(&mut index);
|
||||||
|
ingest_log(&mut index, 0, "served total=10");
|
||||||
|
ingest_log(&mut index, 1000, "served total=20");
|
||||||
|
|
||||||
|
let parsed = PromqlQuery::parse("rate(served_total[2s])").unwrap();
|
||||||
|
let series = index.evaluate_promql(&parsed, None, None);
|
||||||
|
|
||||||
|
assert!(series.is_ok());
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn rate_and_sum_over_occurrence_count_metrics_are_unaffected_by_the_semantics_guard_regardless_of_declared_kind()
|
||||||
|
{
|
||||||
|
// "requests" (declared via `declare_requests_metric`) has no value_param — it's a bare
|
||||||
|
// occurrence count, not a captured gauge/counter reading — so the semantics guard must not
|
||||||
|
// apply to it even though its semantics defaults to `Gauge`.
|
||||||
|
let mut index = EngineSnapshot::default();
|
||||||
|
declare_requests_metric(&mut index);
|
||||||
|
ingest_log(&mut index, 0, "request path=/a");
|
||||||
|
ingest_log(&mut index, 1000, "request path=/a");
|
||||||
|
|
||||||
|
assert!(
|
||||||
|
PromqlQuery::parse("rate(requests[2s])")
|
||||||
|
.is_ok_and(|parsed| index.evaluate_promql(&parsed, None, None).is_ok())
|
||||||
|
);
|
||||||
|
assert!(
|
||||||
|
PromqlQuery::parse("sum(requests)")
|
||||||
|
.is_ok_and(|parsed| index.evaluate_promql(&parsed, None, None).is_ok())
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn bare_query_over_string_kind_metric_is_still_allowed() {
|
fn bare_query_over_string_kind_metric_is_still_allowed() {
|
||||||
let mut index = EngineSnapshot::default();
|
let mut index = EngineSnapshot::default();
|
||||||
@@ -2109,6 +2282,7 @@ fn declare_error_category_metric(index: &mut EngineSnapshot) {
|
|||||||
"category".to_string(),
|
"category".to_string(),
|
||||||
"other".to_string(),
|
"other".to_string(),
|
||||||
)])),
|
)])),
|
||||||
|
semantics: Default::default(),
|
||||||
})
|
})
|
||||||
.unwrap();
|
.unwrap();
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -1,50 +0,0 @@
|
|||||||
# 097 — Metrics have no gauge/counter distinction, so `rate()`/`sum()`/`avg()` can be applied without a semantics guard
|
|
||||||
|
|
||||||
## Summary
|
|
||||||
|
|
||||||
`MetricPrimitiveKind` only distinguishes data *type*
|
|
||||||
(`String`/`Number`/`Timestamp`/`Duration`/`Boolean`), not metric
|
|
||||||
*semantics* (counter vs. gauge). PromQL's `gauge()` op is a bespoke
|
|
||||||
query-time "take the last point" function rather than something tied
|
|
||||||
to how a metric was declared, and nothing stops calling `rate()` on a
|
|
||||||
metric that's conceptually a gauge, or `sum()`/`avg()` on a
|
|
||||||
monotonically-increasing counter without rating it first.
|
|
||||||
|
|
||||||
## Where
|
|
||||||
|
|
||||||
- `src/engine/data_value.rs:70-76` — `MetricPrimitiveKind`:
|
|
||||||
`String, Number, Timestamp, Duration, Boolean`. No `Counter`/`Gauge`
|
|
||||||
variant.
|
|
||||||
- `src/engine/metric_primitive.rs:8-14` — `MetricPrimitive` mirrors
|
|
||||||
the same type-only distinction (`String, Number(Number), Timestamp,
|
|
||||||
Duration(i64), Boolean`).
|
|
||||||
- `src/engine/promql.rs:9-16` — `AggOp` includes `Gauge` as one of six
|
|
||||||
query-time aggregation ops (`Sum, Avg, Count, Rate, Histogram,
|
|
||||||
Gauge`), evaluated in `src/engine/snapshot/mod.rs:993-1001` by
|
|
||||||
taking only `points.last()` per series — a query-side convenience,
|
|
||||||
not a property of the declared metric.
|
|
||||||
- The only type guard on aggregation today is
|
|
||||||
`src/engine/snapshot/mod.rs:888-896`, which rejects `sum`/`avg` on
|
|
||||||
`MetricPrimitiveKind::String` — there's no equivalent guard for
|
|
||||||
applying `rate()` to a non-monotonic value or `sum()`/`avg()` to an
|
|
||||||
un-rated counter.
|
|
||||||
|
|
||||||
## Scope
|
|
||||||
|
|
||||||
- Add a semantics field to metric declarations (e.g. extend
|
|
||||||
`MetricPrimitiveKind` with `Counter`/`Gauge` variants, or add a
|
|
||||||
separate `MetricSemantics` enum alongside the existing type field —
|
|
||||||
decide which based on whether type and semantics should ever vary
|
|
||||||
independently for the same declaration).
|
|
||||||
- Thread that through `DeclareMetricRequest`/`MetricDeclarationRecord`/
|
|
||||||
`MetricDeclarationDto` (the same three-places pattern noted in this
|
|
||||||
repo's `CLAUDE.md` for declaration features: `engine.rs` message +
|
|
||||||
handler, `settings.rs`/`profile.rs` persistence, `api.rs` route,
|
|
||||||
plus a webapp UI affordance in `Params.tsx`/`Metrics`'s declaration
|
|
||||||
form).
|
|
||||||
- Use it to validate PromQL queries at evaluation time (e.g. warn or
|
|
||||||
reject `rate()` on a declared gauge, or `sum()`/`avg()` directly on a
|
|
||||||
declared counter without an inner `rate()`), and consider whether
|
|
||||||
the current ad hoc `gauge()` PromQL wrapper becomes redundant once
|
|
||||||
the declaration itself carries this information (a bare selector on
|
|
||||||
a declared gauge could just work the way `gauge()` does today).
|
|
||||||
@@ -11,6 +11,7 @@ import {
|
|||||||
updateMetric,
|
updateMetric,
|
||||||
type BucketValue,
|
type BucketValue,
|
||||||
type MetricDeclaration,
|
type MetricDeclaration,
|
||||||
|
type MetricSemantics,
|
||||||
type ParamDeclaration,
|
type ParamDeclaration,
|
||||||
} from '../lib/api'
|
} from '../lib/api'
|
||||||
import { FormFooter, PopOutButton, SectionHeader, useSectionCollapsed } from './CollapsibleSection'
|
import { FormFooter, PopOutButton, SectionHeader, useSectionCollapsed } from './CollapsibleSection'
|
||||||
@@ -82,6 +83,7 @@ export function Metrics({ range, activePreset }: { range: TimeRange | null; acti
|
|||||||
// (ticket 037) rather than re-entering a regex/capture-group/kind per param.
|
// (ticket 037) rather than re-entering a regex/capture-group/kind per param.
|
||||||
const [labelParams, setLabelParams] = useState<string[]>([])
|
const [labelParams, setLabelParams] = useState<string[]>([])
|
||||||
const [valueParam, setValueParam] = useState<string>('')
|
const [valueParam, setValueParam] = useState<string>('')
|
||||||
|
const [semantics, setSemantics] = useState<MetricSemantics>('gauge')
|
||||||
const [submitError, setSubmitError] = useState<string | null>(null)
|
const [submitError, setSubmitError] = useState<string | null>(null)
|
||||||
const [submitting, setSubmitting] = useState(false)
|
const [submitting, setSubmitting] = useState(false)
|
||||||
const [collapsed, setCollapsed] = useSectionCollapsed('metrics')
|
const [collapsed, setCollapsed] = useSectionCollapsed('metrics')
|
||||||
@@ -141,6 +143,7 @@ export function Metrics({ range, activePreset }: { range: TimeRange | null; acti
|
|||||||
setRegex('')
|
setRegex('')
|
||||||
setLabelParams([])
|
setLabelParams([])
|
||||||
setValueParam('')
|
setValueParam('')
|
||||||
|
setSemantics('gauge')
|
||||||
setSubmitError(null)
|
setSubmitError(null)
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -151,6 +154,7 @@ export function Metrics({ range, activePreset }: { range: TimeRange | null; acti
|
|||||||
setRegex(metric.message_match_regex)
|
setRegex(metric.message_match_regex)
|
||||||
setLabelParams(metric.label_params)
|
setLabelParams(metric.label_params)
|
||||||
setValueParam(metric.value_param ?? '')
|
setValueParam(metric.value_param ?? '')
|
||||||
|
setSemantics(metric.semantics ?? 'gauge')
|
||||||
setSubmitError(null)
|
setSubmitError(null)
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -182,6 +186,7 @@ export function Metrics({ range, activePreset }: { range: TimeRange | null; acti
|
|||||||
message_match_regex: regex,
|
message_match_regex: regex,
|
||||||
label_params: labelParams,
|
label_params: labelParams,
|
||||||
value_param: valueParam || null,
|
value_param: valueParam || null,
|
||||||
|
semantics,
|
||||||
}
|
}
|
||||||
const saved = editingName ? await updateMetric(editingName, request) : await declareMetric(request)
|
const saved = editingName ? await updateMetric(editingName, request) : await declareMetric(request)
|
||||||
resetForm()
|
resetForm()
|
||||||
@@ -276,6 +281,22 @@ export function Metrics({ range, activePreset }: { range: TimeRange | null; acti
|
|||||||
))}
|
))}
|
||||||
</select>
|
</select>
|
||||||
</div>
|
</div>
|
||||||
|
<div>
|
||||||
|
<label
|
||||||
|
className="mb-1 block text-xs text-text-subtle"
|
||||||
|
title="Gauge: a point-in-time reading (e.g. queue depth) — rate() over it is rejected. Counter: a monotonically-increasing total (e.g. requests served) — sum()/avg() directly on it is rejected; wrap it in rate() first."
|
||||||
|
>
|
||||||
|
Semantics
|
||||||
|
</label>
|
||||||
|
<select
|
||||||
|
className="w-full rounded border border-border-soft bg-surface-1 px-2 py-1 text-xs text-text focus:border-border-strong focus:outline-none"
|
||||||
|
value={semantics}
|
||||||
|
onChange={(e) => setSemantics(e.target.value as MetricSemantics)}
|
||||||
|
>
|
||||||
|
<option value="gauge">Gauge (point-in-time reading)</option>
|
||||||
|
<option value="counter">Counter (monotonically increasing)</option>
|
||||||
|
</select>
|
||||||
|
</div>
|
||||||
{editingName && (
|
{editingName && (
|
||||||
<div className="rounded border border-accent bg-accent/5 px-2 py-1 text-xs text-accent-text">
|
<div className="rounded border border-accent bg-accent/5 px-2 py-1 text-xs text-accent-text">
|
||||||
Editing "{editingName}"
|
Editing "{editingName}"
|
||||||
@@ -345,6 +366,10 @@ export function Metrics({ range, activePreset }: { range: TimeRange | null; acti
|
|||||||
<span className="text-text-subtle">Value param: </span>
|
<span className="text-text-subtle">Value param: </span>
|
||||||
<span className="font-mono">{selectedMetric.value_param ?? <span className="text-text-faint">none</span>}</span>
|
<span className="font-mono">{selectedMetric.value_param ?? <span className="text-text-faint">none</span>}</span>
|
||||||
</div>
|
</div>
|
||||||
|
<div>
|
||||||
|
<span className="text-text-subtle">Semantics: </span>
|
||||||
|
<span className="font-mono">{selectedMetric.semantics ?? 'gauge'}</span>
|
||||||
|
</div>
|
||||||
</div>
|
</div>
|
||||||
)}
|
)}
|
||||||
{groups.length === 0 ? (
|
{groups.length === 0 ? (
|
||||||
|
|||||||
@@ -1,6 +1,8 @@
|
|||||||
import { getJson, postJson, putJson, sendJson } from './shared'
|
import { getJson, postJson, putJson, sendJson } from './shared'
|
||||||
import type { MetricPrimitive } from './logs'
|
import type { MetricPrimitive } from './logs'
|
||||||
|
|
||||||
|
export type MetricSemantics = 'gauge' | 'counter'
|
||||||
|
|
||||||
export interface MetricDeclaration {
|
export interface MetricDeclaration {
|
||||||
name: string
|
name: string
|
||||||
message_match_regex: string
|
message_match_regex: string
|
||||||
@@ -11,6 +13,8 @@ export interface MetricDeclaration {
|
|||||||
/** Fixed labels applied on a match/non-match, on top of any captured params. */
|
/** Fixed labels applied on a match/non-match, on top of any captured params. */
|
||||||
matched_labels?: Record<string, string> | null
|
matched_labels?: Record<string, string> | null
|
||||||
unmatched_labels?: Record<string, string> | null
|
unmatched_labels?: Record<string, string> | null
|
||||||
|
/** Point-in-time reading ('gauge', e.g. queue depth) vs. monotonically-increasing total ('counter', e.g. requests served); guards which PromQL aggregations are valid. Defaults to 'gauge'. */
|
||||||
|
semantics?: MetricSemantics
|
||||||
}
|
}
|
||||||
|
|
||||||
export interface BucketValue {
|
export interface BucketValue {
|
||||||
|
|||||||
Reference in New Issue
Block a user