Skip to content
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

Closed
wants to merge 8 commits into from

Conversation

WarpWing
Copy link

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.

@WarpWing
Copy link
Author

(I'll look into fixing format errors)

@lenguyenthanh
Copy link
Member

(I'll look into fixing format errors)

for formatting, you can just use sbt prepare

@WarpWing
Copy link
Author

WarpWing commented Apr 20, 2024

I'm getting some weird errors on my end for compiling. I think it has to do with my Java version, I have Java 8, maybe I should switch to a higher JDK since it uses openjdk21.
image
image

Running sbt compile is where I run into errors. I'll work around it.

@WarpWing
Copy link
Author

WarpWing commented Apr 20, 2024

@lenguyenthanh Apologies for the wait, I just needed to update my Java.
image

@lenguyenthanh
Copy link
Member

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


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 =>
Copy link
Collaborator

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


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 =>
Copy link
Collaborator

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
Copy link
Collaborator

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?

@WarpWing WarpWing closed this Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants