carverauto/serviceradar
1
1
Fork 0
Code Issues 190 Pull requests 16 Projects Releases 173 Packages Wiki Activity Actions

fixing silent filter errors in srql #2500

Merged
mfreeman451 merged 2 commits from refs/pull/2500/head into main 2025-12-03 18:12:49 +00:00
Conversation 2 Commits 2 Files changed 13 +746 -13
mfreeman451 commented 2025-12-03 06:54:02 +00:00 (Migrated from github.com)
Owner
Copy link

Imported from GitHub pull request.

Original GitHub pull request: #2050
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/2050
Original created: 2025-12-03T06:54:02Z
Original updated: 2025-12-03T18:12:52Z
Original head: carverauto/serviceradar:2049-bugsrql-inconsistent-error-handling-for-filters
Original base: main
Original merged: 2025-12-03T18:12:49Z by @mfreeman451

User description

IMPORTANT: Please sign the Developer Certificate of Origin

Thank you for your contribution to ServiceRadar. Please note, when contributing, the developer must include
a DCO sign-off statement indicating the DCO acceptance in one commit message. Here
is an example DCO Signed-off-by line in a commit message:

Signed-off-by: J. Doe <j.doe@domain.com>

Describe your changes

Issue ticket number and link

Code checklist before requesting a review

  • I have signed the DCO?
  • The build completes without errors?
  • All tests are passing when running make test?

PR Type

Bug fix, Tests

Description

  • Replace silent filter field ignoring with explicit error returns across nine SRQL query modules

  • Add unit tests validating unknown filter fields return proper error messages

  • Update documentation with comprehensive supported filter fields reference per entity

  • Fix stats filter validation in logs and otel_metrics modules to reject unknown fields

Diagram Walkthrough

flowchart LR
  A["Silent catch-all<br/>_ => {}"] -->|Replace| B["Explicit error return<br/>InvalidRequest"]
  B -->|Applied to| C["9 Query Modules"]
  C -->|Includes| D["logs, traces, services<br/>pollers, cpu_metrics<br/>memory_metrics, disk_metrics<br/>timeseries_metrics, otel_metrics"]
  E["Add unit tests"] -->|Verify| B
  F["Update documentation"] -->|Reference| G["Supported filter fields<br/>by entity"]

