-
Notifications
You must be signed in to change notification settings - Fork 326
Add support for metrics in prometheus exporter #1105
Conversation
@@ -187,7 +55,7 @@ func TestMetricsEndpointOutput(t *testing.T) { | |||
if err != nil { | |||
t.Fatalf("failed to create prometheus exporter: %v", err) | |||
} | |||
view.RegisterExporter(exporter) | |||
//view.RegisterExporter(exporter) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: if these are not needed any more then just remove them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM except some nits.
exporter/prometheus/prometheus.go
Outdated
// ExportMetrics exports to the Prometheus. | ||
// Each OpenCensus Metric will be converted to | ||
// corresponding Prometheus Metric: | ||
// SumData will be converted to Untyped Metric, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These comments no longer apply here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed. I have referred to Metric Types (like TypeCumulativeInt64) in modified comment instead of stats aggregation type (like sum).
exporter/prometheus/prometheus.go
Outdated
} | ||
return prometheus.NewConstHistogram(desc, uint64(v.Count), v.Sum, points, labelValues...) | ||
default: | ||
return nil, pointTypeError(point) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: leave a TODO to support Summary metrics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
view.RegisterExporter(exporter) | ||
reportPeriod := time.Millisecond | ||
view.SetReportingPeriod(reportPeriod) | ||
//view.RegisterExporter(exporter) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a few more commented lines that can be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed all of them.
exporter/prometheus/prometheus.go
Outdated
c.mu.Lock() | ||
defer c.mu.Unlock() | ||
func pointTypeError(point metricdata.Point) error { | ||
return fmt.Errorf("point type %T is not yet supported", point) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: the error message could be a bit confusing, consider "point type %T doesn't match with metric type".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
…n#1105) * Add prometheus support. * remove view related code and refactor metric specific code. * fix review comments. * remove dead code and fix comments. * fix error message.
* Add prometheus support. * remove view related code and refactor metric specific code. * fix review comments. * remove dead code and fix comments. * fix error message.
fixes #1040