-
-
Notifications
You must be signed in to change notification settings - Fork 90
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
Improve frameLag calculation to handle spikes and faster recovery #559
Conversation
(I'll look into fixing format errors) |
for formatting, you can just use |
@lenguyenthanh Apologies for the wait, I just needed to update my Java. |
No need to apology and thanks for your contribution! So nice that it passes the CI. I'll let other to check if it worth merging. |
src/main/scala/Lag.scala
Outdated
|
||
export trustedStats.getIfPresent as sessionLag | ||
|
||
private val clientReports = groupedWithin[(User.Id, Int)](256, 947.millis): lags => | ||
private val clientReports = groupedWithin[(User.Id, Int)](256, 947.millis) { lags => |
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.
please don't revert this syntax
src/main/scala/Lag.scala
Outdated
|
||
export clientReports.apply as recordClientLag | ||
|
||
def recordTrustedLag(millis: Int, userId: Option[User.Id], domain: Domain) = | ||
Monitor.lag.roundFrameLag(millis, domain) | ||
userId.foreach: uid => | ||
userId.foreach { uid => |
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.
off-topic syntax change
(prev * (1 - trustedRefreshFactor) + millis * trustedRefreshFactor).toInt | ||
sessionLag(uid).fold(millis) { prev => | ||
val weight = | ||
if millis < prev then trustedNormalRefreshFactor else trustedSpikeRefreshFactor |
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.
This isn't about spikes, it's just about the current lag being any worse than the previous one.
So we would always be twice as slow recording higher lag than lower lag.
I expect that as a result our lag estimation would consistently be under-evaluated?
Implemented ideas as mentioned in #395 where massive spikes can cause the weighted average to skyrocket and take a long time to recover. The changes introduce separate refresh factors for spike and normal values, cap the maximum recorded value, and adjust the calculation to allow faster recovery when recording lower values.