Skip to content
Open
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
9 changes: 1 addition & 8 deletions core/cli/src/install.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import { findConflicts, withoutConflicts } from '@dotcom-tool-kit/conflict'
import { formatUninstalledHooks } from './messages'
import { importEntryPoint } from './plugin/entry-point'
import { runInit } from './init'
import { guessSystemCode } from './systemCode'
import { enableTelemetry } from './telemetry'
import { TelemetryRecorder } from '@dotcom-tool-kit/telemetry'

Expand Down Expand Up @@ -131,14 +130,8 @@ export default async function installHooks(logger: Logger, metrics: TelemetryRec

await runInit(logger, config)

const systemCode = await guessSystemCode(config)
let scoped = metrics
if (systemCode) {
scoped = metrics.scoped({ systemCode })
}

const errors: Error[] = []
const hooks = (await loadHookInstallations(logger, scoped, config)).unwrap(
const hooks = (await loadHookInstallations(logger, metrics, config)).unwrap(
'hooks were found to be invalid when installing'
)

Expand Down
17 changes: 8 additions & 9 deletions core/cli/src/tasks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,11 @@ export async function runTasks(
) {
const errors: ErrorSummary[] = []

// TODO:CC:20260205: Remove the default dotcom-tool-kit once the systemCode is a required option
// (see TODO in root schema) but at the moment we can't have it undefined
// because it is required by the client-metrics-server
const systemCode = (await guessSystemCode(config)) || 'dotcom-tool-kit'

if (tasks.length === 0) {
logger.warn(`no task configured for ${command}: skipping assignment...`)
}
Expand All @@ -98,7 +103,7 @@ export async function runTasks(
try {
logger.info(styles.taskHeader(`running ${styles.task(task.id)} task`))
await task.run({ files, command, cwd: config.root, config })
scoped.recordEvent('tasks.completed', { success: true })
scoped.recordEvent('tasks.completed', systemCode, { success: true })
} catch (error) {
// if there's an exit code, that's a request from the task to exit early
if (error instanceof ToolKitError && error.exitCode) {
Expand All @@ -111,7 +116,7 @@ export async function runTasks(
task: task.id,
error: error as Error
})
scoped.recordEvent('tasks.completed', { success: false })
scoped.recordEvent('tasks.completed', systemCode, { success: false })
}
}

Expand Down Expand Up @@ -171,11 +176,5 @@ export async function runCommands(
const config = await loadConfig(logger, { root: process.cwd() })
enableTelemetry(metrics, config.pluginOptions['app root'].options as RootOptions)

const systemCode = await guessSystemCode(config)
let scoped = metrics
if (systemCode) {
scoped = metrics.scoped({ systemCode })
}

return runCommandsFromConfig(logger, config, commands, files, scoped)
return runCommandsFromConfig(logger, config, commands, files, metrics)
}
5 changes: 3 additions & 2 deletions lib/telemetry/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -147,9 +147,10 @@ export class TelemetryRecorder {
* per-namespace as our backend server validates that the events are
* structured properly for both correctness and security purposes.
*/
recordEvent<N extends Namespace>(namespace: N, details: NamespaceSchemas[N]) {
recordEvent<N extends Namespace>(namespace: N, systemCode: string, details: NamespaceSchemas[N]) {
const event: TelemetryEvent = {
namespace: `dotcom-tool-kit.${namespace}`,
systemCode,
eventTimestamp: Date.now(),
data: { ...this.attributes, ...details }
Comment on lines +150 to 155
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: I think users that call the recordEvent function shouldn't have to concern themselves with metadata explicitly, and the systemCode in particular is something that should be passed around in the scoped metadata rather than calling guessSystemCode with the same fallback every time we want to record an event. I think it makes more sense to move the fallback logic here and get the systemCode from the scoped attributes?

it will also resolve the breaking change Mx LLM is talking about which is a little bit of a concern even though no one has enabled the telemetry the function arguments are still processed and will cause a TypeError if you update the telemetry package but are still using another Tool Kit package that calls the old recordEvent function.

Suggested change
recordEvent<N extends Namespace>(namespace: N, systemCode: string, details: NamespaceSchemas[N]) {
const event: TelemetryEvent = {
namespace: `dotcom-tool-kit.${namespace}`,
systemCode,
eventTimestamp: Date.now(),
data: { ...this.attributes, ...details }
recordEvent<N extends Namespace>(namespace: N, details: NamespaceSchemas[N]) {
const data = { ...this.attributes, ...details }
const event: TelemetryEvent = {
namespace: `dotcom-tool-kit.${namespace}`,
systemCode: data.systemCode || 'dotcom-tool-kit',
eventTimestamp: Date.now(),
data

}
Expand Down Expand Up @@ -177,7 +178,7 @@ export class MockTelemetryClient extends TelemetryRecorder {
return this
}
// eslint-disable-next-line @typescript-eslint/no-empty-function -- mocked function
override recordEvent(_namespace: string, _details: NamespaceSchemas[Namespace]) {}
override recordEvent(_namespace: string, _systemCode: string, _details: NamespaceSchemas[Namespace]) {}
}

export { TelemetryEvent }
1 change: 1 addition & 0 deletions lib/telemetry/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,5 +8,6 @@ export type TelemetryAttributes = Record<string, string>
export interface TelemetryEvent<N extends Namespace = Namespace> {
eventTimestamp: number
namespace: `dotcom-tool-kit.${N}`
systemCode: string
data: TelemetryAttributes & NamespaceSchemas[N]
}
53 changes: 33 additions & 20 deletions lib/telemetry/test/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ describe('attribute handling', () => {

test('event attribute included', () => {
const recorder = new TelemetryRecorder(mockProcessor, { foo: 'bar' })
recorder.recordEvent('tasks.completed', { success: true })
recorder.recordEvent('tasks.completed', 'mock-system-code', { success: true })
expect(metricsMock).toHaveBeenCalledWith(
expect.objectContaining({ data: expect.objectContaining({ foo: 'bar' }) })
)
Expand All @@ -67,7 +67,7 @@ describe('attribute handling', () => {
test('parent attributes inherited', () => {
const recorder = new TelemetryRecorder(mockProcessor, { foo: 'bar' })
const child = recorder.scoped({ baz: 'qux' })
child.recordEvent('tasks.completed', { success: true })
child.recordEvent('tasks.completed', 'mock-system-code', { success: true })
expect(metricsMock).toHaveBeenCalledWith(
expect.objectContaining({ data: expect.objectContaining({ foo: 'bar', baz: 'qux' }) })
)
Expand All @@ -76,7 +76,7 @@ describe('attribute handling', () => {
test('grandparent attributes inherited', () => {
const recorder = new TelemetryRecorder(mockProcessor, { foo: 'bar' })
const grandchild = recorder.scoped({ baz: 'qux' }).scoped({ test: 'pass' })
grandchild.recordEvent('tasks.completed', { success: true })
grandchild.recordEvent('tasks.completed', 'mock-system-code', { success: true })
expect(metricsMock).toHaveBeenCalledWith(
expect.objectContaining({ data: expect.objectContaining({ foo: 'bar', baz: 'qux', test: 'pass' }) })
)
Expand All @@ -85,15 +85,15 @@ describe('attribute handling', () => {
test('parent attributes overridable', () => {
const recorder = new TelemetryRecorder(mockProcessor, { foo: 'bar' })
const child = recorder.scoped({ foo: 'baz' })
child.recordEvent('tasks.completed', { success: true })
child.recordEvent('tasks.completed', 'mock-system-code', { success: true })
expect(metricsMock).toHaveBeenCalledWith(
expect.objectContaining({ data: expect.objectContaining({ foo: 'baz' }) })
)
})

test("can't override event metadata", () => {
const recorder = new TelemetryRecorder(mockProcessor, { namespace: 'foo', eventTimestamp: '137' })
recorder.recordEvent('tasks.completed', { success: true })
recorder.recordEvent('tasks.completed', 'mock-system-code', { success: true })
expect(metricsMock).toHaveBeenCalledWith(
expect.objectContaining({ namespace: 'dotcom-tool-kit.tasks.completed' })
)
Expand Down Expand Up @@ -142,21 +142,21 @@ describe('conditionally enabled', () => {

test('no metrics are sent by default', () => {
const telemetryProcess = new TelemetryProcess(logger)
telemetryProcess.root().recordEvent('tasks.completed', { success: true })
telemetryProcess.root().recordEvent('tasks.completed', 'mock-system-code', { success: true })
expect(metricsMock).not.toHaveBeenCalled()
})

test('metrics are sent when enabled', () => {
const telemetryProcess = new TelemetryProcess(logger, true)
telemetryProcess.root().recordEvent('tasks.completed', { success: true })
telemetryProcess.root().recordEvent('tasks.completed', 'mock-system-code', { success: true })
expect(metricsMock).toHaveBeenCalled()
})

test('recorded metrics are back-sent once telemetry is enabled', () => {
const telemetryProcess = new TelemetryProcess(logger, false)
telemetryProcess.root().recordEvent('tasks.completed', { success: true })
telemetryProcess.root().recordEvent('tasks.completed', { success: true })
telemetryProcess.root().recordEvent('tasks.completed', { success: true })
telemetryProcess.root().recordEvent('tasks.completed', 'mock-system-code', { success: true })
telemetryProcess.root().recordEvent('tasks.completed', 'mock-system-code', { success: true })
telemetryProcess.root().recordEvent('tasks.completed', 'mock-system-code', { success: true })
telemetryProcess.enable()
expect(metricsMock).toHaveBeenCalledTimes(3)
})
Expand All @@ -177,38 +177,51 @@ describe('metrics sent', () => {

test('a metric is sent successfully', async () => {
const listeningPromise = listenForTelemetry(mockServer, 1)
telemetryProcess.root().recordEvent('tasks.completed', { success: true })
telemetryProcess.root().recordEvent('tasks.completed', 'mock-system-code', { success: true })
const metrics = await listeningPromise
expect(metrics).toEqual([[expect.objectContaining({ namespace: 'dotcom-tool-kit.tasks.completed' })]])
expect(metrics).toEqual([
[
expect.objectContaining({
namespace: 'dotcom-tool-kit.tasks.completed',
systemCode: 'mock-system-code'
})
]
])
})

// TODO:IM:20260107 enable this test once we have multiple different metric types
test.skip('metrics of different types are sent successfully', async () => {
const listeningPromise = listenForTelemetry(mockServer, 2)
const recorder = telemetryProcess.root()
recorder.recordEvent('tasks.completed', { success: true })
recorder.recordEvent('tasks.completed', { success: true })
recorder.recordEvent('tasks.completed', 'mock-system-code', { success: true })
recorder.recordEvent('tasks.completed', 'mock-system-code', { success: true })
const metrics = await listeningPromise
expect(metrics.flat()).toEqual([
expect.objectContaining({ namespace: 'dotcom-tool-kit.tasks.completed' }),
expect.objectContaining({ namespace: 'dotcom-tool-kit.tasks.completed' })
expect.objectContaining({
namespace: 'dotcom-tool-kit.tasks.completed',
systemCode: 'mock-system-code'
}),
expect.objectContaining({
namespace: 'dotcom-tool-kit.tasks.completed',
systemCode: 'mock-system-code'
})
])
})

test('buffers multiple metrics sent together', async () => {
const listeningPromise = listenForTelemetry(mockServer, 3, 10)
const recorder = telemetryProcess.root()
recorder.recordEvent('tasks.completed', { success: true })
recorder.recordEvent('tasks.completed', { success: true })
recorder.recordEvent('tasks.completed', { success: true })
recorder.recordEvent('tasks.completed', 'mock-system-code', { success: true })
recorder.recordEvent('tasks.completed', 'mock-system-code', { success: true })
recorder.recordEvent('tasks.completed', 'mock-system-code', { success: true })
const metrics = await listeningPromise
expect(metrics[1]).toHaveLength(2)
})

test('uses timestamp from when recorded, not sent', async () => {
jest.useFakeTimers({ now: 0 })
const listeningPromise = listenForTelemetry(mockServer, 1)
telemetryProcess.root().recordEvent('tasks.completed', { success: true })
telemetryProcess.root().recordEvent('tasks.completed', 'mock-system-code', { success: true })
jest.setSystemTime(20)
const metrics = await listeningPromise
expect(metrics[0][0].eventTimestamp).toBe(0)
Expand Down
2 changes: 1 addition & 1 deletion lib/telemetry/test/metricsProcess.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ const metrics = telemetryProcess.root()
process.on('message', ({ action }) => {
switch (action) {
case 'send':
metrics.recordEvent('tasks.completed', { success: true })
metrics.recordEvent('tasks.completed', 'mock-system-code', { success: true })
break
case 'disconnect':
// unreference everything so that this process's event loop can exit.
Expand Down