feat: Added New Relic Control health check #2841

jsumners-nr · 2024-12-17T18:10:12Z

This PR resolves #2838.

codecov · 2024-12-17T18:25:40Z

Codecov Report

Attention: Patch coverage is 99.37107% with 2 lines in your changes missing coverage. Please review.

Project coverage is 97.36%. Comparing base (46462d0) to head (6accc52).
Report is 8 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/health-reporter.js	99.18%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2841      +/-   ##
==========================================
- Coverage   97.39%   97.36%   -0.03%     
==========================================
  Files         308      310       +2     
  Lines       47330    47707     +377     
==========================================
+ Hits        46099    46452     +353     
- Misses       1231     1255      +24

Flag	Coverage Δ
integration-tests-cjs-18.x	`73.02% <58.49%> (-0.17%)`	⬇️
integration-tests-cjs-20.x	`73.02% <58.49%> (-0.17%)`	⬇️
integration-tests-cjs-22.x	`73.05% <58.49%> (-0.17%)`	⬇️
integration-tests-esm-18.x	`49.83% <49.04%> (-0.01%)`	⬇️
integration-tests-esm-20.x	`49.83% <49.04%> (-0.01%)`	⬇️
integration-tests-esm-22.x	`49.88% <49.04%> (-0.01%)`	⬇️
unit-tests-18.x	`89.18% <99.37%> (+0.06%)`	⬆️
unit-tests-20.x	`89.18% <99.37%> (+0.06%)`	⬆️
unit-tests-22.x	`89.19% <99.37%> (+0.06%)`	⬆️
versioned-tests-18.x	`79.07% <49.04%> (-0.32%)`	⬇️
versioned-tests-20.x	`79.05% <49.04%> (-0.34%)`	⬇️
versioned-tests-22.x	`79.06% <49.04%> (-0.34%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bizob2828

i know this is in draft, just called out the lack of config items defined

lib/health-reporter.js

bizob2828

I haven't completely reviewed but a few comments. Also I don't see tests for every health status. At the very least we should have already have tests for STATUS_HEALTHY, STATUS_INVALID_LICENSE_KEY, STATUS_AGENT_DISABLED, STATUS_CONNECT_ERROR

bizob2828 · 2025-01-24T14:48:26Z

lib/config/default.js

+     * to true health monitoring.
+     */
+    enabled: {
+      env: 'NEW_RELIC_AGENT_CONTROL_ENABLED',


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

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

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.

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

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

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

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

bizob2828 · 2025-01-24T14:49:46Z

lib/config/default.js

+       */
+      delivery_location: {
+        env: 'NEW_RELIC_AGENT_CONTROL_HEALTH_DELIVERY_LOCATION',
+        default: 'file:///var/lib/newrelic-agent_control/fleet'


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

I copied the example string, it seems.

bizob2828 · 2025-01-24T14:58:12Z

lib/health-reporter.js

+      return
+    }
+
+    if (outDir.includes('://') === true) {


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

It is covered by https://github.com/jsumners-nr/node-newrelic/blob/6accc52abc4f63a98e7a10abf25af6c2634433d0/test/unit/agent/agent.test.js#L91-L113

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

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

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

Yes, I have it covered locally.

bizob2828 · 2025-01-24T14:58:36Z

lib/health-reporter.js

+  static STATUS_INTERNAL_UNEXPECTED_ERROR = 'NR-APM-300'
+
+  constructor({
+    agentConfig = { agent_control: { health: {} } },


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

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

bizob2828 · 2025-01-24T14:59:11Z

lib/health-reporter.js

+      return
+    }
+
+    if (checkInterval === undefined) {


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

bizob2828 · 2025-01-24T14:59:35Z

lib/health-reporter.js

+      return
+    }
+
+    if (outDir === undefined) {


defaults to a path in config

bizob2828 · 2025-01-24T15:00:15Z

test/unit/agent/agent.test.js

+const fs = require('node:fs')
+const os = require('node:os')
+const path = require('node:path')
+process.env.NEW_RELIC_AGENT_CONTROL_ENABLED = 'true'


i would just set these in agent config

These can be considered cruft from the original implementation that was environment based only. I was just too lazy to remove them.

bizob2828 · 2025-01-24T15:01:05Z

test/unit/config/config.test.js

+    })
+  })
+
+  await t.test('loads from env', () => {


we have a separate file for env var conversion: test/unit/config/config-env.test.js

bizob2828 · 2025-01-24T15:01:26Z

test/unit/config/config.test.js

+    })
+  })
+
+  await t.test('loads from provided config', () => {


we have a file that also tests defaults test/unit/config/config-defaults.test.js

bizob2828 reviewed Jan 7, 2025

View reviewed changes

lib/health-reporter.js Outdated Show resolved Hide resolved

jsumners-nr force-pushed the issue-2838 branch 2 times, most recently from 9ad5af4 to 79b116c Compare January 13, 2025 17:21

jsumners-nr marked this pull request as ready for review January 13, 2025 18:07

jsumners-nr force-pushed the issue-2838 branch from 79b116c to 77c0af9 Compare January 15, 2025 13:31

jsumners-nr added 6 commits January 16, 2025 13:23

feat: Added New Relic Control health check

2533b8f

add docs and callback

2315c68

add events to agent

7efe832

add events to agent

6cde2e4

updated linting

6a60de8

implement configuration support

c817ebf

jsumners-nr force-pushed the issue-2838 branch from 77c0af9 to c817ebf Compare January 16, 2025 18:23

update for latest spec

f093ad4

jsumners-nr force-pushed the issue-2838 branch from f2b460b to f093ad4 Compare January 22, 2025 20:05

jsumners-nr added 3 commits January 23, 2025 07:54

fix time calculations

c95acdc

support file urls

3aac7ef

kick ci

6accc52

jsumners-nr requested a review from bizob2828 January 24, 2025 14:19

bizob2828 requested changes Jan 24, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Added New Relic Control health check #2841

feat: Added New Relic Control health check #2841

jsumners-nr commented Dec 17, 2024

codecov bot commented Dec 17, 2024 •

edited

Loading

bizob2828 left a comment

bizob2828 left a comment

bizob2828 Jan 24, 2025

jsumners-nr Jan 24, 2025

jsumners-nr Jan 24, 2025

bizob2828 Jan 24, 2025

bizob2828 Jan 24, 2025 •

edited

Loading

bizob2828 Jan 24, 2025

jsumners-nr Jan 24, 2025

bizob2828 Jan 24, 2025

jsumners-nr Jan 24, 2025

bizob2828 Jan 24, 2025

jsumners-nr Jan 24, 2025

bizob2828 Jan 24, 2025

bizob2828 Jan 24, 2025

bizob2828 Jan 24, 2025 •

edited

Loading

jsumners-nr Jan 24, 2025

bizob2828 Jan 24, 2025

jsumners-nr Jan 24, 2025

bizob2828 Jan 24, 2025

bizob2828 Jan 24, 2025

bizob2828 Jan 24, 2025

jsumners-nr Jan 24, 2025

bizob2828 Jan 24, 2025

bizob2828 Jan 24, 2025

feat: Added New Relic Control health check #2841

Are you sure you want to change the base?

feat: Added New Relic Control health check #2841

Conversation

jsumners-nr commented Dec 17, 2024

codecov bot commented Dec 17, 2024 • edited Loading

Codecov Report

bizob2828 left a comment

Choose a reason for hiding this comment

bizob2828 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bizob2828 Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bizob2828 Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 17, 2024 •

edited

Loading

bizob2828 Jan 24, 2025 •

edited

Loading

bizob2828 Jan 24, 2025 •

edited

Loading