feat: domain handling

[AVVS]

We’ve recently integrated spam-agent-node for our production nodejs app monitoring - its a restify 4.x based http webserver. After we’ve integrated monitoring we’ve noticed that our memory was leaking. Upon investigation of heap dumps we’ve discovered that all the requests and responses are retained in memory and our bound to it’s Domain handler, which in turn is retained by existing timer, which is bound to the domain during domain.add(req) call and is originally created when patching http(s)Server in the spa agent, this creates a circular reference and a memory leak and effectively makes usage of the product impossible as we now have to restart the servers every day in order for them to operate.

A few screenshots and investigation paths:

spm-agent-node (https://github.com/sematext/spm-agent-nodejs/blob/master/lib/httpServerAgent.js#L30)
                               ^ measure/Timer (https://github.com/felixge/node-measured/blob/master/lib/metrics/Timer.js#L10)
                               ^ measure/Meter (https://github.com/felixge/node-measured/blob/master/lib/metrics/Meter.js#L7)

This PR makes sure that we exit current domain. The whole thing is really buggy, but this gets the job done. I've been running the server for the past several hours and memory is not leaking anymore

[otisg]

Looks like this somehow broke the build, @AVVS. We'll need to have a look.

[AVVS]

@otisg noticed a problem with stats being overwritten. If it's not too much hassle - I can add standard eslint instead of jshint as more modern approach

[otisg]

I can add standard eslint instead of jshint as more modern approach

What say you, @megastef ? Looks like we went through something like that in logagent-js already - sematext/logagent-js#60

[AVVS]

Should all be good now, please review/comment/merge ;)

[AVVS]

actually that interval that wasnt working before helped fix the leak. this doesnt really solve the issue yet, will work on it a bit more

[AVVS]

seems like the ctx.stats = Measured.createCollection() is the root cause of this or smth

[megastef]

Thanks for the contribution! @AVVS as far I know domains are deprecated see nodejs/node#66 and https://nodejs.org/api/domain.html, and this PR would add domain handling - I just wonder if we would create here a problem for future node versions.

[megastef]

Could we remove yarn.lock? https://github.com/AVVS/spm-agent-nodejs/blob/4faffe52d639ddfb1cd7692530c8bf7c44ff21ca/yarn.lock

[AVVS]

@megastef its up to you, for me it works much faster than general npm i --production and allows to pin all deps while making sure they wont update to something unexpected

[megastef]

Sure I see the value in it. We would need to maintain the yarn.lock file, I'm just afraid that we forget to update it - so we need a script to generate it before release.

[AVVS]

just use yarn for managing deps. They have just as good tooling - yarn will manage package.json for you This is similar no `npm-check -u` alias ycheck='yarn upgrade-interactive' yarn add <pkg> to add to deps yarn add <pkg> -D to add to dev

…

On Mar 8, 2017, at 9:49 AM, Stefan Thies ***@***.***> wrote: Sure I see the value in it. We would need to maintain the yarn.lock file, I'm just afraid that we forget to update it - so we need a script to generate it before release. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#11 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABol0YvqWYJGTXPJZ_oKFRsGnDBNJX5pks5rjuojgaJpZM4MWOpa>.