From 9993408735a7390e1c8d07eda583f892fc7c02f5 Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Mon, 1 Jul 2024 18:29:05 -0700 Subject: [PATCH] [experiment] Delete stable experiments http2_stats_fix, keepalive_fix and keepalive_server_fix (#37076) Closes #37076 COPYBARA_INTEGRATE_REVIEW=https://github.com/grpc/grpc/pull/37076 from yashykt:DeleteExperiments e0f37a79f4f7fb26fbd9840976d19638ef53f2fa PiperOrigin-RevId: 648547673 --- bazel/experiments.bzl | 3 -- .../chttp2/transport/chttp2_transport.cc | 16 ++---- .../transport/chttp2/transport/frame_data.cc | 3 -- src/core/lib/experiments/experiments.cc | 51 ------------------- src/core/lib/experiments/experiments.h | 27 ---------- src/core/lib/experiments/experiments.yaml | 22 -------- src/core/lib/experiments/rollouts.yaml | 6 --- test/core/end2end/tests/http2_stats.cc | 3 -- .../chttp2/ping_configuration_test.cc | 2 - 9 files changed, 5 insertions(+), 128 deletions(-) diff --git a/bazel/experiments.bzl b/bazel/experiments.bzl index 8774185732a..09056684ac5 100644 --- a/bazel/experiments.bzl +++ b/bazel/experiments.bzl @@ -25,9 +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", "max_pings_wo_data_throttle": "max_pings_wo_data_throttle", "monitoring_experiment": "monitoring_experiment", "multiping": "multiping", diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc index a819179421e..e1ef1d46e98 100644 --- a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc +++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc @@ -456,15 +456,11 @@ static void read_channel_args(grpc_chttp2_transport* t, if (t->is_client) { t->keepalive_permit_without_calls = channel_args.GetBool(GRPC_ARG_KEEPALIVE_PERMIT_WITHOUT_CALLS) - .value_or(grpc_core::IsKeepaliveFixEnabled() - ? g_default_client_keepalive_permit_without_calls - : false); + .value_or(g_default_client_keepalive_permit_without_calls); } else { t->keepalive_permit_without_calls = channel_args.GetBool(GRPC_ARG_KEEPALIVE_PERMIT_WITHOUT_CALLS) - .value_or(grpc_core::IsKeepaliveServerFixEnabled() - ? g_default_server_keepalive_permit_without_calls - : false); + .value_or(g_default_server_keepalive_permit_without_calls); } t->settings_timeout = @@ -1468,11 +1464,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 3476ab12481..9ef84e8d475 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 e100621f1d0..7eb9a20c504 100644 --- a/src/core/lib/experiments/experiments.cc +++ b/src/core/lib/experiments/experiments.cc @@ -47,17 +47,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."; -const char* const additional_constraints_keepalive_fix = "{}"; -const char* const description_keepalive_server_fix = - "Allows overriding keepalive_permit_without_calls for servers. Refer " - "https://github.com/grpc/grpc/pull/33917 for more information."; -const char* const additional_constraints_keepalive_server_fix = "{}"; const char* const description_max_pings_wo_data_throttle = "Experiment to throttle pings to a period of 1 min when " "GRPC_ARG_HTTP2_MAX_PINGS_WITHOUT_DATA limit has reached (instead of " @@ -138,12 +127,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, - additional_constraints_keepalive_server_fix, nullptr, 0, false, false}, {"max_pings_wo_data_throttle", description_max_pings_wo_data_throttle, additional_constraints_max_pings_wo_data_throttle, nullptr, 0, false, true}, @@ -214,17 +197,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."; -const char* const additional_constraints_keepalive_fix = "{}"; -const char* const description_keepalive_server_fix = - "Allows overriding keepalive_permit_without_calls for servers. Refer " - "https://github.com/grpc/grpc/pull/33917 for more information."; -const char* const additional_constraints_keepalive_server_fix = "{}"; const char* const description_max_pings_wo_data_throttle = "Experiment to throttle pings to a period of 1 min when " "GRPC_ARG_HTTP2_MAX_PINGS_WITHOUT_DATA limit has reached (instead of " @@ -305,12 +277,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, - additional_constraints_keepalive_server_fix, nullptr, 0, false, false}, {"max_pings_wo_data_throttle", description_max_pings_wo_data_throttle, additional_constraints_max_pings_wo_data_throttle, nullptr, 0, false, true}, @@ -381,17 +347,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."; -const char* const additional_constraints_keepalive_fix = "{}"; -const char* const description_keepalive_server_fix = - "Allows overriding keepalive_permit_without_calls for servers. Refer " - "https://github.com/grpc/grpc/pull/33917 for more information."; -const char* const additional_constraints_keepalive_server_fix = "{}"; const char* const description_max_pings_wo_data_throttle = "Experiment to throttle pings to a period of 1 min when " "GRPC_ARG_HTTP2_MAX_PINGS_WITHOUT_DATA limit has reached (instead of " @@ -472,12 +427,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, - additional_constraints_keepalive_server_fix, nullptr, 0, false, false}, {"max_pings_wo_data_throttle", description_max_pings_wo_data_throttle, additional_constraints_max_pings_wo_data_throttle, nullptr, 0, false, true}, diff --git a/src/core/lib/experiments/experiments.h b/src/core/lib/experiments/experiments.h index a1b2662ed4d..88c396e64c1 100644 --- a/src/core/lib/experiments/experiments.h +++ b/src/core/lib/experiments/experiments.h @@ -66,10 +66,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; } inline bool IsMaxPingsWoDataThrottleEnabled() { return false; } #define GRPC_EXPERIMENT_IS_INCLUDED_MONITORING_EXPERIMENT inline bool IsMonitoringExperimentEnabled() { return true; } @@ -103,10 +99,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; } inline bool IsMaxPingsWoDataThrottleEnabled() { return false; } #define GRPC_EXPERIMENT_IS_INCLUDED_MONITORING_EXPERIMENT inline bool IsMonitoringExperimentEnabled() { return true; } @@ -139,10 +131,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; } inline bool IsMaxPingsWoDataThrottleEnabled() { return false; } #define GRPC_EXPERIMENT_IS_INCLUDED_MONITORING_EXPERIMENT inline bool IsMonitoringExperimentEnabled() { return true; } @@ -175,9 +163,6 @@ enum ExperimentIds { kExperimentIdEventEngineDns, kExperimentIdEventEngineListener, kExperimentIdFreeLargeAllocator, - kExperimentIdHttp2StatsFix, - kExperimentIdKeepaliveFix, - kExperimentIdKeepaliveServerFix, kExperimentIdMaxPingsWoDataThrottle, kExperimentIdMonitoringExperiment, kExperimentIdMultiping, @@ -227,18 +212,6 @@ inline bool IsEventEngineListenerEnabled() { inline bool IsFreeLargeAllocatorEnabled() { return IsExperimentEnabled(); } -#define GRPC_EXPERIMENT_IS_INCLUDED_HTTP2_STATS_FIX -inline bool IsHttp2StatsFixEnabled() { - return IsExperimentEnabled(); -} -#define GRPC_EXPERIMENT_IS_INCLUDED_KEEPALIVE_FIX -inline bool IsKeepaliveFixEnabled() { - return IsExperimentEnabled(); -} -#define GRPC_EXPERIMENT_IS_INCLUDED_KEEPALIVE_SERVER_FIX -inline bool IsKeepaliveServerFixEnabled() { - return IsExperimentEnabled(); -} #define GRPC_EXPERIMENT_IS_INCLUDED_MAX_PINGS_WO_DATA_THROTTLE inline bool IsMaxPingsWoDataThrottleEnabled() { return IsExperimentEnabled(); diff --git a/src/core/lib/experiments/experiments.yaml b/src/core/lib/experiments/experiments.yaml index f7417410509..ffed3b48294 100644 --- a/src/core/lib/experiments/experiments.yaml +++ b/src/core/lib/experiments/experiments.yaml @@ -91,28 +91,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/09/30 - owner: yashkt@google.com - test_tags: [] -- name: keepalive_fix - description: - Allows overriding keepalive_permit_without_calls. - Refer https://github.com/grpc/grpc/pull/33428 for more information. - expiry: 2024/06/30 - owner: yashkt@google.com - test_tags: [] - allow_in_fuzzing_config: false -- name: keepalive_server_fix - description: - Allows overriding keepalive_permit_without_calls for servers. - Refer https://github.com/grpc/grpc/pull/33917 for more information. - expiry: 2024/12/31 - owner: yashkt@google.com - test_tags: [] - allow_in_fuzzing_config: false - name: max_pings_wo_data_throttle description: Experiment to throttle pings to a period of 1 min when diff --git a/src/core/lib/experiments/rollouts.yaml b/src/core/lib/experiments/rollouts.yaml index a0f25b5dc91..0566ffbab7e 100644 --- a/src/core/lib/experiments/rollouts.yaml +++ b/src/core/lib/experiments/rollouts.yaml @@ -73,12 +73,6 @@ windows: true - name: free_large_allocator default: false -- name: http2_stats_fix - default: true -- name: keepalive_fix - default: false -- name: keepalive_server_fix - default: false - name: max_pings_wo_data_throttle default: false - name: monitoring_experiment diff --git a/test/core/end2end/tests/http2_stats.cc b/test/core/end2end/tests/http2_stats.cc index dfd1e029e01..b80f979b220 100644 --- a/test/core/end2end/tests/http2_stats.cc +++ b/test/core/end2end/tests/http2_stats.cc @@ -192,9 +192,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 CoreEnd2endTest::TestNotification(this); g_server_call_ended_notify = new CoreEnd2endTest::TestNotification(this); diff --git a/test/core/transport/chttp2/ping_configuration_test.cc b/test/core/transport/chttp2/ping_configuration_test.cc index 7524f4872ad..705276ff38e 100644 --- a/test/core/transport/chttp2/ping_configuration_test.cc +++ b/test/core/transport/chttp2/ping_configuration_test.cc @@ -198,8 +198,6 @@ TEST_F(ConfigurationTest, ModifyServerDefaults) { int main(int argc, char** argv) { ::testing::InitGoogleTest(&argc, argv); grpc::testing::TestEnvironment env(&argc, argv); - grpc_core::ForceEnableExperiment("keepalive_fix", true); - grpc_core::ForceEnableExperiment("keepalive_server_fix", true); grpc_init(); auto ret = RUN_ALL_TESTS(); grpc_shutdown();