[draft]qlog: Implement MoQQLogFactory to enhance logging for QUIC connections#242
[draft]qlog: Implement MoQQLogFactory to enhance logging for QUIC connections#242akash-a-n wants to merge 4 commits into
Conversation
- Add setWriteExecutor() for asynchronous off-thread disk I/O - Pre-format logs on calling thread, move serialized data to executor - Add setDir() for directory prefix support via derivePath() - Avoid this capture by moving all data into lambda - Update FileMLoggerFactory to support new features - Add test coverage for FileMLogger and FileMLoggerFactory
- New wrapper factory that applies per-session sampling - Uses folly::Random::oneIn() for thread-safe sampling - Enables selective logging (e.g., 1%% of sessions) in production - Add test coverage for SamplingMLoggerFactory
- Log DCID for incoming QUIC connections in onNewConnectionImpl() - Log DCID for WebTransport CONNECT in onWebTransportConnectImpl() - Enable MLogger creation and attachment to MoQ sessions
akash-a-n
requested review from
Oxyd,
TimEvens,
afrind,
gmarzot,
michalhosna,
mondain,
peterchave and
suhasHere
as code owners
akash-a-n
left a comment
akash-a-n
left a comment
There was a problem hiding this comment.
@afrind
I wanted to get you thoughts on this integration pattern that wraps the TransportFactory.
Since we use HQServer for WebTransport connections, I am unable to set an asynchronous logger for qlogs because proxygen's HQServerTransportFactory's make function hardcodes streaming to false.
Ideally having a factory in proxygen makes more sense as its the owner, but I don't know if they can be persuaded in time for the change.
Let me know if this method is fine or if you have any other ideas?
@akash-a-n made 1 comment.
Reviewable status: 0 of 2 files reviewed, all discussions resolved (waiting on afrind, gmarzot, michalhosna, mondain, Oxyd, peterchave, suhasHere, and TimEvens).
afrind
left a comment
afrind
left a comment
There was a problem hiding this comment.
If you have a targeted change to proxygen, we can get it landed upstream pretty quickly. Seems like that would be a little cleaner than this wrapped pattern?
@afrind reviewed all commit messages and made 1 comment.
Reviewable status: 0 of 2 files reviewed, all discussions resolved (waiting on gmarzot, michalhosna, mondain, Oxyd, peterchave, suhasHere, and TimEvens).
akash-a-n
left a comment
akash-a-n
left a comment
There was a problem hiding this comment.
create this, but I need your help with CLA:
facebook/proxygen#622
@akash-a-n made 1 comment.
Reviewable status: 0 of 2 files reviewed, all discussions resolved (waiting on gmarzot, michalhosna, mondain, Oxyd, peterchave, suhasHere, and TimEvens).
Summary: This change provides a way for downstream apps like moxygen and moqx to have a configurable qlogger. Currently, There isn't a straight forward way to enable async logging or log sampling without a wrapper of our own, something like this: openmoq/moxygen#242 ___ Differential Revision: D107700435 fbshipit-source-id: c9f768325d1ab85deb7eb6185670da3284861fe8
Summary: This change provides a way for downstream apps like moxygen and moqx to have a configurable qlogger. Currently, There isn't a straight forward way to enable async logging or log sampling without a wrapper of our own, something like this: openmoq/moxygen#242 ___ Differential Revision: D107700435 fbshipit-source-id: c9f768325d1ab85deb7eb6185670da3284861fe8
akash-a-n
force-pushed
the
feature/mlog-sampling
branch
from
6ae1cc9 to
49a31a3
Compare
2 participants
This change is