Skip to content
This repository has been archived by the owner on Oct 23, 2023. It is now read-only.

Add support for sending events through proxy #397

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

ndmanvar
Copy link

For both HTTP and HTTPS endpoints. Uses tunnel-agent for https.

proxy: {
host: options.proxyHost,
port: options.proxyPort,
proxyAuth: null // TODO: Add ability to specify creds/auth
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will take care of this (for HTTPProxyTransport also)

@ndmanvar
Copy link
Author

ndmanvar commented Nov 17, 2017

@kamilogorek Can you please review and give me your thoughts/comments? (I know proxyAuth is still incomplete/TODO but wanted to get your thoughts/review on the rest of it)

@ndmanvar
Copy link
Author

Still need to write tests

@ndmanvar ndmanvar force-pushed the add_proxy_support branch 3 times, most recently from 33203dc to 954ee7a Compare November 17, 2017 09:32
For both HTTP and HTTPS endpoints. Uses tunnel-agent for https.
@ndmanvar
Copy link
Author

@kamilogorek can you please review when you get a chance? (:

@kamilogorek
Copy link
Contributor

@ndmanvar will do! I just wasn't sure if you're done with the tests :)

Copy link
Contributor

@kamilogorek kamilogorek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 = {};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't seem to use this object anywhere in tests

@@ -15,6 +15,92 @@ describe('transports', function() {
https.agent.maxSockets.should.equal(100);
});

it('should set HTTPS proxy transport when proxy config is specified and request is sent', function(done) {
var option = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/option/options/g

this.transport = http;
this.options = options || {};
this.agent = httpAgent;
this.options.host = options.proxyHost;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also check that it has been created correctly and assert on foo:123

@kamilogorek
Copy link
Contributor

@MaxBittker could you also take a peak at this code as well?

Copy link
Contributor

@MaxBittker MaxBittker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could this be commented to be more explanatory? not sure what these two branches mean

@ghost
Copy link

ghost commented May 28, 2018

Any updates on this? Would lodging a support request as a paying customer expedite this in anyway?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants