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

Instrumentation/HTTP2/HTTPS: Enforce strictNullChecks and noUnusedLocals #406

Merged
merged 2 commits into from
Mar 9, 2019
Merged

Instrumentation/HTTP2/HTTPS: Enforce strictNullChecks and noUnusedLocals #406

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
45b0d59
Enforce/strictNullChecks: HTTP2 and HTTPS Plugins
mayurkale22 Mar 8, 2019
4833bcc
Add check for isNaN
mayurkale22 Mar 8, 2019
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
37 changes: 20 additions & 17 deletions packages/opencensus-instrumentation-http2/src/http2.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,10 @@
* limitations under the License.
*/

import {Func, HeaderGetter, HeaderSetter, MessageEventType, Span, SpanKind, TraceOptions, Tracer} from '@opencensus/core';
import {Func, HeaderGetter, HeaderSetter, MessageEventType, Span, SpanKind, TraceOptions} from '@opencensus/core';
import {HttpPlugin} from '@opencensus/instrumentation-http';
import * as http from 'http';
import * as http2 from 'http2';
import * as net from 'net';
import * as shimmer from 'shimmer';
import * as tls from 'tls';
import * as url from 'url';
import * as uuid from 'uuid';

Expand Down Expand Up @@ -138,19 +135,20 @@ export class Http2Plugin extends HttpPlugin {
}

request.on('response', (responseHeaders: http2.IncomingHttpHeaders) => {
const statusCode = responseHeaders[':status'] || 200;
span.addAttribute(
Http2Plugin.ATTRIBUTE_HTTP_STATUS_CODE,
`${responseHeaders[':status']}`);
span.setStatus(
Http2Plugin.parseResponseStatus(+responseHeaders[':status']));
Http2Plugin.ATTRIBUTE_HTTP_STATUS_CODE, `${statusCode}`);
span.setStatus(Http2Plugin.parseResponseStatus(+statusCode));
});

