Skip to content

Commit

Permalink
fix: add mayurkale22 recommendations
Browse files Browse the repository at this point in the history 
Signed-off-by: Olivier Albertini <olivier.albertini@montreal.ca>
  • Loading branch information
@OlivierAlbertini
OlivierAlbertini committed Aug 11, 2019
1 parent a8519f0 commit 2e4d0ef
Show file tree
Hide file tree
Showing 13 changed files with 204 additions and 91 deletions.
7 changes: 3 additions & 4 deletions packages/opentelemetry-plugin-http/package.json
View file
Expand Up @@ -38,16 +38,15 @@
"access": "public"
},
"devDependencies": {
"@types/got": "^9.6.6",
"@types/mocha": "^5.2.7",
"@types/nock": "^10.0.3",
"@types/node": "^12.6.9",
"@types/request-promise-native": "^1.0.16",
"@types/semver": "^6.0.1",
"@types/shimmer": "^1.0.1",
"@types/sinon": "^7.0.13",
"@types/axios": "0.14.0",
"@types/got": "9.6.6",
"@types/superagent": "4.1.3",
"@types/request-promise-native": "1.0.16",
"@types/superagent": "^4.1.3",
"@opentelemetry/scope-async-hooks": "^0.0.1",
"@opentelemetry/scope-base": "^0.0.1",
"axios": "^0.19.0",
Expand Down
2 changes: 1 addition & 1 deletion ...metry-plugin-http/src/enums/attributes.ts → ...y-plugin-http/src/enums/attributeNames.ts
View file
Expand Up @@ -18,7 +18,7 @@
* Attributes Names according to Opencensus HTTP Specs since there is no specific OpenTelemetry Attributes
* https://github.com/open-telemetry/opentelemetry-specification/blob/master/work_in_progress/opencensus/HTTP.md#attributes
*/
export enum Attributes {
export enum AttributeNames {
HTTP_HOSTNAME = 'http.hostname',
// NOT ON OFFICIAL SPEC
ERROR = 'error',
Expand Down
104 changes: 50 additions & 54 deletions packages/opentelemetry-plugin-http/src/http.ts
View file
Expand Up @@ -21,6 +21,7 @@ import {
SpanOptions,
Logger,
Tracer,
Attributes,
} from '@opentelemetry/types';
import {
ClientRequest,
Expand All @@ -41,34 +42,27 @@ import {
HttpRequestArgs,
} from './types';
import { Format } from './enums/format';
import { Attributes } from './enums/attributes';
import { AttributeNames } from './enums/attributeNames';
import { Utils } from './utils';
import { SpanFactory } from './models/spanFactory';

/**
* Http instrumentation plugin for Opentelemetry
*/
export class HttpPlugin extends BasePlugin<Http> {
static readonly component = 'http';
options!: HttpPluginConfig;

// TODO: BasePlugin should pass the logger or when we enable the plugin
protected _logger!: Logger;
protected readonly _tracer!: Tracer;
/**
* Used to create span with default attributes
*/
protected _spanFactory!: SpanFactory;

constructor(public moduleName: string, public version: string) {
super();
// TODO: remove this once a logger will be passed
// https://github.com/open-telemetry/opentelemetry-js/issues/193
this._logger = new NoopLogger();
}

/** Patches HTTP incoming and outcoming request functions. */
protected patch() {
this._spanFactory = new SpanFactory(this._tracer, HttpPlugin.component);
this._logger.debug(
'applying patch to %s@%s',
this.moduleName,
Expand Down Expand Up @@ -131,7 +125,7 @@ export class HttpPlugin extends BasePlugin<Http> {
*/
protected _getPatchIncomingRequestFunction() {
return (original: (event: string, ...args: unknown[]) => boolean) => {
return this.incomingRequestFunction(original);
return this._incomingRequestFunction(original);
};
}

Expand All @@ -141,7 +135,7 @@ export class HttpPlugin extends BasePlugin<Http> {
*/
protected _getPatchOutgoingRequestFunction() {
return (original: Func<ClientRequest>): Func<ClientRequest> => {
return this.outgoingRequestFunction(original);
return this._outgoingRequestFunction(original);
};
}

Expand Down Expand Up @@ -172,14 +166,15 @@ export class HttpPlugin extends BasePlugin<Http> {
* span when the response is finished.
* @param request The original request object.
* @param options The arguments to the original function.
* @param span representing the current operation
*/
private getMakeRequestTraceFunction(
private _getMakeRequestTraceFunction(
request: ClientRequest,
options: ParsedRequestOptions,
span: Span
): Func<ClientRequest> {
return (): ClientRequest => {
this._logger.debug('makeRequestTrace');
this._logger.debug('makeRequestTrace by injecting context into header');

const propagation = this._tracer.getHttpTextFormat();
const opts = Utils.getIncomingOptions(options);
Expand All @@ -202,26 +197,27 @@ export class HttpPlugin extends BasePlugin<Http> {
: null;

const host = opts.hostname || opts.host || 'localhost';
span.setAttributes({
[Attributes.HTTP_URL]: `${opts.protocol}//${opts.hostname}${opts.path}`,
[Attributes.HTTP_HOSTNAME]: host,
[Attributes.HTTP_METHOD]: method,
[Attributes.HTTP_PATH]: opts.path || '/',
});

const attributes: Attributes = {
[AttributeNames.HTTP_URL]: `${opts.protocol}//${opts.hostname}${opts.path}`,
[AttributeNames.HTTP_HOSTNAME]: host,
[AttributeNames.HTTP_METHOD]: method,
[AttributeNames.HTTP_PATH]: opts.path || '/',
};

if (userAgent) {
span.setAttribute(Attributes.HTTP_USER_AGENT, userAgent);
attributes[AttributeNames.HTTP_USER_AGENT] = userAgent;
}

if (response.statusCode) {
span
.setAttributes({
[Attributes.HTTP_STATUS_CODE]: response.statusCode,
[Attributes.HTTP_STATUS_TEXT]: response.statusMessage,
})
.setStatus(Utils.parseResponseStatus(response.statusCode));
attributes[AttributeNames.HTTP_STATUS_CODE] = response.statusCode;
attributes[AttributeNames.HTTP_STATUS_TEXT] =
response.statusMessage;
span.setStatus(Utils.parseResponseStatus(response.statusCode));
}

span.setAttributes(attributes);

if (this.options.applyCustomAttributesOnSpan) {
this.options.applyCustomAttributesOnSpan(span, request, response);
}
Expand All @@ -238,7 +234,7 @@ export class HttpPlugin extends BasePlugin<Http> {
};
}

private incomingRequestFunction(
private _incomingRequestFunction(
original: (event: string, ...args: unknown[]) => boolean
) {
const plugin = this;
Expand Down Expand Up @@ -278,10 +274,7 @@ export class HttpPlugin extends BasePlugin<Http> {
spanOptions.parent = spanContext;
}

const span = plugin._spanFactory.createInstance(
`${method} ${pathname}`,
spanOptions
);
const span = plugin._startHttpSpan(`${method} ${pathname}`, spanOptions);

return plugin._tracer.withSpan(span, () => {
plugin._tracer.wrapEmitter(request);
Expand All @@ -306,31 +299,28 @@ export class HttpPlugin extends BasePlugin<Http> {
const userAgent = (headers['user-agent'] ||
headers['User-Agent']) as string;

span.setAttributes({
[Attributes.HTTP_URL]: Utils.getUrlFromIncomingRequest(
const attributes: Attributes = {
[AttributeNames.HTTP_URL]: Utils.getUrlFromIncomingRequest(
requestUrl,
headers
),
[Attributes.HTTP_HOSTNAME]: hostname,
[Attributes.HTTP_METHOD]: method,
});
[AttributeNames.HTTP_HOSTNAME]: hostname,
[AttributeNames.HTTP_METHOD]: method,
[AttributeNames.HTTP_STATUS_CODE]: response.statusCode,
[AttributeNames.HTTP_STATUS_TEXT]: response.statusMessage,
};

if (requestUrl) {
span.setAttributes({
[Attributes.HTTP_PATH]: requestUrl.path || '/',
[Attributes.HTTP_ROUTE]: requestUrl.pathname || '/',
});
attributes[AttributeNames.HTTP_PATH] = requestUrl.path || '/';
attributes[AttributeNames.HTTP_ROUTE] = requestUrl.pathname || '/';
}

if (userAgent) {
span.setAttribute(Attributes.HTTP_USER_AGENT, userAgent);
attributes[AttributeNames.HTTP_USER_AGENT] = userAgent;
}

span
.setAttributes({
[Attributes.HTTP_STATUS_CODE]: response.statusCode,
[Attributes.HTTP_STATUS_TEXT]: response.statusMessage,
})
.setAttributes(attributes)
.setStatus(Utils.parseResponseStatus(response.statusCode));

if (plugin.options.applyCustomAttributesOnSpan) {
Expand All @@ -345,7 +335,7 @@ export class HttpPlugin extends BasePlugin<Http> {
};
}

private outgoingRequestFunction(
private _outgoingRequestFunction(
original: Func<ClientRequest>
): Func<ClientRequest> {
const plugin = this;
Expand Down Expand Up @@ -390,28 +380,34 @@ export class HttpPlugin extends BasePlugin<Http> {
// the first operation, therefore we create a span and call withSpan method.
if (!currentSpan) {
plugin._logger.debug('outgoingRequest starting a span without context');
const span = plugin._spanFactory.createInstance(
operationName,
spanOptions
);
const span = plugin._startHttpSpan(operationName, spanOptions);
return plugin._tracer.withSpan(
span,
plugin.getMakeRequestTraceFunction(request, optionsParsed, span)
plugin._getMakeRequestTraceFunction(request, optionsParsed, span)
);
} else {
plugin._logger.debug('outgoingRequest starting a child span');
const span = plugin._spanFactory.createInstance(operationName, {
const span = plugin._startHttpSpan(operationName, {
kind: spanOptions.kind,
parent: currentSpan,
});
return plugin.getMakeRequestTraceFunction(
return plugin._getMakeRequestTraceFunction(
request,
optionsParsed,
span
)();
}
};
}

private _startHttpSpan(name: string, options: SpanOptions) {
return this._tracer
.startSpan(name, options)
.setAttribute(AttributeNames.COMPONENT, HttpPlugin.component);
}
}

export const plugin = new HttpPlugin('http', process.versions.node);
export const plugin = new HttpPlugin(
HttpPlugin.component,
process.versions.node
);
17 changes: 0 additions & 17 deletions packages/opentelemetry-plugin-http/src/models/spanFactory.ts
View file

This file was deleted.

8 changes: 4 additions & 4 deletions packages/opentelemetry-plugin-http/src/utils.ts
View file
Expand Up @@ -22,7 +22,7 @@ import {
IncomingHttpHeaders,
} from 'http';
import { IgnoreMatcher, ParsedRequestOptions } from './types';
import { Attributes } from './enums/attributes';
import { AttributeNames } from './enums/attributeNames';
import * as url from 'url';

/**
Expand Down Expand Up @@ -143,9 +143,9 @@ export class Utils {
static setSpanOnError(span: Span, obj: IncomingMessage | ClientRequest) {
obj.on('error', error => {
span.setAttributes({
[Attributes.ERROR]: true,
[Attributes.HTTP_ERROR_NAME]: error.name,
[Attributes.HTTP_ERROR_MESSAGE]: error.message,
[AttributeNames.ERROR]: true,
[AttributeNames.HTTP_ERROR_NAME]: error.name,
[AttributeNames.HTTP_ERROR_MESSAGE]: error.message,
});

let status: Status;
Expand Down
2 changes: 1 addition & 1 deletion packages/opentelemetry-plugin-http/test/fixtures/google.json
View file
Expand Up @@ -40,4 +40,4 @@
],
"responseIsBinary": true
}
]
]
41 changes: 40 additions & 1 deletion packages/opentelemetry-plugin-http/test/utils.test.ts
View file
Expand Up @@ -17,10 +17,11 @@
import * as assert from 'assert';
import * as sinon from 'sinon';
import * as url from 'url';
import { CanonicalCode } from '@opentelemetry/types';
import { CanonicalCode, Attributes } from '@opentelemetry/types';
import { RequestOptions } from 'https';
import { IgnoreMatcher } from '../src/types';
import { Utils } from '../src/utils';
import * as http from 'http';

describe('Utils', () => {
describe('parseResponseStatus()', () => {
Expand Down Expand Up @@ -181,4 +182,42 @@ describe('Utils', () => {
assert.strictEqual(answer2, false);
});
});

describe('getUrlFromIncomingRequest()', () => {
it('should return absolute url with localhost', () => {
const path = '/test/1';
const result = Utils.getUrlFromIncomingRequest(url.parse(path), {});
assert.strictEqual(result, `http://localhost${path}`);
});
it('should return absolute url', () => {
const absUrl = 'http://www.google/test/1?query=1';
const result = Utils.getUrlFromIncomingRequest(url.parse(absUrl), {});
assert.strictEqual(result, absUrl);
});
it('should return default url', () => {
const result = Utils.getUrlFromIncomingRequest(null, {});
assert.strictEqual(result, 'http://localhost/');
});
});
describe('setSpanOnError()', () => {
it('should call span methods when we get an error event', done => {
/* tslint:disable-next-line:no-any */
const span: any = {
setAttributes: (obj: Attributes) => {},
setStatus: (status: unknown) => {},
end: () => {},
};
sinon.spy(span, 'setAttributes');
sinon.spy(span, 'setStatus');
sinon.spy(span, 'end');
const req = http.get('http://noop');
Utils.setSpanOnError(span, req);
req.on('error', () => {
assert.strictEqual(span.setAttributes.callCount, 1);
assert.strictEqual(span.setStatus.callCount, 1);
assert.strictEqual(span.end.callCount, 1);
done();
});
});
});
});
15 changes: 15 additions & 0 deletions packages/opentelemetry-plugin-http/test/utils/DummyPropagation.ts
View file
@@ -1,3 +1,18 @@
/**
* Copyright 2019, OpenTelemetry Authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
import { SpanContext, HttpTextFormat } from '@opentelemetry/types';
import * as http from 'http';

Expand Down

0 comments on commit 2e4d0ef

Please sign in to comment.