http: fix upstream_rq stat increment #4055
Conversation
Signed-off-by: Rama <rama.rao@salesforce.com>
|
@mattklein123 PTAL. Fixed upstream_rq increment incase of connection failure in http1. For http2 it seems to work correctly, so just added test validation. |
| @@ -145,6 +145,11 @@ void ConnPoolImpl::onConnectionEvent(ActiveClient& client, Network::ConnectionEv | |||
| // The only time this happens is if we actually saw a connect failure. | |||
| host_->cluster().stats().upstream_cx_connect_fail_.inc(); | |||
| host_->stats().cx_connect_fail_.inc(); | |||
|
|
|||
| // Increment the total requests also. | |||
| host_->cluster().stats().upstream_rq_total_.inc(); | |||
There was a problem hiding this comment.
I think it would be better to consolidate this logic and only increment these stats in 1 place. Can you take a look at the other place we do this and move the increment to when the connection pool is first asked for a stream?
Signed-off-by: Rama <rama.rao@salesforce.com>
|
@mattklein123 consolidated the logic. PTAL when you get a chance? |
| @@ -265,9 +268,7 @@ ConnPoolImpl::StreamWrapper::StreamWrapper(StreamDecoder& response_decoder, Acti | |||
| StreamDecoderWrapper(response_decoder), parent_(parent) { | |||
|
|
|||
| StreamEncoderWrapper::inner_.getStream().addCallbacks(*this); | |||
| parent_.parent_.host_->cluster().stats().upstream_rq_total_.inc(); | |||
| parent_.parent_.host_->cluster().stats().upstream_rq_active_.inc(); | |||
There was a problem hiding this comment.
Arguably rq_active should move also, though, the logic to keep that consistent will be more complicated and I'm not sure it's worth it. Thoughts?
There was a problem hiding this comment.
Yes.It may not be worth the complication.
Signed-off-by: Rama rama.rao@salesforce.com
Description: Increment upstream_rq in case of connection failures also for http1.
Risk Level: Low
Testing: Added Automated tests
Docs Changes: N/A
Release Notes: N/A
Fixes #3950