Skip to content
Merged
Show file tree
Hide file tree
Changes from 135 commits
Commits
Show all changes
150 commits
Select commit Hold shift + click to select a range
99f45d1
Adding a multiple path test to confirm that expected paths are hit
wjuan-AFK Jul 15, 2020
075a4cf
fixing asan problems
wjuan-AFK Jul 17, 2020
ecbf407
Fix Format
wjuan-AFK Jul 17, 2020
8fb11b6
Removeing unused comments
wjuan-AFK Jul 17, 2020
032dbba
Removing unnecessary invoke calls for cleanliness
wjuan-AFK Jul 17, 2020
953e953
Merge branch 'master' into RequestSourceUnitTestBenchmarkMultiplePath
wjuan-AFK Jul 17, 2020
6185edb
Refactoring test to take in request generator parameter instead of using
wjuan-AFK Jul 20, 2020
cded6ad
Changing to pass by reference.
wjuan-AFK Jul 20, 2020
fbfe523
Merge branch 'master' into RequestSourceUnitTestBenchmarkMultiplePath
wjuan-AFK Jul 20, 2020
0d70179
Merge branch 'master' into RequestSourceUnitTestBenchmarkMultiplePath
wjuan-AFK Jul 21, 2020
cdb7efe
Refactoring per comments in PR. Moving helpers, renaming functions.
wjuan-AFK Jul 21, 2020
f15bb47
Adding comments and renaming testbasicfunctionality for clarity
wjuan-AFK Jul 21, 2020
59359a2
Consistent unit test naming convention using should. Clarifying comments
wjuan-AFK Jul 21, 2020
f63df91
Fix format
wjuan-AFK Jul 21, 2020
406bd6b
Adding a clarifying comment for helper function.
wjuan-AFK Jul 22, 2020
f0b697b
Fixing typo and other nits
wjuan-AFK Jul 23, 2020
8425e48
Renaming testBenchmarkClient for clarity
wjuan-AFK Jul 23, 2020
fb4f372
Replacing Flat hash map with flat hash set for simplicity.
wjuan-AFK Jul 24, 2020
c6eae6c
Comment clarification.
wjuan-AFK Jul 24, 2020
96ae38b
Small refactor for factory method for more dependency injection.
wjuan-AFK Jul 27, 2020
b17240f
Fix format.
wjuan-AFK Jul 27, 2020
5c5a146
Refactor to inject the requestsourceconstructor.
wjuan-AFK Jul 27, 2020
29591ff
Moving helper functions.
wjuan-AFK Jul 27, 2020
7d928a9
Merge branch 'RequestSourceUnitTestBenchmarkMultiplePath' into Adding…
wjuan-AFK Jul 27, 2020
ff82d95
Fix format.
wjuan-AFK Jul 27, 2020
98381a4
Adding test using the new mock requestsourceconstructor to get remote
wjuan-AFK Jul 28, 2020
83ac556
Fix format.
wjuan-AFK Jul 28, 2020
38c5084
Adding missing request source constructor files.
wjuan-AFK Jul 28, 2020
fa4e285
Changing helper functions to anonymous namespace.
wjuan-AFK Jul 28, 2020
8551c76
Changing to use a nullable pointer instead of reference for clarity.
wjuan-AFK Jul 28, 2020
437de5e
Formatting fixes per PR.
wjuan-AFK Jul 28, 2020
6cef87e
Fix format.
wjuan-AFK Jul 28, 2020
c84ba4d
Merge branch 'master' into RequestSourceUnitTestBenchmarkMultiplePath
wjuan-AFK Jul 28, 2020
77d33f9
Fix format after merge.
wjuan-AFK Jul 29, 2020
73aa686
Testing a theory.
wjuan-AFK Jul 29, 2020
ef77d65
Fix format.
wjuan-AFK Jul 29, 2020
b9182a0
Merge branch 'RequestSourceUnitTestBenchmarkMultiplePath' into Adding…
wjuan-AFK Jul 29, 2020
e38dd80
Merge branch 'master' into RequestSourceUnitTestBenchmarkMultiplePath
wjuan-AFK Jul 29, 2020
21734e4
Merge branch 'RequestSourceUnitTestBenchmarkMultiplePath' into Adding…
wjuan-AFK Jul 29, 2020
7cda9d2
Merge branch 'master' into AddingTestForRequestSourceFactory
wjuan-AFK Aug 17, 2020
f827aca
Adding initial protos and stubs.
wjuan-AFK Aug 19, 2020
d194736
Adding impl for Dummy request source plugin.
wjuan-AFK Aug 19, 2020
7820cf9
Adding test for request source factory creation.
wjuan-AFK Aug 19, 2020
bee7805
Adding some more protos.
wjuan-AFK Aug 19, 2020
3db68ce
Merge branch 'master' into AddingTestForRequestSourceFactory
wjuan-AFK Aug 19, 2020
c0ec4dd
Merge branch 'AddingTestForRequestSourceFactory' into AddingConfigFac…
wjuan-AFK Aug 19, 2020
a1dd3f2
Fixing mockrequestsourcefactory impacted by the refactor.
wjuan-AFK Aug 20, 2020
0b25261
Adding more factories to the impl.h
wjuan-AFK Aug 20, 2020
c3fa23c
Fixing clang tidy.
wjuan-AFK Aug 20, 2020
547977e
Updating the impl.cc with a stub for the new factories.
wjuan-AFK Aug 20, 2020
c300203
Adding test for file based request source plugin
wjuan-AFK Aug 26, 2020
8f4d4d9
Adding test yaml file.
wjuan-AFK Aug 26, 2020
5ae2d51
Merge branch 'AddingTestForRequestSourceFactory' into AddingConfigFac…
wjuan-AFK Aug 26, 2020
fe74b6d
First try getting requests working.
wjuan-AFK Aug 27, 2020
6d87aea
Switching to using multiple requestOptions instead of one.
wjuan-AFK Sep 1, 2020
ca7a0c2
Removing unused comments.
wjuan-AFK Sep 2, 2020
967e3f8
Adding api dependency injection.
wjuan-AFK Sep 2, 2020
14b7d79
Getting the multiple paths parsing into the test.
wjuan-AFK Sep 3, 2020
c4985bb
Comment clean up and using loadfromfile instead of reading the file and
wjuan-AFK Sep 3, 2020
922639a
Switching to using iterators in a vector instead of a smart pointer.
wjuan-AFK Sep 9, 2020
81dfece
Removing unused comments.
wjuan-AFK Sep 9, 2020
bf08661
Small cleanup.
wjuan-AFK Sep 10, 2020
34900c5
Merge branch 'master' into AddingTestForRequestSourceFactory
wjuan-AFK Sep 10, 2020
768fa81
Merge branch 'AddingTestForRequestSourceFactory' into AddingConfigFac…
wjuan-AFK Sep 10, 2020
e93bb58
Reverting changes to RequestSourceConstructor.
wjuan-AFK Sep 10, 2020
f47cbd4
Removing RPCRequestSourcePlugin
wjuan-AFK Sep 10, 2020
7c5c6b2
Fix format.
wjuan-AFK Sep 10, 2020
1b02cec
Fix format.
wjuan-AFK Sep 10, 2020
4d75602
Fixing clang problems.
wjuan-AFK Sep 11, 2020
c74f535
Fixing clang.
wjuan-AFK Sep 11, 2020
9949fb8
Fix format.
wjuan-AFK Sep 11, 2020
fcf10d3
Fixing clang tidy.
wjuan-AFK Sep 11, 2020
fd9744d
Fix Format
wjuan-AFK Sep 11, 2020
68c1037
Rename requestOptionses to requestOptionsList.
wjuan-AFK Sep 14, 2020
883ee30
Renaming proto.
wjuan-AFK Sep 15, 2020
8c34b25
Updating to take a number of requests.
wjuan-AFK Sep 16, 2020
6ce7e74
Clean up tests.
wjuan-AFK Sep 16, 2020
92927e5
Changing visibility of RequestSourcePlugin's inheritance.
wjuan-AFK Sep 16, 2020
a62f2d5
Switching to using lock_guard.
wjuan-AFK Sep 16, 2020
16f89c6
Clean up.
wjuan-AFK Sep 16, 2020
f4257e3
Fix Format.
wjuan-AFK Sep 16, 2020
d075588
Addressing comments on PR. Cleaning up formatting.
wjuan-AFK Sep 17, 2020
eb10a3b
Getting rid of RequestSourcePluginPtr, and adding comments.
wjuan-AFK Sep 17, 2020
cdbd6bf
Removing unnecessary RequestSourcePlugin interface for improved testa…
wjuan-AFK Sep 17, 2020
17acc5f
Fix format
wjuan-AFK Sep 17, 2020
55a0e62
Cleanup unintentional whitespace.
wjuan-AFK Sep 18, 2020
fa23aad
Swapping to exception instead of Release_assert.
wjuan-AFK Sep 18, 2020
b8e2447
Renaming the request_source_plugin to request_source_plugin_config_fa…
wjuan-AFK Sep 18, 2020
248f439
Using a context struct for extensability.
wjuan-AFK Sep 22, 2020
91703b5
Spacing and comments.
wjuan-AFK Sep 22, 2020
bbc0c2a
Fix format.
wjuan-AFK Sep 22, 2020
517c83d
Adding test for multiple request sources thread lock.
wjuan-AFK Sep 22, 2020
c1d0428
Additional comment
wjuan-AFK Sep 22, 2020
bc86c09
Getting rid of context object because the Register Factory mechanism …
wjuan-AFK Sep 22, 2020
33e3564
Adding option to circumvent file reading.
wjuan-AFK Sep 23, 2020
338a874
Merge branch 'master' into AddingConfigFactory
wjuan-AFK Sep 23, 2020
402ada2
Fix format.
wjuan-AFK Sep 23, 2020
017481f
Getting rid of default constructors.
wjuan-AFK Sep 23, 2020
3ed43fc
Renames for clarity.
wjuan-AFK Sep 23, 2020
247b71b
Fix format.
wjuan-AFK Sep 23, 2020
171fb2e
Splitting factory off into a separate pr.
wjuan-AFK Sep 24, 2020
5df83ed
Proto cleanup/rename
wjuan-AFK Sep 24, 2020
0af3ffd
Comment cleanup.
wjuan-AFK Sep 24, 2020
ff90e59
Proto comments and dummy -> stub clean up.
wjuan-AFK Sep 24, 2020
e57c05f
Cleaning up comments
wjuan-AFK Sep 28, 2020
55244b0
Updating class comments.
wjuan-AFK Sep 28, 2020
c710d37
Refactor to move RequestSourcePlugins to a top level RequestSource fo…
wjuan-AFK Sep 28, 2020
fbb2fdc
Refactor plugins into separate files.
wjuan-AFK Sep 28, 2020
ae0f4a8
Fix format.
wjuan-AFK Sep 28, 2020
e092de0
Merge branch 'master' into AddingConfigFactory
wjuan-AFK Sep 28, 2020
19b7aa0
Adding gmock and gtest to check_format so it stops complaining at me.
wjuan-AFK Sep 28, 2020
dae3098
Updating to prefix increment, and removing auto.
wjuan-AFK Sep 29, 2020
a4b6fb1
Removing gmock and gtest from script to avoid conflict. Add Request_s…
wjuan-AFK Sep 29, 2020
55f685e
Fixing clang-tidy
wjuan-AFK Sep 30, 2020
59c5b94
Using the test value in the stub for better testing.
wjuan-AFK Sep 30, 2020
2ea0c5e
Getting rid of auto. And updating stubplugintest.
wjuan-AFK Sep 30, 2020
d40a073
Further renaming.
wjuan-AFK Sep 30, 2020
7146079
Refactor to move plugins into their own libraries and separate files.
wjuan-AFK Sep 30, 2020
7ab1132
Moving request_source_plugin_config_factory into its own folder.
wjuan-AFK Sep 30, 2020
72c23d0
Refactor to make it testonly.
wjuan-AFK Sep 30, 2020
2b1170c
Using references instead of making a copy of the option list.
wjuan-AFK Oct 1, 2020
9dc3c4b
Passing in a reference to api instead of apiptr.
wjuan-AFK Oct 1, 2020
b555a71
Renaming request_max to total_requests
wjuan-AFK Oct 1, 2020
3924eee
Updating comments and removing explicit.
wjuan-AFK Oct 1, 2020
75eca07
Fix format.
wjuan-AFK Oct 1, 2020
9c93ff9
Merge branch 'master' into AddingConfigFactory
wjuan-AFK Oct 1, 2020
e2af886
Clean up.
wjuan-AFK Oct 1, 2020
edae9be
Rename FilebasedConfigFactory to OptionListFromFileFactory
wjuan-AFK Oct 1, 2020
949d731
Improving Comments.
wjuan-AFK Oct 2, 2020
59d490f
Fix format.
wjuan-AFK Oct 2, 2020
078329a
More comment cleanup.
wjuan-AFK Oct 2, 2020
530cc9c
Adding proto request source factory.
wjuan-AFK Oct 7, 2020
22cc3f8
Merge branch 'master' into AddingProtoOnlyFactory
wjuan-AFK Oct 7, 2020
32489d0
Fix format.
wjuan-AFK Oct 7, 2020
324bb23
Fixing Clang tidy.
wjuan-AFK Oct 7, 2020
5d6bb26
Clarifying proto comments.
wjuan-AFK Oct 8, 2020
32ae13d
Rename, using optional, and moving functions in test.
wjuan-AFK Oct 8, 2020
12708ac
Need to stop forggetting to add blank lines or update the fix format …
wjuan-AFK Oct 8, 2020
eff2f4e
Replacing request with request1
wjuan-AFK Oct 8, 2020
b890df4
Renaming tests for clarity
wjuan-AFK Oct 8, 2020
a907913
Adding assertions for safety.
wjuan-AFK Oct 8, 2020
49a47a4
Clarifying comments on parameters
wjuan-AFK Oct 8, 2020
b711028
Fix format.
wjuan-AFK Oct 8, 2020
8a2d792
Renames based on feedback, and swapping to using uint32 instead of ui…
wjuan-AFK Oct 9, 2020
22b93aa
Fix format.
wjuan-AFK Oct 9, 2020
7b9c175
Cleaning up comments per feedback.
wjuan-AFK Oct 21, 2020
bab49fc
Clarifying comments.
wjuan-AFK Oct 21, 2020
fc7eda6
Removing implementation detail from comment.
wjuan-AFK Oct 22, 2020
49e6eec
Fix Format.
wjuan-AFK Oct 22, 2020
43de3d8
Merge branch 'master' into AddingProtoOnlyFactory
wjuan-AFK Oct 22, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion api/request_source/request_source_plugin.proto
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import "google/protobuf/wrappers.proto";
import "validate/validate.proto";
import "api/client/options.proto";

