ghz icon indicating copy to clipboard operation
ghz copied to clipboard

Support for specifying an array of metadata objects to use for the outgoing requests

Open Kami opened this issue 3 years ago • 3 comments

This pull request updates the code so in addition to specifying a single metadata object which is used for all the outgoing gRPC calls / requests, user can also specify a list of metadata objects to use for the outgoing requests.

This allows people to use different metadata for different requests. It's similar to the change implemented in #87, but this one is for the actual request metadata and for the payload / body / data.

Background, Context, Rationale

#87 implemented support which allows users to specify different messages for unary calls. Those messages are then used in a round robin fashion for the outgoing requests (thanks to @ezsilmar for implementing that).

That functionality comes handy in many scenarios where sending the same data with every request will not results in a representative result (e.g. due to the server logic, caching or similar).

In our case, actual processing performed by the server also depends on the metadata field values.

This means if we want to get a representative result when performing simple gRPC server level benchmark using this tool, we need to send different (and specific) metadata for each outgoing request.

And in that specific case we can't utilize call template data functionality since that data is not available in the template context.

Proposed Implementation

This PR implements a change which allows user to either specify an object directly or an array of objects for the metadata argument.

If an array is specified, we will use different metadata values for different outgoing requests in a round-robin fashion.

This follows similar approach which is already used for the actual messages and was implemented in #86.

Proposed implementation in this PR is just a quick WIP version of this change.

If other people agree this is indeed a good feature / functionality to have, I will clean it up and finish it.

Open Questions

Making sure the change is fully backward compatible (aka that both approaches - single object and an array with objects is supported) is pretty straight forward when metadata is either specified directly as a command line argument or read from a file.

This becomes more problematic though when reading configuration from a JSON or TOML configuration file.

One approach I could think of is to "rewrite" config on load and ensure metadata field value is always an array. This approach is somewhat fragile not ideal so I wonder if there is a better way to handle that (suggestions and feedback is welcome).

Another approach is to have two Config struct definitions - one of the map and one of the array of maps and then internally after parsing we make sure we always convert that field to an array.

EDIT: I pushed a change so we use the same generic approach as we use for the Data argument - this way we can handle this change config wise in a fully backward compatible manner.

TODO

  • [ ] Finish the implementation
    • [x] Make sure the change is fully backward compatible (ensure we support both notations - single object or an array for objects for metadata specified either via command line or config file)
    • [ ] Clean up the code
    • [x] Tests
    • [x] Update docs
    • [x] Update examples
    • [x] Update readme

Kami avatar Oct 20 '20 15:10 Kami

Alright I pushed additional changes:

  • The change is now fully backward compatible not matter how you specify the data (command line argument with actual values, command line argument with file path, json / yaml / toml config file)
  • Added end to end tests
  • Updated readme, docs and examples

I believe functionality wise this should be more or less complete, I just need to clean up and refactor some duplicated code.

If I missed something or some place, please let me know.

Kami avatar Oct 21 '20 10:10 Kami

Hello, thank you for the PR and the detailed description! I understand the problem, and I think this is a sensible approach. I will probably need another read through, but I have left a few comments to double check on from my initial review. I'll probably re-read again when I get some more time. Once we get these addressed and when you feel you've cleaned up the code we can get this merged. Thanks again!

bojand avatar Oct 22 '20 21:10 bojand

@bojand Sorry for the delay - I some how missed your review feedback.

I'll try to address this review feedback this weekend (I've been using those changes for a while now, but I do agree that it needs more clean up, etc. :)).

Kami avatar Jan 06 '21 12:01 Kami