census-ecosystem / opencensus-go-exporter-stackdriver

OpenCensus Go exporter for Stackdriver Monitoring and Trace
Apache License 2.0
67 stars 79 forks source link

GetMetricType and MetricPrefix do not work as their descriptions say in new reporting pipeline #237

Closed yanweiguo closed 4 years ago

yanweiguo commented 4 years ago

Is your feature request related to a problem? Please describe. Description of MetricPrefix:

// MetricPrefix overrides the prefix of a Stackdriver metric display names.
// Optional. If unset defaults to "OpenCensus/".
// Deprecated: Provide GetMetricDisplayName to change the display name of
// the metric.
// If GetMetricDisplayName is non-nil, this option is ignored.

It's about display name, however it is used as metric prefix here

Description of GetMetricType:

// GetMetricType allows customizing the metric type for the given view.
// By default, it will be:
//   "custom.googleapis.com/opencensus/" + view.Name
//
// See: https://cloud.google.com/monitoring/api/ref_v3/rest/v3/projects.metricDescriptors#MetricDescriptor

However it is not used in metrics.go.

Describe the solution you'd like Use GetMetricType instead of MetricPrefix to determine metric prefix.

Describe alternatives you've considered My proposal is what is used in the old pipeline: https://github.com/census-ecosystem/opencensus-go-exporter-stackdriver/blob/61e3caa20a46015704f45956535fe83133eaf62e/stats.go#L564 I think it is a bug. Unless we want to change the behavior, then no alternatives are considered.

yanweiguo commented 4 years ago

cc @songy23 @rghetia Does it make sense to you?

yanweiguo commented 4 years ago

Ah, GetMetricType func(view *view.View) string is using view.View. You are deprecating usage of view.View. Then I think we need something similar to it, say GetMetricPrefix func(name string) string

rghetia commented 4 years ago

MetricPrefix

All of the above is definitely confusing. It should be fixed as follows

@yanweiguo , It is not clear why MetricPrefix will not work for you with exporting Metric?

If it doesn't work then we can implement GetMetricPrefix func(). Not sure if should take string or Metric. I am leaning towards Metric because callee can make decision based on multiple parameters of Metric.

But before we add GetMetricPrefix I would like to know why MetricPrefix does not work.

songy23 commented 4 years ago

As I understand @yanweiguo wants this option in order to specify different prefixes based on the metric names.

yanweiguo commented 4 years ago

But before we add GetMetricPrefix I would like to know why MetricPrefix does not work.

@rghetia Some of our metrics are reported to global resource type with custom.googleapi.com/knative.dev prefix and others are reported to other resource type, for example knative_revision, with knative.dev prefix. Similar to resource type, we need an option to dynamically decide the prefix based on the metric name.