// Configuration for FileBasedPluginRequestSource (plugin name:
// Configuration for OptionsListFromFileRequestSourceFactory (plugin name:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be FileBasedPluginRequestSource

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this naming better expresses that they're both making an optionsListRequestSource.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean isn't this a typo? This appears to be the comment above FileBasedPluginConfig

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OHHH I didn't see File in the middle of the name, never mind!

// "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
Expand All @@ -25,6 +25,18 @@ message FileBasedPluginConfig {
google.protobuf.UInt32Value max_file_size = 3 [(validate.rules).uint32 = {lte: 1000000}];
}

// Configuration for OptionsListFromProtoRequestSourceFactory (plugin name:
// "nighthawk.in-line-options-list-request-source-plugin") that uses rpc
message InLinePluginConfig {
// The options_list will be used to generate Requests in the RequestSource.
nighthawk.client.RequestOptionsList options_list = 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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"If num_requests exceeds the number of RequestOptions in options_list, it will loop."

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch. Sorry about that one.

// limit on the number of requests to be produced.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might want to clarify that it will loop through the configured requests indefinitely but will terminate by normal mechanisms (duration of the benchmark elapsed, or some other failure or termination predicate was met).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. I think that's a fair comment.

google.protobuf.UInt32Value num_requests = 2 [(validate.rules).uint32 = {gte: 0, lte: 1000000}];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1,000,000 would be exhausted in only 100 seconds at 10,000 qps. Is there any reason to limit the range? This constraint could just be {gt: 0}, and the field being unset would indicate looping indefinitely. Or it could be a plain uint32 that is commented to be optional with a default of 0 meaning loop indefinitely.

We should also comment whether every field is required or optional.

}

