quic: add QUIC connection close stats to upstream connections by RenjieTang · Pull Request #17222 · envoyproxy/envoy

RenjieTang · 2021-07-02T17:21:37Z

Commit Message: Allow upstream quic connections to charge connection close stats.
Additional Description: For upstream connections, QuicStatNames reference is now passed down as Server->ClusterManager->ConnPool->EnvoyQuicConnection.
Risk Level: Low
Testing: unit tests
Docs Changes: docs/root/configuration/upstream/cluster_manager/cluster_stats.rst

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

RenjieTang · 2021-07-12T20:04:14Z

/assign @jmarantz

jmarantz · 2021-07-13T15:35:13Z

source/common/http/http3/conn_pool.cc

  return std::make_unique<Http3ConnPoolImpl>(
      host, priority, dispatcher, options, transport_socket_options, random_generator, state,
-      [](HttpConnPoolImplBase* pool) -> ::Envoy::ConnectionPool::ActiveClientPtr {
+      [&](HttpConnPoolImplBase* pool) -> ::Envoy::ConnectionPool::ActiveClientPtr {


I think for non-test uses I'd avoid capturing [&] -- can you capture just what's needed?

jmarantz · 2021-07-13T15:39:01Z

test/common/http/conn_pool_grid_test.cc

  std::shared_ptr<Upstream::MockClusterInfo> cluster_{new NiceMock<Upstream::MockClusterInfo>()};
  NiceMock<Random::MockRandomGenerator> random_;
  AlternateProtocolsCacheSharedPtr alternate_protocols_;
+  Stats::IsolatedStoreImpl scope_;


I think typically in tests if we are instantiating a store we'd call the member variable stats_ or store_ or stats_store_.

Here & below.

jmarantz · 2021-07-13T15:43:13Z

test/integration/server.h

  absl::Notification server_gone_;
  Stats::SymbolTableImpl symbol_table_;
  std::unique_ptr<Stats::AllocatorImpl> stats_allocator_;
+  Quic::QuicStatNames* quic_stat_names_;


use OptRef<Quic:QuicStatNames> rather than a pointer here. It's more idiomatic for Envoy style, and also avoids having an uninitialized value there potentially.

jmarantz · 2021-07-13T15:45:02Z

test/integration/utility.cc

  return sendRequestAndWaitForResponse(*dispatcher, method, url, body, host, content_type, client);
 #else
  ASSERT(false, "running a QUIC integration test without compiling QUIC");
+  (void)quic_stat_names;


use UNREFERENCED_PARAMETER(quic_stat_names) in ./source/common/common/macros.h

jmarantz · 2021-07-13T15:48:52Z

test/integration/utility.cc

                Filesystem::fileSystemForTest(), random_generator);
  Event::DispatcherPtr dispatcher(api.allocateDispatcher("test_thread"));
  TestConnectionCallbacks connection_callbacks(*dispatcher);
+


given the existing usage model and implementation of this, where mock_stats_store is instantiated a few lines up, I think it would be more consistent for you to instantiate a new Quic::QuicStatNames here, using mock_stats_store.symbolTable().

That will avoid the new args to makeSingleRequest, and remove a large number of other changes in this PR I think.

That's an great advice!
done.

jmarantz · 2021-07-13T15:49:45Z

/wait

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

jmarantz

this basically looks great, with a couple of small nits and a suggestion.

test/mocks/server/BUILD

jmarantz · 2021-07-14T00:05:51Z

test/integration/utility.h

 #include "source/common/common/dump_state_utils.h"
 #include "source/common/common/utility.h"
 #include "source/common/http/codec_client.h"
+#include "source/common/quic/quic_stat_names.h"


it shouldn't be necessary to include quic_stat_names.h here in the header; you should include it from the cc

done including it in the cc file

source/common/upstream/cluster_manager_impl.h

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

jmarantz

OK -- I was worried that the lack of a Quic::Context might be a barrier to easily finishing off that PR, and also favor the parallel with the Http::Context and Grpc::Context.

But I don't feel that strongly about it, and you can always add it later (albeit with another large PR).

/assign-from @envoyproxy/senior-maintainers

repokitteh-read-only · 2021-07-14T20:58:50Z

@envoyproxy/senior-maintainers assignee is @alyssawilk

🐱

Caused by: a #17222 (review) was submitted by @jmarantz.

see: more, trace.

alyssawilk · 2021-07-15T16:39:45Z

want to check out CI and I can take a look once that's sorted?
/wait

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

alyssawilk

Nice, thanks for following up on this!

alyssawilk · 2021-07-15T19:13:41Z

test/common/quic/envoy_quic_client_session_test.cc

+
+  EXPECT_EQ(
+      1U, TestUtility::findCounter(
+              store_, "http3.upstream.rx.quic_connection_close_error_code_QUIC_INVALID_FRAME_DATA")


do we have docs for this?
I'm not entirely sure where we'd put them. Currently the upstream stats I'm familiar with are rooted in the cluster so docced in cluster_stats.rst
That said these are only updated when traffic is sent to a cluster, so maybe put them there but make it clear they're not rooted in the cluster and are in fact global? Unless @mattklein123 or @phlax have better suggestions

Yeah I think that might be the best place to put this doc. done.

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

alyssawilk

LGTM, though it looks like one comment of @jmarantz may not be addressed - I'll let him check and merge.
/retest for the windows flake

repokitteh-read-only · 2021-07-19T14:07:30Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #17222 (review) was submitted by @alyssawilk.

see: more, trace.

jmarantz · 2021-07-19T17:23:08Z

/wait

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

alyssawilk · 2021-07-20T13:15:40Z

unfortunately needs a main merge before we can land this

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

alyssawilk · 2021-07-20T21:08:08Z

Hey Renjie, is the windows release failure that uint64 optional issue you mentioned? Looks like it might need one more pass?

RenjieTang · 2021-07-20T21:10:06Z

Hey Renjie, is the windows release failure that uint64 optional issue you mentioned? Looks like it might need one more pass?

Yes it's still the optional<uint64_t> issue. I've filed #17409

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

RenjieTang · 2021-07-21T22:41:30Z

/retest

repokitteh-read-only · 2021-07-21T22:41:34Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #17222 (comment) was created by @RenjieTang.

see: more, trace.

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

1. Update ENVOY_COMMIT to the latest Envoy's commit 2. Update ENVOY_SHA in bazel/repositories.bzl 3. Pass an extra QuicStatNames argument when creating the cluster_manager_factory_ in process_impl.cc. This is to accommodate the recent change in the constructor of Envoy::Upstream::ProdClusterManagerFactory in envoyproxy/envoy#17222. Signed-off-by: Jason Ye jiajunye@google.com

…roxy#17222) * add QUIC stats to upstream connections Signed-off-by: Renjie Tang <renjietang@google.com> * format Signed-off-by: Renjie Tang <renjietang@google.com> * minor ci fixes Signed-off-by: Renjie Tang <renjietang@google.com> * format Signed-off-by: Renjie Tang <renjietang@google.com> * fix compiler error Signed-off-by: Renjie Tang <renjietang@google.com> * address comments. Signed-off-by: Renjie Tang <renjietang@google.com> * clean up unused variables. Signed-off-by: Renjie Tang <renjietang@google.com> * remove unused includes. Signed-off-by: Renjie Tang <renjietang@google.com> * fix ci compile time option Signed-off-by: Renjie Tang <renjietang@google.com> * fix an unrelated Windows ci failure. Signed-off-by: Renjie Tang <renjietang@google.com> * add docs and fix a compile option issue. Signed-off-by: Renjie Tang <renjietang@google.com> * fix include Signed-off-by: Renjie Tang <renjietang@google.com> * fix windows CI failure in config_impl_test Signed-off-by: Renjie Tang <renjietang@google.com> * fix unsuccessful merge to upstream. Signed-off-by: Renjie Tang <renjietang@google.com>

RenjieTang added 7 commits July 2, 2021 10:09

add QUIC stats to upstream connections

bbba2a8

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

Merge remote-tracking branch 'upstream/main' into upstream-stat

32458cf

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

format

979a6f0

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

minor ci fixes

e72f03f

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

Merge remote-tracking branch 'upstream/main' into upstream-stat

db41f16

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

format

90e5ffb

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

fix compiler error

4c1dded

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

RenjieTang marked this pull request as ready for review July 12, 2021 20:03

RenjieTang requested review from asraa, ggreenway, lizan, snowp and yanavlasov as code owners July 12, 2021 20:03

repokitteh-read-only bot assigned jmarantz Jul 12, 2021

jmarantz reviewed Jul 13, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Jul 13, 2021

address comments.

1599731

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

repokitteh-read-only bot removed the waiting label Jul 13, 2021

RenjieTang added 2 commits July 13, 2021 16:23

Merge remote-tracking branch 'upstream/main' into upstream-stat

63264e4

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

clean up unused variables.

b7282cb

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

jmarantz reviewed Jul 14, 2021

View reviewed changes

remove unused includes.

43ddd9e

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

jmarantz previously approved these changes Jul 14, 2021

View reviewed changes

repokitteh-read-only bot assigned alyssawilk Jul 14, 2021

repokitteh-read-only bot added the waiting label Jul 15, 2021

fix ci compile time option

5ef811b

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

alyssawilk reviewed Jul 15, 2021

View reviewed changes

add docs and fix a compile option issue.

83aeec1

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

alyssawilk previously approved these changes Jul 19, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Jul 19, 2021

fix include

bb7470d

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

RenjieTang dismissed alyssawilk’s stale review via bb7470d July 19, 2021 17:27

repokitteh-read-only bot removed the waiting label Jul 19, 2021

jmarantz previously approved these changes Jul 19, 2021

View reviewed changes

RenjieTang mentioned this pull request Jul 19, 2021

Multiple tests fail to build on Windows CI #17409

Closed

alyssawilk added the waiting label Jul 20, 2021

Merge remote-tracking branch 'upstream/main' into upstream-stat

e6e6942

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

RenjieTang dismissed jmarantz’s stale review via e6e6942 July 20, 2021 17:38

repokitteh-read-only bot removed the waiting label Jul 20, 2021

alyssawilk added the waiting label Jul 20, 2021

fix windows CI failure in config_impl_test

a5d26ca

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

repokitteh-read-only bot removed the waiting label Jul 21, 2021

RenjieTang added 2 commits July 21, 2021 21:30

Merge remote-tracking branch 'upstream/main' into upstream-stat

0f60b98

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

fix unsuccessful merge to upstream.

6435d90

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

jmarantz approved these changes Jul 22, 2021

View reviewed changes

jmarantz merged commit 2b716d1 into envoyproxy:main Jul 22, 2021

jiajunye mentioned this pull request Jul 26, 2021

Update Envoy to b4027cf (June 26th 2021) envoyproxy/nighthawk#714

Merged

Conversation

RenjieTang commented Jul 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RenjieTang commented Jul 12, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmarantz commented Jul 13, 2021

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

repokitteh-read-only bot commented Jul 14, 2021

Uh oh!

alyssawilk commented Jul 15, 2021

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

repokitteh-read-only bot commented Jul 19, 2021

Uh oh!

jmarantz commented Jul 19, 2021

Uh oh!

alyssawilk commented Jul 20, 2021

Uh oh!

alyssawilk commented Jul 20, 2021

Uh oh!

RenjieTang commented Jul 20, 2021

Uh oh!

RenjieTang commented Jul 21, 2021

Uh oh!

repokitteh-read-only bot commented Jul 21, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

RenjieTang commented Jul 2, 2021 •

edited

Loading