Add runtime metrics support#1156
Conversation
a2ac4bb to
b564f8f
Compare
rghetia
left a comment
There was a problem hiding this comment.
Thanks for your contribution. See comments inline.
plugin/runmetrics/producer.go
Outdated
| // | ||
| // Supply ProducerOptions to configure the behavior of the Producer. | ||
| // An error might be returned, if creating metrics gauges fails. | ||
| func NewProducer(options ProducerOptions) (*Producer, error) { |
There was a problem hiding this comment.
Instead of creating producer and then require the user to register it, may be it can also register it.
func Enable(options RunMetricsOptions) error {
// Based on options create and register cpu, mem or both producers.
}
func Disable() {
// find producer associated with the process and delete it from registry.
}Restrict to a single producer per process.
After above changes there is no need to export Producer.
plugin/runmetrics/producer.go
Outdated
| } | ||
|
|
||
| // ProducerOptions allows to configure Producer. | ||
| ProducerOptions struct { |
There was a problem hiding this comment.
This should be renamed to RunMetricOptions.
plugin/runmetrics/producer.go
Outdated
| } | ||
|
|
||
| // Read reads the current runtime metrics. | ||
| func (c *Producer) Read() []*metricdata.Metric { |
plugin/runmetrics/producer.go
Outdated
| memStats := &memStats{} | ||
|
|
||
| // General | ||
| memStats.memAlloc, err = producer.createInt64GaugeEntry("mem_alloc", "Bytes of allocated heap objects", metricdata.UnitBytes) |
There was a problem hiding this comment.
name should follow the convention used in opentelemetry-service
There was a problem hiding this comment.
Could you please check if all names are ok with my last change.
There was a problem hiding this comment.
the names look fine. I didn't see cpu_seconds similar to one in vmmetricreceiver
There was a problem hiding this comment.
CPU time is handled by prometheus/procfs in vmmetricsreceiver. I focused on what is available out-of-the-box.
59eb4a2 to
4482f5f
Compare
4482f5f to
22f7943
Compare
|
Any thoughts on adding ie. https://golang.org/pkg/runtime/debug/#GCStats The Prometheus |
|
|
||
| Disable() // ensure no duplicate registrations | ||
| metricproducer.GlobalManager().AddProducer(producer) | ||
| return nil |
There was a problem hiding this comment.
There is a race condition here. Without any mutex there is a possibility of adding two producers.
Maybe you can create a singleton with sync.Once and with mutex delete/add producer when it is enabled/disabled.
You can also create all entries once and simply based on boolean (memory/cpu) read those metrics.
There was a problem hiding this comment.
I wasn't able to see a real data race, but it was inconsistent indeed. I adjusted it, so that there can only be one producer registered. But if you would enable/disable concurrently, the last one wins (obviously).
I didn't go with a singleton, so the producer instance can remain immutable and doesn't need to care about synchronizing on the options or anything like that.
22f7943 to
6a047c5
Compare
This adds a
metricproducer.Producerimplementation under/plugin/which enables the collection of runtime metrics.Fixes #784