mirror of
https://github.com/GreptimeTeam/greptimedb.git
synced 2026-10-15 16:32:34 +00:00
fix(prometheus): honor label matchers in __name__ values query (#9134)
* fix(prometheus): honor label matchers in __name__ values query
`/api/v1/label/__name__/values?match[]={pod="abc"}` dropped every matcher
other than `__name__` and returned all metrics in the schema. No error,
just the wrong list. Grafana's metrics browser sends this request, so
picking a label value there did nothing.
Selectors that only constrain `__name__` keep answering from table
metadata. A selector constraining an ordinary label now goes to the data:
scan each metric engine physical table for distinct `__table_id` in the
time range, map the ids back to metric names, then apply the selector's
own `__name__` matchers.
Only metric engine tables are covered; other engines share no column space
to scan.
Signed-off-by: Dennis Zhuang <killme2008@gmail.com>
* refactor(prometheus): batch-resolve metric names by table id
Building a full table-id-to-name map meant walking every table in the
schema and holding all of them in memory, just to name the handful the
scan returned. Use `tables_by_ids` instead — one batch KV read over the
ids the scan actually produced.
The catalog walk stays, but only to find the physical tables to scan.
Signed-off-by: Dennis Zhuang <killme2008@gmail.com>
* fix(promql): read an absent label as the empty string
A matcher on a label the series does not carry only worked when the table
had no column for it at all. Where the column exists but is NULL on that
row -- the norm for logical metrics sharing a metric engine physical
table, which holds the union of their label columns -- three-valued logic
dropped the row, so `host!="host1"` and `host=""` missed every metric
without a host label.
Coalesce nullable string label columns to "" for matchers that accept the
empty string, rather than only for the OTLP temporality marker. Equality
matchers are untouched; they cannot match NULL either way.
This is the Prometheus compatibility fix #8970 deliberately kept out of
its own scope. The cost is visible in the regex sqlness plan: the
predicate becomes a CASE, so the scan loses its LastRow selector and
grows a FilterExec. Only negative and empty-accepting matchers pay it.
Signed-off-by: Dennis Zhuang <killme2008@gmail.com>
* fix(promql): don't panic on pre-epoch label value bounds
`rewrite_label_values_query` unwrapped `duration_since(UNIX_EPOCH)`, which
returns an error for an instant before the epoch. `start=1969-12-31T23:59:59Z`
parses as valid RFC3339, so the request panicked instead of answering.
Recover the sign from the error branch, and report a value beyond i64
milliseconds as an error rather than wrapping the cast.
Signed-off-by: Dennis Zhuang <killme2008@gmail.com>
* refactor(prometheus): drop applicable_matchers, share the distinct scan
With the planner reading an absent label as empty, the frontend no longer
needs to pre-filter matchers per physical table. Removing that exposed a
second problem: a physical table that never took a column from a logical
table exposes no `__table_id`, and projecting it failed the whole request.
Skip those tables; the only thing that can miss is a metric with no labels.
Also pulls out the plan-build-execute-collect sequence the two label value
scans had in common.
Signed-off-by: Dennis Zhuang <killme2008@gmail.com>
---------
Signed-off-by: Dennis Zhuang <killme2008@gmail.com>
This commit is contained in:
@@ -13,6 +13,7 @@
|
||||
// limitations under the License.
|
||||
|
||||
use std::any::Any;
|
||||
use std::time::SystemTime;
|
||||
|
||||
use common_error::ext::ErrorExt;
|
||||
use common_error::status_code::StatusCode;
|
||||
@@ -202,6 +203,13 @@ pub enum Error {
|
||||
location: Location,
|
||||
},
|
||||
|
||||
#[snafu(display("Time out of the representable millisecond range: {:?}", time))]
|
||||
SystemTimeOutOfRange {
|
||||
time: SystemTime,
|
||||
#[snafu(implicit)]
|
||||
location: Location,
|
||||
},
|
||||
|
||||
#[snafu(display("vector cannot contain metrics with the same labelset"))]
|
||||
SameLabelSet {
|
||||
#[snafu(implicit)]
|
||||
@@ -243,6 +251,7 @@ impl ErrorExt for Error {
|
||||
| UnsupportedMatcherOp { .. }
|
||||
| SameLabelSet { .. }
|
||||
| TimestampOutOfRange { .. }
|
||||
| SystemTimeOutOfRange { .. }
|
||||
| InvalidRegularExpression { .. }
|
||||
| InvalidDestinationLabelName { .. } => StatusCode::InvalidArguments,
|
||||
|
||||
|
||||
@@ -23,9 +23,31 @@ use snafu::{OptionExt, ResultExt};
|
||||
use table::TableRef;
|
||||
|
||||
use crate::promql::error::{
|
||||
DataFusionPlanningSnafu, Result, TimeIndexNotFoundSnafu, TimestampOutOfRangeSnafu,
|
||||
DataFusionPlanningSnafu, Result, SystemTimeOutOfRangeSnafu, TimeIndexNotFoundSnafu,
|
||||
TimestampOutOfRangeSnafu,
|
||||
};
|
||||
|
||||
/// Converts a [`SystemTime`] to a millisecond [`Timestamp`].
|
||||
///
|
||||
/// `duration_since` reports an instant before the epoch as an error rather than
|
||||
/// a negative duration, and an RFC3339 query parameter can name one, so the sign
|
||||
/// is recovered here instead of unwrapping.
|
||||
fn millis_since_epoch(time: SystemTime) -> Result<Timestamp> {
|
||||
let (millis, before_epoch) = match time.duration_since(UNIX_EPOCH) {
|
||||
Ok(duration) => (duration.as_millis(), false),
|
||||
Err(earlier) => (earlier.duration().as_millis(), true),
|
||||
};
|
||||
let millis = i64::try_from(millis)
|
||||
.ok()
|
||||
.with_context(|| SystemTimeOutOfRangeSnafu { time })?;
|
||||
|
||||
Ok(Timestamp::new_millisecond(if before_epoch {
|
||||
-millis
|
||||
} else {
|
||||
millis
|
||||
}))
|
||||
}
|
||||
|
||||
fn build_time_filter(time_index_expr: Expr, start: Timestamp, end: Timestamp) -> Expr {
|
||||
time_index_expr
|
||||
.clone()
|
||||
@@ -67,14 +89,12 @@ pub fn rewrite_label_values_query(
|
||||
})?;
|
||||
|
||||
// We only support millisecond precision at most.
|
||||
let start =
|
||||
Timestamp::new_millisecond(start.duration_since(UNIX_EPOCH).unwrap().as_millis() as i64);
|
||||
let start = millis_since_epoch(start)?;
|
||||
let start = start.convert_to(unit).context(TimestampOutOfRangeSnafu {
|
||||
timestamp: start.value(),
|
||||
unit,
|
||||
})?;
|
||||
let end =
|
||||
Timestamp::new_millisecond(end.duration_since(UNIX_EPOCH).unwrap().as_millis() as i64);
|
||||
let end = millis_since_epoch(end)?;
|
||||
let end = end.convert_to(unit).context(TimestampOutOfRangeSnafu {
|
||||
timestamp: end.value(),
|
||||
unit,
|
||||
@@ -98,3 +118,28 @@ pub fn rewrite_label_values_query(
|
||||
|
||||
Ok(logical_plan)
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use std::time::Duration;
|
||||
|
||||
use super::*;
|
||||
|
||||
#[test]
|
||||
fn millis_before_the_epoch_are_negative() {
|
||||
// `SystemTime::duration_since` reports these as an error; unwrapping it
|
||||
// panicked on any request whose RFC3339 start named a pre-epoch instant.
|
||||
let time = UNIX_EPOCH - Duration::from_millis(1);
|
||||
assert_eq!(millis_since_epoch(time).unwrap().value(), -1);
|
||||
assert_eq!(millis_since_epoch(UNIX_EPOCH).unwrap().value(), 0);
|
||||
|
||||
let time = UNIX_EPOCH + Duration::from_millis(1);
|
||||
assert_eq!(millis_since_epoch(time).unwrap().value(), 1);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn millis_beyond_i64_are_rejected() {
|
||||
let time = UNIX_EPOCH + Duration::from_secs(1 << 60);
|
||||
assert!(millis_since_epoch(time).is_err());
|
||||
}
|
||||
}
|
||||
|
||||
@@ -2687,13 +2687,17 @@ impl PromPlanner {
|
||||
|
||||
let accepts_empty = matcher.is_match("");
|
||||
let column_name = Self::find_case_sensitive_column(table_schema, matcher.name.as_str());
|
||||
// Prometheus reads a label a series does not carry as the empty
|
||||
// string. A row can miss a label two ways: the table has no column
|
||||
// for it, or the column exists but is NULL on that row — the latter
|
||||
// is the norm for logical metrics sharing a physical table, which
|
||||
// holds the union of their label columns.
|
||||
let col = if let Some(column_name) = column_name {
|
||||
let column = DfExpr::Column(Column::from_name(&column_name));
|
||||
let field = table_schema
|
||||
.index_of_column_by_name(None, &column_name)
|
||||
.map(|index| table_schema.field(index));
|
||||
if accepts_empty
|
||||
&& column_name == OTLP_AGGREGATION_TEMPORALITY_LABEL
|
||||
&& let Some(data_type) = field
|
||||
.filter(|field| {
|
||||
field.is_nullable()
|
||||
@@ -2712,7 +2716,6 @@ impl PromPlanner {
|
||||
}
|
||||
} else {
|
||||
DfExpr::Literal(ScalarValue::Utf8(Some(String::new())), None)
|
||||
.alias(matcher.name.clone())
|
||||
};
|
||||
let lit = DfExpr::Literal(ScalarValue::Utf8(Some(matcher.value)), None);
|
||||
let expr = match matcher.op {
|
||||
|
||||
@@ -524,7 +524,7 @@ async fn delta_mixed_ranges_drop_and_float_ranges_sum() {
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn temporality_matchers_treat_null_as_absent() {
|
||||
async fn matchers_read_absent_labels_as_empty() {
|
||||
let marker = OTLP_AGGREGATION_TEMPORALITY_LABEL;
|
||||
let schema = Arc::new(ArrowSchema::new(vec![Field::new(
|
||||
marker,
|
||||
@@ -596,31 +596,82 @@ async fn temporality_matchers_treat_null_as_absent() {
|
||||
);
|
||||
}
|
||||
|
||||
let ordinary_schema = Arc::new(ArrowSchema::new(vec![Field::new(
|
||||
// The rule is about NULL, not about the marker: any nullable label column
|
||||
// can be NULL where the series does not carry the label. A non-nullable one
|
||||
// has nothing to normalize.
|
||||
for (nullable, wants_coalesce) in [(true, true), (false, false)] {
|
||||
let ordinary_schema = Arc::new(ArrowSchema::new(vec![Field::new(
|
||||
"label",
|
||||
ArrowDataType::Utf8,
|
||||
nullable,
|
||||
)]));
|
||||
let ordinary_scan = LogicalPlanBuilder::scan(
|
||||
"ordinary_labels",
|
||||
provider_as_source(Arc::new(
|
||||
MemTable::try_new(ordinary_schema, vec![vec![]]).unwrap(),
|
||||
)),
|
||||
None,
|
||||
)
|
||||
.unwrap()
|
||||
.build()
|
||||
.unwrap();
|
||||
let PromExpr::VectorSelector(selector) =
|
||||
parser::parse(r#"metric{label!="delta"}"#).unwrap()
|
||||
else {
|
||||
unreachable!()
|
||||
};
|
||||
let expressions = PromPlanner::matchers_to_expr(selector.matchers, ordinary_scan.schema())
|
||||
.unwrap()
|
||||
.iter()
|
||||
.map(ToString::to_string)
|
||||
.join(" AND ");
|
||||
assert_eq!(
|
||||
wants_coalesce,
|
||||
expressions.contains("coalesce"),
|
||||
"nullable={nullable}: {expressions}"
|
||||
);
|
||||
}
|
||||
|
||||
// The other way a label goes absent is having no column for it at all. The
|
||||
// literal standing in for the label has to be usable as a predicate.
|
||||
let schema = Arc::new(ArrowSchema::new(vec![Field::new(
|
||||
"label",
|
||||
ArrowDataType::Utf8,
|
||||
true,
|
||||
)]));
|
||||
let ordinary_scan = LogicalPlanBuilder::scan(
|
||||
let batch = RecordBatch::try_new(
|
||||
schema.clone(),
|
||||
vec![Arc::new(StringArray::from(vec![Some("value")]))],
|
||||
)
|
||||
.unwrap();
|
||||
let scan = LogicalPlanBuilder::scan(
|
||||
"ordinary_labels",
|
||||
provider_as_source(Arc::new(
|
||||
MemTable::try_new(ordinary_schema, vec![vec![]]).unwrap(),
|
||||
MemTable::try_new(schema, vec![vec![batch]]).unwrap(),
|
||||
)),
|
||||
None,
|
||||
)
|
||||
.unwrap()
|
||||
.build()
|
||||
.unwrap();
|
||||
let PromExpr::VectorSelector(selector) = parser::parse(r#"metric{label!="delta"}"#).unwrap()
|
||||
let PromExpr::VectorSelector(selector) = parser::parse(r#"metric{absent!="delta"}"#).unwrap()
|
||||
else {
|
||||
unreachable!()
|
||||
};
|
||||
let expressions = PromPlanner::matchers_to_expr(selector.matchers, ordinary_scan.schema())
|
||||
let expressions = PromPlanner::matchers_to_expr(selector.matchers, scan.schema()).unwrap();
|
||||
assert_eq!(
|
||||
r#"Utf8("") != Utf8("delta")"#,
|
||||
expressions.iter().map(ToString::to_string).join(" AND ")
|
||||
);
|
||||
|
||||
let plan = LogicalPlanBuilder::from(scan)
|
||||
.filter(conjunction(expressions).unwrap())
|
||||
.unwrap()
|
||||
.iter()
|
||||
.map(ToString::to_string)
|
||||
.join(" AND ");
|
||||
assert!(!expressions.contains("coalesce"), "{expressions}");
|
||||
.build()
|
||||
.unwrap();
|
||||
let (_, batches) = execute(plan, &build_query_engine_state()).await;
|
||||
let rows = batches.iter().map(|batch| batch.num_rows()).sum::<usize>();
|
||||
assert_eq!(1, rows);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
|
||||
Reference in New Issue
Block a user