SDSTOR-21921 Improve LOG_EVERY_N logic with memory optimization and t… by xiaoxichen · Pull Request #882 · eBay/HomeStore

xiaoxichen · 2026-05-07T16:30:35Z

…ime-based rate limiting

This change enhances LOG_EVERY_N macros to fix memory leaks and add new capabilities:

Refactor check_logged_already() to use hash-based keys instead of full message strings
Clear entire map every 300s to prevent unbounded memory growth
Store millisecond offsets and counts for time-based rate limiting
Log first occurrence (count==1) in addition to every Nth occurrence
Add HS_LOG_EVERY_N_SEC for time-based rate limiting (log every N seconds)
Add HS_LOG_EVERY_N_OR_SEC for hybrid rate limiting (every N occurrences OR every M seconds)

Memory impact: Reduces per-entry storage from ~124 bytes to ~20 bytes (6x improvement)

Resolves: #880

Copilot

Pull request overview

This PR enhances HomeStore’s rate-limited logging macros (HS_LOG_EVERY_N*) to reduce per-thread memory growth and add time-based (seconds) and hybrid (count-or-time) rate limiting, addressing Issue #880.

Changes:

Refactors rate-limit tracking to use hashed keys with periodic per-thread cleanup to bound memory usage.
Adds HS_LOG_EVERY_N_SEC and HS_LOG_EVERY_N_OR_SEC macros for time-based and hybrid rate limiting.
Adjusts HS_LOG_EVERY_N behavior to log the first occurrence and then rate-limit subsequent logs.

Reviewed changes

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

File	Description
src/lib/common/homestore_assert.hpp	Adds time-based/hybrid LOG_EVERY macros and refactors the per-thread tracking logic for memory-bounded rate limiting.
conanfile.py	Bumps package version to 7.5.7.
.gitignore	Ignores `.worktrees/` directory.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…ime-based rate limiting This change enhances LOG_EVERY_N macros to fix memory leaks and add new capabilities: - Refactor check_logged_already() to check_and_format_log() which consolidates rate-limiting decision and suffix formatting into single function - Use hash-based keys instead of full message strings for memory efficiency - Clear entire map every 300s to prevent unbounded memory growth - Store millisecond offsets and counts for time-based rate limiting - Log first occurrence (count==1) in addition to every Nth occurrence - Add HS_LOG_EVERY_N_SEC for time-based rate limiting (log every N seconds) - Add HS_LOG_EVERY_N_OR_SEC for hybrid rate limiting (every N occurrences OR every M seconds) - Use consistent suffix format: "Last logged X.Xs ago, N occurrences" Memory impact: Reduces per-entry storage from ~124 bytes to ~20 bytes (6x improvement) Resolves: eBay#880

Copilot

Pull request overview

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

Use milliseconds directly in format string instead of converting to seconds, avoiding rvalue binding issue with fmt::make_format_args. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

- Fix count tracking: reset to 0 instead of 1 to avoid off-by-one in rate limiting (was logging at 10, 19, 28... instead of 10, 20, 30...) - Use happened flag to detect first occurrence instead of count==1 - Use swap with empty map to actually release memory during cleanup (clear() only removes entries but retains bucket array capacity) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Copilot

Pull request overview

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

codecov-commenter · 2026-05-07T18:33:17Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 45.94595% with 20 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (stable/v7.x@2b0478f). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
src/lib/common/homestore_assert.hpp	45.94%	11 Missing and 9 partials ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@              Coverage Diff               @@
##             stable/v7.x     #882   +/-   ##
==============================================
  Coverage               ?   48.22%           
==============================================
  Files                  ?      110           
  Lines                  ?    12935           
  Branches               ?     6220           
==============================================
  Hits                   ?     6238           
  Misses                 ?     2577           
  Partials               ?     4120

☔ 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.

- Add #include <functional> for std::hash to avoid relying on transitive includes - Handle edge case when both freq=0 and interval_sec=0: always log (fallback behavior) instead of only logging first occurrence then suppressing all subsequent logs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 2 out of 3 changed files in this pull request and generated no new comments.

xiaoxichen · 2026-05-08T03:30:23Z

@Besroy I think it is ready for human :) after passing AI. The Copilot review did a good job through that most of the comments make sense

Besroy · 2026-05-08T03:46:11Z

Generally LGTM. Just have a small concern: it works as expected only when interval_sec < 300 or freq * avg_time_per_occur < 300s. Otherwise, the behavior effectively becomes "restart counting every 5 minutes, and log the first occurrence after each reset." Is that acceptable by design?
If so, it may be worth adding a brief note in check_and_format_log to make this behavior explicit.

xiaoxichen · 2026-05-08T04:43:03Z

agree, aside from doc we can have a warning log for interval_sec>300, however the freq * avg_time_per_occur > 300s cases it seems like a right behavior.

- Add function comment documenting the 300s cleanup limitation - Log one-time warning when interval_sec exceeds cleanup period - Clarifies expected behavior when using large interval_sec values Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Besroy

LGTM

JacksonYao287 · 2026-05-09T02:06:32Z

@xiaoxichen pls use squash merge when merging PR , we`d better have one final commit for a single PR

xiaoxichen · 2026-05-09T02:36:07Z

oops，sorry my bad，forgot to squash

xiaoxichen requested review from Besroy and Copilot May 7, 2026 16:30

Copilot started reviewing on behalf of xiaoxichen May 7, 2026 16:31 View session

Copilot AI reviewed May 7, 2026

View reviewed changes

xiaoxichen force-pushed the log-every-n-improvements branch from 53a4ca3 to a7cf764 Compare May 7, 2026 17:29

xiaoxichen requested a review from Copilot May 7, 2026 17:31

Copilot started reviewing on behalf of xiaoxichen May 7, 2026 17:32 View session

Copilot AI reviewed May 7, 2026

View reviewed changes

Comment thread src/lib/common/homestore_assert.hpp Outdated

Comment thread src/lib/common/homestore_assert.hpp

Comment thread src/lib/common/homestore_assert.hpp Outdated

xiaoxichen and others added 2 commits May 8, 2026 01:38

Fix fmt::make_format_args compilation error

eeb7a9c

Use milliseconds directly in format string instead of converting to seconds, avoiding rvalue binding issue with fmt::make_format_args. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

xiaoxichen requested a review from Copilot May 7, 2026 18:19

Copilot started reviewing on behalf of xiaoxichen May 7, 2026 18:20 View session

Copilot AI reviewed May 7, 2026

View reviewed changes

Comment thread src/lib/common/homestore_assert.hpp

Comment thread src/lib/common/homestore_assert.hpp

xiaoxichen requested a review from Copilot May 8, 2026 03:02

Copilot started reviewing on behalf of xiaoxichen May 8, 2026 03:03 View session

Copilot AI reviewed May 8, 2026

View reviewed changes

Besroy approved these changes May 8, 2026

View reviewed changes

xiaoxichen merged commit 568f9ff into eBay:stable/v7.x May 8, 2026
21 checks passed

Conversation

xiaoxichen commented May 7, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented May 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

xiaoxichen commented May 8, 2026

Uh oh!

Besroy commented May 8, 2026

Uh oh!

xiaoxichen commented May 8, 2026

Uh oh!

Besroy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JacksonYao287 commented May 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xiaoxichen commented May 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-commenter commented May 7, 2026 •

edited

Loading

JacksonYao287 commented May 9, 2026 •

edited

Loading