Updates for counts API #1347
Updates for counts API #1347
Conversation
WalkthroughThe changes introduce conditional filtering and grouping to the counts query functionality. New logic allows SQL-like filter strings to be generated from condition structures, with validation for operator-value consistency. The HTTP handler and related modules are updated to support filter-based queries, returning results in a consistent, JSON-formatted structure. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant HTTP_Handler as get_counts
participant QueryModule as CountsRequest
participant AlertsUtils as get_filter_string
participant DB
Client->>HTTP_Handler: POST /get_counts (CountsRequest)
HTTP_Handler->>QueryModule: Parse and validate request
alt With conditions
QueryModule->>AlertsUtils: get_filter_string(conditions)
AlertsUtils-->>QueryModule: SQL WHERE clause string
QueryModule->>QueryModule: get_df_sql() (build SQL)
QueryModule->>DB: Execute SQL query
DB-->>QueryModule: Records
else Without conditions
QueryModule->>QueryModule: get_bin_density()
end
QueryModule-->>HTTP_Handler: Records as JSON
HTTP_Handler-->>Client: JSON response (startTime, endTime, count)
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
⏰ Context from checks skipped due to timeout of 90000ms (7)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/prism/logstream/mod.rs (1)
352-353: Minor:conditions: NonerepetitionSince most internal callers will want “no conditions”, consider deriving
DefaultforCountsRequestwithconditionsdefaulting toNoneso this noise can be eliminated.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/alerts/alerts_utils.rs(1 hunks)src/handlers/http/query.rs(5 hunks)src/prism/logstream/mod.rs(1 hunks)src/query/mod.rs(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: coverage
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
🔇 Additional comments (2)
src/query/mod.rs (2)
317-324:group_byfield is defined but never used
CountConditionsexposes agroup_byvector, yet no call-site (includingget_df_sql) consumes it. Either wire it into the SQL generation or drop the field to avoid a misleading public API.Would you like guidance on integrating dynamic
GROUP BYsupport?
338-340:CountsRequestnow requires manual initialisation ofconditionseverywhereBe sure every
CountsRequestinstantiation (CLI, tests, etc.) setsconditions: Noneto avoid breaking changes. Consider#[serde(default)]on the new field to maintain backward compatibility with existing payloads.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/handlers/http/query.rs (1)
398-401: Staticfieldslist breaks grouped / projected COUNT queriesThe response always hard-codes
"fields": vec!["startTime", "endTime", "count"]Yet
get_df_sql()can emit queries containingGROUP BYcolumns (or other projections), so the first N columns of every record batch will be missing from the advertised schema. Clients relying on thefieldsheader will mis-align values.Reuse the header derived from the first JSON record instead:
- let res = json!({ - "fields": vec!["startTime", "endTime", "count"], - "records": records, - }); + let fields = records + .first() + .and_then(|v| v.as_object()) + .map(|m| m.keys().cloned().collect::<Vec<_>>()) + .unwrap_or_default(); + let res = json!({ + "fields": fields, + "records": records, + });
🧹 Nitpick comments (1)
src/handlers/http/query.rs (1)
383-389: Hard-codingsend_null = true&fields = trueoverrides caller intentIn the SQL branch the generated
Queryforcessend_nullandfieldstotrue, ignoring any query-string flags (sendNull,fields) that the client might have supplied. This changes API behaviour silently.Unless there is a strong reason, forward the user’s original intent:
- send_null: true, - fields: true, + send_null: counts_request.send_null, + fields: counts_request.fields,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/handlers/http/query.rs(5 hunks)src/otel/traces.rs(12 hunks)src/prism/logstream/mod.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/prism/logstream/mod.rs
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: coverage
🔇 Additional comments (1)
src/otel/traces.rs (1)
356-380: All good – only cosmetic refactor in helpersample_attributes()The newly-split
AnyValue::StringValuelines improve readability without touching logic or semantics.
Nothing else to call out.
parmesant
force-pushed
the
counts-api-update
branch
from
899f6fd to
cb70c09
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/handlers/http/query.rs (1)
392-404: Staticfieldsvector still wrong & dynamic header discardedThe response continues to hard-code
["start_time", "endTime", "count"], even when the SQL the backend generates may containGROUP BYcolumns (or any other projection).
We already flagged this in a previous review; clients will silently drop the additional columns returned by DataFusion.Re-use the header that is already available from the JSON objects or the
fieldsoutput ofget_records_and_fields.- let json_records = record_batches_to_json(&records)?; - let records = json_records.into_iter().map(Value::Object).collect_vec(); - - let res = json!({ - "fields": vec!["start_time", "endTime", "count"], - "records": records, - }); + let json_records = record_batches_to_json(&records)?; + let records = json_records + .into_iter() + .map(Value::Object) + .collect_vec(); + + // Derive the real field order from the first record + let fields = records + .first() + .map(|obj| obj.as_object().unwrap().keys().cloned().collect_vec()) + .unwrap_or_default(); + + let res = json!({ + "fields": fields, + "records": records, + });Failing to fix this will break every grouped COUNT request.
🧹 Nitpick comments (1)
src/handlers/http/query.rs (1)
379-390: Avoid hard-wiringsend_null/fields; honour caller intentThe new branch unconditionally sets
send_null: trueandfields: true.
This quietly overrides any flags supplied via query-string parameters, diverging from the behaviour of the non-SQL path and surprising API consumers.Either propagate the existing URL parameters or expose explicit JSON fields in
CountsRequestso that callers stay in control.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/handlers/http/query.rs(5 hunks)src/otel/traces.rs(12 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/otel/traces.rs
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: coverage
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/alerts/mod.rs (1)
346-367: Formatting logic duplicated – extract small helper
generate_filter_messagenow repeats the “value or no-value” formatting for both operands and again in the single-condition branch. A 3-line helper eliminates the duplication and future drift:+fn fmt_expr(c: &ConditionConfig) -> String { + match &c.value { + Some(v) => format!("{} {} {}", c.column, c.operator, v), + None => format!("{} {}", c.column, c.operator), + } +} @@ - let expr1_msg = if let Some(val) = &expr1.value { - format!("{} {} {}", expr1.column, expr1.operator, val) - } else { - format!("{} {}", expr1.column, expr1.operator) - }; - let expr2_msg = if let Some(val) = &expr2.value { - format!("{} {} {}", expr2.column, expr2.operator, val) - } else { - format!("{} {}", expr2.column, expr2.operator) - }; - - format!("[{} {op} {}]", expr1_msg, expr2_msg) + format!("[{} {op} {}]", fmt_expr(expr1), fmt_expr(expr2)) @@ - if let Some(val) = &expr.value { - format!("{} {} {}", expr.column, expr.operator, val) - } else { - format!("{} {}", expr.column, expr.operator) - } + fmt_expr(expr)Cleaner, easier to maintain, and no behavioural change.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/alerts/alerts_utils.rs(3 hunks)src/alerts/mod.rs(4 hunks)src/query/mod.rs(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/alerts/alerts_utils.rs
- src/query/mod.rs
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: coverage
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Default aarch64-apple-darwin
🔇 Additional comments (2)
src/alerts/mod.rs (2)
223-256: Enum nowPartialEq/Eq– solid for value-presence checksDeriving
PartialEq/EqonWhereConfigOperatorenables the new validation logic below without boiler-platematches!calls or manual implementations. Good call.
326-330:ConditionConfig.valueis nowOption– audit all call-sitesChanging
valuetoOption<String>is a breaking change. Althoughgenerate_filter_messageandvalidate_configswere updated, other helpers (alerts_utils::match_alert_operator, SQL builders, etc.) may still assumeSome(v).
Please grep/scan the codebase to ensure every pattern ‑condition.value.unwrap()oris_some()logic – is revised, or runtime panics will surface.
parmesant
force-pushed
the
counts-api-update
branch
from
bb6399e to
dfe5191
Compare
counts API will also work with applied filters
parmesant
force-pushed
the
counts-api-update
branch
from
dfe5191 to
8077565
Compare
counts API will also work with applied filters
Fixes #XXXX.
Description
This PR has:
Summary by CodeRabbit
New Features
Bug Fixes
Style
Documentation