util: Send only last metric value to Graphite #31829
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewed 2 of 3 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained
pkg/util/metric/graphite_exporter.go, line 73 at r1 (raw file):
} err = b.Push() ge.pm.clearMetrics()
What is the desired behavior here in the presence of errors? That we clear the metrics regardless of any error? That we clear the metrics regardless of error as long as that error came from b.Push? If the intention is the latter, which is what the code currently implements, then I'd suggest defering the clear call and still returning b.Push(). Regardless, let's be deliberate about this and comment our intention.
pkg/util/metric/prometheus_exporter.go, line 92 at r1 (raw file):
return err } // Clear metrics for reuse.
We can replace this with a call to clearMetrics after the loop, right?
pkg/util/metric/prometheus_exporter.go, line 115 at r1 (raw file):
func (pm *PrometheusExporter) clearMetrics() { for _, family := range pm.families { family.Metric = []*prometheusgo.Metric{}
We can just set it to nil to avoid the allocation if the family never gets any metrics.
Cockroach was emitting a ridiculously large number of metrics per minute because an instance of PrometheusExporter was being reused without clearing it after every iteration. Release note: Fix bug in graphite metrics sender where cockroach was collecting and sending all data points since startup instead of only the latest data point.
There was a problem hiding this comment.
Thanks for reviewing @nvanbenschoten. I've updated.
RFAL
Reviewable status:
pkg/util/metric/graphite_exporter.go, line 73 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
What is the desired behavior here in the presence of errors? That we clear the metrics regardless of any error? That we clear the metrics regardless of error as long as that error came from
b.Push? If the intention is the latter, which is what the code currently implements, then I'd suggestdefering the clear call and still returningb.Push(). Regardless, let's be deliberate about this and comment our intention.
Yes, we clear regardless of errors. I have made it defer and added a lengthy comment. Please ping if comment is not clear.
pkg/util/metric/prometheus_exporter.go, line 92 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
We can replace this with a call to
clearMetricsafter the loop, right?
Done.
pkg/util/metric/prometheus_exporter.go, line 115 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
We can just set it to
nilto avoid the allocation if the family never gets any metrics.
Done.
There was a problem hiding this comment.
bors r+
Reviewed 2 of 2 files at r2.
Reviewable status:
31829: util: Send only last metric value to Graphite r=nvanbenschoten a=neeral Cockroach was emitting a ridiculously large number of metrics per minute because an instance of PrometheusExporter was being reused without clearing it after every iteration. Release note: Fix bug in graphite metrics sender where cockroach was collecting and sending all data points since startup instead of only the latest data point. 31837: closedts: bump kv.closed_timestamp.target_duration default r=nvanbenschoten a=nvanbenschoten This change bumps the closed timestamp target duration to 30s. We're not using closed timestamps, except in special cases in 2.1, so it doesn't make sense for the setting to be so low if it risks pushing live transactions. Release note: None Co-authored-by: neeral <neeral@users.noreply.github.com> Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
Build succeeded |
Cockroach was emitting a ridiculously large number of metrics per minute
because an instance of PrometheusExporter was being reused without
clearing it after every iteration.
Release note:
Fix bug in graphite metrics sender where cockroach was collecting and
sending all data points since startup instead of only the latest data
point.