Skip to content

Commit 817f1c8

Browse files
ferhatelmasferhatelmas
authored
fix: single callback for memory/handle collector (#1118)
Signed-off-by: ferhat elmas <elmas.ferhat@gmail.com>
1 parent 934a3e4 commit 817f1c8

4 files changed

Lines changed: 299 additions & 51 deletions

File tree

‎src/internal/monitoring/system/handles-collector.test.ts‎

Lines changed: 139 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,139 @@
1+
import { Meter, Observable } from '@opentelemetry/api'
2+
import { afterEach, describe, expect, test, vi } from 'vitest'
3+
import { HandlesCollector } from './handles-collector'
4+
5+
type ProcessWithActiveResources = NodeJS.Process & {
6+
_getActiveHandles?: () => unknown[] | null | undefined
7+
_getActiveRequests?: () => unknown[] | null | undefined
8+
}
9+
10+
interface CapturedInstrument {
11+
name: string
12+
observable: Observable<Record<string, string>>
13+
}
14+
15+
interface CapturedObservation {
16+
name: string
17+
value: number
18+
labels?: Record<string, string>
19+
}
20+
21+
function createMockMeter(): {
22+
meter: Meter
23+
invokeBatch: () => CapturedObservation[]
24+
} {
25+
const instruments: CapturedInstrument[] = []
26+
const batchCallbacks: Array<
27+
(observable: {
28+
observe: (
29+
metric: Observable<Record<string, string>>,
30+
value: number,
31+
labels?: Record<string, string>
32+
) => void
33+
}) => void
34+
> = []
35+
36+
const meter = {
37+
createObservableGauge(name: string) {
38+
const observable = {
39+
addCallback: vi.fn(),
40+
removeCallback: vi.fn(),
41+
}
42+
instruments.push({ name, observable })
43+
return observable
44+
},
45+
addBatchObservableCallback(callback: (observable: never) => void) {
46+
batchCallbacks.push(callback as (typeof batchCallbacks)[number])
47+
},
48+
} as unknown as Meter
49+
50+
const invokeBatch = (): CapturedObservation[] => {
51+
expect(batchCallbacks).toHaveLength(1)
52+
53+
const observations: CapturedObservation[] = []
54+
const observable = {
55+
observe(
56+
metric: Observable<Record<string, string>>,
57+
value: number,
58+
labels?: Record<string, string>
59+
) {
60+
const instrument = instruments.find((candidate) => candidate.observable === metric)
61+
expect(instrument).toBeDefined()
62+
observations.push({ name: instrument!.name, value, labels })
63+
},
64+
}
65+
66+
batchCallbacks[0](observable)
67+
return observations
68+
}
69+
70+
return { meter, invokeBatch }
71+
}
72+
73+
describe('HandlesCollector', () => {
74+
afterEach(() => {
75+
vi.restoreAllMocks()
76+
})
77+
78+
test('observes active handle and request gauges from one batch callback', () => {
79+
const processWithActiveResources = process as ProcessWithActiveResources
80+
const getActiveHandles = vi
81+
.spyOn(processWithActiveResources, '_getActiveHandles')
82+
.mockReturnValue([{}, {}])
83+
const getActiveRequests = vi
84+
.spyOn(processWithActiveResources, '_getActiveRequests')
85+
.mockReturnValue([{}])
86+
87+
const { meter, invokeBatch } = createMockMeter()
88+
const collector = new HandlesCollector({
89+
prefix: 'test',
90+
labels: { tenant: 'tenant-a' },
91+
})
92+
93+
collector.updateMetricInstruments(meter)
94+
collector.enable()
95+
96+
expect(invokeBatch()).toEqual([
97+
{
98+
name: 'test.nodejs.active_handles.total',
99+
value: 2,
100+
labels: { tenant: 'tenant-a' },
101+
},
102+
{
103+
name: 'test.nodejs.active_requests.total',
104+
value: 1,
105+
labels: { tenant: 'tenant-a' },
106+
},
107+
])
108+
expect(getActiveHandles).toHaveBeenCalledTimes(1)
109+
expect(getActiveRequests).toHaveBeenCalledTimes(1)
110+
})
111+
112+
test('observes zero when active resource APIs return nullish values', () => {
113+
const processWithActiveResources = process as ProcessWithActiveResources
114+
vi.spyOn(processWithActiveResources, '_getActiveHandles').mockReturnValue(undefined)
115+
vi.spyOn(processWithActiveResources, '_getActiveRequests').mockReturnValue(null)
116+
117+
const { meter, invokeBatch } = createMockMeter()
118+
const collector = new HandlesCollector({
119+
prefix: 'test',
120+
labels: { tenant: 'tenant-a' },
121+
})
122+
123+
collector.updateMetricInstruments(meter)
124+
collector.enable()
125+
126+
expect(invokeBatch()).toEqual([
127+
{
128+
name: 'test.nodejs.active_handles.total',
129+
value: 0,
130+
labels: { tenant: 'tenant-a' },
131+
},
132+
{
133+
name: 'test.nodejs.active_requests.total',
134+
value: 0,
135+
labels: { tenant: 'tenant-a' },
136+
},
137+
])
138+
})
139+
})

‎src/internal/monitoring/system/handles-collector.ts‎

Lines changed: 22 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -3,27 +3,33 @@ import { BaseCollector } from './base-collector'
33

44
export class HandlesCollector extends BaseCollector {
55
updateMetricInstruments(meter: Meter): void {
6-
meter
7-
.createObservableGauge(`${this.namePrefix}nodejs.active_handles.total`, {
6+
const activeHandlesGauge = meter.createObservableGauge(
7+
`${this.namePrefix}nodejs.active_handles.total`,
8+
{
89
description: 'Number of active libuv handles',
9-
})
10-
.addCallback((observable) => {
11-
if (!this._enabled) return
12-
// @ts-expect-error - _getActiveHandles is not in types but exists
13-
const handles = process._getActiveHandles?.() || []
14-
observable.observe(handles.length, this.labels)
15-
})
10+
}
11+
)
1612

17-
meter
18-
.createObservableGauge(`${this.namePrefix}nodejs.active_requests.total`, {
13+
const activeRequestsGauge = meter.createObservableGauge(
14+
`${this.namePrefix}nodejs.active_requests.total`,
15+
{
1916
description: 'Number of active libuv requests',
20-
})
21-
.addCallback((observable) => {
17+
}
18+
)
19+
20+
meter.addBatchObservableCallback(
21+
(observable) => {
2222
if (!this._enabled) return
23+
24+
// @ts-expect-error - _getActiveHandles is not in types but exists
25+
const handles = process._getActiveHandles?.() ?? []
2326
// @ts-expect-error - _getActiveRequests is not in types but exists
24-
const requests = process._getActiveRequests?.() || []
25-
observable.observe(requests.length, this.labels)
26-
})
27+
const requests = process._getActiveRequests?.() ?? []
28+
observable.observe(activeHandlesGauge, handles.length, this.labels)
29+
observable.observe(activeRequestsGauge, requests.length, this.labels)
30+
},
31+
[activeHandlesGauge, activeRequestsGauge]
32+
)
2733
}
2834

2935
protected internalEnable(): void {

‎src/internal/monitoring/system/memory-collector.test.ts‎

Lines changed: 111 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,111 @@
1+
import { Meter, Observable } from '@opentelemetry/api'
2+
import { afterEach, describe, expect, test, vi } from 'vitest'
3+
import { ExternalMemoryCollector } from './memory-collector'
4+
5+
interface CapturedInstrument {
6+
name: string
7+
observable: Observable<Record<string, string>>
8+
}
9+
10+
interface CapturedObservation {
11+
name: string
12+
value: number
13+
labels?: Record<string, string>
14+
}
15+
16+
function createMockMeter(): {
17+
meter: Meter
18+
instruments: CapturedInstrument[]
19+
invokeBatch: () => CapturedObservation[]
20+
} {
21+
const instruments: CapturedInstrument[] = []
22+
const batchCallbacks: Array<
23+
(observable: {
24+
observe: (
25+
metric: Observable<Record<string, string>>,
26+
value: number,
27+
labels?: Record<string, string>
28+
) => void
29+
}) => void
30+
> = []
31+
32+
const meter = {
33+
createObservableGauge(name: string) {
34+
const observable = {
35+
addCallback: vi.fn(),
36+
removeCallback: vi.fn(),
37+
}
38+
instruments.push({ name, observable })
39+
return observable
40+
},
41+
addBatchObservableCallback(callback: (observable: never) => void) {
42+
batchCallbacks.push(callback as (typeof batchCallbacks)[number])
43+
},
44+
} as unknown as Meter
45+
46+
const invokeBatch = (): CapturedObservation[] => {
47+
expect(batchCallbacks).toHaveLength(1)
48+
49+
const observations: CapturedObservation[] = []
50+
const observable = {
51+
observe(
52+
metric: Observable<Record<string, string>>,
53+
value: number,
54+
labels?: Record<string, string>
55+
) {
56+
const instrument = instruments.find((candidate) => candidate.observable === metric)
57+
expect(instrument).toBeDefined()
58+
observations.push({ name: instrument!.name, value, labels })
59+
},
60+
}
61+
62+
batchCallbacks[0](observable)
63+
return observations
64+
}
65+
66+
return { meter, instruments, invokeBatch }
67+
}
68+
69+
describe('ExternalMemoryCollector', () => {
70+
afterEach(() => {
71+
vi.restoreAllMocks()
72+
})
73+
74+
test('observes all memory gauges from one process.memoryUsage read', () => {
75+
const memoryUsage = vi.spyOn(process, 'memoryUsage').mockReturnValue({
76+
rss: 10,
77+
heapTotal: 20,
78+
heapUsed: 30,
79+
external: 40,
80+
arrayBuffers: 50,
81+
})
82+
83+
const { meter, invokeBatch } = createMockMeter()
84+
const collector = new ExternalMemoryCollector({
85+
prefix: 'test',
86+
labels: { tenant: 'tenant-a' },
87+
})
88+
89+
collector.updateMetricInstruments(meter)
90+
collector.enable()
91+
92+
expect(invokeBatch()).toEqual([
93+
{
94+
name: 'test.nodejs.memory.external',
95+
value: 40,
96+
labels: { tenant: 'tenant-a' },
97+
},
98+
{
99+
name: 'test.nodejs.memory.array_buffers',
100+
value: 50,
101+
labels: { tenant: 'tenant-a' },
102+
},
103+
{
104+
name: 'test.nodejs.memory.rss',
105+
value: 10,
106+
labels: { tenant: 'tenant-a' },
107+
},
108+
])
109+
expect(memoryUsage).toHaveBeenCalledTimes(1)
110+
})
111+
})

‎src/internal/monitoring/system/memory-collector.ts‎

Lines changed: 27 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -3,54 +3,46 @@ import { BaseCollector } from './base-collector'
33

44
export class ExternalMemoryCollector extends BaseCollector {
55
updateMetricInstruments(meter: Meter): void {
6-
meter
7-
.createObservableGauge(`${this.namePrefix}nodejs.memory.external`, {
6+
const externalMemoryGauge = meter.createObservableGauge(
7+
`${this.namePrefix}nodejs.memory.external`,
8+
{
89
description: 'Node.js external memory size in bytes',
910
unit: 'By',
10-
})
11-
.addCallback((observable) => {
12-
if (!this._enabled) return
13-
try {
14-
const mem = process.memoryUsage()
15-
if (mem.external !== undefined) {
16-
observable.observe(mem.external, this.labels)
17-
}
18-
} catch {
19-
// ignore errors
20-
}
21-
})
11+
}
12+
)
2213

23-
meter
24-
.createObservableGauge(`${this.namePrefix}nodejs.memory.array_buffers`, {
14+
const arrayBuffersMemoryGauge = meter.createObservableGauge(
15+
`${this.namePrefix}nodejs.memory.array_buffers`,
16+
{
2517
description: 'Node.js ArrayBuffers memory size in bytes',
2618
unit: 'By',
27-
})
28-
.addCallback((observable) => {
19+
}
20+
)
21+
22+
const rssMemoryGauge = meter.createObservableGauge(`${this.namePrefix}nodejs.memory.rss`, {
23+
description: 'Resident Set Size - total memory allocated for the process',
24+
unit: 'By',
25+
})
26+
27+
meter.addBatchObservableCallback(
28+
(observable) => {
2929
if (!this._enabled) return
30+
3031
try {
3132
const mem = process.memoryUsage()
33+
if (mem.external !== undefined) {
34+
observable.observe(externalMemoryGauge, mem.external, this.labels)
35+
}
3236
if (mem.arrayBuffers !== undefined) {
33-
observable.observe(mem.arrayBuffers, this.labels)
37+
observable.observe(arrayBuffersMemoryGauge, mem.arrayBuffers, this.labels)
3438
}
39+
observable.observe(rssMemoryGauge, mem.rss, this.labels)
3540
} catch {
3641
// ignore errors
3742
}
38-
})
39-
40-
meter
41-
.createObservableGauge(`${this.namePrefix}nodejs.memory.rss`, {
42-
description: 'Resident Set Size - total memory allocated for the process',
43-
unit: 'By',
44-
})
45-
.addCallback((observable) => {
46-
if (!this._enabled) return
47-
try {
48-
const mem = process.memoryUsage()
49-
observable.observe(mem.rss, this.labels)
50-
} catch {
51-
// ignore errors
52-
}
53-
})
43+
},
44+
[externalMemoryGauge, arrayBuffersMemoryGauge, rssMemoryGauge]
45+
)
5446
}
5547

5648
protected internalEnable(): void {

0 commit comments

Comments
 (0)