File Walkthrough
Relevant files
Bug fix
9 files
cpu_metrics.rs
Replace silent filter catch-all with error return               
+36/-1   
disk_metrics.rs
Replace silent filter catch-all with error return               
+43/-1   
logs.rs
Fix silent filter and stats filter error handling               
+70/-2   
memory_metrics.rs
Replace silent filter catch-all with error return               
+43/-1   
otel_metrics.rs
Fix silent filter and stats filter error handling               
+48/-2   
pollers.rs
Replace silent filter catch-all with error return               
+43/-1   
services.rs
Replace silent filter catch-all with error return               
+43/-1   
timeseries_metrics.rs
Replace silent filter catch-all with error return               
+43/-1   
traces.rs
Replace silent filter catch-all with error return               
+43/-1   
Documentation
4 files
srql-language-reference.md
Add comprehensive supported filter fields documentation   
+191/-1 
proposal.md
Document fix rationale and implementation details               
+39/-0   
spec.md
Define requirements and test scenarios for fix                     
+70/-0   
tasks.md
Track implementation tasks and verification steps               
+33/-0   
Imported from GitHub pull request. Original GitHub pull request: #2050 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/2050 Original created: 2025-12-03T06:54:02Z Original updated: 2025-12-03T18:12:52Z Original head: carverauto/serviceradar:2049-bugsrql-inconsistent-error-handling-for-filters Original base: main Original merged: 2025-12-03T18:12:49Z by @mfreeman451 --- ### **User description** ## IMPORTANT: Please sign the Developer Certificate of Origin Thank you for your contribution to ServiceRadar. Please note, when contributing, the developer must include a [DCO sign-off statement]( https://developercertificate.org/) indicating the DCO acceptance in one commit message. Here is an example DCO Signed-off-by line in a commit message: ``` Signed-off-by: J. Doe <j.doe@domain.com> ``` ## Describe your changes ## Issue ticket number and link ## Code checklist before requesting a review - [ ] I have signed the DCO? - [ ] The build completes without errors? - [ ] All tests are passing when running make test? ___ ### **PR Type** Bug fix, Tests ___ ### **Description** - Replace silent filter field ignoring with explicit error returns across nine SRQL query modules - Add unit tests validating unknown filter fields return proper error messages - Update documentation with comprehensive supported filter fields reference per entity - Fix stats filter validation in logs and otel_metrics modules to reject unknown fields ___ ### Diagram Walkthrough ```mermaid flowchart LR A["Silent catch-all<br/>_ => {}"] -->|Replace| B["Explicit error return<br/>InvalidRequest"] B -->|Applied to| C["9 Query Modules"] C -->|Includes| D["logs, traces, services<br/>pollers, cpu_metrics<br/>memory_metrics, disk_metrics<br/>timeseries_metrics, otel_metrics"] E["Add unit tests"] -->|Verify| B F["Update documentation"] -->|Reference| G["Supported filter fields<br/>by entity"] ``` <details> <summary><h3> File Walkthrough</h3></summary> <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td><strong>Bug fix</strong></td><td><details><summary>9 files</summary><table> <tr> <td><strong>cpu_metrics.rs</strong><dd><code>Replace silent filter catch-all with error return</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2050/files#diff-400c860c805cd3e1a5e7f7c42cece63cce3e62c1f3fd8b49a5803e26b40fe31e">+36/-1</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>disk_metrics.rs</strong><dd><code>Replace silent filter catch-all with error return</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2050/files#diff-b5a536baa37176f971e96c789991202b483314e2a85d4556489b4568a5675162">+43/-1</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>logs.rs</strong><dd><code>Fix silent filter and stats filter error handling</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2050/files#diff-f4d3b33667f2e79a6ebe4cfff931f93c728d9a81c305ac13586e623850a504db">+70/-2</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>memory_metrics.rs</strong><dd><code>Replace silent filter catch-all with error return</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2050/files#diff-a12a227c4b5c224359799853f5fd7bbf626ec14b1e7eae40e4c65b1db6699417">+43/-1</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>otel_metrics.rs</strong><dd><code>Fix silent filter and stats filter error handling</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2050/files#diff-8106bfa2099b8a6d945b338532f8b1108467e2f0592ddbd64d546fcbce3e3613">+48/-2</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>pollers.rs</strong><dd><code>Replace silent filter catch-all with error return</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2050/files#diff-e2192a3ba3041ed995e0abf9973489b7dff550949ff75da41d31bf514e765cc4">+43/-1</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>services.rs</strong><dd><code>Replace silent filter catch-all with error return</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2050/files#diff-ad5e7e1a352406ed473a765de4856625f1bede590e7e2c724163bcd1e7b313e9">+43/-1</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>timeseries_metrics.rs</strong><dd><code>Replace silent filter catch-all with error return</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2050/files#diff-8450bff3aef4c1271bbf9b96809b4a5ac7b218e3f98c8b60387e35dbf4845bb0">+43/-1</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>traces.rs</strong><dd><code>Replace silent filter catch-all with error return</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2050/files#diff-f390b0aef82baa9c3438719276dca70be826318d7446074a83b4527558528e19">+43/-1</a>&nbsp; &nbsp; </td> </tr> </table></details></td></tr><tr><td><strong>Documentation</strong></td><td><details><summary>4 files</summary><table> <tr> <td><strong>srql-language-reference.md</strong><dd><code>Add comprehensive supported filter fields documentation</code>&nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2050/files#diff-7fa04d96755318e393180aaae41cb1374c1da332b3fca76c29e443fd7146ef5f">+191/-1</a>&nbsp; </td> </tr> <tr> <td><strong>proposal.md</strong><dd><code>Document fix rationale and implementation details</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2050/files#diff-a913209a981d4c6b3e8339293fcb062e02065c6485c215304d34a66dfca38997">+39/-0</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>spec.md</strong><dd><code>Define requirements and test scenarios for fix</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2050/files#diff-8ea36913f5d5236389f519283c919ffd1c3d1124de5684b954873958051c8dbb">+70/-0</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>tasks.md</strong><dd><code>Track implementation tasks and verification steps</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2050/files#diff-138a6873eaaab55e8974878fd16911bae1de7b8c708e006b996744428c2ecd5b">+33/-0</a>&nbsp; &nbsp; </td> </tr> </table></details></td></tr></tr></tbody></table> </details> ___
qodo-code-review[bot] commented 2025-12-03 06:54:38 +00:00 (Migrated from github.com)
Author
Owner
Copy link

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/2050#issuecomment-3605363257
Original created: 2025-12-03T06:54:38Z

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪
Breaking behavior change

