wip: sync cross agent tests

Author: amychisholm03

lib/config/index.js

@@ -1257,8 +1257,8 @@ function setTraceIDRatioSampler({ key, value, config, paths }) {
       }
       logger.trace('Setting %s environment variable', envVar)
     } else {
-      logger.error('Not setting %s environment variable. Setting sampler root to default.', envVar)
-      config.distributed_tracing.sampler.root = 'default'
+      logger.error('Not setting %s environment variable. Setting sampler %s to default.', envVar, key)
+      config.distributed_tracing.sampler[key] = 'default'
     }
   }
 }

lib/samplers/adaptive-sampler.js

@@ -33,6 +33,10 @@ class AdaptiveSampler {
     }
   }
 
+  toString() {
+    return 'AdaptiveSampler'
+  }
+
   get sampled() {
     return this._sampled
   }

lib/transaction/index.js

@@ -1065,20 +1065,26 @@ function decideSamplingFromW3cData({ transaction, traceparent, tracestate }) {
  * @param {Transaction} params.transaction The transaction to update.
  * @param {Tracestate} params.tracestate A tracestate object with embedded New Relic intrinsics.
  * @param {object} params.sampler The sampler to use, AdaptiveSampler or TraceIdRatioBasedSampler.
- * @param {string} params.samplerType A string describing the sampler being used
- * ('default', 'always_on', 'always_off').
+ * @param {string|object} params.samplerType A string describing the sampler being used
+ * ('default', 'always_on', 'always_off'), or an object if 'trace_id_ratio_based'.
  */
 function applySamplingStrategy({ transaction, tracestate, sampler, samplerType }) {
   if (sampler?.toString() === 'TraceIdRatioBasedSampler') {
     transaction.sampled = sampler.shouldSample(transaction.traceId)
-    // Force calc priority because transaction.sampled likely changed
+    // Force calc priority because transaction.sampled likekly has changed
     transaction._calculatePriority(sampler)
     return
   }
 
   // TODO: Make these their own samplers?
   switch (samplerType) {
     case 'default': {
+      // This case is if we're coming from the deprecated newrelic header logic
+      // where we don't want to override the sampled and priority values
+      if (!tracestate) {
+        return
+      }
+
       transaction.sampled = tracestate?.intrinsics ? tracestate.isSampled : null
       transaction.priority = tracestate?.intrinsics ? tracestate.priority : null
       break
@@ -1196,8 +1202,20 @@ const _dtDefineAttrsFromTraceData = function _dtDefineAttrsFromTraceData(data, t
   this.traceId = data.tr
 
   if (data.pr) {
+    // TODO: We need to consider the sampler before blindly accepting this value.
     this.priority = data.pr
     this.sampled = data.sa != null ? data.sa : this.sampled
+    // we don't have traceparent or tracestate, but this will update
+    // sampling decision based on our samplers
+    const {
+      remote_parent_sampled: parentSampled,
+      remote_parent_not_sampled: parentNotSampled
+    } = this.agent.config.distributed_tracing.sampler
+    if (data?.sa) {
+      applySamplingStrategy({ transaction: this, sampler: this.agent.remoteParentSampledSampler, samplerType: parentSampled })
+    } else {
+      applySamplingStrategy({ transaction: this, sampler: this.agent.remoteParentNotSampledSampler, samplerType: parentNotSampled })
+    }
   }
 
   if (data.tx) {

test/integration/distributed-tracing/trace-context-cross-agent.test.js

@@ -18,7 +18,7 @@ const recordSupportability = require('../../../lib/agent').prototype.recordSuppo
 
 const getDescendantValue = function (object, descendants) {
   const arrayDescendants = descendants.split('.')
-  const noop = () => {}
+  const noop = () => { }
   while (arrayDescendants.length && (object = object[arrayDescendants.shift()])) {
     noop()
   }
@@ -268,7 +268,7 @@ async function runTestCase(testCase, parentTest) {
     testExpectedFixtureKeys(testCase, [
       'account_id',
       'expected_metrics',
-      'force_sampled_true',
+      'force_adaptive_sampled_true',
       'inbound_headers',
       'intrinsics',
       'outbound_payloads',
@@ -279,7 +279,13 @@ async function runTestCase(testCase, parentTest) {
       'trusted_account_key',
       'web_transaction',
       'comment',
-      'transaction_events_enabled'
+      'transaction_events_enabled',
+      // Sampling
+      'remote_parent_sampled',
+      'remote_parent_not_sampled',
+      'root',
+      'ratio',
+      'expected_priority_between'
     ])
 
     if (testCase.outbound_payloads) {
@@ -325,21 +331,64 @@ async function runTestCase(testCase, parentTest) {
   })
 
   await parentTest.test('trace context: ' + testCase.test_name, (t, end) => {
-    const agent = helper.instrumentMockedAgent({})
+    // Sampling config has to be given on agent initialization
+    const initConfig = {
+      distributed_tracing: {
+        enabled: true,
+        sampler: {
+          root: {},
+          remote_parent_sampled: {},
+          remote_parent_not_sampled: {}
+        }
+      }
+    }
+    initConfig.distributed_tracing.sampler.remote_parent_sampled = testCase.remote_parent_sampled ?? 'default'
+    initConfig.distributed_tracing.sampler.remote_parent_not_sampled = testCase.remote_parent_not_sampled ?? 'default'
+    initConfig.distributed_tracing.sampler.root = testCase.root ?? 'default'
+    if (testCase.ratio) {
+      // The ratio to use for all of the trace ID ratio samplers defined in the test.
+      // For testing purposes we are not defining different ratios for each trace ID ratio sampler instance.
+      if (initConfig.distributed_tracing.sampler.root === 'trace_id_ratio_based') {
+        initConfig.distributed_tracing.sampler.root = {
+          trace_id_ratio_based: {
+            ratio: testCase.ratio
+          }
+        }
+      }
+      if (initConfig.distributed_tracing.sampler.remote_parent_sampled === 'trace_id_ratio_based') {
+        initConfig.distributed_tracing.sampler.remote_parent_sampled = {
+          trace_id_ratio_based: {
+            ratio: testCase.ratio
+          }
+        }
+      }
+      if (initConfig.distributed_tracing.sampler.remote_parent_not_sampled === 'trace_id_ratio_based') {
+        initConfig.distributed_tracing.sampler.remote_parent_not_sampled = {
+          trace_id_ratio_based: {
+            ratio: testCase.ratio
+          }
+        }
+      }
+    }
+    const agent = helper.instrumentMockedAgent(initConfig)
     agent.recordSupportability = recordSupportability
     agent.config.trusted_account_key = testCase.trusted_account_key
     agent.config.account_id = testCase.account_id
     agent.config.primary_application_id = 4657
     agent.config.span_events.enabled = testCase.span_events_enabled
-    agent.config.transaction_events.enabled = testCase.transaction_events_enabled
-    agent.config.distributed_tracing.enabled = true
+    // Some new tests should have this set to true, but they are undefined instead
+    if (testCase.transaction_events_enabled !== undefined) {
+      agent.config.transaction_events.enabled = testCase.transaction_events_enabled
+    } else agent.config.transaction_events.enabled = true
+
     t.after(() => helper.unloadAgent(agent))
 
     const agentApi = new API(agent)
 
     const transactionType = testCase.web_transaction ? TYPES.WEB : TYPES.BG
 
     helper.runInTransaction(agent, transactionType, function (transaction) {
+      console.debug(testCase.test_name)
       transaction.baseSegment = transaction.trace.add('MyBaseSegment', (segment) => {
         recorder(
           transaction,
@@ -354,13 +403,7 @@ async function runTestCase(testCase, parentTest) {
         agentApi.noticeError(new Error('should error'))
       }
 
-      // monkey patch this transaction object
-      // to force sampled to be true.
-      if (testCase.force_sampled_true) {
-        transaction.agent.transactionSampler.shouldSample = function stubShouldSample() {
-          return true
-        }
-      }
+      forceAdaptiveSamplers(agent, testCase.force_adaptive_sampled_true)
 
       for (const inboundHeader of testCase.inbound_headers.values()) {
         transaction.acceptDistributedTraceHeaders(testCase.transport_type, inboundHeader)
@@ -406,6 +449,16 @@ async function runTestCase(testCase, parentTest) {
         transaction.trace.root.touch()
         transaction.end()
 
+        // check `expected_priority_between`
+        if (testCase.expected_priority_between) {
+          const [minPriority, maxPriority] = testCase.expected_priority_between
+          const actualPriority = transaction.priority
+          assert.ok(
+            actualPriority >= minPriority && actualPriority <= maxPriority,
+            `Expected transaction.priority (${actualPriority}) to be between ${minPriority} and ${maxPriority}`
+          )
+        }
+
         // These tests assume setting a transport type even when there are not valid
         // trace headers. This is slightly inconsistent with the spec. Given DT
         // (NR format) does not include transport when there is no trace AND the
@@ -446,3 +499,19 @@ async function runTestCase(testCase, parentTest) {
     end()
   })
 }
+
+function forceAdaptiveSamplers(agent, forceAdaptiveSampled) {
+  if (forceAdaptiveSampled === undefined || forceAdaptiveSampled === null) return
+  // "Whether to force a transaction to be sampled or not when the adaptive sampler is used"
+  // implies this affects all samplers that are adaptive samplers
+  const samplers = [agent.transactionSampler, agent.remoteParentSampledSampler, agent.remoteParentNotSampledSampler]
+  for (const sampler of samplers) {
+    if (sampler?.toString() === 'AdaptiveSampler') {
+      // monkey patch this transaction object
+      // to force sampled to the given value.
+      sampler.shouldSample = function stubShouldSample() {
+        return forceAdaptiveSampled
+      }
+    }
+  }
+}

test/lib/cross_agent_tests/distributed_tracing/README.md

@@ -14,7 +14,12 @@ the agent under test. Here's what the various fields in each test case mean:
 | `account_id` | The account id the agent would receive on connect. |
 | `web_transaction` | Whether the transaction that's tested is a web transaction or not. |
 | `raises_exception` | Whether to simulate an exception happening within the transaction or not, resulting in a transaction error event. |
-| `force_sampled_true` | Whether to force a transaction to be sampled or not. |
+| `root` | The sampler to use for transactions at the root of a trace. |
+| `remote_parent_sampled` | The sampler to use for transactions with a remote parent. |
+| `remote_parent_not_sampled` | The sampler to use for transactions with a remote parent that is not sampled. |
+| `expected_priority_between` | The inclusive range of the expected priority value on the generated transaction event. |
+| `force_adaptive_sampled_true` | Whether to force a transaction to be sampled or not when the adaptive sampler is used. |
+| `ratio` | The ratio to use for all of the trace ID ratio samplers defined in the test. For testing purposes we are not defining different ratios for each trace ID ratio sampler instance. If that is necessary, we will need a different way to configure the ratios. |
 | `transport_type` | The transport type for the inbound request. |
 | `inbound_headers` | The headers you should mock coming into the agent. |
 | `outbound_payloads` | The exact/expected/unexpected values for outbound `w3c` headers. |
@@ -23,6 +28,14 @@ the agent under test. Here's what the various fields in each test case mean:
 | `span_events_enabled` | Whether span events are enabled in the agent or not. |
 | `transaction_events_enabled` | Whether transaction events are enabled in the agent or not. |
 
+The samplers that can referenced in the `root`, `remote_parent_sampled`, and `remote_parent_not_sampled` fields are:
+
+- `default`: Use the adaptive sampler.
+- `adaptive`: Use the adaptive sampler.
+- `trace_id_ratio_based`: Use the trace ID ratio sampler.
+- `always_on`: Use the always on sampler.
+- `always_off`: Use the always off sampler.
+
 The `outbound_payloads` and `intrinsics` field can have nested values, for example:
 ```javascript
 ...

test/lib/cross_agent_tests/distributed_tracing/distributed_tracing.json

@@ -796,78 +796,6 @@
             ["Supportability/DistributedTrace/CreatePayload/Success", 2]
         ]
     },
-    {
-        "test_name": "payload_from_trusted_partnership_account",
-        "trusted_account_key": "44",
-        "account_id": "11",
-        "web_transaction": true,
-        "raises_exception": false,
-        "force_sampled_true": false,
-        "span_events_enabled": true,
-        "major_version": 0,
-        "minor_version": 1,
-        "transport_type": "HTTP",
-        "inbound_payloads": [
-            {
-                "v": [0, 1],
-                "d": {
-                    "ac": "33",
-                    "ap": "2827902",
-                    "tx": "e8b91a159289ff74",
-                    "pr": 0.123456,
-                    "sa": false,
-                    "ti": 1518469636035,
-                    "tr": "d6b4ba0c3a712ca",
-                    "tk": "44",
-                    "ty": "App"
-                }
-            }
-        ],
-        "outbound_payloads": [
-            {
-                "exact": {
-                    "v": [0, 1],
-                    "d.ac": "11",
-                    "d.pr": 0.123456,
-                    "d.sa": false,
-                    "d.tr": "d6b4ba0c3a712ca",
-                    "d.tk": "44",
-                    "d.ty": "App"
-                },
-                "expected": ["d.ap", "d.tx", "d.ti"],
-                "unexpected": ["d.id"]
-            }
-        ],
-        "intrinsics": {
-            "target_events": ["Transaction"],
-            "common":{
-                "exact": {
-                    "parent.type": "App",
-                    "parent.app": "2827902",
-                    "parent.account": "33",
-                    "parent.transportType": "HTTP",
-                    "traceId": "d6b4ba0c3a712ca",
-                    "priority": 0.123456,
-                    "sampled": false
-                },
-                "expected": ["parent.transportDuration", "guid"],
-                "unexpected": ["grandparentId", "cross_process_id", "nr.tripId", "nr.pathHash", "nr.referringPathHash", "nr.guid", "nr.referringTransactionGuid", "nr.alternatePathHashes"]
-            },
-            "Transaction": {
-                "exact": {
-                    "parentId": "e8b91a159289ff74"
-                }
-            }
-        },
-        "expected_metrics": [
-            ["DurationByCaller/App/33/2827902/HTTP/all", 1],
-            ["DurationByCaller/App/33/2827902/HTTP/allWeb", 1],
-            ["TransportDuration/App/33/2827902/HTTP/all", 1],
-            ["TransportDuration/App/33/2827902/HTTP/allWeb", 1],
-            ["Supportability/DistributedTrace/AcceptPayload/Success", 1],
-            ["Supportability/DistributedTrace/CreatePayload/Success", 1]
-        ]
-    },
     {
         "test_name": "payload_has_larger_minor_version",
         "trusted_account_key": "33",