Count proxy.process.http.incoming_requests at transaction start#12823
Count proxy.process.http.incoming_requests at transaction start#12823bneradt merged 1 commit intoapache:masterfrom
Conversation
The proxy.process.http.incoming_requests metric was only incremented in
HttpTransact::HandleRequest(), which is called late in the request
processing pipeline. Requests that received error or redirect responses
before reaching HandleRequest were never counted, causing significant
underreporting during high-error-rate scenarios.
This fix moves the increment to HttpSM::attach_client_session(), which
is called at the start of every transaction. This ensures all requests
are counted, including those that fail header parsing, receive remap
redirects, or hit early error conditions. This aligns with the
documented behavior that incoming_requests should include errors and
redirects (see doc/appendices/command-line/traffic_top.en.rst):
"Total number of client requests serviced by Traffic Server. This
includes both successful content-bearing responses as well as
errors, redirects, and not-modified IMS responses."
|
The new location looks better, but I'm wondering if this is the earliest possible point to increment the metric. I think H2 requests that have decoding errors (e.g. unrecognized pseudo headers, compression errors, etc) are still not counted. |
You're right that H2 HPACK decode errors occur before new_transaction() and won't be counted. This fix addresses HTTP-level early failures (remap redirects, header parse errors, early error responses) which was the primary concern — these are now counted correctly. H2/H3 frame-level decode failures are a different category. Counting those would require incrementing in the frame processing code before decode, which is more involved. I suggest doing this simple move change which relatively easily makes the current counting more accurate. |
maskit
left a comment
There was a problem hiding this comment.
I wondered if the constructor of ProxyTransaction is a good place. Http1ClientTransact, which is not created on every request, is tricky though.
We'll probably need to define "incoimng_request" first when we make this more accurate. I agree that frame-level decode failure is a different category, but I'd say a header block (not the frame but the compressed binary header representation) is an incoming request even if it's not decodable. And Incremental Forwarding of HTTP Messages would make things much more complicated (incomplete request is officially a valid request).
So, yeah, I don't think we should look for the perfect place at this time. It would be nice if we could use the constructor without much effort.
The proxy.process.http.incoming_requests metric was only incremented in HttpTransact::HandleRequest(), which is called late in the request processing pipeline. Requests that received error or redirect responses before reaching HandleRequest were never counted, causing significant underreporting during high-error-rate scenarios.
This fix moves the increment to HttpSM::attach_client_session(), which is called at the start of every transaction. This ensures all requests are counted, including those that fail header parsing, receive remap redirects, or hit early error conditions. This aligns with the documented behavior that incoming_requests should include errors and redirects (see doc/appendices/command-line/traffic_top.en.rst):