Skip to content

Commit 18cfdbb

Browse files
authored
fix: replace statsd-client with hot-shots (#4806)
* fix: replace statsd-client with hot-shots * chore: comment * chore: fix test * chore: refactor code * chore: jsdoc * chore: better framework check * chore: variable name
1 parent 724a5f7 commit 18cfdbb

File tree

8 files changed

+226
-238
lines changed

8 files changed

+226
-238
lines changed

package-lock.json

Lines changed: 57 additions & 173 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

packages/build/package.json

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,7 @@
8080
"figures": "^4.0.0",
8181
"filter-obj": "^3.0.0",
8282
"got": "^10.0.0",
83+
"hot-shots": "9.3.0",
8384
"indent-string": "^5.0.0",
8485
"is-plain-obj": "^4.0.0",
8586
"js-yaml": "^4.0.0",
@@ -105,7 +106,6 @@
105106
"rfdc": "^1.3.0",
106107
"safe-json-stringify": "^1.2.0",
107108
"semver": "^7.0.0",
108-
"statsd-client": "0.4.7",
109109
"string-width": "^5.0.0",
110110
"strip-ansi": "^7.0.0",
111111
"supports-color": "^9.0.0",
@@ -120,7 +120,6 @@
120120
"devDependencies": {
121121
"@netlify/nock-udp": "^3.1.0",
122122
"@types/node": "^14.18.31",
123-
"@types/statsd-client": "^0.4.3",
124123
"atob": "^2.1.2",
125124
"ava": "^4.0.0",
126125
"c8": "^7.12.0",

packages/build/src/core/main.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,7 @@ export default async function buildSite(flags: Partial<BuildCLIFlags> = {}): Pro
107107
testOpts,
108108
errorParams,
109109
})
110-
await reportError({ error, framework, statsdOpts })
110+
await reportError(error, statsdOpts, framework)
111111

