From 84ee28e6956ed7cd51462aad52e64782dd5ca34b Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Fri, 12 Apr 2024 17:02:20 -0700 Subject: [PATCH] [experiments] Stabilize http2_stats_fix (#36351) Closes #36351 COPYBARA_INTEGRATE_REVIEW=https://github.com/grpc/grpc/pull/36351 from yashykt:RemoveHttp2StatsFix 768a7a4de486c0d20cf835c447552af02eedcf6f PiperOrigin-RevId: 624332015 --- bazel/experiments.bzl | 1 - .../chttp2/transport/chttp2_transport.cc | 8 +++----- .../ext/transport/chttp2/transport/frame_data.cc | 3 --- src/core/lib/experiments/experiments.cc | 15 --------------- src/core/lib/experiments/experiments.h | 11 ----------- src/core/lib/experiments/experiments.yaml | 6 ------ test/core/end2end/tests/http2_stats.cc | 3 --- 7 files changed, 3 insertions(+), 44 deletions(-) diff --git a/bazel/experiments.bzl b/bazel/experiments.bzl index e366840a1e6..0e5f00b71eb 100644 --- a/bazel/experiments.bzl +++ b/bazel/experiments.bzl @@ -25,7 +25,6 @@ EXPERIMENT_ENABLES = { "event_engine_dns": "event_engine_dns", "event_engine_listener": "event_engine_listener", "free_large_allocator": "free_large_allocator", - "http2_stats_fix": "http2_stats_fix", "keepalive_fix": "keepalive_fix", "keepalive_server_fix": "keepalive_server_fix", "monitoring_experiment": "monitoring_experiment", diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc index a1e490e44b6..6be0152cae6 100644 --- a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc +++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc @@ -1471,11 +1471,9 @@ static void perform_stream_op_locked(void* stream_op, frame_hdr[3] = static_cast(len >> 8); frame_hdr[4] = static_cast(len); - if (grpc_core::IsHttp2StatsFixEnabled()) { - s->stats.outgoing.framing_bytes += GRPC_HEADER_SIZE_IN_BYTES; - s->stats.outgoing.data_bytes += - op_payload->send_message.send_message->Length(); - } + s->stats.outgoing.framing_bytes += GRPC_HEADER_SIZE_IN_BYTES; + s->stats.outgoing.data_bytes += + op_payload->send_message.send_message->Length(); s->next_message_end_offset = s->flow_controlled_bytes_written + static_cast(s->flow_controlled_buffer.length) + diff --git a/src/core/ext/transport/chttp2/transport/frame_data.cc b/src/core/ext/transport/chttp2/transport/frame_data.cc index e10dea34134..334eff52f22 100644 --- a/src/core/ext/transport/chttp2/transport/frame_data.cc +++ b/src/core/ext/transport/chttp2/transport/frame_data.cc @@ -78,9 +78,6 @@ void grpc_chttp2_encode_data(uint32_t id, grpc_slice_buffer* inbuf, grpc_slice_buffer_move_first_no_ref(inbuf, write_bytes, outbuf); stats->framing_bytes += header_size; - if (!grpc_core::IsHttp2StatsFixEnabled()) { - stats->data_bytes += write_bytes; - } } grpc_core::Poll grpc_deframe_unprocessed_incoming_frames( diff --git a/src/core/lib/experiments/experiments.cc b/src/core/lib/experiments/experiments.cc index 1c7a2cb7c95..9eee1458bec 100644 --- a/src/core/lib/experiments/experiments.cc +++ b/src/core/lib/experiments/experiments.cc @@ -48,9 +48,6 @@ const char* const additional_constraints_event_engine_listener = "{}"; const char* const description_free_large_allocator = "If set, return all free bytes from a \042big\042 allocator"; const char* const additional_constraints_free_large_allocator = "{}"; -const char* const description_http2_stats_fix = - "Fix on HTTP2 outgoing data stats reporting"; -const char* const additional_constraints_http2_stats_fix = "{}"; const char* const description_keepalive_fix = "Allows overriding keepalive_permit_without_calls. Refer " "https://github.com/grpc/grpc/pull/33428 for more information."; @@ -170,8 +167,6 @@ const ExperimentMetadata g_experiment_metadata[] = { additional_constraints_event_engine_listener, nullptr, 0, false, true}, {"free_large_allocator", description_free_large_allocator, additional_constraints_free_large_allocator, nullptr, 0, false, true}, - {"http2_stats_fix", description_http2_stats_fix, - additional_constraints_http2_stats_fix, nullptr, 0, true, true}, {"keepalive_fix", description_keepalive_fix, additional_constraints_keepalive_fix, nullptr, 0, false, false}, {"keepalive_server_fix", description_keepalive_server_fix, @@ -253,9 +248,6 @@ const char* const additional_constraints_event_engine_listener = "{}"; const char* const description_free_large_allocator = "If set, return all free bytes from a \042big\042 allocator"; const char* const additional_constraints_free_large_allocator = "{}"; -const char* const description_http2_stats_fix = - "Fix on HTTP2 outgoing data stats reporting"; -const char* const additional_constraints_http2_stats_fix = "{}"; const char* const description_keepalive_fix = "Allows overriding keepalive_permit_without_calls. Refer " "https://github.com/grpc/grpc/pull/33428 for more information."; @@ -375,8 +367,6 @@ const ExperimentMetadata g_experiment_metadata[] = { additional_constraints_event_engine_listener, nullptr, 0, true, true}, {"free_large_allocator", description_free_large_allocator, additional_constraints_free_large_allocator, nullptr, 0, false, true}, - {"http2_stats_fix", description_http2_stats_fix, - additional_constraints_http2_stats_fix, nullptr, 0, true, true}, {"keepalive_fix", description_keepalive_fix, additional_constraints_keepalive_fix, nullptr, 0, false, false}, {"keepalive_server_fix", description_keepalive_server_fix, @@ -458,9 +448,6 @@ const char* const additional_constraints_event_engine_listener = "{}"; const char* const description_free_large_allocator = "If set, return all free bytes from a \042big\042 allocator"; const char* const additional_constraints_free_large_allocator = "{}"; -const char* const description_http2_stats_fix = - "Fix on HTTP2 outgoing data stats reporting"; -const char* const additional_constraints_http2_stats_fix = "{}"; const char* const description_keepalive_fix = "Allows overriding keepalive_permit_without_calls. Refer " "https://github.com/grpc/grpc/pull/33428 for more information."; @@ -580,8 +567,6 @@ const ExperimentMetadata g_experiment_metadata[] = { additional_constraints_event_engine_listener, nullptr, 0, true, true}, {"free_large_allocator", description_free_large_allocator, additional_constraints_free_large_allocator, nullptr, 0, false, true}, - {"http2_stats_fix", description_http2_stats_fix, - additional_constraints_http2_stats_fix, nullptr, 0, true, true}, {"keepalive_fix", description_keepalive_fix, additional_constraints_keepalive_fix, nullptr, 0, false, false}, {"keepalive_server_fix", description_keepalive_server_fix, diff --git a/src/core/lib/experiments/experiments.h b/src/core/lib/experiments/experiments.h index 27e9fb3c92d..a48a97d63dc 100644 --- a/src/core/lib/experiments/experiments.h +++ b/src/core/lib/experiments/experiments.h @@ -74,8 +74,6 @@ inline bool IsEventEngineClientEnabled() { return false; } inline bool IsEventEngineDnsEnabled() { return false; } inline bool IsEventEngineListenerEnabled() { return false; } inline bool IsFreeLargeAllocatorEnabled() { return false; } -#define GRPC_EXPERIMENT_IS_INCLUDED_HTTP2_STATS_FIX -inline bool IsHttp2StatsFixEnabled() { return true; } inline bool IsKeepaliveFixEnabled() { return false; } inline bool IsKeepaliveServerFixEnabled() { return false; } #define GRPC_EXPERIMENT_IS_INCLUDED_MONITORING_EXPERIMENT @@ -120,8 +118,6 @@ inline bool IsEventEngineDnsEnabled() { return false; } #define GRPC_EXPERIMENT_IS_INCLUDED_EVENT_ENGINE_LISTENER inline bool IsEventEngineListenerEnabled() { return true; } inline bool IsFreeLargeAllocatorEnabled() { return false; } -#define GRPC_EXPERIMENT_IS_INCLUDED_HTTP2_STATS_FIX -inline bool IsHttp2StatsFixEnabled() { return true; } inline bool IsKeepaliveFixEnabled() { return false; } inline bool IsKeepaliveServerFixEnabled() { return false; } #define GRPC_EXPERIMENT_IS_INCLUDED_MONITORING_EXPERIMENT @@ -167,8 +163,6 @@ inline bool IsEventEngineDnsEnabled() { return true; } #define GRPC_EXPERIMENT_IS_INCLUDED_EVENT_ENGINE_LISTENER inline bool IsEventEngineListenerEnabled() { return true; } inline bool IsFreeLargeAllocatorEnabled() { return false; } -#define GRPC_EXPERIMENT_IS_INCLUDED_HTTP2_STATS_FIX -inline bool IsHttp2StatsFixEnabled() { return true; } inline bool IsKeepaliveFixEnabled() { return false; } inline bool IsKeepaliveServerFixEnabled() { return false; } #define GRPC_EXPERIMENT_IS_INCLUDED_MONITORING_EXPERIMENT @@ -206,7 +200,6 @@ enum ExperimentIds { kExperimentIdEventEngineDns, kExperimentIdEventEngineListener, kExperimentIdFreeLargeAllocator, - kExperimentIdHttp2StatsFix, kExperimentIdKeepaliveFix, kExperimentIdKeepaliveServerFix, kExperimentIdMonitoringExperiment, @@ -261,10 +254,6 @@ inline bool IsEventEngineListenerEnabled() { inline bool IsFreeLargeAllocatorEnabled() { return IsExperimentEnabled(kExperimentIdFreeLargeAllocator); } -#define GRPC_EXPERIMENT_IS_INCLUDED_HTTP2_STATS_FIX -inline bool IsHttp2StatsFixEnabled() { - return IsExperimentEnabled(kExperimentIdHttp2StatsFix); -} #define GRPC_EXPERIMENT_IS_INCLUDED_KEEPALIVE_FIX inline bool IsKeepaliveFixEnabled() { return IsExperimentEnabled(kExperimentIdKeepaliveFix); diff --git a/src/core/lib/experiments/experiments.yaml b/src/core/lib/experiments/experiments.yaml index 281e8129b39..9ab849a823b 100644 --- a/src/core/lib/experiments/experiments.yaml +++ b/src/core/lib/experiments/experiments.yaml @@ -98,12 +98,6 @@ expiry: 2024/08/01 owner: alishananda@google.com test_tags: [resource_quota_test] -- name: http2_stats_fix - description: - Fix on HTTP2 outgoing data stats reporting - expiry: 2024/03/31 - owner: yashkt@google.com - test_tags: [] - name: keepalive_fix description: Allows overriding keepalive_permit_without_calls. diff --git a/test/core/end2end/tests/http2_stats.cc b/test/core/end2end/tests/http2_stats.cc index 5e497f7b8f2..b12d7a431a0 100644 --- a/test/core/end2end/tests/http2_stats.cc +++ b/test/core/end2end/tests/http2_stats.cc @@ -193,9 +193,6 @@ class NewFakeStatsPlugin : public FakeStatsPlugin { // This test verifies the HTTP2 stats on a stream CORE_END2END_TEST(Http2FullstackSingleHopTest, StreamStats) { - if (!IsHttp2StatsFixEnabled()) { - GTEST_SKIP() << "Test needs http2_stats_fix experiment to be enabled"; - } g_mu = new Mutex(); g_client_call_ended_notify = new Notification(); g_server_call_ended_notify = new Notification();