QUIC: qlog - #1398
Conversation
|
We ran this patch on production CDN traffic and hit a trace-loss bug in The qlog file is named by the client's source connection id ( Naming the file by the original DCID fixes it: it is at least 8 bytes, effectively unique, and is already what --- a/src/event/quic/ngx_event_quic_qlog.c
+++ b/src/event/quic/ngx_event_quic_qlog.c
@@ static ngx_int_t ngx_quic_qlog_open(...)
- if (qc->path == NULL || qc->path->cid == NULL) {
+ if (qc->tp.original_dcid.len == 0) {
return NGX_ERROR;
}
- file.len = dir->len + 1 + qc->path->cid->len * 2 + sizeof(".sqlog");
+ file.len = dir->len + 1 + qc->tp.original_dcid.len * 2 + sizeof(".sqlog");
@@
- p = ngx_hex_dump(p, qc->path->cid->id, qc->path->cid->len);
+ p = ngx_hex_dump(p, qc->tp.original_dcid.data,
+ qc->tp.original_dcid.len);
|
There was a problem hiding this comment.
Pull request overview
This PR introduces optional QUIC qlog (JSON-SEQ) generation to nginx’s QUIC implementation, including build-time enablement and runtime configuration, with per-connection .sqlog output intended for qvis consumption.
Changes:
- Adds
--with-quic_qlog_modulebuild flag andNGX_QUIC_QLOGconditional compilation for qlog support. - Adds new HTTP-level directives to enable/parameterize qlog (path, sampling, max size, importance, allow-list).
- Hooks qlog event emission into QUIC connection lifecycle, packet send/receive, frame parsing, loss, and metric updates.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/http/v3/ngx_http_v3_module.c | Adds HTTP/server directives for configuring QUIC qlog behavior. |
| src/event/quic/ngx_event_quic.h | Extends QUIC config with qlog parameters and defines qlog importance levels. |
| src/event/quic/ngx_event_quic.c | Hooks qlog init/start/TP events into connection creation and adds packet receive start/end logging. |
| src/event/quic/ngx_event_quic_transport.c | Records parsed frame length on ngx_quic_frame_t to support logging. |
| src/event/quic/ngx_event_quic_ssl.c | Logs remote transport parameters when received via TLS callbacks. |
| src/event/quic/ngx_event_quic_qlog.h | Declares qlog API and provides no-op stubs when qlog is not compiled in. |
| src/event/quic/ngx_event_quic_qlog.c | Implements qlog writing, buffering, and QUIC event/frame serialization. |
| src/event/quic/ngx_event_quic_output.c | Adds packet send start/end logging and frame logging on transmit paths; logs metric updates. |
| src/event/quic/ngx_event_quic_migration.c | Logs metric updates when path validation completes / RTT init occurs. |
| src/event/quic/ngx_event_quic_connection.h | Adds qc->qlog pointer under NGX_QUIC_QLOG. |
| src/event/quic/ngx_event_quic_ack.c | Logs metric updates and packet loss triggers into qlog. |
| auto/options | Adds --with-quic_qlog_module configure option. |
| auto/modules | Conditionally compiles qlog implementation and sets NGX_QUIC_QLOG. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| file.len = dir->len + 1 + qc->tp.original_dcid.len * 2 + sizeof(".sqlog"); | ||
| file.data = ngx_pnalloc(c->pool, file.len); | ||
| if (file.data == NULL) { | ||
| return NGX_ERROR; | ||
| } | ||
|
|
||
| p = file.data; | ||
|
|
||
| p = ngx_cpymem(p, dir->data, dir->len); | ||
|
|
||
| if (!ngx_path_separator(*(p - 1))) { | ||
| *p++ = '/'; | ||
| } else { | ||
| file.len--; | ||
| } | ||
|
|
||
| p = ngx_hex_dump(p, qc->tp.original_dcid.data, qc->tp.original_dcid.len); | ||
|
|
||
| p = ngx_cpymem(p, ".sqlog", sizeof(".sqlog") - 1); | ||
| *p = '\0'; | ||
|
|
||
| qc->qlog->fd = ngx_open_file(file.data, NGX_FILE_WRONLY, NGX_FILE_TRUNCATE, | ||
| NGX_FILE_DEFAULT_ACCESS); |
| if (f->u.close.reason.len > 0) { | ||
| ngx_qlog_write_char(p, end, ','); | ||
| ngx_qlog_write_pair_strv(p, end, "reason", &f->u.close.reason); | ||
| } | ||
|
|
Thank you for catching this! I was aware that the RFC allows the client's source connection ID to be zero-length, but I falsely assumed it wouldn't happen in real-world traffic... Thanks again for the catch and the detailed write-up. Fixed. |
|
Heads-up on a latent fd bug in
Trigger is memory pressure at the first qlog connection in a worker, so it's rare, but the failure mode (cross-talk on a recycled fd 0) is nasty and silent. Fix is two lines in qc->qlog = ngx_pcalloc(c->pool, sizeof(ngx_quic_qlog_t));
if (qc->qlog == NULL) {
return NGX_ERROR;
}
qc->qlog->fd = NGX_INVALID_FILE; /* add */
if (ngx_quic_qlog_init_worker_buffers() != NGX_OK) {
qc->qlog->closed = 1; /* add */
return NGX_ERROR;
}Setting |
Added events: - connectivity:connection_started https://datatracker.ietf.org/doc/html/draft-ietf-quic-qlog-quic-events-03#connectivity-connectionstarted - connectivity:connection_closed https://datatracker.ietf.org/doc/html/draft-ietf-quic-qlog-quic-events-03#connectivity-connectionclosed - transport:parameters_set https://datatracker.ietf.org/doc/html/draft-ietf-quic-qlog-quic-events-03#transport-parametersset - transport:packet_sent https://datatracker.ietf.org/doc/html/draft-ietf-quic-qlog-quic-events-03#transport-packetsent - transport:packet_received https://datatracker.ietf.org/doc/html/draft-ietf-quic-qlog-quic-events-03#transport-packetreceived - recovery:metrics_updated https://datatracker.ietf.org/doc/html/draft-ietf-quic-qlog-quic-events-03#recovery-metricsupdated - recovery:packet_lost https://datatracker.ietf.org/doc/html/draft-ietf-quic-qlog-quic-events-03#recovery-packetlost
Thanks again for finding this, and for the suggested fix. Fixed. |
|
Third one from running this under sustained production HTTP/3 traffic (verified against PR head Mechanism
if (ngx_quic_qlog_flush(qlog) != NGX_OK) {
return NGX_ERROR;
}
ngx_quic_qlog_out_owner = qlog; /* re-owns a qlog the flush just closed */
...
ngx_quic_qlog_out_last = ngx_cpymem(ngx_quic_qlog_out_last, buf, size); /* strands one event */
if (qc->qlog && qc->qlog->fd != NGX_INVALID_FILE) { /* false: fd already closed by max_size */
(void) ngx_quic_qlog_flush(qc->qlog);
...
}
EvidenceTwo production worker cores, both faulting in Recipe: FixSmallest change is to not re-own a qlog the flush just closed: if (ngx_quic_qlog_flush(qlog) != NGX_OK) {
return NGX_ERROR;
}
if (qlog->closed) {
return NGX_OK; /* flush hit max_size and closed the fd; owner already cleared */
}
ngx_quic_qlog_out_owner = qlog;Belt-and-suspenders, also have Both changes as a single commit on top of the current PR head ( |
|
Thanks for the great work, I will take a look |
This patch adds qlog support for QUIC.
The work was developed as part of my master's thesis at the Czech Technical University in Prague (CTU), on top of upstream nginx QUIC
master. During development I split it into three branches with increasing scope:quic-qlog,quic-qlog-extended, andhttp3-qlog. For upstream review, this PR submits only the first and smallest branch,quic-qlog; I am also attaching the the thesis PDF for broader design and evaluation context.Summary
--with-quic_qlog_module.sqlogfile per QUIC connectionngx_event_quic_qlog.c/.hConfiguration
The patch adds the following directives in
httpandservercontexts:quic_qlog on|offquic_qlog_path <directory>quic_qlog_importance core|base|extraquic_qlog_sample <N>quic_qlog_max_size <size>quic_qlog_allow <address-or-cidr>Design notes
The implementation targets the qlog draft family used by qvis interoperability (
qlog_version: 0.3, JSON-SEQ), and the resulting logs are intended to be consumed by qvis:draft-ietf-quic-qlog-main-schema-05: https://datatracker.ietf.org/doc/html/draft-ietf-quic-qlog-main-schema-05draft-ietf-quic-qlog-quic-events-03: https://datatracker.ietf.org/doc/html/draft-ietf-quic-qlog-quic-events-03draft-ietf-quic-qlog-h3-events-03: https://datatracker.ietf.org/doc/html/draft-ietf-quic-qlog-h3-events-03Testing
Regression testing was done against the nginx Perl test suite, and qlog-specific behavior was exercised through a dedicated
nginx-testsfork:Performance notes
Relative to upstream
master, compiling qlog support in but leaving it disabled did not measurably affect throughput. With qlog enabled, thequic-qlogbranch reduced throughput by about 8.1% on average and increased p95 latency by about 8.4%. The complete benchmark methodology and results are included in the attached thesis.