Repository: iter8-tools/iter8
Pull request #1211 (2022-04-05 15:26:58)

gRPC
The gRPC chart does not properly pass values into the gRPC task.

Please see the new unit test (TestGenGRPC) which is failing.


Patch:
diff --git a/base/collect_grpc.go b/base/collect_grpc.go
index baf28f13..e5606039 100644
--- a/base/collect_grpc.go
+++ b/base/collect_grpc.go
@@ -1,7 +1,6 @@
 package base
 
 import (
-	"encoding/json"
 	"errors"
 	"time"
 
@@ -10,28 +9,11 @@ import (
 	gd "github.com/mcuadros/go-defaults"
 )
 
-/*
-Credit: The inputs for this task, including their JSON/YAML tags and go doc comments are derived from https://github.com/bojand/ghz.
-*/
-
-// versionGRPC contains call and host information needed to send gRPC requests to each version.
-// this struct holds inputs that is specific to each version
-type versionGRPC struct {
-	// Call A fully-qualified method name in 'package.Service/method' or 'package.Service.Method' format.
-	Call string `json:"call" yaml:"call"`
-	// Host gRPC host in the hostname[:port] format.
-	Host string `json:"host" yaml:"host"`
-}
-
-// collectGRPCInputs holds all the inputs for this task
-type collectGRPCInputs struct {
-	// even though runner.Config is embedded,
-	// many of the fields in runner.Config will not be supported.
-	// Refer to iter8.tools documentation for supported fields.
-	runner.Config
-	// VersionInfo is a non-empty list of versionGRPC values.
-	VersionInfo []*versionGRPC `json:"versionInfo" yaml:"versionInfo"`
-}
+// // collectGRPCInputs holds all the inputs for this task
+// type collectGRPCInputs struct {
+// 	// Config is the ghz runner.Config object embedded in the input
+// 	runner.Config
+// }
 
 const (
 	// CollectGRPCTaskName is the name of this task which performs load generation and metrics collection for gRPC services.
@@ -57,23 +39,23 @@ type collectGRPCTask struct {
 	// TaskMeta has fields common to all tasks
 	TaskMeta
 	// With contains the inputs to this task
-	With collectGRPCInputs `json:"with" yaml:"with"`
+	With runner.Config `json:"with" yaml:"with"`
 }
 
 // initializeDefaults sets default values for the collect task
 func (t *collectGRPCTask) initializeDefaults() {
 	// set defaults
-	gd.SetDefaults(&t.With.Config)
+	gd.SetDefaults(&t.With)
 	// if dial timeout is zero, then set a default...
 	if t.With.DialTimeout == 0 {
 		td, _ := time.ParseDuration("10s")
 		t.With.DialTimeout = runner.Duration(td)
 	}
 	// always count errors
-	t.With.Config.CountErrors = countErrorsDefault
+	t.With.CountErrors = countErrorsDefault
 	// todo: document how to use security credentials
 	// remove this default altogether after enabling secure
-	t.With.Config.Insecure = insecureDefault
+	t.With.Insecure = insecureDefault
 }
 
 // validate task inputs
@@ -81,29 +63,14 @@ func (t *collectGRPCTask) validateInputs() error {
 	return nil
 }
 
-// getGhzConfig constructs ghz's runner.Config for version j based on task inputs
-func (t *collectGRPCTask) getGhzConfig(j int) (*runner.Config, error) {
-	ghzc := t.With.Config
-	ghzc.Call = t.With.VersionInfo[j].Call
-	ghzc.Host = t.With.VersionInfo[j].Host
-
-	return &ghzc, nil
-}
-
 // resultForVersion collects gRPC test result for a given version
-func (t *collectGRPCTask) resultForVersion(j int) (*runner.Report, error) {
+func (t *collectGRPCTask) resultForVersion() (*runner.Report, error) {
 	// the main idea is to run ghz with proper options
-	ghzc, err := t.getGhzConfig(j)
-	if err != nil {
-		return nil, err
-	}
-	ghzcBytes, _ := json.MarshalIndent(ghzc, "", "	")
-	log.Logger.WithStackTrace(string(ghzcBytes)).Trace("runner config")
 
-	opts := runner.WithConfig(ghzc)
+	opts := runner.WithConfig(&t.With)
 
 	// todo: supply all the allowed options
-	igr, err := runner.Run(t.With.VersionInfo[j].Call, t.With.VersionInfo[j].Host, opts)
+	igr, err := runner.Run(t.With.Call, t.With.Host, opts)
 	if err != nil {
 		e := errors.New("ghz run failed")
 		log.Logger.WithStackTrace(err.Error()).Error(e)
@@ -139,96 +106,70 @@ func (t *collectGRPCTask) run(exp *Experiment) error {
 
 	t.initializeDefaults()
 
-	// 2. validate input (without going crazy in terms of rules)
-	// idea is to check for the most obvious errors here
-	// validation also happens at the chart/values level
-	if len(t.With.VersionInfo) == 0 {
-		log.Logger.Error("collect task must specify info for at least one version")
-		return errors.New("collect task must specify info for at least one version")
-	}
-
-	// 3. collect raw results from ghz for each version
+	// 2. collect raw results from ghz
 
-	// initialize a slice to hold reports from the ghz tests
-	gr := make([]*runner.Report, len(t.With.VersionInfo))
-
-	// run ghz test for each version sequentially
-	// collect ghz report for each version
+	// run ghz test
+	// collect ghz report
 	// ghz reports will be further processed to populate metrics
-	for j := range t.With.VersionInfo {
-		log.Logger.Trace("initiating ghz for version ", j)
-		var data *runner.Report
-		var err error
-		if t.With.VersionInfo[j] == nil { // nothing to do for this version
-			data = nil
-		} else {
-			// this is where we get the raw ghz data for version j
-			data, err = t.resultForVersion(j)
-			if err == nil {
-				gr[j] = data
-			} else {
-				return err
-			}
-		}
+	data, err := t.resultForVersion()
+	if err != nil {
+		return err
 	}
 
-	// 4. The inputs for this task determine the number of versions participating in the experiment.
-	// Hence, init insights with num versions
-	err = exp.Result.initInsightsWithNumVersions(len(t.With.VersionInfo))
+	// 3. Init insights with num versions: always 1 in this task
+	err = exp.Result.initInsightsWithNumVersions(1)
 	if err != nil {
 		return err
 	}
 	in := exp.Result.Insights
 
-	// 5. Populate all metrics collected by this task
-	for i := range t.With.VersionInfo { // for each version
-		if gr[i] != nil { // assuming there is some raw ghz result to process for this version
-			// populate grpc request count
-			// todo: this logic breaks for looped experiments. Fix when we get to loops.
-			m := gRPCMetricPrefix + "/" + gRPCRequestCountMetricName
-			mm := MetricMeta{
-				Description: "number of gRPC requests sent",
-				Type:        CounterMetricType,
-			}
-			in.updateMetric(m, mm, i, float64(gr[i].Count))
+	// 4. Populate all metrics collected by this task
+	if data != nil { // assuming there is some raw ghz result to process
+		// populate grpc request count
+		// todo: this logic breaks for looped experiments. Fix when we get to loops.
+		m := gRPCMetricPrefix + "/" + gRPCRequestCountMetricName
+		mm := MetricMeta{
+			Description: "number of gRPC requests sent",
+			Type:        CounterMetricType,
+		}
+		in.updateMetric(m, mm, 0, float64(data.Count))
 
-			// populate error count & rate
-			ec := float64(0)
-			for _, count := range gr[i].ErrorDist {
-				ec += float64(count)
-			}
+		// populate error count & rate
+		ec := float64(0)
+		for _, count := range data.ErrorDist {
+			ec += float64(count)
+		}
 
-			// populate count
-			// todo: This logic breaks for looped experiments. Fix when we get to loops.
-			m = gRPCMetricPrefix + "/" + gRPCErrorCountMetricName
-			mm = MetricMeta{
-				Description: "number of responses that were errors",
-				Type:        CounterMetricType,
-			}
-			in.updateMetric(m, mm, i, ec)
-
-			// populate rate
-			// todo: This logic breaks for looped experiments. Fix when we get to loops.
-			m = gRPCMetricPrefix + "/" + gRPCErrorRateMetricName
-			rc := float64(gr[i].Count)
-			if rc != 0 {
-				mm = MetricMeta{
-					Description: "fraction of responses that were errors",
-					Type:        GaugeMetricType,
-				}
-				in.updateMetric(m, mm, i, ec/rc)
-			}
+		// populate count
+		// todo: This logic breaks for looped experiments. Fix when we get to loops.
+		m = gRPCMetricPrefix + "/" + gRPCErrorCountMetricName
+		mm = MetricMeta{
+			Description: "number of responses that were errors",
+			Type:        CounterMetricType,
+		}
+		in.updateMetric(m, mm, 0, ec)
 
-			// populate latency sample
-			m = gRPCMetricPrefix + "/" + gRPCLatencySampleMetricName
+		// populate rate
+		// todo: This logic breaks for looped experiments. Fix when we get to loops.
+		m = gRPCMetricPrefix + "/" + gRPCErrorRateMetricName
+		rc := float64(data.Count)
+		if rc != 0 {
 			mm = MetricMeta{
-				Description: "gRPC Latency Sample",
-				Type:        SampleMetricType,
-				Units:       StringPointer("msec"),
+				Description: "fraction of responses that were errors",
+				Type:        GaugeMetricType,
 			}
-			lh := latencySample(gr[i].Details)
-			in.updateMetric(m, mm, i, lh)
+			in.updateMetric(m, mm, 0, ec/rc)
+		}
+
+		// populate latency sample
+		m = gRPCMetricPrefix + "/" + gRPCLatencySampleMetricName
+		mm = MetricMeta{
+			Description: "gRPC Latency Sample",
+			Type:        SampleMetricType,
+			Units:       StringPointer("msec"),
 		}
+		lh := latencySample(data.Details)
+		in.updateMetric(m, mm, 0, lh)
 	}
 	return nil
 }
diff --git a/base/experiment.go b/base/experiment.go
index f787532f..b6caabe6 100644
--- a/base/experiment.go
+++ b/base/experiment.go
@@ -130,7 +130,7 @@ type taskMetaWith struct {
 	// TaskMeta has fields common to all tasks
 	TaskMeta
 	// With is the raw representation of task inputs
-	With interface{} `json:"with,omitempty" yaml:"with,omitempty"`
+	With map[string]interface{} `json:"with,omitempty" yaml:"with,omitempty"`
 }
 
 // UnmarshallJSON will unmarshal an experiment spec from bytes
@@ -164,15 +164,30 @@ func (s *ExperimentSpec) UnmarshalJSON(data []byte) error {
 			switch *t.Task {
 			case CollectHTTPTaskName:
 				cht := &collectHTTPTask{}
-				json.Unmarshal(tBytes, cht)
+				err := json.Unmarshal(tBytes, cht)
+				if err != nil {
+					e := errors.New("json unmarshal error")
+					log.Logger.WithStackTrace(err.Error()).Error(e)
+					return e
+				}
 				tsk = cht
 			case CollectGRPCTaskName:
 				cgt := &collectGRPCTask{}
-				json.Unmarshal(tBytes, cgt)
+				err := json.Unmarshal(tBytes, cgt)
+				if err != nil {
+					e := errors.New("json unmarshal error")
+					log.Logger.WithStackTrace(err.Error()).Error(e)
+					return e
+				}
 				tsk = cgt
 			case AssessTaskName:
 				at := &assessTask{}
-				json.Unmarshal(tBytes, at)
+				err := json.Unmarshal(tBytes, at)
+				if err != nil {
+					e := errors.New("json unmarshal error")
+					log.Logger.WithStackTrace(err.Error()).Error(e)
+					return e
+				}
 				tsk = at
 			default:
 				log.Logger.Error("unknown task: " + *t.Task)
diff --git a/charts/iter8lib/Chart.yaml b/charts/iter8lib/Chart.yaml
index 4646de17..a3095cbf 100644
--- a/charts/iter8lib/Chart.yaml
+++ b/charts/iter8lib/Chart.yaml
@@ -1,6 +1,6 @@
 apiVersion: v2
 name: iter8lib
-version: 0.9.0
+version: 0.9.1
 description: Iter8 library chart
 type: library
 home: https://iter8.tools
diff --git a/charts/iter8lib/templates/_task-grpc.tpl b/charts/iter8lib/templates/_task-grpc.tpl
index 1b1ade93..e48ee493 100644
--- a/charts/iter8lib/templates/_task-grpc.tpl
+++ b/charts/iter8lib/templates/_task-grpc.tpl
@@ -3,89 +3,5 @@
 # collect Iter8's built-in gRPC latency and error-related metrics
 - task: gen-load-and-collect-metrics-grpc
   with:
-
-    {{- if .Values.protoFile }}
-    proto: {{ .Values.protoFile | quote }}
-    {{- end }}
-
-    {{- if .Values.protosetFile }}
-    protoset: {{ .Values.protosetFile | quote }}
-    {{- end }}
-
-    {{- if .Values.total }}
-    total: {{ .Values.total | int }}
-    {{- end }}
-
-    {{- if .Values.rps }}
-    rps: {{ .Values.rps | int }}
-    {{- end }}
-
-    {{- if .Values.concurrency }}
-    concurrency: {{ .Values.concurrency | int }}
-    {{- end }}
-
-    {{- if .Values.connections }}
-    connections: {{ .Values.connections | int }}
-    {{- end }}
-
-    {{- if .Values.duration }}
-    duration: {{ .Values.duration | quote }}
-    {{- end }}
-
-    {{- if .Values.maxDuration }}
-    max-duration: {{ .Values.maxDuration | quote }}
-    {{- end }}
-
-    {{- if .Values.streamInterval }}
-    stream-interval: {{ .Values.streamInterval | quote }}
-    {{- end }}
-
-    {{- if .Values.streamCallDuration }}
-    stream-call-duration: {{ .Values.streamCallDuration | quote }}
-    {{- end }}
-
-    {{- if .Values.streamCallCount }}
-    stream-call-count: {{ .Values.streamCallCount | int }}
-    {{- end }}
-
-    {{- if .Values.connectTimeout }}
-    connect-timeout: {{ .Values.connectTimeout | quote }}
-    {{- end }}
-
-    {{- if .Values.keepalive }}
-    keepalive: {{ .Values.keepalive | quote }}
-    {{- end }}
-
-    {{- if .Values.data }}
-    data:
-{{ toYaml .Values.data | indent 6 }}
-    {{- end }}
-
-    {{- if .Values.dataFile }}
-    data-file: {{ .Values.dataFile | quote }}
-    {{- end }}
-
-    {{- if .Values.binaryDataFile }}
-    binary-file: {{ .Values.binaryDataFile | quote }}
-    {{- end }}
-
-    {{- if .Values.metadata }}
-    metadata:
-{{ toYaml .Values.metadata | indent 6 }}
-    {{- end }}
-
-    {{- if .Values.metadataFile }}
-    metadata-file: {{ .Values.metadataFile | quote }}
-    {{- end }}
-
-    {{- if .Values.reflectMetadata }}
-    reflectMetadata:
-{{ toYaml .Values.reflectMetadata | indent 6 }}
-    {{- end }}
-
-    {{- ""}}
-    versionInfo:
-    - host: {{ required "A valid host is required!" .Values.host | quote }}
-      call: {{ required "A valid call is required!" .Values.call | quote }}
+{{ toYaml .Values | indent 4 }}
 {{- end }}    
-


Test patch:
diff --git a/action/gen_test.go b/action/gen_test.go
index 5e1ba031..9cbf497b 100644
--- a/action/gen_test.go
+++ b/action/gen_test.go
@@ -1,10 +1,12 @@
 package action
 
 import (
+	"encoding/json"
 	"os"
 	"testing"
 
 	"github.com/iter8-tools/iter8/base"
+	"github.com/iter8-tools/iter8/driver"
 	"github.com/stretchr/testify/assert"
 )
 
@@ -18,3 +20,29 @@ func TestGen(t *testing.T) {
 	err := gOpts.LocalRun()
 	assert.NoError(t, err)
 }
+
+func TestGenGRPC(t *testing.T) {
+	// fix gOpts
+	os.Chdir(t.TempDir())
+	gOpts := NewGenOpts()
+	gOpts.ChartsParentDir = base.CompletePath("../", "")
+	gOpts.ChartName = "load-test-grpc"
+	gOpts.Values = []string{"host=localhost:50051", "call=helloworld.Greeter.SayHello", "proto=helloworld.proto", "protoset=helloworld.protoset", "data.name=frodo", "SLOs.grpc/error-rate=0", "SLOs.grpc/latency/mean=150"}
+	err := gOpts.LocalRun()
+	assert.NoError(t, err)
+
+	fd := &driver.FileDriver{
+		RunDir: "./",
+	}
+	exp, err := base.BuildExperiment(false, fd)
+	assert.NoError(t, err)
+	assert.Equal(t, 2, len(exp.Tasks))
+
+	m := make(map[string]interface{}, 0)
+	b, _ := json.Marshal(exp.Tasks[0])
+	json.Unmarshal(b, &m)
+	m = m["with"].(map[string]interface{})
+	s := m["proto"].(string)
+	assert.Equal(t, "helloworld.proto", s)
+
+}
diff --git a/base/collect_grpc_test.go b/base/collect_grpc_test.go
index 98b08921..dd1d5f0f 100644
--- a/base/collect_grpc_test.go
+++ b/base/collect_grpc_test.go
@@ -26,14 +26,10 @@ func TestRunCollectGRPCUnary(t *testing.T) {
 		TaskMeta: TaskMeta{
 			Task: StringPointer(CollectGRPCTaskName),
 		},
-		With: collectGRPCInputs{
-			Config: runner.Config{
-				Data: map[string]interface{}{"name": "bob"},
-			},
-			VersionInfo: []*versionGRPC{{
-				Call: "helloworld.Greeter.SayHello",
-				Host: internal.TestLocalhost,
-			}},
+		With: runner.Config{
+			Data: map[string]interface{}{"name": "bob"},
+			Call: "helloworld.Greeter.SayHello",
+			Host: internal.TestLocalhost,
 		},
 	}
 
@@ -84,19 +80,15 @@ func TestMockGRPCWithSLOsAndPercentiles(t *testing.T) {
 		TaskMeta: TaskMeta{
 			Task: StringPointer(CollectGRPCTaskName),
 		},
-		With: collectGRPCInputs{
-			Config: runner.Config{
-				N:           100,
-				RPS:         20,
-				C:           1,
-				Timeout:     runner.Duration(20 * time.Second),
-				Data:        map[string]interface{}{"name": "bob"},
-				DialTimeout: runner.Duration(20 * time.Second),
-			},
-			VersionInfo: []*versionGRPC{{
-				Call: "helloworld.Greeter.SayHello",
-				Host: internal.TestLocalhost,
-			}},
+		With: runner.Config{
+			N:           100,
+			RPS:         20,
+			C:           1,
+			Timeout:     runner.Duration(20 * time.Second),
+			Data:        map[string]interface{}{"name": "bob"},
+			DialTimeout: runner.Duration(20 * time.Second),
+			Call:        "helloworld.Greeter.SayHello",
+			Host:        internal.TestLocalhost,
 		},
 	}
 
@@ -150,5 +142,4 @@ func TestMockGRPCWithSLOsAndPercentiles(t *testing.T) {
 
 	count := gs.GetCount(callType)
 	assert.Equal(t, int(ct.With.N), count)
-
 }
diff --git a/charts/load-test-grpc/Chart.yaml b/charts/load-test-grpc/Chart.yaml
index 484d62c7..7c950ce5 100644
--- a/charts/load-test-grpc/Chart.yaml
+++ b/charts/load-test-grpc/Chart.yaml
@@ -1,6 +1,6 @@
 apiVersion: v2
 name: load-test-grpc
-version: 0.9.1
+version: 0.9.2
 description: Load test, benchmark, and validate gRPC services
 type: application
 keywords:
diff --git a/charts/load-test-grpc/values.yaml b/charts/load-test-grpc/values.yaml
index e69d5bd1..a5735444 100644
--- a/charts/load-test-grpc/values.yaml
+++ b/charts/load-test-grpc/values.yaml
@@ -5,102 +5,8 @@
 
 ##################################
 
-### protoFile  Protocol Buffer .proto file that defines the gRPC service. 
-### Example: /user/me/helloworld.proto
-protoFile: null
-
-### protoURL  URL of Protocol Buffer .proto file that defines the gRPC service.
-### Example: https://user.me/helloworld.proto
-protoURL: null
-
-### protosetFile  .protoset file compiled from one or more .proto source files.
-### Example: /user/me/allworlds.protoset
-protosetFile: null
-
-### protosetURL  URL of .protoset file compiled from one or more .proto source files.
-### Example:  https://user.me/allworlds.protoset
-protosetURL: null
-
-### total Number of requests. 
-### If unspecified, a default value of 200 will be used.
-total:  null
-
-### rps Number of requests per second (RPS). 
-### If unspecified, or set to zero, then there will be no rate limit.
-### The rps will be distributed among all the workers (see concurrency).
-rps:  null
-
-### concurrency Number of workers to run concurrently.
-### If unspecified, a default value of 50 will be used.
-concurrency:  null
-
-### connections Number of connections to use.
-### Concurrency is distributed evenly among all the connections.
-### If unspecified, a default value of 1 will be used.
-connections:  null
-
-### duration Duration for the experiment to send requests. 
-### When duration is reached, experiment stops sending requests. 
-### If duration is specified, 'total' is ignored. Examples: 10s, 3m.
-duration: null
-
-### maxDuration Maximum duration for the experiment to send requests 
-### with 'total' setting respected. 
-### If duration is reached before 'total' requests are completed, 
-### experiment stops sending requests. Examples: 10s, 3m.
-maxDuration: null
-
-### streamInterval Interval between message sends in stream requests. Example: 1s
-streamInterval: null
-
-### streamCallDuration Duration after which experiment will close the stream in each streaming call. Example: 30s
-streamCallDuration: null
-
-### streamCallCount Count of messages sent, after which client will close the stream in each streaming call.
-streamCallCount: null
-
-### connectTimeout  Connection timeout duration for the initial connection dial.
-### If unspecified, a default value of 10s is used. Example: 20s
-connectTimeout: null
-
-### keepalive  Keepalive time duration. Only used if present and above 0s.
-keepalive: null
-
-### host  Host and port of the gRPC service
-### This field is required.
-host: null
-
-### call  Fully-qualified method name.
-### Specified in 'package.Service/method' or 'package.Service.Method' format.
-### This field is required.
-call: null
-
-### data  Call data YAML object.
-data: null
-
-### dataFile File path for call data JSON file.
-### Examples: /home/user/file.json or ./file.json.
-dataFile: null
-
-### dataURL URL of the call data JSON file.
-dataURL: null
-
-### binaryDataFile File path for the call data as serialized binary message or multiple count-prefixed messages.
-### Examples: /home/user/file.bin or ./file.bin
-binaryDataFile: null
-
-### binaryDataURL URL of the call data as serialized binary message or multiple count-prefixed messages.
-binaryDataURL: null
-
-### metadata  Call metadata YAML object in the map[string]string format.
-metadata: null
-
-### metadataFile File path for call metadata JSON file.
-### Examples: /home/user/metadata.json or ./metadata.json
-metadataFile: null
-
-### metadataURL URL of the call metadata JSON file.
-metadataURL: null
+### All parameters accepted by this chart (with the exception of SLOs), and their default values are documented here. In particular, please refer to the json/yaml tags of the fields:
+### https://github.com/bojand/ghz/blob/983a3bf8085b4e6146241df559a91e3a8399071d/runner/config.go#L60
 
 ### SLOs  A map of service level objectives (SLOs) that the app needs to satisfy.
 ### Keys are metric names. Values are acceptable upper limits for the metric.