Route bare ottyl_seq/ottyl_ts Promql queries to the primary (ticket 107)
Audited every EngineQuery variant for the cross-snapshot join ticket 107 anticipated: Logs/Timeline/Stats/ParamDeclarations only ever read primary data (raw text + param declarations, which never left the primary), and MetricDeclarations/BucketValues/Promql only ever read profile data -- no variant actually needs both, so no join/fan-out layer is needed after all. One real gap surfaced by that audit: a bare ottyl_seq/ottyl_ts Promql query is evaluated by iterating self.logs_range() (evaluate_promql's reserved-metric branch), but such queries were routed to the profile actor's snapshot alongside real declared metrics -- and the profile actor stores no raw log rows, so these queries silently returned empty series instead of real data. EngineHandle::ask now parses the query just far enough to check its metric name and routes ottyl_seq/ottyl_ts to the primary instead, where evaluate_promql already works correctly against real log data. Added an actor-level integration test (promql_bare_ottyl_seq_works_through_the_real_actor_pipeline) that fails without this fix -- verified by temporarily disabling the routing check and confirming the test catches it. Also deduplicated config/profile.rs's local `ask` test helper, which had already once diverged from api::EngineHandle::ask's routing logic and caused ticket 104 test failures (fixed then, but the duplication itself was the root cause) -- it now delegates to the real one. 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
3ee6dddb0e
commit
9322aac7e1
+49
-8
@@ -248,15 +248,23 @@ fn mime_for_extension(extension: &str) -> &'static str {
|
|||||||
}
|
}
|
||||||
|
|
||||||
impl EngineHandle {
|
impl EngineHandle {
|
||||||
async fn ask(&self, query: EngineQuery) -> Result<EngineResponse, StatusCode> {
|
pub(crate) async fn ask(&self, query: EngineQuery) -> Result<EngineResponse, StatusCode> {
|
||||||
// Metric declarations and bucket data live on the profile actor (ticket 104), not the
|
// Metric declarations and bucket data live on the profile actor (ticket 104), not the
|
||||||
// primary -- route those query variants through its reader instead.
|
// primary -- route those query variants through its reader instead. Exception: a bare
|
||||||
if matches!(
|
// `ottyl_seq`/`ottyl_ts` Promql query isn't real metric data, it's derived from the
|
||||||
query,
|
// primary's raw ingested sequence/timestamps (ticket 107) -- the profile actor's
|
||||||
EngineQuery::MetricDeclarations
|
// snapshot holds no raw log rows to compute that from, so route those to the primary.
|
||||||
| EngineQuery::BucketValues { .. }
|
let is_reserved_promql = matches!(&query, EngineQuery::Promql { query, .. }
|
||||||
| EngineQuery::Promql { .. }
|
if crate::engine::promql::PromqlQuery::parse(query)
|
||||||
) {
|
.is_ok_and(|parsed| crate::engine::snapshot::query::ottyl_reserved_numeric_value_fn(&parsed.metric_name).is_some()));
|
||||||
|
if !is_reserved_promql
|
||||||
|
&& matches!(
|
||||||
|
query,
|
||||||
|
EngineQuery::MetricDeclarations
|
||||||
|
| EngineQuery::BucketValues { .. }
|
||||||
|
| EngineQuery::Promql { .. }
|
||||||
|
)
|
||||||
|
{
|
||||||
let profile_handle = self.profile_reader.handle();
|
let profile_handle = self.profile_reader.handle();
|
||||||
let guard = profile_handle
|
let guard = profile_handle
|
||||||
.enter()
|
.enter()
|
||||||
@@ -1588,6 +1596,39 @@ mod tests {
|
|||||||
assert_eq!(points[1], json!([2000, 1.0]));
|
assert_eq!(points[1], json!([2000, 1.0]));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Ticket 107: a bare `ottyl_seq`/`ottyl_ts` Promql query is derived from the primary's raw
|
||||||
|
/// ingested sequence/timestamps, not from any declared metric's bucket data -- it must keep
|
||||||
|
/// working through the real actor pipeline (`EngineHandle::ask` routes it to the primary,
|
||||||
|
/// not the profile actor, which stores no raw log rows to compute it from) with no metric
|
||||||
|
/// ever declared.
|
||||||
|
#[tokio::test]
|
||||||
|
async fn promql_bare_ottyl_seq_works_through_the_real_actor_pipeline() {
|
||||||
|
let (app, engine, _settings, _dir) = test_app();
|
||||||
|
|
||||||
|
for (sequence, text) in [(0u64, "first"), (1u64, "second"), (2u64, "third")] {
|
||||||
|
engine
|
||||||
|
.tx
|
||||||
|
.send(EngineMessage::Ingest(IngestedLine {
|
||||||
|
sequence,
|
||||||
|
timestamp: Timestamp::from_millis(sequence as i64 * 1000),
|
||||||
|
text: text.into(),
|
||||||
|
}))
|
||||||
|
.await
|
||||||
|
.unwrap();
|
||||||
|
}
|
||||||
|
engine.barrier().await;
|
||||||
|
|
||||||
|
let (status, body) = get_json(&app, "/api/query?promql=ottyl_seq").await;
|
||||||
|
assert_eq!(status, StatusCode::OK);
|
||||||
|
let series = body.as_array().unwrap();
|
||||||
|
assert_eq!(series.len(), 1);
|
||||||
|
let points = series[0]["points"].as_array().unwrap();
|
||||||
|
assert_eq!(
|
||||||
|
*points,
|
||||||
|
vec![json!([0, 0.0]), json!([1000, 1.0]), json!([2000, 2.0])]
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
#[tokio::test]
|
#[tokio::test]
|
||||||
async fn promql_query_rejects_malformed_query_with_400() {
|
async fn promql_query_rejects_malformed_query_with_400() {
|
||||||
let (app, _engine, _settings, _dir) = test_app();
|
let (app, _engine, _settings, _dir) = test_app();
|
||||||
|
|||||||
+3
-15
@@ -751,21 +751,9 @@ mod tests {
|
|||||||
}
|
}
|
||||||
|
|
||||||
async fn ask(engine: &EngineHandle, query: EngineQuery) -> EngineResponse {
|
async fn ask(engine: &EngineHandle, query: EngineQuery) -> EngineResponse {
|
||||||
// Metric declarations and bucket data live on the profile actor (ticket 104), not the
|
// Delegates to the real routing in `api::EngineHandle::ask` rather than reimplementing
|
||||||
// primary -- mirrors the routing in `api::EngineHandle::ask`.
|
// it -- an earlier duplicate here diverged from it and silently read stale/empty data.
|
||||||
let profile_query = matches!(
|
engine.ask(query).await.expect("engine unavailable")
|
||||||
query,
|
|
||||||
EngineQuery::MetricDeclarations
|
|
||||||
| EngineQuery::BucketValues { .. }
|
|
||||||
| EngineQuery::Promql { .. }
|
|
||||||
);
|
|
||||||
let read_handle = if profile_query {
|
|
||||||
engine.profile_reader.handle()
|
|
||||||
} else {
|
|
||||||
engine.reader.handle()
|
|
||||||
};
|
|
||||||
let guard = read_handle.enter().expect("engine unavailable");
|
|
||||||
guard.handle_query_ro(query)
|
|
||||||
}
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
|
|||||||
@@ -1,58 +0,0 @@
|
|||||||
# 107 — Fan out and merge queries that need both raw logs and profile-derived data
|
|
||||||
|
|
||||||
## Summary
|
|
||||||
|
|
||||||
Today, `Logs`/`Timeline` queries read raw log text *and*
|
|
||||||
profile-derived params/metrics from a single `EngineSnapshot` in one
|
|
||||||
pass — e.g. `query.rs`'s log-entry building calls `log_entry_params`
|
|
||||||
against `compiled_params` from the same snapshot the raw text came
|
|
||||||
from. Once params/logs live on the primary and metrics live on the
|
|
||||||
profile actor (per tickets 103-106), any query needing both becomes a
|
|
||||||
real join across two snapshots instead of one borrow. This is the
|
|
||||||
piece of [[0001-actor-split]]'s design explicitly called out as "a
|
|
||||||
query-fan-out/merge layer is still needed."
|
|
||||||
|
|
||||||
## Where
|
|
||||||
|
|
||||||
- `src/engine/snapshot/query.rs` (`handle_query_inner` and the log-
|
|
||||||
entry-building helpers around line 70-117 per prior research) — the
|
|
||||||
read path that currently assumes one `EngineSnapshot` has everything
|
|
||||||
a `Logs`/`Timeline` response needs.
|
|
||||||
- `src/engine/message.rs:12-27` (`EngineQuery`) — currently answered
|
|
||||||
entirely off the primary's `left_right` reader
|
|
||||||
(`EngineHandle::ask`/`handle_query_ro`, per
|
|
||||||
[[0001-actor-split]]'s "queries already bypass this actor" note);
|
|
||||||
after the split, some query variants need data from both actors'
|
|
||||||
read sides.
|
|
||||||
- `src/api/mod.rs` — wherever `EngineQuery` results are turned into
|
|
||||||
HTTP responses; the merge/join logic likely belongs here or in a new
|
|
||||||
helper alongside `query.rs`, not duplicated per handler.
|
|
||||||
- Dashboard/PromQL panel queries (`src/engine/promql.rs`) — confirm
|
|
||||||
whether these are profile-only reads (metrics live entirely on the
|
|
||||||
profile actor) and therefore need no fan-out, versus `Logs`/
|
|
||||||
`Timeline` which do.
|
|
||||||
|
|
||||||
## Scope
|
|
||||||
|
|
||||||
- Identify every `EngineQuery` variant that currently reads both raw
|
|
||||||
log data and profile-derived data (params, metrics) from one
|
|
||||||
snapshot, and design the two-snapshot join for each — likely: read
|
|
||||||
matching log entries from the primary by sequence range, then look
|
|
||||||
up each entry's profile-derived fields (declared param values,
|
|
||||||
metric-driven fields) from the profile actor's snapshot by the same
|
|
||||||
sequence, filtering/merging results.
|
|
||||||
- Confirm ordering/consistency expectations: the profile actor can lag
|
|
||||||
the primary (per [[0001-actor-split]]'s accepted eviction/lag
|
|
||||||
tradeoff), so a joined query may find a primary-side log entry with
|
|
||||||
no corresponding profile-side data yet — decide and document the
|
|
||||||
behavior (omit those fields vs. error) rather than leaving it
|
|
||||||
implicit.
|
|
||||||
- Update PromQL/dashboard panel query handling only if it's confirmed
|
|
||||||
to need primary-side data too (e.g. `ottyl_seq`/`ottyl_ts` bare
|
|
||||||
queries per prior ticket history) — otherwise leave it reading the
|
|
||||||
profile actor alone.
|
|
||||||
- Out of scope: query result caching or performance optimization
|
|
||||||
beyond what's needed for correctness — this ticket is about
|
|
||||||
producing correct joined results, not making them fast (see
|
|
||||||
[[0005-additional-read-indexes]] for read-path performance work,
|
|
||||||
which should be sequenced after this settles).
|
|
||||||
Reference in New Issue
Block a user