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
qlog: defer creation of the qlog file in QLOGDIR until the first write #4310
base: master
Are you sure you want to change the base?
Conversation
If the syscall to create the qlog file takes a non-negligible amount of time, this can block the connection's run loop, or, even worse, the server's run loop. By creating the qlog file on the first write, it will be created by a the qlogger's Go routine, and not be able to block the run loop. The qlogger uses a buffered channel (capacity: 50). The qlogger will therefore only block if more than 50 qlog events are queued before the syscall returns.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #4310 +/- ##
==========================================
- Coverage 84.13% 84.10% -0.03%
==========================================
Files 150 150
Lines 15368 15393 +25
==========================================
+ Hits 12929 12945 +16
- Misses 1940 1946 +6
- Partials 499 502 +3 ☔ View full report in Codecov by Sentry. |
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.
generally looks good to me
log.Printf("Failed to create qlog file %s: %s", path, err.Error()) | ||
return nil | ||
} | ||
return NewConnectionTracer(utils.NewBufferedWriteCloser(bufio.NewWriter(f), f), p, connID) |
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.
have you intentionally removed the buffered writer?
// won't block the connection, since qlogs are serialized from a separate Go routine, | ||
// that takes events from a buffered channel (size: eventChanSize). | ||
type lazyWriteCloser struct { | ||
createFile func() (*os.File, error) |
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.
createFile func() (*os.File, error) | |
createWriter func() (io.WriteCloser, error) |
// This means that creating the qlog (there's no guarantee how long the syscalls will take) | ||
// won't block the connection, since qlogs are serialized from a separate Go routine, | ||
// that takes events from a buffered channel (size: eventChanSize). | ||
type lazyWriteCloser struct { |
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.
should this util be exported just like utils.NewBufferedWriteCloser
If the syscall to create the qlog file takes a non-negligible amount of time, this can block the connection's run loop, or, even worse, the server's run loop.
By creating the qlog file on the first write, it will be created by a the qlogger's Go routine, and not be able to block the run loop. The qlogger uses a buffered channel (capacity: 50). The qlogger will therefore only block if more than 50 qlog events are queued before the syscall returns.
@birneee Would you mind taking a look at this PR?