Conversation
blocksync/metrics.go
Outdated
|
|
||
| // unregisterGauge removes a gauge metric from the Prometheus registry. No new | ||
| // data will be generated in the http /metrics endpoint for this metric. | ||
| func unregisterGauge(g metrics.Gauge) bool { |
There was a problem hiding this comment.
I discussed this with @hvanz on Slack - I really don't think we should be unregistering Prometheus metrics. It seems risky, especially if external metrics systems come to rely on specific metrics being present. Plus it's really non-standard.
If we do really want to unregister metrics, there needs to be a very good reason.
There was a problem hiding this comment.
We discussed it synchronously. The best solution will be to unify certain metrics, such as the height, into a node-wide set of metrics. But that will need a bit more work on infra.
I agree we probably want to not unregister upon catching up, even if the resulting graph looks somewhat awkward.
There was a problem hiding this comment.
Thanks for the discussions! As a result, we won't unregister the metrics, and we'll have to think about a better solution for metrics at the node level.
* Add 4 metrics related to block * Remove unused function * Unregister metrics when BlockSync finishes * Add changelog * Rename method * Move method * Do not export unregister method * Do not unregister metrics
* Add 4 metrics related to block * Remove unused function * Unregister metrics when BlockSync finishes * Add changelog * Rename method * Move method * Do not export unregister method * Do not unregister metrics
Closes #543
PR checklist
.changelog(we use unclog to manage our changelog)docs/orspec/) and code comments