Skip to content

feat: Add retention size limit for rotated files #1040

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

Merged
merged 2 commits into from
Jul 21, 2025

Conversation

Kubuxu
Copy link
Contributor

@Kubuxu Kubuxu commented Jul 17, 2025

This adds a new flag to the observer command to limit the size of the
rotated files. When the rotated files exceed this size, the oldest files
are deleted.

This is in support of #1041

This adds a new flag to the observer command to limit the size of the
rotated files. When the rotated files exceed this size, the oldest files
are deleted.

Signed-off-by: Jakub Sztandera <[email protected]>
@Copilot Copilot AI review requested due to automatic review settings July 17, 2025 16:11
@github-project-automation github-project-automation bot moved this to Todo in F3 Jul 17, 2025
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds a new retention size limit feature for rotated files in the observer command. When rotated files exceed the specified size limit, the oldest files are automatically deleted to maintain storage bounds.

Key changes:

  • Added maxRetentionSize configuration option with CLI flag support
  • Implemented size-based file cleanup logic that removes oldest files when size limit is exceeded
  • Updated file rotation logic to track retained files and their sizes

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File Description
observer/options.go Adds maxRetentionSize field and WithMaxRetentionSize option function
observer/observer.go Implements size-based cleanup logic in rotateMessages method
cmd/f3/observer.go Adds CLI flag for retention size limit with megabyte conversion

Copy link

codecov bot commented Jul 17, 2025

Codecov Report

Attention: Patch coverage is 0% with 35 lines in your changes missing coverage. Please review.

Project coverage is 64.94%. Comparing base (e7f84e7) to head (7bfdbd4).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
observer/observer.go 0.00% 27 Missing ⚠️
observer/options.go 0.00% 8 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1040      +/-   ##
==========================================
- Coverage   65.34%   64.94%   -0.41%     
==========================================
  Files          80       80              
  Lines        9702     9733      +31     
==========================================
- Hits         6340     6321      -19     
- Misses       2867     2910      +43     
- Partials      495      502       +7     
Files with missing lines Coverage Δ
observer/options.go 5.80% <0.00%> (-0.20%) ⬇️
observer/observer.go 0.00% <0.00%> (ø)

... and 4 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@BigLep BigLep moved this from Todo to In review in F3 Jul 17, 2025
Copy link
Member

@BigLep BigLep left a comment

Choose a reason for hiding this comment

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

High level question: do we feel good about the decision to be writing code to managing retention of files on disk rather than using other standard external tooling for this? (I'm no expert here but I assume this problem isn't unique to us and that there are general solutions that operators apply to prevent too much state from accumulating).

@BigLep
Copy link
Member

BigLep commented Jul 17, 2025

High level question: do we feel good about the decision to be writing code to managing retention of files on disk rather than using other standard external tooling for this? (I'm no expert here but I assume this problem isn't unique to us and that there are general solutions that operators apply to prevent too much state from accumulating).

Thinking on this more, I can see why going with doing this within f3-observer itself makes sense given we already have FS-related logic around retention and rotation. Maybe if we we were doing more infra work it would make sense to invest in using "Kubernetes Native Solutions" or "Existing Sidecar Images" (e.g., logrotate), but I agree it's not worth over complicating now. And I definitely think easier to maintain to keep this logic in Go than writing our own sidecar that is doing rotation/pruning with shell scripting.

Signed-off-by: Jakub Sztandera <[email protected]>
@Kubuxu Kubuxu enabled auto-merge (squash) July 18, 2025 14:57
@Kubuxu Kubuxu merged commit 993d8f7 into main Jul 21, 2025
20 of 23 checks passed
@Kubuxu Kubuxu deleted the feat/observer-size-retention branch July 21, 2025 12:58
@github-project-automation github-project-automation bot moved this from In review to Done in F3 Jul 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants