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

feat: Removed transaction from segment. Introduced a new enterSegment and enterTransaction to make context propagation more clear #2646

Merged
merged 3 commits into from
Oct 31, 2024
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
45 changes: 31 additions & 14 deletions api.js
Original file line number Diff line number Diff line change
Expand Up @@ -961,19 +961,21 @@

const shim = this.shim
const tracer = this.agent.tracer
const parent = tracer.getTransaction()
const parentTx = tracer.getTransaction()

assignCLMSymbol(shim, handle)
return tracer.transactionNestProxy('web', function startWebSegment() {
const tx = tracer.getTransaction()
const context = tracer.getContext()
const tx = context?.transaction
const parent = context?.segment

if (!tx) {
return handle.apply(this, arguments)
}

if (tx === parent) {
if (tx === parentTx) {
logger.debug('not creating nested transaction %s using transaction %s', url, tx.id)
return tracer.addSegment(url, null, null, true, handle)
return tracer.addSegment(url, null, parent, true, handle)
}

logger.debug(
Expand All @@ -985,10 +987,16 @@
tx.nameState.setName(NAMES.CUSTOM, null, NAMES.ACTION_DELIMITER, url)
tx.url = url
tx.applyUserNamingRules(tx.url)
tx.baseSegment = tracer.createSegment(url, recordWeb)
tx.baseSegment = tracer.createSegment({
name: url,
recorder: recordWeb,
transaction: tx,
parent
})
const newContext = context.enterSegment({ transaction: tx, segment: tx.baseSegment })
tx.baseSegment.start()

const boundHandle = tracer.bindFunction(handle, tx.baseSegment)
const boundHandle = tracer.bindFunction(handle, newContext)
maybeAddCLMAttributes(handle, tx.baseSegment)
let returnResult = boundHandle.call(this)
if (returnResult && shim.isPromise(returnResult)) {
Expand Down Expand Up @@ -1061,19 +1069,21 @@
const tracer = this.agent.tracer
const shim = this.shim
const txName = group + '/' + name
const parent = tracer.getTransaction()
const parentTx = tracer.getTransaction()

assignCLMSymbol(shim, handle)
return tracer.transactionNestProxy('bg', function startBackgroundSegment() {
const tx = tracer.getTransaction()
const context = tracer.getContext()
const tx = context?.transaction
const parent = context?.segment

if (!tx) {
return handle.apply(this, arguments)
}

if (tx === parent) {
if (tx === parentTx) {
logger.debug('not creating nested transaction %s using transaction %s', txName, tx.id)
return tracer.addSegment(txName, null, null, true, handle)
return tracer.addSegment(txName, null, parent, true, handle)
}

logger.debug(
Expand All @@ -1085,11 +1095,17 @@
)

tx._partialName = txName
tx.baseSegment = tracer.createSegment(name, recordBackground)
tx.baseSegment = tracer.createSegment({
name,
recorder: recordBackground,
transaction: tx,
parent
})
const newContext = context.enterSegment({ transaction: tx, segment: tx.baseSegment })
tx.baseSegment.partialName = group
tx.baseSegment.start()

const boundHandle = tracer.bindFunction(handle, tx.baseSegment)
const boundHandle = tracer.bindFunction(handle, newContext)
maybeAddCLMAttributes(handle, tx.baseSegment)
let returnResult = boundHandle.call(this)
if (returnResult && shim.isPromise(returnResult)) {
Expand Down Expand Up @@ -1524,12 +1540,13 @@
const metadata = {}

const segment = this.agent.tracer.getSegment()
if (!segment) {
const transaction = this.agent.tracer.getTransaction()
if (!(segment || transaction)) {
logger.debug('No transaction found when calling API#getTraceMetadata')
} else if (!this.agent.config.distributed_tracing.enabled) {
logger.debug('Distributed tracing disabled when calling API#getTraceMetadata')
} else {
metadata.traceId = segment.transaction.traceId
metadata.traceId = transaction.traceId

const spanId = segment.getSpanId()
if (spanId) {
Expand All @@ -1547,7 +1564,7 @@
* @param {string} params.traceId Identifier for the feedback event.
* Obtained from {@link getTraceMetadata}.
* @param {string} params.category A tag for the event.
* @param {string} params.rating A indicator of how useful the message was.

Check warning on line 1567 in api.js

View workflow job for this annotation

GitHub Actions / lint (lts/*)

The type 'getTraceMetadata' is undefined

Check warning on line 1567 in api.js

View workflow job for this annotation

GitHub Actions / lint (lts/*)

The type 'getTraceMetadata' is undefined
* @param {string} [params.message] The message that triggered the event.
* @param {object} [params.metadata] Additional key-value pairs to associate
* with the recorded event.
Expand Down Expand Up @@ -1901,7 +1918,7 @@
transaction.ignoreApdex = true
}

/**

Check warning on line 1921 in api.js

View workflow job for this annotation

GitHub Actions / lint (lts/*)

Missing JSDoc @returns declaration

Check warning on line 1921 in api.js

View workflow job for this annotation

GitHub Actions / lint (lts/*)

Missing JSDoc @returns declaration
* Run a function with the passed in LLM context as the active context and return its return value.
*
* @example
Expand Down
45 changes: 29 additions & 16 deletions lib/agent.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@
const uninstrumented = require('./uninstrumented')
const util = require('util')
const createSpanEventAggregator = require('./spans/create-span-event-aggregator')
const createContextManager = require('./context-manager/create-context-manager')
const {
maybeAddDatabaseAttributes,
maybeAddExternalAttributes,
Expand Down Expand Up @@ -207,7 +206,7 @@
periodMs: config.event_harvest_config.report_period_ms,
limit: config.event_harvest_config.harvest_limits.analytic_event_data,
config,
enabled: (config) => config.transaction_events.enabled

Check warning on line 209 in lib/agent.js

View workflow job for this annotation

GitHub Actions / lint (lts/*)

'config' is already declared in the upper scope on line 157 column 16

Check warning on line 209 in lib/agent.js

View workflow job for this annotation

GitHub Actions / lint (lts/*)

'config' is already declared in the upper scope on line 157 column 16
},
this
)
Expand All @@ -218,7 +217,7 @@
limit: config.event_harvest_config.harvest_limits.custom_event_data,
metricNames: NAMES.CUSTOM_EVENTS,
config,
enabled: (config) => config.custom_insights_events.enabled

Check warning on line 220 in lib/agent.js

View workflow job for this annotation

GitHub Actions / lint (lts/*)

'config' is already declared in the upper scope on line 157 column 16

Check warning on line 220 in lib/agent.js

View workflow job for this annotation

GitHub Actions / lint (lts/*)

'config' is already declared in the upper scope on line 157 column 16
},
this
)
Expand All @@ -228,7 +227,7 @@
periodMs: DEFAULT_HARVEST_INTERVAL_MS,
limit: MAX_ERROR_TRACES_DEFAULT,
config,
enabled: (config) => config.error_collector.enabled && config.collect_errors

Check warning on line 230 in lib/agent.js

View workflow job for this annotation

GitHub Actions / lint (lts/*)

'config' is already declared in the upper scope on line 157 column 16

Check warning on line 230 in lib/agent.js

View workflow job for this annotation

GitHub Actions / lint (lts/*)

'config' is already declared in the upper scope on line 157 column 16
},
this.collector,
this.harvester
Expand All @@ -239,7 +238,7 @@
periodMs: config.event_harvest_config.report_period_ms,
limit: config.event_harvest_config.harvest_limits.error_event_data,
config,
enabled: (config) => config.error_collector.enabled && config.error_collector.capture_events

Check warning on line 241 in lib/agent.js

View workflow job for this annotation

GitHub Actions / lint (lts/*)

'config' is already declared in the upper scope on line 157 column 16

Check warning on line 241 in lib/agent.js

View workflow job for this annotation

GitHub Actions / lint (lts/*)

'config' is already declared in the upper scope on line 157 column 16
},
this
)
Expand All @@ -257,9 +256,8 @@

this.errors = new ErrorCollector(config, errorTraceAggregator, errorEventAggregator, this.metrics)

this._contextManager = createContextManager(this.config)
// Transaction tracing.
this.tracer = new Tracer(this, this._contextManager)
this.tracer = new Tracer(this)
this.traces = new TransactionTraceAggregator(
{
periodMs: DEFAULT_HARVEST_INTERVAL_MS,
Expand Down Expand Up @@ -293,13 +291,7 @@
// Set up all the configuration events the agent needs to listen for.
this._listenForConfigChanges()

// Entity tracking metrics.
this.totalActiveSegments = 0
this.segmentsCreatedInHarvest = 0
this.segmentsClearedInHarvest = 0
// Used by shutdown code as well as entity tracking stats
this.activeTransactions = 0

this.initCounters()
this.llm = {}

// Finally, add listeners for the agent's own events.
Expand Down Expand Up @@ -598,11 +590,33 @@
)
}

// Reset the counters.
this.resetCounters()
}

Agent.prototype.initCounters = function initCounters() {
// Entity tracking metrics.
this.totalActiveSegments = 0
this.segmentsCreatedInHarvest = 0
this.segmentsClearedInHarvest = 0
// Used by shutdown code as well as entity tracking stats
this.activeTransactions = 0
}

Agent.prototype.incrementCounters = function incrementCounters() {
++this.totalActiveSegments
++this.segmentsCreatedInHarvest
}

Agent.prototype.decrementCounters = function decrementCounters(transaction) {
--this.activeTransactions
this.totalActiveSegments -= transaction.numSegments
this.segmentsClearedInHarvest += transaction.numSegments
}

Agent.prototype.resetCounters = function resetCounters() {
this.segmentsCreatedInHarvest = 0
this.segmentsClearedInHarvest = 0
}
/**
* Public interface for passing configuration data from the collector
* on to the configuration, in an effort to keep them at least somewhat
Expand Down Expand Up @@ -753,9 +767,7 @@
logger.debug('Ignoring %s (%s).', transaction.name, transaction.id)
}

--this.activeTransactions
this.totalActiveSegments -= transaction.numSegments
this.segmentsClearedInHarvest += transaction.numSegments
this.decrementCounters(transaction)
}

Agent.prototype.setLambdaArn = function setLambdaArn(arn) {
Expand Down Expand Up @@ -834,6 +846,7 @@
*/
Agent.prototype.getLinkingMetadata = function getLinkingMetadata() {
const segment = this.tracer.getSegment()
const transaction = this.tracer.getTransaction()
const config = this.config

const linkingMetadata = {
Expand All @@ -842,8 +855,8 @@
'hostname': config.getHostnameSafe()
}

if (config.distributed_tracing.enabled && segment) {
linkingMetadata['trace.id'] = segment.transaction.traceId
if (config.distributed_tracing.enabled && segment && transaction) {
linkingMetadata['trace.id'] = transaction.traceId
const spanId = segment.getSpanId()
if (spanId) {
linkingMetadata['span.id'] = spanId
Expand Down
4 changes: 2 additions & 2 deletions lib/attributes.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ class Attributes {
* @param {string} scope
* The scope of the attributes this will collect. Must be `transaction` or
* `segment`.
* @param {number} [limit=Infinity]
* @param {number} [limit]
* The maximum number of attributes to retrieve for each destination.
*/
constructor(scope, limit = Infinity) {
Expand Down Expand Up @@ -104,7 +104,7 @@ class Attributes {
* @param {DESTINATIONS} destinations - The default destinations for this key.
* @param {string} key - The attribute name.
* @param {string} value - The attribute value.
* @param {boolean} [truncateExempt=false] - Flag marking value exempt from truncation
* @param {boolean} [truncateExempt] - Flag marking value exempt from truncation
*/
addAttribute(destinations, key, value, truncateExempt = false) {
if (this.attributeCount + 1 > this.limit) {
Expand Down
2 changes: 1 addition & 1 deletion lib/collector/remote-method.js
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ RemoteMethod.prototype._reportDataUsage = function reportDataUsage(sent, receive
* you're doing it wrong.
*
* @param {object} payload Serializable payload.
* @param {object} [nrHeaders=null] NR request headers from connect response.
* @param {object} [nrHeaders] NR request headers from connect response.
* @param {Function} callback What to do next. Gets passed any error.
*/
RemoteMethod.prototype.invoke = function invoke(payload, nrHeaders, callback) {
Expand Down
10 changes: 3 additions & 7 deletions lib/context-manager/async-local-context-manager.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
'use strict'

const { AsyncLocalStorage } = require('async_hooks')
const Context = require('./context')

/**
* Class for managing state in the agent.
Expand All @@ -17,12 +18,7 @@ const { AsyncLocalStorage } = require('async_hooks')
* @class
*/
class AsyncLocalContextManager {
/**
* @param {object} config New Relic config instance
*/
constructor(config) {
this._config = config

constructor() {
this._asyncLocalStorage = new AsyncLocalStorage()
}

Expand All @@ -32,7 +28,7 @@ class AsyncLocalContextManager {
* @returns {object} The current active context.
*/
getContext() {
return this._asyncLocalStorage.getStore() || null
return this._asyncLocalStorage.getStore() || new Context()
}

/**
Expand Down
29 changes: 29 additions & 0 deletions lib/context-manager/context.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
/*
* Copyright 2024 New Relic Corporation. All rights reserved.
* SPDX-License-Identifier: Apache-2.0
*/

'use strict'

module.exports = class Context {
constructor(transaction, segment) {
this._transaction = transaction
this._segment = segment
}

get segment() {
return this._segment
}

get transaction() {
return this._transaction
}

enterSegment({ segment, transaction = this.transaction }) {
return new this.constructor(transaction, segment)
}

enterTransaction(transaction) {
return new this.constructor(transaction, transaction.trace.root)
}
}
29 changes: 0 additions & 29 deletions lib/context-manager/create-context-manager.js

This file was deleted.

11 changes: 8 additions & 3 deletions lib/db/parsed-statement.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,9 @@ function ParsedStatement(type, operation, collection, raw) {
}
}

ParsedStatement.prototype.recordMetrics = function recordMetrics(segment, scope) {
ParsedStatement.prototype.recordMetrics = function recordMetrics(segment, scope, transaction) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think we will eventually remove segment from the parameters and then retrieve it from the transaction in the function body?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't understand. Recorders are registered at the start of recording of a segment. In tracer they are added to transaction and binding the appropriate segment:

  if (recorder) {
    transaction.addRecorder(recorder.bind(null, segment))
  }

Then once a transaction end it iterates over all the recorders and calls the function with the scope and the transaction

Transaction.prototype.record = function record() {
  const name = this.name
  for (let i = 0, l = this._recorders.length; i < l; ++i) {
    this._recorders[i](name, this)
  }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't this work make transaction the entry point of the API? As in, wouldn't we attempt to retrieve a segment from the transaction?

const duration = segment.getDurationInMillis()
const exclusive = segment.getExclusiveDurationInMillis()
const transaction = segment.transaction
const type = transaction.isWeb() ? DB.WEB : DB.OTHER
const thisTypeSlash = this.type + '/'
const operation = DB.OPERATION + '/' + thisTypeSlash + this.operation
Expand Down Expand Up @@ -68,7 +67,13 @@ ParsedStatement.prototype.recordMetrics = function recordMetrics(segment, scope)
}

if (this.raw) {
transaction.agent.queries.add(segment, this.type.toLowerCase(), this.raw, this.trace)
transaction.agent.queries.add({
segment,
transaction,
type: this.type.toLowerCase(),
query: this.raw,
trace: this.trace
})
}
}

Expand Down
6 changes: 3 additions & 3 deletions lib/db/query-sample.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ QuerySample.prototype.merge = function merge(sample) {
}

QuerySample.prototype.prepareJSON = function prepareJSON(done) {
const transaction = this.trace.segment.transaction
const transaction = this.trace.transaction
const sample = this
const trace = sample.trace

Expand All @@ -56,7 +56,7 @@ QuerySample.prototype.prepareJSON = function prepareJSON(done) {
}

QuerySample.prototype.prepareJSONSync = function prepareJSONSync() {
const transaction = this.trace.segment.transaction
const transaction = this.trace.transaction
const sample = this
const trace = sample.trace

Expand Down Expand Up @@ -99,7 +99,7 @@ QuerySample.prototype.getParams = function getParams() {
}

if (this.tracer.config.distributed_tracing.enabled) {
this.trace.segment.transaction.addDistributedTraceIntrinsics(params)
this.trace.transaction.addDistributedTraceIntrinsics(params)
}

return params
Expand Down
Loading
Loading