-
Notifications
You must be signed in to change notification settings - Fork 495
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix memory leak caused by registry ignoring metrics of external types #264
base: master
Are you sure you want to change the base?
Conversation
reverted out the move to sync.Map to keep backward compatibility before go 1.9. still not increasing in memory, this is ready for review. |
Thanks for tracking this down and submitting a PR. I wonder why this check was there in the first place. Nothing obvious in blame, let me see if I can track down @rcrowley and understand what the reasoning was for doing the check in the first place. |
I eventually did see some slow leaks come from other places in the code but haven't opened a PR for those yet (One of my running variants of this is perfectly happy with this change, no increase in memory, the other is increasing coming out of meter.go (they consume this slightly differently so I am hoping to get some more pprof dumps from the one that is increasing (albeit very slowly) from go-metrics) when I build a passthrough route in my main application (Java Ratpack) into the sidecar container using this library! |
(I know our big leak from registry.go was the fact we have a wrapper metric that uses other metrics types so it was being discarded from registry and just kept allocating new objects for the same metrics) |
This probably calls for a broader discussion of what makes a "metric" and if we should be using an interface here rather than manual type checking 🤔 I think the main thing preventing the use of an interface is that we want to make it possible for metrics to generate a @rcrowley and I exchanged some texts about this, and he confirmed my suspicion that the check is there to guard from crashes down the line, since both the argument to On that note, in your use of wrapper metric types, how were you accessing them later? Do you have your own function that uses |
In our case (and I'm trying to infer why it was chosen to be built a particular way because I'm not on the team that built it) We have an HTTPMetric that contains a Meter, Timer, and Histogram that all trickle into an internal metric pipeline with zero configuration for a consuming application team. This way anything using net/http or gin get metrics basically for free (https://youtu.be/cnHfK4MZA2Y?t=933 if you're curious about the whole services for free thing to our internal platform). |
@michaelschlies right, but if |
@michaelschlies also, isn't a Lines 8 to 28 in 9180854
FWIW, |
I can pass that along to the team that actually built this library internally, I was just trying to figure out why my application that started out at 2mb RAM utilized grew like crazy shrug |
Much appreciated. We should def fix this leak, but some feedback from them
would be helpful in figuring out a longer term plan for the registry.
On Sat, Aug 24, 2019 at 12:49 Michael Schlies ***@***.***> wrote:
I can pass that along to the team that actually built this library
internally, I was just trying to figure out why my application that started
out at 2mb RAM utilized grew like crazy *shrug*
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#264?email_source=notifications&email_token=AAAFQREICDFBJ7DV6HBCHODQGGGGBA5CNFSM4IMTQN2KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5CGLFQ#issuecomment-524576150>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAAFQRBNWAJQVWK2TGHI6JTQGGGGBANCNFSM4IMTQN2A>
.
--
Sent from Russia, with love. But also from my phone.
|
Apparently the person that built the specific Http metric struct isn't actually here anymore, so that's a bit of a bummer, but the only kind of funkiness appears to be in the histogram:
So I can't get the background on the why but we do have a fair amount of automation in several downstream applications wrapped in using our library (wrapping go-metrics in this case). |
fixes #244