Http Conn Man: Per Listener downstream_rq_*xx stats #1783
Conversation
Signed-off-by: Constance Caramanolis <ccaramanolis@lyft.com>
| /** | ||
| * @return ConnectionManagerStats& the listener stats to write to. | ||
| */ | ||
| virtual ConnectionManagerStats& listenerStats() PURE; |
There was a problem hiding this comment.
Unless you are going to increment all stats in the context of the listener (which I don't think we want to do), I don't think it makes sense to create the entire tree for the listener. I would just do 2xx/3xx/4xx/5xx and make sure it is under the http.{name} namespace inside the listener scope.
There was a problem hiding this comment.
I was debating about this technique. Not sure if eventually we wanted to get all of the stats for ConnectionManager. I'll scope this to 2xx/3xx/4xx/5xx and ping back when the change is in.
ccaraman
changed the title
|
@mattklein123 ready for review |
| @@ -74,7 +74,8 @@ class HttpConnectionManagerImplTest : public Test, public ConnectionManagerConfi | |||
| POOL_TIMER(fake_stats_))}, | |||
| "", | |||
| fake_stats_}, | |||
| tracing_stats_{CONN_MAN_TRACING_STATS(POOL_COUNTER(fake_stats_))} { | |||
| tracing_stats_{CONN_MAN_TRACING_STATS(POOL_COUNTER(fake_stats_))}, | |||
| listener_stats_{CONN_MAN_LISTENER_STATS(POOL_COUNTER_PREFIX(fake_stats_, "http.fake."))} { | |||
There was a problem hiding this comment.
nit: for consistency I would make this use the same prefix as above which is "".
There was a problem hiding this comment.
Won't fix. If I were to do that, the Http Conn Man stats and the per listener stats would be under the same scope and the values for rq_*xx will double. I think this would make the tests confusing.
I can change the prefix in the constructor to be listener._.http.fake.
There was a problem hiding this comment.
What you should do is actually make a new scope with some prefix in it, instead of passing the same raw stats object. (Or pass a different isolated store).
There was a problem hiding this comment.
Passed a different isolated store.
|
Oh also I think this should have docs somewhere also. (With the existing HTTP conn man stats docs). |
Expose the listener stats scope to Http Connection Manager and increment the downstream_rq stats for 2xx, 3xx, 4xx, and 5xx.
Signed-off-by: Constance Caramanolis ccaramanolis@lyft.com