-
Notifications
You must be signed in to change notification settings - Fork 327
Add runtime metrics support #1156
Add runtime metrics support #1156
Conversation
a2ac4bb
to
b564f8f
Compare
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be renamed to RunMetricOptions.
plugin/runmetrics/producer.go
Outdated
} | ||
|
||
// Read reads the current runtime metrics. | ||
func (c *Producer) Read() []*metricdata.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.
nit: rename 'c' to 'p'.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
name should follow the convention used in opentelemetry-service
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.
Could you please check if all names are ok with my last change.
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.
the names look fine. I didn't see cpu_seconds similar to one in vmmetricreceiver
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.Producer
implementation under/plugin/
which enables the collection of runtime metrics.Fixes #784