112112
return { success, severityCode, logs }
113113
}
@@ -122,7 +122,7 @@ const handleBuildSuccess = async function ({ framework, dry, logs, timers, durat
122122
logBuildSuccess(logs)
123123

124124
logTimer(logs, durationNs, 'Netlify Build', systemLog)
125-
await reportTimers({ timers, statsdOpts, framework })
125+
await reportTimers(timers, statsdOpts, framework)
126126
}
127127

128128
// Handles the calls and errors of telemetry reports

packages/build/src/error/report.ts

Lines changed: 30 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1,22 +1,29 @@
1+
import type { Tags } from 'hot-shots'
2+
13
import { isNetlifyMaintainedPlugin } from '../plugins/internal.js'
2-
import { closeClient, normalizeTagName, startClient } from '../report/statsd.js'
4+
import {
5+
closeClient,
6+
formatTags,
7+
InputStatsDOptions,
8+
normalizeTagName,
9+
startClient,
10+
validateStatsDOptions,
11+
} from '../report/statsd.js'
312

413
import { getErrorInfo } from './info.js'
514

615
const TOP_PARENT_TAG = 'run_netlify_build'
716

8-
// Record error rates of the build phase for monitoring.
9-
// Sends to statsd daemon.
10-
export const reportError = async function ({
11-
error,
12-
statsdOpts: { host, port },
13-
framework,
14-
}: {
15-
error: Error
16-
statsdOpts: { host?: string; port: number }
17-
framework?: string
18-
}) {
19-
if (host === undefined) {
17+
/**
18+
* Record error rates of the build phase for monitoring.
19+
* Sends to statsd daemon.
20+
*/
21+
export const reportError = async function (
22+
error: Error,
23+
statsdOpts: InputStatsDOptions,
24+
framework?: string,
25+
): Promise<void> {
26+
if (!validateStatsDOptions(statsdOpts)) {
2027
return
2128
}
2229

@@ -30,14 +37,16 @@ export const reportError = async function ({
3037

3138
const parent = pluginName ? pluginName : TOP_PARENT_TAG
3239
const stage = pluginName ? errorInfo.location?.event : errorInfo.stage
33-
const client = await startClient(host, port)
34-
35-
const frameworkTag: { framework?: string } = framework === undefined ? {} : { framework }
36-
client.increment('buildbot.build.stage.error', 1, {
37-
stage: stage ?? 'system',
38-
parent,
39-
...frameworkTag,
40-
})
40+
const statsDTags: Tags = { stage: stage ?? 'system', parent }
41+
42+
// Do not add a framework tag if empty string or null/undefined
43+
if (framework) {
44+
statsDTags.framework = framework
45+
}
46+
47+
const client = await startClient(statsdOpts)
48+
49+
client.increment('buildbot.build.stage.error', 1, formatTags(statsDTags))
4150

4251
await closeClient(client)
4352
}
Lines changed: 54 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -1,40 +1,69 @@
11
import { promisify } from 'util'
22

33
import slugify from '@sindresorhus/slugify'
4-
import StatsdClient from 'statsd-client'
4+
import { StatsD } from 'hot-shots'
55

6-
// TODO: replace with `timers/promises` after dropping Node < 15.0.0
7-
const pSetTimeout = promisify(setTimeout)
6+
export interface InputStatsDOptions {
7+
host?: string
8+
port?: number
9+
}
810

9-
// See https://github.com/msiebuhr/node-statsd-client/blob/45a93ee4c94ca72f244a40b06cb542d4bd7c3766/lib/EphemeralSocket.js#L81
10-
const CLOSE_TIMEOUT = 11
11+
export type StatsDOptions = Required<InputStatsDOptions>
1112

12-
// The socket creation is delayed until the first packet is sent. In our
13-
// case, this happens just before `client.close()` is called, which is too
14-
// late and make it not send anything. We need to manually create it using
15-
// the internal API.
16-
export const startClient = async function (host: string, port: number): Promise<StatsdClient> {
17-
const client = new StatsdClient({ host, port, socketTimeout: 0 })
13+
export const validateStatsDOptions = function (statsdOpts: InputStatsDOptions): statsdOpts is StatsDOptions {
14+
return !!(statsdOpts && statsdOpts.host && statsdOpts.port)
15+
}
1816

19-
// @ts-expect-error using internals :D
20-
await promisify(client._socket._createSocket.bind(client._socket))()
17+
/**
18+
* Start a new StatsD Client and a new UDP socket
19+
*/
20+
export const startClient = async function (statsdOpts: StatsDOptions): Promise<StatsD> {
21+
const { host, port } = statsdOpts
2122

22-
return client
23+
return new StatsD({
24+
host,
25+
port,
26+
// This caches the dns resolution for subsequent sends of metrics for this instance
27+
// Because we only try to send the metrics on close, this comes only into effect if `bufferFlushInterval` time is exceeded
28+
cacheDns: true,
29+
// set the maxBufferSize to infinite and the bufferFlushInterval very high, so that we only
30+
// send the metrics on close or if more than 10 seconds past by
31+
maxBufferSize: Infinity,
32+
bufferFlushInterval: 10_000,
33+
})
2334
}
2435

25-
// UDP packets are buffered and flushed at regular intervals by statsd-client.
26-
// Closing force flushing all of them.
27-
export const closeClient = async function (client: StatsdClient): Promise<void> {
28-
client.close()
29-
30-
// statsd-client does not provide a way of knowing when the socket is done
31-
// closing, so we need to use the following hack.
32-
await pSetTimeout(CLOSE_TIMEOUT)
33-
await pSetTimeout(CLOSE_TIMEOUT)
36+
/**
37+
* Close the StatsD Client
38+
*
39+
* UDP packets are buffered and flushed every 10 seconds and
40+
* closing the client forces all buffered metrics to be flushed.
41+
*/
42+
export const closeClient = async function (client: StatsD): Promise<void> {
43+
await promisify(client.close.bind(client))()
3444
}
3545

36-
// Make sure the timer name does not include special characters.
37-
// For example, the `packageName` of local plugins includes dots.
46+
/**
47+
* Make sure the timer name does not include special characters.
48+
* For example, the `packageName` of local plugins includes dots.
49+
*/
3850
export const normalizeTagName = function (name: string): string {
3951
return slugify(name, { separator: '_' })
4052
}
53+
54+
/**
55+
* Formats and flattens the tags as array
56+
* We do this because we might send the same tag with different values `{ tag: ['a', 'b'] }`
57+
* which cannot be represented in an flattened object and `hot-shots` also supports tags as array in the format `['tag:a', 'tag:b']`
58+
*/
59+
export const formatTags = function (tags: Record<string, string | string[]>): string[] {
60+
return Object.entries(tags)
61+
.map(([key, value]) => {
62+
if (Array.isArray(value)) {
63+
return value.map((subValue) => `${key}:${subValue}`)
64+
} else {
65+
return `${key}:${value}`
66+
}
67+
})
68+
.flat()
69+
}

packages/build/src/time/report.ts

Lines changed: 42 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,29 +1,58 @@
1-
import { closeClient, startClient } from '../report/statsd.js'
1+
import type { Tags, StatsD } from 'hot-shots'
2+
3+
import {
4+
closeClient,
5+
formatTags,
6+
InputStatsDOptions,
7+
startClient,
8+
StatsDOptions,
9+
validateStatsDOptions,
10+
} from '../report/statsd.js'
211

312
import { addAggregatedTimers } from './aggregate.js'
413
import { roundTimerToMillisecs } from './measure.js'
514

6-
// Record the duration of a build phase, for monitoring.
7-
// Sends to statsd daemon.
8-
export const reportTimers = async function ({ timers, statsdOpts: { host, port }, framework }) {
9-
if (host === undefined) {
15+
interface Timer {
16+
metricName: string
17+
stageTag: string
18+
parentTag: string
19+
durationNs: number
20+
tags: Record<string, string | string[]>
21+
}
22+
23+
/**
24+
* Record the duration of a build phase, for monitoring.
25+
* Sends to statsd daemon.
26+
*/
27+
export const reportTimers = async function (
28+
timers: Timer[],
29+
statsdOpts: InputStatsDOptions,
30+
framework?: string,
31+
): Promise<void> {
32+
if (!validateStatsDOptions(statsdOpts)) {
1033
return
1134
}
1235

13-
const timersA = addAggregatedTimers(timers)
14-
await sendTimers({ timers: timersA, host, port, framework })
36+
await sendTimers(addAggregatedTimers(timers), statsdOpts, framework)
1537
}
1638

17-
const sendTimers = async function ({ timers, host, port, framework }) {
18-
const client = await startClient(host, port)
39+
const sendTimers = async function (timers: Timer[], statsdOpts: StatsDOptions, framework?: string): Promise<void> {
40+
const client = await startClient(statsdOpts)
1941
timers.forEach((timer) => {
20-
sendTimer({ timer, client, framework })
42+
sendTimer(timer, client, framework)
2143
})
2244
await closeClient(client)
2345
}
2446

25-
const sendTimer = function ({ timer: { metricName, stageTag, parentTag, durationNs, tags }, client, framework }) {
47+
const sendTimer = function (timer: Timer, client: StatsD, framework?: string): void {
48+
const { metricName, stageTag, parentTag, durationNs, tags } = timer
2649
const durationMs = roundTimerToMillisecs(durationNs)
27-
const frameworkTag = framework === undefined ? {} : { framework }
28-
client.distribution(metricName, durationMs, { stage: stageTag, parent: parentTag, ...tags, ...frameworkTag })
50+
const statsDTags: Tags = { stage: stageTag, parent: parentTag, ...tags }
51+
52+
// Do not add a framework tag if empty string or null/undefined
53+
if (framework) {
54+
statsDTags.framework = framework
55+
}
56+
57+
client.distribution(metricName, durationMs, formatTags(statsDTags))
2958
}

packages/build/tests/error_reporting/tests.js

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,27 @@
1+
import dns from 'dns'
2+
13
import { intercept, cleanAll } from '@netlify/nock-udp'
24
import { Fixture } from '@netlify/testing'
35
import test from 'ava'
6+
import sinon from 'sinon'
7+
8+
test.before(() => {
9+
const origLookup = dns.lookup
10+
// we have to stub dns lookup as hot-shots is caching dns and therefore calling dns.lookup directly
11+
sinon.stub(dns, 'lookup').callsFake((host, options, cb = options) => {
12+
if (options === cb) {
13+
options = {}
14+
}
15+
if (host.startsWith(`errorreportingtest.`)) {
16+
cb(undefined, host, 4)
17+
} else {
18+
origLookup(host, options, cb)
19+
}
20+
})
21+
})
422

523
test.after(() => {
24+
dns.lookup.restore()
625
cleanAll()
726
})
827

@@ -31,7 +50,7 @@ const getTrackingRequestsString = async function (t, fixtureName, used = true) {
3150

3251
const getAllTrackingRequests = async function (t, fixtureName, used) {
3352
// Ensure there's no conflict between each test scope
34-
const host = encodeURI(t.title)
53+
const host = `errorreportingtest.${encodeURI(t.title)}`
3554
const port = '1234'
3655
const scope = intercept(`${host}:${port}`, { persist: true, allowUnknown: true })
3756

packages/build/tests/time/tests.js

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,27 @@
1+
import dns from 'dns'
2+
13
import { intercept, cleanAll } from '@netlify/nock-udp'
24
import { Fixture } from '@netlify/testing'
35
import test from 'ava'
6+
import sinon from 'sinon'
7+
8+
test.before(() => {
9+
const origLookup = dns.lookup
10+
// we have to stub dns lookup as hot-shots is caching dns and therefore calling dns.lookup directly
11+
sinon.stub(dns, 'lookup').callsFake((host, options, cb = options) => {
12+
if (options === cb) {
13+
options = {}
14+
}
15+
if (host.startsWith(`timetest.`)) {
16+
cb(undefined, host, 4)
17+
} else {
18+
origLookup(host, options, cb)
19+
}
20+
})
21+
})
422

523
test.after(() => {
24+
dns.lookup.restore()
625
cleanAll()
726
})
827

@@ -70,7 +89,7 @@ const getTimerRequestsString = async function (t, fixtureName, flags) {
7089

7190
const getAllTimerRequests = async function (t, fixtureName, flags = {}) {
7291
// Ensure there's no conflict between each test scope
73-
const host = encodeURI(t.title)
92+
const host = `timetest.${encodeURI(t.title)}`
7493
const port = '1234'
7594
const scope = intercept(`${host}:${port}`, { persist: true, allowUnknown: true })
7695

0 commit comments

Comments
 (0)