Integrate go exporter metric tests with mock metric api #67

mabdi3 · 2020-07-14T18:11:23Z

As described in the proposal, we want to integrate those writing exporters with our mock api instead having to call the google cloud libraries directly. In short, I simply replaced the current client calls with calls to our mock client.

nilebox

Needs some cleanup, otherwise LGTM.

nilebox · 2020-07-16T00:47:03Z

exporter/metric/metric_test.go

@@ -39,14 +40,20 @@ var (
 )

 func TestDescToMetricType(t *testing.T) {
+	cloudMock := cloudmock.NewCloudMock()
+	client := cloudMock.MetricServiceClient


This is different from the approach in #66 where []option.ClientOption{option.WithGRPCConn(mock.ClientConn())} is used instead.
Shall we use the same approach in both PRs for consistency (whichever is preferrable)?

I'm not too sure about the usability of the approach in #66 here since the metric exporters here are defined literally by their respective structs and not by using the newMetricExporter function defined here.

Seems to me there is a way to proceed, one involving reverting the changes to newMetricExporter and passing in the correct clientOptions to direct requests to our mock api library. After looking closer, it seems newMetricExporter is the more "commerical" method of creating an exporter where one would actually want to connect to the google cloud instance instead of our mock api. Correct me if I'm misunderstanding, but I shouldn't have touched that file entirely.

Mocking should be still as close to the actual code being used as possible, so it's preferable to use constructor functions over directly creating structs when possible.

If I understand correctly, using newMetricExporter will increase the code coverage in tests, and will test the actual code executed when the metric exporter is being initialized in

opentelemetry-operations-go/exporter/metric/cloudmonitoring.go

Line 73 in 8fcf645

me, err := newMetricExporter(&o)

It's fine to merge the change as is, and then you may consider creating another PR switching to newMetricExporter.

I agree, thanks a lot.

exporter/metric/metric_test.go

exporter/metric/metric.go

exporter/metric/metric_test.go

nilebox

Please revert changes in newMetricExporter and invoke it with clientOpts using mock client in the tests instead (see comments).

exporter/metric/metric.go

nilebox

Sorry for the confusion, but seems like adding mock to existing test is useless.
If you want to test the exporter with a mock server, you need to add new integration tests instead, which will be invoking the ExportMetrics function

opentelemetry-operations-go/exporter/metric/metric.go

Lines 173 to 174 in 4866baa

    
           // ExportMetrics exports OpenTelemetry Metrics to Google Cloud Monitoring. 
        
           func (me *metricExporter) ExportMetrics(ctx context.Context, cps export.CheckpointSet) error {

, which should invoke the gRPC client and then you can validate the behavior using a mock.

exporter/metric/metric_test.go

nilebox

The direction looks good now, just a few more comments

nilebox · 2020-07-21T02:39:05Z

exporter/metric/metric_test.go

+	// Register counter value
+	counter := metric.Must(meter).NewInt64Counter("counter-a")
+	clabels := []kv.KeyValue{kv.Key("key").String("value")}
+	counter.Add(ctx, 100, clabels...)


Would be nice to add a function for metrics similar to GetNumSpans() to be able to validate that the correct data appeared on the other end.

Alternatively, you may add another test which will construct the exporter via

opentelemetry-operations-go/exporter/metric/cloudmonitoring.go

Lines 49 to 50 in 4866baa

// NewRawExporter creates a new Exporter thats implements metric.Exporter.

func NewRawExporter(opts ...Option) (*Exporter, error) {

and invoke

opentelemetry-operations-go/exporter/metric/metric.go

Line 174 in 4866baa

func (me *metricExporter) ExportMetrics(ctx context.Context, cps export.CheckpointSet) error {

directly and assert that there is no error

exporter/metric/metric_test.go

nilebox

LGTM.
@james-bebbington do you want to have a look before I merge it?

nilebox · 2020-07-22T04:11:54Z

exporter/metric/metric_test.go

@@ -20,15 +20,20 @@ import (
 	"reflect"
 	"testing"

+	"github.com/googleinterns/cloud-operations-api-mock/cloudmock"
+	"github.com/stretchr/testify/assert"
+


nit: these imports should be together with the one below (i.e. no empty line)

Surprising that the linter didn't pick this up. Maybe we need to add some more CI steps

james-bebbington

LGTM. Happy for you to merge

mabdi3 marked this pull request as draft July 14, 2020 21:58

mabdi3 force-pushed the mabdi3/mock-metric-integration branch 2 times, most recently from f9f117a to 9edb0b1 Compare July 15, 2020 19:12

nilebox suggested changes Jul 16, 2020

View reviewed changes

mabdi3 force-pushed the mabdi3/mock-metric-integration branch 3 times, most recently from 2f9dc1e to 4e43654 Compare July 20, 2020 22:49

nilebox suggested changes Jul 20, 2020

View reviewed changes

exporter/metric/metric.go Outdated Show resolved Hide resolved

mabdi3 force-pushed the mabdi3/mock-metric-integration branch 3 times, most recently from 5afe521 to 4eaed39 Compare July 21, 2020 00:23

integrate go exporter metric tests with mock metric api

0a89df6

mabdi3 force-pushed the mabdi3/mock-metric-integration branch from 4eaed39 to 0a89df6 Compare July 21, 2020 00:25

nilebox suggested changes Jul 21, 2020

View reviewed changes

exporter/metric/metric_test.go Show resolved Hide resolved

exporter/metric/metric_test.go Outdated Show resolved Hide resolved

nilebox reviewed Jul 21, 2020

View reviewed changes

mabdi3 force-pushed the mabdi3/mock-metric-integration branch 2 times, most recently from adef7c9 to 4e6825b Compare July 21, 2020 22:13

nilebox marked this pull request as ready for review July 22, 2020 04:10

nilebox approved these changes Jul 22, 2020

View reviewed changes

james-bebbington approved these changes Jul 22, 2020

View reviewed changes

add new integration test and revert redundant code

517aa5d

mabdi3 force-pushed the mabdi3/mock-metric-integration branch from 4e6825b to 517aa5d Compare July 22, 2020 19:09

nilebox merged commit ce9a184 into GoogleCloudPlatform:master Jul 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate go exporter metric tests with mock metric api #67

Integrate go exporter metric tests with mock metric api #67

mabdi3 commented Jul 14, 2020

nilebox left a comment

nilebox Jul 16, 2020

mabdi3 Jul 16, 2020

nilebox Jul 16, 2020

mabdi3 Jul 16, 2020

nilebox left a comment

nilebox left a comment •

edited

nilebox left a comment

nilebox Jul 21, 2020

nilebox Jul 21, 2020

nilebox left a comment

nilebox Jul 22, 2020

james-bebbington Jul 22, 2020

james-bebbington left a comment

	// ExportMetrics exports OpenTelemetry Metrics to Google Cloud Monitoring.
	func (me *metricExporter) ExportMetrics(ctx context.Context, cps export.CheckpointSet) error {

	// NewRawExporter creates a new Exporter thats implements metric.Exporter.
	func NewRawExporter(opts ...Option) (*Exporter, error) {

Integrate go exporter metric tests with mock metric api #67

Integrate go exporter metric tests with mock metric api #67

Conversation

mabdi3 commented Jul 14, 2020

nilebox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nilebox left a comment

Choose a reason for hiding this comment

nilebox left a comment • edited

Choose a reason for hiding this comment

nilebox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nilebox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

james-bebbington left a comment

Choose a reason for hiding this comment

nilebox left a comment •

edited