Skip to content

fix: improve stream stats reset #8426

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.

Sign up for GitHub

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

Jump to bottom
Merged
hengfeiyang merged 3 commits into from
Sep 15, 2025
Merged

fix: improve stream stats reset #8426

hengfeiyang merged 3 commits into from
Sep 15, 2025

Conversation

@hengfeiyang
Copy link
Contributor

@hengfeiyang hengfeiyang commented Sep 14, 2025 •
edited by github-actions bot
Loading

User description

Add some tips:

Warning: !!!
calculate_stats_step_limit_secs good to be greater than 3600 for stats reset,
suggested to run again with:

ZO_CALCULATE_STATS_STEP_LIMIT_SECS=3600 ./openobserve reset -c stream-stats

PR Type

Bug fix, Enhancement

Description

  • Add warning for low stats step limit

  • Guide to rerun with recommended env var

  • Improve stream-stats reset safety messaging

Diagram Walkthrough

flowchart LR
  CLI["CLI reset command"]
  Check["Check calculate_stats_step_limit_secs"]
  Warn["Print warning and rerun tip"]
  ResetOffset["Reset stats update offset"]
  ResetData["Reset stream stats table"]

  CLI -- "reset -c stream-stats" --> Check
  Check -- "< 3600" --> Warn
  Check -- "any value" --> ResetOffset
  ResetOffset --> ResetData
Loading

File Walkthrough
Relevant files
Enhancement
cli.rs
Warn and guide when stats step limit too low                         

src/cli/basic/cli.rs

  • Add threshold check for calculate_stats_step_limit_secs.
  • Print multi-line warning with rerun recommendation.
  • Keep existing reset of offset and stats data.
 +13/-0   

@github-actions
Copy link
Contributor

github-actions bot commented Sep 14, 2025

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

UX/Wording

The warning message has minor grammatical issues and could be clearer; consider clearer phrasing and consistent punctuation to improve user guidance.

 
                    if cfg.limit.calculate_stats_step_limit_secs < 3600 {
                        println!(
                            r#"
------------------------------------------------------------------------------
Warning: !!! 
calculate_stats_step_limit_secs good to be at least 3600 for stats reset,
suggested to run again with:

ZO_CALCULATE_STATS_STEP_LIMIT_SECS=3600 ./openobserve reset -c stream-stats
------------------------------------------------------------------------------
"#
                        );
                    }
Hardcoded Threshold

The 3600-second threshold is hardcoded; consider referencing a constant or configuration source and echoing the current configured value to aid debugging.

 
                    if cfg.limit.calculate_stats_step_limit_secs < 3600 {
                        println!(
                            r#"
------------------------------------------------------------------------------
Warning: !!! 
calculate_stats_step_limit_secs good to be at least 3600 for stats reset,
suggested to run again with:

ZO_CALCULATE_STATS_STEP_LIMIT_SECS=3600 ./openobserve reset -c stream-stats
------------------------------------------------------------------------------
"#
                        );
                    }

@github-actions
Copy link
Contributor

github-actions bot commented Sep 14, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Abort reset on bad config

Exit early after showing the warning to prevent resetting offsets/tables under an
unsuitable configuration. Return Ok(true) (or appropriate status) before executing
the destructive reset calls.

src/cli/basic/cli.rs [226-238]

 if cfg.limit.calculate_stats_step_limit_secs < 3600 {
-    println!(
+    eprintln!(
         r#"
 ------------------------------------------------------------------------------
 Warning: !!! 
 calculate_stats_step_limit_secs good to be at least 3600 for stats reset,
 suggested to run again with:
 
 ZO_CALCULATE_STATS_STEP_LIMIT_SECS=3600 ./openobserve reset -c stream-stats
 ------------------------------------------------------------------------------
 "#
     );
+    return Ok(true);
 }
Suggestion importance[1-10]: 7

__

Why: Early-returning after the warning prevents potentially destructive reset operations under an unsuitable config; it aligns with the new hunk context and is logically sound, though behavior-changing and may depend on CLI expectations.

Medium
General
Send warnings to stderr

Use stderr for warnings so they don't mix with normal output and can be
programmatically detected. Replace println! with eprintln! to correctly classify
this as a warning message.

src/cli/basic/cli.rs [226-238]

 if cfg.limit.calculate_stats_step_limit_secs < 3600 {
-    println!(
+    eprintln!(
         r#"
 ------------------------------------------------------------------------------
 Warning: !!! 
 calculate_stats_step_limit_secs good to be at least 3600 for stats reset,
 suggested to run again with:
 
 ZO_CALCULATE_STATS_STEP_LIMIT_SECS=3600 ./openobserve reset -c stream-stats
 ------------------------------------------------------------------------------
 "#
     );
 }
Suggestion importance[1-10]: 6

__

Why: Redirecting the multi-line warning from println! to eprintln! correctly classifies it as a warning and avoids mixing with normal output; the existing_code matches the new hunk and the change is accurate but moderate in impact.

Low

greptile-apps[bot]
greptile-apps bot reviewed Sep 14, 2025
View reviewed changes
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Greptile Summary

This PR adds a configuration validation warning to the stream-stats reset command in the CLI module. When users run the stream statistics reset operation with calculate_stats_step_limit_secs set below 3600 seconds, the system now displays a prominent warning message suggesting they rerun the command with the recommended value of 3600 seconds.

The change is implemented in src/cli/basic/cli.rs within the existing reset command handler. The modification adds a simple configuration check that compares cfg.limit.calculate_stats_step_limit_secs against the 3600-second threshold and prints a formatted warning message when the value is too low. This warning appears before the actual reset operations (db::compact::stats::set_offset() and infra_file_list::reset_stream_stats()) execute, providing users with guidance while still allowing the operation to proceed.

This enhancement integrates well with the existing stream statistics infrastructure shown in the codebase context, particularly the stats.rs cache management system and the reset_stream_stats database function. The warning helps ensure users configure the system optimally for stream statistics calculations, which can be resource-intensive operations that benefit from appropriate time limits.

Confidence score: 5/5

  • This PR is extremely safe to merge with virtually no risk of causing issues
  • Score reflects the purely additive nature of the change - it only adds an informational warning without modifying core functionality
  • No files require special attention as the change is a simple configuration check with console output

1 file reviewed, no comments

Edit Code Review Bot Settings | Greptile

@hengfeiyang hengfeiyang merged commit 53d23c4 into main Sep 15, 2025
28 checks passed
@hengfeiyang hengfeiyang deleted the feat/stream-stats branch September 15, 2025 05:18
Subhra264 pushed a commit that referenced this pull request Sep 17, 2025
@hengfeiyang @Subhra264
fix: improve stream stats reset (#8426)
c633c59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Subhra264 Subhra264 Subhra264 approved these changes

@YashodhanJoshi1 YashodhanJoshi1 Awaiting requested review from YashodhanJoshi1

+1 more reviewer

@greptile-apps greptile-apps[bot] greptile-apps[bot] left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

☢️ Bug Something isn't working Review effort 2/5 🧹 Updates

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@hengfeiyang @Subhra264