quic: add quic downstream stats in hcm #16625

RenjieTang · 2021-05-21T22:43:20Z

Commit Message: Introduce QuicStats and add Quic connection close stats for downstream connections.
Risk Level: Low
Testing: unit tests
Docs Changes: n/a
Release Notes: n/a.
Platform Specific Features: n/a.

Signed-off-by: Renjie Tang <renjietang@google.com>

RenjieTang · 2021-05-21T22:43:28Z

/wait

jmarantz

This organization does look cleaner. Go for it.

Were there some unit tests for QuicStats in the other PR? In any case we should have some.

jmarantz · 2021-05-22T18:19:19Z

source/common/quic/quic_filter_manager_connection_impl.h

@@ -167,6 +170,7 @@ class QuicFilterManagerConnectionImpl : public Network::ConnectionImplBase,
  absl::optional<std::reference_wrapper<Http::Http3::CodecStats>> codec_stats_;
  absl::optional<std::reference_wrapper<const envoy::config::core::v3::Http3ProtocolOptions>>
      http3_options_;
+  absl::optional<std::reference_wrapper<QuicStats>> quic_stats_;


Use OptRef as a shortcut to this.

jmarantz · 2021-05-22T18:19:54Z

source/common/quic/envoy_quic_server_session.cc

@@ -82,6 +82,8 @@ void EnvoyQuicServerSession::setUpRequestDecoder(EnvoyQuicServerStream& stream)
 void EnvoyQuicServerSession::OnConnectionClosed(const quic::QuicConnectionCloseFrame& frame,
                                                quic::ConnectionCloseSource source) {
  quic::QuicServerSessionBase::OnConnectionClosed(frame, source);
+  ASSERT(quic_stats_.has_value());
+  quic_stats_->get().chargeQuicConnectionCloseStats(frame.quic_error_code, source);


quic_stats_->chargeQuicConnectionCloseStats if you use OptRef (per below).

alyssawilk

Yeah, I'd be inclined to close 16584 off in favor of this - when/if we have QUIC "tcp proxying" we can do a similar thing passing quic stats in via that network filter.

Signed-off-by: Renjie Tang <renjietang@google.com>

jmarantz · 2021-05-24T20:51:27Z

source/common/quic/envoy_quic_server_session.cc

@@ -82,6 +82,9 @@ void EnvoyQuicServerSession::setUpRequestDecoder(EnvoyQuicServerStream& stream)
 void EnvoyQuicServerSession::OnConnectionClosed(const quic::QuicConnectionCloseFrame& frame,
                                                quic::ConnectionCloseSource source) {
  quic::QuicServerSessionBase::OnConnectionClosed(frame, source);
+  if (quic_stats_.has_value()) {


Under what circumstances would quic_stats_ not be set?

We do have other examples where there are optional blocks of stats, but those are runtime-feature enabled for extra stats detail.

I think in your usage model this will always be set except for tests. Can we have more consistent behavior where we always set the stats-ref? In other words, make QuicStats be a required ctor arg for EnvoyQuicServerSession?

Just like other Optref stats, this field is not initialized when the object is created. It will be initialized in QuicHttpServerConnectionImpl constructor.
From the current flow, QuicStats can't be in the constructor and be initialized at the very beginning.

There have been assertions that those other OptRef stats must be initialized before creating a new stream and invoke Envoy bug if the field is not initialized.

I intended to do the same in EnvoyQuicServerSession::OnConnectionClosed(). But unlike the assertions in CreateIncomingStream() where the session is guaranteed to be initialized by HCM, OnConnectionClosed() could be called before the HCM setup.

One example is that in EnvoyQuicDispatcherTest.CloseConnectionDuringFilterInstallation, the connection is closed during processing CHLO.
I'm not sure if that is a valid case in production. But this does mean that if we let the HCM own quic_stats_, we will miss out logging stats when the connection is closed before HCM filter is initialized. Tagging @alyssawilk as Alyssa knows more on this QUIC integration.

There's a difference. The other OptRef stats are optional due to config. These are not. I'm trying to suss out why you need a required object to be declared as Optional, as I suspect it's an indication something else is not quite right (see my other comment).

I see that EnvoyQuicConnection is instantiated by EnvoyQuicDispatcher. I'm inferring there's one of those per thread, so it's not a good place for QuicStats to be owned, though it's fine for it to have a reference.

I also see that it is created in EnvoyQuicDIspatcher::CreateQuicSession. EnvoyQuicDispatcher has member variables so I think we can supply QuicStats as a ctor arg. Let's go up-stack again.

That gets created from an ActiveQuicListener, which gets created from ActiveQuicListenerFactory, and that's created by Server::ListenerImpl::buildUdpListenerFactory. Cool.

ListenerImpl is created from ListenerManagerFactory. I think ListenerManagerFactory can own the QuicImpl, and pass it down through the ctor/chain so it's available where EnvoyQuicConnection is instantiated.

I think this is a better place because I want there to be exactly one of them in a Server, and it should be available unconditionally whenever a EnvoyQuicConnection is instantiated.

The reason is that there can be many different listener scopes, for different ports that Envoy is listening on. But there should be only one QuicStats object (actually a better name for that structure would be QuicStatNames), since it doesn't actually hold any counters. It just holds their lazily symbolized names.

Maybe what I'm saying is that HCM is not quite right because there's one of them for each listener? Not sure. In any case the order-of-construction seems wrong.

The reason the plumbing needs to be a little more complex than what you have here is to get the ownership/sharing model right.

Thanks for the clarification! I now realized that there should only be one QuicStats object in the whole server instance.

I think it makes a lot of sense for ListenerManager to own QuicStats. This way we guaranteed that a QuicStats singleton is initiated during server startup and subsequent Quic connections can use it right away.
I updated #16584 instead of this one since I pursued the listener route on the other branch and thus the change was more gradual.

PTAL.

test/integration/fake_upstream.h

Signed-off-by: Renjie Tang <renjietang@google.com>

RenjieTang · 2021-05-24T22:11:33Z

/wait

Signed-off-by: Renjie Tang <renjietang@google.com>

jmarantz · 2021-05-25T01:43:34Z

source/common/quic/quic_stats.cc

+namespace Quic {
+
+QuicStats::QuicStats(Stats::Scope& scope, bool is_upstream)
+    : scope_(scope), stat_name_pool_(scope.symbolTable()), symbol_table_(scope.symbolTable()),


Don't pass the scope to the ctor; pass it to incCounter instead. Otherwise you are going to wind up replicating the construction of all these StatNames in each scope.

jmarantz · 2021-05-25T02:14:18Z

source/common/quic/envoy_quic_server_session.cc

@@ -82,6 +82,9 @@ void EnvoyQuicServerSession::setUpRequestDecoder(EnvoyQuicServerStream& stream)
 void EnvoyQuicServerSession::OnConnectionClosed(const quic::QuicConnectionCloseFrame& frame,
                                                quic::ConnectionCloseSource source) {
  quic::QuicServerSessionBase::OnConnectionClosed(frame, source);
+  if (quic_stats_.has_value()) {


There's a difference. The other OptRef stats are optional due to config. These are not. I'm trying to suss out why you need a required object to be declared as Optional, as I suspect it's an indication something else is not quite right (see my other comment).

I see that EnvoyQuicConnection is instantiated by EnvoyQuicDispatcher. I'm inferring there's one of those per thread, so it's not a good place for QuicStats to be owned, though it's fine for it to have a reference.

I also see that it is created in EnvoyQuicDIspatcher::CreateQuicSession. EnvoyQuicDispatcher has member variables so I think we can supply QuicStats as a ctor arg. Let's go up-stack again.

That gets created from an ActiveQuicListener, which gets created from ActiveQuicListenerFactory, and that's created by Server::ListenerImpl::buildUdpListenerFactory. Cool.

ListenerImpl is created from ListenerManagerFactory. I think ListenerManagerFactory can own the QuicImpl, and pass it down through the ctor/chain so it's available where EnvoyQuicConnection is instantiated.

I think this is a better place because I want there to be exactly one of them in a Server, and it should be available unconditionally whenever a EnvoyQuicConnection is instantiated.

The reason is that there can be many different listener scopes, for different ports that Envoy is listening on. But there should be only one QuicStats object (actually a better name for that structure would be QuicStatNames), since it doesn't actually hold any counters. It just holds their lazily symbolized names.

Maybe what I'm saying is that HCM is not quite right because there's one of them for each listener? Not sure. In any case the order-of-construction seems wrong.

The reason the plumbing needs to be a little more complex than what you have here is to get the ownership/sharing model right.

jmarantz · 2021-05-27T13:16:44Z

Closing this in favor of the other PR

add quic downstream stats in hcm

d124e5e

Signed-off-by: Renjie Tang <renjietang@google.com>

RenjieTang requested review from alyssawilk and mattklein123 as code owners May 21, 2021 22:43

repokitteh-read-only bot added the waiting label May 21, 2021

RenjieTang changed the title ~~quic:: add quic downstream stats in hcm~~ quic: add quic downstream stats in hcm May 21, 2021

jmarantz reviewed May 22, 2021

View reviewed changes

jmarantz self-assigned this May 22, 2021

alyssawilk reviewed May 24, 2021

View reviewed changes

alyssawilk self-assigned this May 24, 2021

Address comments, add tests, and add QUIC guard.

853180d

Signed-off-by: Renjie Tang <renjietang@google.com>

repokitteh-read-only bot removed the waiting label May 24, 2021

jmarantz reviewed May 24, 2021

View reviewed changes

test/integration/fake_upstream.h Outdated Show resolved Hide resolved

Unify proprocessor style

9a80b10

Signed-off-by: Renjie Tang <renjietang@google.com>

repokitteh-read-only bot added the waiting label May 24, 2021

add class comments.

67110a3

Signed-off-by: Renjie Tang <renjietang@google.com>

repokitteh-read-only bot removed the waiting label May 24, 2021

jmarantz reviewed May 25, 2021

View reviewed changes

jmarantz closed this May 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

quic: add quic downstream stats in hcm #16625

quic: add quic downstream stats in hcm #16625

RenjieTang commented May 21, 2021 •

edited

Loading

RenjieTang commented May 21, 2021

jmarantz left a comment

jmarantz May 22, 2021

jmarantz May 22, 2021

alyssawilk left a comment

jmarantz May 24, 2021

RenjieTang May 24, 2021

jmarantz May 25, 2021

RenjieTang May 25, 2021

RenjieTang commented May 24, 2021

jmarantz May 25, 2021

jmarantz May 25, 2021

jmarantz commented May 27, 2021

quic: add quic downstream stats in hcm #16625

quic: add quic downstream stats in hcm #16625

Conversation

RenjieTang commented May 21, 2021 • edited Loading

RenjieTang commented May 21, 2021

jmarantz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alyssawilk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RenjieTang commented May 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmarantz commented May 27, 2021

RenjieTang commented May 21, 2021 •

edited

Loading