// 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
Expand Down
33 changes: 32 additions & 1 deletion source/request_source/request_options_list_plugin_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,36 @@ RequestSourcePtr OptionsListFromFileRequestSourceFactory::createRequestSourcePlu

REGISTER_FACTORY(OptionsListFromFileRequestSourceFactory, RequestSourcePluginConfigFactory);

std::string OptionsListFromProtoRequestSourceFactory::name() const {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would name this request source consistently with its config proto.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this naming better expresses that they're both making an optionsListRequestSource. I would be open to renaming the configProto, but I'm not sure what a better name would be.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, I didn't realize the RequestSource plugin itself was the same.

It would be easier to understand for new readers if:

  • the purpose of the config protos within the system is obvious from the name, without having to look at the namespace
  • the plugin, factory, and config proto names were consistent with each other

What about:

  • OptionsListRequestSource
  • InlineOptionsListRequestSourceConfig
  • InlineOptionsListRequestSourceFactory
  • FileOptionsListRequestSourceConfig
  • FileOptionsListRequestSourceFactory
    ?

@wjuan-AFK wjuan-AFK Oct 9, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with this. I think this is sufficiently concise while also adding clarification. I had originally included these, but I think the way I had named it introduced parsing difficulty. This is much better.

return "nighthawk.in-line-options-list-request-source-plugin";
}

