Skip to content

Commit d6fd5f6

Browse files
Fadi Hannameta-codesync[bot]
authored andcommitted
Add ODS counter for idle timeout connection terminations
Summary: When `ThriftServer::setIdleTimeout()` is configured, connections that remain idle for the specified duration are terminated via `RocketServerConnection::timeoutExpired()`. Currently there is no counter tracking these events. Adding one gives service owners visibility into idle timeout behavior, following the same pattern as `thrift.dropped_conns`, `thrift.rejected_conns`, and `thrift.accepted_connections`. ## Implementation ### 1. Add `connClosedByIdleTimeout()` to `TServerObserver` **File:** `fbcode/thrift/lib/cpp/server/TServerObserver.h` New virtual method after `connClosed`: ```cpp virtual void connClosedByIdleTimeout() {} ``` ### 2. Add timeseries field + override to `TServerCounters` **File:** `fbcode/common/fb303/cpp/TServerCounters.h` - New field `idleTimeoutConns_` (after `rejectedConns_`) - New override declaration `connClosedByIdleTimeout()` ### 3. Wire up timeseries creation + callback in `TServerCounters.cpp` **File:** `fbcode/common/fb303/cpp/TServerCounters.cpp` - In `setupCounters()`: `idleTimeoutConns_ = makeTimeseries(prefix + ".idle_timeout_conns", COUNT);` - New implementation of `connClosedByIdleTimeout()` that increments the timeseries ### 4. Add `onIdleTimeout()` callback to `RocketServerHandler` **File:** `fbcode/thrift/lib/cpp2/transport/rocket/server/RocketServerHandler.h` New virtual method after `connectionClosing()`: ```cpp virtual void onIdleTimeout() {} ``` ### 5. Override in `ThriftRocketServerHandler` **Files:** - `ThriftRocketServerHandler.h` — declaration: `void onIdleTimeout() final;` - `ThriftRocketServerHandler.cpp` — implementation calls `observer->connClosedByIdleTimeout()` ### 6. Call from `RocketServerConnection::timeoutExpired()` **File:** `fbcode/thrift/lib/cpp2/transport/rocket/server/RocketServerConnection.cpp` Added `frameHandler_->onIdleTimeout()` call before `closeWhenIdle()` in `timeoutExpired()`. ### 7. Update `FakeServerObserver` for testing **File:** `fbcode/thrift/lib/cpp2/transport/core/testutil/FakeServerObserver.h` - New atomic counter `connClosedByIdleTimeout_` - New override `connClosedByIdleTimeout()` that increments the counter ### 8. Add test verification **File:** `fbcode/thrift/lib/cpp2/test/server/ThriftServerTest.cpp` Updated `ConnectionIdleTimeoutTest` to attach a `FakeServerObserver`, verify the counter is 0 before idle timeout, and verify it increments to 1 after the connection is terminated. ## ODS Counter Name The counter will be published as `thrift.idle_timeout_conns` (with aggregation suffixes like `.count.60`, `.count.600`, `.count.3600`). ## Files Modified | File | Change | |------|--------| | `thrift/lib/cpp/server/TServerObserver.h` | New virtual method | | `common/fb303/cpp/TServerCounters.h` | New field + override decl | | `common/fb303/cpp/TServerCounters.cpp` | Create timeseries + impl | | `thrift/lib/cpp2/transport/rocket/server/RocketServerHandler.h` | New virtual method | | `thrift/lib/cpp2/transport/rocket/server/ThriftRocketServerHandler.h` | Override decl | | `thrift/lib/cpp2/transport/rocket/server/ThriftRocketServerHandler.cpp` | Override impl | | `thrift/lib/cpp2/transport/rocket/server/RocketServerConnection.cpp` | Call onIdleTimeout() | | `thrift/lib/cpp2/transport/core/testutil/FakeServerObserver.h` | New counter + override | | `thrift/lib/cpp2/test/server/ThriftServerTest.cpp` | Verify counter in test | | `thrift/lib/cpp2/test/BUCK` | Add mocks dep | Reviewed By: sazonovkirill Differential Revision: D95074970 fbshipit-source-id: e263c7b46be62b8e46aa7ec41e23451d19ef718a
1 parent 5fdaecf commit d6fd5f6

7 files changed

Lines changed: 25 additions & 0 deletions

File tree

third-party/thrift/src/thrift/lib/cpp/server/TServerObserver.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -159,6 +159,8 @@ class TServerObserver {
159159

160160
virtual void connClosed(const ConnectionInfo&) {}
161161

162+
virtual void connClosedByIdleTimeout() {}
163+
162164
virtual void activeConnections(int32_t /*numConnections*/) {}
163165

164166
virtual void tlsError() {}

third-party/thrift/src/thrift/lib/cpp2/test/server/ThriftServerTest.cpp

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,7 @@
8383
#include <thrift/lib/cpp2/test/server/ThriftServerTestUtils.h>
8484
#include <thrift/lib/cpp2/test/util/TestHandler.h>
8585
#include <thrift/lib/cpp2/test/util/TestThriftServerFactory.h>
86+
#include <thrift/lib/cpp2/transport/core/testutil/FakeServerObserver.h>
8687
#include <thrift/lib/cpp2/transport/http2/common/HTTP2RoutingHandler.h>
8788
#include <thrift/lib/cpp2/util/ScopedServerInterfaceThread.h>
8889
#include <thrift/lib/cpp2/util/ScopedServerThread.h>
@@ -2315,8 +2316,12 @@ TEST(ThriftServer, ConnectionIdleTimeoutTest) {
23152316
TestThriftServerFactory<TestHandler> factory;
23162317
auto server = factory.create();
23172318
server->setIdleTimeout(std::chrono::milliseconds(20));
2319+
auto observer = std::make_shared<FakeServerObserver>();
2320+
server->setObserver(observer);
23182321
apache::thrift::util::ScopedServerThread st(server);
23192322

2323+
EXPECT_EQ(observer->connClosedByIdleTimeout_, 0);
2324+
23202325
folly::EventBase base;
23212326
auto socket = folly::AsyncSocket::newSocket(&base, *st.getAddress());
23222327

@@ -2327,6 +2332,11 @@ TEST(ThriftServer, ConnectionIdleTimeoutTest) {
23272332
client.sync_sendResponse(response, 200);
23282333
EXPECT_EQ(response, "test200");
23292334
base.loop();
2335+
2336+
// Wait for idle timeout to fire and close the connection
2337+
/* sleep override */
2338+
std::this_thread::sleep_for(std::chrono::milliseconds(200));
2339+
EXPECT_EQ(observer->connClosedByIdleTimeout_, 1);
23302340
}
23312341

23322342
TEST(ThriftServer, BadSendTest) {

third-party/thrift/src/thrift/lib/cpp2/transport/core/testutil/FakeServerObserver.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ class FakeServerObserver : public apache::thrift::server::TServerObserver {
2929
std::atomic<size_t> connClosed_{0};
3030
std::atomic<size_t> connDropped_{0};
3131
std::atomic<size_t> connRejected_{0};
32+
std::atomic<size_t> connClosedByIdleTimeout_{0};
3233
std::atomic<size_t> activeConns_{0};
3334
std::atomic<size_t> taskKilled_{0};
3435
std::atomic<size_t> taskTimeout_{0};
@@ -63,6 +64,8 @@ class FakeServerObserver : public apache::thrift::server::TServerObserver {
6364

6465
void connRejected() override { ++connRejected_; }
6566

67+
void connClosedByIdleTimeout() override { ++connClosedByIdleTimeout_; }
68+
6669
void activeConnections(int32_t numConnections) override {
6770
activeConns_ = numConnections;
6871
}

third-party/thrift/src/thrift/lib/cpp2/transport/rocket/server/RocketServerConnection.cpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -988,6 +988,7 @@ void RocketServerConnection::timeoutExpired() noexcept {
988988
DestructorGuard dg(this);
989989

990990
if (!isBusy()) {
991+
frameHandler_->onIdleTimeout();
991992
closeWhenIdle();
992993
}
993994
}

third-party/thrift/src/thrift/lib/cpp2/transport/rocket/server/RocketServerHandler.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,8 @@ class RocketServerHandler {
6666

6767
virtual void connectionClosing() = 0;
6868

69+
virtual void onIdleTimeout() {}
70+
6971
virtual Cpp2ConnContext* getCpp2ConnContext() { return nullptr; }
7072

7173
virtual void onBeforeHandleFrame() {}

third-party/thrift/src/thrift/lib/cpp2/transport/rocket/server/ThriftRocketServerHandler.cpp

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -563,6 +563,12 @@ void ThriftRocketServerHandler::connectionClosing() {
563563
}
564564
}
565565

566+
void ThriftRocketServerHandler::onIdleTimeout() {
567+
if (auto* observer = worker_->getServer()->getObserver()) {
568+
observer->connClosedByIdleTimeout();
569+
}
570+
}
571+
566572
template <class F>
567573
void ThriftRocketServerHandler::handleRequestCommon(
568574
Payload&& payload,

third-party/thrift/src/thrift/lib/cpp2/transport/rocket/server/ThriftRocketServerHandler.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,7 @@ class ThriftRocketServerHandler : public RocketServerHandler {
9595
RocketServerFrameContext&& context,
9696
ChannelRequestCallbackFactory clientCallback) final;
9797
void connectionClosing() final;
98+
void onIdleTimeout() final;
9899

99100
apache::thrift::server::TServerObserver::SamplingStatus shouldSample(
100101
const transport::THeader& header);

0 commit comments

Comments
 (0)