[#171] Fix incoming request bug by No sampled request

* internal request call * self request header pinpoint * pinpoint-flags header is bypass flag * Fix HTTP sampled header * npm audit fix
pinpoint-apm · Mar 4, 2024 · 5821771 · 5821771
1 parent 688061b
commit 5821771
Show file tree

Hide file tree

Showing 18 changed files with 267 additions and 100 deletions.
diff --git a/lib/agent.js b/lib/agent.js
@@ -17,7 +17,6 @@ const getConfig = require('./config').getConfig
 const PinpointClient = require('./client/pinpoint-client')
 const dataSenderFactory = require('./client/data-sender-factory')
 const AgentInfo = require('./data/dto/agent-info')
-const wrapped = Symbol('pinpoint-wrapped-function')
 const PinScheduler = require('./metric/ping-scheduler')
 
 class Agent {

diff --git a/lib/context/async-trace.js b/lib/context/async-trace.js
@@ -13,8 +13,8 @@ const BufferedStorage = require('./buffered-storage')
 const Trace = require('./trace')
 
 class AsyncTrace extends Trace {
-  constructor(span, asyncId, traceId, agentInfo, dataSender, sampling) {
-    super(traceId, agentInfo, dataSender, sampling)
+  constructor(span, asyncId, traceId, agentInfo, dataSender) {
+    super(traceId, agentInfo, dataSender)
     this.span = span
     this.asyncId = asyncId
     const createAsyncSpanChunk = SpanChunk.getAsyncFactoryMethod(traceId, agentInfo, asyncId)
@@ -42,7 +42,7 @@ class AsyncTrace extends Trace {
   }
 
   canSampled() {
-    return this.sampling
+    return true
   }
 
   close() {

diff --git a/lib/context/trace-context.js b/lib/context/trace-context.js
@@ -10,7 +10,6 @@ const Trace = require('./trace')
 const TransactionId = require('./transaction-id')
 const TraceId = require('./trace-id')
 const IdGenerator = require('./id-generator')
-const activeTrace = require('../metric/active-trace')
 const log = require('../utils/logger')
 const sampler = require('../sampler/sampler')
 const DisableTrace = require('./disable-trace')
@@ -58,7 +57,7 @@ class TraceContext {
       requestData.parentSpanId,
       requestData.flag)
 
-    return this.createTraceObject(traceId, requestData.sampled, requestData)
+    return this.createTraceObject(traceId, requestData)
   }
 
   newTraceObject(sampling) {
@@ -68,18 +67,18 @@ class TraceContext {
     const transactionId = new TransactionId(this.agentInfo.agentId, this.agentInfo.agentStartTime.toString())
     const spanId = IdGenerator.stringValueOfNext()
     const traceId = new TraceId(transactionId, spanId)
-    return this.createTraceObject(traceId, sampling)
+    return this.createTraceObject(traceId)
   }
 
   // https://github.com/pinpoint-apm/pinpoint/blob/a113e527e73add4e848de9173923b01e06b3cca1/bootstrap-core/src/main/java/com/navercorp/pinpoint/bootstrap/plugin/request/ServletRequestListener.java#L117
-  createTraceObject(traceId, sampling, requestData) {
-    if (!traceId || !this.agentInfo || !sampling) {
+  createTraceObject(traceId, requestData) {
+    if (!traceId || !this.agentInfo) {
       return new DisableTrace()
     }
 
     try {
-      const trace = new Trace(traceId, this.agentInfo, this.dataSender, sampling, requestData)
-      activeTrace.register(trace)
+      const trace = new Trace(traceId, this.agentInfo, this.dataSender, requestData)
+      // activeTrace.register(trace)
       return trace
     } catch (e) {
       log.error('Fail to create trace object', e)
@@ -95,7 +94,7 @@ class TraceContext {
         trace.spanRecorder.span.markElapsedTime()
       }
       trace.close()
-      activeTrace.remove(trace)
+      // activeTrace.remove(trace)
     } catch (e) {
       log.error('Fail to complete trace object', e)
     }
@@ -106,6 +105,10 @@ class TraceContext {
   }
 
   makeTrace(requestData) {
+    if (requestData && typeof requestData.sampled === 'boolean' && requestData.sampled === false) {
+      return new DisableTrace()
+    }
+
     const transactionId = requestData && requestData.transactionId
     if (transactionId) {
       if (this.enableSampling) {

diff --git a/lib/context/trace-id.js b/lib/context/trace-id.js
@@ -12,7 +12,7 @@ class TraceId {
     this.transactionId = transactionId
     this.spanId = spanId
     this.parentSpanId = parentSpanId || "-1"
-    this.flag = flag || 0
+    this.flag = flag
   }
 
   transactionSequence() {

diff --git a/lib/context/trace.js b/lib/context/trace.js
@@ -16,7 +16,7 @@ const log = require('../utils/logger')
 const calledTraceBlockEnd = Symbol('called-traceBlockEnd')
 
 class Trace {
-  constructor(traceId, agentInfo, dataSender, sampling, requestData) {
+  constructor(traceId, agentInfo, dataSender, requestData) {
     this.traceId = traceId
     this.agentInfo = agentInfo
     this.dataSender = dataSender
@@ -26,13 +26,11 @@ class Trace {
 
     this.spanEventRecorder = null
 
-
     const createSpanChunk = SpanChunk.getFactoryMethod(traceId, agentInfo)
     this.storage = new BufferedStorage(dataSender, createSpanChunk)
 
     this.callStack = []
     this.sequence = 0
-    this.sampling = sampling
   }
 
   traceBlockBegin() {
@@ -86,7 +84,7 @@ class Trace {
   }
 
   canSampled() {
-    return this.sampling
+    return true
   }
 
   getStartTime() {

diff --git a/lib/instrumentation/http-shared.js b/lib/instrumentation/http-shared.js
@@ -30,7 +30,7 @@ exports.clearPathMatcher = function () {
   pathMatcher = null
 }
 
-exports.instrumentRequest = function (agent, moduleName) {
+exports.instrumentRequest = function (agent) {
   return function (original) {
     return function (event, req, res) {
       if (event !== 'request') {
@@ -58,7 +58,9 @@ exports.instrumentRequest = function (agent, moduleName) {
           return result
         }
 
-        recordRequest(trace.spanRecorder, requestData)
+        trace.spanRecorder.recordRpc(requestData.rpcName)
+        trace.spanRecorder.recordEndPoint(requestData.endPoint)
+        trace.spanRecorder.recordRemoteAddr(requestData.remoteAddress)
         trace.spanRecorder.recordApi(defaultPredefinedMethodDescriptorRegistry.nodeServerMethodDescriptor)
         if (requestData && typeof requestData.searchParams === 'string') {
           trace.spanRecorder.recordAttribute(annotationKey.HTTP_PARAM, requestData.searchParams)
@@ -166,10 +168,4 @@ exports.traceOutgoingRequest = function (agent, moduleName) {
       }
     }
   }
-}
-
-const recordRequest = (recorder, requestData) => {
-  recorder.recordRpc(requestData.rpcName)
-  recorder.recordEndPoint(requestData.endPoint)
-  recorder.recordRemoteAddr(requestData.remoteAddress)
 }
diff --git a/lib/instrumentation/request-data.js b/lib/instrumentation/request-data.js
@@ -13,11 +13,9 @@ class RequestData {
     this.parentSpanId = null
     this.parentApplicationName = null
     this.parentApplicationType = null
-    this.flags = null
     this.host = null
     this.sampled = null
     this.isRoot = true
-
     this.rpcName = null
     this.endPoint = null
     this.remoteAddress = null

diff --git a/lib/instrumentation/request-header-utils.js b/lib/instrumentation/request-header-utils.js
@@ -21,7 +21,6 @@ class RequestHeaderUtils {
     }
 
     let requestData = new RequestData()
-
     // https://nodejs.org/api/url.html#url_url_parse_urlstring_parsequerystring_slashesdenotehost
     // url.parse throw error
     try {
@@ -39,33 +38,44 @@ class RequestHeaderUtils {
     if (remoteAddress) {
       requestData.remoteAddress = remoteAddress.replace('::ffff:', '')
     }
-    if (this.getHeader(request, PinpointHeader.HTTP_TRACE_ID)) {
-      requestData = this.readPinpointHeader(request, requestData)
-    }
+    this.setPinpointHeader(request, requestData)
     log.debug('>> Read DATA from http header \n', requestData)
     return requestData
   }
 
-  static readPinpointHeader(request, requestData) {
-    requestData.transactionId = TransactionId.toTransactionId(this.getHeader(request, PinpointHeader.HTTP_TRACE_ID))
-    if (requestData.transactionId) {
-      const spanId = this.getHeader(request, PinpointHeader.HTTP_SPAN_ID)
-      if (spanId) {
-        requestData.spanId = spanId
-      }
+  static setPinpointHeader(request, requestData) {
+    const httpSampledHeader = this.getHeader(request, PinpointHeader.HTTP_SAMPLED)
+    if (httpSampledHeader) {
+      requestData.sampled = samplingFlag.isSamplingFlag(httpSampledHeader)
+      return
+    }
 
-      const parentSpanId = this.getHeader(request, PinpointHeader.HTTP_PARENT_SPAN_ID)
-      if (parentSpanId) {
-        requestData.parentSpanId = parentSpanId
-      }
+    const transactionId = TransactionId.toTransactionId(this.getHeader(request, PinpointHeader.HTTP_TRACE_ID))
+    if (!transactionId) {
+      return
+    }
+
+    requestData.transactionId = transactionId
+    const spanId = this.getHeader(request, PinpointHeader.HTTP_SPAN_ID)
+    if (spanId) {
+      requestData.spanId = spanId
+    }
 
-      requestData.parentApplicationName = this.getHeader(request, PinpointHeader.HTTP_PARENT_APPLICATION_NAME)
-      requestData.parentApplicationType = Number(this.getHeader(request, PinpointHeader.HTTP_PARENT_APPLICATION_TYPE))
-      requestData.flags = Number(this.getHeader(request, PinpointHeader.HTTP_FLAGS))
-      requestData.host = this.getHeader(request, PinpointHeader.HTTP_HOST)
-      requestData.sampled = samplingFlag.isSamplingFlag(this.getHeader(request, PinpointHeader.HTTP_SAMPLED))
-      requestData.isRoot = false
+    const parentSpanId = this.getHeader(request, PinpointHeader.HTTP_PARENT_SPAN_ID)
+    if (parentSpanId) {
+      requestData.parentSpanId = parentSpanId
     }
+
+    requestData.parentApplicationName = this.getHeader(request, PinpointHeader.HTTP_PARENT_APPLICATION_NAME)
+    requestData.parentApplicationType = Number(this.getHeader(request, PinpointHeader.HTTP_PARENT_APPLICATION_TYPE))
+
+    const flags = this.getHeader(request, PinpointHeader.HTTP_FLAGS)
+    if (flags) {
+      requestData.flags = Number(flags)
+    }
+
+    requestData.host = this.getHeader(request, PinpointHeader.HTTP_HOST)
+    requestData.isRoot = false
     return requestData
   }
 
@@ -92,7 +102,9 @@ class RequestHeaderUtils {
       this.setHeader(request, PinpointHeader.HTTP_PARENT_SPAN_ID, trace.traceId.spanId)
       this.setHeader(request, PinpointHeader.HTTP_PARENT_APPLICATION_NAME, agent.config.applicationName)
       this.setHeader(request, PinpointHeader.HTTP_PARENT_APPLICATION_TYPE, agent.config.serviceType)
-      this.setHeader(request, PinpointHeader.HTTP_FLAGS, trace.traceId.flag)
+      if (trace.traceId.flag) {
+        this.setHeader(request, PinpointHeader.HTTP_FLAGS, trace.traceId.flag)
+      }
       this.setHeader(request, PinpointHeader.HTTP_HOST, host)
     }
     if (log && log.isDebug() && request && typeof request.getHeaders === 'function') {

diff --git a/lib/metric/active-trace.js b/lib/metric/active-trace.js
@@ -9,7 +9,6 @@
 const SimpleCache = require('../utils/simple-cache')
 const HistogramSchema = require('./histogram-schema')
 const ActiveTraceHistogram = require('./active-trace-histogram')
-const log = require('../utils/logger')
 
 const KEY_PREFIX = '_AT_'
 

diff --git a/lib/metric/agent-stats-monitor.js b/lib/metric/agent-stats-monitor.js
@@ -7,7 +7,6 @@
 'use strict'
 
 const ResourceStatsCollector = require('./resource-stats-collector')
-const activeTrace = require('./active-trace')
 const log = require('../utils/logger')
 
 class AgentStatsMonitor {
@@ -48,7 +47,7 @@ class AgentStatsMonitor {
       collectInterval: 1000,
       memory: this.resourceStatCollector.getMemoryStats(),
       cpu: cpuStatus,
-      activeTrace: activeTrace.getCurrentActiveTraceHistogram(),
+      // activeTrace: activeTrace.getCurrentActiveTraceHistogram(),
     }
   }
 }

diff --git a/lib/sampler/sampler.js b/lib/sampler/sampler.js
@@ -27,6 +27,9 @@ const getIsSampling = (sampling, sampleRate) => () => {
   return false
 }
 
+const getSamplingCountGenerator = () => samplingCountGenerator
+
 module.exports = {
  getIsSampling
+ , getSamplingCountGenerator
 }
diff --git a/lib/sampler/sampling-flag.js b/lib/sampler/sampling-flag.js
@@ -8,8 +8,8 @@
 
 //https://github.com/naver/pinpoint/blob/ab07664e2ed944e90aa9c44f7e39597f39264c2b/bootstrap-core/src/main/java/com/navercorp/pinpoint/bootstrap/plugin/request/DefaultTraceHeaderReader.java#L78
 
-const SAMPLING_RATE_PREFIX = "s"
-const SAMPLING_RATE_FALSE = SAMPLING_RATE_PREFIX +  "0"
+const SAMPLING_RATE_PREFIX = 's'
+const SAMPLING_RATE_FALSE = SAMPLING_RATE_PREFIX +  '0'
 
 class SamplingFlag {
     isSamplingFlag(samplingFlag) {

diff --git a/package-lock.json b/package-lock.json