Repository: census-ecosystem/opencensus-go-exporter-stackdriver
Pull request #106 (2019-03-15 05:22:30)

Change ExportMetric api to ExportMetricProto
This is a breaking change but only consumer of this api is oc-agent. 
The api is renamed because this api should be used to export metric from core library to backends. After all, the api really exports metric proto and not metric data model.

The other change with this PR is taking slice of metrics instead of single metric. After that change there is no need for bundler. Hence it is removed.



odeke-em: Thank you for working on this change @rghetia!

I have two comments:
* the bundler's purpose is to export metrics after some batching/wait-period on a separate goroutine. With this change, exporting, retrying etc will be performed on the same goroutine that invoked ExportMetricsProto. Please keep that
* I am not too keen about an exported method called ExportMetricsProto, sure there isn't a "Metrics" object per se nor the data model but it is implicit that ExportMetrics operates on the Metrics proto. I'd be fine with keeping the name but changing it to take a slice as it does with this change. However, this isn't a hill I'd lose my marbles on, the naming can be changed if you and others like
rghetia: @odeke-em [reference](https://github.com/census-instrumentation/opencensus-go/pull/1049#discussion_r265242815) for the name change decision. If you have a better suggestion I would be happy to incorporate.

Regarding bundler - it is already bundled on oc-agent side. Do we need additional bundling? 

odeke-em: >@odeke-em reference for the name change decision. If you have a better suggestion I would be happy to incorporate.

@rghetia thanks for the reference. It would be useful to add such references to your commit message otherwise there is no context about the naming. With that I drop my suggestion not to rename.

>Regarding bundler - it is already bundled on oc-agent side. Do we need additional bundling?

The purpose of a bundler is to relieve the goroutine that's invoking ExportMetric* from having to wait until the conversion is done. Think of it like an asynchronous worker that you send results to and it handles exporting for you without waiting. This ocagent-exporter is itself used in the OpenCensus-Service to export to OpenCensus-Proto destinations and with removing a bundler, exporting will be slowed down. You can run experiments and directly see the slow-down. 
songy23: > I see your point about it being used in a bundler on the ocagent-side. However, it can also be used by other folks hence the need for the bundler here.

IMO the `ExportMetricsProto` method should only be used by Agent/Collector. Other libraries should use the OC-Go metrics APIs and won't produce metric protos.
songy23: > Regarding bundler - it is already bundled on oc-agent side. Do we need additional bundling?

Only Agent/Collector would use this API so I would say we don't need it on exporter side.
odeke-em: >Only Agent/Collector would use this API so I would say we don't need it on exporter side.

While that's true(thanks to the both of you for pointing that out), it doesn't seem like a great/robust design to remove a bundler here just because of one known use case that we assume will be the ONLY user. I'd argue that OpenCensus Service would be the one to remove its bundler. What if other companies ingest OpenCensus-Proto Metrics and do something with this exporter but their code isn't open source? What's the impetus to remove it from here? It also seems like a tertiary change to "Change ExportMetric api to ExportMetricProto"
songy23: > it doesn't seem like a great/robust design to remove a bundler here just because of one known use case that we assume will be the ONLY user. I'd argue that OpenCensus Service would be the one to remove its bundler. 

I'm good with this too. We can keep the bundling here and remove the logic from Agent.

> It also seems like a tertiary change to "Change ExportMetric api to ExportMetricProto"

That's true, if other companies depend on the `ExportMetric` they need to change it to `ExportMetricProto` too. While it may add some extra work, I still think it's better to make this name change, especially now we have both the native [metric](https://github.com/census-instrumentation/opencensus-go/tree/master/metric) package and proto-generated metric files. If we leave the API `ExportMetric` but have it ingest proto metrics it would be very confusing.
odeke-em: >That's true, if other companies depend on the ExportMetric they need to change it to ExportMetricProto too. While it may add some extra work, I still think it's better to make this name change, especially now we have both the native metric package and proto-generated metric files. If we leave the API ExportMetric but have it ingest proto metrics it would be very confusing.

I am on the same page with the reasoning to make the name change. Perhaps to ExportProtoMetrics or ExportMetricsProto since it now exports more than one, but also for keeping the bundler changes out of this PR.
rghetia: consensus is to just do the name change. I'll update the PR. 
odeke-em: Thanks @rghetia with the change. However, I personally had also alluded to accepting
>The other change with this PR is taking slice of metrics instead of single metric.

as per https://github.com/census-ecosystem/opencensus-go-exporter-stackdriver/pull/106#issuecomment-473455644
rghetia: > LGTM, thank you @rghetia for this change and for bearing with my feedback!

thanks for reviewing.

Patch:
diff --git a/metrics.go b/metrics.go
index dc29bf7..c37cbad 100644
--- a/metrics.go
+++ b/metrics.go
@@ -49,18 +49,20 @@ type metricPayload struct {
 	metric   *metricspb.Metric
 }
 
-// ExportMetric exports OpenCensus Metrics to Stackdriver Monitoring.
-func (se *statsExporter) ExportMetric(ctx context.Context, node *commonpb.Node, rsc *resourcepb.Resource, metric *metricspb.Metric) error {
-	if metric == nil {
+// ExportMetricsProto exports OpenCensus Metrics Proto to Stackdriver Monitoring.
+func (se *statsExporter) ExportMetricsProto(ctx context.Context, node *commonpb.Node, rsc *resourcepb.Resource, metrics []*metricspb.Metric) error {
+	if len(metrics) == 0 {
 		return errNilMetric
 	}
 
-	payload := &metricPayload{
-		metric:   metric,
-		resource: rsc,
-		node:     node,
+	for _, metric := range metrics {
+		payload := &metricPayload{
+			metric:   metric,
+			resource: rsc,
+			node:     node,
+		}
+		se.protoMetricsBundler.Add(payload, 1)
 	}
-	se.protoMetricsBundler.Add(payload, 1)
 
 	return nil
 }
diff --git a/stackdriver.go b/stackdriver.go
index a6e7748..e42ea9b 100644
--- a/stackdriver.go
+++ b/stackdriver.go
@@ -345,9 +345,9 @@ func (e *Exporter) ExportView(vd *view.Data) {
 	e.statsExporter.ExportView(vd)
 }
 
-// ExportMetric exports OpenCensus Metrics to Stackdriver Monitoring.
-func (e *Exporter) ExportMetric(ctx context.Context, node *commonpb.Node, rsc *resourcepb.Resource, metric *metricspb.Metric) error {
-	return e.statsExporter.ExportMetric(ctx, node, rsc, metric)
+// ExportMetricsProto exports OpenCensus Metrics Proto to Stackdriver Monitoring.
+func (e *Exporter) ExportMetricsProto(ctx context.Context, node *commonpb.Node, rsc *resourcepb.Resource, metrics []*metricspb.Metric) error {
+	return e.statsExporter.ExportMetricsProto(ctx, node, rsc, metrics)
 }
 
 // ExportSpan exports a SpanData to Stackdriver Trace.


Test patch:
diff --git a/equivalence_test.go b/equivalence_test.go
index aeda057..9994dd1 100644
--- a/equivalence_test.go
+++ b/equivalence_test.go
@@ -275,9 +275,7 @@ func TestEquivalenceStatsVsMetricsUploads(t *testing.T) {
 	oce.Flush()
 
 	ma.forEachRequest(func(emr *agentmetricspb.ExportMetricsServiceRequest) {
-		for _, metric := range emr.Metrics {
-			_ = se.ExportMetric(context.Background(), emr.Node, emr.Resource, metric)
-		}
+		_ = se.ExportMetricsProto(context.Background(), emr.Node, emr.Resource, emr.Metrics)
 	})
 	se.Flush()