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
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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.

"test": "npm run test:es && npm run test:ts",
"lerna-test": "lerna run test",
"lerna-test-browser": "lerna run test-browser",
Expand Down
9 changes: 8 additions & 1 deletion packages/zipkin-transport-http/index.d.ts
Original file line number Diff line number Diff line change
@@ -1,14 +1,21 @@
import {Agent as HttpAgent } from 'http';
import {Agent as HttpsAgent} from 'https';
import {JsonEncoder, Logger, model} from 'zipkin';

type Agent = HttpAgent | HttpsAgent;

declare class HttpLogger implements Logger {
constructor(options: {
endpoint: string,
httpInterval?: number,
jsonEncoder?: JsonEncoder,
httpTimeout?: number,
timeout?: number,
maxPayloadSize?: 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.

log?: Console
});

logSpan(span: model.Span): void;
}
export {HttpLogger};
3 changes: 3 additions & 0 deletions packages/zipkin-transport-http/src/HttpLogger.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ class HttpLogger extends EventEmitter {
constructor({
endpoint,
headers = {},
agent = null,
httpInterval = 1000,
jsonEncoder = JSON_V1,
timeout = 0,
Expand All @@ -23,6 +24,7 @@ class HttpLogger extends EventEmitter {
super(); // must be before any reference to *this*
this.log = log;
this.endpoint = endpoint;
this.agent = agent;
this.maxPayloadSize = maxPayloadSize;
this.queue = [];
this.queueBytes = 0;
Expand Down Expand Up @@ -88,6 +90,7 @@ class HttpLogger extends EventEmitter {
body: postBody,
headers: self.headers,
timeout: self.timeout,
agent: self.agent
}).then((response) => {
if (response.status !== 202 && response.status !== 200) {
const err = 'Unexpected response while sending Zipkin data, status:'
Expand Down
37 changes: 37 additions & 0 deletions packages/zipkin-transport-http/test-types/HttpLogger.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
import { expect } from 'chai';

import {Agent as HttpAgent } from 'http';
import {Agent as HttpsAgent} from 'https';
import {jsonEncoder} from 'zipkin';

import { HttpLogger } from 'zipkin-transport-http';

describe('HttpLogger', () => {
it('should have correct type', () => {
const options = {
endpoint: 'testEndpoint',
httpInterval: 1000,
jsonEncoder: jsonEncoder.JSON_V1,
timeout: 0,
maxPayloadSize: 0,
headers: {},
agent: new HttpAgent(),
log: console
};
const httpLogger: HttpLogger = new HttpLogger(options);

expect(httpLogger.logSpan).to.be.a('function');
});

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.


agents.forEach(agent => {
const options = {
endpoint: 'testEndpoint',
agent
};
const httpLogger: HttpLogger = new HttpLogger(options);
});
});
});