From b616b51a58b3956655ad6db9d73a420425190c84 Mon Sep 17 00:00:00 2001 From: William Armiros <54150514+willarmiros@users.noreply.github.com> Date: Thu, 29 Apr 2021 10:35:35 -0600 Subject: [PATCH] fixed content_length and status always ints (#402) --- .../lib/middleware/incoming_request_data.js | 8 ++--- .../attributes/remote_request_data.js | 6 ++-- packages/core/lib/segments/segment_utils.d.ts | 4 +++ packages/core/lib/segments/segment_utils.js | 22 ++++++++++++++ packages/core/lib/utils.d.ts | 2 ++ packages/core/lib/utils.js | 12 ++++++++ .../test/unit/segments/segment_utils.test.js | 29 +++++++++++++++++++ packages/core/test/unit/utils.test.js | 17 +++++++++++ 8 files changed, 90 insertions(+), 10 deletions(-) diff --git a/packages/core/lib/middleware/incoming_request_data.js b/packages/core/lib/middleware/incoming_request_data.js index c0e68c5c..552661ff 100644 --- a/packages/core/lib/middleware/incoming_request_data.js +++ b/packages/core/lib/middleware/incoming_request_data.js @@ -1,3 +1,4 @@ +var { getHttpResponseData } = require('../segments/segment_utils'); /** * Represents an incoming HTTP/HTTPS call. @@ -49,12 +50,7 @@ var getClientIp = function getClientIp(req) { */ IncomingRequestData.prototype.close = function close(res) { - this.response = { - status: res.statusCode || '' - }; - - if (res.headers && res.headers['content-length']) - this.response.content_length = res.headers['content-length']; + this.response = getHttpResponseData(res); }; module.exports = IncomingRequestData; diff --git a/packages/core/lib/segments/attributes/remote_request_data.js b/packages/core/lib/segments/attributes/remote_request_data.js index 69867944..3f5ed865 100644 --- a/packages/core/lib/segments/attributes/remote_request_data.js +++ b/packages/core/lib/segments/attributes/remote_request_data.js @@ -1,3 +1,4 @@ +const { getHttpResponseData } = require('../segment_utils'); var { stripQueryStringFromPath } = require('../../utils'); /** @@ -23,10 +24,7 @@ RemoteRequestData.prototype.init = function init(req, res, downstreamXRayEnabled } if (res) { - this.response = { - status: res.statusCode || '', - content_length: (res.headers && res.headers['content-length']) ? res.headers['content-length'] : 0 - }; + this.response = getHttpResponseData(res); } }; diff --git a/packages/core/lib/segments/segment_utils.d.ts b/packages/core/lib/segments/segment_utils.d.ts index c3511407..197eb823 100644 --- a/packages/core/lib/segments/segment_utils.d.ts +++ b/packages/core/lib/segments/segment_utils.d.ts @@ -1,3 +1,5 @@ +import * as http from 'http'; + export const streamingThreshold: number; export function getCurrentTime(): number; @@ -13,3 +15,5 @@ export function setServiceData(serviceData: any): void; export function setStreamingThreshold(threshold: number): void; export function getStreamingThreshold(): number; + +export function getHttpResponseData(res: http.ServerResponse): object; diff --git a/packages/core/lib/segments/segment_utils.js b/packages/core/lib/segments/segment_utils.js index 2b28980f..5e5c9cfd 100644 --- a/packages/core/lib/segments/segment_utils.js +++ b/packages/core/lib/segments/segment_utils.js @@ -1,3 +1,4 @@ +const { safeParseInt } = require('../utils'); var logger = require('../logger'); var DEFAULT_STREAMING_THRESHOLD = 100; @@ -45,6 +46,27 @@ var utils = { getStreamingThreshold: function getStreamingThreshold() { return utils.streamingThreshold; + }, + + /** + * Parses an HTTP response object to return an X-Ray compliant HTTP response object. + * @param {http.ServerResponse} res + * @returns {Object} - X-Ray response object to be added to (sub)segment + */ + getHttpResponseData: (res) => { + const ret = {}; + if (!res) { + return ret; + } + + const status = safeParseInt(res.statusCode); + if (status !== 0) { + ret.status = status; + } + if (res.headers && res.headers['content-length']) { + ret.content_length = safeParseInt(res.headers['content-length']); + } + return ret; } }; diff --git a/packages/core/lib/utils.d.ts b/packages/core/lib/utils.d.ts index 88dd9930..328862f3 100644 --- a/packages/core/lib/utils.d.ts +++ b/packages/core/lib/utils.d.ts @@ -19,3 +19,5 @@ export function objectWithoutProperties( keys: K[], preservePrototype?: boolean ): Omit; + +export function safeParseInt(val: number | string): number; diff --git a/packages/core/lib/utils.js b/packages/core/lib/utils.js index efc28a8f..8d6dcc92 100644 --- a/packages/core/lib/utils.js +++ b/packages/core/lib/utils.js @@ -240,6 +240,18 @@ var utils = { target[property] = obj[property]; } return target; + }, + + /** + * Safely gets an integer from a string or number + * @param {String | Number} - input to cast to integer + * @returns {Number} - Integer representation of input, or 0 if input is not castable to int + */ + safeParseInt: (val) => { + if (!val || isNaN(val)) { + return 0; + } + return parseInt(val); } }; diff --git a/packages/core/test/unit/segments/segment_utils.test.js b/packages/core/test/unit/segments/segment_utils.test.js index 04994039..a425346c 100644 --- a/packages/core/test/unit/segments/segment_utils.test.js +++ b/packages/core/test/unit/segments/segment_utils.test.js @@ -14,4 +14,33 @@ describe('SegmentUtils', function() { assert.equal(SegmentUtils.streamingThreshold, 10); }); }); + + describe('#getHttpResponseData', () => { + it('should populate attributes as integers', () => { + const responseWithStrings = {statusCode: '200', headers: {'content-length': '42'}}; + const res = SegmentUtils.getHttpResponseData(responseWithStrings); + assert.deepEqual(res, { + 'content_length': 42, + 'status': 200 + }); + }); + + it('should omit missing properties', () => { + const responseWithStatus = {statusCode: 200}; + const responseWithLength = {headers: {'content-length': 42}}; + const emptyResponse = {}; + + const statusRes = SegmentUtils.getHttpResponseData(responseWithStatus); + const lengthRes = SegmentUtils.getHttpResponseData(responseWithLength); + const emptyRes = SegmentUtils.getHttpResponseData(emptyResponse); + + assert.deepEqual(statusRes, { + 'status': 200 + }); + assert.deepEqual(lengthRes, { + 'content_length': 42 + }); + assert.deepEqual(emptyRes, {}); + }); + }); }); diff --git a/packages/core/test/unit/utils.test.js b/packages/core/test/unit/utils.test.js index e79a8a7a..c28b42ef 100644 --- a/packages/core/test/unit/utils.test.js +++ b/packages/core/test/unit/utils.test.js @@ -368,4 +368,21 @@ describe('Utils', function() { assert.equal(Utils.wildcardMatch('?a?b*b?c?.?*.com', '1a2bjkdwfjkewb3c4.myelasticbeanstalkenv.org'), false); }); }); + + describe('#safeParseInt', () => { + it('should cast string to int', () => { + assert.equal(Utils.safeParseInt('42'), 42); + assert.equal(Utils.safeParseInt('15.0'), 15); + }); + + it('should cast numbers to int', () => { + assert.equal(Utils.safeParseInt(42), 42); + assert.equal(Utils.safeParseInt(15.0), 15); + }); + + it('should coerce NaN to zero', () => { + assert.equal(Utils.safeParseInt('not a number'), 0); + assert.equal(Utils.safeParseInt(NaN), 0); + }); + }); });