-
Notifications
You must be signed in to change notification settings - Fork 4
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
Ensure logs are reported after 10s max #65
base: main
Are you sure you want to change the base?
Ensure logs are reported after 10s max #65
Conversation
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 actually means any infrequently reporting client will send logs immediately, which may not be great for the network (the whole point is to capture bursts of requests as one group)
My recommendation is:
reportLogs (log) {
this.logs.push(log)
if (!this.reportingLogs) return
this.reportLogsTimeout && clearTimeout(this.reportLogsTimeout)
if (!this.logsTimeoutStartedAt) {
this.logsTimeoutStartedAt = new Date()
this.reportLogsTimeout = setTimeout(this._reportLogs.bind(this), 3_000)
} else if (this.logsTimeoutStartedAt.getTime() < new Date().getTime() - 10_000) {
this._reportLogs()
} else {
this.reportLogsTimeout = setTimeout(this._reportLogs.bind(this), 3_000)
}
}
async _reportLogs () {
if (!this.logs.length) {
return
}
try {
const bandwidthLogs = this.hasPerformanceAPI
? this._matchLogsWithPerformanceMetrics(this.logs)
: this.logs
await fetch(
this.config.logURL,
{
method: 'POST',
body: JSON.stringify({ bandwidthLogs, logSender: this.config.logSender })
}
)
this.logsTimeoutStartedAt = null
this.onReportLogs(bandwidthLogs)
} catch (e) {
console.log(e)
throw e
} finally {
this.logs = []
this._clearPerformanceBuffer()
}
}
@@ -469,8 +470,15 @@ export class Saturn { | |||
reportLogs (log) { | |||
this.logs.push(log) | |||
if (!this.reportingLogs) return | |||
this.reportLogsTimeout && clearTimeout(this.reportLogsTimeout) | |||
this.reportLogsTimeout = setTimeout(this._reportLogs.bind(this), 3_000) | |||
clearTimeout(this.reportLogsTimeout) |
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.
clearTimeout(this.reportLogsTimeout) | |
this.reportLogsTimeout && clearTimeout(this.reportLogsTimeout) |
this.reportLogsTimeout may be null
Isn't this already the case? After inactivity of 3s, logs will be submitted. |
The goal is to debounce since most clients will requests in bursts. So if I make one saturn request, in the same second I'm likely to make 100 saturn request (see all assets on a web page). The 10 second makes as a maximum debounce time, but we don't want to submit just one log at a time. |
The current implementation reports logs after not seeing any new ones for 3s. The Voyager clients creates frequent requests, which can lead to no logs being reported for a whole while. This PR makes it so that logs will be reported after 10s max.