transport: enable keep-alives and limit number of sockets #284
Conversation
|
I need to confirm yet this actually does what I think it does. |
lib/transports.js
Outdated
| @@ -6,12 +6,16 @@ var util = require('util'); | |||
| function Transport() {} | |||
| util.inherits(Transport, events.EventEmitter); | |||
|
|
|||
| var agentOptions = { keepalive: true, maxSockets: 100 }; | |||
There was a problem hiding this comment.
100 seems high to me? Also, should we provide a maxFreeSockets option? (https://nodejs.org/api/http.html#http_new_agent_options)
There was a problem hiding this comment.
Number of maxFreeSockets won't go over maxSockets number and it defaults to 256 so we should be good here.
|
|
||
| function HTTPTransport(options) { | ||
| this.defaultPort = 80; | ||
| this.transport = http; | ||
| this.options = options || {}; | ||
| this.agent = httpAgent; |
There was a problem hiding this comment.
We could theoretically enable users to change agent options via something like:
Raven.config('___DSN___', {
transport: new raven.transports.HTTPSTransport({agentOpts: { ... }})
});but I'm not sure whether we need to or should; are there cases where users might want to do this (or we might want them to)? Guessing you have more background on our intentions behind transports being pluggable/user-configurable.
There was a problem hiding this comment.
Agree on enabling user to change those settings + adding a comment in the docs about it
|
ping |
|
Ping @mattrobenolt. Can you work on it? Or do you want me to take over? |
lib/transports.js
Outdated
| @@ -6,12 +6,16 @@ var util = require('util'); | |||
| function Transport() {} | |||
| util.inherits(Transport, events.EventEmitter); | |||
|
|
|||
| var agentOptions = { keepalive: true, maxSockets: 100 }; | |||
There was a problem hiding this comment.
Number of maxFreeSockets won't go over maxSockets number and it defaults to 256 so we should be good here.
|
|
||
| function HTTPTransport(options) { | ||
| this.defaultPort = 80; | ||
| this.transport = http; | ||
| this.options = options || {}; | ||
| this.agent = httpAgent; |
There was a problem hiding this comment.
Agree on enabling user to change those settings + adding a comment in the docs about it
It does. This would be nice to have. |
kamilogorek
force-pushed
the
keepalive
branch
2 times, most recently
from
2101f66 to
dbcd4e9
Compare
kamilogorek
force-pushed
the
keepalive
branch
from
dbcd4e9 to
40720ed
Compare
|
I just realised that this code has been partially broken due to incorrect socket name being constructed. Fixed and updated test. This should also fix #214 enough for it to be closed. |
kamilogorek
force-pushed
the
keepalive
branch
from
b8e2c5d to
0953511
Compare
kamilogorek
force-pushed
the
keepalive
branch
from
0953511 to
d2750f8
Compare
No description provided.