Repository: bojand/ghz
Pull request #236 (2020-10-22 09:49:36)

[feature] [option] adding a func as a option to randomize each request 
This PR adds the ability to supply a `changeFunc` function for every request to ensure we can randomize data.
Solves: https://github.com/bojand/ghz/issues/197

I am using this as an internal library, before adding more e.g. to the CLI as well as updating the documentation, I wanted to clarify whether this solution makes sense implementation wise and just have a discussion in general 👍 



bojand: Hello, thanks for the PR! 

The current package API really came more about historically as the CLI API was adapted to expose something that can be used programatically. I think in some ways there is room for improvement from package API perspective. I agree a more generalized functional API to provide data (and metadata) would be useful. It's something I've thought about for a while, and ideally really most of the other parameter options should just result in a functional "data provider", something as conceptualized here. So this is a good start I think!

One comment I have is that perhaps it would be beneficial to expose [`callTemplateData`](https://ghz.sh/docs/calldata) data publicly perhaps, and the function signature could be something like:

```
dataFunc func(mtd *desc.MethodDescriptor, callData *ghz.CallTemplateData) []byte
```
This would allow for clients to utilize the call data, as it may be helpful to have it:

```
// not sure what is better order of params, descriptor first and then data, or other way around?
func changeFunc(mtd *desc.MethodDescriptor, callData *ghz.CallTemplateData) []byte {
	msg := &helloworld.HelloRequest{}
	msg.Name = callData.WorkerID // now we can use call data
	binData, _ := proto.Marshal(msg)
	return binData
}
```
Also just thinking some more, maybe the exposed struct name should be `CallData` perhaps... That might be more appropriate, rather than `CallTemplateData`, but I don't feel too strongly.

Thoughts on all of this?

Thanks again for the PR, this looks like it should be beneficial!
nammn: That sounds great and happy to hear that. Yes, both suggestions make sense. Let me add them 👍 

I think that could potentially include a bigger documentation change as this means that `callTemplateData` can also be used for binary requests. What do you think? Can we add this PR without that documentation change? 

I planned to make this only used for the go package, does this work for you? Or do you plan to have this as a CLI Option as well? 🤔 
bojand: Hello, I am not sure what documentation changes you're referring to, it might be ambiguous. Sorry maybe I caused more confusion than necessary with some of my comments heh. All I was suggesting is:

- Rename `callTemplateData` struct to `CallData`. With respect to code, this should result in changes only to `call_template_data.go`, and `worker.go` files. If you're using VSCode and official Go extension, "Rename Symbol" action in menu seems to do a fine job with this. You do not have to bother with fixing documentation if you do not wish like the markdown files that reference the old struct name. No problem I can address updating the docs for that.
- Apply the new publicly exposed `CallData` struct to your proposed changes so that the data function has signature:

```go
dataFunc func(mtd *desc.MethodDescriptor, callData *runner.CallData) []byte
```

Do not bother trying to document anything extra besides the godoc you already have. Also don't try to somehow expose this via CLI.

I think these changes I propose here are reasonable and should be sufficient for the functionality you need? 
nammn: I did both changes. Checks are passing. Ready for a review 👍 

Patch:
diff --git a/runner/call_template_data.go b/runner/call_template_data.go
index be95f6f..0656fe6 100644
--- a/runner/call_template_data.go
+++ b/runner/call_template_data.go
@@ -18,7 +18,7 @@ var seededRand *rand.Rand = rand.New(
 	rand.NewSource(time.Now().UnixNano()))
 
 // call template data
-type callTemplateData struct {
+type CallData struct {
 	WorkerID           string // unique worker ID
 	RequestNumber      int64  // unique incremented request number for each request
 	FullyQualifiedName string // fully-qualified name of the method call
@@ -42,11 +42,11 @@ var tmplFuncMap = template.FuncMap{
 	"randomString": randomString,
 }
 
-// newCallTemplateData returns new call template data
-func newCallTemplateData(
+// newCallData returns new callData
+func newCallData(
 	mtd *desc.MethodDescriptor,
 	funcs template.FuncMap,
-	workerID string, reqNum int64) *callTemplateData {
+	workerID string, reqNum int64) *CallData {
 	now := time.Now()
 	newUUID, _ := uuid.NewRandom()
 
@@ -61,7 +61,7 @@ func newCallTemplateData(
 		}
 	}
 
-	return &callTemplateData{
+	return &CallData{
 		WorkerID:           workerID,
 		RequestNumber:      reqNum,
 		FullyQualifiedName: mtd.GetFullyQualifiedName(),
@@ -80,14 +80,14 @@ func newCallTemplateData(
 	}
 }
 
-func (td *callTemplateData) execute(data string) (*bytes.Buffer, error) {
+func (td *CallData) execute(data string) (*bytes.Buffer, error) {
 	t := template.Must(template.New("call_template_data").Funcs(td.templateFuncs).Parse(data))
 	var tpl bytes.Buffer
 	err := t.Execute(&tpl, td)
 	return &tpl, err
 }
 
-func (td *callTemplateData) executeData(data string) ([]byte, error) {
+func (td *CallData) executeData(data string) ([]byte, error) {
 	if len(data) > 0 {
 		input := []byte(data)
 		tpl, err := td.execute(data)
@@ -101,7 +101,7 @@ func (td *callTemplateData) executeData(data string) ([]byte, error) {
 	return []byte{}, nil
 }
 
-func (td *callTemplateData) executeMetadata(metadata string) (map[string]string, error) {
+func (td *CallData) executeMetadata(metadata string) (map[string]string, error) {
 	var mdMap map[string]string
 
 	if len(metadata) > 0 {
diff --git a/runner/options.go b/runner/options.go
index 45706b4..9a73e77 100644
--- a/runner/options.go
+++ b/runner/options.go
@@ -15,6 +15,7 @@ import (
 	"text/template"
 	"time"
 
+	"github.com/jhump/protoreflect/desc"
 	"github.com/pkg/errors"
 	"google.golang.org/grpc/credentials"
 )
@@ -58,7 +59,11 @@ type RunConfig struct {
 	streamInterval time.Duration
 
 	// data
-	data     []byte
+	data []byte
+
+	// data func
+	dataFunc func(mtd *desc.MethodDescriptor, callData *CallData) []byte
+
 	binary   bool
 	metadata []byte
 	rmd      map[string]string
@@ -300,6 +305,17 @@ func WithBinaryData(data []byte) Option {
 	}
 }
 
+// WithBinaryDataFunc specifies the binary data func which will be called on each request
+//  WithBinaryDataFunc(changeFunc)
+func WithBinaryDataFunc(data func(mtd *desc.MethodDescriptor, callData *CallData) []byte) Option {
+	return func(o *RunConfig) error {
+		o.dataFunc = data
+		o.binary = true
+
+		return nil
+	}
+}
+
 // WithBinaryDataFromFile specifies the binary data
 //	WithBinaryDataFromFile("request_data.bin")
 func WithBinaryDataFromFile(path string) Option {
@@ -557,7 +573,7 @@ func WithLogger(log Logger) Option {
 	}
 }
 
-// WithTemplateFuncs adds additional tempalte functions
+// WithTemplateFuncs adds additional template functions
 func WithTemplateFuncs(funcMap template.FuncMap) Option {
 	return func(o *RunConfig) error {
 		o.funcs = funcMap
diff --git a/runner/worker.go b/runner/worker.go
index f5a8fb6..b581c17 100644
--- a/runner/worker.go
+++ b/runner/worker.go
@@ -65,7 +65,7 @@ func (w *Worker) makeRequest() error {
 
 	reqNum := atomic.AddInt64(w.reqCounter, 1)
 
-	ctd := newCallTemplateData(w.mtd, w.config.funcs, w.workerID, reqNum)
+	ctd := newCallData(w.mtd, w.config.funcs, w.workerID, reqNum)
 
 	var inputs []*dynamic.Message
 	var err error
@@ -167,7 +167,7 @@ func (w *Worker) makeRequest() error {
 	return err
 }
 
-func (w *Worker) getMessages(ctd *callTemplateData, inputData []byte) ([]*dynamic.Message, error) {
+func (w *Worker) getMessages(ctd *CallData, inputData []byte) ([]*dynamic.Message, error) {
 	var inputs []*dynamic.Message
 
 	if w.cachedMessages != nil {
@@ -186,12 +186,17 @@ func (w *Worker) getMessages(ctd *callTemplateData, inputData []byte) ([]*dynami
 		// Json messages are not cached due to templating
 	} else {
 		var err error
+		if w.config.dataFunc != nil {
+			inputData = w.config.dataFunc(w.mtd, ctd)
+		}
 		inputs, err = createPayloadsFromBin(inputData, w.mtd)
 		if err != nil {
 			return nil, err
 		}
-
-		w.cachedMessages = inputs
+		// We only cache in case we don't dynamically change the binary message
+		if w.config.dataFunc == nil {
+			w.cachedMessages = inputs
+		}
 	}
 
 	return inputs, nil


Test patch:
diff --git a/runner/call_template_data_test.go b/runner/call_template_data_test.go
index 2807468..bdf2897 100644
--- a/runner/call_template_data_test.go
+++ b/runner/call_template_data_test.go
@@ -15,7 +15,7 @@ func TestCallTemplateData_New(t *testing.T) {
 	assert.NoError(t, err)
 	assert.NotNil(t, md)
 
-	ctd := newCallTemplateData(md, nil, "worker_id_123", 100)
+	ctd := newCallData(md, nil, "worker_id_123", 100)
 
 	assert.NotNil(t, ctd)
 	assert.Equal(t, "worker_id_123", ctd.WorkerID)
@@ -41,7 +41,7 @@ func TestCallTemplateData_ExecuteData(t *testing.T) {
 	assert.NoError(t, err)
 	assert.NotNil(t, md)
 
-	ctd := newCallTemplateData(md, nil, "worker_id_123", 200)
+	ctd := newCallData(md, nil, "worker_id_123", 200)
 
 	assert.NotNil(t, ctd)
 
@@ -92,7 +92,7 @@ func TestCallTemplateData_ExecuteMetadata(t *testing.T) {
 	assert.NoError(t, err)
 	assert.NotNil(t, md)
 
-	ctd := newCallTemplateData(md, nil, "worker_id_123", 200)
+	ctd := newCallData(md, nil, "worker_id_123", 200)
 
 	assert.NotNil(t, ctd)
 
@@ -138,7 +138,7 @@ func TestCallTemplateData_ExecuteFuncs(t *testing.T) {
 	assert.NoError(t, err)
 	assert.NotNil(t, md)
 
-	ctd := newCallTemplateData(md, nil, "worker_id_123", 200)
+	ctd := newCallData(md, nil, "worker_id_123", 200)
 
 	assert.NotNil(t, ctd)
 
@@ -249,7 +249,7 @@ func TestCallTemplateData_ExecuteFuncs(t *testing.T) {
 	})
 
 	t.Run("custom functions", func(t *testing.T) {
-		ctd = newCallTemplateData(md, template.FuncMap{
+		ctd = newCallData(md, template.FuncMap{
 			"getSKU": func() string {
 				return "custom-sku"
 			},
diff --git a/runner/options_test.go b/runner/options_test.go
index 844e581..b459821 100644
--- a/runner/options_test.go
+++ b/runner/options_test.go
@@ -4,6 +4,7 @@ import (
 	"encoding/json"
 	"math"
 	"os"
+	"reflect"
 	"runtime"
 	"testing"
 	"time"
@@ -124,6 +125,7 @@ func TestRunConfig_newRunConfig(t *testing.T) {
 			WithDialTimeout(time.Duration(30*time.Second)),
 			WithName("asdf"),
 			WithCPUs(4),
+			WithBinaryDataFunc(changeFunc),
 			WithBinaryData([]byte("asdf1234foobar")),
 			WithMetadataFromFile("../testdata/metadata.json"),
 			WithProtoset("testdata/bundle.protoset"),
@@ -150,6 +152,9 @@ func TestRunConfig_newRunConfig(t *testing.T) {
 		assert.Equal(t, 4, c.cpus)
 		assert.Equal(t, "asdf", c.name)
 		assert.Equal(t, []byte("asdf1234foobar"), c.data)
+		funcName1 := runtime.FuncForPC(reflect.ValueOf(changeFunc).Pointer()).Name()
+		funcName2 := runtime.FuncForPC(reflect.ValueOf(c.dataFunc).Pointer()).Name()
+		assert.Equal(t, funcName1, funcName2)
 		assert.Equal(t, `{"request-id": "{{.RequestNumber}}"}`, string(c.metadata))
 		assert.Equal(t, "", string(c.proto))
 		assert.Equal(t, "testdata/bundle.protoset", string(c.protoset))
diff --git a/runner/run_test.go b/runner/run_test.go
index dfca98c..0c38bac 100644
--- a/runner/run_test.go
+++ b/runner/run_test.go
@@ -10,9 +10,17 @@ import (
 	"github.com/bojand/ghz/internal"
 	"github.com/bojand/ghz/internal/helloworld"
 	"github.com/golang/protobuf/proto"
+	"github.com/jhump/protoreflect/desc"
 	"github.com/stretchr/testify/assert"
 )
 
+func changeFunc(mtd *desc.MethodDescriptor, cd *CallData) []byte {
+	msg := &helloworld.HelloRequest{}
+	msg.Name = "bob"
+	binData, _ := proto.Marshal(msg)
+	return binData
+}
+
 func TestRunUnary(t *testing.T) {
 	callType := helloworld.Unary
 
@@ -352,6 +360,50 @@ func TestRunUnary(t *testing.T) {
 		assert.Equal(t, 1, connCount)
 	})
 
+	t.Run("test binary with func", func(t *testing.T) {
+
+		gs.ResetCounters()
+
+		report, err := Run(
+			"helloworld.Greeter.SayHello",
+			internal.TestLocalhost,
+			WithProtoFile("../testdata/greeter.proto", []string{}),
+			WithTotalRequests(5),
+			WithBinaryDataFunc(changeFunc),
+			WithConcurrency(1),
+			WithTimeout(time.Duration(20*time.Second)),
+			WithDialTimeout(time.Duration(20*time.Second)),
+			WithInsecure(true),
+		)
+
+		assert.NoError(t, err)
+
+		assert.NotNil(t, report)
+
+		assert.Equal(t, 5, int(report.Count))
+		assert.NotZero(t, report.Average)
+		assert.NotZero(t, report.Fastest)
+		assert.NotZero(t, report.Slowest)
+		assert.NotZero(t, report.Rps)
+		assert.Empty(t, report.Name)
+		assert.NotEmpty(t, report.Date)
+		assert.NotEmpty(t, report.Options)
+		assert.NotEmpty(t, report.Details)
+		assert.NotEmpty(t, report.LatencyDistribution)
+		assert.Equal(t, ReasonNormalEnd, report.EndReason)
+		assert.Empty(t, report.ErrorDist)
+
+		assert.NotEqual(t, report.Average, report.Slowest)
+		assert.NotEqual(t, report.Average, report.Fastest)
+		assert.NotEqual(t, report.Slowest, report.Fastest)
+
+		count := gs.GetCount(callType)
+		assert.Equal(t, 5, count)
+
+		connCount := gs.GetConnectionCount()
+		assert.Equal(t, 1, connCount)
+	})
+
 	t.Run("test connections", func(t *testing.T) {
 		gs.ResetCounters()