-
Notifications
You must be signed in to change notification settings - Fork 715
fix: check for interval > 0 #8838
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: loaki07 <[email protected]>
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Overview
Summary
Adds defensive checks to prevent division by zero when histogram_interval is 0 in the cache result writing logic.
The PR adds && interval > 0 checks at two locations in the write_results function:
- Line 811: Before performing modulo operations to align cache start/end times with histogram intervals
- Line 865: Before adjusting the end time based on the interval
While validate_and_adjust_histogram_interval in histogram_interval.rs should prevent 0 values by returning a default (line 151-156), this defensive check ensures safety if histogram_interval arrives from other code paths or edge cases.
Confidence Score: 5/5
- This PR is safe to merge with minimal risk
- The change is a defensive fix that prevents potential division by zero errors without altering any business logic. The checks are conservative and only skip processing when the interval would cause arithmetic errors.
- No files require special attention
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| src/service/search/cache/mod.rs | 5/5 | Added defensive checks to prevent division by zero when histogram_interval is 0, protecting arithmetic operations in cache time range alignment |
Sequence Diagram
sequenceDiagram
participant Caller
participant write_results
participant Cache
Caller->>write_results: Call with histogram_interval
alt histogram_interval is Some(value)
write_results->>write_results: Check interval > 0 (NEW)
alt interval > 0
write_results->>write_results: Convert to microseconds
write_results->>write_results: Align accept_start_time
write_results->>write_results: Align accept_end_time
write_results->>write_results: Set need_adjust_end_time flag
else interval <= 0
write_results->>write_results: Skip alignment (prevented division by zero)
end
else histogram_interval is None
write_results->>write_results: Skip alignment
end
write_results->>write_results: Remove records outside time range
alt need_adjust_end_time is true
write_results->>write_results: Check interval > 0 again (NEW)
alt interval > 0
write_results->>write_results: Adjust accept_end_time
else interval <= 0
write_results->>write_results: Skip adjustment
end
end
write_results->>Cache: Cache filtered results to disk
1 file reviewed, no comments
|
| Status | Total | Passed | Failed | Skipped | Flaky | Pass Rate | Duration |
|---|---|---|---|---|---|---|---|
| All tests passed | 364 | 342 | 0 | 19 | 3 | 94% | 4m 38s |
|
| Status | Total | Passed | Failed | Skipped | Flaky | Pass Rate | Duration |
|---|---|---|---|---|---|---|---|
| All tests passed | 364 | 342 | 0 | 19 | 3 | 94% | 4m 41s |
|
| Status | Total | Passed | Failed | Skipped | Flaky | Pass Rate | Duration |
|---|---|---|---|---|---|---|---|
| All tests passed | 364 | 343 | 0 | 19 | 2 | 94% | 4m 39s |
### **PR Type**
Bug fix
___
### **Description**
- Guard histogram interval must be positive
- Prevent misaligned cache time adjustments
- Skip alignment when interval invalid
___
### Diagram Walkthrough
```mermaid
flowchart LR
A["write_results cache alignment"] -- "is_aggregate && interval present" --> B["validate interval > 0"]
B -- "true" --> C["align start/end to interval"]
B -- "false" --> D["skip alignment/adjustment"]
```
<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><table>
<tr>
<td>
<details>
<summary><strong>mod.rs</strong><dd><code>Add positive-interval guards
to histogram alignment</code>
</dd></summary>
<hr>
src/service/search/cache/mod.rs
<ul><li>Add <code>interval > 0</code> check before histogram
alignment.<br> <li> Guard end-time cache adjustment with positive
interval.<br> <li> Prevent microsecond conversion for zero/invalid
intervals.</ul>
</details>
</td>
<td><a
href="https://github.com/openobserve/openobserve/pull/8838/files#diff-f07bfacc0501b9c234e64b16c1dd8eb0ae8fcbe231f90c81fed72bcc94946f74">+5/-1</a>
</td>
</tr>
</table></td></tr></tr></tbody></table>
</details>
___
---------
Signed-off-by: loaki07 <[email protected]>
PR Type
Bug fix
Description
Guard histogram interval must be positive
Prevent misaligned cache time adjustments
Skip alignment when interval invalid
Diagram Walkthrough
File Walkthrough
mod.rs
Add positive-interval guards to histogram alignmentsrc/service/search/cache/mod.rs
interval > 0check before histogram alignment.