Description: Changing stats filter handling from returning Ok(None) to explicit errors may break
clients relying on prior permissive behavior; confirm API versioning/compat guarantees and
error message stability to avoid denial-of-service from unexpected 400s.
logs.rs [429-444]

Referred Code 
fn build_stats_filter_clause(filter: &Filter) -> Result<Option<(String, Vec<SqlBindValue>)>> {
    match filter.field.as_str() {
        "trace_id" => build_text_clause("trace_id", filter),
        "span_id" => build_text_clause("span_id", filter),
        "service_name" | "service" => build_text_clause("service_name", filter),
        "service_version" => build_text_clause("service_version", filter),
        "service_instance" => build_text_clause("service_instance", filter),
        "scope_name" => build_text_clause("scope_name", filter),
        "scope_version" => build_text_clause("scope_version", filter),
        "severity_text" | "severity" | "level" => build_text_clause("severity_text", filter),
        "body" => build_text_clause("body", filter),
        "severity_number" => build_numeric_clause("severity_number", filter),
        other => Err(ServiceError::InvalidRequest(format!(
            "unsupported filter field for logs stats: '{other}'"
        ))),
    }
Information disclosure

Description: Stats filter clause now errors on unknown fields; if upstream inputs are partially
user-controlled, stricter errors could reveal entity names/field hints—review error
strings for information disclosure policies.
otel_metrics.rs [381-394]

Referred Code 
            FilterOp::NotEq => "(is_slow IS NULL OR is_slow <> ?)".to_string(),
            _ => {
                return Err(ServiceError::InvalidRequest(
                    "is_slow filter only supports equality".into(),
                ))
            }
        }
    }
    other => {
        return Err(ServiceError::InvalidRequest(format!(
            "unsupported filter field for otel_metrics stats: '{other}'"
        )));
    }
};
Ticket Compliance
🟢
🎫 #2049
🟢 Replace silent catch-all filter handling in events/logs (and any other SRQL modules) with
explicit InvalidRequest errors for unknown fields.
Ensure unknown filter fields do not get ignored; they must fail the query clearly (prevent
silent data leakage/confusion).
Align error handling with devices.rs behavior across modules, including stats/filter
contexts.
Codebase Duplication Compliance
⚪
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

⚪
Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
No audit logs: The changes focus on returning explicit InvalidRequest errors for unsupported filters and
adding unit tests/documentation, but there is no evidence in the new code that critical
actions are being logged with user, timestamp, action, and outcome.

Referred Code 
    return Err(ServiceError::InvalidRequest(format!(
        "unsupported filter field for logs: '{other}'"
    )));
}

Learn more about managing compliance generic rules or creating your own custom rules

  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label
Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/2050#issuecomment-3605363257 Original created: 2025-12-03T06:54:38Z --- ## PR Compliance Guide 🔍 <!-- https://github.com/carverauto/serviceradar/commit/ac7f7ff5da500dd294581ad44317f8fb164a53df --> Below is a summary of compliance checks for this PR:<br> <table><tbody><tr><td colspan='2'><strong>Security Compliance</strong></td></tr> <tr><td rowspan=2>⚪</td> <td><details><summary><strong>Breaking behavior change </strong></summary><br> <b>Description:</b> Changing stats filter handling from returning Ok(None) to explicit errors may break <br>clients relying on prior permissive behavior; confirm API versioning/compat guarantees and <br>error message stability to avoid denial-of-service from unexpected 400s.<br> <strong><a href='https://github.com/carverauto/serviceradar/pull/2050/files#diff-f4d3b33667f2e79a6ebe4cfff931f93c728d9a81c305ac13586e623850a504dbR429-R444'>logs.rs [429-444]</a></strong><br> <details open><summary>Referred Code</summary> ```rust fn build_stats_filter_clause(filter: &Filter) -> Result<Option<(String, Vec<SqlBindValue>)>> { match filter.field.as_str() { "trace_id" => build_text_clause("trace_id", filter), "span_id" => build_text_clause("span_id", filter), "service_name" | "service" => build_text_clause("service_name", filter), "service_version" => build_text_clause("service_version", filter), "service_instance" => build_text_clause("service_instance", filter), "scope_name" => build_text_clause("scope_name", filter), "scope_version" => build_text_clause("scope_version", filter), "severity_text" | "severity" | "level" => build_text_clause("severity_text", filter), "body" => build_text_clause("body", filter), "severity_number" => build_numeric_clause("severity_number", filter), other => Err(ServiceError::InvalidRequest(format!( "unsupported filter field for logs stats: '{other}'" ))), } ``` </details></details></td></tr> <tr><td><details><summary><strong>Information disclosure </strong></summary><br> <b>Description:</b> Stats filter clause now errors on unknown fields; if upstream inputs are partially <br>user-controlled, stricter errors could reveal entity names/field hints—review error <br>strings for information disclosure policies.<br> <strong><a href='https://github.com/carverauto/serviceradar/pull/2050/files#diff-8106bfa2099b8a6d945b338532f8b1108467e2f0592ddbd64d546fcbce3e3613R381-R394'>otel_metrics.rs [381-394]</a></strong><br> <details open><summary>Referred Code</summary> ```rust FilterOp::NotEq => "(is_slow IS NULL OR is_slow <> ?)".to_string(), _ => { return Err(ServiceError::InvalidRequest( "is_slow filter only supports equality".into(), )) } } } other => { return Err(ServiceError::InvalidRequest(format!( "unsupported filter field for otel_metrics stats: '{other}'" ))); } }; ``` </details></details></td></tr> <tr><td colspan='2'><strong>Ticket Compliance</strong></td></tr> <tr><td>🟢</td> <td> <details> <summary>🎫 <a href=https://github.com/carverauto/serviceradar/issues/2049>#2049</a></summary> <table width='100%'><tbody> <tr><td rowspan=3>🟢</td> <td>Replace silent catch-all filter handling in events/logs (and any other SRQL modules) with <br>explicit InvalidRequest errors for unknown fields.</td></tr> <tr><td>Ensure unknown filter fields do not get ignored; they must fail the query clearly (prevent <br>silent data leakage/confusion).</td></tr> <tr><td>Align error handling with devices.rs behavior across modules, including stats/filter <br>contexts.</td></tr> </tbody></table> </details> </td></tr> <tr><td colspan='2'><strong>Codebase Duplication Compliance</strong></td></tr> <tr><td>⚪</td><td><details><summary><strong>Codebase context is not defined </strong></summary> Follow the <a href='https://qodo-merge-docs.qodo.ai/core-abilities/rag_context_enrichment/'>guide</a> to enable codebase context checks. </details></td></tr> <tr><td colspan='2'><strong>Custom Compliance</strong></td></tr> <tr><td rowspan=5>🟢</td><td> <details><summary><strong>Generic: Meaningful Naming and Self-Documenting Code</strong></summary><br> **Objective:** Ensure all identifiers clearly express their purpose and intent, making code <br>self-documenting<br> **Status:** Passed<br> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td> <details><summary><strong>Generic: Robust Error Handling and Edge Case Management</strong></summary><br> **Objective:** Ensure comprehensive error handling that provides meaningful context and graceful <br>degradation<br> **Status:** Passed<br> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td> <details><summary><strong>Generic: Secure Error Handling</strong></summary><br> **Objective:** To prevent the leakage of sensitive system information through error messages while <br>providing sufficient detail for internal debugging.<br> **Status:** Passed<br> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td> <details><summary><strong>Generic: Secure Logging Practices</strong></summary><br> **Objective:** To ensure logs are useful for debugging and auditing without exposing sensitive <br>information like PII, PHI, or cardholder data.<br> **Status:** Passed<br> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td> <details><summary><strong>Generic: Security-First Input Validation and Data Handling</strong></summary><br> **Objective:** Ensure all data inputs are validated, sanitized, and handled securely to prevent <br>vulnerabilities<br> **Status:** Passed<br> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td rowspan=1>⚪</td> <td><details> <summary><strong>Generic: Comprehensive Audit Trails</strong></summary><br> **Objective:** To create a detailed and reliable record of critical system actions for security analysis <br>and compliance.<br> **Status:** <br><a href='https://github.com/carverauto/serviceradar/pull/2050/files#diff-f4d3b33667f2e79a6ebe4cfff931f93c728d9a81c305ac13586e623850a504dbR236-R239'><strong>No audit logs</strong></a>: The changes focus on returning explicit InvalidRequest errors for unsupported filters and <br>adding unit tests/documentation, but there is no evidence in the new code that critical <br>actions are being logged with user, timestamp, action, and outcome.<br> <details open><summary>Referred Code</summary> ```rust return Err(ServiceError::InvalidRequest(format!( "unsupported filter field for logs: '{other}'" ))); } ``` </details> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td align="center" colspan="2"> - [ ] Update <!-- /compliance --update_compliance=true --> </td></tr></tbody></table> <details><summary>Compliance status legend</summary> 🟢 - Fully Compliant<br> 🟡 - Partial Compliant<br> 🔴 - Not Compliant<br> ⚪ - Requires Further Human Verification<br> 🏷️ - Compliance label<br> </details>
qodo-code-review[bot] commented 2025-12-03 06:56:01 +00:00 (Migrated from github.com)
Author
Owner
Copy link

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/2050#issuecomment-3605367178
Original created: 2025-12-03T06:56:01Z

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Validate filter operation for boolean field

In build_stats_filter_clause, add validation to ensure only the equality
operator is used for the is_slow field, returning an error for other operators
to prevent incorrect query behavior.

rust/srql/src/query/otel_metrics.rs [361-397]

 fn build_stats_filter_clause(filter: &Filter) -> Result<Option<(String, Vec<SqlBindValue>)>> {
     let mut binds = Vec::new();
     let clause = match filter.field.as_str() {
         "trace_id" => build_text_clause("trace_id", filter, &mut binds)?,
         "span_id" => build_text_clause("span_id", filter, &mut binds)?,
         "service_name" | "service" => build_text_clause("service_name", filter, &mut binds)?,
         "span_name" => build_text_clause("span_name", filter, &mut binds)?,
         "metric_type" | "type" => build_text_clause("metric_type", filter, &mut binds)?,
         "component" => build_text_clause("component", filter, &mut binds)?,
         "http_method" => build_text_clause("http_method", filter, &mut binds)?,
         "http_route" => build_text_clause("http_route", filter, &mut binds)?,
         "http_status_code" => build_text_clause("http_status_code", filter, &mut binds)?,
         "grpc_service" => build_text_clause("grpc_service", filter, &mut binds)?,
         "grpc_method" => build_text_clause("grpc_method", filter, &mut binds)?,
         "grpc_status_code" => build_text_clause("grpc_status_code", filter, &mut binds)?,
         "is_slow" => {
-            let value = parse_bool(filter.value.as_scalar()?)?;
-            binds.push(SqlBindValue::Bool(value));
-            "is_slow = ?".to_string()
+            if filter.op == FilterOp::Eq {
+                let value = parse_bool(filter.value.as_scalar()?)?;
+                binds.push(SqlBindValue::Bool(value));
+                "is_slow = ?".to_string()
+            } else {
+                return Err(ServiceError::InvalidRequest(
+                    "is_slow filter only supports equality".into(),
+                ));
+            }
         }
         other => {
             return Err(ServiceError::InvalidRequest(format!(
                 "unsupported filter field for otel_metrics stats: '{other}'"
             )));
         }
     };
 
     Ok(Some((clause, binds)))
 }

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 8

__

Why: This suggestion identifies a bug where an unsupported filter operation on the is_slow field would be silently accepted, leading to incorrect query logic. The fix prevents this by adding a necessary validation check.

Medium
General
Improve test to handle all success cases

Improve the unknown_stats_filter_field_returns_error test by modifying the panic
message in the Ok arm to include the unexpected success value, making test
failures more informative.

rust/srql/src/query/logs.rs [652-680]

 fn unknown_stats_filter_field_returns_error() {
     let start = Utc.with_ymd_and_hms(2025, 1, 1, 0, 0, 0).unwrap();
     let end = start + ChronoDuration::hours(24);
     let plan = QueryPlan {
         entity: Entity::Logs,
         filters: vec![Filter {
             field: "unknown_field".into(),
             op: FilterOp::Eq,
             value: FilterValue::Scalar("test".to_string()),
         }],
         order: Vec::new(),
         limit: 100,
         offset: 0,
         time_range: Some(TimeRange { start, end }),
         stats: Some("count() as total".to_string()),
     };
 
     let result = build_stats_query(&plan);
     match result {
         Err(err) => {
             assert!(
                 err.to_string().contains("unsupported filter field"),
                 "error should mention unsupported filter field: {}",
                 err
             );
         }
-        Ok(_) => panic!("expected error for unknown stats filter field"),
+        Ok(res) => panic!(
+            "expected error for unknown stats filter field, but got Ok({:?})",
+            res
+        ),
     }
 }
  • Apply / Chat
