-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix(core): Fix logs and metrics flush timeout starvation with continuous logging #18211
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
Conversation
The flush timeout was being reset on every incoming log, preventing flushes when logs arrived continuously. Now, the timer starts on the first log won't get reset, ensuring logs flush within the configured interval. Fixes #18204, getsentry/sentry-react-native#5378
size-limit report 📦
|
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
| flushTimeout = setTimeout(() => { | ||
| flushFn(client); | ||
| // Note: isTimerActive is reset by the flushHook handler above, not here, | ||
| // to avoid race conditions when new items arrive during the flush. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Stuck Timer Halts Automatic Flushing
The isTimerActive flag can get stuck as true when the timer fires but the buffer is empty. This happens because isTimerActive is only reset in the flushHook handler (line 121), but _INTERNAL_flushLogsBuffer returns early without emitting this hook when the buffer is empty. Once stuck, no new timers can start since !isTimerActive evaluates to false, preventing automatic flushing of subsequent logs/metrics until the weight threshold is exceeded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For isTimerActive to be set to true, an item must have come in via afterCaptureHook which in turn starts the timeout. Once the buffer flushes isTimerActive will be set back to false via flushHook.
I don't think this scenario can happen.
The flush timeout was being reset on every incoming log, preventing flushes when logs arrived continuously. Now, the timer starts on the first log won't get reset, ensuring logs flush within the configured interval. Backport of: #18211
isaacs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, this does do what it says :)
chargome
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing!
The flush timeout was being reset on every incoming log, preventing flushes when logs arrived continuously. Now, the timer starts on the first log won't get reset, ensuring logs flush within the configured interval. Backport of: #18211
The flush timeout was being reset on every incoming log, preventing flushes when logs arrived continuously. Now, the timer starts on the first log and won't get reset, ensuring logs flush within the configured interval.
Fixes #18204, getsentry/sentry-react-native#5378
v9 backport: #18214