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

Reported processing duration for http1 and http2 is not comparable #4282

Open
nigoroll opened this issue Feb 22, 2025 · 1 comment
Open

Reported processing duration for http1 and http2 is not comparable #4282

nigoroll opened this issue Feb 22, 2025 · 1 comment

Comments

@nigoroll
Copy link
Member

nigoroll commented Feb 22, 2025

Expected Behavior

Provide statistics which reflect reality as realistically as possible

Current Behavior

On a production system, http2 was disabled. The ttfb and total processing time for cache hits as reported by post-processing of varnishncsa logs then went through the roof:

Image

Context

Regarding ttfb (as deduced from Timestamp Processing:), the reporting point is independent of the protocol here and here.

The timestamp is relative to req->t_first, which is initialized in these places:

  • http1

hs = HTC_RxStuff(req->htc, HTTP1_Complete,
&req->t_first, &req->t_req,
sp->t_idle + SESS_TMO(sp, timeout_linger),
sp->t_idle + SESS_TMO(sp, timeout_idle),
NAN,
cache_param->http_req_size);

  • http2

req->t_first = VTIM_real();
req->t_req = VTIM_real();
req->t_prev = req->t_first;
VSLb_ts_req(req, "Start", req->t_first);
req->acct.req_hdrbytes += h2->rxf_len;

where the dispatched function h2_rx_headers() is called via h2_procframe() from h2_rxframe(), which has the actual receive:

hs = HTC_RxStuff(h2->htc, h2_frame_complete, NULL, NULL, NAN,
VTIM_real() + 0.5, NAN, h2->local_settings.max_frame_size + 9);

One simple option to make the values comparable would be to start "Processing" after the receive also for http/1. Or we could at least take the t_first timestamp also at the beginning of the receive in h2_rxframe() for http2.

Opinions?

@nigoroll
Copy link
Member Author

bugwash: make h2 reporting more like h1.

@nigoroll nigoroll added the a=Implement Issue is ready for implementation label Feb 24, 2025
nigoroll added a commit to nigoroll/varnish-cache that referenced this issue Feb 24, 2025
A newly created request's t_first is now when the first (partial) read of the
HEADERS frame is complete.

t_req is now when all headers are complete.

This should match http1 more closely

Implements varnishcache#4282

Alternatives considered: Add the timestamp as a field to struct h2_sess. I did
not like growing this big structure even more.
@nigoroll nigoroll removed the a=Implement Issue is ready for implementation label Feb 24, 2025
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

No branches or pull requests

1 participant