Adds proxy argument for routing HttpLogger through a proxy #206
Conversation
…a proxy - [package.json] added http/s proxy agents to dependencies - [package.json] added proxy middleware for express to test proxy behaviour - [HttpLogger.js] added proxy parameter in constructor - [HttpLogger.js] added createProxyAgentIfAvailable() instance method - [HttpLogger.js] added agent property to accept proxy agent - [integrationTest.js] replicated existing tests using a simulated proxy server - using http-proxy-middleware instead of express-http-proxy for tests because of conflict with restify - fixed eslint issue
codefromthecrypt
changed the title
| @@ -13,12 +13,15 @@ | |||
| "license": "Apache-2.0", | |||
| "repository": "https://github.com/openzipkin/zipkin-js", | |||
| "dependencies": { | |||
| "http-proxy-agent": "^2.1.0", | |||
There was a problem hiding this comment.
what's the risk of having these dependencies?
|
sorry, it's my first time hearing that question, how should i assess that?
… On 16 Apr 2018, at 18:33, Adrian Cole ***@***.***> wrote:
@adriancole commented on this pull request.
In packages/zipkin-transport-http/package.json:
> @@ -13,12 +13,15 @@
"license": "Apache-2.0",
"repository": "https://github.com/openzipkin/zipkin-js",
"dependencies": {
+ "http-proxy-agent": "^2.1.0",
what's the risk of having these dependencies?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
On Mon, Apr 16, 2018 at 6:44 PM, Joseph Matthias Goh < ***@***.***> wrote:
sorry, it's my first time hearing that question, how should i assess that?
So, I guess I'm assuming that this transport is used by a lot of users
users. Sometimes dependencies can conflict with application code,
especially if they change incompatibly a lot or pull a string of deps that
can also conflict. Sortof asking for some summary judgement
|
|
@adriancole i see, the |
|
do you anticipate this would break those using react or browser functionality? cc @DanielMSchmidt @micaelsampaio |
|
@adriancole i suspect it might, didn't consider that- are there any known open source projects using browser functionality so i can quickly replicate it to test? |
|
@zephinzer could you proceed with the test you mention? |
|
lacking an automatic browser test, it would in my opinion suffice if you manually test by locally publishing this, editing the zipkin-js-example to use a proxy, and testing it https://github.com/openzipkin/zipkin-js-example I don't actually know of an automatic browserify test, and how to stitch it into circleci... maybe @eirslett does perhaps, but seems a separate issue to create that infra cc @openzipkin/devops-tooling |
|
Any chance instead we add the dependencies, we just add an interface for proxy and allow users to inject it? @zephinzer |
|
decoration somehow sounds good. strategically: bear in mind we probably will want to redo the http logger internals soon, so that the "sender" concept is extracted. This will reduce the code about http to very small and makes proxy or alternate impls easy. |
|
This is fixed in #439 hence I am closing this issue. Thanks @zephinzer anyways for the effort! |
Fixes #205