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

fixed content_length and status always ints #402

Merged
merged 1 commit into from
Apr 29, 2021
Merged
Show file tree
Hide file tree
Changes from all 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
8 changes: 2 additions & 6 deletions packages/core/lib/middleware/incoming_request_data.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
var { getHttpResponseData } = require('../segments/segment_utils');

/**
* Represents an incoming HTTP/HTTPS call.
Expand Down Expand Up @@ -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;
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
const { getHttpResponseData } = require('../segment_utils');
var { stripQueryStringFromPath } = require('../../utils');

/**
Expand All @@ -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);
}
};

Expand Down
4 changes: 4 additions & 0 deletions packages/core/lib/segments/segment_utils.d.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import * as http from 'http';

export const streamingThreshold: number;

export function getCurrentTime(): number;
Expand All @@ -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;
22 changes: 22 additions & 0 deletions packages/core/lib/segments/segment_utils.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
const { safeParseInt } = require('../utils');
var logger = require('../logger');

var DEFAULT_STREAMING_THRESHOLD = 100;
Expand Down Expand Up @@ -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']);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we keep recording the content-length even if it's 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes we should IMO, content-length can be 0 if there is a response with an empty body, which could still be valuable for a customer to know.

}
return ret;
}
};

Expand Down
2 changes: 2 additions & 0 deletions packages/core/lib/utils.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,3 +19,5 @@ export function objectWithoutProperties<T extends object, K extends keyof T>(
keys: K[],
preservePrototype?: boolean
): Omit<T, K>;

export function safeParseInt(val: number | string): number;
12 changes: 12 additions & 0 deletions packages/core/lib/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
};

Expand Down
29 changes: 29 additions & 0 deletions packages/core/test/unit/segments/segment_utils.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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, {});
});
});
});
17 changes: 17 additions & 0 deletions packages/core/test/unit/utils.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
});
});