Change ExportMetric api to ExportMetricProto #106

rghetia · 2019-03-15T05:22:30Z

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 · 2019-03-15T12:19:27Z

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 · 2019-03-15T17:06:00Z

@odeke-em reference 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 · 2019-03-15T18:19:26Z

@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?

[EDIT]: The purpose of a bundler is to relieve the goroutine that's invoking ExportMetric* from having to wait until the conversion is done. 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.

songy23 · 2019-03-15T21:30:37Z

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 · 2019-03-15T21:32:18Z

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 · 2019-03-15T21:45:54Z

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 · 2019-03-15T22:01:43Z

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 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 · 2019-03-15T22:04:46Z

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 · 2019-03-15T22:22:24Z

consensus is to just do the name change. I'll update the PR.

odeke-em · 2019-03-16T00:32:42Z

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 #106 (comment)

songy23

LGTM

Change the api to ExportMetricsProto to reflect that it accepts more than one metric.

odeke-em

LGTM, thank you @rghetia for this change and for bearing with my feedback!

odeke-em · 2019-03-16T06:18:42Z

metrics.go

-	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 metrics == nil {


Minor nit: if len(metrics) == 0 instead of if metrics == nil

rghetia · 2019-03-16T07:05:24Z

LGTM, thank you @rghetia for this change and for bearing with my feedback!

thanks for reviewing.

Change ExportMetric api to ExportMetricProto

d887968

googlebot added the cla: yes label Mar 15, 2019

rghetia requested review from odeke-em, songy23 and bogdandrutu March 15, 2019 05:22

restrict to name change.

8942baf

songy23 approved these changes Mar 16, 2019

View reviewed changes

accept slice of metric

e0f2b9f

Change the api to ExportMetricsProto to reflect that it accepts more than one metric.

odeke-em approved these changes Mar 16, 2019

View reviewed changes

fix review comment.

8004405

rghetia merged commit ed9eca6 into census-ecosystem:master Mar 16, 2019

rghetia deleted the change_api branch April 18, 2019 04:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change ExportMetric api to ExportMetricProto #106

Change ExportMetric api to ExportMetricProto #106

rghetia commented Mar 15, 2019

odeke-em commented Mar 15, 2019

rghetia commented Mar 15, 2019

odeke-em commented Mar 15, 2019 •

edited

songy23 commented Mar 15, 2019

songy23 commented Mar 15, 2019

odeke-em commented Mar 15, 2019

songy23 commented Mar 15, 2019

odeke-em commented Mar 15, 2019

rghetia commented Mar 15, 2019

odeke-em commented Mar 16, 2019

songy23 left a comment

odeke-em left a comment

odeke-em Mar 16, 2019

rghetia commented Mar 16, 2019

Change ExportMetric api to ExportMetricProto #106

Change ExportMetric api to ExportMetricProto #106

Conversation

rghetia commented Mar 15, 2019

odeke-em commented Mar 15, 2019

rghetia commented Mar 15, 2019

odeke-em commented Mar 15, 2019 • edited

songy23 commented Mar 15, 2019

songy23 commented Mar 15, 2019

odeke-em commented Mar 15, 2019

songy23 commented Mar 15, 2019

odeke-em commented Mar 15, 2019

rghetia commented Mar 15, 2019

odeke-em commented Mar 16, 2019

songy23 left a comment

Choose a reason for hiding this comment

odeke-em left a comment

Choose a reason for hiding this comment

odeke-em Mar 16, 2019

Choose a reason for hiding this comment

rghetia commented Mar 16, 2019

odeke-em commented Mar 15, 2019 •

edited