From a3f2e082024c3510635bb2e7584c3e4ebcaade7a Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Tue, 24 Oct 2023 12:45:02 -0700 Subject: [PATCH] [http2] Fix framing_bytes stat (#34787) This stat is unused at present, so no need to protect it by an experiment. --- .../chttp2/transport/frame_window_update.cc | 2 +- .../transport/chttp2/transport/hpack_encoder.cc | 10 +++++----- test/core/end2end/tests/http2_stats.cc | 15 +++++++++++++-- 3 files changed, 19 insertions(+), 8 deletions(-) diff --git a/src/core/ext/transport/chttp2/transport/frame_window_update.cc b/src/core/ext/transport/chttp2/transport/frame_window_update.cc index 87dac6aaa92..45c122065f3 100644 --- a/src/core/ext/transport/chttp2/transport/frame_window_update.cc +++ b/src/core/ext/transport/chttp2/transport/frame_window_update.cc @@ -35,7 +35,7 @@ grpc_slice grpc_chttp2_window_update_create( uint32_t id, uint32_t window_delta, grpc_transport_one_way_stats* stats) { static const size_t frame_size = 13; grpc_slice slice = GRPC_SLICE_MALLOC(frame_size); - stats->header_bytes += frame_size; + stats->framing_bytes += frame_size; uint8_t* p = GRPC_SLICE_START_PTR(slice); GPR_ASSERT(window_delta); diff --git a/src/core/ext/transport/chttp2/transport/hpack_encoder.cc b/src/core/ext/transport/chttp2/transport/hpack_encoder.cc index 6c642815539..69f2c92dbc6 100644 --- a/src/core/ext/transport/chttp2/transport/hpack_encoder.cc +++ b/src/core/ext/transport/chttp2/transport/hpack_encoder.cc @@ -42,12 +42,12 @@ namespace grpc_core { namespace { -constexpr size_t kDataFrameHeaderSize = 9; +constexpr size_t kHeadersFrameHeaderSize = 9; } // namespace -// fills p (which is expected to be kDataFrameHeaderSize bytes long) -// with a data frame header +// fills p (which is expected to be kHeadersFrameHeaderSize bytes long) +// with a headers frame header static void FillHeader(uint8_t* p, uint8_t type, uint32_t id, size_t len, uint8_t flags) { // len is the current frame size (i.e. for the frame we're finishing). @@ -99,9 +99,9 @@ void HPackCompressor::Frame(const EncodeHeaderOptions& options, } else { len = options.max_frame_size; } - FillHeader(grpc_slice_buffer_tiny_add(output, kDataFrameHeaderSize), + FillHeader(grpc_slice_buffer_tiny_add(output, kHeadersFrameHeaderSize), frame_type, options.stream_id, len, flags); - options.stats->framing_bytes += kDataFrameHeaderSize; + options.stats->framing_bytes += kHeadersFrameHeaderSize; grpc_slice_buffer_move_first(raw.c_slice_buffer(), len, output); frame_type = GRPC_CHTTP2_FRAME_CONTINUATION; diff --git a/test/core/end2end/tests/http2_stats.cc b/test/core/end2end/tests/http2_stats.cc index d183f520627..ea84194b3d4 100644 --- a/test/core/end2end/tests/http2_stats.cc +++ b/test/core/end2end/tests/http2_stats.cc @@ -244,16 +244,27 @@ CORE_END2END_TEST(Http2FullstackSingleHopTest, StreamStats) { auto client_transport_stats = FakeCallTracer::FakeCallAttemptTracer::transport_stream_stats(); + auto server_transport_stats = FakeServerCallTracer::transport_stream_stats(); EXPECT_EQ(client_transport_stats.outgoing.data_bytes, send_from_client.size()); EXPECT_EQ(client_transport_stats.incoming.data_bytes, send_from_server.size()); - auto server_transport_stats = FakeServerCallTracer::transport_stream_stats(); EXPECT_EQ(server_transport_stats.outgoing.data_bytes, send_from_server.size()); EXPECT_EQ(server_transport_stats.incoming.data_bytes, send_from_client.size()); - // TODO(yashykt): Add tests for framing bytes as well + // At the very minimum, we should have 9 bytes from header frame, 9 bytes from + // data header frame and 5 bytes from the grpc header on data. The actual + // number might be more due to RST_STREAM (13 bytes) and WINDOW_UPDATE (13 + // bytes) frames. + EXPECT_GE(client_transport_stats.outgoing.framing_bytes, 23); + EXPECT_LE(client_transport_stats.outgoing.framing_bytes, 46); + EXPECT_GE(client_transport_stats.incoming.framing_bytes, 23); + EXPECT_LE(client_transport_stats.incoming.framing_bytes, 46); + EXPECT_GE(server_transport_stats.outgoing.framing_bytes, 23); + EXPECT_LE(server_transport_stats.outgoing.framing_bytes, 46); + EXPECT_GE(server_transport_stats.incoming.framing_bytes, 23); + EXPECT_LE(server_transport_stats.incoming.framing_bytes, 46); delete ServerCallTracerFactory::Get(ChannelArgs()); ServerCallTracerFactory::RegisterGlobal(nullptr);