Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added agent option to http transport #439

Merged
merged 8 commits into from
Aug 19, 2019

Conversation

RAWeber
Copy link
Contributor

@RAWeber RAWeber commented Aug 12, 2019

Provide option to set a custom agent to use with node-fetch.

https://www.npmjs.com/package/node-fetch#custom-agent

This will allow users to specify any networking related options. For example, users can provide client certificates to hit mTLS endpoints, or CA certificates to use self-signed certificates.

Will use default agent if no agent option passed.

@codefromthecrypt
Copy link
Member

let's default this to the current version of the transport. ex

zipkin-transport-http/0.19.0

While not here, we've had quite a discussion elsewhere on it. openzipkin/zipkin-reporter-java#142

@RAWeber
Copy link
Contributor Author

RAWeber commented Aug 13, 2019

Based on node-fetches docs and the nodejs http.Agent docs which it refers, I believe the user-agent referred to in: openzipkin/zipkin-reporter-java#142 would simply be a matter of updating the header[user-agent] property, while the Agent option I've added would refer to an http.Agent.

The http.agent would be responsible for managing connection persistence and reuse for HTTP clients, such as sockets, certs, etc.

I believe zipkin-transport-http/0.19.0 could just be added to the default headers. I can add that to this commit if you would like, but it is not necessarily related to the agent parameter.

The current default uses the http.globalAgent, which has the base defaults (this would reflect current behavior where no http.agent is being used)

I've also updated the index.d.ts file to reflect the proper type of the agent option.

@codefromthecrypt
Copy link
Member

sorry wrong agent I guess!

declare class HttpLogger implements Logger {
constructor(options: {
endpoint: string,
httpInterval?: number,
jsonEncoder?: JsonEncoder,
httpTimeout?: number,
headers?: { [name: string]: any },
agent?: Agent | (() => Agent),
Copy link
Member

Choose a reason for hiding this comment

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

is (() => Agent) the same effect as null?

Let's add typescript and javascript tests for this parameter to make sure someone later doesn't accidentally break what you need.

Copy link
Contributor Author

@RAWeber RAWeber Aug 15, 2019

Choose a reason for hiding this comment

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

(() => Agent) is the equivalent of a function which returns an Agent

This follows node-fetch which will accept an HTTP(S) Agent or a function which returns a HTTP(S) Agent

I have also added typescript tests, to ensure the current functionality & also the newly added agent types.

@@ -14,7 +14,7 @@
"lint:ts": "tslint -c tslint.json -e './packages/**/node_modules/**/*.ts' './packages/**/*.ts'",
"lint": "npm-run-all --parallel lint:*",
"test:es": "npm run lerna-test",
"test:ts": "tsr packages/zipkin/test-types/*.test.ts --noAnnotate --libDeclarations && mocha --opts test/mocha-types.opts",
"test:ts": "tsr packages/*/test-types/*.test.ts --noAnnotate --libDeclarations && mocha --opts test/mocha-types.opts",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated this to process test-type/*.test.ts tests in all packages. This reflects the second mocha command which uses test/mocha-types.opts and attempts to run test-type tests in all packages.

@RAWeber
Copy link
Contributor Author

RAWeber commented Aug 15, 2019

Build failed on the Kafka Transport. The only change which could affect this (at least based on my current understanding of the project) is the change to the test:ts however, it failed on the test:es script, so I do not believe this was the cause.

Is there possibly another issue going on? Or an intermittent issue unrelated to these changes?

});

it('should accept Http(s) Agent or function which returns Agent', () => {
const agents = [new HttpAgent(), new HttpsAgent(), () => new HttpAgent(), () => new HttpsAgent()];
Copy link
Member

Choose a reason for hiding this comment

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

so this test makes sure it doesn't crash in the constructor. am I understanding that right?

I wonder if it would crash later. It might be confusing to have a test without assertions I mean.. wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ya it's testing the constructor. I've added an expect statement to confirm this.agent is properly being set and defaulting to null if it is not provided.

Beyond that, it is only being forwarded to the fetch call. Ideally I could confirm that fetch is being called with the agent, however due to the node-fetch implementation and sinon restrictions, it doesn't seem feasible to perform such a test without some hacky solutions.

@codefromthecrypt
Copy link
Member

kicked build in case it is a flake

Copy link
Member

@codefromthecrypt codefromthecrypt left a comment

Choose a reason for hiding this comment

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

nice work

@codefromthecrypt codefromthecrypt merged commit 98b3a68 into openzipkin:master Aug 19, 2019
@jcchavezs
Copy link
Contributor

jcchavezs commented Aug 20, 2019 via email

@RAWeber
Copy link
Contributor Author

RAWeber commented Aug 20, 2019

@jcchavezs looks to be the same agent supported by Axios, as both just accept a Nodejs http(s) Agent.

And I will look into adding some comments/documentation this week!

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

Successfully merging this pull request may close these issues.

3 participants