Add metrics instrumentation#24
Conversation
| import "testing" | ||
|
|
||
| // FailOnError logs the error and terminates the test immediately | ||
| func FailOnError(t *testing.T, err error) { |
There was a problem hiding this comment.
The require package can be used in place of this. Some of the code in the original repo may need to be updated (out of the scope of this PR).
|
|
||
| // InitMetricsCollector is used to select the appropriate metrics collector and | ||
| // set up the required values for instrumenting. | ||
| func InitMetricsCollector(enabled bool) error { |
There was a problem hiding this comment.
I think this InitMetricsCollector can be removed. We can have an InitNopMetricsCollector (effectively does nothing) and an InitPrometheusMetricsCollector (should take in the histogram map and whatever else we add in the future as an argument). Both of them should eventually do SetGlobMetricsCollector. But the decision on which one to call can be left to the caller.
In general, the code in pkg should not depend on anything outside. The dependency on api/log could be an issue if this module is being used elsewhere. But if we break this function down as suggested above, the logging can happen from the caller.
| } | ||
| // Initialize | ||
| histogramMap := make(map[MetricName]*prometheus.HistogramVec) | ||
| globalMetricsCollector.InitMetrics(histogramMap) |
There was a problem hiding this comment.
This needs to be passed in from the caller.
| } | ||
|
|
||
| // InitMetrics satisfies the Collector interface | ||
| func (NopMetricsCollector) InitMetrics(map[MetricName]*prometheus.HistogramVec) {} |
There was a problem hiding this comment.
Referring to the Prometheus library in this file doesn't quite make sense, right? In the future, for esample, there can be a Statsd Metrics collector. metrics.go and nop.go should not be concerned with individual collectors related functionality.
krithika369
left a comment
krithika369
left a comment
There was a problem hiding this comment.
Left one minor comment for change. The rest looks good! Thanks for this MR.
|
|
||
| func InitPrometheusMetricsCollector(histogramMap map[MetricName]*prometheus.HistogramVec) error { | ||
| SetGlobMetricsCollector(&PrometheusClient{histogramMap: histogramMap}) | ||
| globalMetricsCollector.InitMetrics() |
There was a problem hiding this comment.
I think the InitMetrics method is now redundant - its functionality can simply be merged into InitPrometheusMetricsCollector and InitMetrics can simply be removed from the interface.
| return &NopMetricsCollector{} | ||
| } | ||
|
|
||
| func InitNopMetricsCollector() error { |
There was a problem hiding this comment.
Good idea to add error to the signature here and in InitPrometheusMetricsCollector.
2 participants
This PR defines an interface for the metrics collector and provides concrete implementations for Nop and Prometheus.