Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Jun 22, 2021
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e788dd0
add quic version counters in http3 codec stats.
RenjieTang Jun 11, 2021
951e467
add comprehensive test coverage and address comments.
RenjieTang Jun 14, 2021
7871e9d
add notes to stats doc.
RenjieTang Jun 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions docs/root/configuration/http/http_conn_man/stats.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Add all the current version here, and remove stats as we deprecate versions
  2. Add all the current version here, and have old version stats at 0 forever
  3. 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

quic_version_46, Counter, Total number of quic connections that use transport version 46.
quic_version_50, Counter, Total number of quic connections that use transport version 50.
quic_version_51, Counter, Total number of quic connections that use transport version 51.
quic_version_h3_29, Counter, Total number of quic connections that use transport version h3-29.
quic_version_rfc_v1, Counter, Total number of quic connections that use transport version rfc-v1.


Tracing statistics
Expand Down
8 changes: 7 additions & 1 deletion source/common/http/http3/codec_stats.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,13 @@ namespace Http3 {
COUNTER(requests_rejected_with_underscores_in_headers) \
COUNTER(rx_reset) \
COUNTER(tx_reset) \
COUNTER(metadata_not_supported_error)
COUNTER(metadata_not_supported_error) \
COUNTER(quic_version_43) \
COUNTER(quic_version_46) \
COUNTER(quic_version_50) \
COUNTER(quic_version_51) \
COUNTER(quic_version_h3_29) \
COUNTER(quic_version_rfc_v1)

/**
* Wrapper struct for the HTTP/3 codec stats. @see stats_macros.h
Expand Down
2 changes: 1 addition & 1 deletion source/common/quic/envoy_quic_client_session.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ void EnvoyQuicClientSession::connect() {
void EnvoyQuicClientSession::OnConnectionClosed(const quic::QuicConnectionCloseFrame& frame,
quic::ConnectionCloseSource source) {
quic::QuicSpdyClientSession::OnConnectionClosed(frame, source);
onConnectionCloseEvent(frame, source);
onConnectionCloseEvent(frame, source, connection()->transport_version());
}

void EnvoyQuicClientSession::Initialize() {
Expand Down
2 changes: 1 addition & 1 deletion source/common/quic/envoy_quic_server_session.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ void EnvoyQuicServerSession::setUpRequestDecoder(EnvoyQuicServerStream& stream)
void EnvoyQuicServerSession::OnConnectionClosed(const quic::QuicConnectionCloseFrame& frame,
quic::ConnectionCloseSource source) {
quic::QuicServerSessionBase::OnConnectionClosed(frame, source);
onConnectionCloseEvent(frame, source);
onConnectionCloseEvent(frame, source, connection()->transport_version());
}

void EnvoyQuicServerSession::Initialize() {
Expand Down
31 changes: 30 additions & 1 deletion source/common/quic/quic_filter_manager_connection_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

transport_failure_reason_ = absl::StrCat(quic::QuicErrorCodeToString(frame.quic_error_code),
" with details: ", frame.error_details);
if (network_connection_ != nullptr) {
Expand All @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return;
}
switch (version) {
case quic::QUIC_VERSION_43:
codec_stats_->quic_version_43_.inc();
return;
case quic::QUIC_VERSION_46:
codec_stats_->quic_version_46_.inc();
return;
case quic::QUIC_VERSION_50:
codec_stats_->quic_version_50_.inc();
return;
case quic::QUIC_VERSION_51:
std::cout << "logging" << std::endl;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clean up?

codec_stats_->quic_version_51_.inc();
return;
case quic::QUIC_VERSION_IETF_DRAFT_29:
codec_stats_->quic_version_h3_29_.inc();
return;
case quic::QUIC_VERSION_IETF_RFC_V1:
codec_stats_->quic_version_rfc_v1_.inc();
return;
default:
return;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}

void QuicFilterManagerConnectionImpl::closeConnectionImmediately() {
Expand Down
15 changes: 6 additions & 9 deletions source/common/quic/quic_filter_manager_connection_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,13 +137,10 @@ class QuicFilterManagerConnectionImpl : public Network::ConnectionImplBase,
uint32_t bytesToSend() { return bytes_to_send_; }

void setHttp3Options(const envoy::config::core::v3::Http3ProtocolOptions& http3_options) {
http3_options_ =
std::reference_wrapper<const envoy::config::core::v3::Http3ProtocolOptions>(http3_options);
http3_options_ = http3_options;
}

void setCodecStats(Http::Http3::CodecStats& stats) {
codec_stats_ = std::reference_wrapper<Http::Http3::CodecStats>(stats);
}
void setCodecStats(Http::Http3::CodecStats& stats) { codec_stats_ = stats; }

uint32_t maxIncomingHeadersCount() { return max_headers_count_; }

Expand All @@ -154,7 +151,8 @@ class QuicFilterManagerConnectionImpl : public Network::ConnectionImplBase,
protected:
// Propagate connection close to network_connection_callbacks_.
void onConnectionCloseEvent(const quic::QuicConnectionCloseFrame& frame,
quic::ConnectionCloseSource source);
quic::ConnectionCloseSource source,
quic::QuicTransportVersion version);

void closeConnectionImmediately() override;

Expand All @@ -166,9 +164,8 @@ class QuicFilterManagerConnectionImpl : public Network::ConnectionImplBase,

QuicNetworkConnection* network_connection_{nullptr};

absl::optional<std::reference_wrapper<Http::Http3::CodecStats>> codec_stats_;
absl::optional<std::reference_wrapper<const envoy::config::core::v3::Http3ProtocolOptions>>
http3_options_;
OptRef<Http::Http3::CodecStats> codec_stats_;
OptRef<const envoy::config::core::v3::Http3ProtocolOptions> http3_options_;
bool initialized_{false};

private:
Expand Down
7 changes: 7 additions & 0 deletions test/integration/quic_http_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,13 @@ TEST_P(QuicHttpIntegrationTest, ZeroRtt) {
"error_code_QUIC_NO_ERROR",
2u);
}
if (GetParam().second == QuicVersionType::GquicQuicCrypto) {
test_server_->waitForCounterEq("http3.quic_version_50", 2u);
} else if (GetParam().second == QuicVersionType::GquicTls) {
test_server_->waitForCounterEq("http3.quic_version_51", 2u);
} else {
test_server_->waitForCounterEq("http3.quic_version_rfc_v1", 2u);
}
}

// Ensure multiple quic connections work, regardless of platform BPF support
Expand Down