Envoy::ProtobufTypes::MessagePtr
OptionsListFromProtoRequestSourceFactory::createEmptyConfigProto() {
return std::make_unique<nighthawk::request_source::InLinePluginConfig>();
}

RequestSourcePtr OptionsListFromProtoRequestSourceFactory::createRequestSourcePlugin(
const Envoy::Protobuf::Message& message, Envoy::Api::Api&,
Envoy::Http::RequestHeaderMapPtr header) {
const auto& any = dynamic_cast<const Envoy::ProtobufWkt::Any&>(message);
nighthawk::request_source::InLinePluginConfig config;
Envoy::MessageUtil::unpackTo(any, config);
// Locking to avoid issues with multiple threads calling this at the same time and trying to set
// the options_list_
{
Envoy::Thread::LockGuard lock_guard(options_list_lock_);
// Only loading the config into memory the first time.
if (options_list_.options_size() == 0) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: I think that relying on absl::optional here to express if options_list_ was populated slightly simplifies things when considering what happens in the edge case where the passed message actually is empty (I think right now this will repeatedly set options_list_. I see no harm in it, but it seems different from the regular flow).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense to me. Thanks for the suggestion.

options_list_ = config.options_list();
}
}
return std::make_unique<RequestOptionsListRequestSource>(config.num_requests().value(),
std::move(header), options_list_);
}

