Adding config factory by wjuan-AFK · Pull Request #522 · envoyproxy/nighthawk

wjuan-AFK · 2020-09-10T18:57:47Z

Adding config factories for RequestSourcePlugins. Specifically starting with FileBasedRequestSourcePlugin. These will be provided as options to be used inside RequestSourceFactory. This is part of a series of PRs for providing the ability to use statically linked requestSources.

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

a global request generator. Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Adding comments. Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

…TestForRequestSourceFactory Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

request source. Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

wjuan-AFK · 2020-09-30T06:04:26Z

source/request_source/request_options_list_plugin_impl.cc

+    }
+
+    // Increment the counter and get the request_option from the list for the current iteration.
+    auto index = lambda_counter % options_list_->options_size();


Understood.

wjuan-AFK · 2020-09-30T06:04:35Z

source/request_source/request_options_list_plugin_impl.cc

+    // 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++;


wjuan-AFK · 2020-09-30T06:04:43Z

source/request_source/request_options_list_plugin_impl.cc

+    if (content_length > 0) {
+      header->setContentLength(content_length);
+    }
+    for (const auto& option_header : request_option.request_headers()) {


Understood.

wjuan-AFK · 2020-09-30T06:19:29Z

source/request_source/request_options_list_plugin_impl.cc

+      request_max_(request_max) {}
+
+RequestGenerator RequestOptionsListRequestSource::get() {
+  uint32_t counter = 0;


Yes, we can do that.

wjuan-AFK · 2020-09-30T06:22:21Z

source/request_source/request_options_list_plugin_impl.cc

+      util.loadFromFile(config.file_path(), options_list_,
+                        Envoy::ProtobufMessage::getStrictValidationVisitor(), *api, true);
+    }
+    temp_list->CopyFrom(options_list_);


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.

wjuan-AFK · 2020-10-01T05:40:01Z

source/request_source/request_source_plugin_impl.h

+  const uint32_t request_max_;
+};
+
+// Factory that creates a RequestOptionsListRequestSource from a FileBasedPluginConfig proto.


Not a typo. I wanted this to be a more generic version of the RequestOptionsListRequestSource since it seemed the more versatile option.

wjuan-AFK · 2020-10-01T05:41:09Z

source/request_source/request_source_plugin_impl.h

+// 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 {


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.

wjuan-AFK · 2020-10-01T05:41:29Z

source/request_source/request_source_plugin_impl.h

+  // 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,


wjuan-AFK · 2020-10-01T05:42:24Z

test/adaptive_load/adaptive_load_controller_test.cc

 #include "api/client/service.pb.h"
 #include "api/client/service_mock.grpc.pb.h"

+#include "gtest/gtest.h"


Hopefully I've reverted the change that introduced these.

wjuan-AFK · 2020-10-01T05:44:31Z

source/request_source/request_source_plugin_impl.h

+
+// Factory that creates a RequestOptionsListRequestSource from a FileBasedPluginConfig proto.
+// Registered as an Envoy plugin.
+// Implementation of RequestSourceConfigFactory which produces a RequestSource that keeps an


I'm not sure I'm doing a very good job at this, but I'll do my best.

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

mum4k

Looks good, thank you for iterating on this. This is now a complete review and we only have few minor comments left to address.

mum4k · 2020-10-01T20:43:39Z

test/request_source/stub_plugin_impl.h

@@ -0,0 +1,55 @@
+// Implementations of RequestSourceConfigFactory and the RequestSources that those factories make.


We probably want to say something else here, i.e. we should talk about the stub plugin which is implemented in this file.

mum4k · 2020-10-01T20:47:11Z

api/request_source/request_source_plugin.proto