Suggestion importance[1-10]: 3

__

Why: The suggestion correctly identifies that any Ok result from build_stats_query is a test failure, and improves the test's panic message to be more descriptive, which aids in debugging.

Low
  • Update
Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/2050#issuecomment-3605367178 Original created: 2025-12-03T06:56:01Z --- ## PR Code Suggestions ✨ <!-- ac7f7ff --> Explore these optional code suggestions: <table><thead><tr><td><strong>Category</strong></td><td align=left><strong>Suggestion&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </strong></td><td align=center><strong>Impact</strong></td></tr><tbody><tr><td rowspan=1>Possible issue</td> <td> <details><summary>Validate filter operation for boolean field</summary> ___ **In <code>build_stats_filter_clause</code>, add validation to ensure only the equality <br>operator is used for the <code>is_slow</code> field, returning an error for other operators <br>to prevent incorrect query behavior.** [rust/srql/src/query/otel_metrics.rs [361-397]](https://github.com/carverauto/serviceradar/pull/2050/files#diff-8106bfa2099b8a6d945b338532f8b1108467e2f0592ddbd64d546fcbce3e3613R361-R397) ```diff fn build_stats_filter_clause(filter: &Filter) -> Result<Option<(String, Vec<SqlBindValue>)>> { let mut binds = Vec::new(); let clause = match filter.field.as_str() { "trace_id" => build_text_clause("trace_id", filter, &mut binds)?, "span_id" => build_text_clause("span_id", filter, &mut binds)?, "service_name" | "service" => build_text_clause("service_name", filter, &mut binds)?, "span_name" => build_text_clause("span_name", filter, &mut binds)?, "metric_type" | "type" => build_text_clause("metric_type", filter, &mut binds)?, "component" => build_text_clause("component", filter, &mut binds)?, "http_method" => build_text_clause("http_method", filter, &mut binds)?, "http_route" => build_text_clause("http_route", filter, &mut binds)?, "http_status_code" => build_text_clause("http_status_code", filter, &mut binds)?, "grpc_service" => build_text_clause("grpc_service", filter, &mut binds)?, "grpc_method" => build_text_clause("grpc_method", filter, &mut binds)?, "grpc_status_code" => build_text_clause("grpc_status_code", filter, &mut binds)?, "is_slow" => { - let value = parse_bool(filter.value.as_scalar()?)?; - binds.push(SqlBindValue::Bool(value)); - "is_slow = ?".to_string() + if filter.op == FilterOp::Eq { + let value = parse_bool(filter.value.as_scalar()?)?; + binds.push(SqlBindValue::Bool(value)); + "is_slow = ?".to_string() + } else { + return Err(ServiceError::InvalidRequest( + "is_slow filter only supports equality".into(), + )); + } } other => { return Err(ServiceError::InvalidRequest(format!( "unsupported filter field for otel_metrics stats: '{other}'" ))); } }; Ok(Some((clause, binds))) } ``` `[To ensure code accuracy, apply this suggestion manually]` <details><summary>Suggestion importance[1-10]: 8</summary> __ Why: This suggestion identifies a bug where an unsupported filter operation on the `is_slow` field would be silently accepted, leading to incorrect query logic. The fix prevents this by adding a necessary validation check. </details></details></td><td align=center>Medium </td></tr><tr><td rowspan=1>General</td> <td> <details><summary>Improve test to handle all success cases</summary> ___ **Improve the <code>unknown_stats_filter_field_returns_error</code> test by modifying the panic <br>message in the <code>Ok</code> arm to include the unexpected success value, making test <br>failures more informative.** [rust/srql/src/query/logs.rs [652-680]](https://github.com/carverauto/serviceradar/pull/2050/files#diff-f4d3b33667f2e79a6ebe4cfff931f93c728d9a81c305ac13586e623850a504dbR652-R680) ```diff fn unknown_stats_filter_field_returns_error() { let start = Utc.with_ymd_and_hms(2025, 1, 1, 0, 0, 0).unwrap(); let end = start + ChronoDuration::hours(24); let plan = QueryPlan { entity: Entity::Logs, filters: vec![Filter { field: "unknown_field".into(), op: FilterOp::Eq, value: FilterValue::Scalar("test".to_string()), }], order: Vec::new(), limit: 100, offset: 0, time_range: Some(TimeRange { start, end }), stats: Some("count() as total".to_string()), }; let result = build_stats_query(&plan); match result { Err(err) => { assert!( err.to_string().contains("unsupported filter field"), "error should mention unsupported filter field: {}", err ); } - Ok(_) => panic!("expected error for unknown stats filter field"), + Ok(res) => panic!( + "expected error for unknown stats filter field, but got Ok({:?})", + res + ), } } ``` - [ ] **Apply / Chat** <!-- /improve --apply_suggestion=1 --> <details><summary>Suggestion importance[1-10]: 3</summary> __ Why: The suggestion correctly identifies that any `Ok` result from `build_stats_query` is a test failure, and improves the test's panic message to be more descriptive, which aids in debugging. </details></details></td><td align=center>Low </td></tr> <tr><td align="center" colspan="2"> - [ ] Update <!-- /improve_multi --more_suggestions=true --> </td><td></td></tr></tbody></table>
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
1week

   
2weeks

   
Failed compliance check

   
IP cameras

   
NATS

NATS JetStream

  
Possible security concern

   
Review effort 1/5

   
Review effort 2/5

   
Review effort 3/5

   
Review effort 4/5

   
Review effort 5/5

   
UI

   
aardvark

   
accessibility

   
amd64

   
api

   
arm64

   
auth

   
back-end

   
bgp

   
blog

   
bug

Something isn't working

  
build

   
checkers

   
ci-cd

continuous integration-continuous deployments

  
cleanup

   
cnpg

cloud-native postgres

  
codex

   
core

core service

  
dependencies

Pull requests that update a dependency file

  
device-management

   
documentation

Improvements or additions to documentation

  
duplicate

This issue or pull request already exists

  
dusk

   
ebpf

   
enhancement

New feature or request

  
eta 1d

   
eta 1hr

   
eta 3d

   
eta 3hr

   
feature

   
fieldsurvey

   
github_actions

Pull requests that update GitHub Actions code

  
go

Pull requests that update Go code

  
good first issue

Good for newcomers

  
help wanted

Extra attention is needed

  
invalid

This doesn't seem right

  
javascript

Pull requests that update Javascript code

  
k8s

   
log-collector

   
mapper

   
mtr

multi traceroute

  
needs-triage

   
netflow

   
network-sweep

   
observability

   
oracle

Oracle Linux related issues

  
otel

opentelemetry logs, traces, metrics

  
plug-in

   
proton

timeplus proton streaming database

  
python

   
question

Further information is requested

  
reddit

   
redhat

   
research

   
rperf

   
rperf-checker

   
rust

Pull requests that update rust code

  
sdk

   
security

   
serviceradar-agent

   
serviceradar-agent-gateway

   
serviceradar-web

   
serviceradar-web-ng

   
siem

   
snmp

   
sysmon

   
topology

   
ubiquiti

   
wasm

   
wontfix

This will not be worked on

  
zen-engine
No labels
1week
2weeks
Failed compliance check
IP cameras
NATS
Possible security concern
Review effort 1/5
Review effort 2/5
Review effort 3/5
Review effort 4/5
Review effort 5/5
UI
aardvark
accessibility
amd64
api
arm64
auth
back-end
bgp
blog
bug
build
checkers
ci-cd
cleanup
cnpg
codex
core
dependencies
device-management
documentation
duplicate
dusk
ebpf
enhancement
eta 1d
eta 1hr
eta 3d
eta 3hr
feature
fieldsurvey
github_actions
go
good first issue
help wanted
invalid
javascript
k8s
log-collector
mapper
mtr
needs-triage
netflow
network-sweep
observability
oracle
otel
plug-in
proton
python
question
reddit
redhat
research
rperf
rperf-checker
rust
sdk
security
serviceradar-agent
serviceradar-agent-gateway
serviceradar-web
serviceradar-web-ng
siem
snmp
sysmon
topology
ubiquiti
wasm
wontfix
zen-engine
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
carverauto/serviceradar!2500
No description provided.