Add support for sending events through proxy#397
Add support for sending events through proxy#397ndmanvar wants to merge 6 commits intogetsentry:masterfrom
Conversation
lib/transports.js
Outdated
| proxy: { | ||
| host: options.proxyHost, | ||
| port: options.proxyPort, | ||
| proxyAuth: null // TODO: Add ability to specify creds/auth |
There was a problem hiding this comment.
will take care of this (for HTTPProxyTransport also)
|
@kamilogorek Can you please review and give me your thoughts/comments? (I know |
|
Still need to write tests |
ndmanvar
force-pushed
the
add_proxy_support
branch
3 times, most recently
from
33203dc to
954ee7a
Compare
For both HTTP and HTTPS endpoints. Uses tunnel-agent for https.
ndmanvar
force-pushed
the
add_proxy_support
branch
from
954ee7a to
6cce30b
Compare
ndmanvar
force-pushed
the
add_proxy_support
branch
from
21febae to
9a8a4c2
Compare
|
@kamilogorek can you please review when you get a chance? (: |
|
@ndmanvar will do! I just wasn't sure if you're done with the tests :) |
kamilogorek
left a comment
kamilogorek
left a comment
There was a problem hiding this comment.
Looks fine to me. Would you mind adding some docs to describe how to use proxies as well? :)
| httpsProxyTransport.options.agent.proxyOptions.should.exist; | ||
|
|
||
| var _cachedAgent = httpsProxyTransport.agent; | ||
| var requests = {}; |
There was a problem hiding this comment.
You don't seem to use this object anywhere in tests
| }); | ||
|
|
||
| it('should set HTTPS proxy transport when proxy config is specified and request is sent', function(done) { | ||
| var option = { |
| this.transport = http; | ||
| this.options = options || {}; | ||
| this.agent = httpAgent; | ||
| this.options.host = options.proxyHost; |
There was a problem hiding this comment.
How about just using host port etc. instead? If we pass options object to this constructor, it's by definition mentioned to be used by proxy, therefore using a prefix proxyHost seems redundant.
There was a problem hiding this comment.
Or it may be useful to distinguish http from httpProxy options. It's up to you really.
| null, | ||
| null, | ||
| function() { | ||
| httpsProxyTransport.options.agent.proxyOptions.headers.host.should.exist; |
There was a problem hiding this comment.
We could also check that it has been created correctly and assert on foo:123
|
@MaxBittker could you also take a peak at this code as well? |
MaxBittker
left a comment
MaxBittker
left a comment
There was a problem hiding this comment.
hard to review some of the domain-specific logic, but if people are using some version of this successfully already that's a good signal! agree with kamil's comments, but i think this is looking good overall.
| 'api/' + | ||
| client.dsn.project_id + | ||
| '/store/'; | ||
| delete options.hostname; // only 'host' should be set when using proxy |
There was a problem hiding this comment.
slightly better comment here for what will happen if you send hostname or a link to a resource that justifies why?
| delete options.hostname; // only 'host' should be set when using proxy | ||
|
|
||
| if (this.options.proxyAuth) { | ||
| // might be able to use httpOverHttp agent |
There was a problem hiding this comment.
could this be commented to be more explanatory? not sure what these two branches mean
|
Any updates on this? Would lodging a support request as a paying customer expedite this in anyway? |
3 participants
For both HTTP and HTTPS endpoints. Uses tunnel-agent for https.