Unregister metrics when closing broker #991
Conversation
Wanted to do this for a while, but upstream didn't have an unregister method.
|
This will reduce most of the memory leak as it is mostly tied to
Also if a custom I can provide a similar PR for unregistering broker metrics when a Here is a benchmark that can be used with func BenchmarkNewClientWithMeterMetric(b *testing.B) {
setupFunctionalTest(b)
defer teardownFunctionalTest(b)
for i := 0; i < b.N; i++ {
client, err := NewClient(kafkaBrokers, nil)
if err != nil {
b.Fatal(err)
}
safeClose(b, client)
}
}And the |
Why? Is this PR not backwards-compatible in some way I'm missing? Does it make sense to merge this PR and open another one to unregister metrics on |
|
Sorry for the confusion. What I meant is that unregistering all the metrics (including the meters that leak memory) in a backward compatible way (e.g. not introducing breaking changes or requiring calling a new method for cleaning up) is not trivial as they are created in various places. I can submit such a PR for unregistering all the metrics in the next few days but because they will touch the same lines it probably make sense to keep this one till you decide which solution is more sensible. |
|
So I dug through the metrics a bit more and IIUC they fall in three categories:
Have I missed a set or do you have a clever idea for the latter two? |
|
The common scope of all the metrics is the The One solution would be to expose a struct implementing Then whenever we close a So the idea would be to tie the life cycle of the metrics to the life cycle of the Also, one other use case that comes to mind would be to have producer metrics like The PR implementing such behavior should clear things up I hope and provide good material for discussion. |
Right
The registry itself provides a Ugh, this is messy. Thanks for the detailed explanation - I'm gonna merge this PR and leave the rest of it well enough alone. This fixes the majority of the realistic issues and as you mentioned everything else gets super-complicated because we don't have anything which precisely matches the right scopes. Not worth spending time on right now as long as it's just hypothetical. |
|
👍 Fair enough, keeping it simple. The only drawback of using package main
import (
"fmt"
"os"
"github.com/Shopify/sarama"
"github.com/rcrowley/go-metrics"
)
func main() {
appMetricRegistry := metrics.NewRegistry()
appGauge := metrics.GetOrRegisterGauge("m1", appMetricRegistry)
appGauge.Update(1)
config := sarama.NewConfig()
// Use a prefix registry instead of the default local one
config.MetricRegistry = metrics.NewPrefixedChildRegistry(appMetricRegistry, "sarama.")
// Simulate a metric created by sarama without starting a broker
saramaGauge := metrics.GetOrRegisterGauge("m2", config.MetricRegistry)
saramaGauge.Update(2)
fmt.Println("App metrics before:")
metrics.WriteOnce(appMetricRegistry, os.Stdout)
// Unregister all the sarama metrics
// but will unregister the parent registry metrics too...
config.MetricRegistry.UnregisterAll()
fmt.Println("App metrics after:")
metrics.WriteOnce(appMetricRegistry, os.Stdout)
// Output:
// App metrics before:
// gauge m1
// value: 1
// gauge sarama.m2
// value: 2
// App metrics after:
}But this is a bug in go-metrics and one can easily build a different implementation of |
@slaunay @mihasya @lday0321 this should hopefully solve #897? I don't know if I've covered all the cases.