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

Add dynamic labels to the wall profiler #73

Merged
merged 2 commits into from
May 29, 2024
Merged
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
25 changes: 23 additions & 2 deletions src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,32 @@ export function init(config: PyroscopeConfig = {}): void {
setProfiler(new PyroscopeProfiler(processedConfig))
}

// deprecated: please use getLabels
function getWallLabels(): Record<string, number | string> {
return getProfiler().wallProfiler.profiler.getLabels()
return getLabels()
}

// deprecated: please use setLabels
function setWallLabels(labels: Record<string, number | string>): void {
return setLabels(labels)
}

function getLabels(): Record<string, number | string> {
return getProfiler().wallProfiler.profiler.getLabels()
}

function setLabels(labels: Record<string, number | string>): void {
getProfiler().wallProfiler.profiler.setLabels(labels)
}

export function wrapWithLabels(
lbls: Record<string, string | number>,
fn: () => void,
...args: unknown[]
): void {
getProfiler().wallProfiler.profiler.wrapWithLabels(lbls, fn, ...args)
}

function startWallProfiling(): void {
getProfiler().wallProfiler.start()
}
Expand Down Expand Up @@ -69,9 +87,12 @@ export { PyroscopeConfig, PyroscopeHeapConfig, PyroscopeWallConfig }
export default {
SourceMapper,
expressMiddleware,
getWallLabels,
init,
getWallLabels,
setWallLabels,
getLabels,
setLabels,
wrapWithLabels,
start,
startHeapProfiling,
startWallProfiling,
Expand Down
18 changes: 12 additions & 6 deletions src/profilers/heap-profiler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,23 +14,29 @@ export interface HeapProfilerStartArgs {
}

export class HeapProfiler implements Profiler<HeapProfilerStartArgs> {
private labels: Record<string, number | string>
private lastProfiledAt: Date
private sourceMapper: SourceMapper | undefined

constructor() {
this.labels = {}
this.lastProfiledAt = new Date()
}

public getLabels(): Record<string, number | string> {
return this.labels
throw new Error("heap profiler doesn't support labels")
}

public wrapWithLabels(): void {
throw new Error("heap profiler doesn't support labels")
}

public profile(): ProfileExport {
log('profile')

const profile: Profile = heap.profile(undefined, this.sourceMapper, undefined)
const profile: Profile = heap.profile(
undefined,
this.sourceMapper,
undefined
)

const lastProfileStartedAt: Date = this.lastProfiledAt
this.lastProfiledAt = new Date()
Expand All @@ -42,8 +48,8 @@ export class HeapProfiler implements Profiler<HeapProfilerStartArgs> {
}
}

public setLabels(labels: Record<string, number | string>): void {
this.labels = labels
public setLabels(): void {
throw new Error("heap profiler doesn't support labels")
}

public start(args: HeapProfilerStartArgs): void {
Expand Down
6 changes: 6 additions & 0 deletions src/profilers/profiler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,12 @@ export interface Profiler<TStartArgs> {

setLabels(labels: Record<string, number | string>): void

wrapWithLabels(
labels: Record<string, number | string>,
fn: () => void,
...args: unknown[]
): void

start(args: TStartArgs): void

stop(): ProfileExport | null
Expand Down
69 changes: 55 additions & 14 deletions src/profilers/wall-profiler.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { time, SourceMapper } from '@datadog/pprof'
import { time, SourceMapper, LabelSet, TimeProfileNode } from '@datadog/pprof'
import { Profile } from 'pprof-format'

import { ProfileExport } from '../profile-exporter'
Expand All @@ -16,27 +16,63 @@ export interface WallProfilerStartArgs {
collectCpuTime: boolean
}

export interface GenerateTimeLabelsArgs {
node: TimeProfileNode
context?: TimeProfileNodeContext
}

export interface TimeProfileNodeContext {
context: ProfilerContext
timestamp: bigint
cpuTime: number
}

export interface ProfilerContext {
labels?: LabelSet
}

// eslint-disable-next-line @typescript-eslint/no-explicit-any
type AnyWrappedCallback = any

export class WallProfiler implements Profiler<WallProfilerStartArgs> {
private labels: Record<string, number | string>
private lastProfiledAt: Date
private lastContext: ProfilerContext
private lastSamplingIntervalMicros!: number

constructor() {
this.labels = {}
this.lastContext = {}
this.lastProfiledAt = new Date()
}

public getLabels(): Record<string, number | string> {
return this.labels
public getLabels(): LabelSet {
return this.lastContext.labels ?? {}
}

public profile(): ProfileExport {
log('profile')
return this.innerProfile(true)
}

public setLabels(labels: Record<string, number | string>): void {
this.labels = labels
public wrapWithLabels(
lbls: LabelSet,
fn: () => void,
...args: unknown[]
): void {
const oldLabels = this.getLabels()
this.setLabels({
...oldLabels,
...lbls,
})
;(fn as AnyWrappedCallback)(...args)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this semicolon necessary?

Suggested change
;(fn as AnyWrappedCallback)(...args)
(fn as AnyWrappedCallback)(...args)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think so, don't ask me why though

image

Copy link
Collaborator

@grafakus grafakus May 29, 2024

Choose a reason for hiding this comment

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

Yes it is; it separates the statements.

If not, the JS engine tries to use the return value of setLabels as a function that will receive the fn parameter.

Adding or not semicolons at the end of each line of JS code can be debated.
But for sure, not having them can create this confusion. Maybe we should configure Prettier to format the code automatically (in a future PR).

this.setLabels({
...oldLabels,
})
}

public setLabels(labels: LabelSet): void {
this.newContext({
labels: labels,
})
}

public start(args: WallProfilerStartArgs): void {
Expand All @@ -53,7 +89,7 @@ export class WallProfiler implements Profiler<WallProfilerStartArgs> {
workaroundV8Bug: true,
collectCpuTime: args.collectCpuTime,
})
time.setContext({})
this.newContext({})
}
}

Expand All @@ -62,13 +98,18 @@ export class WallProfiler implements Profiler<WallProfilerStartArgs> {
return this.innerProfile(false)
}

private innerProfile(restart: boolean): ProfileExport {
time.setContext({})
private newContext(o: ProfilerContext) {
this.lastContext = o
time.setContext(o)
}

const profile: Profile = time.stop(
restart,
(): Record<string, number | string> => this.labels
)
private generateLabels(args: GenerateTimeLabelsArgs): LabelSet {
return { ...(args.context?.context.labels ?? {}) }
}

private innerProfile(restart: boolean): ProfileExport {
this.newContext({})
const profile: Profile = time.stop(restart, this.generateLabels)

const lastProfileStartedAt: Date = this.lastProfiledAt
this.lastProfiledAt = new Date()
Expand Down
94 changes: 73 additions & 21 deletions test/profiler.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,26 +4,35 @@ import busboy from 'busboy'
import { Profile } from 'pprof-format'
import zlib from 'zlib'

type Numeric = number | bigint

const extractProfile = (
req: express.Request,
res: express.Response,
callback: (p: Profile, name: String) => void
callback: (p: Profile, name: string) => void
) => {
const bb = busboy({ headers: req.headers })
bb.on('file', (name, file) => {
file
.toArray()
.then((values) => callback(
Profile.decode(zlib.gunzipSync(values[0])),
name,
))
.then((values) =>
callback(Profile.decode(zlib.gunzipSync(values[0])), name)
)
})
bb.on('close', () => {
res.send('ok')
})
req.pipe(bb)
}

const doWork = (d: number): void => {
const time = +new Date() + d * 1000
let i = 0
while (+new Date() < time) {
i = i + Math.random()
}
}

describe('common behaviour of profilers', () => {
it('should call a server on startCpuProfiling and clear gracefully', (done) => {
Pyroscope.init({
Expand Down Expand Up @@ -142,7 +151,7 @@ describe('common behaviour of profilers', () => {
})()
})

it('should have labels on wall profile', (done) => {
it('should have dynamic labels on wall profile', (done) => {
Pyroscope.init({
serverAddress: 'http://localhost:4446',
appName: 'nodejs',
Expand All @@ -155,37 +164,81 @@ describe('common behaviour of profilers', () => {
samplingIntervalMicros: 100,
},
})
Pyroscope.setWallLabels({
vehicle: 'car',
})
const app = express()
const server = app.listen(4446, () => {
Pyroscope.startWallProfiling()
Pyroscope.wrapWithLabels(
{
vehicle: 'car',
},
() => {
doWork(0.2)
Pyroscope.wrapWithLabels(
{
brand: 'mercedes',
},
() => {
doWork(0.2)
}
)
}
)
})

let closeInvoked = false
const valuesPerLabel = new Map<string, Array<number>>()

app.post('/ingest', (req, res) => {
expect(req.query['spyName']).toEqual('nodespy')
expect(req.query['name']).toEqual('nodejs{}')
extractProfile(req, res, (p: Profile) => {
const s = (idx: Number | bigint) => p.stringTable.strings[Number(idx)]
// now take the first sample and check if it has the label as expected
expect(s(p.sample[0].label[0].key)).toEqual('vehicle')
expect(s(p.sample[0].label[0].str)).toEqual('car')
const s = (idx: Numeric): string => p.stringTable.strings[Number(idx)]

// expect sample, wall types
expect(p.sampleType.map((x) => `${s(x.type)}=${s(x.unit)}`)).toEqual([
'samples=count',
'wall=nanoseconds',
])
// aggregate per labels
p.sample.forEach((x) => {
const key: string = JSON.stringify(
x.label.reduce(
(result, current) => ({
...result,
[s(current.key)]: s(current.str),
}),
{}
)
)
const prev = valuesPerLabel.get(key) ?? [0, 0, 0]
valuesPerLabel.set(
key,
x.value.map((a, i) => Number(a) + prev[i])
)
})
})

if (!closeInvoked) {
closeInvoked = true
;(async () => {
await Pyroscope.stopWallProfiling()
server.close(() => {
// ensure we contain everything expected
const emptyLabels = JSON.stringify({})
const vehicleOnly = JSON.stringify({ vehicle: 'car' })
const vehicleAndBrand = JSON.stringify({
vehicle: 'car',
brand: 'mercedes',
})

expect(valuesPerLabel.keys()).toContain(emptyLabels)
expect(valuesPerLabel.keys()).toContain(vehicleOnly)
expect(valuesPerLabel.keys()).toContain(vehicleAndBrand)

const valuesVehicleOnly = valuesPerLabel.get(vehicleOnly) ?? [0, 0]
const valuesVehicleAndBrand = valuesPerLabel.get(
vehicleAndBrand
) ?? [0, 0]

// ensure the wall time is within a 20% range of each other
const ratio = valuesVehicleOnly[1] / valuesVehicleAndBrand[1]
expect(ratio).toBeGreaterThan(0.8)
expect(ratio).toBeLessThan(1.2)

done()
})
})()
Expand Down Expand Up @@ -213,13 +266,12 @@ describe('common behaviour of profilers', () => {
})

let closeInvoked = false

app.post('/ingest', (req, res) => {
expect(req.query['spyName']).toEqual('nodespy')
expect(req.query['name']).toEqual('nodejs{}')

extractProfile(req, res, (p: Profile) => {
const s = (idx: Number | bigint) => p.stringTable.strings[Number(idx)]
const s = (idx: number | bigint) => p.stringTable.strings[Number(idx)]
// expect sample, wall and cpu types
expect(p.sampleType.map((x) => `${s(x.type)}=${s(x.unit)}`)).toEqual([
'samples=count',
Expand Down