Dirty fix for flaky send to segment#382
Conversation
📝 WalkthroughWalkthroughModifies chunk dispatch in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
MilanPospisil
force-pushed
the
milan_fix_flaky_send_to_segment
branch
from
744e737 to
af52c62
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@metrics_utility/library/storage/segment.py`:
- Around line 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.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: a22ea98d-527e-4790-a225-8a455fee52d3
📒 Files selected for processing (1)
metrics_utility/library/storage/segment.py
| # sleep for 3 seconds | ||
| time.sleep(3) |
There was a problem hiding this comment.
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.
| # 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
|
2 participants
[AAP-73135]
Fixes flaky sending to segment.com, before, segment did not received all the messages. Now it receives most messages (but still not completely guaranteed).
This is a temporary workaround for unreliable delivery of chunked Segment events.
Without pacing, we observe missing chunks in roughly 10–20% of uploads.
Adding a delay between chunk sends significantly reduces missing chunks before release.