Skip to content

Add log which will report dropped log count #9752

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
Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add log which will report dropped log count #9752

wants to merge 2 commits into from
+11 −1

Conversation

@SamarthBagga
Copy link
Contributor

@SamarthBagga SamarthBagga commented Sep 23, 2025 •
edited
Loading

Describe Your Changes

I have added a counter for the throttled logs which gets logged every 1 minute.
Fixes #9498

Checklist

The following checks are mandatory:

@hagen1778 hagen1778 requested a review from makasim September 25, 2025 08:40
@SamarthBagga
Copy link
Contributor Author

SamarthBagga commented Oct 17, 2025

Hi @makasim,
Is this approach correct?

@makasim
Copy link
Contributor

makasim commented Oct 30, 2025

@SamarthBagga Can you please update PR with the solution proposed by @Haleygo in the issue? The suffix one.

@makasim makasim requested a review from Haleygo October 30, 2025 12:10
@makasim
Copy link
Contributor

makasim commented Oct 30, 2025

Please add a changelog.

@SamarthBagga
Copy link
Contributor Author

SamarthBagga commented Nov 3, 2025

@makasim @Haleygo I have updated the PR on the basis of the discussion in the issue. Could you please review this and tell me if my way is correct?

Haleygo
Haleygo reviewed Nov 4, 2025
View reviewed changes
@SamarthBagga
Copy link
Contributor Author

SamarthBagga commented Nov 4, 2025

@Haleygo I have made the suggested changes, could you have a look at it again?

makasim
makasim reviewed Nov 4, 2025
View reviewed changes
lib/logger/throttler.go
if dropped > 0 {
format = fmt.Sprintf("%s. Similar %d log messages were throttled", format, dropped)
}
dropped := lt.dropped.Swap(0)
Copy link
Contributor

@makasim makasim Nov 4, 2025

Choose a reason for hiding this comment

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

The Similar xxx message should be added only if dropped > 0.

Copy link
Contributor Author

@SamarthBagga SamarthBagga Nov 4, 2025

Choose a reason for hiding this comment

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

@Haleygo mentioned in this comment that it is better to print

Similar 0 log messages were dropped due to throttling

as it helps with log filtering. I agree with that reasoning. What do you think?

Haleygo
Haleygo approved these changes Nov 4, 2025
View reviewed changes
Copy link
Contributor

@Haleygo Haleygo left a comment

Choose a reason for hiding this comment

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

LGTM

@codecov
Copy link

codecov bot commented Nov 4, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 0% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 55.79%. Comparing base (8aaa828) to head (b6b4d42).
⚠️ Report is 3879 commits behind head on master.

Files with missing lines Patch % Lines
lib/logger/throttler.go 0.00% 6 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #9752      +/-   ##
==========================================
- Coverage   60.37%   55.79%   -4.58%     
==========================================
  Files         411      529     +118     
  Lines       76609    67364    -9245     
==========================================
- Hits        46253    37589    -8664     
+ Misses      27794    27396     -398     
+ Partials     2562     2379     -183

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@SamarthBagga
Copy link
Contributor Author

SamarthBagga commented Nov 4, 2025

@Haleygo A lint check was failing so I updated the PR with the fix for that. Do I need to sign all the commits I have already made? Could you approve the PR again to run the workflows?

@Haleygo
Copy link
Contributor

Haleygo commented Nov 4, 2025

@Haleygo A lint check was failing so I updated the PR with the fix for that. Do I need to sign all the commits I have already made? Could you approve the PR again to run the workflows?

Yup, all the commits must be signed, see https://docs.victoriametrics.com/victoriametrics/contributing/#pull-request-checklist.

  1. All commits need to be signed .

@SamarthBagga
Copy link
Contributor Author

SamarthBagga commented Nov 4, 2025

@Haleygo Could you suggest a way to sign commits that were already made? I tried a few things, but they were causing some strange issues?

@Haleygo
Copy link
Contributor

Haleygo commented Nov 4, 2025

@Haleygo Could you suggest a way to sign commits that were already made? I tried a few things, but they were causing some strange issues?

I think the simplest approach now is to drop all previous commits and have all changes in a single new commit, since your latest commit is signed, indicating the configuration is correct now.

@SamarthBagga SamarthBagga reopened this Nov 4, 2025
@SamarthBagga @Haleygo
Apply suggestion from @Haleygo
b6b4d42 
Co-authored-by: Hui Wang <haley@victoriametrics.com>
@SamarthBagga
Copy link
Contributor Author

SamarthBagga commented Nov 4, 2025

@Haleygo I have signed the commits, could you have another look?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@makasim makasim makasim left review comments

@Haleygo Haleygo Haleygo approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Throttle logger should periodically report dropped log count

3 participants

@SamarthBagga @makasim @Haleygo