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

Refactor pino instrumentation #2424

Closed
jsumners-nr opened this issue Aug 1, 2024 · 1 comment · Fixed by #2464
Closed

Refactor pino instrumentation #2424

jsumners-nr opened this issue Aug 1, 2024 · 1 comment · Fixed by #2464
Assignees
Labels
points: 3 A few days

Comments

@jsumners-nr
Copy link
Contributor

See the conversation in #2421. Basically, the following needs to be refactored in order to satisfy the SonarQube linter:

// eslint-disable-next-line sonarjs/cognitive-complexity
module.exports = function instrument(shim, tools) {
const pinoVersion = shim.pkgVersion
if (semver.lt(pinoVersion, '7.0.0')) {
shim.logger.debug('Instrumentation only supported on pino >=7.0.0.')
return
}
const agent = shim.agent
const config = agent.config
if (!isApplicationLoggingEnabled(config)) {
shim.logger.debug('Application logging not enabled. Not instrumenting pino.')
return
}
const metrics = agent.metrics
createModuleUsageMetric('pino', metrics)
const symbols = shim.require('./lib/symbols')
shim.wrap(tools, 'asJson', function wrapJson(shim, asJson) {
/**
* Wraps function in pino that is used to construct/serialize a log
* line to json
*
* @param {object} obj data from mixins
* @param {string} msg message of log line
* @param {number} num log level as number
* @param {string} time formatted snippet of json with time(`,"time":<unix time>`)
* @returns {string} serialized log line
*/
return function wrappedAsJson() {
const args = shim.argsToArray.apply(shim, arguments)
const level = this?.levels?.labels?.[args[2]]
// Pino log methods accept a singular object (a merging object) that can
// have a `msg` property for the log message. In such cases, we need to
// update that log property instead of the second parameter.
const useMergeObj = args[1] === undefined && Object.hasOwn(args[0], 'msg')
if (isMetricsEnabled(config)) {
incrementLoggingLinesMetrics(level, metrics)
}
if (isLocalDecoratingEnabled(config)) {
if (useMergeObj === true) {
args[0].msg += agent.getNRLinkingMetadata()
} else {
args[1] += agent.getNRLinkingMetadata()
}
}
/**
* must call original asJson to allow pino
* to construct the entire log line before we
* add to the log aggregator
*/
const logLine = asJson.apply(this, args)
if (isLogForwardingEnabled(config, agent)) {
const chindings = this[symbols.chindingsSym]
const formatLogLine = reformatLogLine({
msg: useMergeObj === true ? args[0].msg : args[1],
logLine,
agent,
chindings,
level,
logger: shim.logger
})
agent.logs.add(formatLogLine)
}
return logLine
}
})
}

Prior to opening the PR for review, I attempted:

function instrument(shim, tools) {
	// snip
	shim.wrap(tools, 'asJson', function wrapJson(shim, asJson) {
		return wrappedAsJson.bind({ shim, agent, symbols, asJson, metrics, config})
	}
}

function wrappedAsJson() {
	const { shim, agent, symbols, asJson, metrics, config } = this
	// implementation
}

But that won't work because wrappedAsJson needs this to be bound to the instrumented instance of Pino.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
points: 3 A few days
Projects
Archived in project
2 participants