quic: add quic version counters in http3 codec stats. #16943

RenjieTang · 2021-06-11T18:30:47Z

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

Commit Message: add quic version counters in http3 codec stats.
Additional Description:
Risk Level: Low
Testing: Integration tests
Docs Changes: docs/root/configuration/http/http_conn_man/stats.rst

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

RenjieTang · 2021-06-11T18:31:04Z

/assign @alyssawilk

alyssawilk

Nice addition!
/wait

alyssawilk · 2021-06-14T12:51:57Z

docs/root/configuration/http/http_conn_man/stats.rst

@@ -194,6 +194,12 @@ On the upstream side all http3 statistics are rooted at *cluster.<name>.http3.*
   rx_reset, Counter, Total number of reset stream frames received by Envoy
   tx_reset, Counter, Total number of reset stream frames transmitted by Envoy
   metadata_not_supported_error, Counter, Total number of metadata dropped during HTTP/3 encoding
+   quic_version_43, Counter, Total number of quic connections that use transport version 43.


Hmmm, we have a general Envoy policy that stats don't get removed, but I think it makes sense that as we remove versions of HTTP/3 that we can remove the stats as they'd be implicitly 0 forevermore.

I think this is OK but I want to run this by @envoyproxy/senior-maintainers for second opinion. Alternately we can not add stats for the versions which are going away, but it'd be annoying between now and Q4 when we remove support for outdated versions.

Second, AFIK (@DavidSchinazi) we plan to remove support for non-H3 QUIC this fall as IETF-quic becomes the standard. I think we should make this EoL clear here now (even through QUIC is in alpha and has no guarantees) as part of bubbling up data on which versions are supported.

To clarify the plan from the QUIC side of things:

The default version today is h3-29

in some number of weeks, h3 (also called RFCv1 in parts of the code) will become the default version, and we plan on supporting that version for years to come

h3-29 will stay supported for some number of months, but then we'll drop support

version 43-51 will all be removed in some number of months

I'd suggest only supporting h3/RFCv1 when QUIC leaves Alpha in Envoy, as that's the only version we'll support long-term.

I think we have two options:

Add all the current version here, and remove stats as we deprecate versions

Add all the current version here, and have old version stats at 0 forever

Only add h3-29 and h3 here, and when we remove h3-29 we'll keep that stat at 0 forever

I'd personally lean towards option 1 if that's acceptable to Envoy, but if that's not an option then I'd suggest option 3.

Yeah, I'd lean towards 1 as well, I just want a thumbs up from a non-googler as a double check. I think if we comment that the stat will be going away as support is removed by EoY that makes it clear for users.

I prefer 1 as well. I want to be able to monitor the QUIC version deprecation process in Envoy as well.
Happy to add more notes in this doc if Envoy maintainers agree.

I hear no objections so let's just go for it :-P

alyssawilk · 2021-06-14T12:54:04Z

source/common/quic/quic_filter_manager_connection_impl.cc

@@ -169,6 +170,34 @@ void QuicFilterManagerConnectionImpl::onConnectionCloseEvent(
    ASSERT(network_connection_ != nullptr);
    network_connection_ = nullptr;
  }
+
+  if (!codec_stats_.has_value()) {
+    // The connection is closed before it can be used. stats are not recorded.


nit: -> was closed before it could be used
stats -> Stats

alyssawilk · 2021-06-14T12:54:12Z

source/common/quic/quic_filter_manager_connection_impl.cc

+    codec_stats_->quic_version_50_.inc();
+    return;
+  case quic::QUIC_VERSION_51:
+    std::cout << "logging" << std::endl;


alyssawilk · 2021-06-14T12:54:51Z

source/common/quic/quic_filter_manager_connection_impl.cc

+    codec_stats_->quic_version_rfc_v1_.inc();
+    return;
+  default:
+    return;


can we make sure we have unit tests for all of these for coverage?

Sure. I added a new EnvoyClientSessionAllQuicVersionTest for this since none of the existing tests cover all QUIC versions.

DavidSchinazi

Nice work! Some thoughts inline.

DavidSchinazi · 2021-06-14T16:40:45Z

source/common/quic/quic_filter_manager_connection_impl.cc

@@ -158,7 +158,8 @@ void QuicFilterManagerConnectionImpl::maybeApplyDelayClosePolicy() {
 }

 void QuicFilterManagerConnectionImpl::onConnectionCloseEvent(
-    const quic::QuicConnectionCloseFrame& frame, quic::ConnectionCloseSource source) {
+    const quic::QuicConnectionCloseFrame& frame, quic::ConnectionCloseSource source,
+    quic::QuicTransportVersion version) {


In general I'd prefer to plumb ParsedQuicVersion instead of QuicTransportVersion. We currently have a 1:1 mapping but that wasn't always the case and won't necessarily stay that way.

DavidSchinazi · 2021-06-14T16:46:51Z

docs/root/configuration/http/http_conn_man/stats.rst

@@ -194,6 +194,12 @@ On the upstream side all http3 statistics are rooted at *cluster.<name>.http3.*
   rx_reset, Counter, Total number of reset stream frames received by Envoy
   tx_reset, Counter, Total number of reset stream frames transmitted by Envoy
   metadata_not_supported_error, Counter, Total number of metadata dropped during HTTP/3 encoding
+   quic_version_43, Counter, Total number of quic connections that use transport version 43.


To clarify the plan from the QUIC side of things:

The default version today is h3-29

in some number of weeks, h3 (also called RFCv1 in parts of the code) will become the default version, and we plan on supporting that version for years to come

h3-29 will stay supported for some number of months, but then we'll drop support

version 43-51 will all be removed in some number of months

I'd suggest only supporting h3/RFCv1 when QUIC leaves Alpha in Envoy, as that's the only version we'll support long-term.

I think we have two options:

Add all the current version here, and remove stats as we deprecate versions

Add all the current version here, and have old version stats at 0 forever

Only add h3-29 and h3 here, and when we remove h3-29 we'll keep that stat at 0 forever

I'd personally lean towards option 1 if that's acceptable to Envoy, but if that's not an option then I'd suggest option 3.

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

RenjieTang · 2021-06-15T16:57:50Z

/retest

repokitteh-read-only · 2021-06-15T16:57:53Z

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

🐱

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

see: more, trace.

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

…bridge-stream * upstream/main: (268 commits) tools: adding dio,better comments (envoyproxy#17104) doc: fix misplaced #[extension-category] for Wasm runtimes (envoyproxy#17078) ci: Speedup deps precheck (envoyproxy#17102) doc: fix wrong link on wasm network filter. (envoyproxy#17079) docs: Added v3 API reference. (envoyproxy#17095) docs: Update include paths in repo (envoyproxy#17098) exception: make Ipv6Instance and Ipv4Instance not throw and remove some try catch pattern (envoyproxy#16122) tools: adding reminders for API shephards (envoyproxy#17081) ci: Fix wasm verify example (envoyproxy#17086) [fuzz]: fix oss fuzz bug 34515, limit maglev table size (envoyproxy#16671) test: silencing flaky test (envoyproxy#17084) Set `validate` flag when the SAN(SubjectAltName) matching is performed (envoyproxy#16816) Listener: reset the file event when destroying listener filters (envoyproxy#16952) docs: link additional filters that emit dynamic metadata (envoyproxy#17059) rds: add config reload time stat for rds (envoyproxy#17033) bazel: Use color by default for build and run commands (envoyproxy#17077) ci: Add timing for docker pull (envoyproxy#17074) [Windows] Adding note section in Original Source HTTP Filter (envoyproxy#17058) quic: add quic version counters in http3 codec stats. (envoyproxy#16943) quiche: change crypto stream factory interfaces (envoyproxy#17046) ... Signed-off-by: Garrett Bourg <bourg@squareup.com>

Commit Message: add quic version counters in http3 codec stats. Additional Description: Risk Level: Low Testing: Integration tests Docs Changes: docs/root/configuration/http/http_conn_man/stats.rst Signed-off-by: Renjie Tang <renjietang@google.com> Signed-off-by: chris.xin <xinchuantao@qq.com>

Commit Message: add quic version counters in http3 codec stats. Additional Description: Risk Level: Low Testing: Integration tests Docs Changes: docs/root/configuration/http/http_conn_man/stats.rst Signed-off-by: Renjie Tang <renjietang@google.com>

add quic version counters in http3 codec stats.

e788dd0

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

repokitteh-read-only bot assigned alyssawilk Jun 11, 2021

alyssawilk reviewed Jun 14, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Jun 14, 2021

DavidSchinazi reviewed Jun 14, 2021

View reviewed changes

add comprehensive test coverage and address comments.

951e467

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

repokitteh-read-only bot removed the waiting label Jun 14, 2021

alyssawilk added the waiting:any label Jun 21, 2021

add notes to stats doc.

7871e9d

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

repokitteh-read-only bot removed the waiting:any label Jun 21, 2021

alyssawilk approved these changes Jun 21, 2021

View reviewed changes

alyssawilk merged commit 4cfda99 into envoyproxy:main Jun 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

quic: add quic version counters in http3 codec stats. #16943

quic: add quic version counters in http3 codec stats. #16943

RenjieTang commented Jun 11, 2021

RenjieTang commented Jun 11, 2021

alyssawilk left a comment

alyssawilk Jun 14, 2021

DavidSchinazi Jun 14, 2021

alyssawilk Jun 14, 2021

RenjieTang Jun 14, 2021

alyssawilk Jun 21, 2021

alyssawilk Jun 14, 2021

alyssawilk Jun 14, 2021

alyssawilk Jun 14, 2021

RenjieTang Jun 14, 2021

DavidSchinazi left a comment

DavidSchinazi Jun 14, 2021

DavidSchinazi Jun 14, 2021

RenjieTang commented Jun 15, 2021

repokitteh-read-only bot commented Jun 15, 2021

quic: add quic version counters in http3 codec stats. #16943

quic: add quic version counters in http3 codec stats. #16943

Conversation

RenjieTang commented Jun 11, 2021

RenjieTang commented Jun 11, 2021

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

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

DavidSchinazi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RenjieTang commented Jun 15, 2021

repokitteh-read-only bot commented Jun 15, 2021