-
Notifications
You must be signed in to change notification settings - Fork 94
Adding config factory #522
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 100 commits
99f45d1
075a4cf
ecbf407
8fb11b6
032dbba
953e953
6185edb
cded6ad
fbfe523
0d70179
cdb7efe
f15bb47
59359a2
f63df91
406bd6b
f0b697b
8425e48
fb4f372
c6eae6c
96ae38b
b17240f
5c5a146
29591ff
7d928a9
ff82d95
98381a4
83ac556
38c5084
fa4e285
8551c76
437de5e
6cef87e
c84ba4d
77d33f9
73aa686
ef77d65
b9182a0
e38dd80
21734e4
7cda9d2
f827aca
d194736
7820cf9
bee7805
3db68ce
c0ec4dd
a1dd3f2
0b25261
c3fa23c
547977e
c300203
8f4d4d9
5ae2d51
fe74b6d
6d87aea
ca7a0c2
967e3f8
14b7d79
c4985bb
922639a
81dfece
bf08661
34900c5
768fa81
e93bb58
f47cbd4
7c5c6b2
1b02cec
4d75602
c74f535
9949fb8
fcf10d3
fd9744d
68c1037
883ee30
8c34b25
6ce7e74
92927e5
a62f2d5
16f89c6
f4257e3
d075588
eb10a3b
cdbd6bf
17acc5f
55a0e62
fa23aad
b8e2447
248f439
91703b5
bbc0c2a
517c83d
c1d0428
bc86c09
33e3564
338a874
402ada2
017481f
3ed43fc
247b71b
171fb2e
5df83ed
0af3ffd
ff90e59
e57c05f
55244b0
c710d37
fbb2fdc
ae0f4a8
e092de0
19b7aa0
dae3098
a4b6fb1
55f685e
59c5b94
2ea0c5e
d40a073
7146079
7ab1132
72c23d0
2b1170c
9dc3c4b
b555a71
3924eee
75eca07
9c93ff9
e2af886
edae9be
949d731
59d490f
078329a
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,35 @@ | ||
| syntax = "proto3"; | ||
|
|
||
| package nighthawk.request_source; | ||
|
|
||
| import "google/protobuf/wrappers.proto"; | ||
| import "validate/validate.proto"; | ||
| import "api/client/options.proto"; | ||
|
|
||
| // Configuration for DummyPluginRequestSource (plugin name: "nighthawk.dummy-request-source-plugin") | ||
|
Collaborator
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 we could use better documentation outlining what these individual plugin types do. This being the public API might be a good place to add a few sentences next to each plugin. That documentation should help the reader understand what the plugin is for. The current documentation is fairly short and requires the reader to already have some context.
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. Understood. I hope that I've added some context for this. |
||
| // that does nothing | ||
| message DummyPluginRequestSourceConfig { | ||
|
Collaborator
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. What is this one used for? It it only used for testing? If so, we could move it down in the file, so this isn't the first thing the reader encounters. Assuming it is just for testing, we should use better terminology than "dummy". A test only implementation that do just one thing are usually called "Stubs".
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. Understood.
Collaborator
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. These names are fairly repetitive. They all currently live in the request_source namespace, but also contain the words "RequestSource". Can we remove one or the other? This especially shows in the name RequestOptionsListPluginRequestSourceConfig which reached a length that makes it hard to parse meaning from it.
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. Ok. |
||
| // Dummy input value. | ||
| google.protobuf.DoubleValue dummy_value = 1; | ||
|
Collaborator
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. Why do we need a value on a plugin that doesn't do anything? I suspect it is for testing, but just making sure I understand it correctly.
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. Yes, this is just for testing. |
||
| } | ||
|
|
||
| // Configuration for RPCPluginRequestSource (plugin name: "nighthawk.rpc-request-source-plugin") | ||
| // that uses rpc | ||
| message RPCPluginRequestSourceConfig { | ||
| string uri = 1; | ||
|
Collaborator
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. Can we add a comment explaining this field? All fields in proto messages should have good comments since they essentially represent the public API. Please apply throughout.
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. Understood. |
||
| } | ||
|
|
||
| // Configuration for FileBasedPluginRequestSource (plugin name: | ||
| // "nighthawk.file-based-request-source-plugin") that uses rpc | ||
| message FileBasedPluginRequestSourceConfig { | ||
| string file_path = 1; | ||
| google.protobuf.UInt32Value num_requests = 2 [(validate.rules).uint32 = {gte: 0, lte: 1000000}]; | ||
| google.protobuf.UInt32Value max_file_size = 3; | ||
|
Collaborator
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. Why is the max_file_size a configuration knob? Do we have a good use case where the user needs to tweak this value?
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. I figured that different users might have different requirements for what they find appropriate as the maximum file size. I wasn't sure where else to pass that parameter in than in the proto. I have added a validate to this as a hard limit as well though. |
||
| } | ||
|
|
||
| // Configuration for RequestOptionsListPluginRequestSource (plugin name: | ||
| // "nighthawk.request-options-list-request-source-plugin") that uses rpc | ||
| message RequestOptionsListPluginRequestSourceConfig { | ||
| nighthawk.client.RequestOptionsList options_list = 1; | ||
| google.protobuf.UInt32Value num_requests = 2 [(validate.rules).uint32 = {gte: 0, lte: 1000000}]; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -64,6 +64,21 @@ envoy_basic_cc_library( | |
| ], | ||
| ) | ||
|
|
||
| envoy_basic_cc_library( | ||
| name = "request_source_plugin_config_factory_lib", | ||
|
Collaborator
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. Is it by omission or intentional that we are leaving the request source factory in the "common" directory rather than in the new "request_source" directory? |
||
| hdrs = [ | ||
| "request_source_plugin_config_factory.h", | ||
| ], | ||
| include_prefix = "nighthawk/common", | ||
| deps = [ | ||
| ":request_source_lib", | ||
| "//api/request_source:request_source_plugin_cc_proto", | ||
| "@envoy//include/envoy/common:base_includes", | ||
| "@envoy//include/envoy/config:typed_config_interface", | ||
| "@envoy//source/common/api:api_lib_with_external_headers", | ||
| ], | ||
| ) | ||
|
|
||
| envoy_basic_cc_library( | ||
| name = "request_source_lib", | ||
| hdrs = [ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| #pragma once | ||
|
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. I hadn't moved this because it was the only file in the include directory, but I see the value in having a similar directory structure to the source directory. |
||
|
|
||
| #include "envoy/api/api.h" | ||
| #include "envoy/common/pure.h" | ||
| #include "envoy/config/typed_config.h" | ||
|
|
||
| #include "nighthawk/common/request_source.h" | ||
|
|
||
| namespace Nighthawk { | ||
|
|
||
| /** | ||
| * A factory that must be implemented for each RequestSourcePlugin. It instantiates the specific | ||
| * RequestSourcePlugin class after unpacking the plugin-specific config proto. | ||
| */ | ||
| class RequestSourcePluginConfigFactory : public Envoy::Config::TypedFactory { | ||
| public: | ||
| ~RequestSourcePluginConfigFactory() override = default; | ||
| std::string category() const override { return "nighthawk.request_source_plugin"; } | ||
| /** | ||
| * Instantiates the specific RequestSourcePlugin class. Casts |message| to Any, unpacks it to the | ||
| * plugin-specific proto, and passes the strongly typed proto to the plugin constructor. | ||
| * | ||
| * @param message Any typed_config proto taken from the TypedExtensionConfig. | ||
| * | ||
| * @param api Api parameter that contains timesystem, filesystem, and threadfactory. | ||
|
Collaborator
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. Looks like we may be missing comment related to the header param.
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. Thanks. Sorry about that. Still a little sloppy on these comments. I'll keep better track of this moving forward. |
||
| * | ||
| * @return RequestSourcePtr Pointer to the new plugin instance. | ||
| * | ||
| * @throw Envoy::EnvoyException If the Any proto cannot be unpacked as the type expected by the | ||
| * plugin. | ||
| */ | ||
| virtual RequestSourcePtr createRequestSourcePlugin(const Envoy::Protobuf::Message& typed_config, | ||
| Envoy::Api::ApiPtr api, | ||
| Envoy::Http::RequestHeaderMapPtr header) PURE; | ||
| }; | ||
|
|
||
| } // namespace Nighthawk | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -81,6 +81,30 @@ envoy_cc_library( | |
| ], | ||
| ) | ||
|
|
||
| envoy_cc_library( | ||
| name = "request_source_plugin_impl", | ||
|
Collaborator
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 a good point to start thinking about slightly different file / directory structure here. The common directory is growing which makes it harder to navigate. How about we create a new request_source directory instead at the same level where common is? In addition to this, we should split out the individual request source plugin implementations into their own files. Placing them all into one will quickly outgrow "good file size". There isn't a hard rule, but as a recommendation I usually start thinking about a refactor when a file reaches about 300 lines. Let's try to refactor these as suggested and see how it looks.
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. Understood. |
||
| srcs = [ | ||
| "request_source_plugin_impl.cc", | ||
| ], | ||
| hdrs = [ | ||
| "request_source_plugin_impl.h", | ||
| ], | ||
| repository = "@envoy", | ||
| visibility = ["//visibility:public"], | ||
| deps = [ | ||
| ":nighthawk_common_lib", | ||
| ":request_impl_lib", | ||
| ":request_source_impl_lib", | ||
| "//include/nighthawk/common:request_source_plugin_config_factory_lib", | ||
| "@envoy//source/common/common:thread_lib_with_external_headers", | ||
| "@envoy//source/common/protobuf:message_validator_lib_with_external_headers", | ||
| "@envoy//source/common/protobuf:protobuf_with_external_headers", | ||
| "@envoy//source/common/protobuf:utility_lib_with_external_headers", | ||
| "@envoy//source/exe:platform_header_lib_with_external_headers", | ||
| "@envoy//source/exe:platform_impl_lib", | ||
| ], | ||
| ) | ||
|
|
||
| envoy_cc_library( | ||
| name = "version_linkstamp", | ||
| srcs = ["version_linkstamp.cc"], | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,151 @@ | ||
| #include "common/request_source_plugin_impl.h" | ||
|
|
||
| #include "external/envoy/source/common/protobuf/message_validator_impl.h" | ||
| #include "external/envoy/source/common/protobuf/utility.h" | ||
| #include "external/envoy/source/exe/platform_impl.h" | ||
|
|
||
| #include "api/client/options.pb.h" | ||
|
|
||
| #include "common/request_impl.h" | ||
| #include "common/request_source_impl.h" | ||
|
|
||
| namespace Nighthawk { | ||
|
|
||
| std::string DummyRequestSourcePluginConfigFactory::name() const { | ||
| return "nighthawk.dummy-request-source-plugin"; | ||
| } | ||
|
|
||
| Envoy::ProtobufTypes::MessagePtr DummyRequestSourcePluginConfigFactory::createEmptyConfigProto() { | ||
| return std::make_unique<nighthawk::request_source::DummyPluginRequestSourceConfig>(); | ||
| } | ||
|
|
||
| RequestSourcePtr DummyRequestSourcePluginConfigFactory::createRequestSourcePlugin( | ||
| const Envoy::Protobuf::Message& message, Envoy::Api::ApiPtr, Envoy::Http::RequestHeaderMapPtr) { | ||
| const auto& any = dynamic_cast<const Envoy::ProtobufWkt::Any&>(message); | ||
| nighthawk::request_source::DummyPluginRequestSourceConfig config; | ||
| Envoy::MessageUtil::unpackTo(any, config); | ||
| return std::make_unique<DummyRequestSource>(config); | ||
| } | ||
|
|
||
| REGISTER_FACTORY(DummyRequestSourcePluginConfigFactory, RequestSourcePluginConfigFactory); | ||
|
|
||
| DummyRequestSource::DummyRequestSource( | ||
| const nighthawk::request_source::DummyPluginRequestSourceConfig& config) | ||
| : dummy_value_{config.has_dummy_value() ? config.dummy_value().value() | ||
| : std::numeric_limits<double>::infinity()} {} | ||
| RequestGenerator DummyRequestSource::get() { | ||
|
|
||
| RequestGenerator request_generator = []() { | ||
| Envoy::Http::RequestHeaderMapPtr header = Envoy::Http::RequestHeaderMapImpl::create(); | ||
| auto returned_request_impl = std::make_unique<RequestImpl>(std::move(header)); | ||
| return returned_request_impl; | ||
| }; | ||
| return request_generator; | ||
| } | ||
|
|
||
| void DummyRequestSource::initOnThread() {} | ||
|
|
||
| std::string FileBasedRequestSourcePluginConfigFactory::name() const { | ||
| return "nighthawk.file-based-request-source-plugin"; | ||
| } | ||
|
|
||
| Envoy::ProtobufTypes::MessagePtr | ||
| FileBasedRequestSourcePluginConfigFactory::createEmptyConfigProto() { | ||
| return std::make_unique<nighthawk::request_source::FileBasedPluginRequestSourceConfig>(); | ||
| } | ||
|
|
||
| RequestSourcePtr FileBasedRequestSourcePluginConfigFactory::createRequestSourcePlugin( | ||
| const Envoy::Protobuf::Message& message, Envoy::Api::ApiPtr api, | ||
| Envoy::Http::RequestHeaderMapPtr header) { | ||
| const auto& any = dynamic_cast<const Envoy::ProtobufWkt::Any&>(message); | ||
| nighthawk::request_source::FileBasedPluginRequestSourceConfig config; | ||
| Envoy::MessageUtil util; | ||
|
|
||
| util.unpackTo(any, config); | ||
| if (api->fileSystem().fileSize(config.file_path()) > config.max_file_size().value()) { | ||
| throw NighthawkException("file size must be less than max_file_size"); | ||
| } | ||
| auto temp_list = std::make_unique<nighthawk::client::RequestOptionsList>(); | ||
|
|
||
| // Locking to avoid issues with multiple threads reading the same file. | ||
| { | ||
| Envoy::Thread::LockGuard lock_guard(file_lock_); | ||
|
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. Why do we need to lock here? Can we spend a code-level comment on that? Also, should we then also have a simple spam test for this, calling it from multiple threads? We might not even want to have any specific expectations in it, but just so we give tsan a good opportunity to have a say about this?
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. I added a test with multiple calls to create, though I'm not sure that this tests for what you want to test for. I added a comment about the lock also.
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. Thanks for that comment, that is helpful. So now that the intent is clear, looking at the test you added, how about adding something like [1] to ensure multiple concurrent calls will only ever result in a single config file being read from disk? Such an amendment would also give the CI TSAN job a chance to verify. [1] void spamCall(std::function<void()>&& call_to_spam, const uint32_t num_threads) {
std::vector<std::thread> threads(num_threads);
auto barrier = std::make_unique<absl::Barrier>(num_threads);
for (auto& thread : threads) {
thread = std::thread([&call_to_spam, &barrier] {
// Allow threads to accrue, to maximize concurrency on the call we are testing.
if (barrier->Block()) {
barrier.reset();
}
call_to_spam();
});
}
for (std::thread& thread : threads) {
thread.join();
}
}
....
TEST_F(FileBasedRequestSourcePluginTest, CreateMultipleRequestSourcePluginReadsFileOnce) {
constexpr uint32_t kNumThreads = 100;
spamCall([kNumThreads]() {
.. insert the current test body which ensures a single config file is read that you have here...
},
kNumThreads);
}
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. Also, it seems to me that there might be value to put every known implementation of
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. After further thought, this doesn't seem appropriate to have as a unit test. The primary problem being that if it were to be an appropriate unit test for a threading issue, then it is inherently trying to be non deterministic, which is not what a unit test should be. It seems like what we want is maybe closer to an integration test, but that's probably best done in a separate pr.
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. A separate PR works for me; maybe file an issue to track? |
||
| // Reading the file only the first time. | ||
| if (options_list_.options_size() == 0) { | ||
| util.loadFromFile(config.file_path(), options_list_, | ||
| Envoy::ProtobufMessage::getStrictValidationVisitor(), *api, true); | ||
| } | ||
| temp_list->CopyFrom(options_list_); | ||
| } | ||
| return std::make_unique<RequestOptionsListRequestSource>(config.num_requests().value(), | ||
| std::move(header), std::move(temp_list)); | ||
| } | ||
|
|
||
| REGISTER_FACTORY(FileBasedRequestSourcePluginConfigFactory, RequestSourcePluginConfigFactory); | ||
|
|
||
| RequestOptionsListRequestSource::RequestOptionsListRequestSource( | ||
| const uint32_t request_max, Envoy::Http::RequestHeaderMapPtr header, | ||
| std::unique_ptr<nighthawk::client::RequestOptionsList> options_list) | ||
| : header_(std::move(header)), options_list_(std::move(options_list)), | ||
| request_max_(request_max) {} | ||
|
|
||
| RequestGenerator RequestOptionsListRequestSource::get() { | ||
| uint32_t counter = 0; | ||
| request_count_.push_back(counter); | ||
| uint32_t& lambda_counter = request_count_.back(); | ||
| RequestGenerator request_generator = [this, lambda_counter]() mutable -> RequestPtr { | ||
| // if request_max is 0, then we never stop generating requests. | ||
| if (lambda_counter >= request_max_ && request_max_ != 0) { | ||
| return nullptr; | ||
| } | ||
|
|
||
| // Increment the counter and get the request_option from the list for the current iteration. | ||
| auto index = lambda_counter % options_list_->options_size(); | ||
| nighthawk::client::RequestOptions request_option = options_list_->options().at(index); | ||
| lambda_counter++; | ||
|
|
||
| // Initialize the header with the values from the default header. | ||
| Envoy::Http::RequestHeaderMapPtr header = Envoy::Http::RequestHeaderMapImpl::create(); | ||
| Envoy::Http::HeaderMapImpl::copyFrom(*header, *header_); | ||
|
|
||
| // Override the default values with the values from the request_option | ||
| header->setMethod(envoy::config::core::v3::RequestMethod_Name(request_option.request_method())); | ||
| const uint32_t content_length = request_option.request_body_size().value(); | ||
| if (content_length > 0) { | ||
| header->setContentLength(content_length); | ||
| } | ||
| for (const auto& option_header : request_option.request_headers()) { | ||
| auto lower_case_key = Envoy::Http::LowerCaseString(std::string(option_header.header().key())); | ||
| header->setCopy(lower_case_key, std::string(option_header.header().value())); | ||
| } | ||
| return std::make_unique<RequestImpl>(std::move(header)); | ||
| }; | ||
| return request_generator; | ||
| } | ||
|
|
||
| void RequestOptionsListRequestSource::initOnThread() {} | ||
|
|
||
| std::string RequestOptionsListRequestSourcePluginConfigFactory::name() const { | ||
| return "nighthawk.request-options-list-request-source-plugin"; | ||
| } | ||
|
|
||
| Envoy::ProtobufTypes::MessagePtr | ||
| RequestOptionsListRequestSourcePluginConfigFactory::createEmptyConfigProto() { | ||
| return std::make_unique<nighthawk::request_source::RequestOptionsListPluginRequestSourceConfig>(); | ||
| } | ||
|
|
||
| RequestSourcePtr RequestOptionsListRequestSourcePluginConfigFactory::createRequestSourcePlugin( | ||
| const Envoy::Protobuf::Message& message, Envoy::Api::ApiPtr, | ||
| Envoy::Http::RequestHeaderMapPtr header) { | ||
| const auto& any = dynamic_cast<const Envoy::ProtobufWkt::Any&>(message); | ||
| nighthawk::request_source::RequestOptionsListPluginRequestSourceConfig config; | ||
| Envoy::MessageUtil::unpackTo(any, config); | ||
| auto temp_list = std::make_unique<nighthawk::client::RequestOptionsList>(config.options_list()); | ||
| return std::make_unique<RequestOptionsListRequestSource>(config.num_requests().value(), | ||
| std::move(header), std::move(temp_list)); | ||
| } | ||
|
|
||
| REGISTER_FACTORY(RequestOptionsListRequestSourcePluginConfigFactory, | ||
| RequestSourcePluginConfigFactory); | ||
|
|
||
| } // namespace Nighthawk | ||
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.
Plz add comment for the proto and its field.
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 do.