+// mechanism using a minimal version of plugin that does not require a more complicated proto or
+// file reading.
+message StubPluginConfig {
+  // test input value.


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.

Oh I thought I fixed it by making it do something.

test/request_source/request_source_plugin_test.cc

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

wjuan-AFK · 2020-10-05T14:55:57Z

/retest

repokitteh-read-only · 2020-10-05T14:56:00Z

🔨 rebuilding ci/circleci: clang_tidy (failed build)

🐱

Caused by: a #522 (comment) was created by @wjuan-AFK.

see: more, trace.

wjuan-AFK added 30 commits July 15, 2020 20:04

Adding a multiple path test to confirm that expected paths are hit

99f45d1

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

fixing asan problems

075a4cf

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Fix Format

ecbf407

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Removeing unused comments

8fb11b6

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Removing unnecessary invoke calls for cleanliness

032dbba

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Merge branch 'master' into RequestSourceUnitTestBenchmarkMultiplePath

953e953

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Refactoring test to take in request generator parameter instead of using

6185edb

a global request generator. Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Changing to pass by reference.

cded6ad

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Merge branch 'master' into RequestSourceUnitTestBenchmarkMultiplePath

fbfe523

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Merge branch 'master' into RequestSourceUnitTestBenchmarkMultiplePath

0d70179

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Refactoring per comments in PR. Moving helpers, renaming functions.

cdb7efe

Adding comments. Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Adding comments and renaming testbasicfunctionality for clarity

f15bb47

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Consistent unit test naming convention using should. Clarifying comments

59359a2

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Fix format

f63df91

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Adding a clarifying comment for helper function.

406bd6b

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Fixing typo and other nits

f0b697b

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Renaming testBenchmarkClient for clarity

8425e48

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Replacing Flat hash map with flat hash set for simplicity.

fb4f372

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Comment clarification.

c6eae6c

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Small refactor for factory method for more dependency injection.

96ae38b

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Fix format.

b17240f

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Refactor to inject the requestsourceconstructor.

5c5a146

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Moving helper functions.

29591ff

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Merge branch 'RequestSourceUnitTestBenchmarkMultiplePath' into Adding…

7d928a9

…TestForRequestSourceFactory Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Fix format.

ff82d95

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Adding test using the new mock requestsourceconstructor to get remote

98381a4

request source. Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Fix format.

83ac556

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Adding missing request source constructor files.

38c5084

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Changing helper functions to anonymous namespace.

fa4e285

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Changing to use a nullable pointer instead of reference for clarity.

8551c76

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

wjuan-AFK added 8 commits September 30, 2020 18:17

Further renaming.

d40a073

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Refactor to move plugins into their own libraries and separate files.

7146079

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Moving request_source_plugin_config_factory into its own folder.

7ab1132

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Refactor to make it testonly.

72c23d0

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Using references instead of making a copy of the option list.

2b1170c

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Passing in a reference to api instead of apiptr.

9dc3c4b

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Renaming request_max to total_requests

b555a71

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Updating comments and removing explicit.

3924eee

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

wjuan-AFK commented Oct 1, 2020

View reviewed changes

wjuan-AFK requested a review from mum4k October 1, 2020 05:46

wjuan-AFK added waiting-for-review A PR waiting for a review. and removed waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. labels Oct 1, 2020

wjuan-AFK added 4 commits October 1, 2020 15:58

Fix format.

75eca07

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Merge branch 'master' into AddingConfigFactory

9c93ff9

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Clean up.

e2af886

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Rename FilebasedConfigFactory to OptionListFromFileFactory

edae9be

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

mum4k requested changes Oct 1, 2020

View reviewed changes

mum4k added waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. and removed waiting-for-review A PR waiting for a review. labels Oct 1, 2020

oschaaf mentioned this pull request Oct 1, 2020

Add safe proto diff helper #433

Closed

wjuan-AFK added 3 commits October 2, 2020 16:40

Improving Comments.

949d731

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Fix format.

59d490f

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

More comment cleanup.

078329a

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

wjuan-AFK added waiting-for-review A PR waiting for a review. and removed waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. labels Oct 5, 2020

mum4k approved these changes Oct 5, 2020

View reviewed changes

mum4k merged commit a358469 into envoyproxy:master Oct 5, 2020

wjuan-AFK deleted the AddingConfigFactory branch June 9, 2021 15:26

		@@ -0,0 +1,55 @@
		// Implementations of RequestSourceConfigFactory and the RequestSources that those factories make.

Conversation

wjuan-AFK commented Sep 10, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mum4k left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wjuan-AFK commented Oct 5, 2020

Uh oh!

repokitteh-read-only bot commented Oct 5, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants