Oversampling Mitigation #541

carolabadeer · 2022-10-14T23:39:50Z

Issue #, if available: N/A

Description of changes: Implemented oversampling mitigation - added methods to create subsegments without sampling (control sampling decisions at the subsegment level) and an SQS message helper class

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

codecov-commenter · 2022-10-14T23:47:18Z

Codecov Report

Merging #541 (e99c271) into master (a1f4c79) will increase coverage by 0.36%.
The diff coverage is n/a.

❗ Current head e99c271 differs from pull request most recent head 95df8a6. Consider uploading reports for the commit 95df8a6 to get more accurate results

@@            Coverage Diff             @@
##           master     #541      +/-   ##
==========================================
+ Coverage   82.36%   82.73%   +0.36%     
==========================================
  Files          36       37       +1     
  Lines        1758     1784      +26     
==========================================
+ Hits         1448     1476      +28     
+ Misses        310      308       -2

Impacted Files	Coverage Δ
lib/env/sqs_message_helper.js	`83.33% <0.00%> (ø)`
lib/patchers/aws_p.js	`80.26% <0.00%> (+0.53%)`	⬆️
lib/segments/segment.js	`80.81% <0.00%> (+1.05%)`	⬆️
lib/segments/attributes/subsegment.js	`79.26% <0.00%> (+1.34%)`	⬆️
lib/env/aws_lambda.js	`91.52% <0.00%> (+1.69%)`	⬆️
lib/patchers/http_p.js	`94.31% <0.00%> (+2.18%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

atshaw43 · 2022-10-17T16:35:12Z

packages/core/lib/patchers/aws_p.js

@@ -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');


Should we stick with the variable name notTraced since customers are expecting that on segment?

atshaw43 · 2022-10-17T16:37:08Z

packages/core/lib/segments/attributes/subsegment.js

@@ -286,7 +304,7 @@ Subsegment.prototype.close = function close(err, remote) {
  }

  if (root && root.counter > SegmentUtils.getStreamingThreshold()) {
-    if (this.streamSubsegments() && this.parent) {
+    if (this.streamSubsegments() && this.parent) { // this.isSampled ??


Seems this comment needs to be taken out?

atshaw43 · 2022-10-17T16:42:48Z

packages/core/test/unit/env/sqs_message_helper.test.js

+        ]
+    }
+
+    it('testing sqsmessagehelper', function(){


Let's also have a test for when it doesn't have the sampled flag present.

atshaw43 · 2022-10-17T16:46:28Z

packages/core/lib/segments/attributes/subsegment.js

@@ -38,10 +39,24 @@ Subsegment.prototype.init = function init(name) {

 Subsegment.prototype.addNewSubsegment = function addNewSubsegment(name) {
  var subsegment = new Subsegment(name);
+  subsegment.isSampled = true;


We want to set this to the parent's value, not a default true.
It is possible the upstream call was not sampled. This will create an orphaned segment since its parent was never sent to the backend.

Great callout! I believe checking the value of subsegment.segment.notTraced would be more accurate in this case as to capture the parent segment's sampling decision not the parent subsegment (since it is possible to have subsegments of subsegments).

atshaw43 · 2022-10-17T16:49:14Z

packages/core/test/unit/segments/segment.test.js

@@ -262,6 +262,57 @@ describe('Segment', function() {
    });
  });

+  describe('#addSubsegmentWithoutSampling', function (){


Nice set of tests!

atshaw43 · 2022-10-17T17:04:49Z

packages/core/test/unit/segments/segment.test.js

+    })
+
+    it('should not sample subsegment or subsegment of subsegment - mix', function(){
+      var segment = new Segment('parent');


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
packages/core/test/unit/env/aws_lambda.test.js

I believe you need to call "init" here with that variable set.
packages/core/lib/env/aws_lambda.js

Let me know if you need more context here.

Thanks for pointing that out! I've incorporated that change into the updated unit tests

To answer the question above, yes, this was tested in Lambda using the sample sample JSON data used in the Java oversampling mitigation PR.

For trace header with sampled=1:

For trace header with sampled=0:

atshaw43 · 2022-10-17T17:05:42Z

Was this tested in Lambda directly?

atshaw43 · 2022-10-17T17:19:15Z

packages/core/test/unit/segments/segment.test.js

+      assert.equal(child3.isSampled, false);
+    })
+  });
+


After we close the segment, is it possible to check if the emitter sent out the segment or not?
And can we examine the trace header of the subsegment that is sent downstream to see what the sample flag is set to?

willarmiros · 2022-11-01T04:01:38Z

packages/core/lib/patchers/aws_p.js

@@ -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');


willarmiros · 2022-11-01T04:01:49Z

packages/core/lib/segments/attributes/subsegment.js

+};
+
+Subsegment.prototype.addNewSubsegmentWithoutSampling = function addNewSubsegmentWithoutSampling(name){
+  var subsegment = new Subsegment(name);


Nit: use const

willarmiros · 2022-11-01T04:04:35Z

packages/core/lib/env/sqs_message_helper.js

+  constructor() {
+  }
+
+  isSampled(message) { 


Can we make this static? Would need to update docs as well

atshaw43 · 2022-11-08T22:05:19Z

packages/core/lib/context_utils.js

@@ -42,11 +42,11 @@ var contextUtils = {
   * @alias module:context_utils.resolveManualSegmentParams
   */

-  resolveManualSegmentParams: function resolveManualSegmentParams(params) {
+  resolveManualSegmentParams: function resolveManualSegmentParams (params) {


Confirmed with Carol,
The linter has not been run in a long time which is causing the large code churn.

atshaw43 · 2022-11-08T22:14:53Z

packages/core/lib/patchers/aws3_p.ts

@@ -110,15 +110,21 @@ const getXRayMiddleware = (config: RegionResolvedConfig, manualSegment?: Segment
    return next(args);
  }

-  const subsegment: Subsegment = segment.addNewSubsegment(service);
+  let subsegment: Subsegment;
+  if (segment.notTraced == false || !segment.subsegments[segment.subsegments.length - 1].notTraced) {


Would this also work?
if (!segment.subsegments[segment.subsegments.length - 1].notTraced)

This is more of a design decision: check the parent sampling decision here for unsampled segments, or only check the subsegment and rely on the emitter not sending the entire segment if it is unsampled down the line. If the latter was used in the other SDKs, I am happy to incorporate the same design here as well.

atshaw43 · 2022-11-08T22:16:59Z

packages/core/lib/patchers/aws_p.js


-  var buildListener = function(req) {
+  let subsegment;
+  if (parent.notTraced === false || !parent.subsegments[parent.subsegments.length - 1].notTraced) {


Same here. Can we just look at the subsegment and not have the segment check?

atshaw43 · 2022-11-08T22:42:28Z

packages/core/test/unit/patchers/aws_p.test.js

+      awsClient.call(awsRequest);
+
+      setTimeout(function () {
+        logStub.should.have.been.calledOnce;


If it is not supposed to sample, then should this have emitted?

I believe this is unrelated - just checking if a log is produced. There is another test right below checking the trace header being sent out

…lint" This reverts commit f5ce869.

This reverts commit 7cde0b2.

willarmiros

LGTM - please wait for @atshaw43 review too

added implementation for oversampling mitigation

b2531df

carolabadeer requested a review from a team as a code owner October 14, 2022 23:39

removed unnecessary comments and whitespaces

e1c0e9a

atshaw43 suggested changes Oct 17, 2022

View reviewed changes

addressed feedback and fixed style issues

6533d48

willarmiros reviewed Nov 1, 2022

View reviewed changes

carolabadeer added 5 commits November 3, 2022 12:57

fixed sampling decision bug and updated unit tests

e53c6b4

Make SqsMessageHelper static

9b2660a

Fixed trace headers and added unit tests

5342ac9

fixed style with eslint

7cde0b2

renamed isSampled flag to notTraced and fixed styling with eslint

f5ce869

atshaw43 reviewed Nov 8, 2022

View reviewed changes

carolabadeer added 3 commits November 8, 2022 21:50

Revert "renamed isSampled flag to notTraced and fixed styling with es…

6a8c5d3

…lint" This reverts commit f5ce869.

Revert "fixed style with eslint"

524fbf9

This reverts commit 7cde0b2.

changed isSampled flag name to notTraced and fixed lint issues

95df8a6

carolabadeer force-pushed the oversampling-mitigation branch from e99c271 to 95df8a6 Compare November 9, 2022 07:09

fixed trace header bug

1816619

willarmiros approved these changes Nov 9, 2022

View reviewed changes

atshaw43 approved these changes Nov 10, 2022

View reviewed changes

carolabadeer merged commit 93a4f31 into aws:master Nov 10, 2022

osome-bot mentioned this pull request Jun 20, 2023

[Snyk] Security upgrade semver from 5.7.1 to 7.5.2 OsomePteLtd/aws-xray-sdk-node#3

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Oversampling Mitigation #541

Oversampling Mitigation #541

carolabadeer commented Oct 14, 2022

codecov-commenter commented Oct 14, 2022 •

edited

Loading

atshaw43 Oct 17, 2022 •

edited

Loading

willarmiros Nov 1, 2022

atshaw43 Oct 17, 2022

atshaw43 Oct 17, 2022

atshaw43 Oct 17, 2022

carolabadeer Oct 19, 2022 •

edited

Loading

atshaw43 Oct 17, 2022

carolabadeer Oct 19, 2022

atshaw43 Oct 17, 2022 •

edited

Loading

carolabadeer Oct 19, 2022

carolabadeer Oct 19, 2022 •

edited

Loading

atshaw43 commented Oct 17, 2022

atshaw43 Oct 17, 2022

willarmiros Nov 1, 2022

willarmiros Nov 1, 2022

willarmiros Nov 1, 2022

atshaw43 Nov 8, 2022

atshaw43 Nov 8, 2022

carolabadeer Nov 8, 2022

atshaw43 Nov 8, 2022

carolabadeer Nov 8, 2022

atshaw43 Nov 8, 2022

carolabadeer Nov 8, 2022

willarmiros left a comment

Oversampling Mitigation #541

Oversampling Mitigation #541

Conversation

carolabadeer commented Oct 14, 2022

codecov-commenter commented Oct 14, 2022 • edited Loading

Codecov Report

atshaw43 Oct 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carolabadeer Oct 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atshaw43 Oct 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carolabadeer Oct 19, 2022 • edited Loading

Choose a reason for hiding this comment

atshaw43 commented Oct 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willarmiros left a comment

Choose a reason for hiding this comment

codecov-commenter commented Oct 14, 2022 •

edited

Loading

atshaw43 Oct 17, 2022 •

edited

Loading

carolabadeer Oct 19, 2022 •

edited

Loading

atshaw43 Oct 17, 2022 •

edited

Loading

carolabadeer Oct 19, 2022 •

edited

Loading