request.on('end', () => {
const userAgent =
headers['user-agent'] || headers['User-Agent'] || null;

span.addAttribute(
Http2Plugin.ATTRIBUTE_HTTP_HOST, url.parse(authority).host);
const host = url.parse(authority).host;
if (host) {
span.addAttribute(Http2Plugin.ATTRIBUTE_HTTP_HOST, host);
}
span.addAttribute(
Http2Plugin.ATTRIBUTE_HTTP_METHOD, `${headers[':method']}`);
span.addAttribute(
Expand Down Expand Up @@ -209,21 +207,26 @@ export class Http2Plugin extends HttpPlugin {
}

const propagation = plugin.tracer.propagation;
const getter = {
const getter: HeaderGetter = {
getHeader(name: string) {
return headers[name];
}
} as HeaderGetter;
};

const traceOptions = {
name: headers[':path'],
const traceOptions: TraceOptions = {
name: headers[':path'] || '',
kind: SpanKind.SERVER,
spanContext: propagation ? propagation.extract(getter) : null
} as TraceOptions;
};
if (propagation) {
const spanContext = propagation.extract(getter);
if (spanContext) {
traceOptions.spanContext = spanContext;
}
}

// Respond is called in a stream event. We wrap it to get the sent
// status code.
let statusCode: number = null;
let statusCode: number;
const originalRespond = stream.respond;
stream.respond = function(this: http2.Http2Stream) {
// Unwrap it since respond is not allowed to be called more than once
Expand Down
34 changes: 16 additions & 18 deletions packages/opencensus-instrumentation-http2/test/test-http2.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,10 @@
*/


import {CoreTracer, RootSpan, Span, SpanEventListener, TracerConfig} from '@opencensus/core';
import {logger} from '@opencensus/core';
import {CoreTracer, logger, RootSpan, Span, SpanEventListener} from '@opencensus/core';
import * as assert from 'assert';
import * as http2 from 'http2';
import * as mocha from 'mocha';
import * as semver from 'semver';
import * as shimmer from 'shimmer';

import {plugin} from '../src/';
import {Http2Plugin} from '../src/';
Expand All @@ -39,7 +36,7 @@ class RootSpanVerifier implements SpanEventListener {

function assertSpanAttributes(
span: Span, httpStatusCode: number, httpMethod: string, hostName: string,
path: string, userAgent: string) {
path: string, userAgent?: string) {
assert.strictEqual(
span.status.code, Http2Plugin.parseResponseStatus(httpStatusCode));
assert.strictEqual(
Expand All @@ -48,8 +45,10 @@ function assertSpanAttributes(
span.attributes[Http2Plugin.ATTRIBUTE_HTTP_METHOD], httpMethod);
assert.strictEqual(span.attributes[Http2Plugin.ATTRIBUTE_HTTP_PATH], path);
assert.strictEqual(span.attributes[Http2Plugin.ATTRIBUTE_HTTP_ROUTE], path);
assert.strictEqual(
span.attributes[Http2Plugin.ATTRIBUTE_HTTP_USER_AGENT], userAgent);
if (userAgent) {
assert.strictEqual(
span.attributes[Http2Plugin.ATTRIBUTE_HTTP_USER_AGENT], userAgent);
}
assert.strictEqual(
span.attributes[Http2Plugin.ATTRIBUTE_HTTP_STATUS_CODE],
`${httpStatusCode}`);
Expand Down Expand Up @@ -100,12 +99,14 @@ describe('Http2Plugin', () => {
before(() => {
tracer.registerSpanEventListener(rootSpanVerifier);

plugin.enable(http2, tracer, VERSION, {}, null);
plugin.enable(http2, tracer, VERSION, {}, '');
server = http2.createServer();
server.on('stream', (stream, requestHeaders) => {
const statusCode = requestHeaders[':path'].length > 1 ?
+requestHeaders[':path'].slice(1) :
200;
const path = requestHeaders[':path'];
let statusCode = 200;
if (path) {
statusCode = path.length > 1 ? +path.slice(1) : 200;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: what would you think about using Number(path.slice(1)) to make the type conversion a bit more explicit?

And do we need to worry about malformed numbers here (that would give NaN to the Number constructor call)? If so, I'd say we can do in a follow up PR, but figured I'd mention as I'm thinking of it here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call, added isNaN check. PTAL

}
stream.respond({':status': statusCode, 'content-type': 'text/plain'});
stream.end(`${statusCode}`);
});
Expand Down Expand Up @@ -139,7 +140,7 @@ describe('Http2Plugin', () => {
rootSpanVerifier.endedRootSpans[1].name.indexOf(testPath) >= 0);

const span = rootSpanVerifier.endedRootSpans[1];
assertSpanAttributes(span, 200, 'GET', host, testPath, undefined);
assertSpanAttributes(span, 200, 'GET', host, testPath);
});
});

Expand All @@ -160,8 +161,7 @@ describe('Http2Plugin', () => {
0);

const span = rootSpanVerifier.endedRootSpans[1];
assertSpanAttributes(
span, errorCode, 'GET', host, testPath, undefined);
assertSpanAttributes(span, errorCode, 'GET', host, testPath);
});
});
});
Expand All @@ -179,8 +179,7 @@ describe('Http2Plugin', () => {
assert.ok(root.spans[0].name.indexOf(testPath) >= 0);
assert.strictEqual(root.traceId, root.spans[0].traceId);
const span = root.spans[0];
assertSpanAttributes(
span, statusCode, 'GET', host, testPath, undefined);
assertSpanAttributes(span, statusCode, 'GET', host, testPath);
});
});
});
Expand All @@ -201,8 +200,7 @@ describe('Http2Plugin', () => {
assert.strictEqual(root.traceId, root.spans[0].traceId);

const span = root.spans[0];
assertSpanAttributes(
span, errorCode, 'GET', host, testPath, undefined);
assertSpanAttributes(span, errorCode, 'GET', host, testPath);
});
});
});
Expand Down
4 changes: 2 additions & 2 deletions packages/opencensus-instrumentation-http2/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@
"pretty": true,
"module": "commonjs",
"target": "es6",
"strictNullChecks": false,
"strictNullChecks": true,
"noUnusedLocals": true
},
"include": [
"src/**/*.ts",
Expand All @@ -16,4 +17,3 @@
"node_modules"
]
}

1 change: 0 additions & 1 deletion packages/opencensus-instrumentation-https/src/https.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import * as http from 'http';
import * as https from 'https';
import * as semver from 'semver';
import * as shimmer from 'shimmer';
import * as url from 'url';

