Skip to content

Dirty fix for flaky send to segment #382

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

Draft
MilanPospisil wants to merge 8 commits into
base: devel
Choose a base branch
from
+3 −2
Draft

Dirty fix for flaky send to segment #382

Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
e9fbe2c
Dirty fix for flaky send to segment
MilanPospisil Apr 28, 2026
af52c62
Ruff
MilanPospisil Apr 28, 2026
550ba99
Maybe the fix?
MilanPospisil Apr 28, 2026
69e466c
WIP
MilanPospisil Apr 28, 2026
740b074
WIP
MilanPospisil Apr 28, 2026
9adf348
WIP
MilanPospisil Apr 28, 2026
f8d225f
Only working kind of fix :(
MilanPospisil Apr 28, 2026
b3e4322
Ruff
MilanPospisil Apr 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions metrics_utility/library/storage/segment.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
import hashlib
import json
import sys
import time
import uuid

from metrics_utility.logger import logger
Expand Down Expand Up @@ -174,6 +175,9 @@ def put(self, artifact_name, *, filename=None, fileobj=None, dict=None, event_na
**segment_meta,
)

# sleep for 3 seconds
time.sleep(3)
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Apr 28, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Avoid sleeping after the final chunk.

time.sleep(3) runs on every iteration, so single-chunk uploads still pay a 3-second delay and multi-chunk uploads keep the upstream lock held longer in workers/0-gather-controller-crc.py:24-29 and workers/1-gather-controller-full.py:41-47. Make the pause conditional on i < total_chunks so it only applies between track calls.

Suggested fix 
-            time.sleep(3)
+            if i < total_chunks:
+                time.sleep(3)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
# sleep for 3 seconds
time.sleep(3)
# sleep for 3 seconds
if i < total_chunks:
time.sleep(3)
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@metrics_utility/library/storage/segment.py` around lines 178 - 179, The loop
always calls time.sleep(3), causing an unnecessary delay after the final chunk;
update the loop in segment.py so the sleep is conditional (only when i <
total_chunks) — after calling track(...) check the loop index/remaining chunks
and call time.sleep(3) only between chunks, leaving the final iteration free of
the sleep to avoid holding upstream locks in workers/0-gather-controller-crc.py
and workers/1-gather-controller-full.py; specifically modify the location where
time.sleep(3) is used alongside variables i and total_chunks and the track(...)
call.

Copy link
Copy Markdown
Contributor Author

@MilanPospisil MilanPospisil Apr 28, 2026

Choose a reason for hiding this comment

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

We do not have better solution now, without sleep, the segment is not receiving all messages. With this fix, it is receiving most messages, its much better.

Release is at the door and we need solution quickly.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Apr 28, 2026

Choose a reason for hiding this comment

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

@MilanPospisil, understood — the pacing is needed and I'm not suggesting removing it. The original suggestion just avoids sleeping after the last chunk, since at that point all track() calls are already queued and the next operation is analytics.flush(). The pacing between chunks is fully preserved.

-            # sleep for 3 seconds
-            time.sleep(3)
+            # sleep for 3 seconds between chunks (not after the last one)
+            if i < total_chunks:
+                time.sleep(3)

This is a one-line change that saves 3 seconds on every upload (including single-chunk ones) without touching the core workaround. Given release pressure, that makes it the lowest-risk improvement available.


# Flush to ensure all events are sent
analytics.flush()

Expand Down