Skip to content

Commit e57223c

Browse files
bizob2828bizob2828
authored
refactor: Updated agent to properly handle profiling.enabled via server side config (#3847)
1 parent 4af2f48 commit e57223c

File tree

3 files changed

+89
-122
lines changed

3 files changed

+89
-122
lines changed

‎lib/agent.js‎

Lines changed: 1 addition & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ const { createFeatureUsageMetrics } = require('./util/application-logging')
4242
const HealthReporter = require('./health-reporter')
4343
const Samplers = require('./samplers')
4444
const ProfilingAggregator = require('./aggregators/profiling-aggregator')
45-
const { createStartupProfilingMetrics, createProfilingFlagMetric } = require('./profiling/metrics')
45+
const { createStartupProfilingMetrics } = require('./profiling/metrics')
4646

4747
// Map of valid states to whether or not data collection is valid
4848
const STATES = {
@@ -870,15 +870,6 @@ Agent.prototype._listenForConfigChanges = function _listenForConfigChanges() {
870870
generateEventHarvestSupportMetrics(self, harvestConfig)
871871
}
872872
})
873-
this.config.on('profiling.enabled', async function onProfilingEnabledChange(value) {
874-
createProfilingFlagMetric(self)
875-
self.profilingData.reconfigure(self.config)
876-
if (value === true) {
877-
self.profilingData.start()
878-
} else {
879-
self.profilingData.stop()
880-
}
881-
})
882873
}
883874

884875
/**

‎test/unit/agent/agent.test.js‎

Lines changed: 57 additions & 84 deletions
Original file line numberDiff line numberDiff line change
@@ -1198,93 +1198,66 @@ test('when profiling aggregator has duration set to default (0)', async (t) => {
11981198
})
11991199
})
12001200

1201-
test('when `onConnect` is called to update profiling metrics', async (t) => {
1202-
t.beforeEach((ctx) => {
1203-
ctx.nr = {}
1204-
ctx.nr.agent = helper.loadMockedAgent(null, false)
1205-
})
1206-
1207-
t.afterEach((ctx) => {
1208-
helper.unloadAgent(ctx.nr.agent)
1209-
})
1210-
1211-
await t.test('should add startup profiling metrics when profiling is enabled', (t, end) => {
1212-
const { agent } = t.nr
1213-
1214-
agent.config.profiling.enabled = true
1215-
agent.config.profiling.include = ['heap']
1216-
1217-
agent.onConnect(false, () => {
1218-
const disabled = agent.metrics.getMetric(`${PROFILING.PREFIX}disabled`)
1219-
const enabled = agent.metrics.getMetric(`${PROFILING.PREFIX}enabled`)
1220-
const heap = agent.metrics.getMetric(`${PROFILING.PREFIX}${PROFILING.HEAP}`)
1221-
const cpu = agent.metrics.getMetric(`${PROFILING.PREFIX}${PROFILING.CPU}`)
1222-
assert.equal(disabled, null)
1223-
assert.equal(enabled.callCount, 1)
1224-
assert.equal(heap.callCount, 1)
1225-
assert.equal(cpu, null)
1226-
end()
1227-
})
1228-
})
1229-
1230-
await t.test('should only add profiling flag metric and not type when profiling is disabled', (t, end) => {
1231-
const { agent } = t.nr
1232-
1233-
agent.onConnect(false, () => {
1234-
const disabled = agent.metrics.getMetric(`${PROFILING.PREFIX}disabled`)
1235-
const enabled = agent.metrics.getMetric(`${PROFILING.PREFIX}enabled`)
1236-
const heap = agent.metrics.getMetric(`${PROFILING.PREFIX}${PROFILING.HEAP}`)
1237-
const cpu = agent.metrics.getMetric(`${PROFILING.PREFIX}${PROFILING.CPU}`)
1238-
assert.equal(enabled, null)
1239-
assert.equal(disabled.callCount, 1)
1240-
assert.equal(heap, null)
1241-
assert.equal(cpu, null)
1242-
end()
1243-
})
1244-
})
1245-
})
1246-
1247-
test('when `profiling.enabled` changes', async (t) => {
1248-
t.beforeEach((ctx) => {
1249-
ctx.nr = {}
1250-
const config = {
1251-
profiling: {
1252-
enabled: false
1253-
}
1254-
}
1255-
ctx.nr.agent = helper.loadMockedAgent(config, false)
1256-
})
1201+
/**
1202+
* there is more to apply server-side configuration but it is unrelated to agent
1203+
* the flow is this:
1204+
* - collector returns a 409 which tells the agent to restart
1205+
* - stops harvester
1206+
* - shutdown the client(CollectorApi)
1207+
* - connects to collector
1208+
* - starts flow: preconnect, connect
1209+
* - connect calls `agent.reconfigure`
1210+
* - `agent.reconfigure` calls `config.onConnect` which wires up any changes from server(collector)
1211+
* - calls `agent.onConnect`
1212+
* - `agent.onConnect` calls `harvester.reconfigure` and creates profiling supportability metrics
1213+
*
1214+
* with all that being said we only have to call the important bits on agent/config manually to simulate the flow
1215+
* - `harvester.stop`
1216+
* - `config.onConnect` with the profiling.enabled change
1217+
* - `agent.onConnect`
1218+
*/
1219+
test('should start/stop profiling aggregator and log appropriate supportability metrics', async (t) => {
1220+
const agent = helper.loadMockedAgent(null, false)
12571221

1258-
t.afterEach((ctx) => {
1259-
helper.unloadAgent(ctx.nr.agent)
1222+
t.after(() => {
1223+
helper.unloadAgent(agent)
12601224
})
12611225

1262-
await t.test('should handle changes accordingly', (t) => {
1263-
const { agent } = t.nr
1264-
assert.equal(agent.profilingData.enabled, false)
1265-
assert.ok(!agent.profilingData.sendTimer)
1266-
agent.config.onConnect({ 'profiling.enabled': true })
1267-
assert.equal(agent.profilingData.enabled, true)
1268-
assert.ok(agent.profilingData.sendTimer)
1269-
agent.config.onConnect({ 'profiling.enabled': false })
1270-
assert.equal(agent.profilingData.enabled, false)
1271-
assert.ok(!agent.profilingData.sendTimer)
1272-
agent.config.onConnect({ 'profiling.enabled': true })
1273-
assert.equal(agent.profilingData.enabled, true)
1274-
assert.ok(agent.profilingData.sendTimer)
1275-
})
1276-
1277-
await t.test('should add supportability metrics', (t) => {
1278-
const { agent } = t.nr
1279-
assert.equal(agent.profilingData.enabled, false)
1280-
agent.config.onConnect({ 'profiling.enabled': true })
1281-
assert.equal(agent.profilingData.enabled, true)
1282-
const enabled = agent.metrics.getMetric(`${PROFILING.PREFIX}enabled`)
1283-
assert.equal(enabled.callCount, 1)
1284-
agent.config.onConnect({ 'profiling.enabled': false })
1285-
const disabled = agent.metrics.getMetric(`${PROFILING.PREFIX}disabled`)
1286-
assert.equal(disabled.callCount, 1)
1287-
})
1226+
assert.ok(!agent.profilingData.sendTimer)
1227+
agent.harvester.stop()
1228+
// this is what would happen in server side config
1229+
agent.config.onConnect({ 'profiling.enabled': true })
1230+
// not doing `onConnect` as this value isn't wired up for SSC
1231+
agent.config.profiling.include = ['heap']
1232+
1233+
await new Promise((resolve) => {
1234+
agent.onConnect(false, resolve)
1235+
})
1236+
let disabled = agent.metrics.getMetric(`${PROFILING.PREFIX}disabled`)
1237+
let enabled = agent.metrics.getMetric(`${PROFILING.PREFIX}enabled`)
1238+
let heap = agent.metrics.getMetric(`${PROFILING.PREFIX}${PROFILING.HEAP}`)
1239+
let cpu = agent.metrics.getMetric(`${PROFILING.PREFIX}${PROFILING.CPU}`)
1240+
assert.equal(disabled, null)
1241+
assert.equal(enabled.callCount, 1)
1242+
assert.equal(heap.callCount, 1)
1243+
assert.equal(cpu, null)
1244+
assert.ok(agent.profilingData.sendTimer)
1245+
agent.harvester.stop()
1246+
agent.config.onConnect({ 'profiling.enabled': false })
1247+
// clear out metrics so we can re-assert
1248+
agent.metrics.clear()
1249+
await new Promise((resolve) => {
1250+
agent.onConnect(false, resolve)
1251+
})
1252+
disabled = agent.metrics.getMetric(`${PROFILING.PREFIX}disabled`)
1253+
enabled = agent.metrics.getMetric(`${PROFILING.PREFIX}enabled`)
1254+
heap = agent.metrics.getMetric(`${PROFILING.PREFIX}${PROFILING.HEAP}`)
1255+
cpu = agent.metrics.getMetric(`${PROFILING.PREFIX}${PROFILING.CPU}`)
1256+
assert.equal(disabled.callCount, 1)
1257+
assert.equal(enabled, null)
1258+
assert.equal(heap, null)
1259+
assert.equal(cpu, null)
1260+
assert.ok(!agent.profilingData.sendTimer)
12881261
})
12891262

12901263
test('when event_harvest_config update on connect with a valid config', async (t) => {

‎test/unit/aggregators/profiling-aggregator.test.js‎

Lines changed: 31 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -11,30 +11,22 @@ const sinon = require('sinon')
1111
const ProfilingAggregator = require('#agentlib/aggregators/profiling-aggregator.js')
1212
const helper = require('#testlib/agent_helper.js')
1313
const RUN_ID = 1337
14-
const createProfiler = require('../mocks/profiler')
14+
const WAIT = 180
1515

1616
test.beforeEach((ctx) => {
1717
const sandbox = sinon.createSandbox()
18-
// disabling profiling and setting include list to an empty array
19-
// we will inject mocks in each test below
2018
const agent = helper.loadMockedAgent({
2119
profiling: {
2220
enabled: true,
23-
include: []
21+
include: ['heap', 'cpu']
2422
}
2523
})
26-
const cpuProfiler = createProfiler({ sandbox, name: 'CpuProfiler', data: 'cpu profile data' })
27-
const heapProfiler = createProfiler({ sandbox, name: 'HeapProfiler', data: 'heap profile data' })
28-
const clock = sinon.useFakeTimers()
2924
sandbox.spy(agent.collector, 'send')
3025
const profilingAggregator = new ProfilingAggregator({ runId: RUN_ID, periodMs: 100 }, agent)
3126
const profilingManager = profilingAggregator.profilingManager
3227
sandbox.spy(profilingManager, 'register')
3328
ctx.nr = {
3429
agent,
35-
clock,
36-
cpuProfiler,
37-
heapProfiler,
3830
profilingAggregator,
3931
profilingManager,
4032
sandbox
@@ -43,7 +35,6 @@ test.beforeEach((ctx) => {
4335

4436
test.afterEach((ctx) => {
4537
helper.unloadAgent(ctx.nr.agent)
46-
ctx.nr.clock.restore()
4738
ctx.nr.sandbox.restore()
4839
})
4940

@@ -60,49 +51,52 @@ test('should initialize pprofData and profilingManager', (t) => {
6051
})
6152

6253
test('should send 2 messages per interval', async (t) => {
63-
const { profilingAggregator, clock, agent, cpuProfiler, heapProfiler } = t.nr
54+
const { profilingAggregator, agent } = t.nr
6455
assert.equal(profilingAggregator.profilingManager.register.callCount, 0)
65-
profilingAggregator.profilingManager.profilers.set('CpuProfiler', cpuProfiler)
66-
profilingAggregator.profilingManager.profilers.set('HeapProfiler', heapProfiler)
6756
profilingAggregator.start()
6857
assert.equal(profilingAggregator.profilingManager.register.callCount, 1)
6958
assert.equal(agent.collector.send.callCount, 0)
70-
clock.tick(100)
71-
// need to run in next tick to ensure the promise chain around `profilingManager.collectData` resolves
7259
await new Promise((resolve) => {
73-
process.nextTick(() => {
60+
setTimeout(() => {
7461
assert.equal(agent.collector.send.callCount, 2)
7562
const [cpuCall, heapCall] = agent.collector.send.args
7663
assert.equal(cpuCall[0], 'pprof_data')
77-
assert.equal(cpuCall[1], 'cpu profile data')
64+
assert.equal(Buffer.isBuffer(cpuCall[1]), true)
7865
assert.equal(heapCall[0], 'pprof_data')
79-
assert.equal(heapCall[1], 'heap profile data')
66+
assert.equal(Buffer.isBuffer(heapCall[1]), true)
8067
assert.equal(profilingAggregator.pprofData, null)
8168
resolve()
82-
})
69+
}, WAIT)
8370
})
8471
})
8572

8673
test('should not send any data if there are no profilers registered', async (t) => {
87-
const { profilingAggregator, clock, agent } = t.nr
88-
profilingAggregator.profilingManager.profilers = new Set()
74+
const { profilingAggregator, agent } = t.nr
75+
profilingAggregator.profilingManager.config.include = []
8976
profilingAggregator.start()
9077
assert.equal(agent.collector.send.callCount, 0)
91-
clock.tick(100)
9278
await new Promise((resolve) => {
93-
process.nextTick(() => {
79+
setTimeout(() => {
9480
assert.equal(agent.collector.send.callCount, 0)
9581
resolve()
96-
})
82+
}, WAIT)
83+
})
84+
})
85+
86+
test('should not crash if profilers are started more than once', (t) => {
87+
const { profilingAggregator } = t.nr
88+
profilingAggregator.start()
89+
assert.doesNotThrow(() => {
90+
profilingAggregator.start()
9791
})
9892
})
9993

10094
test('should stop ProfilingManager when aggregator is stopped', (t) => {
101-
const { profilingAggregator, cpuProfiler, heapProfiler } = t.nr
102-
profilingAggregator.profilingManager.profilers.set('CpuProfiler', cpuProfiler)
103-
profilingAggregator.profilingManager.profilers.set('HeapProfiler', heapProfiler)
95+
const { profilingAggregator, sandbox } = t.nr
10496
profilingAggregator.start()
10597
assert.ok(profilingAggregator.sendTimer)
98+
sandbox.spy(profilingAggregator.profilingManager.profilers.get('HeapProfiler'), 'stop')
99+
sandbox.spy(profilingAggregator.profilingManager.profilers.get('CpuProfiler'), 'stop')
106100
for (const [, profiler] of profilingAggregator.profilingManager.profilers) {
107101
assert.equal(profiler.stop.callCount, 0)
108102
}
@@ -112,3 +106,12 @@ test('should stop ProfilingManager when aggregator is stopped', (t) => {
112106
assert.equal(profiler.stop.callCount, 1)
113107
}
114108
})
109+
110+
test('should not crash if profilers are stopped more than once', (t) => {
111+
const { profilingAggregator } = t.nr
112+
profilingAggregator.start()
113+
profilingAggregator.stop()
114+
assert.doesNotThrow(() => {
115+
profilingAggregator.stop()
116+
})
117+
})

0 commit comments

Comments
 (0)