-
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 111 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 @@ | ||
| // Config protos for the Request Source Plugin Config Factories. | ||
| syntax = "proto3"; | ||
|
|
||
| package nighthawk.request_source; | ||
|
|
||
| import "google/protobuf/wrappers.proto"; | ||
| import "validate/validate.proto"; | ||
| import "api/client/options.proto"; | ||
|
|
||
| // Configuration for FileBasedPluginRequestSource (plugin name: | ||
| // "nighthawk.file-based-request-source-plugin") | ||
| // The factory will load the RequestOptionsList from the file, and then passes it to the | ||
| // requestSource it generates. The resulting request source will loop over the RequestOptionsList it | ||
| // is passed. | ||
| message FileBasedPluginConfig { | ||
| // The file_path is the path to a file that contains a RequestOptionList in json or yaml format. | ||
| string file_path = 1; | ||
| // The pluginfactory makes requestSources that will generate requests from the RequestOptionList | ||
| // up to num_requests number of times. If num_requests exceeds the number of RequestOptions in the | ||
| // RequestOptionList located in the file at file_path, it will loop. num_requests = 0 means no | ||
| // limit on the number of requests to be produced. | ||
| google.protobuf.UInt32Value num_requests = 2 [(validate.rules).uint32 = {gte: 0, lte: 1000000}]; | ||
| // The pluginfactory will load the file located in file_path as long as it is below max_file_size, | ||
| // if it's too large it will throw an error. | ||
| google.protobuf.UInt32Value max_file_size = 3 [(validate.rules).uint32 = {lte: 1000000}]; | ||
| } | ||
|
|
||
| // Configuration for StubPluginRequestSource (plugin name: "nighthawk.stub-request-source-plugin") | ||
| // The plugin does nothing. This is for testing and comparison of the Request Source Plugin Factory | ||
| // mechanism using a minimal version of plugin that does not require a more complicated proto or | ||
| // file reading. | ||
| message StubPluginConfig { | ||
| // test input value. | ||
|
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 improve this comment? We could say what the input argument is for / what does the plugin do with it. It raises a question as to why a plugin that does nothing need an input argument.
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. Sure.
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. We had a comment thread about improving this comment, but looks like the it wasn't actually done. Repeating the comment: Can we improve this comment? We could say what the input argument is for / what does the plugin do with it. It raises a question as to why a plugin that does nothing need an input argument.
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. Oh I thought I fixed it by making it do something. |
||
| google.protobuf.DoubleValue test_value = 1; | ||
| } | ||
| 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,39 @@ | ||
| #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; | ||
| // All request source plugins will be in this category. | ||
| 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 typed_config Any typed_config proto taken from the TypedExtensionConfig. This should be | ||
| // a type listed in request_source_plugin_config.proto | ||
| // | ||
| // @param api Api parameter that contains timesystem, filesystem, and threadfactory. | ||
| // | ||
| // @param header RequestHeaderMapPtr parameter that acts as a template header for the | ||
| // requestSource to modify when generating requests. | ||
| // | ||
| // @return RequestSourcePtr Pointer to the new instance of RequestSource. | ||
| // | ||
| // @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 |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| load( | ||
| "@envoy//bazel:envoy_build_system.bzl", | ||
| "envoy_cc_library", | ||
| "envoy_package", | ||
| ) | ||
|
|
||
| licenses(["notice"]) # Apache 2 | ||
|
|
||
| envoy_package() | ||
|
|
||
| envoy_cc_library( | ||
| name = "request_source_plugin_impl", | ||
| srcs = [ | ||
| "request_options_list_plugin_impl.cc", | ||
|
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. The choice of file name here doesn't directly relate to what it contains. Looks like it contains the File Based Request source. Can we call the file accordingly, e.g. "file_based_plugin.cc" or "file_based_request_source.cc". We should also adjust the name of the BUILD target to match. EDIT: I noticed that the file contains multiple request source implementations. If we end up keeping the RequestOptionsListRequestSource, let's have it in its own file and library also.
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 wanted this to be called the RequestOptionsListPluginImpl because I figured both the FileBasedPluginConfigFactory, and the RequestOptionListPluginConfigFactory would make the same RequestOptionListPlugin, just loading from a different location. |
||
| "stub_plugin_impl.cc", | ||
|
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. We probably shouldn't combine these two plugin implementations in one BUILD target. It is unlikely that anyone will use the stub_plugin in practice and when combined like this, they are forced to build and possibly link it in. Can we split them out into two targets? Once done, we can additionally mark the stub_plugin target as "testonly" by setting Essentially we should end up in a setup where for each plugin we have a separate header file, cc file and BUILD target.
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.
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'm not sure how testonly=1 works. My attempts to use it gives me: envoy_cc_library() got unexpected keyword argument: testonly |
||
| ], | ||
| hdrs = [ | ||
| "request_source_plugin_impl.h", | ||
| ], | ||
| repository = "@envoy", | ||
| visibility = ["//visibility:public"], | ||
| deps = [ | ||
| "//include/nighthawk/common:request_source_plugin_config_factory_lib", | ||
| "//source/common:nighthawk_common_lib", | ||
| "//source/common:request_impl_lib", | ||
| "//source/common:request_source_impl_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", | ||
| ], | ||
| ) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,93 @@ | ||
| #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" | ||
|
|
||
| #include "request_source/request_source_plugin_impl.h" | ||
|
|
||
| namespace Nighthawk { | ||
| std::string FileBasedRequestSourcePluginConfigFactory::name() const { | ||
| return "nighthawk.file-based-request-source-plugin"; | ||
| } | ||
|
|
||
| Envoy::ProtobufTypes::MessagePtr | ||
| FileBasedRequestSourcePluginConfigFactory::createEmptyConfigProto() { | ||
| return std::make_unique<nighthawk::request_source::FileBasedPluginConfig>(); | ||
| } | ||
|
|
||
| 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::FileBasedPluginConfig 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>(); | ||
|
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. We should choose a better variable name here, "temp_list" doesn't actually communicate anything useful to the reader. All variables are essentially temporary and the fact that this is a list can be seen from the object type. If we do keep this (see my comment below), let's try to come up with a more descriptive name. I can help by suggesting such a name once I understand why do we need this variable.
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 had originally wanted to pass the ownership of the copied list because I felt that it made sense for the RequestSources to have full ownership of their own contents. I know that the factory is longer lived than the requestsources, but it seemed more logical to have the lifetime be tied to the requestsource rather than the factory. That being said, I acknowledge that the memory leak is prohibitive and unnecessary. I'll get rid of it. |
||
|
|
||
| // Locking to avoid issues with multiple threads reading the same file. | ||
| { | ||
| Envoy::Thread::LockGuard lock_guard(file_lock_); | ||
| // 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_); | ||
|
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. Since we load options_list_ only once, why do we need to make a copy here? Can't we give a const reference to all the created plugins instead?
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 had thought it made more sense for the requestSource to have ownership of the list, but I think that the factory does have a longer lifetime than the requestSources it generates, so that probably would work. |
||
| } | ||
| 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; | ||
|
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. Do we need to define the counter locally as a variable? Or can we just push 0 on and take its address? request_count_.push_back(0);
uint32_t& lambda_counter = request_count_.back();
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, we can do that. |
||
| request_count_.push_back(counter); | ||
| uint32_t& lambda_counter = request_count_.back(); | ||
| RequestGenerator request_generator = [this, lambda_counter]() mutable -> RequestPtr { | ||
|
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 you help me understand the need for the "mutable" keyword here?
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 need this keyword because if I do not have mutable, the compiler will complain about the incrementing of the lambda_counter later in this lambda. |
||
| // 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(); | ||
|
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. The "auto" keyword here actually hides a type that would be useful to see. Ideally we only use auto when the type is too long or redundant in the statement. https://google.github.io/styleguide/cppguide.html#Type_deduction
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. |
||
| nighthawk::client::RequestOptions request_option = options_list_->options().at(index); | ||
| lambda_counter++; | ||
|
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. In c++, we should prefer pre-increment and pre-decrement: https://google.github.io/styleguide/cppguide.html#Preincrement_and_Predecrement
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 |
||
|
|
||
| // Initialize the header with the values from the default header. | ||
| Envoy::Http::RequestHeaderMapPtr header = Envoy::Http::RequestHeaderMapImpl::create(); | ||
| Envoy::Http::HeaderMapImpl::copyFrom(*header, *header_); | ||
|
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. Since we are making a copy of the original header map anyway, why do we need to pass it in as a pointer to the plugin? Can't the plugin take the header as a const reference to make it clear on the API layer that the header isn't owned or modified? Or is there a reason that forces us to give ownership?
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 think that it is more consistent with the way that the other plugins do it, and it is possible that the plugin would want to modify the header. Generally it also seems like something that the RequestSource should own. In the factories_impl.cc where this RequestSource will be created, I believe the header passed in does not have a lifetime greater than that of the RequestSource, and that might matter also, though I'm not sure if that is correct. |
||
|
|
||
| // 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()) { | ||
|
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 another place where auto might be hiding useful type information.
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. |
||
| 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() {} | ||
|
|
||
| } // namespace Nighthawk | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,124 @@ | ||
| // Implementations of RequestSourceConfigFactory and the RequestSources that those factories make. | ||
| #pragma once | ||
|
|
||
| #include "envoy/registry/registry.h" | ||
|
|
||
| #include "nighthawk/common/request_source_plugin_config_factory.h" | ||
|
|
||
| #include "external/envoy/source/common/common/lock_guard.h" | ||
| #include "external/envoy/source/common/common/thread.h" | ||
|
|
||
| #include "api/client/options.pb.h" | ||
| #include "api/request_source/request_source_plugin.pb.h" | ||
|
|
||
| #include "common/uri_impl.h" | ||
|
|
||
| namespace Nighthawk { | ||
|
|
||
| // Stub Request Source implementation for comparison. | ||
| class DummyRequestSource : public RequestSource { | ||
|
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. We probably wanted to rename this class to StubRequestSource. We should also rename the factory so we only refer to the plugin using one name. And as per my previous comment - we can move it to its own header file and BUILD target.
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. |
||
| public: | ||
| explicit DummyRequestSource(const nighthawk::request_source::StubPluginConfig& config); | ||
| // The generator function will only return empty headers. | ||
| // The function is threadsafe. | ||
| RequestGenerator get() override; | ||
|
|
||
| // default implementation | ||
|
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. Note - comments on functions that are overrides aren't required and shouldn't be present, since the authoritative location to describe these is on the parent class (on the interface). Let's remove any comments we added to overrides. If the parent classes on some of these don't have good comments, we should instead work on improving those in a separate PR.
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. |
||
| void initOnThread() override; | ||
| }; | ||
|
|
||
| // Factory that creates a DummyRequestSource from a DummyRequestSourcePluginConfig proto. | ||
| // Registered as an Envoy plugin. | ||
| // Stub implementation of RequestSourceConfigFactory which produces a RequestSource. | ||
| // RequestSources are used to get RequestGenerators which generate requests for the benchmark | ||
| // client. All plugins configuration are specified in the request_source_plugin.proto This class is | ||
| // thread-safe, but it doesn't do anything. Usage: assume you are passed an appropriate Any type | ||
| // object called config, an Api object called api, and a default header called header. auto& | ||
| // config_factory = | ||
| // Envoy::Config::Utility::getAndCheckFactoryByName<RequestSourcePluginConfigFactory>( | ||
| // "nighthawk.stub-request-source-plugin"); | ||
| // RequestSourcePtr plugin = | ||
| // config_factory.createRequestSourcePlugin(config, std::move(api), std::move(header)); | ||
|
|
||
| class DummyRequestSourcePluginConfigFactory : public virtual RequestSourcePluginConfigFactory { | ||
| public: | ||
| // This is a hardcoded string. | ||
| std::string name() const override; | ||
| // This returns an empty version of the expected StubPluginConfig from request_source_plugin.proto | ||
| Envoy::ProtobufTypes::MessagePtr createEmptyConfigProto() override; | ||
| // This is the primary method that is used to get RequestSources. | ||
| // This implementation is thread safe, but the RequestSource it generates doesn't do much. | ||
| RequestSourcePtr createRequestSourcePlugin(const Envoy::Protobuf::Message& message, | ||
| Envoy::Api::ApiPtr api, | ||
| Envoy::Http::RequestHeaderMapPtr header) override; | ||
| }; | ||
|
|
||
| // This factory will be activated through RequestSourceFactory in factories.h | ||
| DECLARE_FACTORY(DummyRequestSourcePluginConfigFactory); | ||
|
|
||
| // Sample Request Source for small RequestOptionsLists. Loads a copy of the RequestOptionsList in | ||
| // memory and replays them. | ||
| // @param request_max The number of requests the requestGenerator produced by get() will generate. 0 | ||
|
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. If I understand correctly what "request_max" specifies, a name like "total_requests" would probably be more descriptive.
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. Agreed. |
||
| // means it is unlimited. | ||
| // @param header the default header that will be overridden by values taken from the options_list, | ||
| // any values not overridden will be used. | ||
| // @param options_list A copy of the options_list will be loaded in memory. The RequestGenerator | ||
| // produced by get() will use options from the options_list to overwrite values in the header, and | ||
| // create new requests. if request_max is greater than the length of options_list, it will loop. | ||
| // This is not thread safe. | ||
| class RequestOptionsListRequestSource : public RequestSource { | ||
|
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 the practical use case for this request source implementation? Looking back, our plan was to implement the file based request source to serve as an example and a gRPC request source for backward compatibility. Can you help me understand what do we need this one for? EDIT: After looking at the .cc file, it looks like this request source implementation is used internally by the FileBased request source to do its job. If that it the plan, that is fine, but then this shouldn't be part of the API. The API (the header) should only contain the FileBased request source. The .cc file can have a section with anonymous namespace where we define the RequestOptionsListRequestSource internally. Alternatively, if we plan to reuse this by multiple request sources, we should move it into its own library and say (in the header file) that this is for internal purposes only.
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 felt like it made sense for both the RequestSource and the factory that makes the requestSource to live in the same file together, I can move it into its own library if you prefer. |
||
| public: | ||
| explicit RequestOptionsListRequestSource( | ||
|
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. The "explicit" keyword is only needed on constructors with a single argument and has no effect here. https://google.github.io/styleguide/cppguide.html#Implicit_Conversions
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. That makes sense. Thanks. |
||
| const uint32_t request_max, Envoy::Http::RequestHeaderMapPtr header, | ||
| std::unique_ptr<nighthawk::client::RequestOptionsList> options_list); | ||
| // This get function is not thread safe, because multiple threads calling get simultaneously will | ||
| // result in a collision as it attempts to update its request_count_. | ||
| RequestGenerator get() override; | ||
|
|
||
| // default implementation | ||
| void initOnThread() override; | ||
|
|
||
| private: | ||
| Envoy::Http::RequestHeaderMapPtr header_; | ||
| const std::unique_ptr<nighthawk::client::RequestOptionsList> options_list_; | ||
| std::vector<uint32_t> request_count_; | ||
| const uint32_t request_max_; | ||
| }; | ||
|
|
||
| // Factory that creates a RequestOptionsListRequestSource from a FileBasedPluginConfig proto. | ||
|
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. Possible typo, looks like this factory creates a FileBasedRequestSourcePlugin rather than a RequestOptionsListRequestSource.
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. Not a typo. I wanted this to be a more generic version of the RequestOptionsListRequestSource since it seemed the more versatile option. |
||
| // Registered as an Envoy plugin. | ||
| // Implementation of RequestSourceConfigFactory which produces a RequestSource that keeps an | ||
|
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 comment contains a mix of good class comments and implementation comments. We shouldn't comment or describe implementation here as it is bound to change and isn't part of the API contract. An example of implementation comments here are:
On the other hand saying that the class is not thread safe is a very good comment here, but it doesn't need more details. Please use your best judgment to adjust the comments so they talk about what a class does and how it can be used, but never about how it does its job.
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'm not sure I'm doing a very good job at this, but I'll do my best. |
||
| // RequestOptionsList in memory RequestSources are used to get RequestGenerators which generate | ||
| // requests for the benchmark client. All plugins configuration are specified in the | ||
| // request_source_plugin.proto This class is not thread-safe, because it loads its RequestOptionlist | ||
| // in memory from a file when first called. The in memory RequestOptionsList is protected by | ||
| // file_lock_. Usage: assume you are passed an appropriate Any type object called config, an Api | ||
| // object called api, and a default header called header. auto& config_factory = | ||
| // Envoy::Config::Utility::getAndCheckFactoryByName<RequestSourcePluginConfigFactory>( | ||
| // "nighthawk.file-based-request-source-plugin"); | ||
| // RequestSourcePtr plugin = | ||
| // config_factory.createRequestSourcePlugin(config, std::move(api), std::move(header)); | ||
| class FileBasedRequestSourcePluginConfigFactory : public virtual RequestSourcePluginConfigFactory { | ||
| public: | ||
| std::string name() const override; | ||
| // This returns an empty version of the expected FileBasedPluginConfig from | ||
| // request_source_plugin.proto | ||
| Envoy::ProtobufTypes::MessagePtr createEmptyConfigProto() override; | ||
| // This is the primary method that is used to get RequestSources. | ||
| // This implementation is not thread safe. Only the first call to createRequestSourcePlugin will | ||
| // load the file from memory and subsequent calls just make a copy of the options_list that was | ||
| // already loaded. The FileBasedRequestSourcePluginConfigFactory will not work with multiple | ||
| // different files for this reason. | ||
| RequestSourcePtr createRequestSourcePlugin(const Envoy::Protobuf::Message& message, | ||
|
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 this method would throw if the file cannot be parsed correctly. Can we mention that in the comment?
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. Sure. |
||
| Envoy::Api::ApiPtr api, | ||
| Envoy::Http::RequestHeaderMapPtr header) override; | ||
|
|
||
| private: | ||
| Envoy::Thread::MutexBasicLockable file_lock_; | ||
| nighthawk::client::RequestOptionsList options_list_ ABSL_GUARDED_BY(file_lock_); | ||
| }; | ||
|
|
||
| // This factory will be activated through RequestSourceFactory in factories.h | ||
| DECLARE_FACTORY(FileBasedRequestSourcePluginConfigFactory); | ||
|
|
||
| } // 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.