-
Notifications
You must be signed in to change notification settings - Fork 92
Adaptive Load library for calling Nighthawk Service #493
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.
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
Changes from 49 commits
8ea442d
5ac755a
b8c25a5
1c19c68
7050686
0776563
16fd8f6
c383010
6e1a483
4ef1140
4111bf4
871a959
1fd77c1
edc36b2
4d0364e
aed6d94
d9ae87d
a05a6f5
8cd4d21
d814a96
5f5a885
7e20a78
9048267
306c0ec
d33f543
442cca9
677b783
cefb366
f3684df
5463051
46e0e25
f634642
3c39faa
b9c8f2b
5fc4db4
64e7852
12807f1
e8e960f
94fbcbe
6142700
afbc049
48e7121
de747eb
0b76542
fd8d03d
7adbabb
6306b4e
1ece783
4616e6f
70705e9
e576bc1
1fca528
cd6f6da
8d642bb
b1284f5
5cf83e4
24cf230
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| #pragma once | ||
| #include "envoy/common/pure.h" | ||
|
|
||
| #include "external/envoy/source/common/common/statusor.h" | ||
| #include "external/envoy/source/common/protobuf/protobuf.h" | ||
|
|
||
| #include "api/client/options.pb.h" | ||
| #include "api/client/service.grpc.pb.h" | ||
|
|
||
| namespace Nighthawk { | ||
|
|
||
| /** | ||
| * An interface for interacting with a Nighthawk Service gRPC stub. | ||
| */ | ||
| class NighthawkServiceClient { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Curious what you think of this train of thought: At first I was going to ask if you could rename this to be Adaptive Load specific, since NighthawkServiceClient is the kind of name that could be used by a future class in Nighthawk, for creating a generic client for the NighthawkService. Then I looked at the code, and I don't think there's anything adaptive-load specific about this class or the impl
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right, moved it to |
||
| public: | ||
| virtual ~NighthawkServiceClient() = default; | ||
|
|
||
| /** | ||
| * Runs a single benchmark using a Nighthawk Service. | ||
| * | ||
| * @param nighthawk_service_stub Nighthawk Service gRPC stub. | ||
| * @param command_line_options Nighthawk Service benchmark request proto generated by the | ||
| * StepController, without the duration set. | ||
| * @param duration Duration to insert into the benchmark request. | ||
| * | ||
| * @return StatusOr<ExecutionResponse> If we reached the Nighthawk Service, this is the raw | ||
| * ExecutionResponse proto, containing the benchmark data or possibly an error message from | ||
| * Nighthawk Service; if we had trouble communicating with the Nighthawk Service, we return an | ||
| * error status. | ||
| */ | ||
| virtual absl::StatusOr<nighthawk::client::ExecutionResponse> PerformNighthawkBenchmark( | ||
| nighthawk::client::NighthawkService::StubInterface* nighthawk_service_stub, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not a request for change, but I wanted to hear your rationale: why have this as an argument on the function, rather than an argument to the class constructor? It's interesting to me that neither this nor the impl version seem to hold any state at all, which leads to a question of why they need to be classes at all.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This used to be a bare function because it had no need to track state across calls. But in order to mock out this functionality for testing, with gtest it's much simpler to make a library export an interface rather than plain functions. Leaving it stateless seems simpler than storing the stub and taking on responsibility for its lifetime. The intent here is definitely not to introduce a new layer into the system architecture, only to wrap a stateless helper function in a class for unit test purposes.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense. Thanks for explaining. |
||
| const nighthawk::client::CommandLineOptions& command_line_options, | ||
| const Envoy::Protobuf::Duration& duration) PURE; | ||
| }; | ||
|
|
||
| } // namespace Nighthawk | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| #include "adaptive_load/nighthawk_service_client_impl.h" | ||
|
|
||
| #include "external/envoy/source/common/common/assert.h" | ||
|
|
||
| namespace Nighthawk { | ||
|
|
||
| absl::StatusOr<nighthawk::client::ExecutionResponse> | ||
| NighthawkServiceClientImpl::PerformNighthawkBenchmark( | ||
| nighthawk::client::NighthawkService::StubInterface* nighthawk_service_stub, | ||
| const nighthawk::client::CommandLineOptions& command_line_options, | ||
| const Envoy::Protobuf::Duration& duration) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems like an odd api to me. Accepting a const command_line_options reference and then immediately copying it into a new object feels counterproductive. There also doesn't seem to me to be a good reason to have duration as its own parameter as opposed to set on the command_line_options here, since the first and only thing you do with duration is set it in options. Could you accept a const reference to command line options, with the duration properly included, and then use that in ExecutionRequest?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This API has always bothered me a bit. It made more sense when it was a private helper and I was trying to save lines at the call sites. Changing this as you suggest. |
||
| nighthawk::client::CommandLineOptions options = command_line_options; | ||
| *options.mutable_duration() = duration; | ||
|
|
||
| nighthawk::client::ExecutionRequest request; | ||
| nighthawk::client::ExecutionResponse response; | ||
| *request.mutable_start_request()->mutable_options() = options; | ||
|
|
||
| ::grpc::ClientContext context; | ||
| std::shared_ptr<::grpc::ClientReaderWriterInterface<nighthawk::client::ExecutionRequest, | ||
| nighthawk::client::ExecutionResponse>> | ||
| stream(nighthawk_service_stub->ExecutionStream(&context)); | ||
|
|
||
| if (!stream->Write(request)) { | ||
| return absl::UnknownError("Failed to write request to the Nighthawk Service gRPC channel."); | ||
| } else if (!stream->WritesDone()) { | ||
| return absl::UnknownError("WritesDone() failed on the Nighthawk Service gRPC channel."); | ||
| } | ||
|
|
||
| bool got_response = false; | ||
| while (stream->Read(&response)) { | ||
| RELEASE_ASSERT(!got_response, | ||
| "Nighthawk Service has started responding with more than one message."); | ||
| got_response = true; | ||
| } | ||
| if (!got_response) { | ||
| return absl::UnknownError("Nighthawk Service did not send a gRPC response."); | ||
| } | ||
| ::grpc::Status status = stream->Finish(); | ||
| if (!status.ok()) { | ||
| return absl::Status(static_cast<absl::StatusCode>(status.error_code()), status.error_message()); | ||
| } | ||
| return response; | ||
| } | ||
|
|
||
| } // namespace Nighthawk | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| #include "nighthawk/adaptive_load/nighthawk_service_client.h" | ||
|
|
||
| #include "external/envoy/source/common/common/statusor.h" | ||
| #include "external/envoy/source/common/protobuf/protobuf.h" | ||
|
|
||
| #include "api/client/options.pb.h" | ||
| #include "api/client/service.grpc.pb.h" | ||
|
|
||
| namespace Nighthawk { | ||
|
|
||
| class NighthawkServiceClientImpl : public NighthawkServiceClient { | ||
| public: | ||
| absl::StatusOr<nighthawk::client::ExecutionResponse> PerformNighthawkBenchmark( | ||
| nighthawk::client::NighthawkService::StubInterface* nighthawk_service_stub, | ||
| const nighthawk::client::CommandLineOptions& command_line_options, | ||
| const Envoy::Protobuf::Duration& duration) override; | ||
| }; | ||
|
|
||
| } // namespace Nighthawk |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,177 @@ | ||
| #include "external/envoy/source/common/protobuf/protobuf.h" | ||
|
|
||
| #include "api/client/options.pb.h" | ||
| #include "api/client/service.grpc.pb.h" | ||
| #include "api/client/service_mock.grpc.pb.h" | ||
|
|
||
| #include "grpcpp/test/mock_stream.h" | ||
|
|
||
| #include "adaptive_load/nighthawk_service_client_impl.h" | ||
| #include "gmock/gmock.h" | ||
| #include "gtest/gtest.h" | ||
|
|
||
| namespace Nighthawk { | ||
|
|
||
| namespace { | ||
|
|
||
| using ::Envoy::Protobuf::util::MessageDifferencer; | ||
| using ::nighthawk::client::CommandLineOptions; | ||
| using ::nighthawk::client::ExecutionRequest; | ||
| using ::nighthawk::client::ExecutionResponse; | ||
| using ::testing::_; | ||
| using ::testing::DoAll; | ||
| using ::testing::HasSubstr; | ||
| using ::testing::Return; | ||
| using ::testing::SaveArg; | ||
| using ::testing::SetArgPointee; | ||
|
|
||
| TEST(PerformNighthawkBenchmark, UsesSpecifiedDuration) { | ||
| const int kExpectedSeconds = 123; | ||
| ExecutionRequest request; | ||
| nighthawk::client::MockNighthawkServiceStub mock_nighthawk_service_stub; | ||
| EXPECT_CALL(mock_nighthawk_service_stub, ExecutionStreamRaw) | ||
| .WillOnce([&request](grpc_impl::ClientContext*) { | ||
| auto* mock_reader_writer = | ||
| new grpc::testing::MockClientReaderWriter<ExecutionRequest, ExecutionResponse>(); | ||
| EXPECT_CALL(*mock_reader_writer, Read(_)).WillOnce(Return(true)).WillOnce(Return(false)); | ||
| EXPECT_CALL(*mock_reader_writer, Write(_, _)) | ||
| .WillOnce(::testing::DoAll(::testing::SaveArg<0>(&request), Return(true))); | ||
| EXPECT_CALL(*mock_reader_writer, WritesDone()).WillOnce(Return(true)); | ||
| EXPECT_CALL(*mock_reader_writer, Finish()).WillOnce(Return(::grpc::Status::OK)); | ||
| return mock_reader_writer; | ||
| }); | ||
| Envoy::Protobuf::Duration duration; | ||
| duration.set_seconds(kExpectedSeconds); | ||
| NighthawkServiceClientImpl client; | ||
| absl::StatusOr<ExecutionResponse> response_or = client.PerformNighthawkBenchmark( | ||
| &mock_nighthawk_service_stub, CommandLineOptions(), duration); | ||
| EXPECT_TRUE(response_or.ok()); | ||
| EXPECT_EQ(request.start_request().options().duration().seconds(), kExpectedSeconds); | ||
| } | ||
|
|
||
| TEST(PerformNighthawkBenchmark, UsesSpecifiedCommandLineOptions) { | ||
| const int kExpectedRps = 456; | ||
| ExecutionRequest request; | ||
| nighthawk::client::MockNighthawkServiceStub mock_nighthawk_service_stub; | ||
| EXPECT_CALL(mock_nighthawk_service_stub, ExecutionStreamRaw) | ||
| .WillOnce([&request](grpc_impl::ClientContext*) { | ||
| auto* mock_reader_writer = | ||
| new grpc::testing::MockClientReaderWriter<ExecutionRequest, ExecutionResponse>(); | ||
| EXPECT_CALL(*mock_reader_writer, Read(_)).WillOnce(Return(true)).WillOnce(Return(false)); | ||
| EXPECT_CALL(*mock_reader_writer, Write(_, _)) | ||
| .WillOnce(::testing::DoAll(::testing::SaveArg<0>(&request), Return(true))); | ||
| EXPECT_CALL(*mock_reader_writer, WritesDone()).WillOnce(Return(true)); | ||
| EXPECT_CALL(*mock_reader_writer, Finish()).WillOnce(Return(::grpc::Status::OK)); | ||
| return mock_reader_writer; | ||
| }); | ||
| CommandLineOptions command_line_options; | ||
| command_line_options.mutable_requests_per_second()->set_value(kExpectedRps); | ||
| NighthawkServiceClientImpl client; | ||
| absl::StatusOr<ExecutionResponse> response_or = client.PerformNighthawkBenchmark( | ||
| &mock_nighthawk_service_stub, command_line_options, Envoy::Protobuf::Duration()); | ||
| EXPECT_TRUE(response_or.ok()); | ||
| EXPECT_EQ(request.start_request().options().requests_per_second().value(), kExpectedRps); | ||
| } | ||
|
|
||
| TEST(PerformNighthawkBenchmark, ReturnsNighthawkResponseSuccessfully) { | ||
| ExecutionResponse expected_response; | ||
| nighthawk::client::MockNighthawkServiceStub mock_nighthawk_service_stub; | ||
| EXPECT_CALL(mock_nighthawk_service_stub, ExecutionStreamRaw) | ||
| .WillOnce([&expected_response](grpc_impl::ClientContext*) { | ||
| auto* mock_reader_writer = | ||
| new grpc::testing::MockClientReaderWriter<ExecutionRequest, ExecutionResponse>(); | ||
| EXPECT_CALL(*mock_reader_writer, Read(_)) | ||
| .WillOnce(DoAll(SetArgPointee<0>(expected_response), Return(true))) | ||
| .WillOnce(Return(false)); | ||
| EXPECT_CALL(*mock_reader_writer, Write(_, _)).WillOnce(Return(true)); | ||
| EXPECT_CALL(*mock_reader_writer, WritesDone()).WillOnce(Return(true)); | ||
| EXPECT_CALL(*mock_reader_writer, Finish()).WillOnce(Return(::grpc::Status::OK)); | ||
| return mock_reader_writer; | ||
| }); | ||
| NighthawkServiceClientImpl client; | ||
| absl::StatusOr<ExecutionResponse> response_or = client.PerformNighthawkBenchmark( | ||
| &mock_nighthawk_service_stub, CommandLineOptions(), Envoy::Protobuf::Duration()); | ||
| EXPECT_TRUE(response_or.ok()); | ||
| ExecutionResponse actual_response = response_or.value(); | ||
| EXPECT_TRUE(MessageDifferencer::Equivalent(actual_response, expected_response)); | ||
| EXPECT_EQ(actual_response.DebugString(), expected_response.DebugString()); | ||
| } | ||
|
|
||
| TEST(PerformNighthawkBenchmark, ReturnsErrorIfNighthawkServiceDoesNotSendResponse) { | ||
| nighthawk::client::MockNighthawkServiceStub mock_nighthawk_service_stub; | ||
| EXPECT_CALL(mock_nighthawk_service_stub, ExecutionStreamRaw) | ||
| .WillOnce([](grpc_impl::ClientContext*) { | ||
| auto* mock_reader_writer = | ||
| new grpc::testing::MockClientReaderWriter<ExecutionRequest, ExecutionResponse>(); | ||
| EXPECT_CALL(*mock_reader_writer, Read(_)).WillOnce(Return(false)); | ||
| EXPECT_CALL(*mock_reader_writer, Write(_, _)).WillOnce(Return(true)); | ||
| EXPECT_CALL(*mock_reader_writer, WritesDone()).WillOnce(Return(true)); | ||
| return mock_reader_writer; | ||
| }); | ||
| NighthawkServiceClientImpl client; | ||
| absl::StatusOr<ExecutionResponse> response_or = client.PerformNighthawkBenchmark( | ||
| &mock_nighthawk_service_stub, CommandLineOptions(), Envoy::Protobuf::Duration()); | ||
| ASSERT_FALSE(response_or.ok()); | ||
| EXPECT_EQ(response_or.status().code(), absl::StatusCode::kUnknown); | ||
| EXPECT_THAT(response_or.status().message(), | ||
| HasSubstr("Nighthawk Service did not send a gRPC response.")); | ||
| } | ||
|
|
||
| TEST(PerformNighthawkBenchmark, ReturnsErrorIfNighthawkServiceWriteFails) { | ||
| nighthawk::client::MockNighthawkServiceStub mock_nighthawk_service_stub; | ||
| EXPECT_CALL(mock_nighthawk_service_stub, ExecutionStreamRaw) | ||
| .WillOnce([](grpc_impl::ClientContext*) { | ||
| auto* mock_reader_writer = | ||
| new grpc::testing::MockClientReaderWriter<ExecutionRequest, ExecutionResponse>(); | ||
| EXPECT_CALL(*mock_reader_writer, Write(_, _)).WillOnce(Return(false)); | ||
| return mock_reader_writer; | ||
| }); | ||
| NighthawkServiceClientImpl client; | ||
| absl::StatusOr<ExecutionResponse> response_or = client.PerformNighthawkBenchmark( | ||
| &mock_nighthawk_service_stub, CommandLineOptions(), Envoy::Protobuf::Duration()); | ||
| ASSERT_FALSE(response_or.ok()); | ||
| EXPECT_EQ(response_or.status().code(), absl::StatusCode::kUnknown); | ||
| EXPECT_THAT(response_or.status().message(), HasSubstr("Failed to write")); | ||
| } | ||
|
|
||
| TEST(PerformNighthawkBenchmark, ReturnsErrorIfNighthawkServiceWritesDoneFails) { | ||
| nighthawk::client::MockNighthawkServiceStub mock_nighthawk_service_stub; | ||
| EXPECT_CALL(mock_nighthawk_service_stub, ExecutionStreamRaw) | ||
| .WillOnce([](grpc_impl::ClientContext*) { | ||
| auto* mock_reader_writer = | ||
| new grpc::testing::MockClientReaderWriter<ExecutionRequest, ExecutionResponse>(); | ||
| EXPECT_CALL(*mock_reader_writer, Write(_, _)).WillOnce(Return(true)); | ||
| EXPECT_CALL(*mock_reader_writer, WritesDone()).WillOnce(Return(false)); | ||
| return mock_reader_writer; | ||
| }); | ||
| NighthawkServiceClientImpl client; | ||
| absl::StatusOr<ExecutionResponse> response_or = client.PerformNighthawkBenchmark( | ||
| &mock_nighthawk_service_stub, CommandLineOptions(), Envoy::Protobuf::Duration()); | ||
| ASSERT_FALSE(response_or.ok()); | ||
| EXPECT_EQ(response_or.status().code(), absl::StatusCode::kUnknown); | ||
| EXPECT_THAT(response_or.status().message(), HasSubstr("WritesDone() failed")); | ||
| } | ||
|
|
||
| TEST(PerformNighthawkBenchmark, ReturnsErrorIfNighthawkServiceGrpcStreamClosesAbnormally) { | ||
| nighthawk::client::MockNighthawkServiceStub mock_nighthawk_service_stub; | ||
| EXPECT_CALL(mock_nighthawk_service_stub, ExecutionStreamRaw) | ||
| .WillOnce([](grpc_impl::ClientContext*) { | ||
| auto* mock_reader_writer = | ||
| new grpc::testing::MockClientReaderWriter<ExecutionRequest, ExecutionResponse>(); | ||
| EXPECT_CALL(*mock_reader_writer, Read(_)).WillOnce(Return(true)).WillOnce(Return(false)); | ||
| EXPECT_CALL(*mock_reader_writer, Write(_, _)).WillOnce(Return(true)); | ||
| EXPECT_CALL(*mock_reader_writer, WritesDone()).WillOnce(Return(true)); | ||
| EXPECT_CALL(*mock_reader_writer, Finish()) | ||
| .WillOnce(Return(::grpc::Status(::grpc::UNKNOWN, "Finish failure status message"))); | ||
| return mock_reader_writer; | ||
| }); | ||
| NighthawkServiceClientImpl client; | ||
| absl::StatusOr<ExecutionResponse> response_or = client.PerformNighthawkBenchmark( | ||
| &mock_nighthawk_service_stub, CommandLineOptions(), Envoy::Protobuf::Duration()); | ||
| ASSERT_FALSE(response_or.ok()); | ||
| EXPECT_EQ(response_or.status().code(), absl::StatusCode::kUnknown); | ||
| EXPECT_THAT(response_or.status().message(), HasSubstr("Finish failure status message")); | ||
| } | ||
|
|
||
| } // namespace | ||
| } // namespace Nighthawk |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,7 +8,7 @@ TO_CHECK="${2:-$PWD}" | |
| bazel run @envoy//tools:code_format/check_format.py -- \ | ||
| --skip_envoy_build_rule_check --namespace_check Nighthawk \ | ||
| --build_fixer_check_excluded_paths=$(realpath ".") \ | ||
| --include_dir_order envoy,nighthawk,external/source/envoy,external,api,common,source,exe,server,client,test_common,test \ | ||
| --include_dir_order envoy,nighthawk,external/source/envoy,external,api,common,source,exe,server,client,grpcpp,test_common,test \ | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, that might have been a hassle to figure out, sorry about that. I wonder if we can improve there, ideally the "fix" doesn't produce something that the "check" objects to :-(
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is definitely a quick fix. It seems to pass all the CI checks. I don't know how it's arriving at the order or how it's checking it. If we figure out how it works, this would no longer be necessary.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's the right tweak to address this. But I do find it's disappointing that check and fix don't line up when the order isn't explicitly specified. I'll file an issue to see if Envoy has progressed here, and if there are changes worth including in our version of this. |
||
| $1 $TO_CHECK | ||
|
|
||
| # The include checker doesn't support per-file checking, so we only | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a line here indicating whether or not this class is thread-safe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a comment to the impl.