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: Added New Relic Control health check #2841

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open
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
4 changes: 4 additions & 0 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@

'use strict'

const HealthReporter = require('./lib/health-reporter')

// Record opening times before loading any other files.
const preAgentTime = process.uptime()
const agentStart = Date.now()
Expand Down Expand Up @@ -154,6 +156,7 @@ function createAgent(config) {
'New Relic requires that you name this application!\n' +
'Set app_name in your newrelic.js or newrelic.cjs file or set environment variable\n' +
'NEW_RELIC_APP_NAME. Not starting!'
agent.healthReporter.setStatus(HealthReporter.STATUS_MISSING_APP_NAME)
throw new Error(message)
}

Expand All @@ -167,6 +170,7 @@ function createAgent(config) {

agent.start(function afterStart(error) {
if (error) {
agent.healthReporter.setStatus(HealthReporter.STATUS_INTERNAL_UNEXPECTED_ERROR)
const errorMessage = 'New Relic for Node.js halted startup due to an error:'
logger.error(error, errorMessage)

Expand Down
45 changes: 28 additions & 17 deletions lib/agent.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ const {
const synthetics = require('./synthetics')
const Harvester = require('./harvester')
const { createFeatureUsageMetrics } = require('./util/application-logging')
const HealthReporter = require('./health-reporter')

// Map of valid states to whether or not data collection is valid
const STATES = {
Expand Down Expand Up @@ -158,7 +159,10 @@ const DEFAULT_HARVEST_INTERVAL_MS = 60000
function Agent(config) {
EventEmitter.call(this)

this.healthReporter = new HealthReporter({ agentConfig: config })

if (!config) {
this.healthReporter.setStatus(HealthReporter.STATUS_CONFIG_PARSE_FAILURE)
throw new Error('Agent must be created with a configuration!')
}

Expand Down Expand Up @@ -342,11 +346,15 @@ Agent.prototype.start = function start(callback) {
'Has a license key been specified in the agent configuration ' +
'file or via the NEW_RELIC_LICENSE_KEY environment variable?'
)
this.healthReporter.setStatus(HealthReporter.STATUS_LICENSE_KEY_MISSING)

this.setState('errored')
sampler.stop()
const self = this
return process.nextTick(function onNextTick() {
callback(new Error('Not starting without license key!'))
self.healthReporter.stop(() => {
callback(new Error('Not starting without license key!'))
})
})
}
logger.info('Starting New Relic for Node.js connection process.')
Expand Down Expand Up @@ -476,23 +484,26 @@ Agent.prototype.stop = function stop(callback) {

sampler.stop()

if (this.collector.isConnected()) {
this.collector.shutdown(function onShutdown(error) {
if (error) {
agent.setState('errored')
logger.warn(error, 'Got error shutting down connection to New Relic:')
} else {
agent.setState('stopped')
logger.info('Stopped New Relic for Node.js.')
}

callback(error)
})
} else {
logger.trace('Collector was not connected, invoking callback.')
this.healthReporter.setStatus(HealthReporter.STATUS_AGENT_SHUTDOWN)
this.healthReporter.stop(() => {
if (agent.collector.isConnected()) {
agent.collector.shutdown(function onShutdown(error) {
if (error) {
agent.setState('errored')
logger.warn(error, 'Got error shutting down connection to New Relic:')
} else {
agent.setState('stopped')
logger.info('Stopped New Relic for Node.js.')
}

callback(error)
})
} else {
logger.trace('Collector was not connected, invoking callback.')

process.nextTick(callback)
}
process.nextTick(callback)
}
})
}

/**
Expand Down
42 changes: 42 additions & 0 deletions lib/config/default.js
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,48 @@ defaultConfig.definition = () => ({
formatter: boolean,
default: true
},

/**
* Collects configuration related to New Relic Agent Control, i.e. centralized
* agent management in container based environments.
*/
agent_control: {
/**
* Indicates that the agent is being managed by Agent Control. Must be set
* to true health monitoring.
*/
enabled: {
env: 'NEW_RELIC_AGENT_CONTROL_ENABLED',
Copy link
Member

Choose a reason for hiding this comment

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

these env overrides are not necessary. it defaults to NEW_RELIC_ path to config value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you suggesting to not provide default values? I don't understand the review comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, you're saying that { agent_control: { enabled } } will already be parsed from NEW_RELIC_AGENT_CONTROL_ENABLED regardless of setting the env key. I was unaware of that. Are you strongly against setting the env key? It is clearer to me.

Copy link
Member

Choose a reason for hiding this comment

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

this key is for overrides. it'll default to what you provided so it just confuses things. we default all env vars here

Copy link
Member

@bizob2828 bizob2828 Jan 24, 2025

Choose a reason for hiding this comment

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

I am against it because this was built to derive env vars. In fact before I built that support here we had drift and was constantly fixing one offs. The intent of the env key is to specify overrides when it doesn't fit our convention.

  • an override if the env var does not follow our standard convention. We should not have

Copy link
Member

Choose a reason for hiding this comment

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

maybe we can add PR separately and rename it envOverride or something, or we could document the structure better

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, renaming it to envVarNameOverride would be a lot clearer. I'll go ahead and remove them.

formatter: boolean,
default: false
},

/**
* Settings specific to the health monitoring aspect of Agent Control.
*/
health: {
/**
* A string file path to a directory that the agent is expected to write
* health status files to. Must be set for health monitoring to be
* enabled.
*/
delivery_location: {
env: 'NEW_RELIC_AGENT_CONTROL_HEALTH_DELIVERY_LOCATION',
default: 'file:///var/lib/newrelic-agent_control/fleet'
Copy link
Member

Choose a reason for hiding this comment

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

the latest spec update has the default as file:///newrelic/apm/health

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copied the example string, it seems.

},

/**
* The time, in seconds, that the agent should wait between writing
* updates to its health status. The default interval is 5 seconds.
*/
frequency: {
env: 'NEW_RELIC_AGENT_CONTROL_HEALTH_FREQUENCY',
formatter: int,
default: 5
}
}
},

/**
* The default Apdex tolerating / threshold value for applications, in
* seconds. The default for Node is apdexT to 100 milliseconds, which is
Expand Down
244 changes: 244 additions & 0 deletions lib/health-reporter.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,244 @@
/*
* Copyright 2024 New Relic Corporation. All rights reserved.
* SPDX-License-Identifier: Apache-2.0
*/

'use strict'

const fs = require('node:fs')
const crypto = require('node:crypto')
const path = require('node:path')
const { fileURLToPath } = require('node:url')

const defaultLogger = require('./logger').child({ component: 'HealthReporter' })

const VALID_CODES = new Map([
['NR-APM-000', 'Healthy.'],
['NR-APM-001', 'Invalid license key.'],
['NR-APM-002', 'License key missing.'],
['NR-APM-003', 'Forced disconnect received from New Relic.'],
['NR-APM-004', 'HTTP error communicating with New Relic.'],
['NR-APM-005', 'Missing application name in agent configuration.'],
['NR-APM-006', 'The maximum number of configured app names is exceeded.'],
['NR-APM-007', 'HTTP proxy is misconfigured.'],
['NR-APM-008', 'Agent is disabled via configuration.'],
['NR-APM-009', 'Failed to connect to the New Relic data collector.'],
['NR-APM-010', 'Agent config could not be parsed.'],
['NR-APM-099', 'Agent has shutdown.'],
// Codes 300 through 399 are reserved for the Node.js Agent.
['NR-APM-300', 'An unexpected error occurred.']
])

function getTime() {
// `process.hrtime.bigint` does not return a value relative to the epoch.
// So we have to perform this lossy calculation because the spec is
// insisting on nanoseconds.
return Date.now() * 1_000_000
}

function writeStatus({ file, healthy = true, code, msg, startTime, callback } = {}) {
const currentTime = getTime()
const yaml = [
`healthy: ${healthy}`,
`status: '${msg}'`,
`last_error: ${code}`,
`start_time_unix_nano: ${startTime}`,
`status_time_unix_nano: ${currentTime}`
].join('\n')
fs.writeFile(file, yaml, { encoding: 'utf8' }, callback)
}

function directoryAvailable(dest) {
try {
fs.accessSync(dest, fs.constants.R_OK | fs.constants.W_OK)
return { available: true }
} catch (error) {
return { available: false, error }
}
}

/**
* HealthReporter implements the "super agent" (New Relic Control) health
* check spec. An instance of the reporter will continually write out the
* current status, as set by `reporter.setStatus`, on the interval defined
* by the environment.
*/
class HealthReporter {
#enabled = false
#status = HealthReporter.STATUS_HEALTHY
#interval
#destFile
#logger
#startTime

static STATUS_HEALTHY = 'NR-APM-000'
static STATUS_INVALID_LICENSE_KEY = 'NR-APM-001'
static STATUS_LICENSE_KEY_MISSING = 'NR-APM-002'
static STATUS_FORCED_DISCONNECT = 'NR-APM-003'
static STATUS_BACKEND_ERROR = 'NR-APM-004'
static STATUS_MISSING_APP_NAME = 'NR-APM-005'
static STATUS_MAXIMUM_APP_NAMES_EXCEEDED = 'NR-APM-006'
static STATUS_HTTP_PROXY_MISCONFIGURED = 'NR-APM-007'
static STATUS_AGENT_DISABLED = 'NR-APM-008'
static STATUS_CONNECT_ERROR = 'NR-APM-009'
static STATUS_CONFIG_PARSE_FAILURE = 'NR-APM-010'
static STATUS_AGENT_SHUTDOWN = 'NR-APM-099'

// STATUS_INTERNAL errors are the Node.js Agent specific error codes.
static STATUS_INTERNAL_UNEXPECTED_ERROR = 'NR-APM-300'

constructor({
agentConfig = { agent_control: { health: {} } },
Copy link
Member

Choose a reason for hiding this comment

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

you don't have to default these as configuration has already done that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Except in testing. This default allows for easier unit testing and fewer ?. guards.

logger = defaultLogger,
setInterval = global.setInterval
} = {}) {
const enabled = agentConfig.agent_control?.enabled
let outDir = agentConfig.agent_control?.health?.delivery_location
let checkInterval = agentConfig.agent_control?.health?.frequency

this.#logger = logger

if (enabled !== true) {
this.#logger.info('new relic agent control disabled, skipping health reporting')
return
}

if (outDir === undefined) {
Copy link
Member

Choose a reason for hiding this comment

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

defaults to a path in config

this.#logger.error('health check output directory not provided, skipping health reporting')
return
}

if (outDir.includes('://') === true) {
Copy link
Member

Choose a reason for hiding this comment

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

looks like you're missing a test where the directory is a file url

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

ah, i was looking in the unit tests for health reporter, my bad

Copy link
Member

Choose a reason for hiding this comment

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

actually when i run the unit tests it is still showing those lines as uncovered

Copy link
Member

@bizob2828 bizob2828 Jan 24, 2025

Choose a reason for hiding this comment

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

that's because in the test you're replacing file:// with '': https://github.com/jsumners-nr/node-newrelic/blob/6accc52abc4f63a98e7a10abf25af6c2634433d0/test/unit/agent/agent.test.js#L94. you should just remove the env var declaration as that takes precedence over config file. there's also a few others places it's specifying it as an env var. once it's cleaned up it'll be covered

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I have it covered locally.

outDir = fileURLToPath(outDir)
}

Check warning on line 113 in lib/health-reporter.js

View check run for this annotation

Codecov / codecov/patch

lib/health-reporter.js#L112-L113

Added lines #L112 - L113 were not covered by tests
const dirCheck = directoryAvailable(outDir)
if (dirCheck.available === false) {
this.#logger.error('health check output directory not accessible, skipping health reporting', { error: dirCheck.error })
return
}

if (checkInterval === undefined) {
Copy link
Member

Choose a reason for hiding this comment

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

same with this, by setting the default in config it'll handle this

this.#logger.debug('health check interval not available, using default 5 seconds')
checkInterval = 5_000
} else {
checkInterval = parseInt(checkInterval, 10) * 1_000
}

this.#startTime = getTime()

const uuid = crypto.randomUUID().replaceAll('-', '')
this.#destFile = path.join(outDir, `health-${uuid}.yaml`)

this.#logger.info(
`new relic control is present, writing health on interval ${checkInterval} milliseconds to ${
this.#destFile
}`
)
this.#interval = setInterval(this.#healthCheck.bind(this), checkInterval)
this.#interval.unref()

this.#enabled = true
this.#logger.info('health reporter initialized')
}

#healthCheck() {
const healthy = this.#status === HealthReporter.STATUS_HEALTHY
writeStatus({
file: this.#destFile,
healthy,
startTime: this.#startTime,
code: this.#status,
msg: VALID_CODES.get(this.#status),
callback: (error) => {
if (error) {
this.#logger.error(`error when writing out health status: ${error.message}`)
}
}
})
}

get enabled() {
return this.#enabled
}

get destFile() {
return this.#destFile
}

/**
* Update the known health status. This status will be written to the health
* file on the next interval. If the provided status is not a recognized
* status, a log will be written and the status will not be updated.
*
* @param {string} status Utilize one of the static status fields.
*/
setStatus(status) {
if (this.#enabled === false) {
return
}

if (VALID_CODES.has(status) === false) {
this.#logger.warn(`invalid health reporter status provided: ${status}`)
return
}

if (
status === HealthReporter.STATUS_AGENT_SHUTDOWN &&
this.#status !== HealthReporter.STATUS_HEALTHY
) {
this.#logger.info(
`not setting shutdown health status due to current status code: ${this.#status}`
)
return
}

this.#status = status
}

/**
* This should be invoked on agent shutdown after setting the status
* to the shutdown status. It will stop the ongoing update interval,
* initiate an immediate write of the status file, and then invoke the
* provided callback.
*
* @param {function} done Callback to be invoked after the status file has
* been updated.
*/
stop(done) {
if (this.#enabled === false) {
done && done()
return
}

clearInterval(this.#interval)

const healthy = this.#status === HealthReporter.STATUS_HEALTHY
let code = this.#status
let msg = VALID_CODES.get(code)
if (healthy === true) {
// We only update the status on shutdown when the last known state is
// the healthy state. Otherwise, we need to leave the current code in
// place, and just update the report time.
code = HealthReporter.STATUS_AGENT_SHUTDOWN
msg = VALID_CODES.get(code)
}

writeStatus({
file: this.#destFile,
startTime: this.#startTime,
healthy,
code,
msg,
callback: (error) => {
if (error) {
this.#logger.error(
`error when writing out health status during shutdown: ${error.message}`
)
}
done && done()
}
})
}
}

module.exports = HealthReporter
Loading
Loading