/** Https instrumentation plugin for Opencensus */
export class HttpsPlugin extends HttpPlugin {
Expand Down
28 changes: 11 additions & 17 deletions packages/opencensus-instrumentation-https/test/test-https.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,16 +14,12 @@
* limitations under the License.
*/

import {CoreTracer, RootSpan, Span, SpanEventListener, TracerConfig} from '@opencensus/core';
import {logger} from '@opencensus/core';
import {CoreTracer, logger, RootSpan, Span, SpanEventListener} from '@opencensus/core';
import * as assert from 'assert';
import * as fs from 'fs';
import {IncomingMessage} from 'http';
import * as https from 'https';
import * as mocha from 'mocha';
import * as nock from 'nock';
import * as shimmer from 'shimmer';
import * as url from 'url';

import {plugin} from '../src/';
import {HttpsPlugin} from '../src/';
Expand Down Expand Up @@ -83,7 +79,7 @@ const httpsOptions = {

function assertSpanAttributes(
span: Span, httpStatusCode: number, httpMethod: string, hostName: string,
path: string, userAgent: string) {
path: string, userAgent?: string) {
assert.strictEqual(
span.status.code, HttpsPlugin.parseResponseStatus(httpStatusCode));
assert.strictEqual(
Expand All @@ -92,8 +88,10 @@ function assertSpanAttributes(
span.attributes[HttpsPlugin.ATTRIBUTE_HTTP_METHOD], httpMethod);
assert.strictEqual(span.attributes[HttpsPlugin.ATTRIBUTE_HTTP_PATH], path);
assert.strictEqual(span.attributes[HttpsPlugin.ATTRIBUTE_HTTP_ROUTE], path);
assert.strictEqual(
span.attributes[HttpsPlugin.ATTRIBUTE_HTTP_USER_AGENT], userAgent);
if (userAgent) {
assert.strictEqual(
span.attributes[HttpsPlugin.ATTRIBUTE_HTTP_USER_AGENT], userAgent);
}
assert.strictEqual(
span.attributes[HttpsPlugin.ATTRIBUTE_HTTP_STATUS_CODE],
`${httpStatusCode}`);
Expand Down Expand Up @@ -127,7 +125,7 @@ describe('HttpsPlugin', () => {
(url: string) => url === `${urlHost}/ignored/function`
]
},
null);
'');
tracer.registerSpanEventListener(rootSpanVerifier);
server = https.createServer(httpsOptions, (request, response) => {
response.end('Test Server Response');
Expand Down Expand Up @@ -170,8 +168,7 @@ describe('HttpsPlugin', () => {
assert.strictEqual(rootSpanVerifier.endedRootSpans.length, 1);

const span = rootSpanVerifier.endedRootSpans[0];
assertSpanAttributes(
span, 200, 'GET', hostName, testPath, undefined);
assertSpanAttributes(span, 200, 'GET', hostName, testPath);
});
});

Expand All @@ -194,8 +191,7 @@ describe('HttpsPlugin', () => {
assert.strictEqual(rootSpanVerifier.endedRootSpans.length, 1);
const span = rootSpanVerifier.endedRootSpans[0];
assertSpanAttributes(
span, httpErrorCodes[i], 'GET', hostName, testPath,
undefined);
span, httpErrorCodes[i], 'GET', hostName, testPath);
});
});
}
Expand All @@ -212,8 +208,7 @@ describe('HttpsPlugin', () => {
assert.ok(root.spans[0].name.indexOf(testPath) >= 0);
assert.strictEqual(root.traceId, root.spans[0].traceId);
const span = root.spans[0];
assertSpanAttributes(
span, 200, 'GET', hostName, testPath, undefined);
assertSpanAttributes(span, 200, 'GET', hostName, testPath);
});
});
});
Expand All @@ -236,8 +231,7 @@ describe('HttpsPlugin', () => {

const span = root.spans[0];
assertSpanAttributes(
span, httpErrorCodes[i], 'GET', hostName, testPath,
undefined);
span, httpErrorCodes[i], 'GET', hostName, testPath);
});
});
});
Expand Down
4 changes: 2 additions & 2 deletions packages/opencensus-instrumentation-https/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@
"pretty": true,
"module": "commonjs",
"target": "es6",
"strictNullChecks": false
"strictNullChecks": true,
"noUnusedLocals": true
},
"include": [
"src/**/*.ts",
Expand All @@ -16,4 +17,3 @@
"node_modules"
]
}