REGISTER_FACTORY(OptionsListFromProtoRequestSourceFactory, RequestSourcePluginConfigFactory);

RequestOptionsListRequestSource::RequestOptionsListRequestSource(
const uint32_t total_requests, Envoy::Http::RequestHeaderMapPtr header,
const nighthawk::client::RequestOptionsList& options_list)
Expand Down Expand Up @@ -72,7 +102,8 @@ RequestGenerator RequestOptionsListRequestSource::get() {
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);
header->setContentLength(
content_length); // Content length is used later in stream_decoder to populate the body
}
for (const envoy::config::core::v3::HeaderValueOption& option_header :
request_option.request_headers()) {
Expand Down
32 changes: 32 additions & 0 deletions source/request_source/request_options_list_plugin_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -81,4 +81,36 @@ class OptionsListFromFileRequestSourceFactory : public virtual RequestSourcePlug
// This factory will be activated through RequestSourceFactory in factories.h
DECLARE_FACTORY(OptionsListFromFileRequestSourceFactory);

// Factory that creates a RequestOptionsListRequestSource from a InLinePluginConfig proto.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is RequestOptionsListRequestSource an old name?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it isn't, but you're not the first person to ask that. I have two different factories that are both making a requestOptionsListRequestSource, one of them is the OptionsListFromFileRequestSourceFactory and the other is the OptionsListFromProtoRequestSourceFactory. I can get rid of the word Request at the front of the RequestOptionsListRequestSource and hopefully that makes it easier.

// Registered as an Envoy plugin.
// Implementation of RequestSourceConfigFactory which produces a RequestSource that keeps an
// RequestOptionsList in memory, and loads it with the RequestOptions passed to it from the config.
// All plugins configuration are specified in the request_source_plugin.proto. 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.in-line-options-list-request-source-plugin");
// RequestSourcePtr plugin =
// config_factory.createRequestSourcePlugin(config, std::move(api), std::move(header));

class OptionsListFromProtoRequestSourceFactory : public virtual RequestSourcePluginConfigFactory {
public:
std::string name() const override;
Envoy::ProtobufTypes::MessagePtr createEmptyConfigProto() override;

// This implementation is not thread safe. Only the first call to createRequestSourcePlugin will

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought the lock would make it thread safe.

Also, if we're making a copy anyway, can we get rid of the lock and just pass the entire unpacked InLinePluginConfig into the OptionsListFromProtoRequestSource constructor?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To add clarification.
The lock doesn't quite make it thread safe except in practice. Because theoretically there COULD be a race condition in which optionsList will be saved, but in practice, every config that's passed in is the same config, so this can't actually occur.

The reason we're passing this by reference and doing this locking thing at all, is because I wanted to reuse the logic for the OptionsListRequestSource, and since the FileBased implementation passes by reference for space reasons, it made sense to do the same here.

// load the options list into memory and subsequent calls just make a copy of the options_list
// that was already loaded.
RequestSourcePtr createRequestSourcePlugin(const Envoy::Protobuf::Message& message,
Envoy::Api::Api& api,
Envoy::Http::RequestHeaderMapPtr header) override;

private:
Envoy::Thread::MutexBasicLockable options_list_lock_;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are saying the class is not thread safe, why do we need a lock? Once we proclaim it thread-unsafe, nobody should be calling it from multiple threads.

If we do include a lock, we should use it to its intended purpose which is to make the class thread safe. Maybe I am misunderstanding it, but looks like we are in a hybrid state where we do pay the cost of locking for no benefit.

Taking a step back, we should probably first agree whether we need this class to be thread safe. If you think we do, can you help by describing a scenario where this needs to be accessed by multiple threads? Once we understand that, we can discuss how to design it to be compatible with the intended use.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be thread safe. Sorry for the confusion.

nighthawk::client::RequestOptionsList options_list_;
};

// This factory will be activated through RequestSourceFactory in factories.h
DECLARE_FACTORY(OptionsListFromProtoRequestSourceFactory);

} // namespace Nighthawk
106 changes: 105 additions & 1 deletion test/request_source/request_source_plugin_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ namespace Nighthawk {

namespace {
using nighthawk::request_source::FileBasedPluginConfig;
using nighthawk::request_source::InLinePluginConfig;
using nighthawk::request_source::StubPluginConfig;
using ::testing::NiceMock;
using ::testing::Test;
Expand All @@ -43,6 +44,19 @@ class FileBasedRequestSourcePluginTest : public Test {
}
};

class InLineRequestSourcePluginTest : public Test {
public:
InLineRequestSourcePluginTest() : api_(Envoy::Api::createApiForTest(stats_store_)) {}
Envoy::Stats::MockIsolatedStatsStore stats_store_;
Envoy::Api::ApiPtr api_;
nighthawk::request_source::InLinePluginConfig
MakeInLinePluginConfig(nighthawk::client::RequestOptionsList options_list, int num_requests) {
nighthawk::request_source::InLinePluginConfig config;
*config.mutable_options_list() = std::move(options_list);
config.mutable_num_requests()->set_value(num_requests);
return config;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would make this a standalone function to clarify that it doesn't need any of the InLineRequestSourcePluginTest state.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok.

};
TEST_F(StubRequestSourcePluginTest, CreateEmptyConfigProtoCreatesCorrectType) {
auto& config_factory =
Envoy::Config::Utility::getAndCheckFactoryByName<RequestSourcePluginConfigFactory>(
Expand Down Expand Up @@ -155,7 +169,6 @@ TEST_F(FileBasedRequestSourcePluginTest,
CreateRequestSourcePluginWithMoreNumRequestsThanInFileGetsWorkingRequestGeneratorThatLoops) {
nighthawk::request_source::FileBasedPluginConfig config = MakeFileBasedPluginConfigWithTestYaml(
TestEnvironment::runfilesPath("test/request_source/test_data/test-config.yaml"));
config.mutable_num_requests()->set_value(4);
Envoy::ProtobufWkt::Any config_any;
config_any.PackFrom(config);
auto& config_factory =
Expand All @@ -175,5 +188,96 @@ TEST_F(FileBasedRequestSourcePluginTest,
EXPECT_EQ(header2->getPathValue(), "/b");
EXPECT_EQ(header3->getPathValue(), "/a");
}

TEST_F(InLineRequestSourcePluginTest, CreateEmptyConfigProtoCreatesCorrectType) {
auto& config_factory =
Envoy::Config::Utility::getAndCheckFactoryByName<RequestSourcePluginConfigFactory>(
"nighthawk.in-line-options-list-request-source-plugin");
const Envoy::ProtobufTypes::MessagePtr empty_config = config_factory.createEmptyConfigProto();
const nighthawk::request_source::InLinePluginConfig expected_config;
EXPECT_EQ(empty_config->DebugString(), expected_config.DebugString());
EXPECT_TRUE(Envoy::MessageUtil()(*empty_config, expected_config));
}

TEST_F(InLineRequestSourcePluginTest, FactoryRegistrationUsesCorrectPluginName) {
nighthawk::request_source::InLinePluginConfig config;
Envoy::ProtobufWkt::Any config_any;
config_any.PackFrom(config);
auto& config_factory =
Envoy::Config::Utility::getAndCheckFactoryByName<RequestSourcePluginConfigFactory>(
"nighthawk.in-line-options-list-request-source-plugin");
EXPECT_EQ(config_factory.name(), "nighthawk.in-line-options-list-request-source-plugin");
}

TEST_F(InLineRequestSourcePluginTest, CreateRequestSourcePluginCreatesCorrectPluginType) {
Envoy::MessageUtil util;
nighthawk::client::RequestOptionsList options_list;
util.loadFromFile(TestEnvironment::runfilesPath("test/request_source/test_data/test-config.yaml"),
options_list, Envoy::ProtobufMessage::getStrictValidationVisitor(), *api_,
true);
nighthawk::request_source::InLinePluginConfig config = MakeInLinePluginConfig(options_list, 2);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be /*num_requests=*/2 here and below

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood.

Envoy::ProtobufWkt::Any config_any;
config_any.PackFrom(config);
auto& config_factory =
Envoy::Config::Utility::getAndCheckFactoryByName<RequestSourcePluginConfigFactory>(
"nighthawk.in-line-options-list-request-source-plugin");
auto header = Envoy::Http::RequestHeaderMapImpl::create();
RequestSourcePtr plugin =
config_factory.createRequestSourcePlugin(config_any, *api_, std::move(header));
EXPECT_NE(dynamic_cast<RequestOptionsListRequestSource*>(plugin.get()), nullptr);
}
TEST_F(InLineRequestSourcePluginTest,
Comment on lines +228 to +245

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: add blank line

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

CreateRequestSourcePluginGetsWorkingRequestGeneratorThatEndsAtNumRequest) {
Envoy::MessageUtil util;
nighthawk::client::RequestOptionsList options_list;
util.loadFromFile(TestEnvironment::runfilesPath("test/request_source/test_data/test-config.yaml"),
options_list, Envoy::ProtobufMessage::getStrictValidationVisitor(), *api_,
true);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you comment this parameter name, /*the_parameter_name=*/true?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing. Added clarifying comments throughout.

nighthawk::request_source::InLinePluginConfig config = MakeInLinePluginConfig(options_list, 2);
Envoy::ProtobufWkt::Any config_any;
config_any.PackFrom(config);
auto& config_factory =
Envoy::Config::Utility::getAndCheckFactoryByName<RequestSourcePluginConfigFactory>(
"nighthawk.in-line-options-list-request-source-plugin");
auto header = Envoy::Http::RequestHeaderMapImpl::create();
RequestSourcePtr plugin =
config_factory.createRequestSourcePlugin(config_any, *api_, std::move(header));
Nighthawk::RequestGenerator generator = plugin->get();
Nighthawk::RequestPtr request = generator();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

request1

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure.

Nighthawk::RequestPtr request2 = generator();
Nighthawk::RequestPtr request3 = generator();
Nighthawk::HeaderMapPtr header1 = request->header();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should do e.g. ASSERT_NE(request2, nullptr); before dereferencing

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense.

Nighthawk::HeaderMapPtr header2 = request2->header();
EXPECT_EQ(header1->getPathValue(), "/a");
EXPECT_EQ(header2->getPathValue(), "/b");
EXPECT_EQ(request3, nullptr);
}
TEST_F(InLineRequestSourcePluginTest,
CreateRequestSourcePluginWithMoreNumRequestsThanInFileGetsWorkingRequestGeneratorThatLoops) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe the reference to File is stale here; also the looping is the main point, might not need to mention that it's a working request generator

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense.

Envoy::MessageUtil util;
nighthawk::client::RequestOptionsList options_list;
util.loadFromFile(TestEnvironment::runfilesPath("test/request_source/test_data/test-config.yaml"),
options_list, Envoy::ProtobufMessage::getStrictValidationVisitor(), *api_,
true);
nighthawk::request_source::InLinePluginConfig config = MakeInLinePluginConfig(options_list, 4);
Envoy::ProtobufWkt::Any config_any;
config_any.PackFrom(config);
auto& config_factory =
Envoy::Config::Utility::getAndCheckFactoryByName<RequestSourcePluginConfigFactory>(
"nighthawk.in-line-options-list-request-source-plugin");
auto header = Envoy::Http::RequestHeaderMapImpl::create();
RequestSourcePtr plugin =
config_factory.createRequestSourcePlugin(config_any, *api_, std::move(header));
Nighthawk::RequestGenerator generator = plugin->get();
Nighthawk::RequestPtr request = generator();
Nighthawk::RequestPtr request2 = generator();
Nighthawk::RequestPtr request3 = generator();
Nighthawk::HeaderMapPtr header1 = request->header();
Nighthawk::HeaderMapPtr header2 = request2->header();
Nighthawk::HeaderMapPtr header3 = request3->header();
EXPECT_EQ(header1->getPathValue(), "/a");
EXPECT_EQ(header2->getPathValue(), "/b");
EXPECT_EQ(header3->getPathValue(), "/a");
}
} // namespace
} // namespace Nighthawk