-
Notifications
You must be signed in to change notification settings - Fork 155
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
Oversampling Mitigation #541
Changes from 3 commits
b2531df
e1c0e9a
6533d48
e53c6b4
9b2660a
5342ac9
7cde0b2
f5ce869
6a8c5d3
524fbf9
95df8a6
1816619
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
class SqsMessageHelper { | ||
constructor() { | ||
} | ||
|
||
isSampled(message) { | ||
const {attributes} = message; // extract attributes from message | ||
if (!('AWSTraceHeader' in attributes)) { | ||
return false; | ||
} | ||
return attributes['AWSTraceHeader'].includes('Sampled=1'); | ||
} | ||
} | ||
|
||
export default SqsMessageHelper; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -81,7 +81,8 @@ function captureAWSRequest(req) { | |
|
||
var buildListener = function(req) { | ||
req.httpRequest.headers['X-Amzn-Trace-Id'] = 'Root=' + traceId + ';Parent=' + subsegment.id + | ||
';Sampled=' + (subsegment.segment.notTraced ? '0' : '1'); | ||
';Sampled=' + (subsegment.isSampled ? '1' : '0'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we stick with the variable name notTraced since customers are expecting that on segment? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 |
||
|
||
}; | ||
|
||
var completeListener = function(res) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,6 +28,7 @@ Subsegment.prototype.init = function init(name) { | |
this.start_time = SegmentUtils.getCurrentTime(); | ||
this.in_progress = true; | ||
this.counter = 0; | ||
this.isSampled = true; | ||
}; | ||
|
||
/** | ||
|
@@ -39,9 +40,23 @@ Subsegment.prototype.init = function init(name) { | |
Subsegment.prototype.addNewSubsegment = function addNewSubsegment(name) { | ||
var subsegment = new Subsegment(name); | ||
this.addSubsegment(subsegment); | ||
subsegment.isSampled = subsegment.segment? !subsegment.segment.notTraced : true; | ||
return subsegment; | ||
}; | ||
|
||
Subsegment.prototype.addSubsegmentWithoutSampling = function addSubsegmentWithoutSampling(subsegment){ | ||
subsegment.isSampled = false; | ||
this.addSubsegment(subsegment); | ||
}; | ||
|
||
Subsegment.prototype.addNewSubsegmentWithoutSampling = function addNewSubsegmentWithoutSampling(name){ | ||
var subsegment = new Subsegment(name); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: use |
||
subsegment.isSampled = false; | ||
this.addSubsegment(subsegment); | ||
|
||
return subsegment; | ||
}; | ||
|
||
/** | ||
* Adds a subsegment to the array of subsegments. | ||
* @param {Subsegment} subsegment - The subsegment to append. | ||
|
@@ -60,11 +75,14 @@ Subsegment.prototype.addSubsegment = function(subsegment) { | |
subsegment.segment = this.segment; | ||
subsegment.parent = this; | ||
|
||
if (subsegment.end_time === undefined) { | ||
if (subsegment.end_time === undefined && subsegment.isSampled) { | ||
this.incrementCounter(subsegment.counter); | ||
} | ||
|
||
this.subsegments.push(subsegment); | ||
// don't push to subsegment array if subsegment is not sampled | ||
if(subsegment.isSampled){ | ||
this.subsegments.push(subsegment); | ||
} | ||
|
||
}; | ||
|
||
/** | ||
|
@@ -362,7 +380,7 @@ Subsegment.prototype.streamSubsegments = function streamSubsegments() { | |
var open = []; | ||
|
||
this.subsegments.forEach(function(child) { | ||
if (!child.streamSubsegments()) { | ||
if (!child.streamSubsegments()) { | ||
open.push(child); | ||
} | ||
}); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,85 @@ | ||
var assert = require('chai').assert; | ||
var chai = require('chai'); | ||
var sinonChai = require('sinon-chai'); | ||
|
||
import SqsMessageHelper from '../../../lib/env/sqs_message_helper'; | ||
|
||
chai.should(); | ||
chai.use(sinonChai); | ||
|
||
describe('#SqsMessageHelper', function (){ | ||
|
||
// sample records from https://docs.aws.amazon.com/lambda/latest/dg/with-sqs.html | ||
const sampleSqsMessageEvent = { | ||
"Records": [ | ||
{ | ||
"messageId": "059f36b4-87a3-44ab-83d2-661975830a7d", | ||
"receiptHandle": "AQEBwJnKyrHigUMZj6rYigCgxlaS3SLy0a...", | ||
"body": "Test message.", | ||
"attributes": { | ||
"ApproximateReceiveCount": "1", | ||
"SentTimestamp": "1545082649183", | ||
"SenderId": "AIDAIENQZJOLO23YVJ4VO", | ||
"ApproximateFirstReceiveTimestamp": "1545082649185", | ||
"AWSTraceHeader":"Root=1-632BB806-bd862e3fe1be46a994272793;Sampled=1" | ||
}, | ||
"messageAttributes": {}, | ||
"md5OfBody": "e4e68fb7bd0e697a0ae8f1bb342846b3", | ||
"eventSource": "aws:sqs", | ||
"eventSourceARN": "arn:aws:sqs:us-east-2:123456789012:my-queue", | ||
"awsRegion": "us-east-2" | ||
}, | ||
{ | ||
"messageId": "2e1424d4-f796-459a-8184-9c92662be6da", | ||
"receiptHandle": "AQEBzWwaftRI0KuVm4tP+/7q1rGgNqicHq...", | ||
"body": "Test message.", | ||
"attributes": { | ||
"ApproximateReceiveCount": "1", | ||
"SentTimestamp": "1545082650636", | ||
"SenderId": "AIDAIENQZJOLO23YVJ4VO", | ||
"ApproximateFirstReceiveTimestamp": "1545082650649", | ||
"AWSTraceHeader":"Root=1-5759e988-bd862e3fe1be46a994272793;Parent=53995c3f42cd8ad8;Sampled=0" | ||
}, | ||
"messageAttributes": {}, | ||
"md5OfBody": "e4e68fb7bd0e697a0ae8f1bb342846b3", | ||
"eventSource": "aws:sqs", | ||
"eventSourceARN": "arn:aws:sqs:us-east-2:123456789012:my-queue", | ||
"awsRegion": "us-east-2" | ||
}, | ||
{ | ||
"messageId": "2e1424d4-f796-459a-8184-9c92662be6da", | ||
"receiptHandle": "AQEBzWwaftRI0KuVm4tP+/7q1rGgNqicHq...", | ||
"body": "Test message.", | ||
"attributes": { | ||
"ApproximateReceiveCount": "1", | ||
"SentTimestamp": "1545082650636", | ||
"SenderId": "AIDAIENQZJOLO23YVJ4VO", | ||
"ApproximateFirstReceiveTimestamp": "1545082650649", | ||
"AWSTraceHeader":"Root=1-5759e988-bd862e3fe1be46a994272793;Parent=53995c3f42cd8ad8" | ||
}, | ||
"messageAttributes": {}, | ||
"md5OfBody": "e4e68fb7bd0e697a0ae8f1bb342846b3", | ||
"eventSource": "aws:sqs", | ||
"eventSourceARN": "arn:aws:sqs:us-east-2:123456789012:my-queue", | ||
"awsRegion": "us-east-2" | ||
} | ||
] | ||
} | ||
|
||
describe('SqsMessageHelper isSampled', function(){ | ||
let sqsMessageHelper = new SqsMessageHelper(); | ||
|
||
it('should return true when AWSTraceHeader has Sampled=1', function(){ | ||
assert.equal(sqsMessageHelper.isSampled(sampleSqsMessageEvent.Records[0]), true) | ||
}); | ||
|
||
it('should return false when AWSTraceHeader has Sampled=0', function(){ | ||
assert.equal(sqsMessageHelper.isSampled(sampleSqsMessageEvent.Records[1]), false) | ||
}); | ||
|
||
it('should return false when AWSTraceHeader has no Sampled flag', function(){ | ||
assert.equal(sqsMessageHelper.isSampled(sampleSqsMessageEvent.Records[2]), false) | ||
}); | ||
|
||
}) | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,8 @@ var SegmentUtils = require('../../../lib/segments/segment_utils'); | |
var Segment = require('../../../lib/segments/segment'); | ||
var Subsegment = require('../../../lib/segments/attributes/subsegment'); | ||
var logger = require('../../../lib/logger'); | ||
var Lambda = require('../../../lib/env/aws_lambda'); | ||
var Context = require('../../../lib/context_utils'); | ||
|
||
chai.should(); | ||
chai.use(sinonChai); | ||
|
@@ -262,6 +264,90 @@ describe('Segment', function() { | |
}); | ||
}); | ||
|
||
describe('#addSubsegmentWithoutSampling', function (){ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice set of tests! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you! |
||
let sandbox, setSegmentStub; | ||
|
||
beforeEach(function() { | ||
sandbox = sinon.createSandbox(); | ||
setSegmentStub = sandbox.stub(Context, 'setSegment'); | ||
}); | ||
|
||
afterEach(function() { | ||
sandbox.restore(); | ||
}); | ||
|
||
it('should have isSampled flag set to false for subsegment of Lambda facade segment', function(){ | ||
process.env._X_AMZN_TRACE_ID; | ||
Lambda.init(); | ||
|
||
setSegmentStub.should.have.been.calledOnce; | ||
|
||
let facade = setSegmentStub.args[0][0]; | ||
let unsampledSegment = facade.addNewSubsegmentWithoutSampling('unsampled-subsegment'); | ||
assert.equal(unsampledSegment.isSampled, false); | ||
}) | ||
|
||
it('should have isSampled flag set to true for subsegment of Lambda facade segment', function(){ | ||
process.env._X_AMZN_TRACE_ID; | ||
Lambda.init(); | ||
|
||
setSegmentStub.should.have.been.calledOnce; | ||
|
||
let facade = setSegmentStub.args[0][0]; | ||
let sampledSubsegment = facade.addNewSubsegment('sampled-subsegment'); | ||
assert.equal(sampledSubsegment.isSampled, true); | ||
}) | ||
|
||
it('should have isSampled flag set to false', function(){ | ||
var segment = new Segment('parent'); | ||
var child = new Subsegment('child'); | ||
segment.addSubsegmentWithoutSampling(child); | ||
|
||
assert.equal(child.isSampled, false); | ||
}) | ||
|
||
it('should have isSampled flag set to false for new subsegment', function(){ | ||
var segment = new Segment('parent'); | ||
var child = segment.addNewSubsegmentWithoutSampling('child'); | ||
|
||
assert.equal(child.isSampled, false); | ||
}) | ||
|
||
it('should not contain the child subsegment if not sampled', function(){ | ||
var segment = new Segment('parent'); | ||
var child = new Subsegment('child'); | ||
segment.addSubsegmentWithoutSampling(child); | ||
|
||
assert.notEqual(segment.subsegments[0], child); | ||
}) | ||
|
||
|
||
it('should not sample subsegment or subsegment of subsegment', function(){ | ||
var segment = new Segment('parent'); | ||
var child = new Subsegment('child'); | ||
var child2 = new Subsegment('child-2'); | ||
segment.addSubsegmentWithoutSampling(child); | ||
child.addSubsegmentWithoutSampling(child2) | ||
|
||
assert.equal(child.isSampled, false); | ||
assert.equal(child2.isSampled, false); | ||
}) | ||
|
||
it('should not sample subsegment or subsegment of subsegment - mix', function(){ | ||
var segment = new Segment('parent'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Us needing to create the segment here signals that we re not running through the lambda specific code. When the code thinks it is in lambda it will create a facade segment for us. The facade segment's trace data is filled out by what is in this variable. You can see an example in aws_lambda.test.js process.env._X_AMZN_TRACE_ID I believe you need to call "init" here with that variable set. Let me know if you need more context here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for pointing that out! I've incorporated that change into the updated unit tests There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
var child = new Subsegment('child'); | ||
var child2 = new Subsegment('child-2'); | ||
var child3 = new Subsegment('child-3'); | ||
segment.addSubsegmentWithoutSampling(child); | ||
child.addSubsegment(child2) | ||
child.addSubsegmentWithoutSampling(child3) | ||
|
||
assert.equal(child.isSampled, false); | ||
assert.equal(child2.isSampled, true); | ||
assert.equal(child3.isSampled, false); | ||
}) | ||
}); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After we close the segment, is it possible to check if the emitter sent out the segment or not? |
||
describe('#addError', function() { | ||
var err, exceptionStub, sandbox, segment; | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make this
static
? Would need to update docs as well